Skip to content

fix(runtime): harden command runner per review (#1387)#1548

Merged
oscharko merged 1 commit into
feat/keiko-agent-native-editor-foundation-and-runtimefrom
claude/issue-1387-hardening
Jun 26, 2026
Merged

fix(runtime): harden command runner per review (#1387)#1548
oscharko merged 1 commit into
feat/keiko-agent-native-editor-foundation-and-runtimefrom
claude/issue-1387-hardening

Conversation

@oscharko

Copy link
Copy Markdown
Contributor

Summary

Follow-up to PR #1546 (merged) landing the review-driven hardening for Issue #1387's controlled command executor. The core implementation merged at 8be49afe; this PR addresses the actionable findings from the security audit, verifier, and PR review.

Refs #1387

Scope

  • In scope: parse-boundary identifier caps, SSE/redaction test coverage, a UI subtitle gap, and doc accuracy. No behavioral change to the execution path.
  • Out of scope: anything beyond the review findings.

Changes (review finding → fix)

Finding Fix
Security L1 / verifier: taskId/requestId unbounded at the parse boundary Cap taskId ≤256 and requestId ≤128 (token charset [A-Za-z0-9._:-]) in parseCommandTaskRunRequest, so an oversized/non-token id cannot reach the manager, audit ledger, or SSE fan-out (the 16 KB body cap was the only prior backstop) + tests
PR review M4: Tasks window had no subtitle connectionUtils.subText now renders the project path for commands (consistent with terminal/cwd)
PR review M2: per-run catalog re-discovery looked like double I/O Documented as an intentional untrusted-taskId re-validation (re-derive the vetted task from the current package.json, never trust the client's catalog)
PR review T6: SSE-frame redaction untested Added a routes test asserting Layer-2 redaction is applied to every SSE event frame
PR review T9: fan-out resilience untested Added a manager test proving a throwing SSE subscriber does not block delivery to others
PR review M5: e2e did not mock the SSE channel e2e now deterministically serves /api/commands/events and asserts a run lifecycle event reaches the bounded events log
Security audit I1: doc over-claimed "no absolute paths" Corrected the audit-evidence note: the standard manifest workspaceRoot path is retained; argv values, output bytes, and secrets are excluded

Dispositioned (not changed): audit M1 (the denied vs spawn-error label uses a string match on the shared CommandDeniedError.message; adding a structured kind field is a cross-package change to a shared error, out of #1387 scope — both branches are tested and only the audit label, never execution, is affected) and review M3 (Cancel uses aria-disabled without disabled, mirroring the deliberate, axe-passing TerminalWidget F018 C124 focus-management pattern; onAbort is guarded against activation).

Verification

contracts + server command-runner tests   55 passed
ui CommandsWidget + connectionUtils tests  31 passed
typecheck (ui + server)                    PASS
playwright --grep @command-runner-1387     1 passed (with the new SSE assertion)
  • Core logic + redaction + a11y covered by unit/integration tests; e2e browser smoke updated.
  • Security-sensitive change (parse-boundary caps) reviewed and tested.

🤖 Generated with Claude Code

Addresses review findings on PR #1546 (security audit + verifier + PR review):

- contracts: cap taskId (<=256) and requestId (<=128, token charset) at the
  parse boundary so an oversized/non-token id cannot reach the manager, audit
  ledger, or SSE fan-out (the 16 KB body cap was the only prior backstop) + tests
- server: document the per-run catalog re-discovery as an intentional
  untrusted-taskId re-validation; add a test proving a throwing SSE subscriber
  does not block fan-out to other subscribers
- routes: assert Layer-2 redaction is applied to every SSE event frame
- ui: render the project path as the Tasks window subtitle (connectionUtils)
- e2e: deterministically mock the SSE channel and assert a run lifecycle event
  reaches the bounded events log
- docs: correct the audit-evidence note (the standard manifest workspaceRoot
  path is retained; argv values, output bytes, and secrets are excluded)

Refs #1387

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@oscharko oscharko merged commit 9a40655 into feat/keiko-agent-native-editor-foundation-and-runtime Jun 26, 2026
12 checks passed
@oscharko oscharko deleted the claude/issue-1387-hardening branch June 26, 2026 09:52
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