From f5925d3fd2307a1851c2291587d65b868e95170d Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Mon, 3 Aug 2026 19:44:57 -0400 Subject: [PATCH] fix(ci): address PR policy security review Refs: PLAT-62 --- .github/workflows/callable-pr-policy.yaml | 28 +++- .github/workflows/policy.yaml | 36 ----- .github/workflows/release-on-merge.yaml | 1 - README.md | 4 +- test/pr-policy.test.mjs | 155 ++++++++++++++++++++-- 5 files changed, 169 insertions(+), 55 deletions(-) delete mode 100644 .github/workflows/policy.yaml diff --git a/.github/workflows/callable-pr-policy.yaml b/.github/workflows/callable-pr-policy.yaml index 591d40d..aefe26b 100644 --- a/.github/workflows/callable-pr-policy.yaml +++ b/.github/workflows/callable-pr-policy.yaml @@ -287,6 +287,18 @@ jobs: continue; } + // ── Sequence-item anchor declaration — fail closed ─────────────── + // Any line of the form `- &anchor` (with or without a mapping on + // the same line) is rejected. A standalone `- &name` can be + // followed on the next line by a flow-style mapping that the + // scanner would then misread as structural YAML. The multiline + // alias form `- *name` on subsequent steps is also unreachable + // without first declaring such an anchor. Reject unconditionally. + if (/^[ \t]*-[ \t]+&\S+/.test(line)) { + errs.push(filename + ':' + (i + 1) + ': sequence-item anchor declaration (&name) is not supported — anchors on steps may introduce flow mappings the scanner cannot safely resolve [fnv:' + fnv1a32(line.trim()) + ']'); + continue; + } + // ── Flow-style sequence step — fail closed only for structural keys ─ // `- { ... }` form cannot be safely resolved when it contains a // structural run: or uses: key (including quoted or escaped forms). @@ -371,8 +383,20 @@ jobs: ref = rawVal; } - // Local and docker refs are exempt from SHA pinning. - if (ref.startsWith('./') || ref.startsWith('docker://')) continue; + // Local refs (./) are exempt from SHA pinning. + if (ref.startsWith('./')) continue; + + // Docker refs require an immutable sha256 digest pin. + // Mutable tags, :latest, and bare image names are rejected. + // No # vX.Y.Z comment is required because the digest is the + // immutable identity. + if (ref.startsWith('docker://')) { + if (!/^docker:\/\/.+@sha256:[0-9a-f]{64}$/.test(ref)) { + const short = ref.length > 80 ? ref.slice(0, 77) + '\u2026' : ref; + errs.push(filename + ': "uses: ' + short + '" docker:// ref must be pinned by immutable digest (docker://@sha256:<64 lowercase hex>)'); + } + continue; + } // Validate SHA + version comment. // For quoted refs, combine the unquoted value with any external comment. diff --git a/.github/workflows/policy.yaml b/.github/workflows/policy.yaml deleted file mode 100644 index 9a43e96..0000000 --- a/.github/workflows/policy.yaml +++ /dev/null @@ -1,36 +0,0 @@ -name: policy - -# Self-caller: runs the org-wide PR policy gate on THIS repo's own pull requests. -# -# This workflow is new and will begin enforcing policy on PRs opened AFTER it -# merges to main. PRs that are already open at merge time are not retroactively -# re-evaluated until one of the trigger events fires again (e.g. a new commit). -# -# The check-run name this emits is `policy / pr`, matching the org standard -# documented in callable-pr-policy.yaml. Do not rename the job below — the -# job id (`policy`) is the first segment of that context. -# -# Uses a local path reference because this repo IS the source of the reusable; -# pinning to a SHA of itself would lag by one merge every time either file -# changes. Local `./` refs are exempt from the SHA-pin policy. - -on: - pull_request: - types: [opened, reopened, synchronize, edited, labeled, unlabeled, ready_for_review] - -concurrency: - group: policy-${{ github.event.pull_request.number }} - cancel-in-progress: true - -permissions: - contents: read - issues: read - pull-requests: read - -jobs: - policy: - uses: ./.github/workflows/callable-pr-policy.yaml - secrets: - JIRA_CLOUD_ID: ${{ secrets.JIRA_CLOUD_ID }} - JIRA_SERVICE_ACCOUNT_EMAIL: ${{ secrets.JIRA_SERVICE_ACCOUNT_EMAIL }} - JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }} diff --git a/.github/workflows/release-on-merge.yaml b/.github/workflows/release-on-merge.yaml index 4cd7292..7460afe 100644 --- a/.github/workflows/release-on-merge.yaml +++ b/.github/workflows/release-on-merge.yaml @@ -30,7 +30,6 @@ on: - ".github/workflows/**" - "!.github/workflows/ci.yaml" - "!.github/workflows/labeler.yaml" - - "!.github/workflows/policy.yaml" - "!.github/workflows/release-on-merge.yaml" workflow_dispatch: inputs: diff --git a/README.md b/README.md index a570a7b..6ac4b98 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ The two sanctioned deploy paths are merge to `main` triggering the pipeline and The supply-chain check operates in **diff mode**: for modified or renamed workflow files, the gate fetches the base-branch version at `pr.base.sha` and reports only violations whose normalized fingerprint is absent from the base. Added files must be fully compliant. Historical drift already present in the base branch is handled by the drift audit/remediation backlog, not by this gate. A failure to fetch the base version is a `POLICY-INFRA` error and the file is not silently grandfathered. -> **Workflow file constraints enforced by the supply-chain scanner.** Changed workflow files scanned by this policy must use block-style structural keys and inline `run:`/`uses:` values. The scanner fails closed on YAML forms it cannot safely resolve: flow-style step mappings (`- { uses: ... }`, `- { run: ... }`), escaped or Unicode-encoded structural keys in double-quoted strings (`"u\u0073es"`, `"r\u0075n"`), and YAML aliases or anchors on `run:`, `uses:`, or `permissions:` values (`run: *cmd`, `uses: &anchor ...`). Use the literal unquoted key forms and inline values in all workflow steps. +> **Workflow file constraints enforced by the supply-chain scanner.** Changed workflow files scanned by this policy must use block-style structural keys and inline `run:`/`uses:` values. The scanner fails closed on YAML forms it cannot safely resolve: flow-style step mappings (`- { uses: ... }`, `- { run: ... }`), sequence-item anchor declarations (`- &anchor { uses: ... }` and the multiline form `- &anchor` followed by a flow mapping on the next line), escaped or Unicode-encoded structural keys in double-quoted strings (`"u\u0073es"`, `"r\u0075n"`), and YAML aliases or anchors on `run:`, `uses:`, or `permissions:` values (`run: *cmd`, `uses: &anchor ...`). `docker://` action refs must carry an immutable sha256 digest pin (`docker://@sha256:<64 lowercase hex>`); mutable tags and bare image names are rejected. Use the literal unquoted key forms and inline values in all workflow steps. **`.github/workflows/callable-labeler.yaml`** — Org-wide PR auto-labeler. Label rules live inline here (single source of truth) — consumer repos need only a thin caller with `contents: read`, `pull-requests: write`, and `issues: write`; no per-repo labeler.yml. @@ -64,7 +64,7 @@ The supply-chain check operates in **diff mode**: for modified or renamed workfl **`.github/workflows/release-on-merge.yaml`** — Repo automation (not callable): cuts a tag and GitHub Release for **this** repo whenever a merge to `main` changes a reusable workflow, so Dependabot has a release to advance consumer SHA pins to (see the pinning policy below). -**`.github/workflows/policy.yaml`** — This repo's own thin caller of `callable-pr-policy.yaml`, so PR policy runs on `.github`'s own PRs. Uses a local path reference (`./.github/workflows/callable-pr-policy.yaml`); begins enforcing on PRs opened after its merge to main. +**`.github/workflows/policy.yaml`** — Intentionally absent from this PR. The self-caller must pin `Sea-Haven-Industries/.github/.github/workflows/callable-pr-policy.yaml` to a released 40-char SHA with a matching `# vX.Y.Z` comment; a mutable local `./` path reference is rejected by the supply-chain gate on modified workflow files. The follow-up PR can be opened once this PR merges and `release-on-merge.yaml` cuts the first release containing `callable-pr-policy.yaml`, then using `gh api /repos/Sea-Haven-Industries/.github/commits/vX.Y.Z --jq .sha` to obtain the pin. **`.github/workflows/labeler.yaml`** — This repo's own thin caller of `callable-labeler.yaml`, so the labeler runs on `.github`'s own PRs. diff --git a/test/pr-policy.test.mjs b/test/pr-policy.test.mjs index 1da782b..c6f4247 100644 --- a/test/pr-policy.test.mjs +++ b/test/pr-policy.test.mjs @@ -515,8 +515,9 @@ describe('validateWorkflowContent', () => { assert.deepEqual(v.validateWorkflowContent(content, BASE), []); }); - it('accepts docker:// ref without SHA requirement', () => { - const content = wf('uses: docker://alpine:3.19'); + it('accepts docker:// ref with valid sha256 digest', () => { + const digest = 'a' .repeat(64); + const content = wf('uses: docker://alpine@sha256:' + digest); assert.deepEqual(v.validateWorkflowContent(content, BASE), []); }); @@ -732,6 +733,144 @@ describe('validateWorkflowContent', () => { }); }); +// ── validateWorkflowContent — docker digest pinning ────────────────────────── +describe('validateWorkflowContent — docker digest pinning', () => { + const BASE = 'f.yaml'; + const GOOD_DIGEST = 'b'.repeat(64); + function wf(uses) { + return [ + 'on:', + ' workflow_call:', + 'permissions:', + ' contents: read', + 'jobs:', + ' j:', + ' runs-on: ubuntu-latest', + ' steps:', + ' - ' + uses, + ].join('\n'); + } + + it('accepts docker:// ref with valid lowercase 64-hex sha256 digest', () => { + assert.deepEqual(v.validateWorkflowContent(wf('uses: docker://alpine@sha256:' + GOOD_DIGEST), BASE), []); + }); + + it('accepts docker:// ref for image with tag+digest', () => { + assert.deepEqual(v.validateWorkflowContent(wf('uses: docker://ubuntu:22.04@sha256:' + GOOD_DIGEST), BASE), []); + }); + + it('rejects docker:// ref with mutable tag only', () => { + const errs = v.validateWorkflowContent(wf('uses: docker://alpine:3.19'), BASE); + assert.ok(errs.some(e => e.includes('sha256')), 'mutable tag must fail: ' + errs.join('; ')); + }); + + it('rejects docker:// ref with :latest tag', () => { + const errs = v.validateWorkflowContent(wf('uses: docker://ubuntu:latest'), BASE); + assert.ok(errs.some(e => e.includes('sha256')), ':latest must fail: ' + errs.join('; ')); + }); + + it('rejects bare docker:// ref with no tag or digest', () => { + const errs = v.validateWorkflowContent(wf('uses: docker://ubuntu'), BASE); + assert.ok(errs.some(e => e.includes('sha256')), 'bare image must fail: ' + errs.join('; ')); + }); + + it('rejects docker:// ref with uppercase in sha256 digest', () => { + const errs = v.validateWorkflowContent(wf('uses: docker://alpine@sha256:' + 'A'.repeat(64)), BASE); + assert.ok(errs.some(e => e.includes('sha256')), 'uppercase hex must fail: ' + errs.join('; ')); + }); + + it('rejects docker:// ref with short (63-char) sha256 digest', () => { + const errs = v.validateWorkflowContent(wf('uses: docker://alpine@sha256:' + 'a'.repeat(63)), BASE); + assert.ok(errs.some(e => e.includes('sha256')), 'short digest must fail: ' + errs.join('; ')); + }); + + it('rejects docker:// ref with wrong digest prefix (md5:)', () => { + const errs = v.validateWorkflowContent(wf('uses: docker://alpine@md5:' + 'a'.repeat(32)), BASE); + assert.ok(errs.some(e => e.includes('sha256')), 'non-sha256 digest must fail: ' + errs.join('; ')); + }); +}); + +// ── validateWorkflowContent — anchored flow step ───────────────────────────── +describe('validateWorkflowContent — anchored flow step', () => { + const BASE = 'f.yaml'; + function wf(step) { + return 'permissions: {}\n' + step; + } + + it('rejects anchored flow-style step with uses (- &step { uses: ... })', () => { + const errs = v.validateWorkflowContent(wf(' - &step { uses: actions/checkout@v4 }'), BASE); + assert.ok(errs.some(e => e.includes('anchor') || e.includes('not supported')), 'anchored flow uses must fail: ' + errs.join('; ')); + }); + + it('rejects anchored flow-style step with run (- &step { run: ... })', () => { + const errs = v.validateWorkflowContent(wf(' - &step { run: echo hi }'), BASE); + assert.ok(errs.some(e => e.includes('anchor') || e.includes('not supported')), 'anchored flow run must fail: ' + errs.join('; ')); + }); + + it('rejects anchored flow-style even for non-structural keys', () => { + const errs = v.validateWorkflowContent(wf(' - &data { os: ubuntu, node: 24 }'), BASE); + assert.ok(errs.some(e => e.includes('anchor') || e.includes('not supported')), 'any anchored flow step must fail: ' + errs.join('; ')); + }); + + it('still rejects plain flow-style step with structural uses key', () => { + const errs = v.validateWorkflowContent(wf(' - { uses: actions/checkout@v4 }'), BASE); + assert.ok(errs.some(e => e.includes('flow-style')), 'plain flow uses must still fail: ' + errs.join('; ')); + }); + + it('rejects block-style step with standalone sequence-item anchor (- &ref)', () => { + const content = [ + 'permissions: {}', + 'steps:', + ' - &ref', + ' uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b3 # v7.0.1', + ].join('\n'); + const errs = v.validateWorkflowContent(content, BASE); + assert.ok(errs.some(e => e.includes('anchor') || e.includes('not supported')), 'standalone - &anchor step must fail: ' + errs.join('; ')); + }); + + it('rejects multiline anchored uses: - &step / { uses: ... }', () => { + const content = [ + 'permissions: {}', + 'steps:', + ' - &step', + ' { uses: actions/checkout@v4 }', + ].join('\n'); + const errs = v.validateWorkflowContent(content, BASE); + assert.ok(errs.some(e => e.includes('anchor') || e.includes('not supported')), 'multiline - &step / { uses } must fail: ' + errs.join('; ')); + }); + + it('rejects multiline anchored run: - &step / { run: ... }', () => { + const content = [ + 'permissions: {}', + 'steps:', + ' - &step', + ' { run: echo hi }', + ].join('\n'); + const errs = v.validateWorkflowContent(content, BASE); + assert.ok(errs.some(e => e.includes('anchor') || e.includes('not supported')), 'multiline - &step / { run } must fail: ' + errs.join('; ')); + }); + + it('no regression: plain block-style step without anchor passes pin checks normally', () => { + const PIN = 'actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b3 # v7.0.1'; + const content = [ + 'permissions: {}', + 'steps:', + ' - uses: ' + PIN, + ].join('\n'); + assert.deepEqual(v.validateWorkflowContent(content, BASE), []); + }); + + it('non-sequence mapping-value anchor is not flagged (foo: &anchor value)', () => { + const content = [ + 'permissions: {}', + 'env:', + ' TOKEN: &tok my-value', + ].join('\n'); + const errs = v.validateWorkflowContent(content, BASE); + assert.ok(!errs.some(e => e.includes('sequence-item anchor')), 'mapping-value anchor must not trigger sequence-item rule: ' + errs.join('; ')); + }); +}); + // ── checkCommitLimit ────────────────────────────────────────────────────────── describe('checkCommitLimit', () => { @@ -1343,18 +1482,6 @@ describe('static YAML assertions', () => { assert.ok(/^ pr:$/m.test(yamlText), 'job id must be "pr"'); }); - it('policy.yaml uses job id "policy"', () => { - const policyYaml = readFileSync(join(__dirname, '../.github/workflows/policy.yaml'), 'utf8'); - assert.ok(/^ policy:$/m.test(policyYaml), 'caller job id must be "policy"'); - }); - - it('policy.yaml does not use pull_request_target as a trigger', () => { - const policyYaml = readFileSync(join(__dirname, '../.github/workflows/policy.yaml'), 'utf8'); - assert.ok( - !/^\s*pull_request_target\s*:/m.test(policyYaml), - 'policy.yaml must not declare pull_request_target as an on: trigger' - ); - }); }); // ── normalizeViolationFingerprint ────────────────────────────────────────────