Skip to content

[#809] Fix DSML gateway NPE on abandonRequest and on missing Content-Type - #811

Open
vharseko wants to merge 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:issues/809-dsml-servlet-npe
Open

[#809] Fix DSML gateway NPE on abandonRequest and on missing Content-Type#811
vharseko wants to merge 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:issues/809-dsml-servlet-npe

Conversation

@vharseko

@vharseko vharseko commented Jul 31, 2026

Copy link
Copy Markdown
Member

Fixes #809

1. abandonRequest caused an NPE and leaked the LDAP connection

performLDAPRequest() deliberately returns null for an abandon request (no response element is defined for it in DSMLv2), but doPost() guarded the null only when adding to the response list and then dereferenced it unconditionally, so a batch containing <abandonRequest abandonID="1"/> ended in a NullPointerException escaping to the container as a 500.

Since the connection block was not wrapped in try/finally, connection.close(nextMessageID) was skipped as well: org.opends.server.tools.LDAPConnection closes its socket only in close(), so one connection to the directory server was leaked per request — trivially repeatable until the connection handler is exhausted.

The loop now skips the null result, and the connection is closed in a finally. It is no longer reused either, which fixes a second latent defect: a second batchRequest in the same SOAP body used to be silently skipped, because the leftover connection left connected == false. Now that it really runs, two more things had to follow:

  • the LDAPConnectionOptions are built once per doPost and shared by every batchRequest of the SOAP body, so with ldap.authzidtypeisid=true the authzid of one of them used to reach the bind of the next. It fails in two ways: addSASLProperty() appends to the values of a key, so two <authRequest> in a row produced a multi-valued property, which SASL PLAIN rejects client-side (The "authzid" SASL property only accepts a single value); and a batchRequest carrying no <authRequest> inherited the authorization identity of the previous one, running its operations under an identity the request never asked for. The authzid is now dropped at the top of every iteration, before the authRequest is looked at;
  • the connection is a loop local, so the connection == null check that guarded the reuse is gone.

2. NPE when the request has no Content-Type header

messageFactory was assigned only inside the header loop, when a Content-Type header matching SOAP 1.1 or SOAP 1.2 was present. A POST without that header is legal HTTP and left the field at null, which was then dereferenced when parsing the request (NPE → 500) and again on the response path, where it was swallowed by catch (Exception e) { e.printStackTrace(); } — a client whose credentials were rejected got an empty HTTP 200 instead of the error.

A missing or unsupported Content-Type is now answered with a malformedRequest batch response (SOAP 1.1 is used for the reply), which also replaces the ServletException previously thrown for a non-SOAP content type. It is built by createXMLParsingErrorResponse(), like the other two malformed paths, so the requestID is recovered from the buffered stream and the client can correlate the reply.

A malformed Authorization header no longer breaks out of the header loop either: the Content-Type may still be ahead, and it decides which SOAP version the error is reported with.

Failures to send the response are reported to the container log. java.util.logging is not usable here: LDAPConnection.connectToHost() calls JDKLogging.disableLogging() on every non-verbose connection, which does LogManager.reset() and sets the root level to OFF, so after the first LDAP connection in the JVM the record would be dropped; and the war ships slf4j-api without any SLF4JServiceProvider. init(ServletConfig) now calls super.init(config), without which getServletContext() would throw an NPE.

The four Logger.getLogger(PKG_NAME) calls of safeSetFeature() and createSafeDocument() were dead for exactly that reason, so they go to the container log as well and the servlet has a single logging sink; the rationale sits in the class javadoc.

Both defects predate the Open Identity Platform fork (imported in 1577c0a).

3. Prerequisite: LDAPConnection never connected its socket

Found while writing the regression test: since #279, LDAPConnection.createSocket() binds the newly created client socket to the target server address instead of connecting to it, so every plain or StartTLS connection made through org.opends.server.tools.LDAPConnection fails — with Address already in use or Cannot assign requested address where the bind is refused, and with Socket is not connected where setReuseAddress(true) lets the bind through and the unconnected socket is handed to LDAPWriter. Neither BindException nor SocketException is a ConnectException, so the failure also escaped the per-address catch into the outer catch (Exception ex), losing the result code (-1) on the way; the fix restores both the result code and the failover across the addresses of a host.

Only the callers that reach createSocket() are affected, i.e. plain LDAP or StartTLS without an SSLConnectionFactory: the DSML gateway (the default web.xml has ldap.usessl=false) and the LDAPConnectionArgumentParser tools (import-ldif, export-ldif, backup, restore, rebuild-index, manage-tasks) used without --useSSL. stop-ds and manage-account install an SSLConnectionFactory unconditionally and CryptoManagerImpl sets setUseSSL(true), so all three go through createSSLSocket() and are unaffected; ldapsearch/ldapmodify go through the SDK, which is why this went unnoticed.

It is kept as a separate commit for review, but it cannot be tracked as its own pull request: the DSML tests below need it, and with the default ldap.usessl=false the gateway has been unable to connect at all since #279 (every release from 4.5.5 to 5.1.2), which is exactly what masked the abandon NPE. Fixing the socket alone would re-enable the gateway straight into the crash.

Tests

DSMLServletTestCase drives doPost() against a fake LDAP endpoint (a real socket that answers the binds and records both the message types it receives and the authorization identity of every SASL bind), with the servlet API stubbed through java.lang.reflect.Proxy. Mockito and AssertJ are on the test classpath of every module through the root pom, the hand-written stubs are simply a closer fit for these interfaces:

  • an abandon request is forwarded and the connection is closed (the endpoint sees bind → abandon → unbind);
  • the same over SOAP 1.2, whose reply keeps the SOAP 1.2 envelope;
  • every batchRequest of a SOAP body gets its own connection (bind → abandon → unbind, twice);
  • the authzid of a batchRequest does not survive into the bind of the next one: two <authRequest> in a row bind as dn:cn=first then dn:cn=second, and a batchRequest without an <authRequest> binds with no authzid at all;
  • a request without Content-Type is answered with malformedRequest carrying the requestID, and no connection is opened;
  • an unsupported Content-Type likewise;
  • a credentials error is still reported when Content-Type is missing;
  • a malformed Authorization header does not downgrade the reply to SOAP 1.1.

LDAPConnectionTestCase pins #279 in the module that owns the code: a plain connection to the test server succeeds, and a closed port is reported as CLIENT_SIDE_CONNECT_ERROR instead of escaping the per-address catch without a result code. Both fail when connect() is turned back into bind().

mvn -pl opendj-dsml-servlet testTests run: 69, Failures: 0, Errors: 0, and mvn -pl opendj-server-legacy verify -Pprecommit -Dit.test=LDAPConnectionTestCaseTests run: 2, Failures: 0, Errors: 0. Note that the DSML command needs opendj-server-legacy to have been installed from this branch first, since those tests exercise createSocket(); the full reactor build used by CI is unaffected.

Follow-ups

Two defects of the gateway are deliberately left out, both raised in review and both only observable now that the second and later batchRequest of a SOAP body really run:

@vharseko vharseko added bug java Pull requests that update java code tests Test suites: fixing, enabling, un-disabling labels Jul 31, 2026
@vharseko
vharseko requested a review from maximthomas July 31, 2026 17:12
@maximthomas
maximthomas self-requested a review July 31, 2026 17:36

@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.

Both defects are real and the fixes are correct. I reproduced everything locally: with DSMLServlet.java reverted to master a single <abandonRequest/> throws NullPointerException out of doPost and the fake LDAP endpoint sees bind, abandon with no unbind — the leak, exactly as described. With the patch it sees bind, abandon, unbind.

Worth adding to the description: the socket regression only affects callers that reach createSocket(), i.e. plain LDAP or StartTLS. StopDS and ManageAccount install an SSLConnectionFactory unconditionally and CryptoManagerImpl sets setUseSSL(true), so those take createSSLSocket() and are unaffected. Actually affected: the DSML gateway (default web.xml has ldap.usessl=false) and LDAPConnectionArgumentParser tools (import-ldif, export-ldif, backup, restore, rebuild-index, manage-tasks) without --useSSL. Also note BindException is not a ConnectException, so the broken code escaped the per-address catch into the outer catch (Exception ex) — the fix restores failover across multiple A records.

Four things to address before merge.

The new log call is silently dropped (medium)

opendj-dsml-servlet/src/main/java/org/opends/dsml/protocol/DSMLServlet.java

} catch (Exception e) {
  // the client gets an empty response: at least make the cause visible
  Logger.getLogger(PKG_NAME).log(Level.SEVERE, "Unable to send the DSML response", e);
}

LDAPConnection.connectToHost() calls JDKLogging.disableLogging() on every non-verbose connection, which does LogManager.getLogManager().reset(); Logger.getLogger("").setLevel(Level.OFF). After the first LDAP connection attempt in the JVM this record has no handlers and an OFF root level and is never emitted — I confirmed isLoggable(SEVERE)=false, rootLevel=OFF, rootHandlers=0. printStackTrace() at least reached catalina.out, so this is a net loss.

SLF4J is not an alternative here: the war ships slf4j-api plus the i18n bridge but no SLF4JServiceProvider, so it would be a NOP. The working option is the container log — but init(ServletConfig) never calls super.init(config), so getServletContext() NPEs today:

public void init(ServletConfig config) throws ServletException {
  super.init(config);   // currently missing
  ...
}
...
} catch (Exception e) {
  getServletContext().log("Unable to send the DSML response", e);
}

