Skip to content

ci: add scanning, and pin the supply chain - #285

Open
Murali Chillakuru (mchillakuru) wants to merge 2 commits into
responsibleai:mainfrom
mchillakuru:ci/sec-009-dependabot-codeql
Open

ci: add scanning, and pin the supply chain#285
Murali Chillakuru (mchillakuru) wants to merge 2 commits into
responsibleai:mainfrom
mchillakuru:ci/sec-009-dependabot-codeql

Conversation

@mchillakuru

Copy link
Copy Markdown

Summary

Two related gaps in CI.

Scanning. There was no Dependabot config, no CodeQL workflow, and no dependency
audit, so a known-vulnerable dependency could sit in the tree indefinitely without
anything reporting it.

Supply chain. Every action reference used a floating tag, so a compromised or
force-moved tag would run attacker-controlled code with the workflow token. All 44
references across 10 workflows are now pinned to commit SHAs, following the
@sha # version convention already used in scorecard.yml.

review-escalation.yml also interpolated workflow_dispatch inputs directly into a
shell block. A GitHub expression is substituted as text before the shell runs, so a
value containing shell metacharacters would execute as part of the command. Those
inputs now arrive through env: and are quoted at the point of use.

Closes: SEC-009 (no dependency or code scanning) · SEC-013 (workflow script injection)

Commits

  • ci: pin actions to commit SHAs and pass workflow inputs via env
  • ci: add Dependabot, CodeQL, and a dependency audit gate

Testing

pytest tests/ -q

Full suite green on this branch; 11 files changed, 292 insertions(+), 45 deletions(-). Every change has a regression test, and the
suite was re-run after each commit rather than only at the end.

Notes for reviewer

Each SHA was resolved from its tag through the GitHub API and then verified to be a
real commit in that repository, rather than transcribed by hand. Pins already present
in scorecard.yml were left untouched. All ten workflow files were re-parsed as YAML
after the change.

Requires the workflow token scope to merge, since it edits .github/workflows/.

Risk and rollback

Each commit is a single concern and can be reverted independently. See the notes above
for anything that does not revert cleanly.

Murali Chillakuru added 2 commits July 30, 2026 00:04
Nothing in CI reported a known-vulnerable dependency or scanned source for
common weakness classes. All ten runtime requirements in pyproject.toml are
unbounded (>=), the npm trees under viewer/ and website/ carry advisories that
no job surfaces, and scorecard.yml uploads SARIF without doing code scanning of
its own. A vulnerability could therefore ship without anything objecting.

- .github/dependabot.yml: weekly updates for pip, npm (viewer, website) and
  github-actions. Minor and patch are grouped per ecosystem to keep PR volume
  low; majors stay separate so they get a real review.
- .github/workflows/codeql.yml: CodeQL for python and javascript-typescript on
  push, pull_request and weekly, with security-extended queries. Runs on PRs so
  a finding blocks review rather than surfacing after merge.
- .github/workflows/dependency-audit.yml: pip-audit against the resolved
  dependency set, plus npm audit for both projects.

Both workflows declare least-privilege permissions; only the CodeQL job
requests security-events: write, which is required to upload results.

The npm audit job is intentionally continue-on-error for now. Those lockfiles
have existing advisories, so failing hard immediately would block every
unrelated PR; this makes the findings visible without gating. Remove that flag
once the backlog is cleared.

CI configuration only. No application code or behaviour changes.
Every action reference used a floating tag, so a compromised or force-moved tag would run attacker-controlled code with the workflow's token. All 44 references across 10 workflows now pin the commit SHA, following the @sha # version convention already used in scorecard.yml.

Each SHA was resolved from the tag through the GitHub API and then verified to be a real commit in that repository, rather than transcribed. The pins already present in scorecard.yml were left as they were; Dependabot, added earlier in this branch, will keep all of them current.

review-escalation.yml interpolated workflow_dispatch inputs directly into a shell block. A GitHub expression is substituted as text before the shell runs, so a value containing shell metacharacters would have executed as part of the command. Those inputs, and the equivalent expressions in science.yml, now arrive through env and are quoted at the point of use. Building the argument list as an unquoted ARGS string was also replaced with explicit quoted arguments.

All ten workflow files were re-parsed as YAML after the change.

@changliu2 Chang Liu (changliu2) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pinning work checks out: I resolved all 11 distinct SHAs against upstream and each is a real commit that matches the current major-tag head (github/codeql-action@a2983b8 is the exact commit of released tag v3.37.4, one release line behind but legitimate). No pull_request_target is introduced, CodeQL's security-events: write is correctly job-scoped, and the env:-indirection in review-escalation.yml / science.yml is quoted correctly at every use site.

