fix(server): return 401 instead of 500 when an API key contains non-ASCII characters#1719
Merged
Merged
Conversation
secrets.compare_digest raises TypeError for str arguments containing non-ASCII characters. Any request with a non-ASCII Bearer token or x-api-key header turned every authenticated endpoint into an unhandled 500 instead of a 401, and a configured non-ASCII key did the same. Add compare_keys to admin/auth.py, which compares UTF-8 bytes and keeps the constant-time guarantee, and route all key comparisons through it. Encode with surrogatepass so lone surrogates, which json.loads can produce and which delete_sub_key compares without a validation gate, also return False instead of raising. Fixes jundot#1717
19b95e8 to
7c5035b
Compare
Owner
|
Thanks for the fix. I verified the failure path: the public API auth dependency and the admin key comparisons now go through compare_keys, and the remaining compare_digest usage is contained inside that helper. The surrogatepass handling also covers the JSON-route edge case without changing normal ASCII behavior. This looks good to me, and I'm going to merge it. |
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
2 participants
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.
Fixes #1717.
Problem
Any request whose Bearer token or
x-api-keyheader contains a non-ASCII character makes every authenticated endpoint return HTTP 500 with "comparing strings with non-ASCII characters is not supported". The report shows this on both/v1/modelsand/v1/chat/completions.Root cause
This is not an inference or KV cache problem.
/v1/modelsfails too, and that endpoint never touches the engine.verify_api_keyinserver.pypasses the client-supplied key intosecrets.compare_digest(str, str)inomlx/admin/auth.py. CPython'scompare_digestraisesTypeErrorwhen eitherstrargument contains non-ASCII characters, and FastAPI surfaces that as an unhandled 500.A non-ASCII key is easy to produce by accident; pasting a key through anything that substitutes smart quotes is enough.
validate_api_keyalso accepts non-ASCII printable characters, so a configured main key or sub key can trigger the same TypeError on every request.Fix
Added
compare_keystoomlx/admin/auth.py. It encodes both sides as UTF-8 and compares bytes, whichcompare_digestaccepts for any content, and keeps the constant-time property. All key comparisons (verify_api_key,verify_any_api_key, and the sub-key duplicate and delete checks inadmin/routes.py) now go through it. Removed the now-unusedsecretsimport inroutes.py.Encoding uses
surrogatepassbecausejson.loadscan produce lone surrogates from escape sequences, and strict UTF-8 encoding rejects those. That matters fordelete_sub_key, which comparesrequest.keywithout avalidate_api_keygate; with strict encoding that route would have kept its 500.Testing
tests/test_api_auth.py(helper behavior for non-ASCII and lone surrogates on either side, and the server auth dependency returning 401, not TypeError, for a non-ASCII bearer) and 1 intests/test_admin_api_key.py(delete_sub_keywith a lone-surrogate key returns 404, not 500).--api-key, sent non-ASCIIBearerandx-api-keyvalues to/v1/modelsand/v1/chat/completions. All returned 401 with the standard error body, the correct key returned 200, and the server log contained no unhandled errors.One thing I could not reproduce is the "first request works, then everything fails" sequence in the report. The 500 is per-request and depends only on the bytes the client sends. My read is that the failing requests carried a non-ASCII key the whole time and the successful first request used a different stored credential. Either way the fix removes the 500.
Scope note
This does not make configured non-ASCII keys usable over HTTP. Starlette decodes header values as latin-1, so a client sending UTF-8 non-ASCII bytes will never match a configured non-ASCII key; it now gets a clean 401 instead of a 500. If you would rather also reject non-ASCII keys at creation time (400 going forward, migrate existing, the same shape as the hot-cache "auto" fix in #1612), I am happy to do that as a follow-up.