Skip to content

Harden CI: least-privilege token scopes for the last 3 workflows - #5253

Merged
vivekchand merged 4 commits into
mainfrom
harden/token-permissions-3
Aug 26, 2026
Merged

Harden CI: least-privilege token scopes for the last 3 workflows#5253
vivekchand merged 4 commits into
mainfrom
harden/token-permissions-3

Conversation

@vivekchand

@vivekchand vivekchand commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Closes out the TokenPermissions batch: after this, every workflow in .github/workflows/ declares exactly one top-level permissions: block — 34/34, verified on the current head with a loader that rejects duplicate keys.

These were left for last because all of them touch a release or deploy path, where a blanket contents: read breaks the pipeline silently. Each job was read individually and the scope justified below rather than applied uniformly.

ci.ymlcontents: read, no job elevation

All 13 jobs do the same shape of work: checkout, install, run tests, upload an artifact. Nothing in the file calls the GitHub API with GITHUB_TOKEN, pushes a commit, creates a tag, comments on a PR, or touches a Release. The only uses: beyond checkout/setup are actions/cache, actions/upload-artifact and the local ./.github/actions/setup-openclaw; upload-artifact uses the runtime token, not GITHUB_TOKEN. The read-only floor therefore covers the whole workflow and no job needs an elevated block.

auto-deploy-cloud.ymlcontents: read

Worth stating explicitly because the job name suggests otherwise: this workflow checks out clawmetry-cloud, pushes a branch, and creates, closes and merges a PR there — but every one of those steps authenticates as secrets.CLOUD_REPO_PAT, both as the token: on the second checkout and as GH_TOKEN on the gh steps. A workflow permissions: block does not govern a PAT, so none of that cross-repo work is affected by this change.

The only thing riding on GITHUB_TOKEN is the checkout of this repo, which needs contents: read and nothing else. Keeping the floor read-only means a future step added here cannot quietly pick up write access to clawmetry through the ambient token — it would have to ask for the PAT, which is visible in review.

desktop-artifacts.ymlcontents: read floor, release stays contents: write

job effective scopes why
macos contents: read inherits; checkout + build + upload-artifact
linux contents: read inherits; checkout + build + upload-artifact
windows id-token: write, contents: read unchanged — already declared for azure/login OIDC
release contents: write unchanged — softprops/action-gh-release attaches installers to the tag's Release and publishes it

The elevation deliberately stays on the release job. A workflow-wide contents: write would hand it to all three build jobs and to every third-party action they run, which on the signing paths is the largest surface in this repo.

Note the ordering hazard this file already handles correctly: a job-level permissions: block replaces the top level rather than merging with it, so the windows job's existing block had to already carry contents: read for its checkout to keep working. It does (with a comment saying exactly that), so the new top-level block does not regress it.

release-on-merge.ymlcontents: read floor, job scopes untouched

#5250 was expected to cover this file but landed without a top-level block, leaving it the last workflow in the repo with none — caught by re-running the check across all 34 workflows after merging main into this branch, not assumed from #5250's description.

The change is functionally inert by design. The workflow has exactly one job; that job declares its own contents: write / pull-requests: write / actions: write; and since a job-level block replaces the top level, the new value can never apply to release. Its purpose is that a job added here later defaults to read-only rather than inheriting the repository default, and that the file stops reading as unscoped. The publish, tag and PR-merge scopes stay on the job that needs them.

api-latency-smoke.yml — repairing a workflow the batch had broken

