githubnext / githubnext/gh-aw-cao
Security review: harden CAO Actions trust and variable mutation boundaries
- Dominant language
- JavaScript
- Stars
- 3
- Forks
- 1
- Avg merge
- 49m
- Merged PRs (30d)
- 837
Description
## Scope
Reviewed `.github/cao/src/control.mjs` and `.github/cao/src/policy.mjs`, plus their GitHub Actions integration in `.github/workflows/shared/control.md`, with emphasis on variable use and mutation.
`report.mjs` is not present under `.github/cao/src/` (or elsewhere in this checkout/default-branch code search), so that requested file could not be reviewed. Add its current path or restore it before closing this issue.
## Findings
### 1. Security-incompatible: security-sensitive third-party actions use mutable tags — Medium
`shared/control.md` invokes:
- `actions/create-github-app-token@v3.2.0` at lines 53, 113, and 196 while supplying GitHub App private keys and minting tokens.
- `github/gh-aw-actions/setup-cli@v0.88.4` at line 167.
A tag can be moved. Compromise or retargeting would execute attacker-controlled code with job credentials and runner access. This is particularly sensitive because `setup-cli` runs between admission and precompute.
**More secure solution:** pin every action to a reviewed full commit SHA, retain the release tag only as a comment, and use Dependabot or the release-train process to update pins through review. Continue using `persist-credentials: false` for checkout.
### 2. Mutable-ref TOCTOU when loading orchestrator workflow source — Medium
`controlSourcePath()` derives `ref` from `GITHUB_WORKFLOW_REF` (`control.mjs:612-624`), and `writeOrchestratorPrecompute()` fetches the workflow source using that ref (`control.mjs:938-942`). A branch/tag can change after the run starts, so the worker list parsed during precompute may differ from the exact workflow revision that initiated the run. This conflicts with the exact-revision trust model used for policy admission.
**More secure solution:** fetch the workflow source at validated `GITHUB_WORKFLOW_SHA`, not the mutable ref. Reject the operation if the source cannot be read at that exact SHA.
### 3. Admission state is a mutable file across step boundaries — Low, defense in depth
Admission writes `$RUNNER_TEMP/cao/effective-policy.json` (`control.mjs:461-489`), while precompute later reads and trusts it (`control.mjs:1077-1095`). The mutable-tag `setup-cli` action can run between those operations (`shared/control.md:165-190`). Any intervening compromised action can alter authorization, mode, repository scope, or limits.
**More secure solution:** run admission and precompute before any additional third-party action; preferably recompute policy from the exact workflow SHA in the same process, or verify a digest over the handoff and all authorization-relevant inputs. Do not treat a writable runner file as an authenticated security decision.
### 4. Repository Actions variable is shared mutable availability state — Low
`CAO_GITHUB_API_GATE` is read from a repository variable (`shared/control.md:77`) and mutated through the Actions Variables API (`control.mjs:329-380`, `shared/control.md:109-128,192-211`). Anyone or anything with the applicable Actions-variable write capability can repeatedly extend the gate and deny CAO runs for up to two hours. The read/check/write sequence also has a race between concurrent runs.
This variable does not currently grant authorization, which limits impact to availability. It must never be reused as an authority or integrity signal.
**More secure solution:** keep authorization exclusively in commit-pinned policy and target authority; serialize gate writers with concurrency or use conditional/versioned storage; prefer the dedicated least-privilege GitHub App and remove the generic PAT fallback for variable mutation where feasible. Document the gate as untrusted optimization state.
## Variable-use and mutation assessment
- `policy.mjs:344-395,572-578` mutates only fresh local `effective`/`targetPolicies` values, and every requested change can only narrow checked-in mode and numeric ceilings. This is security-compatible.
- Policy input is schema-constrained, duplicate keys and `${{ ... }}` expressions are rejected, repository/owner/slug values are anchored, and live authority is checked against a target default-branch commit SHA.
- `control.mjs` uses `spawnSync` argument arrays rather than a shell; reviewed interpolated endpoint values are validated before use. No command injection was found.
- Tokens are passed through child-process environments and are not intentionally logged. Keep this invariant: never place tokens in arguments, outputs, summaries, policy, or repository variables.
- GitHub output values have CR/LF removed before append, preventing multiline output-file injection.
- Capacity/disk failures and policy errors fail closed.
## Suggested acceptance criteria
- [ ] Locate and review the requested `report.mjs`, or document that it was removed/renamed.
- [ ] Pin all actions in the reviewed path to full commit SHAs.
- [ ] Resolve workflow source at `GITHUB_WORKFLOW_SHA` only.
- [ ] Ensure no third-party action runs between policy admission and trusted precompute, or eliminate/authenticate the file handoff.
- [ ] Constrain and document `CAO_GITHUB_API_GATE` as non-authoritative mutable state and address concurrent writers.
- [ ] Add regression tests for exact-SHA workflow loading and mutation of the admission handoff/gate.
Contributor guide
Research direction
Start with .github/cao/src/control.mjs, .github/cao/src/policy.mjs, and .github/workflows/shared/control.md, then locate or confirm the missing report.mjs path. Review the listed admission, workflow-loading, action-pinning, and gate-mutation paths before deciding on the implementation approach. Done means the acceptance criteria are met and regression tests cover exact-SHA loading plus admission-handoff and gate mutation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github-actions, javascript
- Domain
- ci-cd, devops, security
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100