CodeIssuesPull RequestsActionsSecurityInsights
✨ AI
More
Settings

fix(mcp): totals that only counted the page, and an XSS rule that could not read escaping #5610

MergedXSccantynz wants to mergefix/mcp-paging-and-xss-accuracymainopened 1h ago
ccantynzcommented 1h ago

Second wave from the cross-platform test session with the Vapron and GateTest instances. Everything here was measured against a real 4,150-file repository, not reasoned about.

Paging — total counted the page, not the matches

Every listing tool returned total: rows.length. With a hard cap of 50 and no offset, gluecron_list_prs reported total: 50 on a repo with thousands of PRs, and everything past the fiftieth was unreachable.

The Vapron session proved it the only way it can be proved: listed PRs with state:"all", got total: 50, then fetched PR #74 directly and got a real merged PR that was not in that listing.

total tracking limit is the dangerous half. An agent that asks for everything, is told it received everything, and acts on a truncated set gets no signal that anything is missing. A cap is fine. A cap that reports itself as the total is not.

  • list_prs, repo_list_issues, search_issues, search_prs take offset, return the true matched count from a COUNT, and carry returned / hasMore / nextOffset.
  • Shared in src/lib/mcp-paging.ts rather than written four times — the reason state-filters.ts exists.
  • limit: 0 returned one row and limit: -5 returned zero: three behaviours for equally invalid values. Both refused now; over-cap is still clamped, since asking for more than we serve is reasonable and hasMore says what happened.

XSS rule — 8 findings, 8 false, now 3 and honest

The earlier suppression cleared constant literals and fully escaped templates (8 → 4). Reading the remaining four against the actual file rather than trusting the report showed two more gaps:

  • Concatenation was unhandled. out.innerHTML = '<div>' + escHtml(msg) + '</div>' is at least as common as interpolation and was reported in full. The RHS is now parsed as a + chain where every operand must independently be a safe literal or an escaper call, so partial escaping (escHtml(a) + b) still fires.
  • Multi-line statements were judged from their first line — a 20-line template was judged on `card.innerHTML = ``, a literal with no visible end, which the predicate must treat as unverifiable. The scanner now passes the whole statement. The line cap can only leave a false positive in place, never create a false negative.
  • Inline scripts inside a server-rendered page are themselves in a template literal, so their backticks arrive escaped and never parsed. Unescaped only when the RHS actually opens with an escaped backtick.

Adversarial cases from the GateTest engine

That session hit the same problem independently and its own predicate failed two of these. Kept as tests:

case required why
escapeHtml(a + b) SAFE a split('+') cuts through the escaper's own arguments
escapeHtml(name; not cleared unbalanced input must never be silenced — a false negative is invisible in a way a false positive is not
escapeHtml(a) + b fires partial escaping
"a + b" + userInput fires + inside a literal must not split the expression

Ours passes all four because it parses rather than pattern-matches.

Remaining 3 are honest

Two contain interpolations that are genuinely not escaper calls (${b.reachable ? 'up' : 'down'}, ${i+1}); one builds HTML through .map(), which needs dataflow this rule deliberately does not attempt. The "8/8 false" in the original report was slightly overstated — two of those four do carry unescaped interpolations, they simply are not attacker-controlled.

Verification

  • 32 unit tests on the predicate, including the four above.
  • Simulated against the real ops-dashboard.ts: 8 → 3.
  • Full CI-safe suite: 5,317 pass, 11 fail — identical to the pre-change baseline (local no-DB artifacts).
ccantynzAI Reviewcommented 1h ago

AI Triage

(no summary)

Priority: medium Risk area: mixed

Suggested labels: (no label suggestions) Suggested reviewers: (no reviewer suggestions)

Suggestions only — nothing has been applied. The PR author stays in control.

gluecron[bot]🤖 botAI Reviewcommented 1h ago

AI review unavailable

The platform's AI balance is exhausted, so AI generation is temporarily unavailable. Nothing was lost. You can queue this as a repair for the internal agent from the repository's Health page, or try again once the balance is restored. The PR is otherwise unchanged.

Cross-repo impact

See what breaks downstream if this PR merges.

Analyze →
⮌ Merged

This pull request was merged into main.

c comment · e edit title · m merge · a approve · r request changes · ? shortcuts