SEZ9 commented on issue #12594:
URL: https://github.com/apache/seatunnel/issues/12594#issuecomment-5964411265
Thanks for the detailed follow-up, and apologies for missing that the PR was
already open before my comment — that explains why it isn't linked from the
issue body. Could you add a closing reference to this issue in the PR
description so the two are tied together?
On the substance, this covers everything I asked for:
- Computing both comparisons into locals before the branch and routing each
through `MessageDigest.isEqual` on the UTF-8 bytes is exactly the shape I had
in mind. Your note on argument order is a good catch — `isEqual` does iterate
over the length of its first argument, so passing the request-supplied value
first is the right call, and documenting that in the helper's javadoc is the
kind of guard that keeps a future refactor from quietly undoing it.
- Rejecting an unset credential explicitly in the helper, rather than
letting a null reach the byte conversion and surface as a 500 through
`ExceptionHandlingFilter`, is the correct behaviour, and having a test that
fails with that NPE if the guard is removed is reassuring.
- `BasicAuthFilterTest` with the
correct/wrong-username/wrong-password/proper-prefix/unset/disabled cases is
what I wanted, and the proper-prefix case in particular is the one the old
`wronguser:wrongpassword` IT could not distinguish.
- Thanks for confirming the Web UI goes through the same `BasicAuthFilter`
registration on `/*` in `JettyService`, and for grepping for other credential
comparisons. That settles my question about whether a second path needed the
same treatment.
Two small things remaining:
1. Your comment appears to have been cut off at the end of the
`BasicAuthenticationIT` paragraph ("cannot tell whether the password is exa…").
Could you finish that thought, or just confirm that the two new end-to-end
cases are the wrong-password and proper-prefix ones you listed?
2. Once the issue reference is in the PR description, I'll do the review on
the PR itself and leave any line-level comments there.
Given ASF Security's confirmation that this is a hardening improvement
rather than a vulnerability, there is no embargo concern, so we can proceed
through the normal review flow.
<!-- streview-comment:1477 -->
--
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]
