Skip to content

Track the 5 open v1 security PRs (board #11 issues-only cleanup) #1929

Description

@cliffhall

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 state parameter #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 unvalidated state is 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.open with _blank #1190 — closed (closed-v1-deprecated). Finding is real — the implicit
    noopener browsers 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-Origin is 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 in safeProxyFetch is 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:

Contributors' branches keep the commits pushed during review; nothing was merged.

Activity

  1. added
    choreMaintenance: deps, build tooling, CI, cleanup — no user-facing behavior change
    on Aug 5, 2026
  2. cliffhall commented on Aug 20, 2026

    @cliffhall
    MemberAuthor

    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/main ships fixes for high-severity vulnerabilities only). Issue body updated with the reasoning per PR.

    PR Call Label
    #1161 closed — reviewed, declined on the merits closed-v1-security-declined
    #1189 closed — not a vulnerability (PKCE covers it); half-built mechanism closed-v1-deprecated
    #1190 closed — real but bounded; tab navigation only closed-v1-deprecated
    #1296 closed — local console exposure; hygiene, not remote closed-v1-deprecated
    #1732 open — plausibly clears the bar; needs a merge call —

    #1732 is the outlier: a DNS-rebinding TOCTOU in safeProxyFetch is an SSRF-guard bypass that can reach 169.254.169.254, with the threat model being a hostile MCP app inducing /fetch rather 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 pinned lookup hook ignored Node's {all: true} shape, so /fetch could only reach literal-IP targets) and stopped the test suite shipping inside the npm tarball.

    Leaving this issue open until #1732 is decided.

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

    choreMaintenance: deps, build tooling, CI, cleanup — no user-facing behavior changev1

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions