Skip to content

ci(deps): repair Dependabot NuGet PRs automatically (#224) - #225

Merged
mforce merged 6 commits into
mainfrom
chore/224-dependabot-lockfix
Oct 26, 2026
Merged

mforce merged 6 commits into
mainfrom
chore/224-dependabot-lockfix

Conversation

@mforce

@mforce mforce commented Oct 26, 2026

Copy link
Copy Markdown
Owner

Closes #224.

Dependabot's NuGet group PRs arrive red. They leave some packages.lock.json files stale, so CI's locked restore fails with NU1004, and they add a PackageReference that Collectify.PostgresMigrations does not need. I fixed #213, #216 and #223 by hand. This PR adds a workflow that makes the same fix automatically after CI, so a NuGet group PR goes green without a hand commit.

Why a workflow and not a project change

I ran the real Dependabot NuGet updater locally on 52cce9e, the commit before #223. Its output matched Dependabot's #223 commit (89a16da) line for line.

  • The updater handles a group one package at a time. When a package is only a transitive dependency of the first project it tries, it pins the package there. Under central package management, pinning means adding <PackageReference Include="..."/> to that project.
  • Giving Collectify.PostgresMigrations a direct reference of its own did not stop the edit. Turning on CentralPackageTransitivePinningEnabled stopped it, but two packages then failed to update.

Neither project-side change I tried fixed both problems. The edits Dependabot makes here only add a PackageReference with no version, because versions live in src/server/Directory.Packages.props. For these version-only updates, reverting the edit loses nothing, and the #223 replay below shows it.

What it does

.github/workflows/dependabot-lockfix.yml is a port of the lockfix workflow that runs in mforce/cluckwork (mforce/cluckwork#203). The main functional addition is the csproj revert. The port is also hardened beyond the original: the copy step checks each destination path, compute reads its scripts and NuGet config from a trusted checkout of main, and the lock allowlist lives in one place.

  • Trigger. It runs on workflow_run after CI. It acts only on a first-attempt run that Dependabot started on a dependabot/nuget/* branch of this repo.
  • compute job. This job has no credential that can push. It checks out the exact SHA CI ran on, reverts every csproj the PR modified to the merge base with main (lockfix-revert.sh), runs dotnet restore src/server/Collectify.slnx --force-evaluate with a trusted nuget.lockfix.config that allows only nuget.org, and uploads the lock files.
  • commit job. This job holds a GitHub App token capped to contents: write, and it runs no project code. Its scripts come from main. It copies the locks onto the PR checkout and runs the same revert. A classifier (lockfix.mjs) then refuses the commit unless every change is an allowlisted lock file, or a csproj whose content equals the merge base. The push is not forced, so it fails if the branch moved.
  • Commit message. The commit names the lock files it regenerated, the csproj edits it reverted, and the merge base.

In release-please.yml, the release-please and groom jobs now read the App secrets from the new app-token environment, which only main can use. Nothing else in that file changes.

Where to start reading

  1. .github/workflows/dependabot-lockfix.yml holds the security model and both jobs.
  2. .github/scripts/lockfix-apply.sh checks paths before copying the locks, then classifies and pushes.
  3. .github/scripts/lockfix.mjs holds the allowlist and the classifier. lockfix-revert.sh is the shared csproj revert.
  4. .github/scripts/lockfix.test.mjs has 18 tests. CI runs them in the client job, next to the vuln-gate tests.
  5. The changes to release-please.yml, ci.yml and CONTRIBUTING.md are small.

Verification

  • Replay of chore(deps): Bump the nuget-minor-patch group with 10 updates #223. I ran the workflow's steps on Dependabot's commit 89a16da, with merge base 52cce9e. The commit they built has the same tree as my hand fix 0782b5e. A second run changed nothing. A foreign file in the tree made the classifier refuse, and the branch tip did not move. A lock file committed as a symlink made it abort without writing through the link.
  • Tests. node --test .github/scripts/lockfix.test.mjs passes 18 of 18; I re-ran it myself. The worker removed each of the two destination checks in turn, and a test failed each time. The first worker reported the same for three classifier checks.
  • Lint. actionlint is clean on the changed lines, and shellcheck is clean on both scripts.
  • Not yet tested. The workflow has not run on GitHub. workflow_run always runs the copy on main, so the first real run will be the next Dependabot NuGet PR after this merges.

Review

Two local review rounds ran before the push.

  • Round 1 found a real hole. The copy step followed a symlink at the destination path. A compromised compute job plus a symlink in the PR tree could overwrite a trusted script while it was running, inside the job that holds the token. The reviewer proved this in a sandbox. The cluckwork original has the same hole. The fix refuses any destination whose real path leaves the PR checkout, or whose git mode is not 100644.
  • Round 1 also fixed a false "no token" comment, removed the duplicated revert logic, and dropped two checks that guarded nothing.
  • Round 2 re-ran the attack in five layouts, and the guard refused all five. It ended with no confirmed finding.

Remaining risk

compute still receives the Actions runtime token, which can write to main's cache scope. The image job restores its layer cache by the image-layers- prefix. To use this, an attacker would need code to run during a restore of a Dependabot-authored tree. NuGet restore does not grant that on its own. The round 1 reviewer traced this path but did not exploit it end to end. I accept this risk for this PR and will track hardening the image cache key as a separate follow-up.

Deferred to follow-ups

  • The CLI guard in lockfix.mjs exits 0 ("commit") if main() is skipped. Its known trigger is fixed and tested; giving "commit" its own exit code would make it fail closed.
  • lockfix-revert.sh could pass --literal-pathspecs. Any extra match is reverted to the base and checked again by the classifier.

After merge

  • Delete the repo-level RELEASE_APP_CLIENT_ID and RELEASE_APP_PRIVATE_KEY at https://github.com/mforce/collectify/settings/secrets/actions. Until then, the environment gate protects nothing. Don't delete them before the merge, because release-please on main reads them until this lands.
  • The first release-please run after the merge is the first real use of the environment secrets.
  • The next Dependabot NuGet group PR should get a commit from the App after its first CI run finishes, and then go green.

…t PRs (#224)

Port mforce/cluckwork's dependabot-lockfix workflow. After CI on a
Dependabot NuGet PR, a credential-free job reverts csproj files Dependabot
modified to the merge base and regenerates every packages.lock.json; a
second job applies them with trusted default-branch scripts, classifies the
diff and pushes a non-forced commit with the App token. CI runs the
classifier's tests.
The PR tree controls where each regenerated lock is copied. A symlink on
that path would let a poisoned artifact overwrite the trusted scripts in
the job that holds the write token. Copy only onto a path with no symlink
that git tracks as a 100644 file.

Move the csproj revert into lockfix-revert.sh, run from a trusted
default-branch checkout in both jobs. Drop the JSON check on the
artifact: a poisoned lock can still be valid JSON. State that compute
still gets the Actions runtime token.
One source with pattern `*` maps every package to the only source, so
the packageSourceMapping block constrained nothing.
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.

Dependabot NuGet group PRs leave stale lock entries and add unneeded PackageReferences

1 participant