Repository navigation
fix(server): decode Basic auth as UTF-8 and split on the first colon - #3290
Open
arshilkxwork wants to merge 2 commits into
Open
arshilkxwork wants to merge 2 commits into
arshilkxwork wants to merge 2 commits into
Conversation
- Decode the Basic credential as UTF-8 (RFC 7617 section 2.1). US-ASCII
turned every non-ASCII byte into U+FFFD, so a password such as
"ädminpass1" never matched the stored hash and the request got 401.
- Split at the first colon. RFC 7617 bars a colon from the user-id only,
and split(":") answered 400 for a password such as "new:pass1234".
- A credential with no colon, an empty user-id or an empty password
still gets 400.
- Add a LoginApiTest case covering both passwords.
- CypherAPI.toUserPass still split the decoded credential on every colon and returned null for anything but two parts, so a password with ':' passed AuthenticationFilter and then failed on the cypher endpoint. It now splits once, like the filter and Gremlin's handler. - Reword the filter comment: RFC 7617 allows UTF-8 as the only declared charset and does not define a default. - Point the Helm wrapper comment at apache#3284; the TODO it named is gone. - Parse the created user with TypeReference like the other tests in LoginApiTest, and say why the Basic header is built by hand.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of the PR
AuthenticationFilterdecoded the Basic credential as US-ASCII and split it on every colon. The user API accepts passwords with non-ASCII characters or a colon, so once an account had such a password it could no longer use Basic auth: the non-ASCII case got 401 and the colon case got 400. The admin account is affected too.Main Changes
The filter now decodes the credential as UTF-8 (RFC 7617 section 2.1) and splits it at the first colon. Everything after that colon is the password. A credential with no colon, an empty user-id or an empty password still gets 400. The grizzly
Charsetsimport is replaced withStandardCharsets, and the TODO that pointed at this issue is gone.CypherAPI.toUserPassparses the header again for the Cypher endpoint. It already decoded UTF-8 but split on every colon, so a colon password would have passed the filter and then failed there. It now splits once as well, which matches Gremlin'sWsAndHttpBasicAuthHandler(split(":", 2)).One edge case changes: a credential with a trailing colon used to lose it, because
String.splitdrops trailing empty strings.user:pass:authenticated with the passwordpassand now triespass:, which gets 401 unless that is the real password.user::goes from 400 to 401.Non-ASCII passwords work for clients that encode the header as UTF-8, such as curl and browsers. Clients that encode ISO-8859-1 still get 401 for them, as before. That includes Jersey's
HttpAuthenticationFeatureand OkHttp'sCredentials.basicdefault, whichOkHttpBasicAuthInterceptorin hugegraph-commons uses. Changing those clients is left for a separate PR.Following the discussion on the issue, two things stay out of this PR:
values.schema.jsonand the Server wrapper inserver-deployment.yaml) stays, since the chart deploys published images that do not have this fix yet. The chart README also ties the space and backslash rules to a separate entrypoint fix. The only Helm edit is the wrapper comment, which named the TODO this PR removes and now points at [Bug] Basic auth decodes the credential as ASCII and splits on every colon: a non-ASCII password answers 401, a password with ':' answers 400 #3284.Verifying these changes
LoginApiTest#testBasicAuthWithNonAsciiOrColonPasswordcreates users with the passwordsädminpass1andnew:pass1234, sendsGET graphspaces/DEFAULT/graphswith a UTF-8 Basic header, and expects 200 for the right password and 401 for a wrong one.run-api-test.sh:LoginApiTest,UserApiTestandCypherApiTestpass (20 tests). With the old filter the new test fails with 401 onädminpass1.a:b:cgets 200 fromGET .../graphs/hugegraph/cypher.admin:pagets 200,admin:wrong401, andadmin:,adminand:pa400, the same as before.Does this PR potentially affect the following parts?
:, and non-ASCII passwords sent as UTF-8Documentation Status
Doc - TODO: required documentation is pending; complete it before merging.Doc - Done: documentation is included here or linked below.Doc - No Need: no user-visible documentation is affected.The ASCII-decoding text in the chart (
values.schema.jsonand the README Validation section) describes the published image the chart deploys. It should change together with the guard once a release ships this fix.