fix(intel): stop feed newlines forging rows in the three sibling prompt blocks (#5881) - #5897
Conversation
|
@Yigtwxx is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
3136638 to
5b3c5ab
Compare
|
Rebased onto Re-verified on the rebased tree:
The red On the overlap with #5895: that PR covers the three TypeScript prompt builders. This one covers those three plus Either way works for me. If you would rather take #5895 for the TypeScript side, I am happy to rebase onto it and reduce this to the seeder half. |
5b3c5ab to
707b235
Compare
|
Rebased onto Re-verified on the rebased tree: One thing worth flagging for triage: #5895, which covered the same issue, was closed on 3 Aug. This is now the only open fix for #5881. |
…locks (koala73#5881) sanitizeForPrompt preserves a lone newline by design: it splits on the newline to drop role-prefixed lines, rejoins, then collapses only runs of 2+ whitespace. That is right for prose bodies and wrong wherever the newline is the delimiter of the block being composed, where one feed-supplied newline forges an extra row the model reads as a separate, real datum. koala73#5857 closed this in chat-analyst-context.ts; these three siblings were declared out of scope there. - deduct-situation.ts: market titles are composed into `- "..."` rows under a "## Prediction Market Odds" header. A title can forge an extra market with an attacker-chosen probability and volume. - brief-why-matters-prompt.ts: fixed at sanitizeStoryFields rather than at the call site, because api/internal/brief-why-matters.ts:303 feeds the legacy buildWhyMattersUserPrompt from the same function and composes the same `Label: value` rows. description is the likeliest carrier -- free-form article body. - _country-brief-context.ts: the worst of the three. headlineLines interpolated item.title raw, so this block was missing both the delimiter guard and the koala73#3724 content sanitization. The sibling source lines are accidentally safe only because JSON.stringify escapes the newline. buildPredictionContext is exported so the guard can be driven through the real builder; the module imports cleanly, so no source-text harness is needed. Each guard is mutation-proven: neutering sanitizeForPromptLine back to sanitizeForPrompt turns 2, 3 and 1 cases red respectively, and dropping _country-brief-context's sanitization entirely turns 2 red. The tests assert the row count the payload declares rather than the absence of the payload string, which would pass for the wrong reason as soon as the string is echoed anywhere. Companion cases pin that ordinary values still render byte-identical.
The fourth site koala73#5881 flags. Same defect class, different runtime: scripts/lib/brief-llm.mjs is the Railway seeder's prompt builder, and both of its blocks compose rows and join them with a newline. buildDigestPrompt is the sharper of the two. It interpolated s.headline, s.category, s.country and s.source raw into `NN. [h:<hash>] [SEV] headline — category · country · source` rows, and the system prompt asks the model to key its output off the [h:<hash>] token. So a feed newline does not just add noise: it mints a numbered row carrying an attacker-chosen hash into the composed brief. buildStoryDescriptionPrompt is guarded at the composition site, not only through sanitizeStoryForPrompt. The production caller does pass sanitized fields, but a builder whose safety depends on its caller is one new call site away from a hole -- which is the lesson docs/solutions/conventions/mutate-each-call-site-a-global-mutant-hides-per-site-holes.md records from the koala73#5857 sweep. The Context: line is deliberately left on plain prose semantics. It is the block's one free-prose sink -- the article body, whose internal newlines are legitimate grounding text -- and tests/brief-llm.test.mjs:1788 locks that contract on purpose. That leaves a residual: Context sits in the same newline-joined block, so a body newline can still forge a trailing `Key: value` row. Closing it means rendering the body under its own structural header, a prompt-shape change rather than a sanitizer fix, so it is flagged rather than done silently. A test pins the current behaviour so the residual stays visible. Mutation-proven: reverting any of the six guards, or aliasing the import back to sanitizeForPrompt, turns the suite red -- no survivors.
707b235 to
5ae8bc2
Compare
Summary
Fixes #5881. Follows #5884, whose "Out of scope" section tracks exactly these modules.
sanitizeForPrompt(server/_shared/llm-sanitize.js:79) preserves a lone newline by design — it splits on\nto drop role-prefixed lines, rejoins, then collapses only runs of 2+ whitespace. That is correct for prose bodies and wrong wherever the newline is the delimiter of the block being composed. There, one feed-supplied\nforges an extra row the model reads as a separate, real datum.sanitizeForPromptLine(#5857) is the same sanitizer plus a full whitespace-run collapse; this applies it at the four line-composing sites the sweep left behind.1.
deduct-situation.ts:69— market titles are composed into- "${title}" — Yes ${pct}% (${vol} volume)rows and joined with\nunder a## Prediction Market Odds (crowd-calibrated)header. A title carrying a newline forges an extra market with an attacker-chosen probability and volume, inside a block the deduction prompt presents as crowd-calibrated evidence.2.
brief-why-matters-prompt.ts:270-278—Headline:/Description:/Source:/Severity:/Category:/Country:rows, joined with\n. Fixed insanitizeStoryFields(:37) rather than at the call site, becauseapi/internal/brief-why-matters.ts:303feeds the legacybuildWhyMattersUserPrompt(shared/brief-llm-core.js:75) from that same function, and it composes the identical row shape. One fix, both paths.descriptionis the likeliest carrier — it is free-form article body.3.
_country-brief-context.ts:210-213— the worst of the three.headlineLinesinterpolateditem.titleraw, so this block was missing both the delimiter guard and the #3724 content sanitization:The sibling
briefSourceContextLinestwo functions above is accidentally safe only becauseJSON.stringifyescapes the newline — nothing there is a deliberate guard.4.
scripts/lib/brief-llm.mjs— the site the issue lists as "also worth checking". Same class, different runtime: this is the Railway seeder's prompt builder.buildDigestPrompt(:508) is the sharpest instance in the whole set. It interpolateds.headline,s.category,s.countryands.sourceraw into`${n}. [h:${shortHash}] [${sev}] ${s.headline} — ${s.category} · ${s.country} · ${s.source}`and the system prompt asks the model to key its output off the
[h:<hash>]token. So a feed newline here does not merely add noise — it mints a numbered row carrying an attacker-chosen story hash into the composed brief.brief-compose.mjs:704does sanitize these fields upstream, but withsanitizeHeadline/sanitizeForPrompt, both of which keep a lone newline. The delimiter guard has to live where the delimiter is.buildStoryDescriptionPrompt(:288) is guarded at the composition site too, not only throughsanitizeStoryForPrompt. The production caller (:360) does pass sanitized fields, but a builder whose safety depends on its caller is one new call site away from a hole — which is the lessondocs/solutions/conventions/mutate-each-call-site-a-global-mutant-hides-per-site-holes.mdrecords from the #5857 sweep.What is deliberately left on prose semantics
The
Context:line inbuildStoryDescriptionPromptis not line-sanitized. It is the block's one free-prose sink — the article body, whose internal newlines are legitimate grounding text — and #5857's own rule is to keepsanitizeForPrompton prose.tests/brief-llm.test.mjs:1788locks that contract explicitly ("locks the contract so a future 'tidy whitespace' change doesn't silently shift behaviour").That leaves a residual, and I would rather name it than quietly break a test that exists to stop exactly this:
Context:is composed into the same newline-joined block as the rows above, so a newline in the body can still forge a trailingKey: valuerow. Closing it properly means rendering the body under its own structural header instead of as a labelled row — a prompt-shape change, not a sanitizer change, and one that would alter the grounding the description path depends on. A test pins the current behaviour so the residual stays visible rather than reading as covered. Happy to take it in this PR if you would rather close it now.Verification
Every guard is mutation-proven, the same way the #5857 guards were:
deduct-situation.ts→sanitizeForPromptbrief-why-matters-prompt.ts→sanitizeForPrompt_country-brief-context.ts→sanitizeForPrompt_country-brief-context.ts→ no sanitization at allbrief-llm.mjsdigestheadline/category/source→ rawbrief-llm.mjsdescriptionheadline→ rawbrief-llm.mjsimport aliased back tosanitizeForPromptNo survivors.
The tests assert the row count the payload declares, not the absence of the forged string — a guard that only greps for the payload passes for the wrong reason the moment that string is echoed anywhere. For the digest block the assertion is specifically that no row begins with the forged
[h:...]token; the forged text does survive inside its own row, inert, which is the correct outcome. Companion cases pin that ordinary values still render byte-identical (Côte d'Ivoire, an unremarkable market title, an ordinary digest row).buildPredictionContextis exported so its guard can be driven through the real builder. The existingtests/deduct-situation-edge-budget.test.mjsavoids importing this module and reads its source text instead, but that approach cannot satisfy the issue's mutation criterion — and the module imports cleanly under the test runner,getRedisCredentials-style side effects included, so no harness was needed.Neighbouring suites, on this branch:
Other gates:
npm run test:data: identical failure set toorigin/main— 45 failing test names on both,commdiff empty in both directions (the OpenAPI contract, docs/i18n, pricing and Docker suites already red on a clean checkout).Out of scope
get-country-intel-brief.ts:122appliessanitizeForPromptto the caller-supplied context snapshot. That one is correct as-is: the snapshot is a deliberately multi-line block, so line-collapsing it would destroy its structure, andparseCountryBriefSourcesparsesSource [N]:rows back out of it. Different vector (caller-controlled rather than feed-controlled), different fix.briefSourceContextLinesin_country-brief-context.tsstill has no LLM prompt injection: news headlines use structural-only sanitizer, bypassing semantic injection filters #3724 content sanitization.JSON.stringifyalready closes the delimiter vector there, and the same strings are round-tripped back to the client as display titles byparseCountryBriefSources, so sanitizing them would change user-visible source titles. Worth its own decision rather than a drive-by.CONTROL_CHARS_REand is outside the JS whitespace class, so\s+does not collapse it. That belongs insanitizeForPromptLineitself rather than at these call sites.sanitizeForPrompt, and no change to any prose sink.Type of change
Affected areas
/api/*) —deduct-situationandget-country-intel-briefprompt assembly; no contract or response-shape changescripts/lib/brief-llm.mjs(Railway brief seeder prompt builders)Checklist
api/rss-proxy.jsallowlist (if adding feeds) — N/A, no feeds added.npm run typecheck)Documentation Alignment Checklist
N/A — this PR does not publish or change any documentation claim. It tightens sanitization inside prompt assembly; no methodology, API/MCP contract, generated doc, Redis key or example changes. Listed for completeness:
news:digest:v1:full:en, already an existing read in_country-brief-context.ts; no key is added, removed or written.