[bug] Fix case-sensitive matching of SDDL keyword operators in conditional expressions (Fixes #131) - #132
Merged
Conversation
…expressions (Fixes #131) Keyword operators were resolved by indexing wordOperators with the raw lexed token text, and by a literal switch on that text for the infix operators. The lexer preserves the case of a word token, correctly, because attribute names are case-preserving, so only one exact capitalisation of each operator parsed. That rejected the spelling Windows itself uses: the operator-name tables in sechost.dll and advapi32.dll spell the four "any" operators with a lowercase any (Member_of_any), whereas MS-DTYP 2.4.4.17.6 and wordOperators spell them Member_of_Any. Because an unrecognised word falls through to being parsed as an attribute name, the resulting error did not even name the operator. Resolve keyword operators through a new lookupWordOperator helper backed by a case-folded copy of wordOperators, at both call sites. The infix site keeps its restriction to the four binary keyword operators via a new isRelationalWordOperator predicate, so the unary Member_of family and Exists/Not_Exists are still rejected between a left- and right-hand side. Serialization is untouched and continues to emit the MS-DTYP capitalisation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked Issue
Closes #131Root Cause
Keyword operators were resolved by indexing
wordOperatorswith the raw lexed token text (parser.go:220), and by a literalswitchon that same text for the infix operators (parser.go:285–296). The lexer preserves the original case of atkWordtoken — correctly, since attribute names are case-preserving — so each operator parsed under exactly one capitalisation, the one hardcoded in the map and the switch.That is not merely a strictness issue, because two capitalisations of the same four tokens are in circulation. MS-DTYP 2.4.4.17.6 spells them
Member_of_Any, andwordOperatorsfollowed the specification. The operator-name tables in Windows' ownsechost.dllandadvapi32.dllspell themMember_of_any, with a lowercaseany. The parser therefore rejected conditions carrying the spelling Windows itself produces. The failure was also badly reported: an unrecognised word falls through to being parsed as an attribute name, so the error surfaced asexpected ')' to close parenthesized expressionand never named the operator.Fix Description
Keyword-operator lookup now goes through
lookupWordOperator, which matches againstwordOperatorsFolded— a lowercase-keyed copy ofwordOperatorsbuilt once at package initialisation. Both call sites use it.wordOperatorsis kept as the canonical, specification-cased table and remains the single source of truth; the folded map is derived from it, so adding an operator in one place keeps both directions consistent. Lowercasing the lookup key was chosen over lowercasing in the lexer because the lexer's output doubles as attribute names, which must retain their case.The infix site needs to admit only the operators that take a left- and a right-hand side. Replacing its
switchwith the shared lookup would have wrongly accepted the unaryMember_offamily andExists/Not_Existsin infix position, so the lookup is gated on a newisRelationalWordOperatorpredicate, mirroring the existingisMemberOfOperatorhelper. Behaviour for unary operators in infix position is unchanged, and there is a test pinning that.Serialization is untouched:
operatorNamesis keyed by token byte and keeps emitting the MS-DTYP capitalisation, so output remains stable and specification-conformant regardless of the case that was parsed.How Verified
Runtime, before the fix —
ace/condition.Parseon each casing:Runtime, after the fix — all of the above parse, and each variant marshals to bytes identical to the canonical spelling.
Regression check — with the new tests in place but
condition.go/parser.goreverted to their pre-fix state,go test ./ace/condition/...reports 27 failures. With the fix applied the package passes, andgo test ./...is green across the repository.gofmt -l ace/is clean andgo vet ./ace/...reports nothing.Test Coverage
Added — in
ace/condition/condition_test.go:TestKeywordOperatorsAreCaseInsensitive— every keyword operator across mixed, lower and upper casings, asserting each variant marshals to the same bytes as the canonical spelling. Covers both parse paths and both spellings of the fouranyoperators.TestSerializeUsesSpecCapitalization— parsesMember_of_anyand asserts the re-serialized text containsMember_of_Any, pinning that the fix did not leak into the output form.TestUnaryKeywordOperatorsRejectedInInfixPosition— assertsMember_ofandExistsare still rejected between a left- and right-hand side, in both casings, guarding theisRelationalWordOperatorgate.Scope of Change
ace/condition/condition.go,ace/condition/parser.go,ace/condition/condition_test.go(member_of == 1), say — is now an error where it previously parsed as an attribute. The canonical casing was already reserved this way before the change, so this makes the existing behaviour consistent across casings rather than introducing a new rule.Risk and Rollout
Confined to the conditional-expression parser; the encoder, decoder and serializer are untouched. The change strictly widens the set of accepted inputs, apart from the keyword-shadowing consequence noted above. Safe to merge without staged rollout.
Notes
The
Member_of_anyspelling was found by recovering the 24-entry operator table fromsechost.dll(.rdataRVA0x070300) andadvapi32.dll(RVA0x06f1e0) on Windows Server 2025; the two tables are byte-identical and both use the lowercaseanyform.Two further divergences between those tables and MS-DTYP were observed and are deliberately not addressed here, to keep this change focused. Each warrants its own issue:
0xa3, named&with precedence 10, is present in both Windows tables but is not defined anywhere in MS-DTYP. This package has no constant for it, so such an ACE cannot round-trip.@TOKEN.appears as a fourth attribute prefix alongside the documented@USER.,@DEVICE.and@RESOURCE., and is currently rejected as an unknown prefix.