Repository navigation
Fix substring matches in WWW-Authenticate parsing - #3041
Whning0513 wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness issue in extract_field_from_www_auth where an auth-param name could be matched as a substring of a different param name (e.g., scope incorrectly matching inside error_scope), and adds regression coverage to prevent reintroducing the bug.
Changes:
- Anchor auth-param name matching to the start of the header or a parameter separator to avoid substring shadowing.
- Escape
field_namein the regex to ensure literal matching. - Add regression test cases covering both “shadowing” (real param present but a decoy exists) and “false positive” (only decoy param exists).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/mcp/client/auth/utils.py |
Updates extract_field_from_www_auth regex to require a real auth-param name boundary and escape the field name. |
tests/client/test_auth.py |
Adds regression cases ensuring substring-shadowing and substring-only params no longer match. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Followed up on two additional parser edge cases. This now keeps the first Bearer challenge when multiple Bearer challenges appear in one WWW-Authenticate header, and it accepts auth-params written with optional whitespace around = (for example |
fede-kamel
left a comment
There was a problem hiding this comment.
I differential-tested this against main and the two sibling PRs for #3009 (#3012, #3050) — full matrix in #3009 (comment).
Within Bearer challenges this behaves correctly in every probe I ran, including multi-challenge headers (Basic …, Bearer …) that regress in #3012. Two findings worth a look:
1. Silent contract change for non-Bearer lookups. Scoping extraction to the Bearer challenge is arguably the right semantics for the two current callers (scope per RFC 6750, resource_metadata per RFC 9728) — but extract_field_from_www_auth is a generic helper, and this changes its observable behavior:
extract_field_from_www_auth(r('Newauth realm="apps"'), "realm")
# main -> "apps"; this branch -> NoneIf Bearer-only is the intended direction, I would make it explicit (docstring at minimum, or fold the Bearer scoping into the two RFC-specific wrappers instead of the generic function) so the semantics are a reviewed decision rather than a side effect.
2. The quote-toggle splitter does not track escapes. _split_www_authenticate_segments flips in_quotes on every ", including RFC 9110 \" escapes, so an unpaired escaped quote before a comma still mis-splits the value (same limitation as main; #3012 handles this case). Since this PR already introduces a real parser, handling \" there would be a small addition that makes the new machinery strictly better than the regex it replaces.
Also flagging for the maintainers that #3050 fixes the same issue with a one-line boundary anchor and no semantic changes, in case the preference is to land the minimal fix first and layer challenge-scoping separately.
|
Thanks for the detailed differential testing. I addressed both findings in d4cfbc5:
I also added coverage for challenge boundaries, empty segments, empty quoted values, optional whitespace, and escaped quoted strings. The focused auth suite reports 143 passed and 1 xfailed; |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Thanks for the PR, and sorry it sat here without a proper review. We're closing most of the open PR backlog. v2 is out and changed a lot of the SDK, so many older PRs no longer apply as written, and we're a small team that realistically doesn't have the capacity to work through the rest. If this still matters to you on v2, the most useful thing you can do is open an issue (or comment on the existing one) with your use case and a repro. Hearing why it matters to you is what we use to decide what to prioritise. |
Fixes #3009.
extract_field_from_www_authcurrently matchesfield_nameas a substring of another auth-param name, so fields likescopecan be shadowed byerror_scopeandresource_metadatacan be shadowed byx_resource_metadata.This change parses comma-delimited auth parameters without splitting quoted values, preserves the generic helper's support for non-Bearer schemes, and scopes the RFC-specific
scopeandresource_metadatahelpers to the first Bearer challenge. It also handles quoted-pair escapes and optional whitespace around=.Tested with:
uv run --frozen pytest tests/client/test_auth.py -q(143 passed, 1 xfailed)src/mcp/client/auth/utils.py(100% statements and branches)uv run --frozen ruff check src/mcp/client/auth/utils.py tests/client/test_auth.pyuv run --frozen ruff format --check src/mcp/client/auth/utils.py tests/client/test_auth.pyuv run --frozen pyright src/mcp/client/auth/utils.py tests/client/test_auth.pyAI assistance was used for parser edge-case analysis and test review; I reviewed and validated the resulting changes.