Repository: scimmyjs/scimmy (js)

Issue:
The SCIMMY filter does not support special character in the attribute name
While integrate with the Entra provisioning service, we found that the remove group member with the Entra sample request would trigger invalid filter exception.

The request body is as shown in this link: https://learn.microsoft.com/en-us/entra/identity/app-provisioning/use-scim-to-provision-users-and-groups#update-group-remove-members

The exception is as:
```json
{
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:Error"
    ],
    "status": "400",
    "scimType": "invalidFilter",
    "detail": "Unexpected token '$ref eq null and value eq f648f8d5ea4e4cd38e9c' in filter for 'remove' op of operation 1 in PatchOp request body"
}
```

The root cause seems to be the '$ref' as the attribute name of the filter.


Pull request:
Allow leading special characters for attribute names in filter strings
Currently, attribute naming rules are different between the `SCIMMY.Types.Attribute` class and the `SCIMMY.Types.Filter` class. For attributes, their name is allowed to begin with the `$`, `-`, and `_` special characters. For filters, attribute names can only begin with the `_` special character. This inconsistency causes filter expression parsing to fail when an attribute name begins with the `$` or `-` special characters, as allowed in the attribute class. A specific example of this is when attempting to filter against the commonly used `$ref` attribute.

The tokenising method of Filter class has had its naming rules relaxed, now allowing for any leading special character in attribute names (fixes #33). This has been achieved by adding explicit logical operator and comparator detection, leaving only the detection of attribute names and potential unescaped string values in "word" tokens. In the Attribute class, a new check of attribute names in the class constructor has been implemented for invalid leading characters, which currently will not match, but is in place for the possible future addition of stricter, spec-compliant naming rules.

The test fixtures for the Attribute and Filter classes have also been updated to check that leading special characters are allowed, and correctly parsed.

Patch:
diff --git a/src/lib/types/attribute.js b/src/lib/types/attribute.js
index 0da5259..309d500 100644
--- a/src/lib/types/attribute.js
+++ b/src/lib/types/attribute.js
@@ -308,7 +308,7 @@ export class Attribute {
     constructor(type, name, config = {}, subAttributes = []) {
         const errorSuffix = `attribute definition '${name}'`;
         // Check for invalid characters in attribute name
-        const [, invalidNameChar] = /^(?:.*?)([^$\-_a-zA-Z0-9])(?:.*?)$/g.exec(name) ?? [];
+        const [, invalidNameChar, invalidNameStart] = /([^-$\w])|(^[^-$\w])/g.exec(name) ?? [];
         
         // Make sure name and type are supplied as strings
         for (let [param, value] of [["type", type], ["name", name]]) if (typeof value !== "string")
@@ -316,7 +316,10 @@ export class Attribute {
         // Make sure type is valid
         if (!CharacteristicValidity.types.includes(type))
             throw new TypeError(`Type '${type}' not recognised in ${errorSuffix}`);
-        // Make sure name is valid
+        // Make sure first character in name is valid
+        if (!!invalidNameStart)
+            throw new TypeError(`Invalid leading character '${invalidNameStart}' in name of ${errorSuffix}`);
+        // Make sure rest of name is valid
         if (!!invalidNameChar)
             throw new TypeError(`Invalid character '${invalidNameChar}' in name of ${errorSuffix}`);
         // Make sure attribute type is 'complex' if subAttributes are defined
diff --git a/src/lib/types/filter.js b/src/lib/types/filter.js
index b8420f5..57ff8f4 100644
--- a/src/lib/types/filter.js
+++ b/src/lib/types/filter.js
@@ -22,8 +22,24 @@ const operators = ["and", "or", "not"];
  * @default
  */
 const comparators = ["eq", "ne", "co", "sw", "ew", "gt", "lt", "ge", "le", "pr", "np"];
+
+// Regular expressions that represent filter syntax
+const lexicon = [
+    // White Space, Number Values
+    /(\s+)/, /([-+]?\d+(?:\.\d+)?(?:[eE][-+]?\d+)?)/,
+    // Boolean Values, Empty Values, String Values
+    /(false|true)+/, /(null)+/, /("(?:[^"]|\\.|\n)*")/,
+    // Logical Groups, Complex Attribute Value Filters
+    /(\((?:.*?)\))/, /(\[(?:.*?)][.]?)/,
+    // Logical Operators and Comparators
+    new RegExp(`(${operators.join("|")})(?=[^a-zA-Z0-9]|$)`),
+    new RegExp(`(${comparators.join("|")})(?=[^a-zA-Z0-9]|$)`),
+    // All other "words"
+    /([-$\w][-$\w._:\/%]*)/
+];
+
 // Parsing Pattern Matcher
-const patterns = /^(?:(\s+)|(-?\d+(?:\.\d+)?(?:[eE][-+]?\d+)?)|(false|true)+|(null)+|("(?:[^"]|\\.|\n)*")|(\((?:.*?)\))|(\[(?:.*?)][.]?)|(\w[-\w._:\/%]*))/;
+const patterns = new RegExp(`^(?:${lexicon.map(({source}) => source).join("|")})`, "i");
 // Split a path by fullstops when they aren't in a filter group or decimal
 const pathSeparator = /(?<![^\w]\d)\.(?!\d[^\w]|[^[]*])/g;
 // Extract attributes and filter strings from path parts
@@ -477,7 +493,7 @@ export class Filter extends Array {
         // Cycle through the query and tokenise it until it can't be tokenised anymore
         while (token = patterns.exec(query)) {
             // Extract the different matches from the token
-            const [literal, space, number, boolean, empty, string, grouping, attribute, maybeWord] = token;
+            const [literal, space, number, boolean, empty, string, grouping, complex, operator, comparator, maybeWord] = token;
             let word = maybeWord;
             
             // If the token isn't whitespace, handle it!
@@ -488,35 +504,40 @@ export class Filter extends Array {
                 if (boolean !== undefined) tokens.push({type: "Boolean", value: boolean === "true"});
                 if (empty !== undefined) tokens.push({type: "Empty", value: "null"});
                 
+                // Handle logical operators and comparators
+                if (operator !== undefined) tokens.push({type: "Operator", value: operator});
+                if (comparator !== undefined) tokens.push({type: "Comparator", value: comparator});
+                
                 // Handle grouped filters
                 if (grouping !== undefined) tokens.push({type: "Group", value: grouping.substring(1, grouping.length - 1)});
                 
-                // Handle attribute filters inline
-                if (attribute !== undefined) word = tokens.pop().value + attribute;
+                // Treat complex attribute filters as words
+                if (complex !== undefined) word = tokens.pop().value + complex;
                 
-                // Handle operators, comparators, and attribute names
+                // Handle attribute names (words), and unescaped string values
                 if (word !== undefined) {
-                    // Compound words when last token was a word ending with "."
-                    if (tokens.length && tokens[tokens.length-1].type === "Word" && tokens[tokens.length-1].value.endsWith("."))
-                        word = tokens.pop().value + word;
-                    
-                    // Derive the token's type by matching against known operators and comparators
-                    let type = (operators.includes(word.toLowerCase()) ? "Operator" : (comparators.includes(word.toLowerCase()) ? "Comparator" : "Word"));
+                    // Start by assuming the token actually is a word
+                    let current = {type: "Word", value: word};
                     
                     // If there was a previous token, make sure it was accurate
                     if (tokens.length) {
                         const previous = tokens[tokens.length-1];
                         
-                        // If the previous token was also a comparator, it may have actually been a word
-                        if (previous.type === "Comparator" && type === "Comparator") previous.type = "Word";
-                        // If the previous token was also an operator...
-                        if (previous.type === "Operator" && type === "Operator"
-                            // ...and that operator was "not", or this operator is NOT "not", it may have been a word    
-                            && (previous.value.toLowerCase() === "not" || word.toLowerCase() !== "not")) type = "Word";
+                        // Compound words when last token was a word ending with "."
+                        if (previous.type === "Word" && previous.value.endsWith("."))
+                            current.value = tokens.pop().value + word;
+                        // If the previous token was a comparator...
+                        else if (previous.type === "Comparator")
+                            // ...this one is almost certainly an unescaped string
+                            current = {type: "Value", value: `"${String(word)}"`};
+                        // If a word does not follow a logical operator...
+                        else if (previous.type !== "Operator")
+                            // It is invalid, so skip all further traversal
+                            break;
                     }
                     
-                    // Store the token
-                    tokens.push({type, value: word});
+                    // If all looks good, store the token
+                    tokens.push(current);
                 }
             }
             


Test patch:
diff --git a/test/lib/types/attribute.js b/test/lib/types/attribute.js
index 91803a8..c219cc6 100644
--- a/test/lib/types/attribute.js
+++ b/test/lib/types/attribute.js
@@ -70,18 +70,37 @@ describe("SCIMMY.Types.Attribute", () => {
             const invalidNames = [
                 [".", "invalid.name"],
                 ["@", "invalid@name"],
-                ["=", "invalid=name"], 
-                ["%", "invalid%name"]
+                ["=", "invalid=name"],
+                [",", "invalid,name"],
+                ["%", "invalid%name"],
+                ["%", "%invalidName"]
             ];
             
             for (let [char, name] of invalidNames) {
                 assert.throws(() => new Attribute("string", name),
                     {name: "TypeError", message: `Invalid character '${char}' in name of attribute definition '${name}'`},
-                    "Attribute instantiated with invalid 'name' argument");
+                    `Attribute instantiated with invalid 'name' argument '${name}'`);
             }
             
-            assert.ok(new Attribute("string", "validName"),
-                "Attribute did not instantiate with valid 'name' argument");
+            const validNames = [
+                "validName",
+                "$validName",
+                "valid$name",
+                "_validName",
+                "valid_name",
+                "-validName",
+                "valid-name",
+                "00validName",
+                "valid00name"
+            ];
+            
+            for (let name of validNames) {
+                try {
+                    new Attribute("string", name);
+                } catch (ex) {
+                    assert.fail(`Attribute did not instantiate with valid 'name' argument '${name}'\r\n[cause]: ${ex}`);
+                }
+            }
         });
         
         it("should not accept 'subAttributes' argument if type is not 'complex'", () => {
diff --git a/test/lib/types/filter.json b/test/lib/types/filter.json
index eef7c89..06837b1 100644
--- a/test/lib/types/filter.json
+++ b/test/lib/types/filter.json
@@ -8,12 +8,16 @@
       {"source": "name.formatted sw \"Bob\"", "target": [{"name": {"formatted": ["sw", "Bob"]}}]},
       {"source": "quota gt 1.5", "target": [{"quota": ["gt", 1.5]}]},
       {"source": "UserType eq null", "target": [{"userType": ["eq", null]}]},
+      {"source": "$ref eq null", "target": [{"$ref": ["eq", null]}]},
+      {"source": "valid$Name eq null", "target": [{"valid$Name": ["eq", null]}]},
+      {"source": "-valid$Name eq -null", "target": [{"-valid$Name": ["eq", "-null"]}]},
       {"source": "active eq false", "target": [{"active": ["eq", false]}]},
       {"source": "emails.primary eq true", "target": [{"emails": {"primary": ["eq", true]}}]}
     ],
     "logical": [
       {"source": "not eq pr", "target": [{"eq": ["not", "pr"]}]},
       {"source": "NOT id pr", "target": [{"id": ["not", "pr"]}]},
+      {"source": "$ref eq null and value eq \"f648f8d5ea4e4cd38e9c\"", "target": [{"$ref": ["eq", null], "value": ["eq", "f648f8d5ea4e4cd38e9c"]}]},
       {"source": "id pr and userName eq \"Test\"", "target": [{"id": ["pr"], "userName": ["eq", "Test"]}]},
       {"source": "userName eq \"Test\" or displayName co \"Bob\"", "target": [{"userName": ["eq", "Test"]}, {"displayName": ["co", "Bob"]}]},
       {"source": "userName eq \"Test\" or displayName co \"Bob\" and quota gt 5", "target": [{"userName": ["eq", "Test"]}, {"displayName": ["co", "Bob"], "quota": ["gt", 5]}]},