fix(server): decode Basic auth as UTF-8 and split on the first colon

[arshilkxwork]

Purpose of the PR

• close [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

AuthenticationFilter decoded the HTTP Basic credential as US-ASCII and split it on every colon. The user API accepts passwords that contain non-ASCII characters or a colon, so an account given such a password, including the admin account, could no longer authenticate with Basic auth. A non-ASCII password received 401, and a password containing a colon received 400.

Main Changes

AuthenticationFilter now decodes the credential as UTF-8 (RFC 7617, section 2.1) and splits it at the first colon, treating the remainder as the password. A credential without a colon, or with an empty user-id or password, still receives 400. The Grizzly Charsets import is replaced with StandardCharsets, and the TODO that referred to this issue is removed.

CypherAPI.toUserPass parses the header separately for the Cypher endpoint. It split the credential on every colon and decoded it with Base64.getUrlDecoder(), which rejects the + and / characters of standard Base64. Such credentials, which are common with non-ASCII passwords, passed the filter and then received 500 from the Cypher endpoint. It now uses Base64.getDecoder() and split(":", 2), consistent with WsAndHttpBasicAuthHandler.

The comment in the Helm Server wrapper (server-deployment.yaml) and the corresponding note in the chart README referred to the removed TODO and now refer to #3284.

Compatibility

• A trailing colon is now part of the password. user:pass: previously authenticated with pass and is now evaluated as pass:; user:: now receives 401 instead of 400.
• Non-ASCII passwords are accepted from clients that encode the header as UTF-8, such as curl and browsers. Clients that encode it as ISO-8859-1 continue to receive 401 for such passwords. These include Jersey's HttpAuthenticationFeature and the OkHttp Credentials.basic default used by OkHttpBasicAuthInterceptor in hugegraph-commons. Changes to those clients are outside the scope of this PR.

Out of scope

In line with the discussion on the issue:

• No additional validation is introduced at user creation or password update. A user name containing a colon still cannot authenticate with Basic auth, as before this change.
• The Helm chart guard on the admin password (values.schema.json and the Server wrapper) is retained, because the chart deploys published images that do not include this fix. Its removal also depends on a separate entrypoint fix for spaces.

Verifying these changes

• Trivial rework / code cleanup without any test coverage. (No Need)
• Already covered by existing tests, such as (please modify tests here).
• Need tests and can be verified as follows:
  • LoginApiTest#testBasicAuthWithNonAsciiOrColonPassword creates users with the passwords ädminpass1 and new:pass1234 and calls GET graphspaces/DEFAULT/graphs with a UTF-8 Basic header. It expects 200 for the correct password and 401 for an incorrect one.
  • CypherApiTest#testBasicAuthWithColonPassword grants cypher_colon_user READ and EXECUTE on the graph through a group and queries the Cypher endpoint with the passwords a:b:c, a:b:c~ and a:b:c?. The latter two produce headers containing + and /. Each password must return the query result, and an incorrect password must receive 401. All entities created by the test are deleted afterwards, and a failed deletion fails the test. The setup relies on the standalone auth API, so the test is skipped on HStore.
  • Local results at c9339f0: the full api-test suite passes on RocksDB configured as in run-api-test.sh (183 tests, 14 skipped) and on HStore with PD and Store started by install-backend.sh (177 tests, 52 skipped). Checkstyle and editorconfig:check report no issues in the changed files.
  • Against the previous code, the REST test fails with 401 for ädminpass1, and the Cypher test fails with 500 (The 'user-password-pair' can't be null) with either the former split(":") or the URL-safe decoder.
  • Unit tests pass locally, except four cases in RocksDBSnapshotRestoreTest that fail on macOS because /var resolves to /private/var. This PR does not modify that code.
  • LoginApiTest#testBasicAuthWithMalformedCredential sends hand-built headers. After a control request with test:test that returns 200, it expects 400 for admin, admin: and :pa, and 401 for test:test: and test::, which covers the trailing-colon changes listed under Compatibility.

Does this PR potentially affect the following parts?

• Dependencies (add/update license info & regenerate_known_dependencies.sh)
• Modify configurations
• The public API
• Other affects (typed here): REST and Cypher Basic auth accept passwords that contain :, and non-ASCII passwords sent as UTF-8
• Nope

Documentation 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 Helm chart (values.schema.json and the README validation section) describes the published image that the chart deploys. It should be updated together with the guard once a release includes this fix.

[imbajin]

Blocking: no. Summary: The new REST regression covers UTF-8 and colon passwords, but the separate Cypher parser change lacks an automated colon-password case. Evidence: Exact-head diff and test-file review.

[bitflicker64]

Blocking: no. Summary: The filter now decodes Basic credentials as UTF-8 and splits on the first colon, and the new LoginApiTest covers both cases on the REST path; the only gap is that the separate Cypher parser change has no automated colon-password case. Evidence: exact-head diff of AuthenticationFilter, CypherAPI and LoginApiTest at 56a4e06; WsAndHttpBasicAuthHandler already uses split(":", 2); no other Basic-auth decoder in hugegraph-server; latest-head workflows are waiting for maintainer approval (action_required), so CI has not run.

[bitflicker64]

Blocking: yes. Summary: The filter change is correct and the REST path now accepts UTF-8 and colon passwords, but CypherAPI.toUserPass still decodes the header with the URL-safe Base64 decoder, so the Cypher endpoint answers 500 for any credential whose Base64 contains + or /, which is a large share of the non-ASCII passwords this PR enables. Evidence: exact-head build of 577f4c3 on a RocksDB server configured as in run-api-test.sh; LoginApiTest, UserApiTest and CypherApiTest pass (33 tests); curl probes with hand-built UTF-8 Basic headers gave 200 on REST and 500 on Cypher for probe:密码pass1 and probe:a:b:c~; the workflows at this head are waiting for maintainer approval, so CI has not run, and the new tests were not run on HStore.

[imbajin]

Supplemental review at 577f4c3b74dccdd4ce8b9e6d35852639bad7d51d: 7/10. I would hold merge until the existing Cypher Basic-auth decoder issue is addressed and current-head CI passes.

The REST UTF-8/first-colon fix is focused, and the added Cypher regression is useful. Its credential does not exercise the standard Base64 + or / alphabet, however. Two concrete cases are:

• cypher_colon_user:a:b:c~ → Y3lwaGVyX2NvbG9uX3VzZXI6YTpiOmN+
• cypher_colon_user:a:b:c? → Y3lwaGVyX2NvbG9uX3VzZXI6YTpiOmM/

Please use the standard Base64 decoder and add Cypher endpoint regressions for both cases, including wrong-password rejection. This complements the existing inline discussion, whose reviewer reports REST 200 versus Cypher 500. Shared parsing is optional; no broader refactor is required.

Evidence boundary: I inspected the current source paths and verified these encodings; I did not rerun repository tests or live probes. The author's colon-password regression report and the reviewer's runtime probes remain their reported evidence. All five workflows at this exact head—License Checker, Server Memory CI, CodeQL, HugeGraph-Server CI and Server Compatibility CI—currently have action_required, so they do not establish a passing CI result.

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 51.75%. Comparing base (0f86e2a) to head (cd10ba8).
⚠️ Report is 20 commits behind head on master.

Files with missing lines	Patch %	Lines
...che/hugegraph/api/filter/AuthenticationFilter.java	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #3290       +/-   ##
=============================================
+ Coverage     41.38%   51.75%   +10.37%     
+ Complexity     6945     2638     -4307     
=============================================
  Files           762      762               
  Lines         66934    67399      +465     
  Branches       8988     9138      +150     
=============================================
+ Hits          27698    34881     +7183     
+ Misses        36062    28991     -7071     
- Partials       3174     3527      +353     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[arshilkxwork]

Thank you for the review. Commit 7e048d1 switches CypherAPI to the standard Base64 decoder and adds both suggested cases (...OmN+ and ...OmM/) to the Cypher test, each with a wrong-password check. The details are in the inline thread. The parsing remains in CypherAPI and is not shared with the filter.

The same commit addresses the pd_store / hstore failure at 577f4c3, which was caused by the new test. Its group setup applies only to standalone mode, so the test is now skipped on HStore.

Local results at 7e048d1: the full api-test suite passes on RocksDB (182 tests, 14 skipped) and on HStore with PD and Store started by install-backend.sh (176 tests, 52 skipped). Unit tests pass except four cases in RocksDBSnapshotRestoreTest, which fail on macOS because /var resolves to /private/var. This PR does not modify that code.

[imbajin]

Blocking: no. Summary: UTF-8 Basic credentials are decoded on the REST and Cypher paths, but the expected charset is not advertised to clients. Evidence: exact-head 401 handling and RFC 7617 section 2.1.

[imbajin]

⚠️ Blocking: no. Summary: Both Basic-auth parsers now unconditionally decode credentials as UTF-8, but the 401 paths do not advertise charset="UTF-8"; the PR also notes that Jersey and OkHttp clients still send ISO-8859-1. Those clients cannot infer the new encoding and continue returning 401 for non-ASCII passwords. Please advertise the expected charset in the Basic WWW-Authenticate challenge for both entry points, add a response-level regression, and document any remaining client limitations. Evidence: RFC 7617 section 2.1 defines the challenge charset parameter as the signal for the expected encoding; the exact-head AuthenticationFilter and CypherAPI 401 paths do not include it.

[arshilkxwork]

Thank you for the suggestion. I agree that advertising the expected charset is the signal RFC 7617 section 2.1 defines, and I would like to confirm the intended scope before implementing it.

At present the 401 responses do not carry a Basic challenge. The WWW-Authenticate header contains the error text instead: on a local build of 7e048d1 it is Missing authentication credentials when no credentials are sent and Incorrect username or password when they are wrong, on both the REST and the Cypher paths. Advertising the charset therefore means replacing that value with a standard challenge such as Basic realm="...", charset="UTF-8".

Browsers respond to a Basic challenge by showing their native login dialog, so this would also change what browser-based clients such as Hubble display when credentials are missing or a session has expired.

Would you prefer this change in this PR, or in a separate PR together with the client-side encoding work for Jersey and OkHttp? I can implement it in either place, with a response-level regression for both entry points and a note on the remaining client limitations. If it belongs here, please also let me know which realm value you would like to use.

[arshilkxwork]

The Server Compatibility CI failure at 7e048d1 is in MultiNodeShortestPathApiTest.testPost on the macOS Intel runner, which timed out after about 33 seconds with "Interrupted, maybe it is timed out or uses too much memory". The same test failed in the same way on the Intel runner in run 38093844679 for another branch, and the remaining jobs at this commit, including the Apple Silicon macOS job, passed. It appears to be unrelated to this change. Could a maintainer re-run that job when convenient?

[bitflicker64]

Blocking: no. Summary: The filter and the Cypher endpoint now decode Basic credentials as UTF-8 with the standard Base64 alphabet and split on the first colon, which matches WsAndHttpBasicAuthHandler, so all three Server parsers agree on charset, Base64 alphabet and split rule; the only new point is a Helm README pointer to the TODO this PR deletes. Evidence: exact-head diff at 7e048d1 and the surrounding code of AuthenticationFilter.authenticate, CypherAPI.toUserPass and WsAndHttpBasicAuthHandler.parseBasicCredentials; git grep for every Basic-credential parser under hugegraph-server, hugegraph-pd and hugegraph-store; CI at this head passes server_rocksdb, pd_store / hstore, server_hbase and helm / lint-and-render, and the one failing job (build-server-macos-rocksdb (macos-15-intel)) fails in MultiNodeShortestPathApiTest.testPost with BackendException: Interrupted, maybe it is timed out or uses too much memory, outside the code changed here. No tests were rerun locally.

[imbajin]

Blocking: no. Summary: The REST and Cypher parsers now cover UTF-8, first-colon splitting and standard Base64 credentials; one test cleanup issue is noted inline. Evidence: exact-head diff at cd10ba8 and BaseApiTest.clearData().

[imbajin]

🧹 Blocking: no. Summary: This helper discards each DELETE response, so a failed final cleanup can leave fixed-name auth records behind while the test succeeds. Evidence: RestClient.delete() returns a Response, and BaseApiTest.clearData() clears only graph/schema data. Please assert each deletion's expected status while continuing the remaining cleanup, so auth cleanup failures fail the test.

[arshilkxwork]

Thank you. In c9339f0, deleteAuth attempts every deletion and then fails the test if any of them did not return 204, listing the affected records. A cleanup failure is therefore no longer silent, and the remaining records are still removed. testBasicAuthWithNonAsciiOrColonPassword in LoginApiTest now asserts 204 on its user deletion for the same reason.

[bitflicker64]

Blocking: no. Summary: At this head the REST filter, the Cypher endpoint and WsAndHttpBasicAuthHandler all decode Basic credentials as UTF-8 with the standard Base64 alphabet and split on the first colon, and the Helm README note from the last round is fixed; one point not raised earlier is that the 400 paths and the two documented compatibility changes have no automated test. Evidence: exact-head diff 0f86e2a..cd10ba8 (6 files) and the surrounding code of AuthenticationFilter.authenticate, CypherAPI.toUserPass and WsAndHttpBasicAuthHandler.parseBasicCredentials; 7e048d1..cd10ba8 changes only helm/hugegraph/README.md; git grep at the head for how hugegraph-test sends Basic credentials (HttpAuthenticationFeature in BaseApiTest.RestClient plus the two hand-built headers); old and new split rules run side by side on JDK 17.0.20.1. CI at this head: ApiTestSuite passed on HBase (182 run, 13 skipped) and on HStore (176 run, 52 skipped). server_rocksdb (Java 17) is red in the Run api test step before any test starts: the jacocoagent.jar download fails with curl: (28) Failed to connect to github.com port 443, so the API tests did not run on RocksDB at this head and that job needs a re-run; affected-module-tests is red only because it reports that job. No tests were rerun locally.

[bitflicker64]

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.

[arshilkxwork]

Thank you. c9339f0 adds LoginApiTest#testBasicAuthWithMalformedCredential, which sends hand-built headers. After a control request with test:test that returns 200, it expects 400 for admin, admin: and :pa, and 401 for test:test: and test::, using the user the class creates in setup(). The two trailing-colon cases correspond to the compatibility notes in the description.