SEPURI-SAI-KRISHNA commented on issue #12594: URL: https://github.com/apache/seatunnel/issues/12594#issuecomment-5946077061
Thanks @SEZ9. The PR is already open: #12595. It predates this comment, which is why it is not linked from the issue body. It does what you describe. Both comparisons are computed into locals before the branch so neither is skipped, and each goes through `MessageDigest.isEqual` on the UTF-8 bytes. One detail worth flagging since it is easy to get backwards: `isEqual` reads exactly as many bytes as its **first** argument holds, which I confirmed against the JDK 17 bytecode rather than the javadoc, so the value from the request is passed first and the configured credential's length does not drive the work. The reasoning is in the helper's javadoc so a later edit does not silently swap the arguments. Your two implementation requests are already in: - **Disabled auth and null credentials behave identically.** The `enableBasicAuth` early return is untouched. For null, `String.equals(null)` was safely `false`, and reading bytes off a null would be an NPE, which `ExceptionHandlingFilter` turns into a 500 rather than a 401, so the helper rejects an unset credential explicitly. There is a test for all three unset combinations, and removing that guard fails it with exactly that NPE. - **Unit test for `BasicAuthFilter`.** `BasicAuthFilterTest` is new, following `ExceptionHandlingFilterTest` in the same package. It covers correct credentials, wrong username, wrong password, a credential that is a proper prefix of the configured one, the three unset combinations, and auth disabled. Six cases, green on all four unit-test legs in CI: JDK 8 and JDK 11, on both Linux and Windows. **On the Web UI auth path: there is no second one, it is the same filter.** `JettyService` registers `BasicAuthFilter` on `/*` of the servlet context, and the Web UI is served by `DefaultServlet` from the `ui` classpath resource inside that same context, so the filter guards the UI and REST API v2 together. I also grepped the repository for other credential comparisons and `BasicAuthFilter.java:87-88` is the only one of this shape; the remaining `.equals` hits on username/password/token names are connector configuration, mock-credential guards and `equals()` implementations on pool keys. So the one PR covers both, and there is nothing else to fix alongside it. `BasicAuthenticationIT` also gained two end-to-end cases, a correct username with a wrong password and a proper prefix of both, since the existing negative case sends `wronguser:wrongpassword` and cannot tell whether the password is examined once the username has failed. That ran on CI at 9 tests, 0 failures. -- 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]
