fix(auth): constant-time password compare and UTF-8 credential decoding - #1291
fix(auth): constant-time password compare and UTF-8 credential decoding#1291Ro-28 wants to merge 1 commit into
Conversation
Three related fixes to the API password authentication, plus test coverage for a module that previously had none. Constant-time comparison: both password checks (the PasswordAuthMiddleware dispatch path and the check_api_password dependency) used `!=` on strings, which short-circuits on the first differing byte and leaks password length and prefix information through response timing. Both now use hmac.compare_digest on encoded bytes. UTF-8 credential decoding: ASGI servers decode header values as latin-1, so the credential string reaching the middleware was the latin-1 decoding of the client's wire bytes, while the configured password was encoded as UTF-8. A password containing characters outside latin-1 (e.g. CJK) could therefore never match, returning 401 with no way for any client to succeed. The new _credential_bytes() helper re-encodes the credential with latin-1 to recover the original wire bytes before comparison, and falls back to UTF-8 for credentials that did not arrive over HTTP. Note the wire-encoding change: clients must send UTF-8 for non-ASCII passwords. Pure-ASCII passwords are unaffected, since their latin-1 and UTF-8 encodings are identical. Docstring correction: PasswordAuthMiddleware claimed it was "always active with default password if OPEN_NOTEBOOK_PASSWORD is not set". The code does the opposite and disables authentication entirely when no password is configured. Adds tests/test_auth.py with 61 tests covering the middleware, the dependency, and the /auth/status endpoint: excluded paths, OPTIONS preflight, malformed headers, init-time vs call-time password reads, wire encodings, and the absence of password leakage in the status response. The tests build a bare FastAPI app and require no database or network. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPzTa2pWc3FenutyvuqUXu
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 3/5
- In
api/auth.py,check_api_passwordrejects Latin-1-representable non-ASCII passwords such aspässwördwith a 401, creating an authentication regression for affected users—make_credential_bytesuse a consistent encoding for comparison and add a regression test.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="api/auth.py">
<violation number="1" location="api/auth.py:132">
P2: A direct call to `check_api_password` with a Latin-1-representable non-ASCII password (e.g. `pässwörd`) now always returns 401. `_credential_bytes` Latin-1-encodes the string before comparing to the configured password's UTF-8 bytes, and those encodings differ, so the credential can never match. The UTF-8 fallback only fires for strings outside Latin-1, so a direct caller with a Latin-1-encodable password is broken while one with, say, CJK characters works — inconsistent. Before this change the function compared strings directly and the correct password matched. Since `check_api_password` has no way to distinguish a direct caller from an over-HTTP credential when the string is Latin-1 encodable, apply the UTF-8 re-encoding for the direct-call path (or gate the Latin-1 wire-byte recovery to the middleware, which always sees HTTP-decoded headers).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # Check password | ||
| if credentials.credentials != password: | ||
| if not hmac.compare_digest( | ||
| _credential_bytes(credentials.credentials), password.encode("utf-8") |
There was a problem hiding this comment.
P2: A direct call to check_api_password with a Latin-1-representable non-ASCII password (e.g. pässwörd) now always returns 401. _credential_bytes Latin-1-encodes the string before comparing to the configured password's UTF-8 bytes, and those encodings differ, so the credential can never match. The UTF-8 fallback only fires for strings outside Latin-1, so a direct caller with a Latin-1-encodable password is broken while one with, say, CJK characters works — inconsistent. Before this change the function compared strings directly and the correct password matched. Since check_api_password has no way to distinguish a direct caller from an over-HTTP credential when the string is Latin-1 encodable, apply the UTF-8 re-encoding for the direct-call path (or gate the Latin-1 wire-byte recovery to the middleware, which always sees HTTP-decoded headers).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/auth.py, line 132:
<comment>A direct call to `check_api_password` with a Latin-1-representable non-ASCII password (e.g. `pässwörd`) now always returns 401. `_credential_bytes` Latin-1-encodes the string before comparing to the configured password's UTF-8 bytes, and those encodings differ, so the credential can never match. The UTF-8 fallback only fires for strings outside Latin-1, so a direct caller with a Latin-1-encodable password is broken while one with, say, CJK characters works — inconsistent. Before this change the function compared strings directly and the correct password matched. Since `check_api_password` has no way to distinguish a direct caller from an over-HTTP credential when the string is Latin-1 encodable, apply the UTF-8 re-encoding for the direct-call path (or gate the Latin-1 wire-byte recovery to the middleware, which always sees HTTP-decoded headers).</comment>
<file context>
@@ -109,7 +128,9 @@ def check_api_password(
# Check password
- if credentials.credentials != password:
+ if not hmac.compare_digest(
+ _credential_bytes(credentials.credentials), password.encode("utf-8")
+ ):
raise HTTPException(
</file context>
lfnovo
left a comment
There was a problem hiding this comment.
Thanks for this, and for the careful write-up in the description. The branch is stale against main, and that changes the scope quite a bit:
secrets.compare_digestalready landed in #1003 (api/auth.py), so thehmac.compare_digesthalf is a no-op rename that now conflicts.- The docstring was corrected in #1026.
check_api_passwordwas removed in #1026 as dead code. The PR re-adds a fix to it, andtests/test_auth.pyimports it, so the whole test module fails to collect on main.
What's still new and correct is the latin-1 recovery of the wire bytes so non-ASCII UTF-8 passwords can authenticate. I confirmed the bug on main: credentials.encode("utf-8") against a latin-1-decoded header can never match a UTF-8 password. Once rebased, the except UnicodeEncodeError fallback becomes unreachable (a latin-1-decoded string is always latin-1-encodable), so _credential_bytes can collapse to a single encode("latin-1") or be inlined.
Could you rebase and re-scope to: the latin-1 fix in dispatch, the TestNonAsciiPassword class, and the TestPasswordAuthMiddleware / TestNoPasswordConfigured coverage (genuinely new, we have no auth tests today)? Drop the check_api_password classes. Please add a one-line CHANGELOG entry under Unreleased → Fixed. Keep in mind AGENTS.md frames this middleware as a dev default rather than production hardening, so smaller is better here.
Fixes the two authentication issues described in #1290, plus a docstring correction, and adds test coverage for a module that previously had none.
Changes to
api/auth.pyConstant-time comparison. Both password checks now use
hmac.compare_digeston encoded bytes instead of!=on strings, so response timing no longer varies with how much of the password prefix was correct.UTF-8 credential decoding. A new
_credential_bytes()helper re-encodes the incoming credential with latin-1 to recover the exact bytes the client put on the wire, and compares those against the configured password's UTF-8 bytes. A credential that is not latin-1 encodable did not arrive over HTTP, so it falls back to UTF-8 for direct callers ofcheck_api_password.Docstring.
PasswordAuthMiddlewarenow says it is disabled entirely whenOPEN_NOTEBOOK_PASSWORDis unset, which is what the code actually does.Behavior change worth reviewing
Clients must send UTF-8 for non-ASCII passwords. Previously, a client sending latin-1 wire bytes for a latin-1-representable password such as
pässwördwould succeed; it now returns 401.Pure-ASCII passwords are unaffected, since their latin-1 and UTF-8 encodings are identical — so this should be invisible to essentially every existing deployment. But it is a real change and I would rather flag it than bury it. If you would prefer to accept both encodings for backward compatibility, that is an easy adjustment.
Tests
Adds
tests/test_auth.pywith 61 tests covering the middleware, thecheck_api_passworddependency, andGET /auth/status:Authorizationheadersexcluded_pathsOPTIONSpreflight, and non-GET methods/auth/statusnever leaking the password valueThe tests build a bare FastAPI app rather than importing the production app, so they need no database and no network. Full suite: 283 passed.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KPzTa2pWc3FenutyvuqUXu