Reject empty Basic auth passwords in BasicAuthMiddleware

[stevehu]

Summary

Ports the light-4j BasicAuthHandler fixes from networknt/light-4j#2805 (issue networknt/light-4j#2804) to BasicAuthMiddleware:

• Reject an empty submitted password before local or LDAP validation. Previously a user configured with password: "" and enableAD: false authenticated with a blank password, and AD-delegated users relied on the directory refusing unauthenticated binds.
• Compare local passwords with MessageDigest.isEqual.
• Handle short Authorization headers without substring exceptions.
• Stop logging credentials (maskHalfString) and attacker-supplied usernames.
• Tests: testEmptyConfiguredPasswordIsRejected, testShortMalformedAuthorizationHeadersAreRejected.

The LdapUtil hardening (filter escaping, connection cleanup, timeouts) arrives with a later light-4j dependency bump; the middleware guard already blocks empty passwords before LDAP.

Test plan

• BasicAuthMiddlewareTest passes (10 tests)

🤖 Generated with Claude Code

[Copilot]

Note

Copilot was unable to run its full agentic suite in this review.

Copilot review overview

Review effort: Lite
Findings: 2

Open (2)

• The auth.trim().length() == 5 check hard-codes the prefix length, but the prefix is already… · New
• This test only asserts the HTTP status code, so it could pass even if the middleware starts… · New

What changed in this PR

This PR hardens Basic authentication handling in the Lambda middleware by rejecting empty passwords and safely handling malformed/short Authorization headers without risking substring bounds errors.

Changes:

• Add a test user with an empty configured password and new unit tests for empty passwords and short/malformed auth headers.
• Update BasicAuthMiddleware to use safer prefix detection (regionMatches) and reduce sensitive data in logs.
• Compare Basic passwords using MessageDigest.isEqual and explicitly reject empty supplied passwords.

File	Description
src/​test/​resources/​config/​basic-auth.yml	Adds a test user with an empty configured password to validate rejection behavior.
src/​test/​java/​com/​networknt/​aws/​lambda/​middleware/​security/​BasicAuthMiddlewareTest.java	Adds regression tests for empty configured passwords and malformed/short Authorization headers.
src/​main/​java/​com/​networknt/​aws/​lambda/​handler/​middleware/​security/​BasicAuthMiddleware.java	Hardens auth header parsing and Basic auth validation; reduces sensitive logging.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[stevehu]

Code review(xhigh · 10 findings)
src/main/java/com/networknt/aws/lambda/handler/middleware/security/UnifiedSecurityMiddleware.java
● 176 [incomplete-fix] The PR stops echoing raw Authorization header fragments in BasicAuthMiddleware, but the sibling entry point UnifiedSecurityMiddleware still logs and returns them: the first 10 chars of unknown-scheme headers (lines 176-178), and the whole header when it is 5 chars or shorter (lines 93-94).
● 97 [altitude] UnifiedSecurityMiddleware has its own Authorization scheme parsing (substring + equalsIgnoreCase, guard trim().length() <= 5), which differs from the regionMatches/== 5 logic this PR put in BasicAuthMiddleware.execute.
src/test/java/com/networknt/aws/lambda/middleware/security/BasicAuthMiddlewareTest.java
● 45 [test-coverage] No test covers the AD branch, where the new empty-password guard is supposed to stop an unauthenticated LDAP bind. The added test uses basic-auth.yml, where enableAD is effectively false at runtime, so it only exercises the local-compare branch.
● 67 [test-coverage] The short-header test asserts only HTTP 401, not the error codes (ERR12003 for x, ERR10046 for Basic eA). It also skips the short-Bearer case, where handleBearerToken used to crash at auth.substring(0, 10).
src/main/java/com/networknt/aws/lambda/handler/middleware/security/BasicAuthMiddleware.java
● 173 [security] The special pseudo-users anonymous and bearer have no password in config, so when enableAD is true, Basic credentials naming them go to LDAP authentication. A directory account with that name then gets the pseudo-user's path grants.
● 202 [removed-behavior] The authorization-failure log no longer includes the request path or the (already authenticated) configured username. The final light-4j revision of #2805 kept both, with the path CR/LF-sanitized, so this port has already drifted from the source it claims to mirror.
● 162 [security] Unknown usernames return immediately, while known AD-delegated usernames make a network LDAP round trip. So valid account names can still be enumerated by response latency, despite the move to constant-time comparison.
● 151 [correctness] handleBasicAuth is public and still calls auth.substring(6) without a length check, so the short-header hardening relies on every caller validating the length first.
● 150 [altitude] The credential-validation logic (parse, empty-password guard, constant-time compare, LDAP dispatch) is hand-copied from light-4j BasicAuthHandler, so each security fix has to be ported manually and the two copies already disagree (see the path-denied log).
● 167 [simplification] The new empty-password block duplicates the user == null block above it (same debug line, same ERR10047 return). The username.equals(user.getUsername()) checks at lines 173 and 184 are also redundant, because users is keyed by username.

I reported 10 findings on PR #185. None of them is a blocker, and the fix holds up: I ran the two new tests against the old BasicAuthMiddleware. The empty-password test returned 200 instead of 401, and the short-header test threw StringIndexOutOfBoundsException, so both tests do catch the bugs the PR fixes.