Three things before merge:

  1. .github/workflows/stale.yml was not touched, so actions/stale@v9 is still on a floating tag. That is the one workflow in the repo whose token carries issues: write + pull-requests: write at workflow level, so it is the highest-value pin remaining — and it makes the "all 44 references across 10 workflows" claim in the description inaccurate (9 existing workflows were pinned).
  2. The github-actions block comment in dependabot.yml says workflows "pin actions by mutable tag (Jaimy de Graaf (@v4), julius (@v5))" and that SHA pinning is a future step. The sibling commit in this same PR did exactly that, so the comment ships already contradicting the tree.
  3. Dependabot's pip updater does not maintain uv.lock, and every runtime constraint in pyproject.toml is an unbounded >=. The combination means the pip entry will generate close to zero PRs and the lockfile the repo actually resolves from stays unwatched — the stated "known-vulnerable dependency sits in the tree" gap isn't fully closed. A package-ecosystem: uv entry (or explicit acknowledgement in the comment) would close it.

Non-blocking: no persist-credentials: false on any checkout, most notably dependency-audit.yml, which then runs pip install -e . on fork PR code with the checkout credential still in .git/config; and pip-audit --strict against the whole interpreter (including pip, setuptools, and pip-audit's own tree) with no continue-on-error will red-flag main on any new transitive advisory, while the npm side is deliberately advisory.

Also, the description says "Every change has a regression test" — the diff contains no test files. Worth correcting so the claim doesn't get trusted later.

No conflict with #292 (disjoint files), but see the note below: #292 lands trustabl/trustabl-action@v0 on a floating tag, which violates the convention this PR establishes.

Inline notes

.github/workflows/stale.yml:21 (unmodified) — actions/stale@v9 left on a floating tag in the only workflow with issues: write + pull-requests: write; contradicts the PR's "all 44 references" claim. Fix: pin to the v9 tag SHA with a # v9 comment, same convention as the rest.

.github/dependabot.yml (github-actions block comment, ~line 78) — Comment states workflows pin by mutable tag and SHA pinning is future work; the other commit in this PR already did it. Fix: reword to state actions are SHA-pinned and Dependabot bumps the SHA + trailing version comment.

.github/dependabot.yml (pip block, ~line 13) — pip ecosystem does not update uv.lock; all pyproject.toml runtime pins are unbounded >=, so this entry will rarely fire. Fix: add a package-ecosystem: uv entry for /, or document why lockfile drift is accepted.

.github/workflows/dependency-audit.yml:38 and :63 — Checkout keeps the token in .git/config; the Python job then executes untrusted fork PR code via pip install -e .. Fix: add with: persist-credentials: false to both checkouts (and to codeql.yml:44).

.github/workflows/dependency-audit.yml:52-54pip-audit --strict audits the entire environment (pip/setuptools/pip-audit itself), hard-fails, and runs on schedule → new transitive advisories will red main while npm is advisory. Fix: scope the audit (pip-audit -r/--ignore-vuln for known-noise) or mirror the npm job's continue-on-error during the burn-down.

Overlap with #292: Fully independent — no file overlap and no merge conflict. #292 adds only .github/workflows/trustabl.yml; #285 touches nine existing workflows plus two new files, none of them trustabl.yml. They will merge in either order. There is a policy interaction, not a technical one: #285 establishes the SHA-pin convention repo-wide, and #292 lands trustabl/trustabl-action@v0 — a floating tag on a low-major version from a brand-new third-party publisher — with workflow-level security-events: write + pull-requests: write on a pull_request trigger. If #285 merges first, #292 becomes the sole unpinned action in the tree and should be blocked on that basis alone.

Verdict: Comment-only (approve once the three must-fix items land). The core supply-chain work is correct and independently verified — every SHA is real and tag-consistent, permissions are properly job-scoped, and no fork-secret exposure is introduced — but the pinning sweep misses the repo's most privileged workflow and the Dependabot config ships a comment and a pip entry that don't match reality.

Must fix before merge

  1. Pin actions/stale@v9 in stale.yml (highest-privilege token in the repo; also makes the PR description accurate).
  2. Fix the self-contradicting github-actions comment in dependabot.yml.
  3. Address the uv.lock / unbounded->= gap in the Dependabot pip entry, or state explicitly that it is out of scope.

Nice to have

  • persist-credentials: false on the new checkouts (dependency-audit.yml, codeql.yml).
  • Soften or scope pip-audit --strict so unrelated PRs and scheduled runs don't break on transitive advisories.
  • github/codeql-action is pinned to the v3.37.4 line while upstream has moved to v4; fine to defer, but Dependabot's # v3 comment will only track within v3.
  • Correct the "Every change has a regression test" line in the PR body.

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.

2 participants