Fix ACI grouped bind rule wrongly rejected when a value contains parentheses#716
Open
vharseko wants to merge 2 commits into
Open
Fix ACI grouped bind rule wrongly rejected when a value contains parentheses#716vharseko wants to merge 2 commits into
vharseko wants to merge 2 commits into
Conversation
…ntheses
BindRule.decode located the matching close parenthesis by counting '(' and ')'
across the whole string, including characters inside a quoted expression. A
grouped bind rule whose value contained a literal parenthesis - for example
(userdn="ldap:///cn=a)b,dc=x" or userdn="ldap:///self") - was split at the
wrong parenthesis and rejected with AciException. Track quote state so
parentheses inside a quoted expression are treated as literal data.
New BindRuleParenTest covers simple and grouped rules with '(' / ')' / '()' in
the value. The full AciTests suite (515 tests) passes except the two cases
already fixed separately by PR OpenIdentityPlatform#675.
Contributor
|
I suggest to add a negative test for malformed input (unbalanced parens without quotes) to confirm it still throws. |
maximthomas
approved these changes
Jul 7, 2026
Confirm BindRule.decode still rejects genuinely malformed grouped bind rules after the quote-aware scan: a missing close parenthesis, an extra unmatched open parenthesis, and a group whose only ')' is embedded in a quoted value (which must not be mistaken for the group's close). All three throw AciException, guarding against a fail-open regression.
maximthomas
approved these changes
Jul 7, 2026
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.
Bug
A grouped ACI bind rule whose expression value contains a literal parenthesis is wrongly rejected with an
AciException, even though the value is legal. For example:decoding fails with "bind rule value ... is invalid", and the
(...variant fails with "missing close parenthesis".Root cause
BindRule.decodefinds the parenthesis that closes a grouped (complex) bind rule by scanning the whole string and counting(and):The scan does not distinguish structural grouping parentheses from parentheses that appear inside a quoted expression (a DN value such as
cn=a)b). The embedded)is counted, so the group is split at the wrong parenthesis and the truncated fragment fails to decode. This affects grouped bind rules only — a simple (ungrouped) bind rule is matched by the"([^"]+)"expression regex and was already handled correctly.The behaviour was fail-closed (a valid ACI rejected, never an invalid one accepted), so this is a correctness fix, not a security fix. It resolves the long-standing
TODOabovedecode()about escaped/embedded parentheses.Fix
Track whether the scan is inside a quoted expression and count
(/)only when outside quotes, so parentheses embedded in a value are treated as literal data. The extracted substring is still fully re-decoded and validated recursively, so there is no fail-open path.Tests
BindRuleParenTest— simple and grouped bind rules with(,)and()embedded in the DN value all decode successfully.AciTestssuite run locally with theslowgroup enabled (mvn -P precommit -pl opendj-server-legacy verify -Dit.test=AciTests): 515 tests, 2 failures — and those two (testValidAcis,testCompare) are the pre-existing failures already fixed by Fix two broken AciTests cases and enable the suite in the default build #675, unrelated to this change. No other regressions.