The most important findings:

• Credential echo still in the other entry point: the PR stops logging header fragments in BasicAuthMiddleware. UnifiedSecurityMiddleware still logs the first 10 characters of an unrecognized Authorization header and returns them in the response body.
• The LDAP case isn't tested: the new empty-password test only covers local passwords. basic-auth.yml doesn't set enableAD, and at runtime it defaults to false even though the config annotation says "default is true". A test using basic-auth-ldap.yml wouldn't need an LDAP server, because the new check rejects the request before LDAP is called.
• anonymous and bearer can log in with Basic credentials: when enableAD is on, a Basic login as either special user goes to LDAP. That code was there before this PR; it's in a function the PR touches.
• The port has already drifted from light-4j: the final light-4j version still logs the request path (cleaned of line breaks) and the username when access to a path is denied. This port dropped both from the log but still returns them in the response.

The rest are lower priority:

• Valid AD usernames can still be guessed from response time, because they trigger an LDAP round trip and unknown names don't.
• The public handleBasicAuth still crashes on a header shorter than 6 characters if a caller skips the length check.
• The short-header test checks only the 401 status, not the error codes, and doesn't cover a short Bearer header.
• Three cleanups: the same validation logic is copied between this repo and light-4j, the Basic/Bearer header parsing is written twice in this repo, and there's a duplicated rejection block.

[Copilot]

Copilot review overview

🟡 Changes recommended

Prevent bare Bearer headers from bypassing authentication and use the sanitized path in the returned status.

Review effort: Lite
Findings: 1

Open (1)

• Reject bearer authentication headers without credentials · New

Resolved since last review (2)

• This test only asserts the HTTP status code, so it could pass even if the middleware starts…
• The auth.trim().length() == 5 check hard-codes the prefix length, but the prefix is already…

[stevehu]

Copilot reviewed your latest commit (404c006). It has one open finding; the two from its first review are marked resolved.

Open finding: a Bearer header with no token is let through (BasicAuthMiddleware.java:73, rated High)

The bug is real, but less serious than "High" suggests. execute sends a bare Bearer or Bearer header straight to handleBearerToken. When allowBearerToken is on, that method only checks the path, so the request succeeds.

Why it's not really an authentication bypass:

• This middleware never checks bearer tokens. handleBearerToken only matches the path; it never reads the token. Bearer garbage already passes exactly the same way, so an empty token gets an attacker nothing new. The real token check has to happen in a JWT/SWT handler further down the chain. If no such handler is configured, any bearer value gets through, empty or not.
• It isn't new. Before this PR, "Bearer" matched substring(0, 6) and took the same path.

It's still worth fixing. It's a single check, and it would make execute reject bare Bearer headers the same way it now rejects bare Basic headers and the way UnifiedSecurityMiddleware already does:

AuthorizationScheme scheme = AuthorizationScheme.parse(auth);
if (scheme != AuthorizationScheme.UNKNOWN && !AuthorizationScheme.hasCredentials(auth, scheme)) {
LOG.error("Invalid/Unsupported authorization header.");
return new Status(INVALID_AUTHORIZATION_HEADER, scheme.name());
}

That would sit just before the BASIC/BEARER dispatch. I'd also add a test with basic-auth-bearer.yml (where allowBearerToken is true) checking that Bearer and Bearer both return ERR12003.

Mentioned in Copilot's summary but not filed as a finding: "use the sanitized path in the returned status." The log now strips line breaks from the path, but new Status(NOT_AUTHORIZED_REQUEST_PATH, requestPath, ...) still puts the raw path in the response body. That's low risk, since the path only goes back to the client who sent it. Using the cleaned-up path in both places would still keep them consistent, and it costs one line.

Do you want me to make both changes and add the test on issue2804-basic-auth? I'd leave them uncommitted for you to check.

[stevehu]

Both Copilot items are fixed on issue2804-basic-auth. The changes aren't committed.

1. A Bearer header with no token is now rejected (BasicAuthMiddleware.execute)

• execute now checks the header for a token or credentials before handing it to either handler. A bare Bearer, Bearer , or Basic header now gets ERR12003 (401), the same way UnifiedSecurityMiddleware already handles them.
• A new test, testBareBearerRejectedWhenBearerTokensAllowed, uses basic-auth-bearer.yml (where allowBearerToken is on) and checks that Bearer, Bearer and bearer are all rejected.
• One existing assertion changed: a bare Bearer header used to get ERR10046 and now gets ERR12003. Bearer x still gets ERR10046 when bearer tokens aren't allowed.

2. The returned error now uses the cleaned-up path

• When a Basic user is denied a path, the cleaned path (line breaks replaced with spaces) now goes into both the log and the returned ERR10071.
• The path-denial test now checks that the response contains /denied forged-line rather than the raw \r\n path.

Results: mvn -o test finished with 95 run, 0 failures, 0 errors and 4 skipped. git diff --check is clean. The impact checks on execute and handleBasicAuth both came back LOW risk.

Not changed: the path-denial errors for the anonymous and bearer users still log and return the raw path. Copilot only flagged the Basic one, but the same one-line fix applies to both if you want them consistent.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The empty-password regression test does not configure enableAD: false, so it does not cover the vulnerable local-password path.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Reject bearer authentication headers without credentials