[bug] Make SDDL tokenizer parenthesis-aware and surface errors on malformed input (#80)#89
Merged
Conversation
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 #80Motivation
CutSDDLrecognised theO:/G:/D:/S:component markers anywhere in the string and silently discarded any characters before the first marker;CutAcestracked parenthesis depth with no lower bound, so unbalanced parentheses silently dropped or desynchronised ACEs. Neither returned an error. Malformed SDDL was therefore accepted and silently truncated, and an ACE body containing:or nested parentheses (conditional / resource-attribute ACEs) could be split incorrectly. A parser for a security-sensitive format should reject malformed input rather than emit a partial, misleading parse.What Changed
CutSDDLnow tracks parenthesis depth and only recognises component markers at depth 0, so a marker-like substring inside an ACE body no longer starts a new component. It returns anerrorfor: a character before the first marker, an unbalanced)(negative depth), or an unbalanced((non-zero depth at end). Signature is now(string, string, []string, []string, error).CutAcesnow preserves nested parentheses (only top-level parens delimit ACEs) and returns anerrorfor an unbalanced), an unbalanced(, or stray characters between/after top-level ACEs. Signature is now([]string, error). A component with no((empty DACL/SACL with only flags) still returns no ACEs and no error.CutAcescalls inCutSDDLnow propagate these errors, wrapped asinvalid DACL/invalid SACL.Design Notes
Marker detection is gated on
depth == 0, which is what makes conditional/resource-attribute ACEs tokenize correctly. The two functions validate parenthesis balance independently:CutSDDLchecks the whole string, and per-componentCutAcescatches a component-local imbalance that a compensating imbalance elsewhere would otherwise mask.Acceptance Criteria Check
TestSddlCut_MalformedReturnsError(unbalanced open paren,extra close paren).TestSddlCut_MalformedReturnsError(leading garbage,trailing garbage).:or nested parens is tokenized as a single ACE —TestSddlCut_ConditionalAceSingleToken(inputD:(XA;;FA;;;WD;(@xD:1))yields one DACL ACE and no SACL).TestSddlCutandTestSddlCut_LowercaseMarkersupdated for the new signature and green.How Verified
sddl/sddl_test.go— addedTestSddlCut_ConditionalAceSingleToken,TestSddlCut_MalformedReturnsError; updatedTestSddlCut/TestSddlCut_LowercaseMarkersfor the new error return.go build ./...andgo test ./...pass.Test Coverage
Scope of Change
sddl/sddl.go,sddl/sddl_test.goCutSDDLandCutAcesgain anerrorreturn value. Both are exported. There are no in-repo callers outside thesddlpackage (the descriptor SDDL parser does not use them), but external importers calling these functions directly must add error handling. Please confirm the signature change is acceptable before merging.Risk and Rollout
The behavioral change for well-formed SDDL is limited to the new (ignored) error return; tokenization of all existing valid cases is unchanged (verified by the retained tests). The risk is the breaking signature change for external callers — flagged above for your decision.