feat(scripts): enforce job-level permissions in the workflow permissions gate - #2553
Open
WilliamBerryiii wants to merge 4 commits into
Open
feat(scripts): enforce job-level permissions in the workflow permissions gate#2553WilliamBerryiii wants to merge 4 commits into
WilliamBerryiii wants to merge 4 commits into
Conversation
…ons gate Test-WorkflowPermissions.ps1 matched only `(?m)^permissions:` and scored the repository 100% while jobs across six workflows carried no permissions block of their own. The gate did not enforce the job-level convention it documented. Parse each workflow with ConvertFrom-Yaml, enumerate jobs from the parsed object graph, and apply a four-state classification: absent workflow + absent job -> violation (inherits repo/org default) empty workflow + absent job -> pass (zero scopes available to inherit) populated workflow + absent job -> violation (implicit, unauditable grant) any workflow + present job -> pass The empty-block pass is the distinction third-party scanners miss; it is why alert #348 against pr-review.lock.yml was a false positive. Remediate the single offender: beval.yml job `evaluate` now declares `contents: read`, which it already inherited. Effective permissions are unchanged; no job's privilege was widened and no generated lock file was edited. Supporting changes: - Add MissingJobPermissions to the ViolationType ValidateSet, additively - Report file-level and job-level metrics separately so compliance-score and violation-count cannot contradict each other - Declare a SARIF rule per violation type, floor startLine at 1, and normalize paths to forward slashes - Correct enforcement claims in workflows.instructions.md, security-model.md, and scripts/security/README.md to describe the rule actually enforced Tests cover every matrix row, null/flow/scalar permissions shapes, four-space job indentation, a lock-file-shaped regression, nested step-level permissions keys, SARIF contract validity, and malformed-workflow degradation. The workflow-with-permissions fixture was in the violating state and gained an explicit job block rather than having its assertion relaxed.
…ibution
Implements review findings RV-001, RV-002, and RV-003 from the job-level
workflow permissions gate review.
RV-001: The documentation added alongside the gate asserted that under a
workflow-level `permissions: {}` a job "can hold no scope regardless of whether
it declares a block". That is false. GitHub adjusts permissions first at the
workflow level and then at the job level, and a job-level block configures a
different set for that job. An empty workflow-level block is a default, not a
ceiling, so a job declaring `contents: write` beneath it does receive that
scope. All affected surfaces now state the accurate reason: a job that declares
no block inherits an empty set and therefore holds nothing.
The gate's four-state verdicts are unchanged; only the stated rationale was
wrong.
RV-002: Add a regression guard asserting no violation path contains a backslash.
The forward-slash normalization had no test, and because the SARIF upload step
is continue-on-error, an unnormalized artifactLocation.uri would fail silently.
RV-003: Constrain job line attribution to the region after the top-level `jobs:`
key. The previous whole-file search could report a same-named key declared
earlier, such as a workflow_call output sharing a job name. Reporting precision
only; job identity has always come from the parsed object graph.
…it job metrics Addresses findings from an independent review pass over the branch diff. workflow-permissions-scan.yml had no PowerShell-Yaml install step. The gate previously used a regex and had no module dependency; it now hard-requires PowerShell-Yaml 0.4.7 via #Requires and Import-Module -ErrorAction Stop. That workflow runs on ubuntu-latest with only a checkout step, so the gate would have failed on every PR. Both upload steps are continue-on-error, so the failure could have been quiet. Adds the setup-ps-modules composite action, matching the pattern pr-validation.yml already uses for the same reason. JobsWithPermissions credited jobs that declare no block. Under an empty workflow-level block the check returns early, so the job-level violation count is always zero and every job was counted as declaring permissions. The gate reported "193/193 declare their own permissions" while pr-review.lock.yml job pre_activation has no block at all. Split into two honest metrics: JobsPassing 193 jobs that pass the job-level check JobsDeclaringOwnBlock 188 jobs that actually declare a block The remaining five pass by inheriting an empty workflow-level block. Log lines and step-summary rows now distinguish the two, and a test pins the difference. Also corrects a fixture header comment that repeated the inheritance claim fixed in the previous commit, and reworks success messages that said jobs "declare permissions" when some pass by inheritance.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2553 +/- ##
==========================================
- Coverage 82.75% 82.57% -0.18%
==========================================
Files 155 155
Lines 20960 21100 +140
Branches 13 13
==========================================
+ Hits 17345 17424 +79
- Misses 3613 3674 +61
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Eval Execution✅ Status: Passed
No changed AI artifacts required evaluation. |
jkim323
requested changes
Jul 30, 2026
jkim323
left a comment
jkim323
left a comment
Collaborator
There was a problem hiding this comment.
Left a couple of things to be looked at!
Comment on lines
+487
to
+493
| if ($model.ParseFailed) { | ||
| $unparsedFiles++ | ||
| $parseMessage = "Workflow '$($file.Name)' could not be parsed as YAML and was not evaluated: $($model.ParseError)" | ||
| Write-SecurityLog " WARN: $parseMessage" -Level Warning -CIAnnotation | ||
| Write-CIAnnotation -Message $parseMessage -Level 'Warning' -File $relativePath -Line 1 | ||
| continue | ||
| } |
Collaborator
There was a problem hiding this comment.
ParseFailed currently fails open: parse/read errors return no violations, so -FailOnViolation exits 0. The new integration test explicitly codifies this behavior.
This regresses coverage from the prior raw-text check, which still evaluated malformed files for a missing top-level permissions: block. A YamlDotNet/GitHub parser mismatch can now remove a workflow from the gate entirely.
It also makes outputs contradictory: parse-failed files reduce compliance-score, but produce violation-count=0 and is-compliant=true. Treat parse failures as violations (or otherwise include them in compliance status and the exit condition).
Suggested fix
if ($model.ParseFailed) {
$unparsedFiles++
$parseMessage = "Workflow '$($file.Name)' could not be parsed as YAML and was not evaluated: $($model.ParseError)"
$violation = [DependencyViolation]::new()
$violation.File = $relativePath
$violation.Line = 1
$violation.Type = 'workflow-permissions'
$violation.Name = $file.Name
$violation.ViolationType = 'MissingPermissions'
$violation.Severity = 'High'
$violation.Description = $parseMessage
$violation.Remediation = 'Fix the YAML syntax so the workflow permissions gate can evaluate this file'
$violation.Metadata = @{ FullPath = $file.FullName; ParseError = $model.ParseError }
$report.AddViolation($violation)
Write-SecurityLog " FAIL: $parseMessage" -Level Error -CIAnnotation
Write-CIAnnotation -Message $parseMessage -Level 'Error' -File $relativePath -Line 1
continue
Comment on lines
+241
to
+256
| $start = 0 | ||
| for ($i = 0; $i -lt $RawLines.Count; $i++) { | ||
| if ($RawLines[$i] -match '^jobs\s*:\s*(#.*)?$') { | ||
| $start = $i + 1 | ||
| break | ||
| } | ||
| } | ||
|
|
||
| $pattern = '^\s+' + [regex]::Escape($JobName) + '\s*:\s*(#.*)?$' | ||
| for ($i = $start; $i -lt $RawLines.Count; $i++) { | ||
| if ($RawLines[$i] -match $pattern) { | ||
| return $i + 1 | ||
| } | ||
| } | ||
|
|
||
| return 0 |
Collaborator
There was a problem hiding this comment.
Get-JobDeclarationLine can still attribute a finding to the wrong line.
If the literal jobs: anchor is not found (for example, quoted or flow-style YAML), $start remains 0 and the search silently scans the entire file, allowing an earlier same-named key such as a workflow_call output to match. Even with a normal anchor, ^\s+ accepts any nested indentation, so a matching key inside an earlier job’s matrix or outputs block can win before the actual job declaration.
Please constrain matching to the direct child indentation under jobs: and return 0 when no reliable block-style anchor is available.
Suggested fix
$start = -1
for ($i = 0; $i -lt $RawLines.Count; $i++) {
if ($RawLines[$i] -match '^["'']?jobs["'']?\s*:\s*(#.*)?$') {
$start = $i + 1
break
}
}
# No anchor means no constrained region; report no line rather than
# falling back to a whole-file search that can match an unrelated key.
if ($start -lt 0) {
return 0
}
# Job keys sit at the first indent level inside 'jobs:'; deeper keys are not jobs.
$jobIndent = -1
for ($i = $start; $i -lt $RawLines.Count; $i++) {
if ($RawLines[$i] -match '^(\s+)\S') {
$jobIndent = $Matches[1].Length
break
}
}
if ($jobIndent -lt 0) {
return 0
}
$pattern = '^\s{' + $jobIndent + '}' + [regex]::Escape($JobName) + '\s*:\s*(#.*)?$'
for ($i = $start; $i -lt $RawLines.Count; $i++) {
if ($RawLines[$i] -match $pattern) {
return $i + 1
}
}
return 0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
scripts/security/Test-WorkflowPermissions.ps1checked one thing: whether a file contained a top-levelpermissions:block, matched with(?m)^permissions:against raw text. It had no job awareness at all. It scored this repository 100% (64/64) while jobs across six workflows carried no declaration of their own.The convention in
.github/instructions/workflows.instructions.mdrequires job-level declarations, anddocs/security/security-model.mdasserted that all workflows already had them. Neither was enforced, and the second was not true.This change taught the gate to parse each workflow with
ConvertFrom-Yaml, enumerate its jobs from the parsed object graph, and apply a four-state classification.permissions: {}Why row 3 is the one that matters
A measurement of all 65 workflows found 0 absent, 7 empty, 58 populated top-level blocks. A rule built only on row 1 would have been unreachable — it would have detected the empty set and shipped as a no-op. Row 3 gives the gate a reachable failure state, and it found exactly one on this tree:
.github/workflows/beval.ymljobevaluate.That job now declares
contents: read— which it already inherited. The effective permission set is identical before and after. No job's privilege was widened anywhere in this change, and no generatedgh-awlock file was edited.Why row 2 is a deliberate pass
An empty workflow-level block grants nothing, so a job beneath it that declares no block inherits an empty set and holds no scope. This is precisely the distinction the third-party Poutine scanner cannot make — its
utils.empty()helper treats an empty map identically to an absent one — which is why it raised code scanning alert #348 against.github/workflows/pr-review.lock.yml, a file that implements Poutine's own documented remediation. That alert was dismissed as a false positive. This change adds no.poutine.ymlsuppression; a precise local gate was preferred over silencing a scanner.Worth stating plainly: an empty workflow-level block is a default, not a ceiling. A job that declares its own block beneath one still receives what it declares, because job-level permissions replace the workflow-level set rather than being capped by it.
Supporting changes
SecurityClasses.psm1gains aMissingJobPermissionsvalue in theViolationTypeValidateSet, which would otherwise throw on the first job-level finding. Additive only.compliance-scoreandviolation-countcannot contradict each other under a mixed model.ruleId, floorsstartLineat 1, and normalizes paths to forward slashes. The previous version hardcoded a singleruleIdandstartLine = 1; since the upload step iscontinue-on-error, a malformed document would have failed silently.workflow-permissions-scan.ymlgains thesetup-ps-modulesstep. The script now depends on PowerShell-Yaml, which is not preinstalled onubuntu-latest.Reviewer note on
compliance-scoreThe score is now denominated in workflow-level plus job-level checks — 257 checks on this tree instead of 64 files. It remains a 0–100 percentage, so the downstream contract shape in
workflow-permissions-scan.ymlandpr-validation.ymlis unchanged, but historical scores are not directly comparable.Two job metrics are published deliberately: 193/193 jobs pass the job-level check, but only 188/193 declare their own block. The remaining five pass by inheriting an empty workflow-level block. Collapsing these into one number would have claimed every job declares permissions, which is false.
Related Issue(s)
Closes #2541
Split out from #2527, which covers the full Poutine scan of
mainand remains open independently.Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
AI Artifacts:
hve-builderand addressed all actionable findings.github/instructions/*.instructions.md).github/prompts/*.prompt.md).github/agents/*.agent.md).github/skills/*/SKILL.md).github/hooks/*/*.json)evals/)Other:
.ps1,.sh,.py)Sample Prompts (for AI Artifact Contributions)
The only AI artifact touched is
.github/instructions/workflows.instructions.md, which is passive guidance applied by glob rather than an invocable artifact. It gains the per-job permissions requirement and the four-state classification table so the documented rule matches the enforced one.User Request:
Not directly invocable. The instruction file applies automatically when editing files under
.github/workflows/.Execution Flow:
An agent editing a workflow file receives the permissions convention as context, including the requirement that every job declare its own
permissions:block whenever the workflow-level block grants any scope, and thepermissions: {}exception.Output Artifacts:
None. The file contributes guidance only.
Success Indicators:
A workflow authored under this guidance passes
npm run lint:permissionswithout aMissingJobPermissionsfinding.For detailed contribution requirements, see:
Testing
Automated validation
npm run lint:permissions(pre-remediation)beval.ymljobevaluate, exit 1npm run lint:permissions(post-remediation)npm run test:ps -- -TestPath "scripts/tests/security/"npm run lint:psnpm run lint:yamlnpm run lint:mdnpm run validate:localnpm run validate:docsdocs:lintbecausedocs/docusaurusdependencies are not installed here. Unrelated to this change; no Docusaurus files are touched.The pre-remediation failure is deliberate evidence: it demonstrates the gate has a reachable failure state, which the original top-level-only rule did not.
Test coverage added
A case per matrix row; null, flow-style, and scalar
permissionsvalue shapes; four-space job indentation; a lock-file-shaped regression asserting row 2 passes; a nested step-levelpermissionskey that must not count as a job declaration; SARIFruleIdresolution andstartLinefloor; malformed-workflow degradation; a no-backslash assertion on violation paths; a job-line-attribution regression using aworkflow_calloutput that shares a job name; and a metrics case pinning that a job passing by inheritance is not counted as declaring a block.The existing
workflow-with-permissions.ymlfixture was itself in the row-3 state. Rather than relax its passing assertion, the fixture was brought into compliance so the assertion stays meaningful. No existing fixture or assertion was deleted.Security analysis
scripts/security/ps-module-versions.jsonand installed via the cachedsetup-ps-modulescomposite action.Manual testing
Manual testing was not performed. All verification was automated, plus diff-level inspection of the privilege-neutrality claim.
Checklist
Required Checks
AI Artifact Contributions
hve-builderreview mode to review contributionhve-builderreviewRequired Local Checks
The following local-safe validation commands must pass before merging:
npm run validate:localnpm run validate:docs(N/A — fails locally atdocs:lintbecausedocs/docusaurusdependencies are not installed in this environment; unrelated to this change, which touches no Docusaurus files. See Pin eslint to 9.39.4 in docs/docusaurus and regenerate lockfile (PR #2519) #2534.)npm run spell-checknpm run lint:md-linksSecurity Considerations
Additional Notes
An independent review pass caught a CI-breaking defect
A subagent review of the diff found that
workflow-permissions-scan.ymlhad no PowerShell-Yaml install step. The original script was pure regex with no module dependency; the new one hard-requires the module via both#RequiresandImport-Module -ErrorAction Stop. That workflow runs onubuntu-latestwith only a checkout step, so the gate would have failed on every PR — and because both upload steps arecontinue-on-error: true, it could have failed quietly. The same review found the job metric was crediting jobs that declare no block. Both are fixed here, with the metric split into two honest numbers.Known behavior: unparseable workflows do not block
A workflow that fails to parse emits a warning annotation and is not counted as compliant, so the score drops — but it produces no violation, so
-FailOnViolationdoes not trip on it. This matches the behavior the plan specified and mirrorsTest-DangerousWorkflow.ps1. A malformed workflow would be caught bylint:yaml(actionlint) in the same PR gate, so the defense-in-depth is intact. Flagging it explicitly because reviewers may prefer a fail-closed posture for a security gate; that would be a deliberate follow-up rather than a silent change here.Backwards compatibility
The
MissingJobPermissionsValidateSet addition is purely additive.compliance-scoreandviolation-countkeep their shape and type; only the score denominator changes, which is called out above for anyone tracking the number over time.