Repository navigation
Track the 5 open v1 security PRs (board #11 issues-only cleanup) #1929
Description
Summary
Five open security PRs target v1/main and were the last items still sitting on
project board #11 as PR cards. Board #11 is issues-only (see AGENTS.md →
Issue-driven Work Style), and the umbrella they each reference — #1819,
"go-live 7/9: triage and bulk-close the v1 PR backlog" — is closed, so
removing those cards would have left this work with no board tracking at all.
This issue is that tracking. The PR cards were removed from #11; each PR
below is reachable from here.
Decisions — 4 of 5 resolved
Each PR was put through repeated Copilot review rounds until clean, then given
an explicit call. The governing rule: v1/main is deprecated and ships fixes
for high-severity vulnerabilities only — not general hardening,
defense-in-depth, or hygiene work, however sound.
- Security: OAuth callback does not validate
stateparameter #1189 — closed (closed-v1-deprecated). Not a vulnerability. v1 already
implements PKCE (saveCodeVerifier/codeVerifier+ SDK 1.25.2 sending
code_challenge), which defeats code injection; the unvalidatedstateis a
half-built mechanism (state()generates and sends a token that is then
discarded), so completing it is hardening. v2 reached the same conclusion
independently and does a shape check only. - Security: Potential reverse tabnabbing via
window.openwith_blank#1190 — closed (closed-v1-deprecated). Finding is real — the implicit
noopenerbrowsers apply to<a target="_blank">does not extend to
window.open()— but it is bounded by a hostile-server-plus-user-click
precondition and yields tab navigation, not code execution or data access.
v2 is unaffected; nothing to forward-port. - fix(server): redact sensitive env vars and headers from connection logs #1296 — closed (
closed-v1-deprecated). Legitimate finding, and the review
rounds sharpened it from a name-pattern denylist into unconditional
redaction. But the exposure is local — the user's own console — so it is
hygiene, not a remotely exploitable vulnerability. - fix: reject requests with missing Origin header in origin validation middleware (CWE-346) #1161 — closed (
closed-v1-security-declined). Reviewed and declined on the
merits: every origin-validated route also requires the session token, so
missing-Originis not an unauthenticated bypass, and rejecting it breaks
non-browser callers (curl/CI) while putting v1 at odds with v2's documented
posture. - fix: prevent DNS-rebinding TOCTOU in safeProxyFetch by pinning resolved IPs #1732 — kept open. The one PR that plausibly clears the bar. A DNS-rebinding
TOCTOU insafeProxyFetchis an SSRF-guard bypass reachable at the cloud
metadata service (169.254.169.254) — the threat model being a hostile MCP
app inducing/fetch, not a token-holding attacker. Closing it under a
"high-severity only" rationale would contradict itself. Needs a merge call.
Notes carried out of the review
Found during review and not addressed, since none is v1-shippable:
- Same log-leak class as fix(server): redact sensitive env vars and headers from connection logs #1296 remains in the STDIO/SSE paths —
command=/args=
logged verbatim and the SSEurl=, which can carry embedded credentials. Worth
an issue against v2. client/src/components/OAuthFlowProgress.tsx:381has the same barewindow.open
as Security: Potential reverse tabnabbing viawindow.openwith_blank#1190 (its sibling at :251 is already hardened). v1 only.- Fork PRs on this repo run no workflows at all (
gh pr checks→ "no checks
reported"), so nothing onv1/mainis verified by the repo's own gates. That
materially raises the risk of any change here and is part of why the bar is high. - Security: OAuth callback does not validate
stateparameter #1189's contributor commits carry a DCO name/email mismatch that would need a
force-push rewrite to repair. Moot now that it's closed.
Contributors' branches keep the commits pushed during review; nothing was merged.
Activity
docs: make triage runnable — state-based test, both-board sweep, boar…
cliffhall commentedon Aug 20, 2026
Dispositions recorded — 4 of 5 closed, #1732 kept open.
Each of the five went through repeated Copilot review rounds until a review came back clean, then got an explicit call against the governing rule (
v1/mainships fixes for high-severity vulnerabilities only). Issue body updated with the reasoning per PR.closed-v1-security-declinedclosed-v1-deprecatedclosed-v1-deprecatedclosed-v1-deprecated#1732 is the outlier: a DNS-rebinding TOCTOU in
safeProxyFetchis an SSRF-guard bypass that can reach169.254.169.254, with the threat model being a hostile MCP app inducing/fetchrather than a token-holding attacker. Closing it under a "high-severity only" rationale would have contradicted itself, so it stays open pending that decision. Note its review rounds also fixed a functionality-breaking bug in the PR (the pinnedlookuphook ignored Node's{all: true}shape, so/fetchcould only reach literal-IP targets) and stopped the test suite shipping inside the npm tarball.Leaving this issue open until #1732 is decided.