fix(request): handle multiple WWW-Authenticate header lines#463
Open
Rjected wants to merge 1 commit into
Open
Conversation
…lenge Servers that emit one `WWW-Authenticate` line per challenge (RFC 9110 §5.3) caused `tempo request` to drop every challenge except the last, surfacing a misleading "Unsupported payment method: <other>" when tempo was in fact offered. Read every value via a new `HttpResponse::headers_all`, and surface the underlying parse error when no supported method survives so a malformed tempo challenge is no longer masked by a sibling stripe one. Also drop the duplicate `headers_from_reqwest` call in `from_reqwest` that was appending every header twice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
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
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.
Summary
tempo requestwas silently dropping payment challenges when the origin server emittedWWW-Authenticateas separate header lines (one per challenge) rather than as a single comma-joined value. Both forms are valid per RFC 9110 §5.3 / §11.6.1, but our HTTP layer'sheader()helper returns only the last value for a name, so any challenge listed before the final one was silently discarded. When a server listedtempofirst and another method (e.g.stripe) second, the CLI failed with the misleading:even though the response advertised tempo.
Reproduction
Any MPP origin that emits one
WWW-Authenticateper challenge repros — e.g. PostalForm:Tempo's own demo origins merge their challenges into a single header, which is why this hadn't surfaced in-house.
Root cause
HttpResponse::headerwalksself.headers(a flatVec<(String, String)>) with.rev().find(...)and returns only the last matching value.reqwest::HeaderMap::iter()yields one entry per header line, so twoWWW-Authenticatelines land as twoVecentries and only the second reachesparse_payment_challenge.The local
split_payment_challengesalready handles the merged form correctly — the input is just missing the tempo half by the time it gets there.Fix
HttpResponse::headers_all(name)returning every value for a header in receipt order.header()keeps its existing last-value semantics (compatible with the only callers in tree,payment-receiptand friends).parse_payment_challengenow collects everywww-authenticatevalue viaheaders_all, joins them, and feeds them to the existing splitter. Both wire formats now behave identically.filter_map(|r| r.ok())silently dropped parse failures, so a malformedtempochallenge alongside a validstripechallenge surfaced asUnsupported payment method: stripe— exactly the symptom that made this bug hard to diagnose. When no supported method is found, we now surface the underlyingmppparse error viaPaymentError::ChallengeParseSource. TheUnsupportedPaymentMethodpath is preserved for the genuinely-no-tempo case.HttpResponse::from_reqwestwas callingheaders_from_reqwest(response.headers())twice (once before reading the body, once after) andextending the result, so every header ended up in theVectwice. Removed the second call —reqwest's response headers don't change between header receipt and body read, and trailers go through a separate API. Doesn't affect the bug above (duplicates of the last header are still all the last header) but worth fixing in the same change.Tests
Three new regression tests in
query::challenge:test_parse_payment_challenge_separate_headers_tempo_first— twoWWW-Authenticateheaders, tempo listed first, selects tempo.test_parse_payment_challenge_separate_headers_stripe_first— same but stripe first; this is the actual repro and previously failed withUnsupported payment method: stripe.test_parse_payment_challenge_surfaces_parse_error_over_unsupported_method— single header containing a deliberately malformed tempo challenge plus a valid stripe challenge; asserts the error mentions the parse failure rather thanUnsupported payment method: stripe.Plus two
headers_allunit tests inhttp::response.Test plan
cargo test -p tempo-request— 344 passed, 0 failed (149 lib + 182 + 13 in two integration suites; all three new regression tests included).cargo clippy -p tempo-request --tests -- -D warnings— clean.tempo requestagainst a server emitting per-challengeWWW-Authenticatelines (e.g.https://postalform.com/api/machine/mpp/orders) and confirm the tempo challenge is selected.🤖 Generated with Claude Code