authzid accumulates across batch requests (medium)

connection = null in the finally is right, and it also fixes a second latent bug: previously a 2nd <batchRequest> in the same SOAP body was silently skipped (connection != null left connected == false). I verified this — two empty batch requests open one connection on master and two on this branch.

That makes the following reachable, because connOptions is built once per doPost and addSASLProperty appends to a per-key list:

connOptions.addSASLProperty("authzid=" + batchRequest.authRequest.getPrincipal());

With ldap.authzidtypeisid=true and two batch requests each carrying an <authRequest>, the second bind fails client-side. Reproduced:

<ns0:errorResponse type="couldNotConnect">
  <ns0:message>org.opends.server.tools.LDAPConnectionException:
    The "authzid" SASL property only accepts a single value</ns0:message>
</ns0:errorResponse>

Build LDAPConnectionOptions per batch request, or clear authzid before setting it.

The two commits are not independently mergeable (medium)

testAbandonRequestIsProcessedAndConnectionIsClosed cannot pass without a8616b08a8 — against a ~/.m2 copy of opendj-server-legacy built from master it fails with couldNotConnect: Address already in use (Bind failed). So the socket fix cannot be split off into its own issue, and mvn -pl opendj-dsml-servlet test only reports 64 passing tests when opendj-server-legacy has already been rebuilt from this branch. CI's full-reactor mvn verify is fine; the command in the description is misleading.

