fix(ci): drop unused write scopes from the xtest xct job (DSPX-4847) - #610
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ChangesPermissions hardening
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to The permission reduction preserves xct’s required read access and does not affect pull-request result comments. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the workflow gate Comment |
|
Verified on this PR's own run (35266888678) — all green. The scopes are actually gone. No Nothing needed them. All 10 PR commenting still works.
Note for whoever reviews: |
The xct job declares checks: write and pull-requests: write, but uses neither. There is no github.token, GITHUB_TOKEN, github-script, gh CLI or API call anywhere in the job, and none of the composite actions it invokes touch the token either. PR comments are posted by publish-results, which holds its own pull-requests: write; no junit publisher exists in this file. The scopes are not inert. On a workflow_call the token xct holds belongs to the *calling* repository, so every caller is forced to raise its ceiling to match -- and xct is eight concurrent matrix legs each building and running Go, Java and JS dependency trees plus buf, uv, maven and npm. That is a lot of third-party code sitting next to a token that can write checks and PR comments on someone else's repo. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
5344548 to
207ea13
Compare
|
The caller granted no explicit permissions, so every job in the called workflow ran with the repo default: Contents, Packages, Actions, Checks, PullRequests, SecurityEvents, Statuses, Deployments, Issues and Pages all write. That token sat beside 'npm ci' in lib, cli and web-app, and beside the Go/Java/JS dependency builds in the called xtest workflow. Declare the ceiling at the calling job instead. A called workflow can only narrow a caller's grant, so this bounds the whole tree at contents: read, packages: read and pull-requests: write. opentdf/tests#610 had to land first: xtest's xct job declared checks: write and pull-requests: write that it never used, and a callee asking for more than the caller grants fails the call. With that merged, checks: write is no longer needed anywhere here; pull-requests: write stays for xtest's publish-results job, which comments coverage on the PR. This file is the caller, so it runs from the PR ref and the change tests itself. Per-job blocks inside reusable_build-and-test.yaml would narrow this further but that file is pinned @main and cannot be validated from a PR; tracked separately. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
* fix(ci): scope down the build-and-test GITHUB_TOKEN (DSPX-4847) The caller granted no explicit permissions, so every job in the called workflow ran with the repo default: Contents, Packages, Actions, Checks, PullRequests, SecurityEvents, Statuses, Deployments, Issues and Pages all write. That token sat beside 'npm ci' in lib, cli and web-app, and beside the Go/Java/JS dependency builds in the called xtest workflow. Declare the ceiling at the calling job instead. A called workflow can only narrow a caller's grant, so this bounds the whole tree at contents: read, packages: read and pull-requests: write. opentdf/tests#610 had to land first: xtest's xct job declared checks: write and pull-requests: write that it never used, and a callee asking for more than the caller grants fails the call. With that merged, checks: write is no longer needed anywhere here; pull-requests: write stays for xtest's publish-results job, which comments coverage on the PR. This file is the caller, so it runs from the PR ref and the change tests itself. Per-job blocks inside reusable_build-and-test.yaml would narrow this further but that file is pinned @main and cannot be validated from a PR; tracked separately. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com> * pithify comments * Remove 'what are github workflow permissions' comment --------- Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
2 participants
What
xctdeclares two write scopes it never uses:Both comments are stale. This PR removes them, leaving
contents: read(checkout) andpackages: read(ghcr pulls viastart-up-with-containers).Why they're dead
github.token,GITHUB_TOKEN,github-script,ghCLI, or API call of any kind anywhere in thexctjob body (xtest.yml:329-833). The only secrets it references areBUF_TOKENandPERSONAL_ACCESS_TOKEN_OPENTDF, neither of which is governed by these scopes.xtest/setup-cli-tool/action.yaml,opentdf/platform/test/start-up-with-containers,opentdf/platform/test/start-additional-kas.publish-resultsjob (:1437), which holds its ownpull-requests: write. That job is untouched here.checks: writehas nothing to publish.Why it's worth doing
On a
workflow_callthe tokenxctholds is the caller's, and a called workflow's declared permissions force the caller to raise its ceiling to match. So these two scopes are granted to every downstream repo that calls xtest — currentlyopentdf/web-sdk, where they're the only thing preventing a tight caller-sidepermissionsblock (that's PR B of DSPX-4847).And
xctis a poor place to hold them: eight concurrent matrix legs, each building and running Go, Java, and JS dependency trees plusbuf,uv,maven, andnpm.Testing
Self-testing —
xtest.ymlrunson: pull_request, so this PR's own xtest run executes the modified workflow from this ref. If anything inxctactually needed either scope, this PR goes red.To confirm the scopes are really gone, check any
xctleg's Set up job log:ChecksandPullRequestsshould now readnone.publish-resultsshould still comment on this PR as usual.actionlintis clean on the file (theSC2086infos at:616/:634are pre-existing and untouched).Not included
Deliberately scoped to the two dead scopes:
xtestcapstone job (:1479) could go fromcontents: readto{}— it only echoes to$GITHUB_STEP_SUMMARY— but that's a separate, smaller argument.PERSONAL_ACCESS_TOKEN_OPENTDF(:556) is a long-lived user-scoped PAT passed asBUF_INPUT_HTTPS_PASSWORDintomake. It's empty on everyworkflow_callrun (nosecrets:block in theworkflow_callcontract) andPrepare java clistill succeeds, which suggests it may buy nothing — but it's populated on this repo's own runs, so that needs confirming separately before removal.Tracked as DSPX-4847.
Summary by CodeRabbit