Skip to content

[#1159] Keep administrator-typed DNs intact in dsconfig batch lines, tools.properties and the SDK LDAPUrl - #1166

Open
vharseko wants to merge 3 commits into
OpenIdentityPlatform:masterfrom
vharseko:issue-1159-admin-typed-dn
Open

vharseko wants to merge 3 commits into
OpenIdentityPlatform:masterfrom
vharseko:issue-1159-admin-typed-dn

Conversation

@vharseko

@vharseko vharseko commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Fixes #1159

A DN typed by an administrator was changed before it reached the parser in three places. This PR fixes all three and documents the rules that stay.

1. dsconfig batch lines (DSConfig.toCommandArgs)

A batch line is now split into arguments the way a POSIX shell splits it, without expanding variables:

  • Outside quotes, a backslash escapes the next char, as before.
  • Inside double quotes, a backslash escapes only ", \, $ and `. Before any other char it is kept, so --set base-dn:"cn=a\,b,dc=x" now gives cn=a\,b,dc=x instead of cn=a,b,dc=x.
  • Single quotes are supported and keep everything literally: 'base-dn:o=My Company' is one argument.
  • Quoted and unquoted parts of one word make one argument ("a"b gives ab; before, it gave two arguments), and an empty quoted word ("") is an empty argument.
  • A tab separates arguments as a space does.
  • A trailing backslash is kept. Before, it threw StringIndexOutOfBoundsException.
  • A quote that is not closed is an error, as in a POSIX shell. dsconfig prints The following batch command has a quote that is not closed: ... and stops the batch with exit code 2 (ERROR_USER_DATA), as it stops on a command that fails. Before, an unclosed " silently took the rest of the line.

How handleBatch joins lines (DSConfig.nextBatchCommand):

  • A line continues on the next one only when it ends in an unescaped backslash, that is, an odd number of backslashes. Before, pa\\ at the end of a line was taken as a continuation, so it ran together with the next command, or was dropped at the end of the file. --commandFilePath writes such lines on UNIX, because it escapes a backslash in a value that ends a command.
  • A command whose last line continues still runs at the end of the file. Before, it was dropped.
  • Only the echoed command is trimmed. Before, the trim ran before the split, so description:value\ at the end of a line gave description:value\ instead of description:value .
  • A line that holds only blanks is skipped, as an empty line is. Before, it ran dsconfig with no subcommand, which failed with ERR_DSCFG_ERROR_MISSING_SUBCOMMAND and stopped the batch.
  • Empty lines and lines that start with # stay skipped inside a continued command, as before, so a line of a long command can be commented out. A POSIX shell would end the command there instead.

The batch file and standard input are still read in the platform charset, as --commandFilePath writes them (FileWriter), so non-ASCII characters in a batch line depend on the platform as before.

Compatibility (for the release notes): a single quote in a batch line is now a quote character, not a literal. A line such as --set description:O'Brien that worked before is now rejected. Write O\'Brien or "O'Brien".

2. Windows quoting in CommandBuilder.escapeValue

This differs from the suggested fix in the issue. Doubling every backslash inside the quotes would break the printed command on the Windows command line itself: a Windows program splits its command line by the C runtime rules, which take backslashes literally unless they precede a double quote. The value is now quoted by those rules. Backslashes before a " or before the closing quote are doubled, and " is escaped as \". Before, a value with " (any ACI) was printed broken, and a value ending in \ escaped the closing quote.

With the batch tokenizer above, the Windows form now reads back as the same value in a batch file, including cn=a\,b,dc=x. One difference remains and cannot be removed while the command also stays valid for cmd: a run of two or more backslashes that is not followed by a quote, and \$ or \`, are halved in a batch line but kept on the Windows command line. The UNIX form is not changed.

The target is the C runtime split of the Java program's command line. Characters that cmd.exe interprets itself (&, |, <, >) are not caret-escaped: cmd.exe toggles its quote state at every ", including an escaped one, so after \" they are unquoted for it, as on master. The dsconfig description and the admin guide now say so.

3. tools.properties (ArgumentParser, both readers)

  • The file is read as UTF-8. If it is not valid UTF-8, it is read as ISO-8859-1, as before, so files written for earlier releases keep working. A leading UTF-8 byte order mark is skipped in both cases: it is removed as bytes before decoding, so a UTF-8 file with a BOM that later got one Latin-1 byte still has its first key read.
  • The backslash keeps its meaning in the properties format. Reading the file differently would break \uXXXX escapes, line continuations, and values already written with doubled backslashes. The template config/tools.properties, the Files section of the tool man pages, the developer guide, and the production password note now say that a backslash must be doubled.

4. SDK LDAPUrl

  • A run of %XX octets is decoded as UTF-8 (RFC 4516 section 2.1), so ldap:///cn=J%C3%B6rg,dc=x gives cn=Jörg,dc=x. A run that is not valid UTF-8 as a whole (such as %f6, which this class used to produce for ö) decodes to one char per octet, as before. That includes valid UTF-8 octets in the same run: %C3%B6%F6 gives öö.
  • A char outside the allowed set is encoded as the octets of its UTF-8 encoding, two hex digits each, including chars outside the BMP. Before, Ж became %416, and a char below U+0010 got a single hex digit.

Documentation

  • man-pages/_description-dsconfig.adoc and admin-guide/chap-admin-tools.adoc: how a batch line is split, with a DN example; that a single quote is now a quote character, an unclosed quote is rejected, an escaped trailing backslash does not continue the line, and empty lines, lines of blanks and comments are skipped, also inside a continued command; and that the Windows equivalent command follows the C runtime rules, not cmd.exe.
  • man-pages/_files.adoc, server-dev-guide/chap-ldap-operations.adoc, and admin-guide/chap-production.adoc: the encoding of tools.properties and doubled backslashes.

The changed pages were rendered with AsciidoctorJ to check that the backslashes and quotes come out as written.

Tests

  • DSConfigParseTest: 11 new batch lines with their exact argument lists, 4 lines with an unclosed quote that must be rejected with its message, 12 batch files with the commands nextBatchCommand reads from them (continuation, an escaped trailing backslash, an escaped trailing blank, a continued last line, a line of blanks, an empty line and a comment inside a continued command), and 2 commands run through DSConfig.runBatchCommand, the body of the handleBatch loop: a line with an unclosed quote gives exit code 2 and the message, and an escaped trailing blank reaches dsconfig untrimmed.
  • New CommandBuilderTestCase: Windows quoting (ACI quotes, trailing backslash, backslash before a quote, empty value) and the unchanged UNIX form, including the escaped single quote that lets a --commandFilePath file be replayed as a batch.
  • New ArgumentParserPropertiesFileTestCase: UTF-8, ISO-8859-1 fallback, byte order mark (also in the ISO-8859-1 fallback), and doubled backslash, each through --propertiesFilePath and through the explicit-path parseArguments.
  • LDAPUrlTestCase: UTF-8 decoding of the DN and filter, the non-UTF-8 fallback, UTF-8 encoding with a round trip, and two hex digits for an octet below 0x10.

Before the fix, 22 of the new cases failed (5 in LDAPUrlTestCase, 7 in opendj-cli, 10 in DSConfigParseTest). Each of four mutants turned exactly its own tests red: no doubling of a trailing backslash, no ISO-8859-1 fallback in LDAPUrl, the same in ArgumentParser, and no BOM skip. With the fix, the full unit suites pass with no failures: opendj-core 8188, opendj-grizzly 1040, opendj-cli 61, opendj-ldap-toolkit 291, and opendj-config 562 tests.

Review round 1, rebased on 751a4d2ee8: seven more mutants each turned only their own new tests red: an unclosed single quote taking the rest of the line, an unclosed double quote, the trim before the split placed in nextBatchCommand, any trailing backslash continuing the line, a continued last line being dropped, the BOM kept in the ISO-8859-1 fallback, and one hex digit for an octet below 0x10. The full unit suites of the modules this round changes pass: opendj-core 8189, opendj-cli 65, and opendj-config 574 tests, with javadoc (failOnWarnings) on all three. The changed doc pages were rendered with AsciidoctorJ again.

Review round 2, rebased on 85b28b3d0f: seven more mutants each turned only their own new tests red: a line of blanks run as a command, a comment ending a continued command, the batch error path returning success, the batch error path without its message, another message key for an unclosed quote, the trim put back before the split where handleBatch passes the command to dsconfig, and no escape of a single quote in the UNIX form. The full unit suites pass: opendj-core 8190, opendj-cli 66, and opendj-config 580 tests, with javadoc on all three. The changed doc pages were rendered with AsciidoctorJ.

Not changed here

  • The LDAPUrl constructor writes the filter into toString() without percent-encoding it ((cn=J\C3\B6rg)), while equals() encodes it. This is older behavior and not about UTF-8. The new test checks the DN part and the round trip.
  • ArgumentParser.parseArguments(String[], String, boolean) matches property names case-sensitively (the key must be basedn, not baseDN). The --propertiesFilePath reader lowercases them. Nothing in the repository calls the former.

@vharseko vharseko added bug Windows docs tests Test suites: fixing, enabling, un-disabling java Changes to Java sources labels Oct 2, 2026
@vharseko
vharseko requested a review from maximthomas October 2, 2026 16:46

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The fixes keep old inputs readable while switching to UTF-8.

  • ArgumentParser.loadPropertiesFile and LDAPUrl.decodeOctets decode with a strict UTF-8 decoder and fall back to ISO-8859-1, so tools.properties files and URLs written for earlier releases still read as before.
  • The UNIX branch of CommandBuilder.escapeValue is unchanged and still escapes ' (CHARSTOESCAPE), so UNIX --commandFilePath files are not affected by the new single-quote rule.
  • LDAPUrl.percentEncoder walks code points, so a char outside the BMP is encoded as its four UTF-8 octets (%f0%9f%98%80, pinned by testPercentEncodingIsUtf8).

issue (non-blocking): An unterminated single quote in a batch line silently takes the rest of the line into one argument.

opendj-config/src/main/java/org/forgerock/opendj/config/dsconfig/DSConfig.java:1477-1481

At BASE, ' was a literal. Now the ' arm quotes up to the next ' or, when there is none, to the end of the line, with no error. A hand-written line that worked before, set-backend-prop --backend-name userRoot --set description:O'Brien --advanced, now gives the single argument description:OBrien --advanced: the value changes and --advanced is lost. A probe ran the toCommandArgs bodies of both SHAs on this line. The new docs say a line is split "as a POSIX shell does", and a POSIX shell rejects an unterminated quote. An unterminated " was already silent at BASE. Files written by --commandFilePath are not affected.

            case '\'':
                final int end = command.indexOf('\'', i + 1);
                if (end < 0) {
                    throw new IllegalArgumentException("Unterminated single quote: " + command);
                }
                builder.append(command, i + 1, end);
                i = end;
                break;

Do the same after the " loop when i == command.length(). Have handleBatch report the line as an error (a new ERR_DSCFG_ message), and add DSConfigParseTest rows for both quotes. A release-note line saying that ' is now a quote in batch lines would help users with existing files.


issue (non-blocking): A batch line that ends in an escaped blank now gets a value with a literal trailing backslash, where BASE failed.

opendj-config/src/main/java/org/forgerock/opendj/config/dsconfig/DSConfig.java:1412-1413, :1473-1476

handleBatch trims the joined line before toCommandArgs. A trailing \ therefore reaches the parser as a lone trailing backslash, and the new arm keeps it: set-x --set description:value\ gives description:value\ instead of description:value . BASE threw StringIndexOutOfBoundsException on the same input, so the operator saw a failure. HEAD applies a wrong value silently. Through handleBatch the trailing-backslash arm is only reached in this case, because a line that ends in \ is a continuation (:1406). The { "a b\\" } row calls toCommandArgs directly, so it does not go through the trim.

                command += line;
                printlnNoWrap(LocalizableMessage.raw(command.trim()));

                // Append initial arguments to the file line
                final String[] allArgsArray = buildCommandArgs(initialArgs, command);

toCommandArgs already skips leading and trailing blanks, so only the echoed line needs the trim.


issue (non-blocking): handleBatch treats a line that ends in an escaped backslash as a continuation.

opendj-config/src/main/java/org/forgerock/opendj/config/dsconfig/DSConfig.java:1406-1409

This is not in the diff and behaves the same at BASE. But the new docs now promise POSIX splitting, and this breaks it. line.endsWith("\\") does not check whether that backslash is itself escaped. The lines set-x --bindPassword pa\\ and set-y --foo bar run as one command, [set-x, --bindPassword, paset-y, --foo, bar]. When such a line is the last line of the file, its command is dropped at EOF. A UNIX --commandFilePath file hits this too. escapeValue writes a value that ends in \ as \\, and a command's last argument ends its line, because LINE_SEPARATOR (CommandBuilder.java:40-41) goes only between arguments. A follow-up is fine.

    private static boolean continuesOnNextLine(final String line) {
        int backslashes = 0;
        for (int i = line.length() - 1; i >= 0 && line.charAt(i) == '\\'; i--) {
            backslashes++;
        }
        return backslashes % 2 == 1;
    }

issue (non-blocking): The byte order mark is not skipped when the file falls back to ISO-8859-1.

opendj-cli/src/main/java/com/forgerock/opendj/cli/ArgumentParser.java:452-466

The BOM is stripped as U+FEFF after decoding. After the ISO-8859-1 fallback it is the three chars , so nothing is stripped. Take a tools.properties saved as UTF-8 with a BOM and later appended to from a Latin-1 shell, so that it holds one non-UTF-8 byte. Its first key becomes basedn, normalizeArguments looks up basedn, and the default is silently not applied. This is not a regression, but stripping the BOM at the byte level fixes it with no trade-off.

        final byte[] content = Files.readAllBytes(Paths.get(path));
        final int bom = content.length >= 3 && content[0] == (byte) 0xEF && content[1] == (byte) 0xBB
                && content[2] == (byte) 0xBF ? 3 : 0;
        String text;
        try {
            text = UTF_8.newDecoder().decode(ByteBuffer.wrap(content, bom, content.length - bom)).toString();
        } catch (final CharacterCodingException e) {
            text = new String(content, bom, content.length - bom, ISO_8859_1);
        }
        final Properties properties = new Properties();

Pin: write EF BB BF + basedn=dc=example,dc=com\n + x=\xF6\n, and expect the basedn default to apply. It is red at HEAD.


question (non-blocking): Should the Windows equivalent command work when pasted into cmd.exe, or only when split by the C runtime or replayed as a batch line?

opendj-cli/src/main/java/com/forgerock/opendj/cli/CommandBuilder.java:264-282

a"&b is printed as "a\"&b". cmd.exe flips its quote state at every " and gives \ no meaning. After the escaped quote, cmd therefore sees &, |, < and > as unquoted. An ACI such as (targetfilter="(&(objectClass=person))")..., printed as the equivalent command by Task.java:860-872 or the control panel, would split into two commands. BASE split the same way. The C runtime split, which the description promises, is correct. If cmd.exe is a target, this is a Minor issue: caret-escape the metacharacters in the parts cmd sees as unquoted. If it is not, a line in the docs saying that the Windows form follows the C runtime rules is enough. Not run: no Windows host.


suggestion (non-blocking): No test pins the two-hex-digit form of an encoded octet below 0x10.

opendj-core/src/main/java/org/forgerock/opendj/ldap/LDAPUrl.java:298-302

Every octet that testPercentEncodingIsUtf8 encodes is >= 0x10, so the mutant encodedBuffer.append(Integer.toHexString(b & 0xFF)) survives. Such an octet can still reach the encoder: Filter.equality only null-checks the attribute description, and toNormalizedString percent-encodes filter.toString() for equals/hashCode.

    @Test
    public void testPercentEncodingPadsOctetsBelow0x10() {
        final LDAPUrl url1 = new LDAPUrl(false, "h", 389, DN.valueOf("dc=x"),
                SearchScope.WHOLE_SUBTREE, Filter.equality("a\u0001b", "x"));
        final LDAPUrl url2 = new LDAPUrl(false, "h", 389, DN.valueOf("dc=x"),
                SearchScope.WHOLE_SUBTREE, Filter.equality("a\u001b", "x"));
        assertNotEquals(url1, url2);
    }

Pin: HEAD encodes a%01b and a%1b. Under the mutant both are a%1b and the URLs compare equal. This was traced by reading, not run. It needs import static org.testng.Assert.assertNotEquals;.


note (non-blocking): Two statements in the PR text go further than the code.

  • Title, "non-ASCII characters ... in dsconfig batch lines": DSConfig.batchCommandsReader (DSConfig.java:1431-1441) still reads the batch file and stdin in the platform default charset (FileReader, InputStreamReader). Only the splitter changed.
  • Description, LDAPUrl, "Octets that are not UTF-8 ... still decode to one char each": LDAPUrl.decodeOctets falls back to ISO-8859-1 for the whole contiguous %XX run. Valid UTF-8 octets next to an invalid one also decode one char each (%C3%B6%F6 gives öö).

@vharseko
vharseko force-pushed the issue-1159-admin-typed-dn branch from 8d48f40 to d2d2683 Compare October 4, 2026 08:43
@vharseko vharseko changed the title [#1159] Keep the escapes and non-ASCII characters of administrator-typed DNs in dsconfig batch lines, tools.properties and the SDK LDAPUrl [#1159] Keep administrator-typed DNs intact in dsconfig batch lines, tools.properties and the SDK LDAPUrl Oct 4, 2026
@vharseko

vharseko commented Oct 4, 2026 •

Copy link
Copy Markdown
Member Author

Round 1 is addressed in d2d2683. The branch is rebased on 751a4d2ee8 with no conflicts. The description is updated.

Unterminated single quote: fixed. An unclosed ' or " now makes toCommandArgs throw an ArgumentException with the new ERR_DSCFG_ERROR_BATCH_UNCLOSED_QUOTE_1197. handleBatch prints it and stops the batch with ERROR_USER_DATA, as it does for a command that fails. I used a checked ArgumentException rather than IllegalArgumentException: toCommandArgs is called outside main, where only IOException was caught, so an unchecked exception would have ended dsconfig with a stack trace. DSConfigParseTest has 4 rejected lines, including your O'Brien line and an escaped \" before the end of the line. The dsconfig description and the admin guide now say that ' is a quote character, no longer a literal, and show O\'Brien and "O'Brien". The description has a "Compatibility (for the release notes)" line.

Trailing escaped blank: fixed. Only the echoed command is trimmed now. The split gets the untrimmed command, so description:value\ gives description:value .

Escaped backslash at the end of a line: fixed in this PR rather than in a follow-up, because the new docs promise POSIX splitting and --commandFilePath writes such lines on UNIX. The line reading moved to DSConfig.nextBatchCommand(BufferedReader). A line continues only when it ends in an odd number of backslashes (your continuesOnNextLine). I also changed the end-of-file case: a command whose last line continues now runs instead of being dropped. A POSIX shell runs it too. DSConfigParseTest.testBatchFileCommands covers 8 batch files read through nextBatchCommand, including your pa\\ + set-y pair and the trailing \ .

BOM in the ISO-8859-1 fallback: fixed as suggested. The BOM is removed as bytes before decoding. Your pin, through both readers, is red without the fix.

cmd.exe: no. The target is the C runtime split of the Java program's command line. Caret-escaping would need to follow how cmd.exe toggles its quote state at every ", including an escaped one, and % in .bat files on top of that, and the behaviour is the same as on master. The dsconfig description and the admin guide now say that the Windows command follows the C runtime rules and does not escape &, |, <, > for cmd.exe, for example in an ACI with an & filter.

Octet below 0x10: added LDAPUrlTestCase.testPercentEncodingPadsOctetsBelow0x10, with the control chars U+0001 and U+001B written as Java Unicode escapes rather than raw chars in the source. It is red under the Integer.toHexString(b & 0xFF) mutant.

PR text:

  • Title: "non-ASCII characters ... in dsconfig batch lines" is dropped. I did not change the batch reader: --commandFilePath writes the file with FileWriter in the platform charset (DSConfig.java:1353), so reading it as UTF-8 alone would break files that dsconfig itself wrote on a non-UTF-8 platform. The description now says that the batch file and standard input are still read in the platform charset.
  • LDAPUrl: the description now says that a run that is not valid UTF-8 as a whole decodes one char per octet, including the valid octets in it (%C3%B6%F6 gives öö).

Each of seven mutants, one per change above, turned only its own new tests red. The full unit suites pass: opendj-core 8189, opendj-cli 65, and opendj-config 574 tests, with javadoc on all three.

@vharseko
vharseko requested a review from maximthomas October 4, 2026 08:44

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Round 1 is fixed where the bugs are, and CI ran the new cases green at this head.

  • toCommandArgs throws a checked ArgumentException (ERR_DSCFG_ERROR_BATCH_UNCLOSED_QUOTE), and handleBatch catches it per command (DSConfig.java:1406-1409). An unclosed ' or " now stops the batch with exit code 2 instead of swallowing the rest of the line.
  • continuesOnNextLine counts the run of trailing backslashes (DSConfig.java:1454-1460), so a pa\\ written by --commandFilePath no longer joins the next command, and a continued last line still runs (:1450).
  • build-maven (ubuntu-latest, 17) at d2d2683 ran DSConfigParseTest (27 tests), LDAPUrlTestCase, CommandBuilderTestCase and ArgumentParserPropertiesFileTestCase with 0 failures.

issue (non-blocking): A line of blanks in the middle of a batch file runs as a command with no subcommand.

opendj-config/src/main/java/org/forgerock/opendj/config/dsconfig/DSConfig.java:1441, :1445-1446, :1121-1127

nextBatchCommand skips only line.isEmpty(). " " has no trailing backslash, so it is returned. toCommandArgs gives [] for it, and main(initialArgs) runs with no subcommand. Without -n that opens the interactive menu in the middle of the batch, and under --batch the menu reads the remaining stdin lines as its answers. With -n it fails with ERR_DSCFG_ERROR_MISSING_SUBCOMMAND and the batch stops. BASE did the same, but the new end-of-file branch (:1450, trim().isEmpty() ? null) already treats a blank-only fragment as no command, so the two cases now disagree.

            if (line.trim().isEmpty() || line.startsWith("#")) {

Pin: { "set-x\n \nset-y\n", commands(args("set-x"), args("set-y")) } in DSConfigParseTest.batchFiles.


question (non-blocking): Is it deliberate that a blank or # line inside a continuation is skipped and the command keeps joining?

opendj-config/src/main/java/org/forgerock/opendj/config/dsconfig/DSConfig.java:1441-1448

set-x --foo bar \, then an empty line, then set-y gives one command, [set-x, --foo, bar, set-y], and a # comment line in place of the blank does the same. sh runs two commands here. dsconfig runs one, with set-y as a trailing argument, so it fails and stops the batch. BASE behaved the same, and the javadoc says empty lines and comments are skipped. Only the commit subject ("join batch lines as a POSIX shell does") promises more. If you keep the skip, this is a wording fix to that subject or to the docs. If POSIX joining was the intent, it is a small code change:

            if (line.isEmpty() || line.startsWith("#")) {
                if (command.length() > 0) {
                    // As in sh: a blank line, or a comment after a continuation, ends the command.
                    return command.toString();
                }
                continue;
            }

A related edge case: continuesOnNextLine does not track quotes, so --p 'a\ followed by b' gives ab, whereas sh keeps a\<newline>b inside single quotes. No dsconfig value carries a newline, so this does not matter in practice.


suggestion (non-blocking): No test runs the new handleBatch catch, and the unclosed-quote test checks only the exception class.

opendj-config/src/main/java/org/forgerock/opendj/config/dsconfig/DSConfig.java:1405-1411, opendj-config/src/test/java/org/forgerock/opendj/config/dsconfig/DSConfigParseTest.java:98-101

No test in the tree runs dsconfig with --batch or --batchFilePath. DSConfigParseTest calls nextBatchCommand and toCommandArgs directly. These mutants all survive because no test runs that code: setting exitCode = SUCCESS in the catch (the batch would run on past the rejected line), dropping the errPrintln, throwing with another message key, or putting BASE's command.trim() back into the buildCommandArgs call at :1405. The description's "trim before the split" mutant went red only because it was placed inside nextBatchCommand.

    @Test(dataProvider = "unclosedQuotes", expectedExceptions = ArgumentException.class,
            expectedExceptionsMessageRegExp = "The following batch command has a quote that is not closed: .*")
    public void testUnclosedQuoteIsRejected(String line) throws Exception {
        DSConfig.toCommandArgs(line);
    }

Pin: to cover the exit code, move the body of the handleBatch loop into a package-private int runBatchCommand(List<String> initialArgs, String command) and assert that it returns ReturnCode.ERROR_USER_DATA.get() for --set 'a.


suggestion (non-blocking): No test pins the UNIX apostrophe escape, which this PR makes load-bearing.

opendj-cli/src/main/java/com/forgerock/opendj/cli/CommandBuilder.java:232-233, opendj-cli/src/test/java/com/forgerock/opendj/cli/CommandBuilderTestCase.java:51-53

The batch reader now treats ' as a quote and rejects an unclosed one. A --commandFilePath file can still be replayed through --batchFilePath only because CHARSTOESCAPE contains '\''. The only UNIX-form case uses cn=a\,b "x", which has no apostrophe. A mutant that drops '\'' therefore survives, and description:O'Brien would then be written unescaped and stop the replayed batch.

    @Test
    public void testUnixApostropheIsEscaped() {
        assertThat(CommandBuilder.escapeValue("description:O'Brien", true)).isEqualTo("description:O\\'Brien");
    }

… of administrator-typed DNs in dsconfig batch lines, tools.properties and the SDK LDAPUrl

- dsconfig splits a batch line the way a POSIX shell does: inside double
  quotes a backslash escapes only ", \, $ and `, single quotes are
  supported, quoted and unquoted parts join into one argument, an empty
  quoted word is an empty argument, and a trailing backslash is kept.
- CommandBuilder quotes a Windows value by the C runtime rules: the
  backslashes before a quote or the closing quote are doubled and a quote
  is escaped.
- tools.properties is read as UTF-8, or as ISO-8859-1 when it is not valid
  UTF-8, and a leading byte order mark is skipped. The documentation and
  the template state that a backslash must be doubled.
- LDAPUrl percent-encodes and decodes the octets of the UTF-8 encoding,
  two hex digits each; octets that are not UTF-8 still decode one char
  each.

Fixes OpenIdentityPlatform#1159
…tch line, and join batch lines as a POSIX shell does

Review round 1:
- toCommandArgs throws an ArgumentException (ERR_DSCFG_ERROR_BATCH_UNCLOSED_QUOTE_1197) for an
  unclosed ' or "; handleBatch prints it and stops the batch with ERROR_USER_DATA.
- nextBatchCommand: a line continues only when it ends in an odd number of backslashes, a
  continued last line still runs, and only the echoed command is trimmed.
- tools.properties: the byte order mark is skipped as bytes, so the ISO-8859-1 fallback skips it too.
- LDAPUrlTestCase pins two hex digits for an octet below 0x10.
- Docs: a single quote is now a quote character, an unclosed quote is rejected, an escaped
  trailing backslash does not continue the line, and the Windows equivalent command follows the
  C runtime rules, not cmd.exe.
…, and pin the batch error path and the UNIX apostrophe escape

Review round 2:
- nextBatchCommand skips a line that holds only blanks, as it skips an empty line. Before, such a
  line ran dsconfig with no subcommand and stopped the batch.
- Empty lines and comments stay skipped inside a continued command, as before, so a line of a long
  command can be commented out. The dsconfig description and the admin guide now say so.
- The body of the handleBatch loop is now DSConfig.runBatchCommand, so tests can run the exit code
  and the message of a command that cannot be split, and the untrimmed command passed to main.
- testUnclosedQuoteIsRejected checks the message; CommandBuilderTestCase pins the UNIX escape of
  a single quote.
@vharseko
vharseko force-pushed the issue-1159-admin-typed-dn branch from d2d2683 to 0da9588 Compare October 5, 2026 13:27
@vharseko

vharseko commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

Round 2 is addressed in 0da9588. The branch is rebased on 85b28b3d0f with no conflicts. The description is updated.

Line of blanks: fixed. nextBatchCommand now skips a line whose trimmed form is empty, as it skips an empty line, and your pin is in batchFiles together with a blank line at the end of the file. One correction to the failure path: the interactive menu cannot open there. checkForConflictingArguments rejects --batch and --batchFilePath without --no-prompt, and removeBatchArgs keeps -n in the arguments of every command of the batch. So a line of blanks always ran into ERR_DSCFG_ERROR_MISSING_SUBCOMMAND and stopped the batch, which is still a bug and is now fixed.

Blank or # line inside a continuation: deliberate, so I kept the skip. It is how master behaves, and it is useful: an operator can comment out one line of a long command, such as # --set enabled:false \ between two continued lines. With sh semantics that file would run two broken commands instead. The new docs only promise POSIX rules for how a line is split into arguments, so I fixed the wording: the dsconfig description and the admin guide now say that empty lines, lines of blanks and lines that start with # are skipped, also inside a command that continues over several lines. The PR title, which becomes the squash subject, does not claim POSIX joining. I agree on the quote tracking across a continuation: no change.

Tests for the batch error path: done. The body of the handleBatch loop is now static int runBatchCommand(List<String> initialArgs, String command, PrintStream out, PrintStream err). It is static because a DSConfig instance without parsed global arguments cannot print: errPrintln asks isInteractive(), which reads --no-prompt. A command that cannot be split is printed to err, which is where errPrintln wrote it, since a batch always runs with --no-prompt. The echo stays in handleBatch. New tests:

  • testUnclosedQuoteIsRejected checks the message with expectedExceptionsMessageRegExp, as you suggested.
  • testBatchCommandWithUnclosedQuoteFails: set-x --set 'a returns exit code 2 and prints the message.
  • testBatchCommandKeepsEscapedTrailingBlank: set-x\ reaches dsconfig as the argument set-x with its trailing blank, checked in the "The provided argument ... is not recognized" error. No server is needed.

Each of your four mutants now turns only its own test red: returning success from the catch, dropping the message, another message key, and the trim() put back where the command is passed to dsconfig. You were right that the description's "trim before the split" mutant had been placed inside nextBatchCommand. The description now says so.

UNIX apostrophe escape: added your testUnixApostropheIsEscaped. It is red when '\'' is dropped from CHARSTOESCAPE.

Seven mutants, one per change above plus a comment that ends a continued command, each turned only their own new tests red. The full unit suites pass: opendj-core 8190, opendj-cli 66, and opendj-config 580 tests, with javadoc on all three. The two changed doc pages were rendered with AsciidoctorJ.

@vharseko
vharseko requested a review from maximthomas October 5, 2026 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug docs java Changes to Java sources tests Test suites: fixing, enabling, un-disabling Windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DNs typed by administrators lose escapes or non-ASCII characters in dsconfig batch files, tools.properties and the SDK LDAPUrl

2 participants