fix(mcp): totals that only counted the page, and an XSS rule that could not read escaping #5610
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.
⮌ Merged
This pull request was merged into main.
c comment · e edit title · m merge · a approve · r request changes · ? shortcuts
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 —
totalcounted the page, not the matchesEvery listing tool returned
total: rows.length. With a hard cap of 50 and nooffset,gluecron_list_prsreportedtotal: 50on 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", gottotal: 50, then fetched PR #74 directly and got a real merged PR that was not in that listing.totaltrackinglimitis 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_prstakeoffset, return the true matched count from aCOUNT, and carryreturned/hasMore/nextOffset.src/lib/mcp-paging.tsrather than written four times — the reasonstate-filters.tsexists.limit: 0returned one row andlimit: -5returned zero: three behaviours for equally invalid values. Both refused now; over-cap is still clamped, since asking for more than we serve is reasonable andhasMoresays 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:
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.Adversarial cases from the GateTest engine
That session hit the same problem independently and its own predicate failed two of these. Kept as tests:
escapeHtml(a + b)split('+')cuts through the escaper's own argumentsescapeHtml(name;escapeHtml(a) + b"a + b" + userInput+inside a literal must not split the expressionOurs 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
ops-dashboard.ts: 8 → 3.