Skip to content

fix(security): never auto-share a read_terminal snapshot the safety model did not fully see - #2283

Merged
jonathanKingston merged 11 commits into
mainfrom
claude/issue-2280-cdcd14
Sep 5, 2026
Merged

jonathanKingston merged 11 commits into
mainfrom
claude/issue-2280-cdcd14

Conversation

@jonathanKingston

Copy link
Copy Markdown
Collaborator

Closes #2280.

What was wrong

The read_terminal screen judged the trailing 6000 characters of a snapshot, then shared the whole snapshot on a confident "safe". Anything above the window travelled unexamined whenever the tail looked like ordinary build output — which is exactly what #2280 measured on both local models, with the payload held constant and only its position varied.

The comment above the constant said the part beyond the window was "still covered by the approval fallback whenever the classifier cannot vouch for the visible tail". That only held when the tail looked risky.

What this does

A verdict now only vouches for what the model was shown.

  • terminalReadScreenWindow (new, pure, in terminal-read-verdict.ts) splits a snapshot into the screened tail and the part above it. The gate criterion is characters, so one enormous line cannot slip through on a zero line count; the line counts are for the user-facing explanation only.
  • A snapshot with anything above the window goes straight to the existing approval prompt. The reason says how many of its lines were actually screened, e.g. "It is larger than the safety model screens: only the most recent 75 of its 200 lines were screened."
  • No screening call is spent on an oversized snapshot: no verdict on the tail could change that outcome, so it would only add latency (and, once fix(security): give the safety classifier a default that works, and say when it cannot run #2276 lands, cloud spend) before a prompt that is coming anyway.
  • Snapshots that fit the window are unchanged, as are the flagged / could-not-screen paths and "Always allow for this chat".
  • The tool description tells the agent roughly where the window sits so a quick check stays under it instead of prompting, and it now interpolates READ_TERMINAL_DEFAULT_LINES / READ_TERMINAL_MAX_LINES instead of repeating them as literals.

Why this option

Of the four in the issue, this one keeps the full 2000-line capability, adds no latency, and stays fail-closed. Its cost is a prompt for large reads, which "Always allow for this chat" turns into one prompt per chat.

  • Share only what was screened silently truncates the tool to ~6000 chars against a 2000-line cap.
  • Screen in chunks is not viable with the current 8s timeout: the issue's own latency data shows a single window already brushes it on some local models.
  • Cap max_lines does not bound characters, so it does not actually keep a snapshot inside the window.
  • A deterministic secret scanner is worth pursuing separately, but it does nothing for the injection lane and would not change this gate's contract.

Notes for a reviewer

  • The security posture only tightens. Nothing that auto-shared before and was fully screened changes; the only new behaviour is that an unscreened portion now escalates instead of riding along.
  • Overlap with fix(security): give the safety classifier a default that works, and say when it cannot run #2276. That PR reshapes the same guard (the classifier returns { verdict, problem }) and adds a terminal-read-guard.test.ts of its own. Whichever lands second needs a small manual merge in those two files; the logic does not conflict. This PR deliberately keeps its edit to classifyTerminalSnapshot to one line so that merge stays trivial.
  • A classifier test seam (setTerminalSnapshotClassifierForTest) was added, mirroring the existing gate seam, so the gate can be driven with a scripted verdict and no model or HTTP stub.

Testing

  • New terminal-read-guard.test.ts pins the gate contract: a confident safe never auto-shares an oversized snapshot; no screening call is spent on it; the reason names the screened line count; approve, decline, and remember all behave; a break-less oversized line gets a reason without a bogus line count.
  • terminal-read-verdict.test.ts covers the window helper: fits, exactly-at-limit, tail-only screening with the lines above counted (and the straddling line not counted as above), oversized single line, empty.
  • services/security + tools/ test directories: 719 pass. Lint, tsc -p tsconfig.node.json, and oxfmt --check clean.

🤖 Generated with Claude Code

jonathanKingston and others added 2 commits September 2, 2026 21:33
…odel did not fully see

The `read_terminal` screen judged the trailing 6000 characters of a
snapshot but, on a confident "safe", shared the whole thing. A secret or
an instruction to the agent above that window travelled unexamined
whenever the tail looked like ordinary build output (#2280).

A verdict now only vouches for what the model was shown. `terminalReadScreenWindow`
splits a snapshot into the screened tail and the part above it; a snapshot
with anything above the window goes straight to the existing approval
prompt, whose reason says how many of its lines were actually screened.
No screening call is spent on it, since no verdict on the tail could
change that outcome. Snapshots that fit the window are unchanged, as are
the flagged/could-not-screen paths and "Always allow for this chat".

The tool description tells the agent roughly where the window sits so a
quick check stays under it instead of prompting.

Closes #2280

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`pnpm audit --audit-level=high` in precheck started failing on
GHSA-5jgf-p345-68v8, GHSA-f65p-4m7j-42xc, GHSA-fph4-wmhf-6fwf and a
fourth fast-uri advisory, all patched in 3.1.6 (published 2026-09-02).
The transitive path is @modelcontextprotocol/sdk > ajv > fast-uri; the
existing override held it at 3.1.5. Bumped the pin; `minimumReleaseAgeExclude`
already lists fast-uri, so Dependabot's cooldown does not veto it.

Unrelated to the read_terminal change in this branch; every branch that
runs precheck from now on hits it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
github-actions Bot added a commit that referenced this pull request Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🖥️ PR preview

Resolves the overlap with #2276 in terminal-read-guard.ts and its test:
the window gate now sits on the { verdict, problem } screening result,
and the gate tests join the availability tests in one file.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
github-actions Bot added a commit that referenced this pull request Sep 2, 2026
terminalReadScreenWindow counted newlines strictly above the window, so a
line whose terminating newline was the first screened character was
reported as screened although the model saw none of its content. The gate
decision was unaffected (it keys on characters); only the approval prompt
wording undercounted by one.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@jonathanKingston jonathanKingston left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: never auto-share an unscreened read_terminal snapshot

Verdict: no blocking findings; the fail-closed claim holds. Rebase (16 behind, dry merge clean) and let CI typecheck the combined tree before merge.

Path enumeration (gateImpl, terminal-read-guard.ts:155-201)

Path Classifier called Shared Unscreened auto-share
fits window (≤ 6000 chars), confident safe yes, whole snapshot byte-for-byte whole snapshot no
fits, flagged or low confidence yes only after approval no
fits, screen error, timeout or model absent yes, verdict: null only after approval, problem named no
oversized (> 6000) no only after approval, reason via describeUnscreened no
exactly 6000 yes whole no (probe: unscreenedChars = 0)
6001, or a single 7000-char line no after approval; reason says "part of it was not screened" with no bogus line count no
empty snapshot yes (no output yet) n/a
remembered thread no (line 161, before the size check) whole, silently user-granted, see item 1
approval denied none nothing; agent told to ask for a paste no

terminalReadScreenWindow slices and gates on the same text.length measure, and unscreened + screened === text held for every probe (window−1, window, window+1, a 7000-char line, 200 × 80, boundary on a newline, screened tail starting with a newline). CR/LF and ANSI are normalised upstream in takeLastLines and stripTerminalControlSequences, so counts match what the agent receives. READ_TERMINAL_MAX_LINES is still enforced server-side twice (zod .max() at read-terminal-tool.ts:48 and the clamp in takeLastLines); the tool only has action, id, max_lines, so no offset and no sliding window.

Check Result
pnpm test -- terminal-read-guard terminal-read-verdict read-terminal 36/36 pass (after rebuilding node-pty, no linux prebuild here)
oxfmt --check on the 5 changed files pass
pnpm run check:dead-code pass
Dry merge with origin/main clean; #2282 rewrote the classifier internals but the gate's classifier indirection wraps it unchanged

No as casts, no object-literal casts, no exported type predicates; setTerminalSnapshotClassifierForTest mirrors the existing setTerminalReadGateForTest boundary injection, and the window size is a const, not a test-only option.

Non-blocking

  1. "Always allow for this chat" bypasses the classifier and the size gate for the rest of the session (terminal-read-guard.ts:161 and :199). Pre-existing, but the oversized prompt now offers it on every large read and the new test pins it. docs/shell-permissions.md describes the analogous read-outside grant as authorising no command by itself, with every later command re-proved; this grant shares every later snapshot of any size unscreened. Worth a follow-up: omit allowRemember for the oversized reason, or keep screening fitting snapshots in remembered threads.
  2. The user decides an oversized read with less information than before. Previously the tail was screened and the prompt could say "flagged: looks like an API token"; now it says only that the snapshot is larger than the model screens. A deliberate latency trade, but a token in the last ten lines of a 200-line snapshot no longer surfaces in the prompt copy.
  3. Reason arithmetic overstates by up to one line. For 200 lines of 80 chars the window holds 74 whole lines plus 6 characters of the 75th, and the copy says "most recent 75 of its 200 lines were screened". Cosmetic since the gate is by characters; "about 75" or whole lines only would be exact.
  4. classifyTerminalSnapshot still slices at line 103, but the gate only calls it when nothing is unscreened, so the slice is dead in product. If it is ever reached, slice(-6000) can start with a lone low surrogate (probe: 0xde00, isWellFormed false).
  5. Missing tests: gate-level 6000 versus 6001 (auto-share versus prompt with zero classifier calls); the verdict case where exactly one non-newline character of the boundary line is visible (unscreenedLines 0, totalLines 2). The approval body copy changed and no e2e or demo spec reaches the terminal-output-share prompt (only settings-read-terminal.e2e.ts, the settings toggle); per the AGENTS.md visual-copy rule, add one or note why it is exempt.

Residual pre-existing gap, out of scope: two tail reads of different lengths, each under the window, can split a multi-line secret so the model sees each half in isolation. That is model quality rather than gating; an issue would be enough.


Generated by Claude Code

The read_terminal prompt said "only the most recent 75 of its 200 lines
were screened" when the window held 74 whole lines and six characters of
a 75th. A line cut by the window boundary is now in neither count:
terminalReadScreenWindow reports screenedLines alongside unscreenedLines,
and the reason reads "only the most recent 74 of its 200 lines were fully
screened". The gate criterion is still characters, unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011jaQbed96h8aSZLijfiHNB
…over

classifyTerminalSnapshot re-sliced its input to the screening window, but
the gate is its only caller and only calls it for a snapshot that fits the
window in full — a verdict on a slice could never auto-allow what sits
above it. Send the text as given; the verdict then describes all of it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011jaQbed96h8aSZLijfiHNB
… read_terminal prompt asked

A remembered thread used to skip both the size gate and the safety model
for the rest of the session, so a grant taken on an oversized read also
waived a later token the model would have found in a small one.

The prompt asks one of two questions — share what the model never vouched
for (larger than the window, or no usable verdict), or share what it
positively flagged — and the grant now remembers the answer to that
question only. Fitting snapshots in a remembered thread are still
screened; a flagged one prompts on its own question. Oversized and
could-not-screen reads stay at one prompt per chat, which is what the
grant was offered for. Nothing widens: every path is at least as closed
as before.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011jaQbed96h8aSZLijfiHNB
A snapshot of exactly the window size is screened whole and auto-shared
on a safe verdict; one character over prompts with no screening call.
terminalReadScreenWindow counts a line with exactly one character inside
the window as neither above the window nor screened.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011jaQbed96h8aSZLijfiHNB

Copy link
Copy Markdown
Collaborator Author

Pushed five commits: 099ee77 merges main (clean, 17 commits); b3ca03c makes the reason count only lines the model saw whole ("74 of its 200 lines were fully screened", singular handled) via a new screenedLines on terminalReadScreenWindow, gate criterion unchanged; de405dc removes the dead re-slice in classifyTerminalSnapshot since the gate is the only caller and only passes snapshots that fit; 5878dca scopes "Always allow for this chat" to the question the prompt actually asked (unscreened versus flagged), so a remembered thread still screens fitting snapshots and a flagged one prompts on its own question, while oversized and could-not-screen reads keep one prompt per chat as the PR intended; 08658e1 pins the 6000 versus 6001 gate boundary (zero classifier calls when oversized) and the one-visible-character verdict case. Every path is at least as closed as before and no UI copy changed. Typecheck, eslint, oxfmt, dead-code and 45 targeted tests pass. docs/shell-permissions.md still has no read_terminal section; worth a small follow-up if you want the gate and the per-cause grant recorded as contract.


Generated by Claude Code

github-actions Bot added a commit that referenced this pull request Sep 4, 2026
jonathanKingston and others added 2 commits September 4, 2026 11:22
Precheck failed three times in a row on this branch at the same step,
each time with ERR_SOCKET_TIMEOUT from the registry's quick-audit
endpoint on a GitHub-hosted runner, while unrelated branches passed
either side of it and the same audit takes two seconds from a laptop.
pnpm's own two retries fit inside one bad minute.

Wrap the step in the same retry shape the lint step already uses: up to
three attempts, a minute apart, and only when the output shows a
transport failure. A real advisory still exits 1 on the first attempt,
so the gate does not get any looser. Verified against a fake pnpm for
the three cases (transient failure then clean, advisory, registry down
throughout).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jonathanKingston
jonathanKingston marked this pull request as ready for review September 4, 2026 23:52
github-actions Bot added a commit that referenced this pull request Sep 4, 2026
@copse-release-bot

Copy link
Copy Markdown
Contributor

Reference screenshots ready for review

Review GitHub’s image diffs in screenshot PR #2371.
Merge it (or enable auto-merge) to apply the accepted PNGs to claude/issue-2280-cdcd14.

Rendered for 9e039abba775 by CI run 33931082329; the immutable artifact is reference-screenshot-candidates-33931082329.

If this source branch moves, a later successful render closes the stale review PR and replaces this link.

@jonathanKingston
jonathanKingston merged commit 8e7c6f8 into main Sep 5, 2026
23 checks passed
@jonathanKingston
jonathanKingston deleted the claude/issue-2280-cdcd14 branch September 5, 2026 00:13
github-actions Bot added a commit that referenced this pull request Sep 5, 2026

This branch was previously deployed

1 inactive deployment
github-pages — 9e039abb Deployed Sep 4, 2026 by jonathanKingston via deploy / deploy #3521
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

read_terminal: the safety screen covers the last 6000 chars, but the whole snapshot is shared

2 participants