The ordering also matters the other way round: with the default ldap.usessl=false the gateway has been unable to connect at all since #279 (every release 4.5.5 through 5.1.2), which is what masked the abandon NPE. Fixing the socket without fixing the NPE would re-enable the gateway straight into the crash — so both belong in this PR.

malformedRequest response drops the requestID (low)

createXMLParsingErrorResponse() deliberately re-parses the buffered stream with SAX to recover the request ID and calls batchResponse.setRequestID(...). The new block builds a bare ErrorResponse, so clients cannot correlate the reply:

ErrorResponse errorResponse = objFactory.createErrorResponse();
errorResponse.setType(MALFORMED_REQUEST);
errorResponse.setMessage("Content-Type does not match SOAP 1.1 or SOAP 1.2");
batchResponses.add(objFactory.createBatchResponseErrorResponse(errorResponse));

Nothing has been read from is at that point and the mark is still valid, so createXMLParsingErrorResponse(is, objFactory, batchResponse, "Content-Type does not match SOAP 1.1 or SOAP 1.2") would keep the two malformed paths consistent.

Nits

  • Dead null check: with the finally nulling it, connection is always null at if ( connection == null ), and the method-scope declaration is no longer needed — a loop-local LDAPConnection connection = new LDAPConnection(...) inside the try reads better. Given #790/#793 this will probably be flagged by CodeQL.
  • Mockito is already available: org.mockito:mockito-all:1.10.19:test and assertj-core come from the root pom's top-level <dependencies>, so they are already on opendj-dsml-servlet's test classpath — the hand-rolled Proxy stubs are a fair choice, but "no new test dependency" isn't a constraint. Worth a word in the description to pre-empt the question.
  • Silent failures in the fake server: FakeLdapServer.serve() ends with e.printStackTrace(), so a server-side error prints and the test still passes. Record the exception and assert it is null in awaitDisconnect().
  • defaultValue(Method) primitive gap: returns null for char/byte/short/float/double returns, which would NPE out of the proxy. No call site hits it today; returnType.isPrimitive() catch-all removes the trap.
  • Reply SOAP version: a malformed Authorization header breaks out of the header loop, so Content-Type may never be examined and a SOAP 1.2 request gets a SOAP 1.1 reply. continue instead of break avoids it.
  • HTTP status: an unsupported Content-Type now yields 200 + malformedRequest instead of the previous ServletException → 500. Better, and consistent with how XML parse errors are already reported, but 415 alongside the body would be more correct HTTP — and deployments alerting on 5xx will see this class of request vanish.
  • setReuseAddress(true): harmless, and it now applies to the implicit local bind that connect() performs — the flag survives the connect. Pre-#279 (new Socket(addr, port)) did not set it; either way not a defect. connect() still has no timeout, so connectToHost's timeout parameter never reaches the socket connect — pre-existing, but a candidate follow-up.
  • Shared BatchResponse: all batch requests in one SOAP body share a single <batchResponse> whose requestID is overwritten each iteration, so two batches produce one <batchResponse requestID="2"> holding both batches' elements. Pre-existing, but only observable now that the second batch actually runs.
  • Coverage gaps: no test for the multi-batchRequest reconnect, for the SOAP 1.2 path, or for an abandon mid-batch letting later operations run; and createSocket() has no test in the module that owns it — a few lines binding a ServerSocket and asserting connectToHost succeeds would pin the #279 regression in opendj-server-legacy.
  • Why this went unnoticed: nothing in the repo referenced DSMLServlet from a test before this PR, and although the module wires Cargo/Tomcat into pre-integration-test it has no IT classes — the container starts, deploys and stops without a single request. DSMLServletTestCase is this servlet's first coverage; nice to have it.

