Assert tokenFile/httpsTokenFile are paths in mkDevVm (fleet-diff review finding) #66

New issue

Closed

opened 2026-09-03 16:25:31 +01:00 by vnprc-agent · 0 comments

vnprc-agent commented

2026-09-03 16:25:31 +01:00

mkDevVm in flake.nix accepts tokenFile / httpsTokenFile as either a Nix path or a string, and passes whichever through to agenix. That type looseness is a real security gap.

The failure

As a path, the derivation closure contains only that one file (/nix/store/…-agent-pr-token.age / -forge-token.age).
As a string, Nix strings carry the source path of the repo that produced the value, so the closure retains the entire secrets repo — every machine's ciphertext — even though only one file is actually read.

If the secrets repo ever changes forgeTokenFile from a path value to a string, every Forge-enabled dev VM silently regresses to carrying the full secrets repo, and no check catches it:

The consumed-file carve-out sweeps home.file only.
The sibling Forgejo matcher deliberately accepts both the in-root and carved spellings, so it can't tell the forms apart.
Every affected machine's derivation changes, so a fleet diff sees the churn and reads as expected.

This is the "tools/source reference" site noted in the allod/tools#136 review thread too: interpolating "${allod-tools}/…" and "${gitPolicySource}/…" moves every dev machine's derivation on every source commit, and a string leak is strictly worse because it carries secrets.

The fix (already the house style)

Assert both effective builder values are paths (or null) at the mkDevVm boundary, overridable seams included:

tokenFile == null || builtins.isPath tokenFile
httpsTokenFile == null || builtins.isPath httpsTokenFile

This is exactly the contract modules/pi-provider-lifecycle.nix already enforces for Pi tokens. It complements — but is distinct from — allod/archetypes#21, which carves out the consumed file at the source; this adds an assertion at the boundary where overrides arrive, so a string can never regress through either token argument.

`mkDevVm` in `flake.nix` accepts `tokenFile` / `httpsTokenFile` as either a Nix *path* or a *string*, and passes whichever through to agenix. That type looseness is a real security gap. ## The failure - As a **path**, the derivation closure contains only that one file (`/nix/store/…-agent-pr-token.age` / `-forge-token.age`). - As a **string**, Nix strings carry the *source path* of the repo that produced the value, so the closure retains the **entire `secrets` repo** — every machine's ciphertext — even though only one file is actually read. If the secrets repo ever changes `forgeTokenFile` from a path value to a string, **every Forge-enabled dev VM silently regresses to carrying the full secrets repo**, and no check catches it: - The consumed-file carve-out sweeps `home.file` only. - The sibling Forgejo matcher deliberately accepts both the in-root and carved spellings, so it can't tell the forms apart. - Every affected machine's derivation changes, so a fleet diff sees the churn and reads as expected. This is the "tools/source reference" site noted in the allod/tools#136 review thread too: interpolating `"${allod-tools}/…"` and `"${gitPolicySource}/…"` moves every dev machine's derivation on every source commit, and a string leak is strictly worse because it carries secrets. ## The fix (already the house style) Assert both effective builder values are paths (or null) at the `mkDevVm` boundary, overridable seams included: ```nix tokenFile == null || builtins.isPath tokenFile httpsTokenFile == null || builtins.isPath httpsTokenFile ``` This is exactly the contract `modules/pi-provider-lifecycle.nix` already enforces for Pi tokens. It complements — but is distinct from — `allod/archetypes#21`, which carves out the *consumed file* at the source; this adds an assertion at the *boundary where overrides arrive*, so a string can never regress through either token argument.

vnprc-agent referenced this issue from allod/tools

2026-09-03 16:25:56 +01:00

Add fleet-diff: check which machines a change rebuilds against a declared expectation #136