Skip to content

[Bug]: sanitizePublicComment leaks raw scores in estimated-score wording #929

Description

@philluiz2323

[Bug]: sanitizePublicComment leaks raw score numbers in "estimated score N → M" — the exact wording the score engine emits

Summary

sanitizePublicComment (the shared public-safety sanitizer) redacts the raw
score-transition numbers (N -> M) only for the effective/projected
prefixes — but not for the estimated score prefix, which is the exact
wording buildGateDeltas actually emits. So a gate-delta explanation like
"... changes estimated score 32.5 -> 41.2." is only half-sanitized: the
words estimated score become private context, but the raw internal scores
32.5 -> 41.2 survive. These strings flow into every
gittensory_explain_score_breakdown / POST /v1/scoring/explain-breakdown
response via the score-breakdown's own public-safety sanitization layer — which
is explicitly there to neutralize exactly this.

The estimated score is something the product itself classifies as private
(estimated score, score estimate, scoreability are forbidden public terms),
so leaking the raw value is a scoreability leak by the project's own definition.

Evidence

The sanitizer strips the numeric transition only for two prefixes:

// src/github/commands.ts — sanitizePublicComment
.replace(/\b(?:effective|projected) score(?: changes?)?\b(?:\s+from)?\s+[-+]?\d+(?:\.\d+)?\s*(?:->||to)\s*[-+]?\d+(?:\.\d+)?/gi, "private context")  // line 1487
...
.replace(/\b(public score estimate|estimated score|score estimate|...)\b/gi, "private context")  // line 1489 — replaces the WORDS only, leaves the numbers

The score engine emits the estimated score wording (not effective/projected):

// src/scoring/preview.ts — buildGateDeltas (lines 642, 652, 662)
explanation: `Open PR pressure changes estimated score ${current.scoreEstimate.estimatedMergedScore} -> ${best.scoreEstimate.estimatedMergedScore}.`,
explanation: `Credibility changes estimated score ${current...} -> ${best...}.`,
explanation: `Linked issue/no-issue context changes estimated score ${current...} -> ${linked...}.`,

Running the real sanitizer on the real engine wording (verified by direct execution):

"Open PR pressure changes estimated score 32.5 -> 41.2."
  => "Open PR pressure changes private context 32.5 -> 41.2."   ← raw 32.5 -> 41.2 LEAKS

// vs the tested prefixes, which are fully redacted:
"projected score changes 12.3 -> 45.6"  => "private context"     ✓

These explanations are surfaced through the score-breakdown's per-string
sanitization (whose presence proves the design intent that the breakdown be
public-safe):

// src/services/score-breakdown.ts
function gateHighlightsFor(preview) {
  return preview.gateDeltas.map((delta) => ({
    gate: delta.gate,
    explanation: sanitizePublicComment(delta.explanation),   // ← line 182: half-redacts the estimated-score string
  }));
}

Exposed via the MCP tool gittensory_explain_score_breakdown and the route
POST /v1/scoring/explain-breakdown.

Why it's wrong

The sanitizer's documented contract — encoded in test/unit/github-commands.test.ts
— is that the score-transition numbers must be redacted:

expect(sanitizePublicComment("projected score changes 12.3 -> 45.6")).not.toMatch(/projected score changes|12\.3|45\.6/i);  // line 157
expect(sanitizePublicComment("effective score 0 -> 42")).not.toMatch(/effective score|0|42/i);                              // line 158

Both passing cases use the two prefixes line 1487 handles. The test never
covers the estimated score prefix
— which is the wording buildGateDeltas
emits — so the gap shipped green. The sanitizer's whole job is to make these
strings public-safe for reuse; it fails its contract for the one prefix the
engine actually produces.

Reachability

Every gittensory_explain_score_breakdown / /v1/scoring/explain-breakdown
response whose preview has gate deltas carries the half-redacted string with the
raw scores. The same sanitizePublicComment is the shared sanitizer for other
surfaces (e.g. scenario summaries), so any current or future reuse of a gate-delta
string on a genuinely cross-user/public surface leaks the numbers too.

Suggested fix

Generalize the number-stripping regex (line 1487) to cover the estimated
prefix the engine emits — and ideally any prefix:

.replace(/\b(?:effective|projected|estimated) score(?: changes?)?\b(?:\s+from)?\s+[-+]?\d+(?:\.\d+)?\s*(?:->||to)\s*[-+]?\d+(?:\.\d+)?/gi, "private context")

Even more robust: after the phrase replacements, add a catch-all that redacts any
residual bare score transition left behind:

.replace(/\bprivate context\b\s+[-+]?\d+(?:\.\d+)?\s*(?:->||to)\s*[-+]?\d+(?:\.\d+)?/gi, "private context")

Add a regression test asserting sanitizePublicComment("estimated score 12.3 -> 45.6")
(and the full buildGateDeltas wording) drops 12.3/45.6.

Test status

Not covered — and the suite gives false confidence: the contract test asserts the
number-redaction only for the effective/projected prefixes, never the
estimated score prefix the engine actually uses, so the leak is invisible to CI.

Confidence note

Moderate-high. The leak is proven by executing the real sanitizer on the real
engine wording, the strings demonstrably flow through the score-breakdown's
public-safety layer, and it violates the sanitizer's own tested contract. Honest
caveat on severity: the primary current consumer (/v1/scoring/explain-breakdown)
is self-scoped (requireContributorAccess), so today a contributor sees their own
estimated score (which /v1/scoring/preview also returns). But the sanitizer is
designed to redact these (every breakdown string is run through it), the leaked
value is a product-defined private "scoreability" number, and the shared sanitizer
serves other surfaces — so this is a genuine defect in the public-safety boundary,
exactly the class the project hardens proactively (#913 broadened this same
function to redact rewards?/rankings?).

Distinct from prior reports

Unrelated to the manual-retrigger PR-files refresh, predicted-gate, gate-403, or
BYOK. It is the same public-safety-sanitizer family as #913/#907/#904/#902/#909
but a distinct, unaddressed gap (the estimated score numeric-transition prefix).

Metadata

Metadata

Assignees

No one assigned

    Labels

    slopAI slop and/or attempts to game additional points via manipulation or alt profiles.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions