bitflicker64 commented on code in PR #3290:
URL: https://github.com/apache/hugegraph/pull/3290#discussion_r4248286568
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -184,19 +184,16 @@ protected User authenticate(ContainerRequestContext
context) {
if (auth.startsWith(BASIC_AUTH_PREFIX)) {
auth = auth.substring(BASIC_AUTH_PREFIX.length());
- // TODO: decode the Basic credential as UTF-8 and split it on the
first colon only.
- // Decoding as ASCII and splitting on every colon makes a
non-ASCII password answer
- // 401 and a password containing ':' answer 400, although both
were accepted at
- // account creation. The Helm chart (helm/hugegraph) refuses such
admin passwords in
- // its schema and Server wrapper; drop that guard once this is
fixed.
- auth = new String(DatatypeConverter.parseBase64Binary(auth),
Charsets.ASCII_CHARSET);
- String[] values = auth.split(":");
- if (values.length != 2) {
+ // RFC 7617 section 2.1 allows only UTF-8 as the declared charset,
and it bars a
+ // colon from the user-id alone, so split on the first one and
keep the rest
+ auth = new String(DatatypeConverter.parseBase64Binary(auth),
StandardCharsets.UTF_8);
+ int colon = auth.indexOf(':');
+ if (colon < 0) {
Review Comment:
Minor: the 400 paths in this block and the two behaviour changes listed
under Compatibility have no automated test. Every API test authenticates
through Jersey's `HttpAuthenticationFeature` with a non-empty user and
password, and the two hand-built headers (`LoginApiTest.basicAuthGet`,
`CypherApiTest.basicAuthCypher`) are well-formed as well, so nothing sends a
credential without a colon, with an empty field, or with a trailing colon.
Codecov reports one partially covered line for this file in the patch. The
description checks `admin`, `admin:` and `:pa` by hand with curl, and nothing
checks `user:pass:` or `user::`.
Requested change: next to `testBasicAuthWithNonAsciiOrColonPassword`, send
hand-built headers for `admin`, `admin:` and `:pa` and expect 400, and for one
of the users created there send `<name>:<password>:` and `<name>::` and expect
401.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
