Skip to content

auth: four reproducible defects on the HTTP/JWKS surface (S-07..S-10) #231

Closed

Description

@ViperJuice

From the 2026-09-01 codebase review (plans/codebase-review-2026-09-01.md). Four defects on one surface, all rated MEDIUM, all "S" effort — a good plan-detailed cluster rather than four separate changes.

  • S-09 — a non-ASCII byte in Authorization is a 500, not a 401. I verified this. src/pmcp/transport/http.py:477 calls hmac.compare_digest(incoming, f"Bearer {auth_token}") with no try. Confirmed empirically: hmac.compare_digest('Bearer é', 'Bearer x') raises TypeError: comparing strings with non-ASCII characters is not supported. An unauthenticated caller can therefore turn any request into a server error by sending one non-ASCII byte. Fix: compare bytes, not str.
  • S-07 — unknown-kid tokens force a JWKS refetch per request, and rate limiting runs after auth. An unauthenticated client can drive outbound fetches at will. Partially mitigated from the dependency side by fix(deps): close the advisories on the auth path and fail CI on new ones (D-01) #228 (PyJWT 2.13.0 closes PYSEC-2026-177), but PMCP's own missing cooldown and the auth-before-rate-limit ordering are still ours to fix.
  • S-08 — the JWKS fetch has no explicit timeout and runs under the refresh lock, so it inherits aiohttp's 300 s default while holding the lock.
  • S-10 — protected-resource metadata advertises a Host-derived resource and skips the Host allowlist (the review rates this HIGH when allowed_origins is configured).

These are worth doing together because they share one file and one threat model, and because S-07 and S-10 interact: the metadata route is the cheapest place to provoke the fetch amplification.

Care needed: #210 and #211 already reworked this area (IP-literal classification, and no longer vouching for URLs PMCP did not verify). Any fix must not re-introduce the "we validated this" implication that #211 removed — see the verified/url_verified fields and the caveat threaded into next_step.

Activity

  1. ViperJuice commented on Oct 1, 2026

    @ViperJuice
    CollaboratorAuthor

    All three items in this issue's scope are fixed on main by #325 (merged as 89559db), per the plan merged in PR 284: S-07 (forced JWKS refresh limited to one per 10 s, re-checked under the lock; 12,000 unknown-kid requests over 60 s caused 6 fetches in review), S-08 (5 s total fetch timeout plus a shared failure backoff; 20 waiters on a hung endpoint caused 1 fetch), S-10 (metadata resource from configuration only; no request header changes it). The panel also found and #325 fixed an unauthenticated 500 from a forged alg/key mismatch (now 401). Wording nits: #326.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions