mirror of https://github.com/OpenIdentityPlatform/OpenDJ.git

Valery Kharseko
22 hours ago 458f009de6ccb989ac565da62ac407f20755303e
refs
author Valery Kharseko <vharseko@3a-systems.ru>
Sunday, October 4, 2026 09:23 +0200
committer GitHub <noreply@github.com>
Sunday, October 4, 2026 09:23 +0200
commit458f009de6ccb989ac565da62ac407f20755303e
tree 06d7183cdc64481f5f473de4d2bf166169d5af31 tree | zip | gz
parent 5e8c08fb1ac4a2cdaa5d11ce1e852dab573ef86a view | diff
[#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
2 files modified
1 files added
178 ■■■■■ changed files
opendj-ldap-toolkit/src/main/java/com/forgerock/opendj/ldap/tools/Utils.java 32 ●●●●● diff | view | raw | blame | history
opendj-ldap-toolkit/src/test/java/com/forgerock/opendj/ldap/tools/ControlArgumentTestCase.java 134 ●●●●● diff | view | raw | blame | history
opendj-ldap-toolkit/src/test/java/com/forgerock/opendj/ldap/tools/LDAPSearchTestCase.java 12 ●●●●● diff | view | raw | blame | history