From 93bcfd09c1d407bbd125b909193cf92786e98380 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 9 Jul 2026 10:01:41 +0300 Subject: [PATCH 1/2] Fix NullPointerException decoding an ACI bind rule with a missing and/or operand (#719) An empty group such as "()" decodes to a null bind rule. BindRule.decode dereferenced that null when it appeared as the left or right operand of an "and"/"or" bind rule (e.g. "(()or)"), throwing a NullPointerException instead of an AciException. Reject the missing operand - and a bare "()" group - with an AciException. --- .../authorization/dseecompat/BindRule.java | 26 ++++++ .../dseecompat/BindRuleOperandTest.java | 85 +++++++++++++++++++ 2 files changed, 111 insertions(+) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleOperandTest.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java index 288a28f590..5c94c730ae 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java @@ -212,6 +212,14 @@ else if (!inQuotes && currentChar == ')') bindruleStr.substring(currentPos + 1); return createBindRule(bindrule_1, remainingBindruleStr); } + /* + * An empty group such as "()" decodes to a null bind rule. A group + * with no bind rule inside is not valid, so reject it instead of + * returning null and letting the caller dereference it later. + */ + if (bindrule_1 == null) { + throw new AciException(WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX.get(input)); + } return bindrule_1; } else @@ -295,6 +303,15 @@ private static BindRule parseAndCreateBindrule(Matcher bindruleMatcher) throws A */ private static BindRule createBindRule(BindRule bindrule, String remainingBindruleStr) throws AciException { + /* + * The left operand of an "and"/"or" bind rule must exist. It is null when + * the left side reduced to an empty group such as "()" (for example the + * bind rule "()or userdn=..."). A complex bind rule with a null left side + * would later be dereferenced during evaluation, so reject it here. + */ + if (bindrule == null) { + throw new AciException(WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX.get(remainingBindruleStr)); + } Pattern remainingBindrulePattern = Pattern.compile(remainingBindruleRegex); Matcher remainingBindruleMatcher = remainingBindrulePattern.matcher(remainingBindruleStr); if (remainingBindruleMatcher.find()) { @@ -318,6 +335,15 @@ private static BindRule createBindRule(BindRule bindrule, boolean negate=determineNegation(ruleExpr); remainingBindrule=ruleExpr.toString(); BindRule bindrule_2 = BindRule.decode(remainingBindrule); + /* + * The right operand of an "and"/"or" bind rule must exist. It is null + * when the boolean operator is not followed by a bind rule, e.g. the + * "or" in "(()or)" has nothing to its right (issue #719). + */ + if (bindrule_2 == null) { + throw new AciException( + WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX.get(remainingBindruleStr)); + } bindrule_2.setNegate(negate); return new BindRule(bindrule, bindrule_2, operand); } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleOperandTest.java b/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleOperandTest.java new file mode 100644 index 0000000000..8e769a0d25 --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleOperandTest.java @@ -0,0 +1,85 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions Copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package org.opends.server.authorization.dseecompat; + +import static org.assertj.core.api.Assertions.*; + +import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.DN; +import org.opends.server.DirectoryServerTestCase; +import org.opends.server.TestCaseUtils; +import org.opends.server.types.DirectoryException; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +/** + * Verifies that {@link BindRule#decode(String)} rejects a boolean ("and"/"or") + * bind rule that is missing one of its operands, instead of throwing a + * {@link NullPointerException}. An empty group such as "()" decodes to a null + * bind rule; before this fix, using such a group as the left side, or leaving + * the right side of the boolean empty (e.g. "(()or)"), dereferenced that null + * while parsing and failed with an NPE rather than a diagnosable + * {@link AciException} (issue #719). + */ +@SuppressWarnings("javadoc") +public class BindRuleOperandTest extends DirectoryServerTestCase +{ + @BeforeClass + public void setUp() throws Exception + { + TestCaseUtils.startFakeServer(); + } + + @AfterClass + public void tearDown() throws DirectoryException + { + TestCaseUtils.shutdownFakeServer(); + } + + @DataProvider(name = "bindRulesWithMissingOperand") + public Object[][] bindRulesWithMissingOperand() + { + return new Object[][] { + // the exact bind rule from issue #719: "or" with no right operand and an + // empty left group + { "(()or)" }, + // empty right operand after a valid left operand + { "(userdn=\"ldap:///self\" or)" }, + { "(userdn=\"ldap:///self\" and)" }, + // empty left operand before a valid right operand + { "()or userdn=\"ldap:///self\"" }, + // an empty group is not a bind rule on its own + { "()" }, + }; + } + + @Test(dataProvider = "bindRulesWithMissingOperand") + public void rejectsMissingOperand(String bindRule) + { + assertThatThrownBy(() -> BindRule.decode(bindRule)).isInstanceOf(AciException.class); + } + + /** Reproduces the full ACI from issue #719 through {@link Aci#decode}. */ + @Test + public void decodeOfAciWithMissingOperandThrowsAciException() + { + String aci = "(version 3.0; acl \"ac\"; allow (search)(()or) (userdn=\"ldap:///self\"); )"; + assertThatThrownBy(() -> Aci.decode(ByteString.valueOfUtf8(aci), DN.rootDN())) + .isInstanceOf(AciException.class); + } +} From 6d478626b59a7b068fbaeb0a4e1f21c429986a10 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 9 Jul 2026 15:56:23 +0300 Subject: [PATCH 2/2] Address review comments on #719 bind-rule operand fix - Hoist the null left-operand guard into BindRule.decode, above the remaining-chars test, so one check rejects both a bare empty group ("()") and a missing left operand ("()or userdn=..."), and the message names the full offending input. Remove the now-redundant null guard in createBindRule (its only null-capable caller is guarded upstream). - Correct the bindrule_2 comment: "(()or)" is caught by the left guard, so use the input that actually reaches it ('userdn="ldap:///self" or'). - Document decode's null base case in the javadoc and fix the stale blank-bind-rule comment. - Add tests: malformed "(())", "( )", '... or ()', "not ()"; positive controls for valid nested/complex rules; and an ACI-level missing-left regression case. --- .../authorization/dseecompat/BindRule.java | 41 +++++++++-------- .../dseecompat/BindRuleOperandTest.java | 45 +++++++++++++++++-- 2 files changed, 61 insertions(+), 25 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java index 5c94c730ae..96bbef501c 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java @@ -142,7 +142,9 @@ private BindRule(BindRule left, BindRule right, EnumBooleanTypes booleanType) { /** * Decode an ACI bind rule string representation. * @param input The string representation of the bind rule. - * @return A BindRule class representing the bind rule. + * @return A BindRule class representing the bind rule, or {@code null} if + * {@code input} is null or blank. This null is the recursion base case for + * an empty group; callers must reject it rather than dereference it. * @throws AciException If the string is an invalid bind rule. */ public static BindRule decode (String input) throws AciException { @@ -153,8 +155,10 @@ public static BindRule decode (String input) throws AciException { String bindruleStr = input.trim(); if (bindruleStr.isEmpty()) { - // A blank bind rule (e.g. "( )") must be rejected as a - // syntax error rather than throwing StringIndexOutOfBoundsException. + // A blank bind rule (e.g. "( )") decodes to null rather than + // throwing StringIndexOutOfBoundsException on charAt(0). This null is + // the recursion base case for an empty group; the caller rejects it as + // a syntax error (see the bindrule_1 == null check below). return null; } char firstChar = bindruleStr.charAt(0); @@ -202,6 +206,17 @@ else if (!inQuotes && currentChar == ')') if (numOpen > numClose) { throw new AciException(WARN_ACI_SYNTAX_BIND_RULE_MISSING_CLOSE_PAREN.get(input)); } + /* + * An empty group such as "()" or "( )" decodes to a null bind rule. + * Such a group is not a valid bind rule on its own, and using it as the + * left operand of an "and"/"or" (e.g. "()or userdn=...") would build a + * complex rule with a null left side that is later dereferenced during + * evaluation. Reject both cases here, before the remaining-chars test, + * so the message names the full offending input rather than a fragment. + */ + if (bindrule_1 == null) { + throw new AciException(WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX.get(input)); + } /* * If there are remaining chars => there MUST be an operand (AND / OR) * otherwise there is a syntax error @@ -212,14 +227,6 @@ else if (!inQuotes && currentChar == ')') bindruleStr.substring(currentPos + 1); return createBindRule(bindrule_1, remainingBindruleStr); } - /* - * An empty group such as "()" decodes to a null bind rule. A group - * with no bind rule inside is not valid, so reject it instead of - * returning null and letting the caller dereference it later. - */ - if (bindrule_1 == null) { - throw new AciException(WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX.get(input)); - } return bindrule_1; } else @@ -303,15 +310,6 @@ private static BindRule parseAndCreateBindrule(Matcher bindruleMatcher) throws A */ private static BindRule createBindRule(BindRule bindrule, String remainingBindruleStr) throws AciException { - /* - * The left operand of an "and"/"or" bind rule must exist. It is null when - * the left side reduced to an empty group such as "()" (for example the - * bind rule "()or userdn=..."). A complex bind rule with a null left side - * would later be dereferenced during evaluation, so reject it here. - */ - if (bindrule == null) { - throw new AciException(WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX.get(remainingBindruleStr)); - } Pattern remainingBindrulePattern = Pattern.compile(remainingBindruleRegex); Matcher remainingBindruleMatcher = remainingBindrulePattern.matcher(remainingBindruleStr); if (remainingBindruleMatcher.find()) { @@ -338,7 +336,8 @@ private static BindRule createBindRule(BindRule bindrule, /* * The right operand of an "and"/"or" bind rule must exist. It is null * when the boolean operator is not followed by a bind rule, e.g. the - * "or" in "(()or)" has nothing to its right (issue #719). + * "or" in "userdn=\"ldap:///self\" or" has nothing to its right + * (issue #719). */ if (bindrule_2 == null) { throw new AciException( diff --git a/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleOperandTest.java b/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleOperandTest.java index 8e769a0d25..cd6ef77570 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleOperandTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleOperandTest.java @@ -65,6 +65,14 @@ public Object[][] bindRulesWithMissingOperand() { "()or userdn=\"ldap:///self\"" }, // an empty group is not a bind rule on its own { "()" }, + // a nested empty group is still empty + { "(())" }, + // a whitespace-only group is empty once trimmed + { "( )" }, + // empty right *group*: reaches the decode-level guard from createBindRule + { "(userdn=\"ldap:///self\" or ())" }, + // "not" applied to an empty group + { "not ()" }, }; } @@ -74,11 +82,40 @@ public void rejectsMissingOperand(String bindRule) assertThatThrownBy(() -> BindRule.decode(bindRule)).isInstanceOf(AciException.class); } - /** Reproduces the full ACI from issue #719 through {@link Aci#decode}. */ - @Test - public void decodeOfAciWithMissingOperandThrowsAciException() + @DataProvider(name = "validBindRules") + public Object[][] validBindRules() + { + return new Object[][] { + // a valid rule inside a nested group must still decode: guards must not + // start over-rejecting non-empty groups + { "((userdn=\"ldap:///self\"))" }, + // a valid complex rule with both operands present + { "(userdn=\"ldap:///self\" or userdn=\"ldap:///anyone\")" }, + }; + } + + @Test(dataProvider = "validBindRules") + public void acceptsValidBindRule(String bindRule) throws Exception + { + assertThat(BindRule.decode(bindRule)).isNotNull(); + } + + @DataProvider(name = "acisWithMissingOperand") + public Object[][] acisWithMissingOperand() + { + return new Object[][] { + // the full ACI from issue #719 + { "(version 3.0; acl \"ac\"; allow (search)(()or) (userdn=\"ldap:///self\"); )" }, + // missing *left* operand: previously accepted and stored with left == null, + // then NPEd during evaluation. Must now be rejected at decode time. + { "(version 3.0; acl \"ac\"; allow (search) ()or userdn=\"ldap:///self\"; )" }, + }; + } + + /** Reproduces missing-operand ACIs from issue #719 through {@link Aci#decode}. */ + @Test(dataProvider = "acisWithMissingOperand") + public void decodeOfAciWithMissingOperandThrowsAciException(String aci) { - String aci = "(version 3.0; acl \"ac\"; allow (search)(()or) (userdn=\"ldap:///self\"); )"; assertThatThrownBy(() -> Aci.decode(ByteString.valueOfUtf8(aci), DN.rootDN())) .isInstanceOf(AciException.class); }