Skip to content

fix: bound machine exec streams - #85

Merged
andrei-hasna merged 2 commits into
mainfrom
task/5c326cda-pr83-reviewed-remediation
Aug 9, 2026
Merged

fix: bound machine exec streams#85
andrei-hasna merged 2 commits into
mainfrom
task/5c326cda-pr83-reviewed-remediation

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • reject machines exec --script input while reading stdin as soon as it exceeds 65,536 characters
  • cap stdout and stderr inside the remote process-group helper instead of after full collection
  • preserve stream separation, exact exit codes, timeout exit 124, script stdin, and shell quoting

PR #84 already landed the AWS secret-assignment redaction, private-target error redaction, and returned-string suffix bounds. This follow-up is rebased on that merge and contains only the remaining gaps.

Validation

  • bun test test/exec.test.ts test/remote.test.ts — 19 pass, 0 fail
  • bun test test/cli.test.ts -t 'machines exec' — 2 pass, 0 fail
  • bun run verify:release — 564 pass, 0 fail; consumer conformance ok; release verification passed
  • shield review — no security issues in 6 staged files
  • redacted staged gitleaks report — 0 findings
  • git diff --cached --check — clean before commit

Todos: 5c326cda-dffe-4396-8684-0a2d0cd744f1


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Reject oversized scripts while incrementally reading stdin and cap stdout
and stderr during remote collection while preserving exit and timeout
semantics.

Agent: agent-ea
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #85 @ 8463d52 — lens: correctness+security+gates, reviewer Trebius (1 of 1)

Reviewed the complete diff against freshly fetched origin/main 8fdb66f and the surrounding execution, redaction, CLI, and test paths in src/commands/exec.ts, src/remote.ts, src/cli/index.ts, src/redaction.ts, test/exec.test.ts, test/remote.test.ts, and test/cli.test.ts.

Commands and gates:

  • git log --oneline origin/main..HEAD — exit 0; one commit, 8463d52.
  • git diff origin/main...HEAD --stat — exit 0; 6 files, 193 insertions, 20 deletions.
  • bun install — exit 0; setup only, 116 packages installed.
  • bun run typecheck — exit 0; PASS; TypeScript emits no pass/fail count.
  • bun run test — exit 0; PASS; 564 pass, 0 fail, 3002 expect() calls across 62 files.
  • Focused security repro — exit 0. Actual output: {"redacted_claim":true,"truncated":true,"output_chars":19,"leaked_near_complete_token":true,"contains_redaction_marker":false}.

Blocking P0/P1 findings:

  1. P1, high confidence — pre-redaction stream truncation can expose a near-complete credential while the result claims it is redacted. src/remote.ts:251-262 keeps only the first maxOutputChars before src/commands/exec.ts:96-118 applies credential-pattern redaction. A synthetic fixed-length access-key shape cut from 20 to 19 characters no longer matches the exact-length detector; with maxOutputChars=19, the truncation suffix cannot fit and the fallback at src/commands/exec.ts:118 returns those 19 characters verbatim. The returned object still sets redacted: true at src/commands/exec.ts:185. This is reachable through the approved machines exec --max-output-chars path and reduces a fixed-length secret to one unknown character. Preserve enough overlap beyond the visible cap for redaction, or redact incrementally before discarding bytes, then add a regression proving a boundary-split credential never appears in returned stdout/stderr.

Non-blocking follow-ups: none.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #85 @ 612d32a — lens: correctness+security+gates, reviewer Trebius (1 of 1)

Focused re-review of the one blocking defect named at 8463d52 and its direct regressions.

Fix applied in 612d32a:

  • The runner retains a bounded 512-character lookahead beyond the visible output cap so boundary-crossing credentials reach the existing redactor before final truncation.
  • If a counted truncation suffix cannot fit, the fallback emits only [truncated] instead of raw pre-redaction content.
  • Added a regression for a fixed-length credential split at the visible boundary.

Verification:

  • Regression before fix: bun run test -- test/exec.test.ts — exit 1; 11 pass, 1 fail. Actual failure exposed the unredacted boundary prefix.
  • Focused regression after fix: bun run test -- test/exec.test.ts — exit 0; 12 pass, 0 fail, 36 expect() calls.
  • Focused security repro after fix — exit 0. Actual output: {"redacted_claim":true,"truncated":true,"output_chars":11,"leaked_near_complete_token":false,"safe_marker_only":true}.
  • git diff --check — exit 0.
  • bun run typecheck — exit 0; PASS; TypeScript emits no pass/fail count.
  • bun run test — exit 0; PASS; 565 pass, 0 fail, 3005 expect() calls across 62 files.
  • gitleaks git --staged --redact=100 --no-banner . before commit — exit 0; no leaks found.
  • gitleaks git --log-opts 'origin/main..HEAD' --redact=100 --no-banner . before push — exit 0; 2 commits scanned; no leaks found.

Blocking P0/P1 findings: none remain. The named P1 is fixed and the focused re-review passes.

Non-blocking follow-ups: none.

@andrei-hasna
andrei-hasna merged commit cd6f02a into main Aug 9, 2026
2 checks passed
@andrei-hasna
andrei-hasna deleted the task/5c326cda-pr83-reviewed-remediation branch August 9, 2026 16:46
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.

1 participant