Repository navigation
[SEC-REVIEW-2] Independent trust-boundary review and the carried compose exposure #89
Description
Activity
- added a commit that references this issue
on Aug 30, 2026 - added 6 commits that reference this issue
on Aug 31, 2026 - linked a pull request that will close this issuefix(89): five High security findings from the final trust-boundary review #94
on Aug 31, 2026 Taking over as authorized for consolidated remediation on branch fix/96-release-readiness-audit, based on current origin/main f935fdb. Primary ownership is limited to the concrete exact-main audit findings and their causal tests/evidence. No UI redesign. Issue #96 is the umbrella tracker and this issue remains linked for its own acceptance semantics. Independent review will not be self-claimed.
PR #97 candidate-remediates the remaining concrete release-audit code findings: durable collections are bounded and Agent publication is atomic; tokenless access is loopback Host plus same-origin Origin only; every effective public bind requires a strong token; and runtime credentials no longer enter Terraform variables, cloud-init, plans, or state. Terraform validation passes with the pinned 1.9.8 container and Volcengine provider 0.0.58; npm audit reports zero vulnerabilities; bounded credential/state/artifact scans are clean. The author cannot satisfy the required independent part-5 security review, so this issue stays open for that review and protected-main integration evidence.
PR #97 is integrated as 53f2529. Its concrete release-audit remediations (bounded durable retention, atomic Agent publication, tokenless Host/Origin enforcement, strong-token public bind, and Terraform credential-state removal) pass on the exact merge, including protected-main run 33396729636. Draft PR #98 records the integration. The required independent security acceptance remains open and is not self-certified.
Re-opened acceptance work — Medium credential boundary
The independent exact-main review found one reachable Medium issue: the configured Ark key enters the Agent container PID namespace, where a same-UID command can read process environment through /proc and write the secret into an otherwise authorized Git artifact; changed blobs are not currently rejected against configured secret bytes before verification/promotion.
I am taking this concrete remainder on branch fix/89-ark-secret-boundary, based on current origin/main at 61fba86.
Primary ownership: apps/server/src/container-codex-runner.ts and causal tests; Shepherd changed-blob validation/promotion boundary and causal tests. Excluded: UI, dependency changes, all non-README documentation, external live execution. The fix will keep raw provider credentials out of the Agent PID namespace where feasible and add fail-closed secret scanning before artifacts can verify/promote. Issue #89 remains open until the fix is merged and independently re-audited.
Remediation is committed in PR #108 (
e34b2ac) and is undergoing independent re-audit. The branch adds binary-safe configured-sensitive-value scanning at both trust transitions (Agent workspace → trusted Plane and exact detached commit → promotion), fail-closed filesystem identity checks, non-disclosing authority-denial persistence, and causal regressions.Observed branch gates:
npm run checkgreen (862 server + 20 web), coverage >=80% in all metrics, Playwright 48/48 at both viewports, real container 1/1, no-model live preflight green, Terraform valid, npm audit 0. Issue remains open until the independent verdict.Completed on protected main via PR #108, merge commit
52ef61968185d659cd2ad95e218800e19223b25a.Independent read-only re-audit verdict: pass; no High/Medium blocker remains. The auditor passed 125/125 affected tests and independently proved the real
/procboundary with a synthetic-key negative control: readable to the same UID without Codex sandboxing, unreadable while scanning every/proc/*/environundercodex sandbox linux --full-auto. It verified import-before-mutation scanning, tracked/untracked/ignored coverage, fail-closed symlink/special/race handling, Plane rollback, exact immutable pre-verifier/CAS snapshot scanning, non-disclosing errors, and complete production service wiring.Exact merged-main
npm run checkalso passed: 26 root tests, 862 server passed + 3 documented live-only skips, 20 web, strict types, and both builds. Branch evidence additionally passed all 80% coverage floors, Playwright 48/48 across both viewports, real container 1/1, Terraform validation, npm audit 0, and live no-spend discovery.One non-blocking Low edge remains documented: configured sensitive values shorter than four characters are not scanned. Real Ark credentials exceed that floor; this does not reopen the completed High/Medium acceptance. Per the user’s explicit documentation constraint, no non-README project docs were modified.
TASKS ID:
SEC-REVIEW(parts 4–5)PRD: 0.3, 13, 17
Successor to #49, which has now been closed three times with its scope unsatisfied — twice by a linked PR, the last of which was mine. Its title and history no longer match what remains, so this issue carries the remainder with precise scope.
docs/TASKS.mdrecordsSEC-REVIEWasPARTIALat 70%, parts 1–3 evidenced and parts 4–5 open.Already done (evidence in
docs/BUILD_LOG.md, integrated ata62e29b)sensitiveValues; no path found by which either reaches persisted state, a model prompt, a 4xx/5xx body, the DOM, browser storage, a URL, the logger, or a tracked file. Two low observations recorded:/api/systemhas no redaction pass, andARK_API_KEYhas no minimum length against a 4-character redaction floor.SEC-01.Remaining
Part 4 — independent read-only security review
Must not be performed by @lemonishi. That constraint is not procedural throat-clearing; it has already paid for itself. Inside this issue's audit surface sits code I wrote:
config.ts's token floor,scripts/check-deploy-auth-token.mjs, thedeploy-existing-ecs.shguard, the OPS-06A-gate tests, and theSEC-01fix itself.SEC-01is the argument. The part-3 audit found that the bearer hook decided from the raw request target while Fastify percent-decodes before routing, soGET /%61pi/systemreturned 200 unauthenticated where/api/systemreturned 401 — on every route, includingPOST /%61pi/shepherd/demo/reset, which removes Plane worktrees and restores protected HEAD. I had worked beside that hook repeatedly without seeing it, and my first correction silently widened the/api/healthexemption. Both were caught by a reviewer who was not me. The existing suite could not see it:app.test.tsenumerates 16 control routes and asserts 401 for each, all canonical spellings.Part 5 — fix every High/Medium finding with causal tests
Depends on part 4.
The carried OPS-06 High finding — a posture decision, not an implementation detail
docker-compose.yml:16setsHOST: 0.0.0.0and:23publishes"${PUBLIC_PORT:-3000}:3000"— short syntax with no address prefix binds every interface.README:169documentsdocker compose up --buildandREADME:258/264the.env.productionlifecycle commands, all against that same file. The guard inscripts/deploy-existing-ecs.shcannot reach any of them, so a tokenless server can still be published on every interface by a documented command.The API behind it performs prompt-triggered command and file execution (
SECURITY.md), and incontainer-volumemode the control plane holds the container engine socket (docs/DEVIATIONS.md), so this is not only API abuse.Options, with the one already rejected and why:
docker-compose.ymlto127.0.0.1— matchesdocker-compose.state-volume.yml:60, but the existing-ECS profile is meant to be reachable behind a security group (README:261-263) and the same file serves the local Docker path.AGENTS.md's minimal-fix rule forbids removing intended functionality, so this was not taken.loadConfigguard thate6a5878deleted. Closes the exposure for every compose entry point at once, keeps the tokenless loopback demo, and would make the deploy-script guard and its Compose-parser machinery unnecessary. Probably the strongest option.Acceptance
C,S,Idocs/TASKS.mdSEC-REVIEWrow anddocs/BUILD_LOG.mdupdated from observed resultsNote for whoever takes this
Do not put a closing keyword for this issue in a PR body unless the PR genuinely completes every part. That is what closed #49 twice, including once by me.
SEC-REVIEWis listed under Required before submission in both PRD 13 and the hackathon cut indocs/TASKS.md.