[PR] fix: return ERROR_TOKEN when basic auth credentials are missing [shenyu]

["lll-peanut (via GitHub)" <gi...@apache.org>]

lll-peanut opened a new pull request, #6533:
URL: https://github.com/apache/shenyu/pull/6533

   Fixes #6474
   
     ## What's changed
     - BasicAuthPlugin.doExecute now short-circuits with ERROR_TOKEN when the
       Authorization header and URI user info are both missing, instead of passing
       null to the authentication strategy.
     - DefaultBasicAuthAuthenticationStrategy.authenticate now uses Objects.equals
       so a null credential fails authentication instead of throwing NPE.
   
     ## Why
     A request without credentials produced `authorization == null`, and the
     default strategy dereferenced it (`authentication.equals(...)`), causing a
     NullPointerException (HTTP 500) instead of the expected 401 ERROR_TOKEN.
   
     ## Tests
     - BasicAuthPluginTest.testDoExecuteWithoutAuthorization: request without
       Authorization header / user info must return Illegal authorization and not
       invoke the chain (failed with NPE before the fix).
     - DefaultBasicAuthAuthenticationStrategyTest.testAuthenticateWithNullAuthentication:
       authenticate(null) must return false, not throw.
     - `mvn -pl shenyu-plugin/shenyu-plugin-security/shenyu-plugin-basic-auth test`
       → 15 tests passed.
   
     ## Manual verification
     - no credentials        → {"code":401,"message":"Illegal authorization"}
     - Authorization: test:test123 → request forwarded to upstream (200)
     - wrong credentials     → 401 Illegal authorization
   
   Make sure that:
   
   - [X ] You have read the [contribution guidelines](https://shenyu.apache.org/community/contributor-guide).
   - [X] You submit test cases (unit or integration tests) that back your changes.
   - [X] Your local test passed `./mvnw clean install -Dmaven.javadoc.skip=true`.
   


-- 
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: notifications-unsubscribe@shenyu.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: return ERROR_TOKEN when basic auth credentials are missing [shenyu]

["Aias00 (via GitHub)" <gi...@apache.org>]

Aias00 merged PR #6533:
URL: https://github.com/apache/shenyu/pull/6533


-- 
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: notifications-unsubscribe@shenyu.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: return ERROR_TOKEN when basic auth credentials are missing [shenyu]

["Aias00 (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@shenyu.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org