[#1156] Keep colons inside the -J/--control value, and report an unreadable...

Valery Kharseko

yesterday 458f009de6ccb989ac565da62ac407f20755303e

[#1156] Keep colons inside the -J/--control value, and report an unreadable control value file instead of failing with a NullPointerException (#1163)

## Problem

The `-J, --control
{controloid[:criticality[:value|::b64value|:<filePath]]}` option of
`ldapsearch`, `ldapmodify`, `ldapcompare`, `ldapdelete` and
`ldappasswordmodify` cut the control value at its first `:`. `-J
2.16.840.1.113730.3.4.18:true:dn:uid=bjensen,ou=People,dc=example,dc=com`
sent the proxied-auth value `dn`, `1.2.3.4:false:urn:example:value` sent
`urn`, and a `:<C:\path` file on Windows lost everything after the drive
letter. When the file in `:<filePath` could not be read, `getControl()`
returned `null`, and `addControlsToRequest()` then failed with a
`NullPointerException`. See #1156.

## Cause

`Utils.getControl()` split the whole argument on every colon with
`argString.split(":")` and used only the third piece as the value (or
the fourth for base64). Its file branch caught every exception and
returned `null`.

## Change

- `getControl()` splits at the first two colons only, `split(":", 3)`.
Everything after the second colon is the value, so a plain value, the
base64 string and the file path can all contain colons. A value that
starts with `:` is base64, and one that starts with `<` is a file path,
as before.
- An unreadable file throws `DecodeException`, with
`ERR_FILEARG_CANNOT_READ_FILE` as the cause message. No new message is
added. `readControls()` turns it into the tool's usual `Invalid control
specification '…'` error.
- An invalid base64 value is reported the same way. Before,
`ByteString.valueOfBase64()` threw a `LocalizedIllegalArgumentException`
that `readControls()` did not catch, so the tool failed with an
exception.

Two edge cases change behaviour because the argument is now read
literally:

- `oid:true:` sends the control with an empty value. Before, it was sent
without a value. `oid:true::` (empty base64) also gives an empty value.
- `oid:` is rejected because its criticality is empty. Before, it was
accepted as `oid`.

## Tests

- New `ControlArgumentTestCase` checks `Utils.readControls()` directly.
It covers 11 valid forms: OID only, criticality, the `pwpolicy` alias,
`dn:` and `u:` proxied-auth IDs, a URN, an LDAP URL, base64 with a colon
in the decoded value, and empty values. It also covers 4 invalid ones:
bad criticality, with and without a value, a missing file, and bad
base64. Another case reads the value from a file whose path holds a
colon. On Windows the drive letter supplies the colon; elsewhere the
file name does.
- `LDAPSearchTestCase`: `testLdapSearchWithControlValueHoldingColons`
sends the proxied-auth `dn:` value through `-J` to the mocked server,
which checks the OID, criticality and value it receives. A missing
control value file is added to `invalidArgs`.

Without the fix (`Utils.java` from master): 9 of the 16
`ControlArgumentTestCase` cases fail (value `dn`, `urn`, `ldap`, `null`
control, uncaught `LocalizedIllegalArgumentException`). Both new
`LDAPSearchTestCase` cases fail too: one with result code 81, the other
with a `NullPointerException`. With the fix, all 309 unit tests of
`opendj-ldap-toolkit` pass.

Fixes #1156

 opendj-ldap-toolkit/src/main/java/com/forgerock/opendj/ldap/tools/Utils.java

@@ -18,8 +18,10 @@
 */
package com.forgerock.opendj.ldap.tools;

import static com.forgerock.opendj.cli.ArgumentConstants.OPTION_LONG_CONTROL;
import static com.forgerock.opendj.cli.ArgumentConstants.USE_SYSTEM_STREAM_TOKEN;
import static com.forgerock.opendj.cli.CliConstants.NO_WRAPPING_BY_DEFAULT;
import static com.forgerock.opendj.cli.CliMessages.ERR_FILEARG_CANNOT_READ_FILE;
import static com.forgerock.opendj.cli.Utils.filterExitCode;
import static com.forgerock.opendj.cli.Utils.readBytesFromFile;
import static com.forgerock.opendj.cli.Utils.secondsToTimeString;
@@ -313,19 +315,21 @@
    /**
     * Parse the specified command line argument to create the appropriate
     * LDAPControl. The argument string should be in the format
     * controloid[:criticality[:value|::b64value|:<fileurl]]
     * controloid[:criticality[:value|::b64value|:<filePath]]
     * <p>
     * Everything after the second colon is the value, so the value, the
     * base64 string and the file path may all contain colons.
     *
     * @param argString
     *            The argument string containing the encoded control
     *            information.
     * @return The control decoded from the provided string, or
     *         <CODE>null</CODE> if an error occurs while parsing the argument
     *         value.
     * @return The control decoded from the provided string.
     * @throws org.forgerock.opendj.ldap.DecodeException
     *             If an error occurs.
     *             If the criticality is invalid, the base64 value cannot be
     *             decoded or the file cannot be read.
     */
    private static GenericControl getControl(final String argString) throws DecodeException {
        final String[] control = argString.split(":");
        final String[] control = argString.split(":", 3);
        final int nbControlElements = control.length;

        final String controlOID = readControlID(control[0]);
@@ -339,14 +343,20 @@
        }

        final ByteString controlValue;
        if (control[2].isEmpty()) {
            controlValue = ByteString.valueOfBase64(control[3]);
        if (control[2].startsWith(":")) {
            try {
                controlValue = ByteString.valueOfBase64(control[2].substring(1));
            } catch (final LocalizedIllegalArgumentException e) {
                throw DecodeException.error(e.getMessageObject(), e);
            }
        } else if (control[2].startsWith("<")) {
            // Read data from the file.
            final String filePath = control[2].substring(1);
            try {
                controlValue = ByteString.wrap(readBytesFromFile(control[2].substring(1)));
            } catch (final Exception e) {
                return null;
                controlValue = ByteString.wrap(readBytesFromFile(filePath));
            } catch (final IOException e) {
                throw DecodeException.error(
                        ERR_FILEARG_CANNOT_READ_FILE.get(filePath, OPTION_LONG_CONTROL, e.getMessage()), e);
            }
        } else {
            controlValue = ByteString.valueOfUtf8(control[2]);

 opendj-ldap-toolkit/src/test/java/com/forgerock/opendj/ldap/tools/ControlArgumentTestCase.java

New file
@@ -0,0 +1,134 @@
/*
 * 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 com.forgerock.opendj.ldap.tools;

import static com.forgerock.opendj.ldap.tools.ToolsMessages.ERR_TOOL_INVALID_CONTROL_STRING;
import static com.forgerock.opendj.util.OperatingSystem.isWindows;
import static org.fest.assertions.Assertions.assertThat;
import static org.testng.Assert.fail;

import java.io.File;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.List;

import org.forgerock.opendj.ldap.ByteString;
import org.forgerock.opendj.ldap.ResultCode;
import org.forgerock.opendj.ldap.controls.Control;
import org.forgerock.testng.ForgeRockTestCase;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

import com.forgerock.opendj.cli.CommonArguments;
import com.forgerock.opendj.cli.StringArgument;

/** Tests the parsing of the {@code -J/--control} argument by {@link Utils#readControls(StringArgument)}. */
@Test
public final class ControlArgumentTestCase extends ForgeRockTestCase {

    @DataProvider
    public Object[][] validControls() {
        return new Object[][] {
            { "1.2.3.4", "1.2.3.4", false, null },
            { "1.2.3.4:true", "1.2.3.4", true, null },
            { "1.2.3.4:FALSE:value", "1.2.3.4", false, "value" },
            { "1.2.3.4:true:", "1.2.3.4", true, "" },
            { "pwpolicy:true", "1.3.6.1.4.1.42.2.27.8.5.1", true, null },
            // Everything after the second colon is the value, colons included.
            { "2.16.840.1.113730.3.4.18:true:dn:uid=bjensen,ou=People,dc=example,dc=com",
              "2.16.840.1.113730.3.4.18", true, "dn:uid=bjensen,ou=People,dc=example,dc=com" },
            { "2.16.840.1.113730.3.4.18:true:u:bjensen", "2.16.840.1.113730.3.4.18", true, "u:bjensen" },
            { "1.2.3.4:false:urn:example:value", "1.2.3.4", false, "urn:example:value" },
            { "1.2.3.4:false:ldap://host:1389/dc=example", "1.2.3.4", false, "ldap://host:1389/dc=example" },
            // The base64 form decodes to a value that can itself hold colons.
            { "1.2.3.4:true::" + base64("dn:uid=bjensen"), "1.2.3.4", true, "dn:uid=bjensen" },
            { "1.2.3.4:true::", "1.2.3.4", true, "" },
        };
    }

    @Test(dataProvider = "validControls")
    public void testValidControl(final String argument, final String oid, final boolean critical,
            final String value) throws Exception {
        final Control control = readSingleControl(argument);
        assertThat(control.getOID()).isEqualTo(oid);
        assertThat(control.isCritical()).isEqualTo(critical);
        if (value == null) {
            assertThat(control.hasValue()).isFalse();
        } else {
            assertThat(control.hasValue()).isTrue();
            assertThat(control.getValue().toString()).isEqualTo(value);
        }
    }

    @Test
    public void testValueReadFromFileWhosePathHoldsAColon() throws Exception {
        // On Windows the drive letter puts a colon in every absolute path; elsewhere the file name carries one.
        final File dir = Files.createTempDirectory("control-value").toFile();
        final File file = new File(dir, isWindows() ? "control-value.ber" : "control:value.ber");
        try {
            final byte[] bytes = { 0x04, 0x03, 'a', ':', 'b' };
            Files.write(file.toPath(), bytes);

            final Control control = readSingleControl("1.2.3.4:true:<" + file.getAbsolutePath());
            assertThat(control.getOID()).isEqualTo("1.2.3.4");
            assertThat(control.isCritical()).isTrue();
            assertThat(control.getValue()).isEqualTo(ByteString.wrap(bytes));
        } finally {
            file.delete();
            dir.delete();
        }
    }

    @DataProvider
    public Object[][] invalidControls() {
        final String missingFile = new File(System.getProperty("java.io.tmpdir"), "no-such-dir-1156/value.ber")
                .getAbsolutePath();
        return new Object[][] {
            { "1.2.3.4:invalidcriticality" },
            { "1.2.3.4:invalidcriticality:value" },
            { "1.2.3.4:true:<" + missingFile },
            { "1.2.3.4:true::not*base64" },
        };
    }

    @Test(dataProvider = "invalidControls")
    public void testInvalidControlIsReportedAsAnInvalidControlString(final String argument) throws Exception {
        try {
            readControls(argument);
            fail("Expected the control '" + argument + "' to be rejected");
        } catch (final LDAPToolException e) {
            assertThat(e.getResultCode()).isEqualTo(ResultCode.CLIENT_SIDE_PARAM_ERROR.intValue());
            assertThat(e.getMessage()).isEqualTo(ERR_TOOL_INVALID_CONTROL_STRING.get(argument).toString());
        }
    }

    private static Control readSingleControl(final String argument) throws Exception {
        final List<Control> controls = readControls(argument);
        assertThat(controls).hasSize(1);
        return controls.get(0);
    }

    private static List<Control> readControls(final String argument) throws Exception {
        final StringArgument controlArg = CommonArguments.controlArgument();
        controlArg.addValue(argument);
        controlArg.setPresent(true);
        return Utils.readControls(controlArg);
    }

    private static String base64(final String value) {
        return ByteString.valueOfBytes(value.getBytes(StandardCharsets.UTF_8)).toBase64String();
    }
}

 opendj-ldap-toolkit/src/test/java/com/forgerock/opendj/ldap/tools/LDAPSearchTestCase.java

@@ -12,6 +12,7 @@
 * information: "Portions Copyright [year] [name of copyright owner]".
 *
 *  Copyright 2016 ForgeRock AS.
 *  Portions Copyright 2026 3A Systems, LLC.
 */
package com.forgerock.opendj.ldap.tools;

@@ -142,6 +143,11 @@
        argLists.add(args("-b", "", "-J", "1.2.3.4:invalidcriticality", "(objectClass=*)"));
        reasonList.add(ERR_TOOL_INVALID_CONTROL_STRING.get("1.2.3.4:invalidcriticality"));

        argLists.add(args("-b", "", "-J", "1.2.3.4:true:<src/test/resources/no-such-control-value.ber",
                          "(objectClass=*)"));
        reasonList.add(ERR_TOOL_INVALID_CONTROL_STRING.get(
                "1.2.3.4:true:<src/test/resources/no-such-control-value.ber"));

        argLists.add(args("-b", "", "-s", "invalid", "(objectClass=*)"));
        reasonList.add(ERR_MCARG_VALUE_NOT_ALLOWED.get("searchScope", "invalid"));

@@ -236,6 +242,12 @@
    }

    @Test
    public void testLdapSearchWithControlValueHoldingColons() throws Exception {
        controls.add(ProxiedAuthV2RequestControl.newControl("dn:uid=bjensen,ou=People,dc=example,dc=com"));
        runToolOnMockedServer("-J", "2.16.840.1.113730.3.4.18:true:dn:uid=bjensen,ou=People,dc=example,dc=com");
    }

    @Test
    public void testLdapSearchWithSimplePaged() throws Exception {
        controls.add(SimplePagedResultsControl.newControl(true, 10, ByteString.empty()));
        runToolOnMockedServer("--simplePageSize", "10");