Aias00 commented on PR #6533:
URL: https://github.com/apache/shenyu/pull/6533#issuecomment-5156774921
## Review: fix: return ERROR_TOKEN when basic auth credentials are missing
**Verdict: Approve (ship it).** No blocking issues — the fix is correct,
minimal, fail-closed, and well-tested.
### Correctness
- Before: a request with no `Authorization` header and no URI userInfo
produced `authorization == null` (`StringUtils.defaultString(null, null)`),
which then hit `authentication.equals(...)` in
`DefaultBasicAuthAuthenticationStrategy.authenticate` → `NullPointerException`
(HTTP 500) instead of the expected 401.
- After: `BasicAuthPlugin.doExecute` short-circuits on
`StringUtils.isBlank(authorization)` → `ShenyuResultEnum.ERROR_TOKEN` (code
401, "Illegal authorization", confirmed in `ShenyuResultEnum.java:43`), and
`DefaultBasicAuthAuthenticationStrategy.authenticate` now uses `Objects.equals`
so a `null` credential returns `false` rather than NPE. The two changes are
complementary defense-in-depth (the plugin guard also protects custom SPI
strategies from a null-credential NPE), not redundant.
- No fail-open / auth-bypass is introduced: `chain.execute` is unreachable
without a successful `authenticate`. Confirmed `authenticate` is only invoked
when `basicAuthRuleHandle` is non-null
(`Objects.nonNull(authenticationStrategy)` derives from the handle via
`Optional`), so the `null`-handle cast path is not reachable here.
### Tests
- `BasicAuthPluginTest.testDoExecuteWithoutAuthorization` is a genuine
regression test: pre-fix it throws NPE synchronously from `doExecute` (failing
`verifyComplete()`); post-fix it asserts the body contains "Illegal
authorization" and `chain` is never invoked. Good.
-
`DefaultBasicAuthAuthenticationStrategyTest.testAuthenticateWithNullAuthentication`
directly covers `authenticate(..., null) → false`.
### Nits (optional)
1. `BasicAuthPluginTest.testDoExecuteWithoutAuthorization` reassigns the
shared instance field `exchange` instead of a local variable. Harmless given
`@BeforeEach`, but a local `ServerWebExchange noAuthExchange = ...` would be
clearer.
2. Consider adding the empty-header case (`Authorization: ""`) — it
exercises the `defaultString("", null) → "" → isBlank` branch of the new guard,
distinct from the pure-null path.
CI: all build / e2e / it / it-k8s / CodeQL checks pass. No license-header or
generated-artifact issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