Carried in 36e773d. Two earlier hardening commits each added a top-level permissions: contents: read to this file — 210696e below concurrency:, 85873f0 (#5250) above it. Neither conflicted in git and both were individually correct, but YAML forbids a repeated mapping key, so GitHub rejected the workflow and failed every run at startup with zero jobs. The check had been dead on main since #5250 landed.

The fix keeps the block above concurrency: and folds the other comment's point into it. Effective scope is unchanged: contents: read, one job, no job-level override.

Why the existing guard missed it, and the correction

This is worth flagging because it undercuts how the earlier revisions of this PR described their own verification. yaml.safe_load resolves a duplicate key last-one-wins and raises nothing — so test_workflow_parses_as_yaml, and the "all 34 workflows parse" check cited in this PR's earlier description, both passed happily on a file GitHub refuses to run. Parsing cleanly was never evidence of a single permissions: block; it just could not see the second one.

36e773d adds tests/test_workflow_has_no_duplicate_keys next to the existing guard, using a SafeLoader subclass whose mapping constructor rejects a repeat, auto-discovered over every workflow. Re-inserting the duplicate fails the new test while the old parse test still passes — which is precisely the blind spot being closed.

Verification

Re-run on the current head (36e773d), which includes main (bringing #5250/#5251/#5252):

  • All 34 workflow files parse and none repeats a mapping key, checked with a duplicate-rejecting loader rather than plain safe_load.
  • Every workflow declares a top-level permissions: block; each of the four files this PR touches has exactly one.
  • Effective per-job permissions resolved and inspected for all four files; the tables above are that output, not an assumption. release-on-merge's job scopes confirmed unchanged.
  • api-latency-smoke resolves to contents: read with its single smoke job.
  • 184 workflow tests pass.
  • scripts/check_action_refs.py — 17 action references, OK.

No functional change beyond reviving api-latency-smoke.yml: additive permissions: blocks and one de-duplication, no step, trigger or job logic touched.

No-PRD: CI-only change confined to .github/ and tests/, exempt from the product-record gate.

ci.yml, auto-deploy-cloud.yml and desktop-artifacts.yml were the last
workflows with no top-level `permissions:` block, so each job ran on the
repository's ambient GITHUB_TOKEN default. Declare a `contents: read`
floor on all three and leave the one job that genuinely writes elevated
at job level.

- ci.yml: all 13 jobs checkout, install, test and upload an artifact.
  None calls the GitHub API with GITHUB_TOKEN, pushes, tags, comments or
  touches a Release, so the read-only floor covers the whole workflow and
  no job needs an elevated block.

- auto-deploy-cloud.yml: every cross-repo action authenticates as
  secrets.CLOUD_REPO_PAT, which a workflow permissions block does not
  govern. Only the checkout of this repo rides on GITHUB_TOKEN, so
  `contents: read` is the complete requirement.

- desktop-artifacts.yml: macos and linux inherit the read-only floor;
  windows already re-declares `id-token: write` + `contents: read` for
  azure/login; release keeps its job-level `contents: write` so
  action-gh-release can attach installers and publish the tag's Release.
  The elevation stays on that job rather than the workflow so the build
  jobs, and the third-party actions they run, cannot write to the repo.

No-PRD: CI-only change under .github/, exempt from the product-record gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016UY8n2YF46YMGvThcJPwvk
@8090-software-factory

Copy link
Copy Markdown

✅ Drift Bot (ClawMetry): no drift detected

Drift Bot analyzed the changed files against this project's blueprints and requirements and found no drift.

@8090-software-factory

Copy link
Copy Markdown

✅ Drift Bot (ClawMetry): no drift detected

Drift Bot analyzed the changed files against this project's blueprints and requirements and found no drift.

#5250 was expected to cover this file but landed without a top-level
permissions block, leaving it the last workflow in the repo with none.

Functionally inert by design: the workflow has exactly one job, that job
declares its own contents/pull-requests/actions write scopes, and a
job-level block REPLACES the top level rather than merging with it — so
this value never applies to `release`. It is here so any job added later
defaults to read-only instead of inheriting the repository default, and
so the file no longer reads as unscoped. The publish, tag and PR scopes
stay on the job that needs them.

Verified: 34 workflows parse; no workflow is now missing a top-level
permissions block; release job scopes unchanged
(contents/pull-requests/actions: write); 158 workflow tests pass.

No-PRD: CI-only change confined to .github/, exempt from the product-record gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016UY8n2YF46YMGvThcJPwvk

Copy link
Copy Markdown
Owner Author

✨ auto-fixed: branch was BEHIND main (base 09b255dc → current 41d4accf); updated via GitHub merge-base update — CI now running on new head 8eec031a.


Generated by Claude Code

@8090-software-factory

Copy link
Copy Markdown

✅ Drift Bot (ClawMetry): no drift detected

Drift Bot analyzed the changed files against this project's blueprints and requirements and found no drift.

Copy link
Copy Markdown
Owner Author

E2E Gate (required) failed on 90f0abe — runner-queue starvation, not this PR. Recording the evidence rather than silently re-running.

What failed

E2E Gate is an aggregator (scripts/e2e_gate.py, MAX_WAIT: 1800) that polls the 12 required checks. It started 13:43:08 and gave up at 14:13:30:

FAIL: timed out after 1800s. Still pending:
  - MOAT Keystone: 0/1        - MOAT Verifier: 0/1
  - E2E Browser Tests: 0/1    - Entitlement API tests: 0/1
  - API Tests (3 OS): 0/3     - pip install matrix: 0/4
  - Wheel install & assets: 0/1
  - Store invariants: 0/1

No test failed. Those 8 checks never got a runner: their jobs were created at 13:43:47 and are still queued now, ~30 min later.

Why it isn't this PR's

The gate's 30-minute budget is simply shorter than the queue latency the repo is seeing today.

What I'm not doing

No fix ported, because there isn't one that belongs here. Raising MAX_WAIT in e2e-gate.yml would be a different concern from token scopes, would widen this PR, and would paper over a capacity problem rather than fix it. That trade-off is the maintainer's call, not something to slip into a hardening PR.

I'm holding my single re-run rather than spending it now: a fresh poll started while ~20 runs are still queued ahead would just time out again. I'll re-run E2E Gate once the ci.yml jobs for this commit have actually landed, and keep watching until this is green or closed.

If the queue is being starved by the ~20 open dependabot PRs each running the full matrix, that's worth a look independently of this PR.


Generated by Claude Code

Two hardening PRs each added a top-level `permissions: contents: read`
to this file -- 210696e put one below `concurrency:`, 85873f0 put another
above it. Neither conflicted in git and both were individually right, but
YAML forbids a repeated mapping key: GitHub rejected the workflow and
failed every run at startup, with zero jobs, listed under the file path
instead of "API Latency Smoke". The check has been off on main since
85873f0 landed, and the last real run was 2026-08-22.

Keep the block above `concurrency:` and fold the removed comment's point
about job-level elevation into it. The effective scope is unchanged:
`contents: read`, one job, no job-level override.

The pre-merge guard could not see this. `yaml.safe_load` resolves a
duplicate key last-one-wins and raises nothing, so
test_workflow_parses_as_yaml passed on a file GitHub refuses to run. Add
test_workflow_has_no_duplicate_keys next to it, using a SafeLoader
subclass whose mapping constructor rejects a repeat -- auto-discovered
over every workflow, like the guards around it.

Verified: 34 workflows parse and none repeats a key; every workflow still
declares a top-level permissions block; api-latency-smoke resolves to
contents: read with its single `smoke` job; 184 workflow tests pass; and
re-inserting the duplicate fails the new test while the old parse test
still passes -- the blind spot this closes.

No-PRD: CI-only change under .github/ and tests/, exempt from the product-record gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFma7np7KETvSM5EyyenBJ
@8090-software-factory

Copy link
Copy Markdown

✅ Drift Bot (ClawMetry): no drift detected

Drift Bot analyzed the changed files against this project's blueprints and requirements and found no drift.

Copy link
Copy Markdown
Owner Author

Pushed 36e773d, which fixes a break this hardening pass caused on main.

Two earlier PRs in this series each added a top-level permissions: contents: read to .github/workflows/api-latency-smoke.yml210696e below concurrency:, 85873f0 above it. They did not conflict in git, and both were individually correct, but a repeated mapping key makes the file invalid: GitHub rejects the workflow and fails every run at startup with zero jobs, listed under the file path rather than "API Latency Smoke". The API-latency check has been off on main since 85873f0 merged; the last real run was 2026-08-22. That is also why this PR's push run of that workflow is red.

The fix keeps one block and folds the removed comment's point about job-level elevation into it. Effective scope is unchanged: contents: read, one job, no job-level override.

The pre-merge guard could not catch it. yaml.safe_load resolves duplicate keys last-one-wins and raises nothing, so test_workflow_parses_as_yaml passed on a file GitHub refuses to run. 36e773d adds test_workflow_has_no_duplicate_keys alongside it — a SafeLoader subclass whose mapping constructor rejects a repeat, auto-discovered over every workflow like the guards around it. Re-inserting the duplicate fails the new test while the old parse test still passes.

Verified: 34 workflows parse and none repeats a key; every workflow still declares a top-level permissions: block; 184 workflow tests pass. The two sibling repos were scanned for the same pattern and are clean.

One thing worth a look: main stays broken until this merges. If that is too long to wait, the api-latency-smoke.yml hunk alone is a three-line revert that can go straight in.

The earlier E2E Gate failure on this branch was a timeout — it waited 30 minutes for the CI run, which sat queued for 48.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Closing the loop on my note above: all 36 checks are green on 36e773d, E2E Gate (required) included (passed 14:54:52).

No re-run was needed — the runner backlog drained on its own, the ci.yml jobs got runners at ~14:40 and all reported, so the gate's poll succeeded well inside its 1800s budget. The earlier red was purely the queue, exactly as diagnosed; nothing in the diff changed to fix it.

Nothing outstanding from my side. This is waiting on review.

One thing worth flagging for whoever merges: api-latency-smoke.yml is still broken on main — I confirmed the duplicate permissions key is present at 41d4acc (two top-level blocks, line 26). 36e773d here is the only fix, so API Latency Smoke stays dead on main until this merges or the fix is cherry-picked.


Generated by Claude Code

@vivekchand
vivekchand merged commit 850ede8 into main Aug 26, 2026
37 checks passed
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.

2 participants