[ViperJuice]

Summary

This PR adds a detailed codebase review document (plans/codebase-review-2026-09-01.md) that audits the PMCP gateway codebase at commit d743bc4 (v2.7.3). The review is a read-only analysis with no production code changes—the document itself is the sole artifact.

Overview

The review documents the current state of the codebase across multiple dimensions:

• Security findings (18 items): Including agent-chosen npm package execution, .env leakage to downstream servers, untrusted repository configuration, HTTP/JWKS authentication defects, and feedback submission exfiltration
• Correctness findings (16 items): Server-initiated request handling gaps, cancelled request leaks, UTF-8 decoding failures, missing cancellation notifications, and schema drift
• Performance findings (5 items): Uncached manifest loading (~200ms per call), lifecycle lock contention across retries, blocking I/O on event loop, and registry fetch inefficiencies
• Maintainability findings (11 items): Code concentration in three large files, missing type strictness, test hermiticity issues, and architectural improvements
• Test/CI findings (2 items): Non-hermetic tests reaching npm registry and reading home config, plus release workflow input validation gaps

Key Sections

1. Executive Summary: Highlights material improvements since June review (redaction leaks, unguarded connect path, orphaned tasks all fixed) and identifies remaining trust boundary risks
2. Baseline: Documents clean lint/type checks (85% coverage, green CI), with two baseline observations about slow tests and dependency advisories
3. Detailed Findings: 18 security, 16 correctness, 5 performance, 11 maintainability, and 2 test/CI findings with evidence, impact, fixes, and effort estimates
4. Top 10 Actions: Prioritized table of immediate work items with effort levels
5. Strengths: Section 8 documents what the codebase does well
6. Work Order: Section 9 suggests implementation sequence

Notable Details

• Every CRITICAL/HIGH/MEDIUM finding is either reproduced with a script (listed in Appendix B) or traced end-to-end with line numbers
• Severity is relative to PMCP's threat model (local-first gateway also run as shared HTTP service)
• Includes reproducible examples for key findings (e.g., arbitrary npm package execution, .env leakage, UTF-8 crashes)
• Effort estimates use S (≤half day), M (≤few days), L (phase) scale
• References previous review (2026-06-15) and tracks which items are now fixed

https://claude.ai/code/session_014tAYaQ1aLW7iqps1zaXjVm

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[ViperJuice]

Merging. Spot-verified a sample of the findings against the tree rather than taking them on trust; the ones I checked hold:

• S-02 — confirmed at src/pmcp/tools/handlers.py:2942: _check_api_key_available calls load_dotenv(Path.cwd() / ".env") as a side effect of a lookup, mutating the gateway's process environment.
• S-09 — confirmed empirically: hmac.compare_digest('Bearer é', …) raises TypeError: comparing strings with non-ASCII characters is not supported, and the call at src/pmcp/transport/http.py:477 is not wrapped, so it is a 500 rather than a 401.
• D-01 — confirmed by pip-audit, and it is more urgent than its position in the top-ten suggests. See below.

One correction of emphasis. D-01 is listed tenth as "refresh uv.lock", but the pinned PyJWT 2.10.1 carries PYSEC-2026-176, a verifier-side algorithm allow-list bypass in jwt.decode() — and src/pmcp/auth.py:512 is exactly jwt.decode(..., algorithms=list(allowed_algorithms)). SECURITY.md states that signatures are only accepted for the operator-configured algorithm allowlist and that the token's own alg header is never trusted; that control is currently undermined by the dependency. Also present: PYSEC-2026-175 (PyJWKClient passing its uri to urllib.request.urlopen, adjacent to the #210/#211 JWKS work) and PYSEC-2026-177 (a JWKS refetch forced by unknown-kid traffic — which is this review's own S-07, reachable through the dependency as well as the gateway). pyjwt[crypto]>=2.10.0 is also unbounded below.

Being handled first, as its own PR, with pip-audit added to CI so it cannot recur silently.

Staleness note: this reviews d743bc4. #223 and #225 have landed since, so S-18 is partly addressed — client/manager.py no longer interpolates caught exceptions (the remaining instances are in identity.py, cli.py, policy/policy.py and config/loader.py). T-findings should be reconciled with #226, already filed.

[ViperJuice]

Thanks for the spot-checks. One clarification on the PyJWT emphasis, since it affects what the follow-up PR needs to guarantee:

PYSEC-2026-176 (GHSA-jq35-7prp-9v3f) is the verifier-side allow-list bypass when jwt.decode() is called with a PyJWK key object. In the locked 2.10.1 the substitution happens only when both algorithms is None and isinstance(key, PyJWK); otherwise the alg not in algorithms check stays in force (jwt/api_jws.py::_verify_signature). PMCP meets neither condition: _select_jwk_key (src/pmcp/auth.py:465-476) returns key.key, the underlying cryptography public key, and the jwt.decode call at :512 always passes algorithms=list(allowed_algorithms). So the SECURITY.md statement about the algorithm allowlist holds on the current tree, which is why D-01 marks that advisory "not reachable: PMCP passes raw keys".

The upgrade is still right: PYSEC-2026-120 (crit header unvalidated, fixed in 2.12.0) is reachable by any verified token, and the >=2.10.0 lower bound should become >=2.13.0. PYSEC-2026-175 and -177 live in PyJWKClient, which PMCP does not use (it has its own AsyncJWKS); S-07 is PMCP's own copy of the -177 pattern and needs its own fix regardless of the dependency bump.

Agree on the rest: the client/manager.py instances behind S-18 are gone after #225, and the T-findings should be reconciled with #226.

---

Generated by Claude Code