Fix type-mismatch, index-out-of-bounds and boxed-equality CodeQL alerts

[vharseko]

Fixes five CodeQL alerts that are genuine defects: java/index-out-of-bounds (#700), java/type-mismatch-access (#684, #685), java/type-mismatch-modification (#696) and java/reference-equality-of-boxed-types (#559).

1. BCrypt.char64() indexes one past the end of the table

privatestaticbytechar64(charx) {
if (x < 0 || x > index_64.length) { // should be >=return -1;
}
returnindex_64[x];
}

index_64 has exactly 128 entries, so U+0080 passes the guard and index_64[128] throws.

This is reachable from the public API. BcryptPasswordStorageScheme.passwordMatches() calls BCrypt.checkpw(plaintext, storedPassword), which calls hashpw(), where the salt region is taken from the stored value with no character validation at all:

real_salt = salt.substring(off + 3, off + 25);
...
saltb = decode_base64(real_salt, BCRYPT_SALT_LEN); // -> char64() per character

A malformed or hostile {BCRYPT}userPassword value therefore makes a bind throw ArrayIndexOutOfBoundsException instead of failing cleanly.

Verified against a real build, calling the public API with a stored hash whose salt contains U+0080:

Build	BCrypt.checkpw("secret", "$2a$10$" + U+0080 + ...)
master	java.lang.ArrayIndexOutOfBoundsException: Index 128 out of bounds for length 128
this branch	java.lang.IllegalArgumentException: Bad salt length — the intended error path

char64 was also exercised by reflection over the whole 0..0x10FF range: it now returns -1 for every value above 127 and never throws, while the decoding table itself is unchanged (char64('A') == 2).

2. EntryCacheConfigManager looks up a Map<DN, Integer> with a ByteString

The map is declared as Map<DN, Integer> cacheNameToLevelMap and populated with configuration.dn() in loadAndInstallEntryCache(). applyConfigurationChange() reads it correctly with a DN, but three call sites used configuration.dn().toNormalizedByteString():

• isConfigurationChangeAcceptable() — containsKey() always returned false, so the check that rejects a second entry cache at an already used cache level never fired and the conflicting configuration was accepted silently.
• applyConfigurationDelete() — remove() never removed anything, so the entry stayed in the map after the cache was deleted and a later cache created under the same DN saw a stale cache level.

Both now use the DN directly. The ByteString import is no longer needed.

3. GenerateMessageFileMojo compares boxed Integer ordinals with ==

MessagePropertyKey instances are used as keys of a TreeMap, so compareTo() defines key identity. Two problems:

• ordinal == k.ordinal is a reference comparison. For ordinals above the Integer cache range, two keys with the same ordinal fell through to ordinal.compareTo(k.ordinal), which returns 0, so the TreeMap treated them as one key and the second put() overwrote the first. That also defeats the explicit duplicate-ordinal check in getCategoryMap(), which is supposed to fail the build with "The ordinal value X has been previously defined" — instead a message silently disappeared from the generated documentation.
• The final return 0 made a key with an ordinal compare equal to a key without one, dropping entries the same way.

compareTo() is now a total order: Objects.equals() for the ordinals, keys without an ordinal sort first, ties broken by description.

4. protocol.properties defines ordinal 1508 twice

The duplicate-ordinal check restored by (3) immediately fails the build, because the data it
guards has been broken since 2016:

• protocol.properties:788 — ERR_CANNOT_DECODE_CONTROL_VALUE_1508
• protocol.properties:863 — ERR_HTTP_ERROR_WHILE_PROCESSING_REQUEST_1508 (added in OPENDJ-3031)

The collapsing compareTo() hid it: the two keys landed in one TreeMap entry, so
getCategoryMap() only ever saw one ordinal 1508. The corruption is visible in the checked-in
opendj-doc-generated-ref/src/main/asciidoc/reference/appendix-log-messages.adoc:11914, which
documents ID 1508 under the name ERR_HTTP_ERROR_WHILE_PROCESSING_REQUEST_1508 but with the
message text of ERR_CANNOT_DECODE_CONTROL_VALUE, while the latter appears nowhere in the
reference.

ERR_HTTP_ERROR_WHILE_PROCESSING_REQUEST has no Java reference and no locale variants, and 1538
is the highest ordinal in the file, so it is renumbered to 1539; 1508 goes back to
ERR_CANNOT_DECODE_CONTROL_VALUE, which is used by ExternalChangelogRequestControl and exists
in all six translations. It is the only duplicate ERR_ ordinal in the repository.

5. MessageRefEntry.compareTo() has the same defect

MessageRefEntry is held in a TreeSet, and its compareTo() returned 0 whenever either
ordinal was null, so a null-ordinal entry compared equal to everything. With (3) sorting
null-ordinal keys first, the first entry inserted becomes the root of the tree and every
subsequent add() is discarded as a duplicate — tool would drop from 480 documented messages
to 1. Fixed the same way, with the already unique xmlId breaking ties.

Testing

mvn -pl opendj-doc-maven-plugin compile, and
mvn -pl opendj-server-legacy verify -P precommit -Dit.test=BCryptTest — Tests run: 77, Failures: 0, Errors: 0. (Note that mvn test runs nothing in opendj-server-legacy: surefire is
bound to phase none at opendj-server-legacy/pom.xml:641-651 and the tests run under failsafe in
the precommit profile.)

The log reference generation was driven over the real message files for all 19 categories
configured in opendj-doc-generated-ref/pom.xml, using the compiled MessagePropertyKey and
MessageRefEntry classes:

category	master	this branch
protocol	248 documented	249
tool	480 documented	499
admin_tool	1 documented	260
quickSetup	1 documented	14
other 15	—	unchanged

BCrypt.char64() was compared against master by reflection over the whole char range: exactly
one code unit behaves differently — U+0080, ArrayIndexOutOfBoundsException becomes -1 — and the
decoding table itself is unchanged (char64('A') == 2). The new BCryptTest case fails on
master with ArrayIndexOutOfBoundsException: Index 128 out of bounds for length 128 and passes
here with the intended IllegalArgumentException: Bad salt length.

Note that opendj-doc-generated-ref is built by the ordinary CI job on Linux — opendj-packages
is in the default module list (pom.xml:298) and its distribution-unix profile adds the doc
module — which is where the ordinal 1508 failure showed up.

[maximthomas]

All three defects are real, and the BCrypt and EntryCacheConfigManager fixes are correct as-is. The comparator fix in GenerateMessageFileMojo is also correct in isolation, but the mojo's downstream code depended on the broken ordering — as written it breaks the doc build in two ways.

I simulated both comparators against the real message files (opendj-server-legacy/src/messages/org/opends/messages/*.properties, copied unfiltered into opendj.jar per opendj-server-legacy/pom.xml:516) over the 19 categories configured in opendj-doc-generated-ref/pom.xml:136-154:

category	master	this PR
tool	480 messages documented	1 documented
protocol	248 documented	BUILD FAILURE
others	unchanged	unchanged

protocol log reference now fails the build (blocking)

opendj-server-legacy/src/messages/org/opends/messages/protocol.properties has a genuine duplicate ordinal:

• protocol.properties:788 — ERR_CANNOT_DECODE_CONTROL_VALUE_1508
• protocol.properties:863 — ERR_HTTP_ERROR_WHILE_PROCESSING_REQUEST_1508

On master the buggy comparator collapsed these into one TreeMap entry, so the duplicate check in getCategoryMap() never fired. With the fix both survive and the mojo throws MojoExecutionException: The ordinal value '1508' ... has been previously defined in protocol.

The PR restores the check without fixing the data it now rejects. ERR_HTTP_ERROR_WHILE_PROCESSING_REQUEST has zero Java references and exists only in the base properties file (no locale variants), and 1538 is the highest ordinal there — so renumbering it to 1539 (or deleting it) is a one-line fix.

tool category collapses from 480 documented messages to 1 (blocking)

MessageRefEntry.compareTo() in opendj-doc-maven-plugin/src/main/java/org/forgerock/opendj/maven/doc/GenerateMessageFileMojo.java:180-186 has the same return 0 defect the PR just fixed in MessagePropertyKey, and is left untouched:

if (this.ordinal != null && mre.ordinal != null) {
returnthis.ordinal.compareTo(mre.ordinal);
}
return0; // any null-ordinal entry compares "equal" to everything

getCategoryMap() feeds keys into TreeSet<MessageRefEntry> in errorMessages.keySet() order. The new comparator sorts null-ordinal keys first, so the first entry inserted becomes the root with a null ordinal, and every subsequent add() compares 0 against it and is discarded as a duplicate. tool.properties has 19 ERR_ keys with no ordinal (ERR_LDAP_CONN_BAD_HOST_NAME, ERR_FAILED_TO_CONNECT, ...); on master they were swallowed inside the TreeMap, so the TreeSet root had a real ordinal and 480 entries survived.

Fix MessageRefEntry.compareTo() in the same PR — the class already has a unique xmlId to break ties, and MESSAGE_NO_ORDINAL (used in toMap(), line 167) shows null-ordinal entries are meant to be documented:

@OverridepublicintcompareTo(MessageRefEntrymre) {
if (!Objects.equals(ordinal, mre.ordinal)) {
if (ordinal == null) {
return -1;
}
if (mre.ordinal == null) {
return1;
}
returnordinal.compareTo(mre.ordinal);
}
returnxmlId.compareTo(mre.xmlId);
}

That turns the regression into a real improvement — it also fixes the pre-existing collapses in admin_tool and quickSetup:

category	master	PR as-is	PR + this fix
tool	480	1	499
admin_tool	1	1	260
quickSetup	1	1	14

(Sorting null ordinals last in MessagePropertyKey instead would restore tool to 480 but leaves the other two broken — I'd take the fuller fix.)

Stated testing does not cover the mojo change (blocking)

mvn -pl opendj-doc-maven-plugin,opendj-server-legacy compile never runs generate-xml-messages-doc. That goal is bound to prepare-package in opendj-doc-generated-ref (opendj-doc-generated-ref/pom.xml:124-128), a module built only under the packages profile on Linux (opendj-packages/pom.xml:54). CI runs mvn verify -P precommit (.github/workflows/build.yml:97), which never builds it — so neither CI nor the stated testing would catch either issue above; they surface first in an RPM/DEB/release build.

Please validate with:

mvn -Ppackages -pl opendj-doc-generated-ref -am package

and diff the generated target/asciidoc/source/partials/log-message-reference.adoc.

Missing regression test for BCrypt.char64() (minor)

The fix is correct and the reachability analysis holds: BcryptPasswordStorageScheme.passwordMatches() (opendj-server-legacy/src/main/java/org/opends/server/extensions/BcryptPasswordStorageScheme.java:118) catches only IllegalArgumentException, so pre-fix the ArrayIndexOutOfBoundsException escaped into bind processing; post-fix crypt_raw()'s length check (BCrypt.java:635) throws the catchable IllegalArgumentException.

opendj-server-legacy/src/test/java/org/opends/server/extensions/BCryptTest.java already exists — nothing currently stops >= from regressing to >:

@Test(expectedExceptions = IllegalArgumentException.class)
publicvoidcheckPwRejectsNonAsciiSalt() {
BCrypt.checkpw("secret", "$2a$10$�aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
}

Nits

• EntryCacheConfigManager behaviour change: isConfigurationChangeAcceptable() previously always returned true for the level-collision case; it now rejects a change that moves a cache onto an occupied level. Correct, but admin-visible on upgrade — worth a release note. (DN.equals()/hashCode() both delegate to toNormalizedByteString() at opendj-core/src/main/java/org/forgerock/opendj/ldap/DN.java:450-467, so the new DN keys are semantically identical and cheaper — hashCode is memoised.)
• Dead check in char64(): x < 0 is unreachable since char is unsigned; if (x >= index_64.length) alone would be clearer while you're on the line.

[vharseko]

Thanks — both blocking findings reproduce, fixed in 9e1ee83.

I re-ran the check independently, driving the compiledMessagePropertyKey / MessageRefEntry
classes over all 19 categories rather than a hand-copy of the comparators, and got your table to
the digit:

category	master	PR as it was	now
protocol	248	BUILD FAILURE	249
tool	480	1	499
admin_tool	1	1	260
quickSetup	1	1	14
other 15	—	unchanged	unchanged

1. Ordinal 1508

Renumbered ERR_HTTP_ERROR_WHILE_PROCESSING_REQUEST to 1539 rather than deleting it, so 1508 goes
back to ERR_CANNOT_DECODE_CONTROL_VALUE — the one that is actually used
(ExternalChangelogRequestControl.java:74) and exists in all six translations. A scan of every
base *.properties in the repo confirms it was the only duplicate ERR_ ordinal.

The damage is visible in the checked-in reference,
opendj-doc-generated-ref/src/main/asciidoc/reference/appendix-log-messages.adoc:11914: ID 1508 is
documented under the name ERR_HTTP_ERROR_WHILE_PROCESSING_REQUEST_1508 but carries the message
text of ERR_CANNOT_DECODE_CONTROL_VALUE, and the latter appears nowhere in the file. That file
was generated by the old comparator (its ADMIN_TOOL section holds a single entry), so
regenerating it is worth a follow-up — it needs a Linux packages build, so I left it out of this
PR.

2. MessageRefEntry.compareTo()

Applied essentially your patch. xmlId is a fine tie-breaker: it is MessagePropertyKey.toString()
of a TreeMap key, and message keys contain no characters that getXmlId() would rewrite.

3. Testing

Half right, and the half that matters is fixed: mvn -pl opendj-doc-maven-plugin,opendj-server-legacy compile indeed never runs generate-xml-messages-doc, and the Testing section is rewritten.

But CI does build opendj-doc-generated-ref. opendj-packages is in the default module list
(pom.xml:298), not gated behind -Ppackages, and its distribution-unix profile
(opendj-packages/pom.xml:54) adds the doc module on Linux. All five build-maven (ubuntu-latest, *)
jobs on this PR failed on ordinal 1508 with OpenDJ Doc Generated References ... FAILURE in the
reactor summary, after OpenDJ Server had passed — that is how the duplicate surfaced. macOS and
Windows were green because distribution-mac / distribution-windows do not include the module,
which also means -pl opendj-doc-generated-ref cannot be used to validate on macOS.

4. BCryptTest

Added, but not with the proposed input — it does not guard the fix. ^ and @ are plain ASCII,
char64() returns -1 for them on master too, so that stored value fails identically on both
sides:

input	master	this branch
"$2a$10$^@aaaa…"	IllegalArgumentException: Bad salt length	IllegalArgumentException: Bad salt length
"$2a$10$" + (char) 0x80 + "aaa…"	ArrayIndexOutOfBoundsException: Index 128 out of bounds for length 128	IllegalArgumentException: Bad salt length

The test uses U+0080, the first character past the end of the 128-entry table.
mvn -pl opendj-server-legacy verify -P precommit -Dit.test=BCryptTest → Tests run: 77, Failures: 0, Errors: 0. (Worth noting for anyone else validating: mvn test runs nothing in that
module — surefire is bound to phase none at opendj-server-legacy/pom.xml:641-651 and everything
runs under failsafe in the precommit profile.)

5. Nits

x < 0 dropped from char64(). To make sure that stayed behaviour-preserving I compared the
method against both baselines by reflection over the whole char range: 0 differing code units vs
the previous commit of this branch, and exactly one vs master — U+0080, where
ArrayIndexOutOfBoundsException becomes -1. The decoding table is untouched.

Agreed on the EntryCacheConfigManager release note; the level-collision check now actually
rejects, which is admin-visible on upgrade.

One more, found while checking those numbers

admin_tool reaches 260 rather than its 316 ERR_ keys because parseString() truncates the
description at the last _ even when there is no ordinal:

finalStringdescription = key.substring(0, li).toUpperCase(); // ERR_CTRL_PANEL_INVALID_DAY -> ERR_CTRL_PANEL_INVALID

ERR_CTRL_PANEL_INVALID_{DAY,ENTRY,HOUR,MINUTE,TIME} all collapse onto ERR_CTRL_PANEL_INVALID,
so four of the five vanish and the survivor is documented under a truncated name. admin_tool
loses 56 of 316 that way and quickSetup 3 of 17 — same class of defect, present on master too,
and a one-line fix (keep the whole key when there is no ordinal). Fold it into this PR, or open a
separate issue?