Conversation
maximthomas
left a comment
There was a problem hiding this comment.
praise: The fixes keep old inputs readable while switching to UTF-8.
ArgumentParser.loadPropertiesFileandLDAPUrl.decodeOctetsdecode with a strict UTF-8 decoder and fall back to ISO-8859-1, sotools.propertiesfiles and URLs written for earlier releases still read as before.- The UNIX branch of
CommandBuilder.escapeValueis unchanged and still escapes'(CHARSTOESCAPE), so UNIX--commandFilePathfiles are not affected by the new single-quote rule. LDAPUrl.percentEncoderwalks code points, so a char outside the BMP is encoded as its four UTF-8 octets (%f0%9f%98%80, pinned bytestPercentEncodingIsUtf8).
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.decodeOctetsfalls back to ISO-8859-1 for the whole contiguous%XXrun. Valid UTF-8 octets next to an invalid one also decode one char each (%C3%B6%F6givesöö).
8d48f40 to
d2d2683
Compare
|
Round 1 is addressed in d2d2683. The branch is rebased on Unterminated single quote: fixed. An unclosed Trailing escaped blank: fixed. Only the echoed command is trimmed now. The split gets the untrimmed command, so 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 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 Octet below 0x10: added PR text:
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. |
maximthomas
left a comment
There was a problem hiding this comment.
praise: Round 1 is fixed where the bugs are, and CI ran the new cases green at this head.
toCommandArgsthrows a checkedArgumentException(ERR_DSCFG_ERROR_BATCH_UNCLOSED_QUOTE), andhandleBatchcatches 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.continuesOnNextLinecounts the run of trailing backslashes (DSConfig.java:1454-1460), so apa\\written by--commandFilePathno longer joins the next command, and a continued last line still runs (:1450).build-maven (ubuntu-latest, 17)at d2d2683 ranDSConfigParseTest(27 tests),LDAPUrlTestCase,CommandBuilderTestCaseandArgumentParserPropertiesFileTestCasewith 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.
d2d2683 to
0da9588
Compare
|
Round 2 is addressed in 0da9588. The branch is rebased on Line of blanks: fixed. Blank or Tests for the batch error path: done. The body of the
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 UNIX apostrophe escape: added your 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. |
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.
dsconfigbatch lines (DSConfig.toCommandArgs)A batch line is now split into arguments the way a POSIX shell splits it, without expanding variables:
",\,$and`. Before any other char it is kept, so--set base-dn:"cn=a\,b,dc=x"now givescn=a\,b,dc=xinstead ofcn=a,b,dc=x.'base-dn:o=My Company'is one argument."a"bgivesab; before, it gave two arguments), and an empty quoted word ("") is an empty argument.StringIndexOutOfBoundsException.dsconfigprintsThe 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
handleBatchjoins lines (DSConfig.nextBatchCommand):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.--commandFilePathwrites such lines on UNIX, because it escapes a backslash in a value that ends a command.description:value\at the end of a line gavedescription:value\instead ofdescription:value.dsconfigwith no subcommand, which failed withERR_DSCFG_ERROR_MISSING_SUBCOMMANDand stopped the batch.#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
--commandFilePathwrites 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'Brienthat worked before is now rejected. WriteO\'Brienor"O'Brien".2. Windows quoting in
CommandBuilder.escapeValueThis 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 forcmd: 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.exeinterprets itself (&,|,<,>) are not caret-escaped:cmd.exetoggles its quote state at every", including an escaped one, so after\"they are unquoted for it, as on master. Thedsconfigdescription and the admin guide now say so.3.
tools.properties(ArgumentParser, both readers)\uXXXXescapes, line continuations, and values already written with doubled backslashes. The templateconfig/tools.properties, theFilessection of the tool man pages, the developer guide, and the production password note now say that a backslash must be doubled.4. SDK
LDAPUrl%XXoctets is decoded as UTF-8 (RFC 4516 section 2.1), soldap:///cn=J%C3%B6rg,dc=xgivescn=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%F6givesöö.Жbecame%416, and a char below U+0010 got a single hex digit.Documentation
man-pages/_description-dsconfig.adocandadmin-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, notcmd.exe.man-pages/_files.adoc,server-dev-guide/chap-ldap-operations.adoc, andadmin-guide/chap-production.adoc: the encoding oftools.propertiesand 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 commandsnextBatchCommandreads 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 throughDSConfig.runBatchCommand, the body of thehandleBatchloop: a line with an unclosed quote gives exit code 2 and the message, and an escaped trailing blank reachesdsconfiguntrimmed.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--commandFilePathfile be replayed as a batch.ArgumentParserPropertiesFileTestCase: UTF-8, ISO-8859-1 fallback, byte order mark (also in the ISO-8859-1 fallback), and doubled backslash, each through--propertiesFilePathand through the explicit-pathparseArguments.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 inDSConfigParseTest). Each of four mutants turned exactly its own tests red: no doubling of a trailing backslash, no ISO-8859-1 fallback inLDAPUrl, the same inArgumentParser, 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 innextBatchCommand, 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 wherehandleBatchpasses the command todsconfig, 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
LDAPUrlconstructor writes the filter intotoString()without percent-encoding it ((cn=J\C3\B6rg)), whileequals()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 bebasedn, notbaseDN). The--propertiesFilePathreader lowercases them. Nothing in the repository calls the former.