Skip to content

ci(security): recover Token-Permissions 0 -> 10 — measured, not inferred (#127) - #128

Merged
GmanFooFoo merged 2 commits into
mainfrom
linus/2026-07-28-token-permissions
Jul 28, 2026
Merged

ci(security): recover Token-Permissions 0 -> 10 — measured, not inferred (#127)#128
GmanFooFoo merged 2 commits into
mainfrom
linus/2026-07-28-token-permissions

Conversation

@GmanFooFoo

@GmanFooFoo GmanFooFoo commented Jul 28, 2026

Copy link
Copy Markdown
Member

Closes the first of the four categories in issue #127, per the work order neckarshore-planning/docs/plans/2026-07-27-trustscope-127-linus-workorder.md.

The change

One line of substance. .github/workflows/dependabot-auto-merge.yml carried contents: write + pull-requests: write at workflow level; both now sit on the dependabot-auto-merge job, and the workflow default is contents: read.

Effective permissions for the job are unchanged — auto-merge behaves identically. Scorecard reads the top-level block and caps Token-Permissions at 0 for the whole repository on any top-level write, which is why this one line cost 7,5 weighted points.

Introduced by PR #64 on 2026-07-10, four days after the last run that measured this check at 10.

Measured, not inferred

The work order's DoD is explicit that "the YAML looks right" does not count. Method: git archive origin/main into two throwaway trees, apply the candidate change to one, run Scorecard --local against both. The product repo was never written to during measurement.

Scorecard pinned by digest to the same build every other estate number was produced at:

gcr.io/openssf/scorecard@sha256:54c7ea4ddec6e3941887cb7933898c352f59e7f59e17a7a730f97ed348a8dfce
GitVersion: v5.1.1-45-g40bbc9c9   GitCommit: 40bbc9c958
before after
Token-Permissions 0 10
aggregate 5,9 6,7 (arithmetic)

The aggregate figure is arithmetic, not a second live run — but the weights were validated first by reproducing the measured 5,9 exactly from the 16 applicable per-check scores, so it is arithmetic on a checked ruler.

The open question this settled: e2e.yml:212 carries a job-level contents: write, so it was genuinely unclear whether removing the top-level write would reach 10 or merely leave 0. It reaches 10 — job-level writes are a Warn, not a cap. Both remaining warns are job-level.

Second commit: an unrelated test-hygiene rider

lib/resolve-report.test.ts restored its console.error spy on the test's last line, outside any try/finally. Any assertion throwing above it left console.error mocked for the rest of that Vitest worker — a failing test silencing its neighbours. vitest.config.ts sets neither restoreMocks nor clearMocks, so nothing else restored it (checked, not assumed).

Provenance: a CodeRabbit finding on PR #122 that was resolved-to-unblock a live security fix rather than fixed. Carried on a board item since, with the instruction to ride the next PR touching this repo rather than spend a PR of its own. It is a separate commit so the security diff stays readable on its own.

Baseline moved — worth knowing before anyone quotes a number

Re-measured today before touching anything, as the work order's §1c required. The live aggregate is 5,9, not the 5,5 measured on 07-26: overnight Dependabot merges took Vulnerabilities from 0 to 6 (4 open advisories, down from 17).

Two further findings from that run, neither of them mine to fix here:

  1. Maintained scores 0 for the reason "project was created in last 90 days". That is repo age, not a defect, and it self-heals. It also carries 7,5 weight — so part of the gap to any target number is simply not buyable.
  2. Code-Review scores 0"Found 0/24 approved changesets". That is the known estate ceiling and it is category 3 of the work order, still open.

Recomputed arc at today's numbers: Token-Permissions alone 6,7 · plus Vulnerabilities fully cleared 7,1 · plus Maintained ageing in 7,9.

Important: the 7,1 in the campaign narrative survives, but by a different route than the work order assumed, and it now depends on clearing 4 advisories rather than 17. Any public number must state the Scorecard version it was quoted at — issue #127 itself was generated at v5.5.0 while every estate measurement runs v5.1.1-45. That reconciliation is MASCHIN's, not settled here.

Verification

  • npm run typecheck — clean
  • npm run lint — clean
  • npm test — 274 passed, 6 skipped, 33 files
  • YAML parses
  • Token-Permissions re-measured at the pinned build: 10

🤖 Generated with Claude Code

https://claude.ai/code/session_01KU225SLKQ6xHT9qMMJvZYm

Summary by CodeRabbit

  • Chores

    • Improved automation workflow permissions by applying write access only where required.
  • Tests

    • Improved test cleanup to ensure temporary console error handling is consistently restored, preventing interference with other tests.

GmanFooFoo and others added 2 commits July 28, 2026 09:30
)

Scorecard's Token-Permissions check reads the workflow-level `permissions:`
block and caps the check at 0 for the ENTIRE repository on any top-level
write scope. PR #64 (2026-07-10) introduced `contents: write` +
`pull-requests: write` at the top level — four days after the last run that
measured this check at 10. That single line is the whole regression.

The two scopes `gh pr merge --auto` actually needs now live on the one job
that needs them; the workflow default is `contents: read`. Effective
permissions for the job are unchanged, so auto-merge behaviour is identical.

MEASURED, not inferred — OpenSSF Scorecard pinned to the same build every
other estate number was produced at, v5.1.1-45-g40bbc9c9
(gcr.io/openssf/scorecard@sha256:54c7ea4ddec6e3941887cb7933898c352f59e7f59e17a7a730f97ed348a8dfce),
run `--local` against a `git archive` of origin/main before and after:

  Token-Permissions  0 -> 10
  aggregate          5.9 -> 6.7 (weights validated by reproducing the
                                 measured 5.9 from the per-check scores)

The open question this measurement settled: `e2e.yml:212` carries a job-level
`contents: write`, so it was genuinely unclear whether removing the top-level
write would reach 10 or merely leave 0. Job-level writes are a Warn, not a cap
— both remaining warns are job-level and the check still scores 10.

Refs: #127

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KU225SLKQ6xHT9qMMJvZYm
`errSpy.mockRestore()` sat on the last line of the test, outside any
try/finally. If an assertion above it threw, console.error stayed mocked for
the remainder of that Vitest worker and silently swallowed output in every
later test sharing it — a failing test poisoning its neighbours.

Verified against the ground rather than assumed: vitest.config.ts sets neither
`restoreMocks` nor `clearMocks`, so nothing else restores it.

Provenance: a CodeRabbit finding on PR #122 that was resolved-to-unblock a
live security fix rather than fixed. Resolving a thread is only honest if the
item survives the click; this is that survival.

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

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
trustscope Ready Ready Preview, Comment Jul 28, 2026 7:31am

Request Review

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 112cb52f-67f4-477f-b91d-ab389003c45f

📥 Commits

Reviewing files that changed from the base of the PR and between 8fcbf2f and 1feba10.

📒 Files selected for processing (2)
  • .github/workflows/dependabot-auto-merge.yml
  • lib/resolve-report.test.ts

📝 Walkthrough

Walkthrough

The Dependabot auto-merge workflow now limits write permissions to its job, and the report-generation error test guarantees restoration of its console.error spy after execution or assertion failures.

Changes

Workflow permission scoping

Layer / File(s) Summary
Job-scoped auto-merge permissions
.github/workflows/dependabot-auto-merge.yml
Workflow-level contents access is read-only, while the auto-merge job receives contents: write and pull-requests: write permissions.

Test mock cleanup

Layer / File(s) Summary
Guaranteed console spy restoration
lib/resolve-report.test.ts
The generic error test restores its console.error spy in a finally block around the report call and assertions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the security-oriented permission change and its measured Token-Permissions improvement.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch linus/2026-07-28-token-permissions

Comment @coderabbitai help to get the list of available commands.

@GmanFooFoo
GmanFooFoo merged commit 7b5d877 into main Jul 28, 2026
14 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.

1 participant