Skip to content

security(deps): block Dependabot from editing the hand-maintained render lockfile (A5) - #293

Merged
Gavin-Borges merged 3 commits into
mainfrom
chore/dependabot-render-lockfile-guard
Aug 26, 2026
Merged

security(deps): block Dependabot from editing the hand-maintained render lockfile (A5)#293
Gavin-Borges merged 3 commits into
mainfrom
chore/dependabot-render-lockfile-guard

Conversation

@Gavin-Borges

Copy link
Copy Markdown
Owner

Closes A5. Adds a path-scoped guard blocking Dependabot from editing environments/requirements-ci-render.txt.

Why the existing protections cannot cover this file

environments/requirements-ci-render.txt is tier 4 in CONTRIBUTING.md's dependency-tier table: hand-maintained via pip download + pip hash, with no .in source.

That means tools/check_lockfile_freshness.py cannot see it at all - it walks LOCKFILE_PAIRS, and this file is deliberately not a member. So the repo's main lockfile gate is blind to it by construction, not by oversight.

The only other lever is .github/dependabot.yml's ignore: list, and that list is keyed by package name. A name-keyed guard is inherently reactive: it protects a transitive dependency only after someone has been bitten by Dependabot reaching it and has named it.

The 2026-08-24 cycle is the proof. Three names - fastjsonschema, nbformat, pygments - reached this file and rewrote its platform-specific wheel hashes, despite two direct entries (ipykernel, nbclient) and one earlier transitive entry (platformdirs) already sitting on that list. A name-keyed guard cannot protect a target reached transitively by a name it has not seen yet, and adding those three only defends against exactly those three next time.

The fix

A new step in dependency-lockfile-check.yml blocks the actor from touching the path, independent of which package moved: if the PR author is dependabot[bot] and the diff touches environments/requirements-ci-render.txt, the check fails with a message pointing at the correct remedy (add the newly-reached name to the ignore list, then update the file by hand per its own header recipe, in a separate PR).

Two implementation details, both chosen against existing precedent in this repo rather than invented:

  • github.event.pull_request.user.login, not github.actor - matching the reliability reasoning pr-review-check.yml already documents for its own TRUSTED_BOTS check.
  • fetch-depth: 0 added to the checkout, and the diff scoped as git diff --name-only "origin/$BASE_REF...HEAD" - mirroring pii_scan.yml's established, working diff-scoping rather than a new pattern.

Validation

No CI run can exercise this until Dependabot next opens a PR, so it was validated locally instead:

  • yaml.safe_load confirms the file parses and that the folded multi-line if: resolves to the expected single-line expression.
  • The grep -qx match logic was self-tested against both a touching and a non-touching file list (match, then correct exit 1 on no-match).
  • Existing gates re-run: ruff clean, doc line-citation and commit-reference gates pass, git diff --check clean, zero banned typographic characters, full fast test suite green via the pre-push gate.

Scope note: this guard is deliberately narrow - one actor, one path. It does not attempt to police tier-4 edits by humans, who are the intended maintainers of that file.

…der lockfile (A5)

environments/requirements-ci-render.txt is tier 4 (CONTRIBUTING.md's
dependency-tier table): hand-maintained via pip download + pip hash, no .in
source. tools/check_lockfile_freshness.py cannot see it - it only walks
LOCKFILE_PAIRS, which this file is deliberately not part of.

.github/dependabot.yml's ignore: list is the only lever Dependabot itself
offers, and it is keyed by package name, so it can only ever be reactive: it
protects a transitive dependency once someone has already been bitten by
Dependabot reaching it and named it. Three names (fastjsonschema, nbformat,
pygments) reached this file in a single 2026-08-24 cycle despite two direct
entries (ipykernel, nbclient) and one earlier transitive one (platformdirs)
already on that list. A name-keyed guard cannot protect a target reached
transitively by a name it has not seen yet.

Adds a step to dependency-lockfile-check.yml that blocks the actor from
touching this path at all, independent of which package moved: if the PR
author is dependabot[bot] and the diff touches
environments/requirements-ci-render.txt, the check fails with a message
pointing at the correct fix. Uses pull_request.user.login rather than
github.actor, matching the reliability reasoning pr-review-check.yml already
documents for its own TRUSTED_BOTS check. fetch-depth: 0 added to the
checkout step so the diff against origin/$BASE_REF resolves; the pattern
(git diff "origin/$BASE_REF...HEAD") mirrors pii_scan.yml's own
established, working diff-scoping.

Validated: python -c 'yaml.safe_load(...)' confirms the file parses and the
new if: condition folds to the expected single-line expression; the grep
match/no-match logic was self-tested against both a touching and a
non-touching file list.

Signed-off-by: Gavin Borges <gavinmborges1104@gmail.com>
@Gavin-Borges

Copy link
Copy Markdown
Owner Author

Verification note before merge: this guard does not currently block a merge

The guard itself is correct and I could not fault its logic. But I checked whether it can actually stop the thing it targets, and it cannot yet. Three facts, each read from the live API rather than from a workflow file:

1. Its check is advisory, not required. Ruleset 16846770 ("Protect Main Branch", active) requires exactly five contexts:

test (3.13)
Require human review
check_dco
Cited commits resolve
Cited lines still hold their content

Check lockfile freshness and hash pinning - the job this new step lives in - is not among them.

2. The human-review gate deliberately auto-passes for the exact actor this targets. .github/workflows/pr-review-check.yml carries TRUSTED_BOTS = ['dependabot[bot]', 'renovate[bot]'] and returns early with "Bypassing human-review requirement; CI suite enforces correctness."

That comment's stated justification is that Dependabot PRs "are gated by 15+ CI checks" - which is precisely the assumption this PR exists because it does not hold for this file.

3. Auto-merge and admin bypass are both live. allow_auto_merge=true; the ruleset carries a bypass actor RepositoryRole 5 at mode=always.

Net effect

On a Dependabot PR that edits environments/requirements-ci-render.txt, this step turns one advisory check red and the merge button stays green. The guard is accurate documentation of a rule that nothing enforces.

The fix is one action, and it is yours

Add Check lockfile freshness and hash pinning to the ruleset's required_status_checks. That closes A5 properly and, as a side effect, makes the pre-existing freshness/hash-pinning checks blocking too - which is SEC-1 in miniature.

I deliberately did not implement a second guard in the required test (3.13) job instead. A pytest test cannot see the PR diff without importing CI context into the test suite, so it would guard the guard rather than the file - a second check that also does not block, which reads as coverage it does not provide.

Recommendation: merge this PR (the step is correct and costs nothing), then make that check required in the same sitting. Until the second half happens, A5 should be considered open, not closed.

@Gavin-Borges
Gavin-Borges merged commit eeaf791 into main Aug 26, 2026
21 checks passed
@Gavin-Borges
Gavin-Borges deleted the chore/dependabot-render-lockfile-guard branch August 26, 2026 05:41
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.

1 participant