@vharseko

vharseko commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Thanks — every point checked out, and the two medium ones were reproduced before fixing. Pushed as f13ffb0; the description has been rewritten accordingly.

The new log call is silently dropped

Confirmed: connectToHost() calls JDKLogging.disableLogging() (LogManager.reset() + root level OFF) on every non-verbose connection, so the record has no chance after the first LDAP connection in the JVM. Worth noting that the two pre-existing safeSetFeature() calls in the same file are dead for exactly the same reason.

Took the container log as suggested, with the missing super.init(config):

super.init(config);
...
getServletContext().log("Unable to send the DSML response", e);

authzid accumulates across batch requests

Reproduced verbatim. getSingleValue(values, ERR_LDAPAUTH_AUTHZID_SINGLE_VALUED) in doSASLPlain rejects the second bind client-side, before anything reaches the wire:

<ns0:errorResponse type="couldNotConnect"><ns0:message>org.opends.server.tools.LDAPConnectionException:
  The "authzid" SASL property only accepts a single value</ns0:message></ns0:errorResponse>

Fixed by dropping the previous value rather than rebuilding the options, since getSASLProperties() hands out the live map:

connOptions.getSASLProperties().remove("authzid");
connOptions.addSASLProperty("authzid=" + batchRequest.authRequest.getPrincipal());

testAuthzIdIsNotAccumulatedAcrossBatchRequests pins it: two batch requests with an authRequest each, and the endpoint has to see bind → abandon → unbind twice.

The two commits are not independently mergeable

Agreed — both stay in this PR. The description now says so explicitly, and the mvn -pl opendj-dsml-servlet test line carries the caveat that opendj-server-legacy has to be installed from this branch first.

malformedRequest response drops the requestID

Fixed as suggested; nothing has been read from is at that point, so the SAX pass recovers the ID:

batchResponses.add(createXMLParsingErrorResponse(is, objFactory, batchResponse,
    "Content-Type does not match SOAP 1.1 or SOAP 1.2"));

Nits

  • Dead null check — the connection is now a loop local created per batch request, and the connection == null check is gone.
  • Reply SOAP versioncontinue instead of break. Note the case only triggers on an Authorization value that is not valid Base64: Basic <base64 without a colon> decodes fine and fails later on the missing password, so the new testMalformedAuthorizationKeepsTheRequestSoapVersion uses Basic !!!.
  • Silent failures in the fake server — the exception is recorded and asserted null in awaitDisconnect(). The endpoint now also serves connections in a loop, which is what the multi-batch tests need.
  • defaultValue(Method) primitive gap — all eight primitives are covered now.
  • Mockito is already available — right, mockito-all and assertj-core are in the root pom's top-level <dependencies>. The description no longer claims otherwise.
  • Coverage gaps — added: SOAP 1.2 (request and reply), the multi-batchRequest reconnect, the authzid, and LDAPConnectionTestCase in opendj-server-legacy for Restore IT test for server-legacy and fix many errors #279. An abandon mid-batch is still uncovered.
  • HTTP status — kept 200 + malformedRequest, for consistency with how XML parse errors are already reported and because DSMLv2 clients read the body. Happy to add 415 alongside the body if you would rather have the HTTP-level signal.
  • Shared BatchResponse and the missing connect() timeout — both pre-existing; left out of this PR, they deserve their own issues.

