fix: reject stray trailing token after a valid auth-param in AuthChallengeParser#142
fix: reject stray trailing token after a valid auth-param in AuthChallengeParser#142OmarAlJarrah wants to merge 1 commit into
Conversation
AuthChallengeParser accepted a malformed challenge of the shape `Scheme key=token <token>` — a bare token directly following an otherwise valid auth-param with no separating comma (e.g. `Bearer realm="x" garbage`). The continuation loop only kept the param list open while the next non-whitespace char was a comma, so it broke on the stray token and left the cursor parked on it. The outer parse loop then re-read that token as the scheme of a phantom second challenge, so `Digest realm=value extra` parsed into two challenges instead of being treated as a single malformed challenge. RFC 7235 §2.1 permits only a comma (another auth-param or the next challenge) or end-of-input after an auth-param; a bare token there has no production. The parser now skips the stray tail to the next top-level comma via recoverToNextChallenge and emits the challenge with the params parsed before the garbage, consistent with the parser's existing lenient "preserve prior params" recovery on malformed continuations. This stops the stray token from being silently dropped or misread as a phantom challenge scheme. Adds regression tests for the unquoted and quoted forms (`Digest realm=value extra`, `Digest realm="value" extra`) and for a stray token sitting between a valid param and a comma-separated next challenge. Closes #111
|
Looks good to merge. This closes a real lenient-recovery gap: after parsing a valid auth-param, breaking out of the loop on any non-comma character left the cursor parked on a stray trailing token, which the outer loop then misread as the scheme of a phantom second challenge. Routing through the existing Verified the recovery path emits the current challenge with the params parsed so far rather than discarding them, that quoted values aren't affected, and that a legitimate comma-separated second challenge is still parsed correctly. The three added tests cover the unquoted, quoted, and comma-separated-next-challenge cases, which is the right spread. No public API surface change, so no apiCheck impact. |
Problem
AuthChallengeParseraccepted a malformedWWW-Authenticate/Proxy-Authenticatevalue of the shapeScheme key=token <token>— a bare token directly following an otherwise valid auth-param with no separating comma, e.g.Bearer realm="x" garbage.The continuation loop kept the param list open only while the next non-whitespace character was a comma, so when it hit the stray token it broke out and left the cursor parked on that token. The top-level parse loop then read the stray token as the scheme of a phantom second challenge. As a result
Digest realm=value extraparsed into two challenges (digestwithrealm=value, plus a bareextra) instead of being treated as a single malformed challenge. The stray token was accepted without complaint either way, and no regression test pinned the behaviour, so it was free to drift.Change
RFC 7235 §2.1 permits only a comma (introducing another auth-param or the next challenge) or end-of-input after an auth-param; a bare token in that position has no grammar production. When the continuation loop now sees a non-comma, non-EOF character after a valid auth-param, it skips the malformed tail to the next top-level comma via
recoverToNextChallengeand emits the challenge with the params parsed before the garbage. This:Emitting the challenge with the previously-parsed params (rather than discarding the whole challenge) keeps the behaviour consistent with the parser's existing lenient recovery on malformed continuations, which already preserves prior params.
Tests
Added regression tests for:
Digest realm=value extra(unquoted) — one challenge,realm=value, no phantomextrascheme.Digest realm="value" extra(quoted) — same.Digest realm=value extra, Basic realm="x"— the stray token is skipped to the next comma and the followingBasicchallenge is still parsed.Gated build
Ran (worktree root, scoped to the touched module):
Result: BUILD SUCCESSFUL. No public-API change, so
apiCheckpasses against the committed.apisnapshot with noapiDumpneeded.Closes #111