Repository navigation
Diagnostic report and comparison security acceptance: exact reviewed text, safe rendering #363
Description
Goal
Close the security-review gap for the stacked diagnostic-report and compare features before they are considered review-ready. Track verification of current HEAD, not speculative claims that every previously reported issue is still present.
Existing implementation
- #360 report redaction/review; #361 compare/diff/report review.
- Relevant:
src/clipboard/report.ts,src/output/compare.ts,src/ui/ComparePanel.ts, report/compare review UI and clipboard writer. - Previous reviews raised parser/redaction differentials, private-path/secret bypasses, invisible and bidi Unicode, incorrect word boundaries, long-line clipping, Markdown/terminal escape injection and displayed-vs-copied text discrepancies. Some have already received fixes in the drafts; reproduce against fresh heads before changing code.
Security invariants
- Exactly the approved text is copied; no hidden suffix, truncated display, altered re-edit or stale buffer.
- Report copies always require explicit review; cancel leaves clipboard unchanged; failure is closed. A best-effort scanner cannot promise complete secret detection. Preserve legitimate Unicode without losing security boundaries.
- Copy/review formatting, wrapping, normalized search spans and final clipboard text use consistent well-tested representations. Support long lines, combining marks, Unicode separators, zero-width and bidi controls, display-width edge cases.
- Untrusted diff filenames/hunks, commands and program output cannot inject terminal controls, clickable spoofed actions, Markdown structure or misleading trusted UI chrome.
- Comparison
c(plain diff) versusr(report) semantics must be explicit and safe; sensitive-output exposure should be disclosed where review is not invoked. No automatic external transmission.
Acceptance
- Reproduce or rule out each outstanding reported vector against latest Copy as Report: reviewed Markdown/plain reports from command blocks #360/Compare output: diff two runs of a command #361 heads.
- Add regression/adversarial tests for parser differential, token splitting, private paths, control/escape sequences, Markdown fences, edited content, exact clipboard bytes, long/narrow reports and comparison copies.
- Run focused unit and real-PTY tests; record which platform/shell paths were actually exercised.
- Maintain clear PR evidence and any remaining limitations; no unverified claim of complete redaction.
Activity
raiseCatError commentedon Oct 10, 2026
Verified against current heads (#360
79e3f40, #361dafd69f).Fixed this pass
c) sent raw output to the clipboard (escape bytes, bidi/invisible characters). It now uses the report character policy, and the copy note says when it looks sensitive and was not redacted (rreviews and redacts).Directory/Statusfacts accepted line breaks and backticks; now folded to one line.Checked, not reproducible: scanner/review/clipboard differential, token splitting with zero-width, combining and full-width characters, bidi controls in key names, private paths, URL credentials, auth headers, edited text (rescanned, labelled not redacted).
Tests:
tests/compareSecurity.test.ts, additions totests/copyReport.test.ts; typecheck clean; compare/report unit and live PTY tests pass. The full suite showed load-related timeouts in unrelated live/fuzzy tests that pass alone. Redaction remains pattern-based, not a guarantee. Not yet covered: physical clipboard behavior in a real terminal host.raiseCatError commentedon Oct 11, 2026
Security acceptance follow-up on the additive local workflow candidates; published #360/#361 updates and final Actions are pending.
Confirmed additional authored-UI injection: restored command text in the Compare chooser's “with …” header bypassed display sanitization. 5570d34 sanitizes that label consistently with the picker/diff. Format and Unicode separator controls become visible in authored review UI. Actual plain diff clipboard output remains sanitized.
Confirmed Markdown structure injection: restored lifecycleText was a raw Status fact in a report. 25af5ba uses the existing delimiter-sized inline-code representation for that fact, including ordinary statuses. A hostile regression covers HTML, links and backtick delimiters. Plain reports retain their factual format.
The existing live Report assertion was stale after that security fix. ee5483e corrects the assertion and its misleading title. Focused macOS checks passed:
They exercised explicit report review, cancel preserving clipboard contents, final sanitized/redacted clipboard bytes, restored transcripts, narrow/NO_COLOR/Safe presentation, backend failure/timeout/size bounds and EPIPE. Earlier focused comparison security/clipboard and hostile pin tests passed; the full final cross-feature gates are running again.
Current report candidate: d51e80f.
Current comparison candidate: eb4fdf9.
Current combined workflow candidate: 5e45e87.
The secret scanner remains best-effort. This is headless evidence with test clipboard backends, not physical terminal validation or a comprehensive-redaction guarantee. The shared frontend is not frozen.
raiseCatError commentedon Oct 12, 2026
Current-head security stabilization found and reproduced two additional defects before acceptance:
<img src="https://example.invalid/leak"> password=hunter2escaped its inline-code boundary; unfinished key/credential values could also consume command/output fences and later sections. Comparison labels containing[open](https://example.invalid/leak) cookie: xhad the same defect. The scanner patterns were not weakened. Correction736100eredacts independent source fragments and aggregates findings before report formatting;d7163ebapplies the same boundary to comparison labels/diffs and rebuilds from the chosen comparison snapshot on redaction toggles.Failing regressions were observed before each correction. Focused report/comparison checks: 33 passed, zero failed/skipped; source build passed. Tests cover hostile markup, incomplete credential/key fields, preserved later sections, exact Enter text, redaction toggles, short scrolling and refusal when a body cannot fit. Independent review follow-up is underway. These commits are additive candidates on the report/comparison branches, not yet final published PR heads. Canonical, actual clipboard-backend PTY, cross-platform and latest-head CI evidence must still pass before the security/interface gate is accepted. Physical clipboard QA remains pending; redaction remains best-effort.
raiseCatError commentedon Oct 12, 2026
Additional bounded report security probe reproduced a RangeError with approximately 300 KB of text containing 150,000 short backtick runs.
Math.max(...runs)exceeded JavaScript's argument limit in report fences, inline facts and comparison fences.The correction uses one iterative delimiter-sizing helper without materializing/spreading all run lengths. Original report/comparison regressions failed on the uncorrected implementation; 36 affected unit tests plus source build passed with the correction, preserving output spaces and safe enclosing delimiters. Commits
a60b2e7/3ac8039are retained on the acceptance ancestry and promoted additively to reportd12d23band comparison2cbbcb7; both siblings retain their independence. The unchanged report run was allowed to finish before changing its worktree; corrected report canonical verification is now serialized before the later heads. No final report/security/frontend acceptance is claimed yet.raiseCatError commentedon Oct 13, 2026
Bounded final source review found no material report/compare defect on Report
7bf0dbeaf15d4c09a9b49bbce0eef0996efd219f, Compare459d715af3e986bd92c857ac74eed03a39f90ecdand References3518231155225e4b3130e467ede7b703f366f8a0. Confirmed additive ancestry and relevant security code/test equivalence across these candidates.Reviewed fragment redaction before Markdown delimiters, sanitized/rescanned edits and explicit unredacted warnings, cloned comparison snapshots, exact approved text passed to the clipboard backend, cancellation without writes, backend failure/timeout/size/EPIPE rejection and short-panel confirmation guards. This is a read-only review, not a claim that final tests or physical clipboard QA ran.
Known limits remain: pattern scanning cannot recognize every unknown/custom/encoded secret or arbitrary private path; a reported external clipboard-tool failure cannot guarantee rollback after that tool has already changed the clipboard. Final current-head canonical, cross-platform live stand-in byte checks and CI remain required before frontend acceptance. No issue closure or physical QA is implied.