One correction to my own description, which you were right to flag: stop-ds and manage-account install an SSLConnectionFactory unconditionally and CryptoManagerImpl sets setUseSSL(true), and createSSLOrBasicSocket() branches on the factory, not on useSSL() — so all three take createSSLSocket() and are unaffected by #279. The affected set is the DSML gateway plus the LDAPConnectionArgumentParser tools without --useSSL.

Also worth adding to your BindException observation: on macOS the bind is not always refused. setReuseAddress(true) lets it through on a port that is already listening, and the unconnected socket then reaches LDAPWriter, so the failure surfaces as Socket is not connected with result code -1 — same outer catch (Exception ex), same loss of the result code. LDAPConnectionTestCase asserts CLIENT_SIDE_CONNECT_ERROR on a closed port, and both of its tests fail if connect() is turned back into bind().

mvn -pl opendj-dsml-servlet test → 68 passing; mvn -pl opendj-server-legacy verify -Pprecommit -Dit.test=LDAPConnectionTestCase → 2 passing.

@vharseko
vharseko requested a review from maximthomas August 3, 2026 08:41

@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.

All four points from the previous round are addressed, and I re-checked each against the code rather than the description:

  • container logsuper.init(config) is in place, so getServletContext() is valid; the catch now writes there;
  • authzidgetSASLProperties() does hand out the live map (return saslProperties;) and addSASLProperty() stores the key verbatim, so remove("authzid") is the right handle — but see below;
  • commit split — description now states the dependency and carries the caveat on the mvn -pl command;
  • requestIDcreateXMLParsingErrorResponse() reuses the buffered stream; is is untouched at that point, and the new assertion pins it.

Both suites are green here: DSMLServletTestCase 10/10 (the eight of this PR plus two probes of my own) and LDAPConnectionTestCase 2/2 against a real server under -Pprecommit. One defect remains in the authzid fix.

authzid still survives into a batch request that has no authRequest (medium)

opendj-dsml-servlet/src/main/java/org/opends/dsml/protocol/DSMLServlet.java

The remove sits inside the authRequest != null branch, so it only runs when the next batch request also asks for an authorization identity:

