ci: cover every workflow with the hardening guards, and fix the gaps they surface - #126
ci: cover every workflow with the hardening guards, and fix the gaps they surface#126dberzan wants to merge 14 commits into
Conversation
publish-docs.yml is the only workflow that puts a secret in a job environment
(FERN_TOKEN), and was the least hardened of the four:
- actions/checkout@v4 was the single unpinned uses: reference out of 36 in
the repository. Pin it to 9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0
(v7.0.0), the SHA already used 14 times here.
- No permissions: block, so the job inherited the repository default
GITHUB_TOKEN scope. Add permissions: contents: read, matching ci.yml,
dco.yml and security.yml.
- No persist-credentials: false, so the token was written to .git/config and
stayed readable by every later step in the job -- including the npm
install and fern generate steps.
- npm install -g fern-api resolved its whole transitive tree at run time.
Pin to 5.109.0.
dco.yml only reads history with git log and git show, so it does not need
persisted credentials either.
Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
test_changed_workflows_pin_every_action_to_a_commit and
test_changed_workflows_do_not_persist_checkout_credentials each iterated the
hardcoded tuple ("ci.yml", "security.yml"), so dco.yml and publish-docs.yml
were never loaded and could drift from the invariant the tests exist to hold
-- which is how the gaps fixed in the previous commit went unnoticed.
Replace the tuple with a glob over .github/workflows/ so a workflow added
later is covered the day it lands, and rename both tests to match what they
now assert. Add a third guard: every workflow declares permissions: and does
not use write-all.
All three guards fail against the workflows as they stood before the previous
commit, and pass after it.
Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
The project logs CI changes in CHANGELOG.md (see the 0.1.0 DCO entry and the 0.2.0 CI routing entries), so record both halves under Unreleased > Security. Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
The previous commit pinned publish-docs.yml to fern-api@5.109.0, which was wrong: fern/fern.config.json already pins the Fern CLI at 5.66.1, and ci.yml's docs lane derives the version from that file to run fern check. A hardcoded pin here would have validated docs with one CLI version and published them with another, and tests/test_ci_workflows.py already asserts the ci.yml side of that contract. Derive the version the same way ci.yml does, with the same format guard, and add a test asserting publish-docs never hardcodes it again. Add a pinned setup-node step so the version-parsing step does not depend on whichever Node the runner happens to ship -- ci.yml's docs lane already does this. Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
Code review found the new guards were step-scoped, and a probe workflow confirmed it: one declaring permissions: contents: read at the top, a job with permissions: write-all, and a second job with an unpinned reusable-workflow uses: at job level passed all thirteen tests. GitHub applies a job-level permissions: block over the workflow-level one, and a job-level uses: has no steps list for _all_uses to walk. Both are now covered. The same probe fails two guards after this commit. Also give the Fern install lookup a next(..., None) default so a restructured workflow reports a missing install step instead of raising StopIteration, and correct the changelog entry, which still claimed a hardcoded fern-api pin that the previous commit replaced. Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
| checkout_steps = [step for step in _all_steps(_load(workflow_name)) if step.get("uses", "").startswith("actions/checkout@")] | ||
| assert checkout_steps | ||
| assert all(step.get("with", {}).get("persist-credentials") == "false" for step in checkout_steps) | ||
| assert re.fullmatch(r"[^@]+@[0-9a-f]{40}", uses), f"{workflow_name}: {uses}" |
There was a problem hiding this comment.
[P2] Allow same-commit local uses references. GitHub local actions and local reusable workflows use the supported ./... syntax without an at-ref and execute from the caller's commit. Because the helper now includes both step- and job-level references, this assertion rejects both forms even though no remote revision can float. Exempt validated local paths before requiring a 40-character SHA for external actions.
| echo "::error file=fern/fern.config.json::Invalid Fern CLI version: $FERN_VERSION" | ||
| exit 1 | ||
| fi | ||
| npm install --global "fern-api@$FERN_VERSION" |
There was a problem hiding this comment.
[P1] Lock the dependency tree before the secret-bearing step. fern-api 5.66.1 still has an unshrinkwrapped optional dependency on @boundaryml/baml with a semver range; BAML accepts @scarf/scarf 1.x, whose currently resolved package runs a postinstall script. This still resolves and executes mutable lifecycle code immediately before the global Fern process runs with FERN_TOKEN. Use a committed lockfile with npm ci and npx --no-install, or omit optional dependencies and lifecycle scripts if docs publishing does not need them.
rng1995
left a comment
There was a problem hiding this comment.
The workflow hardening improves action pinning and token scope, but the Fern installation still executes an unlocked transitive lifecycle script immediately before the secret-bearing publish step, and the new global guard rejects supported local action and reusable-workflow references. Workflow tests passed (13 tests), and both updated action SHAs match their official tags. Requesting changes for the two remaining hardening gaps noted inline.
Maintainer review (rng1995, PR NVIDIA#126, P1) found that pinning fern-api's top-level version does not pin its dependency tree: npm re-resolves every transitive dependency from semver ranges on each install. fern-api@5.66.1 has an optional dependency on @boundaryml/baml@^0.219.0, which depends on @scarf/scarf@^1.3.0, which resolves to 1.4.0 and declares postinstall: node ./report.js. That script still ran in the job immediately before FERN_TOKEN was used. Pass --ignore-scripts on both npm installs (publish-docs.yml and ci.yml's docs-validation lane, which runs the identical command). fern-api declares no scripts of its own and npm links its bin natively, so nothing the CLI needs is skipped -- this alone removes the lifecycle-script execution vector. Also pass --omit=optional, which drops @boundaryml/baml and the eight native binaries it pulls in. This rests on one assumption that cannot be verified in CI: publish-docs.yml only runs on push to main, gated on github.run_number > 1, so it never executes on a pull request. Because BAML is declared optional, fern-api must already tolerate its absence at install time; the residual risk is a lazily-required docs code path. If docs publishing breaks on the merge commit, the rollback is to drop --omit=optional and keep --ignore-scripts, which retains the security property that motivated this change. Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
… refs Two changes to the workflow guards, both from the same review round: Add test_every_workflow_npm_install_ignores_lifecycle_scripts, asserting every npm install line in any workflow's run steps carries --ignore-scripts. It follows the same glob-every-workflow shape as the other guards, so a fifth workflow is covered the day it lands. --omit=optional is deliberately not asserted: it is a judgement about one CLI's optional dependency, not a repository-wide invariant, and asserting it would block a future workflow that legitimately needs optional dependencies. Confirmed non-vacuous by the previous commit: before it, this guard failed against the real, unpatched ci.yml and publish-docs.yml. Relax test_every_workflow_pins_every_action_to_a_commit so a same-repo reference (uses: ./...) no longer needs a commit SHA. GitHub always resolves a local composite action or reusable workflow from the caller's own commit, so nothing about it can float, and the guard previously rejected the only syntax GitHub supports for it (reviewer rng1995, PR NVIDIA#126, P2). A local reference that escapes the repository (./../outside) is still rejected, and docker://image:tag is unaffected -- a mutable container tag is not a local reference and stays rejected. The exemption lives in the guard, not in _all_uses, so a future guard can still assert something about local references specifically. Probed with throwaway workflow files (removed before this commit): ./.github/actions/x failed the guard before this change and passes after; ./../outside and docker://alpine:3.18 failed before and still fail after. Also update the one pre-existing assertion that hardcoded the old install line verbatim. Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
Same convention as the earlier commits in this branch: CI security changes are logged under Unreleased > Security. Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
Required code review (mattpocock-skills:code-review, high effort, range ea9b239..HEAD) found that _is_local_reference only checked for a ./ prefix, so a malformed reference like ./.github/actions/build@main was classified as local and fully exempted from the commit-pin requirement. GitHub has no @ref syntax for a local path, so a string like this is either a typo or a misunderstanding of the syntax, not a reference nothing can float -- it should still be judged by the same pinning rule as everything else. Probed with a throwaway workflow (removed before this commit): the bogus reference passed the guard before this change and fails it after, with the pre-existing SHA-format assertion doing the rejecting since a local prefix no longer short-circuits it. Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
docs/README.md, docs/AGENTS.md, and docs/developer-guide.mdx all told a contributor (or an agent following docs/AGENTS.md) to run npm install -g fern-api with no flags -- worse than the workflows were before this branch's earlier commits, since the docs never even pinned a version. Add --ignore-scripts --omit=optional to all three, matching what the workflows now do, so lifecycle-script execution isn't reduced in CI while staying open on a contributor's own machine. Deliberately not adding a version pin here: fern/fern.config.json is the single source of truth the workflows derive their version from, and hardcoding a version into three docs files would reintroduce the exact drift risk an earlier commit on this branch removed. Also correct the CHANGELOG entry from two commits ago, which described the local-reference exemption's rejection list without the @ref case this branch's most recent commit closed. Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
test_every_workflow_declares_least_privilege_permissions only rejects
the write-all shorthand and a missing permissions: block; an explicit
{contents: write, packages: write, id-token: write} block passes
cleanly despite being just as broad. The name and docstring promised
more than the assertion delivers.
Rename to test_every_workflow_declares_explicit_permissions and add a
docstring line stating the scope directly, so a reader doesn't infer a
least-privilege guarantee that isn't there. Not strengthening the
assertion: a release workflow legitimately needs a granular write, and
this guard isn't meant to block that.
Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
The previous commit pinned fern-api's own version and added --ignore-scripts
--omit=optional. Testing that fix against npm rather than assuming it, two
things turned out to be wrong:
- --omit=optional is silently ignored for a --global install. Installing
fern-api@5.66.1 globally with and without the flag produces byte-identical
trees on npm 10.9.9 and 11.12.1; @scarf/scarf and its postinstall land
either way. The flag only takes effect for a local install.
- Pinning the top-level version leaves every transitive edge floating.
fern-api ^0.219.0 -> @boundaryml/baml -> @scarf/scarf ^1.3.0 is re-resolved
from semver on every run, so a compromised republish still reaches the job.
Commit fern/package.json and fern/package-lock.json, and install with
`npm ci --prefix fern --ignore-scripts --omit=optional`. npm ci reproduces the
lockfile exactly -- every version and integrity hash -- so nothing resolved at
run time reaches the step holding FERN_TOKEN. --omit=optional is honoured here
because this is a local install, and it leaves fern-api alone in the tree.
Both workflows now invoke ./fern/node_modules/.bin/fern rather than a binary
on PATH, so a PATH entry cannot substitute the CLI.
Verified end to end: the exact command sequence installs one package and
`fern check` reports 0 errors against this repository.
Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
…nest
Three guards, each proven against a probe:
- no workflow may resolve a Node dependency tree from the registry; only
`npm ci` against the committed lockfile installs into a job.
- every npm command carries --ignore-scripts. Widened from `npm install` to
cover `npm ci`, and anchored to the start of a line so a comment mentioning
npm is no longer matched as a command.
- fern/package.json, fern/package-lock.json and fern/fern.config.json name
the same CLI version. A lockfile adds a second home for that version, and
this stops the two drifting -- docs validated by one CLI and published by
another is the failure this PR already fixed once.
Reverting ci.yml to `npm install --global` fails the first two; changing
fern.config.json's version alone fails the third.
Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
…changelog docs/README.md, docs/AGENTS.md and docs/developer-guide.mdx told contributors to run `npm install -g ... fern-api` -- unpinned, and with the --omit=optional flag that a global install ignores. They now use the same lockfile install CI runs. The changelog entry claiming --omit=optional "drops the tree's only optional dependency" was wrong for the same reason and is rewritten to describe what was measured rather than what the flag name suggests. Signed-off-by: berzan93-prog <224507836+berzan93-prog@users.noreply.github.com>
|
Thanks — both were right, and testing the P1 fix instead of assuming it turned up a second problem in my own patch. I've gone with your first option, the lockfile. P1 — lifecycle scriptsConfirmed the chain against the registry before changing anything: My earlier commit said pinning the version stopped the tree floating. That was wrong — npm re-resolves every transitive edge from semver on each run. The first fix was also wrong, in a way worth flagging: Now: That closes both halves of what you wrote. It no longer resolves: every package in the lock carries an exact version and an integrity hash, so a compromised republish can't substitute. It no longer executes: Verified end to end rather than reasoned about: the exact command sequence installs one package, and Two notes on the shape, both reversible if you'd rather:
P2 — local referencesAgreed.
Also in here
Say the word on any of these and I'll drop them. Verification
|
Summary
Closes #125.
tests/test_ci_workflows.pyalready encodes two CI hardening invariants — every action pinned to a commit SHA, and no checkout persisting credentials — but both iterated the hardcoded tuple("ci.yml", "security.yml").dco.ymlandpublish-docs.ymlwere never loaded, and both had drifted.permissions:declaredpersist-credentials: falseci.ymlsecurity.ymldco.ymlpublish-docs.ymlThe workflows
publish-docs.ymlis the only workflow that puts a secret in a job environment (FERN_TOKEN), and was the least hardened of the four.actions/checkout@v4was the single unpinneduses:reference out of 36 in the repository. It now uses9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0, the SHA already used 14 times here.permissions:block, so the job inherited the repository defaultGITHUB_TOKENscope. It now declarescontents: read, matching the other three workflows.persist-credentials: false, so the token was written to.git/configand stayed readable by every later step in the job — includingnpm installandfern generate.npm install -g fern-apiresolved its whole transitive tree at run time. It now installs the versionfern/fern.config.jsonpins, derived exactly asci.yml's docs lane already does, with the same format guard and a pinnedsetup-nodeso the lookup does not depend on the runner's ambient Node. Docs are now validated and published by the same CLI version — a hardcoded pin here would have let those two drift, which is the mistake my first attempt made.dco.ymlonly reads history withgit logandgit show, so it getspersist-credentials: falsetoo.The guards
Both existing guards now glob
.github/workflows/rather than naming files, so a workflow added later is covered the day it lands — that is the part that stops this recurring. Both are renamed fromtest_changed_workflows_*totest_every_workflow_*, since the old names would now be wrong. Three guards are added or widened:permissions:, and nopermissions:value anywhere iswrite-all— including job-level blocks, which override the workflow-level one;uses:, i.e. reusable workflows, which have nostepslist for the step-level walk to reach;publish-docs.ymlnever hardcodes the Fern CLI version again.Granular write scopes (
contents: writeand friends) are deliberately still allowed — a release workflow will legitimately need one. The invariant is that the scope is declared and never blanket.The guards are not vacuous
Against the workflows as they stood before the first commit, three fail:
And against a probe workflow that declares
permissions: contents: readat the top, escalates one job topermissions: write-all, and references a reusable workflow at@main, two fail. Before the last commit that same probe passed all thirteen — which is how those two holes were found.One thing to decide knowingly
publish-docs.ymlis the only workflow that never runs on a pull request — it ispushtomainonly, gated ongithub.run_number > 1. So thepersist-credentials: falsechange on the publish path cannot be exercised before merge.fern generate --docsauthenticates to Fern withFERN_TOKENrather than over git, so this should be inert, but you know Fern's internals better than I do. If you would rather land the rest first and that line separately, say so and I will split it.Verification
make lint—All checks passed!(alsoruff check .as CI runs it)make test—5460 passed, 18 skippedmake build— built the sdist and wheelAlso ran the two CI-only gates:
scripts/check_oss_boundary.py(passed) andscripts/ci/check_public_benchmarks.py --require-files tests/golden(passed, 2 files). Python 3.13.12, macOS 15 (arm64),uv sync --extra all --extra dev.Release Impact
CHANGELOG.md