if (batchRequest.authRequest != null) {
  if (authenticationIsID) {
    connOptions.getSASLProperties().remove("authzid");
    connOptions.addSASLProperty("authzid=" + batchRequest.authRequest.getPrincipal());

A body whose first batch request carries an authRequest and whose second does not leaves the first authzid in the shared options. Recording the SASL credentials at the fake endpoint:

body: batch "1" <authRequest principal="dn:cn=first"/>, batch "2" no authRequest

bind 1: mech=PLAIN creds=dn:cn=first|u:user|password
bind 2: mech=PLAIN creds=dn:cn=first|u:user|password     <-- batch 2 runs as cn=first

The control case is correct (dn:cn=first then dn:cn=second), which is why testAuthzIdIsNotAccumulatedAcrossBatchRequests passes: both of its batch requests carry an authRequest. The operations of the second batch execute under an identity the request never asked for — gated on ldap.authzidtypeisid=true (the web.xml default is false) and still subject to proxied-auth privileges on the server, hence medium rather than high.

Clearing unconditionally at the top of each iteration covers both cases:

// the options are shared by all the batch requests of this SOAP body
connOptions.getSASLProperties().remove("authzid");
if (batchRequest.authRequest != null) {
  if (authenticationIsID) {
    connOptions.addSASLProperty("authzid=" + batchRequest.authRequest.getPrincipal());

A mixed body would make a good third case in that test.

Nits

  • Two logging sinks in one class: four Logger.getLogger(PKG_NAME) calls remain (safeSetFeature ×2, createSafeDocument ×2) and are dead for exactly the reason now documented in the commit message. All four are instance methods, so getServletContext().log(...) is available to them too.
  • One bind per batch request: two batch requests now cost two binds (confirmed), where the extras used to be skipped. Password verification is deliberately expensive, so a body holding many batch requests is a cheap amplifier for a client with valid credentials — a cap on the number of batch requests per body would bound it.
  • Shared batchResponse: the new multi-batch test makes it observable — its own reply is <batchResponse requestID="2"> holding both batches' authResponse elements. Agreed it is pre-existing and belongs in its own issue; just noting the tests now document the odd shape.
  • LDAPConnectionTestCase is Linux-only in CI: .github/workflows/build.yml sets -P precommit only when runner.os == 'Linux', and opendj-server-legacy disables surefire, so the test runs on the five Ubuntu matrix entries and nowhere else. That is the platform where the bind is refused, so #279 is pinned where it matters; the macOS variant you describe (Socket is not connected) is not covered.
  • findFreePort() for the closed-port case is inherently racy, but rerunFailingTestsCount=3 absorbs a stray bind. Fine as is.
  • Unrelated: GrizzlyLDAPConnectionFactoryTestCase.testClientSideConnectTimeout fails in my environment on this branch. It is in opendj-grizzly, untouched here, and depends on an unroutable address — mentioning it only so it is not mistaken for a regression of this PR.

createSocket() has been binding the new client socket to the target server
address instead of connecting to it since OpenIdentityPlatform#279, so every plain or StartTLS
connection made through org.opends.server.tools.LDAPConnection fails with
"Address already in use" (server on the same host) or "Cannot assign
requested address" (remote server). Affects the DSML gateway, stop-ds,
manage-account and the other tools built on LDAPConnectionArgumentParser.
… on missing Content-Type

performLDAPRequest() returns null for an abandon request, but doPost()
dereferenced the result unconditionally, so a batch containing
<abandonRequest/> ended in a NullPointerException; as the connection was
closed after the loop instead of in a finally, one LDAP connection was
leaked per request.

messageFactory was only assigned when a SOAP 1.1 or SOAP 1.2 Content-Type
header was present, and was then dereferenced both when parsing the request
and when sending the response: a POST without Content-Type ended in a
NullPointerException, and, when an error response had already been queued,
in an empty HTTP 200 instead of that error. A missing or unsupported
Content-Type is now answered with a malformedRequest batch response.

Also log the failure instead of printing the stack trace when the response
cannot be sent, and add regression tests for both defects.
Report the failure to send the response to the container log: the
java.util.logging record was dropped, as connectToHost() resets the
LogManager and turns the root logger off on every non-verbose
connection. This needs super.init(config), without which
getServletContext() throws.

Drop the authzid of the previous batch request before setting the new
one: the connection options are shared by the whole SOAP body and
addSASLProperty() appends to the values of a key, so a second
authRequest made SASL PLAIN reject a multi-valued authzid. Now that the
connection is never reused, make it a loop local and remove the dead
null check that guarded the reuse.

Build the malformed Content-Type response with
createXMLParsingErrorResponse(), like the other two malformed paths, so
that the requestID is recovered; and keep reading the headers after a
malformed Authorization one, so that the reply keeps the SOAP version of
the request.

Cover the SOAP 1.2 path, the per-batch-request connection and the
authzid, let the fake LDAP endpoint serve several connections and fail
the test on a server-side error, and pin the createSocket() regression
of OpenIdentityPlatform#279 with a test in the module that owns it.
…uest of a SOAP body

The connection options are built once per doPost() and shared by all the batch
requests of the SOAP body, but the authzid was dropped only when the next batch
request carried an authRequest of its own. A body whose first batch request asks
for an authorization identity and whose second does not left the first authzid in
the options, so the operations of the second one ran under an identity the request
never asked for. It is gated on ldap.authzidtypeisid=true, which the shipped
web.xml leaves at false, and still subject to the proxied-auth privileges of the
server. The clearing now happens at the top of every iteration, before the
authRequest is looked at.

DSMLServletTestCase records the authorization identity of every SASL bind at the
fake endpoint: the existing test now asserts the identities themselves instead of
the mere absence of an error, and a new one pins the mixed body, where the second
batch request must bind with no authzid at all.

The four remaining Logger.getLogger(PKG_NAME) calls are replaced by
getServletContext().log(), so the class has a single logging sink: they were dead
for the reason already documented for the response path, which moves to the class
javadoc.
@vharseko
vharseko force-pushed the issues/809-dsml-servlet-npe branch from f13ffb0 to 9ddd7da Compare August 3, 2026 11:57
@vharseko vharseko added the security Security fixes / CodeQL code-scanning alerts label Aug 3, 2026
@vharseko

vharseko commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

The remaining defect is fixed and the branch has been rebased onto master, so this is a force push: 9ddd7da146.

authzid surviving into a batch request without authRequest

Confirmed and fixed. The clearing now happens at the top of every iteration, before the authRequest is looked at, exactly as suggested:

// The connection options are shared by all the batch requests of this
// SOAP body, so the authzid of the previous one must not survive into
// the bind of this one: it would run under an authorization identity
// it never asked for, and addSASLProperty() appends to the values of
// a key, which SASL PLAIN rejects as a multi-valued authzid.
connOptions.getSASLProperties().remove("authzid");

Worth spelling out why the existing test could not have caught this: FakeLdapServer only recorded message.getProtocolOpType(), so testAuthzIdIsNotAccumulatedAcrossBatchRequests passed on the absence of an error — the multi-valued authzid is rejected client-side, an inherited one is not. The endpoint now decodes the SASL PLAIN credentials (authzid NUL authid NUL password) and exposes getReceivedAuthzIds(), so both tests assert the identities themselves:

  • testAuthzIdIsNotAccumulatedAcrossBatchRequests[dn:cn=first, dn:cn=second];
  • testAuthzIdDoesNotSurviveIntoBatchRequestWithoutAuthRequest (new, the mixed body) → [dn:cn=first, ].

The new one reproduces your finding with the previous nesting restored:

testAuthzIdDoesNotSurviveIntoBatchRequestWithoutAuthRequest
  the second batch request ran under the authorization identity of the first one:
  Lists differ at element [1]:  != dn:cn=first expected [] but found [dn:cn=first]

mvn -pl opendj-dsml-servlet testTests run: 69, Failures: 0, Errors: 0.

Two logging sinks

Done. All four Logger.getLogger(PKG_NAME) calls in safeSetFeature() ×2 and createSafeDocument() now go to getServletContext().log(); the java.util.logging imports are gone and the rationale moved from the response path to the class javadoc, where it covers every call site.

rerunFailingTestsCount=3 absorbing a stray bind

That premise no longer holds — and in fact never did. #804 removed the setting from opendj-server-legacy/pom.xml for exactly this reason: the TestNG provider ignores rerunFailingTestsCount up to and including surefire 3.5.x, so nothing in that module was ever rerun (see #801). The branch was based on a master that predates it, which is presumably where the assumption came from; after the rebase the setting is gone from this branch too.

So testConnectToClosedPortIsAConnectError has no safety net. The window is the one between findFreePorts() closing its ServerSocket and connectToHost() reaching the port, with the port taken from the ephemeral range — I am leaving it as is, but say the word if you would rather see it hardened.

The rest

  • one bind per batch request — agreed that a body with many batch requests is an amplifier; it is the direct consequence of the extra batches no longer being skipped. A cap on the number of batch requests per body is new behaviour, so I would rather track it separately;
  • shared batchResponse — pre-existing, separate issue, as you say; the multi-batch tests just make the shape visible;
  • LDAPConnectionTestCase is Linux-only — confirmed, -P precommit is set only for runner.os == 'Linux' and opendj-server-legacy disables surefire. That is the platform where the bind is refused, so Restore IT test for server-legacy and fix many errors #279 stays pinned where it matters;
  • GrizzlyLDAPConnectionFactoryTestCase.testClientSideConnectTimeout — nothing in opendj-grizzly is touched here and CI is green on this branch; if it reproduces for you outside this PR it deserves its own issue.

The description has been updated: the authzid bullet now describes both failure modes, the logging paragraph covers all four call sites, and the test list and counts match the current suite.

@vharseko

vharseko commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

The two follow-ups are filed and linked from the description:

@vharseko
vharseko requested a review from maximthomas August 3, 2026 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug java Pull requests that update java code security Security fixes / CodeQL code-scanning alerts tests Test suites: fixing, enabling, un-disabling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DSML gateway: NPE on abandonRequest leaks the LDAP connection, and NPE when Content-Type is absent

2 participants