fix(build): re-include local bin overrides in the generated .dockerignore - #374
fix(build): re-include local bin overrides in the generated .dockerignore#374initializ-mk wants to merge 1 commit into
Conversation
…nore The generated .dockerignore unconditionally excludes .local-bins/, but a build with local bin overrides (--local-bin or bin_overrides.*.local) emits `COPY .local-bins/<name> /usr/local/bin/<name>` lines that read straight from the build context — docker then fails with: failed to calculate checksum of ref …: "/.local-bins/forge": not found Append a !.local-bins/<name> negation per override after the blanket exclusion (dockerignore last-match-wins), keeping unrelated stash content excluded. Extracts the config+CLI override union into localBins(), shared with copyLocalBins.
initializ-mk
left a comment
There was a problem hiding this comment.
Review: re-include local bin overrides in .dockerignore
The fix is correct, well-scoped, and well-tested — it unblocks a real CI failure. One defense-in-depth finding on unvalidated bin-override names (pre-existing, but this PR extends it to a new sink). Not blocking; can be a fast follow-up. CI fully green.
The fix is right ✅
- dockerignore semantics:
!.local-bins/<name>negations appended after the blanket.local-bins/exclusion — last-match-wins re-enters each override while unrelated stash content stays excluded. - Deterministic: names sorted → reproducible
.dockerignore(cache-stable). localBins()extraction shared betweencopyLocalBinsand the dockerignore generation prevents drift — the right refactor, since a mismatch would reintroduce this exact bug.- No default-path change (
len(bins) > 0gate), pinned byTestDockerignore_NoLocalBins_KeepsPlainExclusion; the re-inclusion test checks config + CLI overrides AND the ordering-after-exclusion invariant. Good coverage.
Finding — Low/Medium (defense-in-depth): bin-override names are unvalidated
name reaches three sinks unsanitized, and nothing validates its charset (parseLocalBins checks the path, not the name; ValidateForgeConfig has no bin_overrides check):
- fs write —
dst := filepath.Join(localBinsDir, name)incopyLocalBins; a../name traverses outside.local-bins/(pre-existing). - Dockerfile —
COPY .local-bins/<name> /usr/local/bin/<name>(pre-existing). .dockerignore— the new!.local-bins/<name>line (see inline) — a newline injects arbitrary dockerignore patterns; a glob re-includes the whole stash.
Threat model: for local forge build on your own agent this is self-inflicted → a non-issue. But Forge's CI/BYO story includes a platform building untrusted BYO forge.yaml, where a crafted bin_overrides.<name> becomes an injection vector across all three sinks. Impact is bounded (traversal → build host's output area; dockerignore re-includes limited to context contents), but a real hygiene gap once configs aren't self-authored.
Recommendation: validate the name against a strict binary-name pattern (e.g. ^[a-zA-Z0-9_.-]{1,64}$) in both parseLocalBins (CLI) and ValidateForgeConfig (bin_overrides) — closes all three sinks at the source. Since the more dangerous sinks (fs traversal, COPY) already exist on main, fair to land this dockerignore fix now and validate names in a fast follow-up.
The dockerignore fix is sound and merge-ready on its own.
| } | ||
| sort.Strings(names) | ||
| for _, name := range names { | ||
| dockerignoreContent += fmt.Sprintf("!.local-bins/%s\n", name) |
There was a problem hiding this comment.
Defense-in-depth (Low/Medium): name is written here unsanitized. It's fully user-controlled (bin_overrides.<name> in forge.yaml, or --local-bin name=…) and nothing validates its charset — parseLocalBins checks only the path, and ValidateForgeConfig has no bin_overrides check. A newline in a name injects arbitrary .dockerignore lines here (e.g. !../secrets or !. re-including unintended build-context paths); a glob like * re-includes the whole .local-bins/ stash you just excluded.
This PR adds .dockerignore as a third sink alongside the pre-existing ones — copyLocalBins's filepath.Join(localBinsDir, name) (a ../ name traverses outside .local-bins/) and the Dockerfile COPY .local-bins/<name> /usr/local/bin/<name>. For local self-authored builds this is self-inflicted, but for a platform building untrusted BYO forge.yaml it's a real injection vector.
Closing it at the source — validate the name against ^[a-zA-Z0-9_.-]{1,64}$ in parseLocalBins and ValidateForgeConfig — fixes all three sinks at once. Pre-existing on main, so it needn't block this fix; worth a fast follow-up (a bin_overrides name is a binary basename, so the strict pattern costs nothing).
Problem
The generated
.dockerignoreunconditionally excludes.local-bins/, but a build with local bin overrides (--local-bin name=/pathorbin_overrides.<name>.local) generates a Dockerfile that COPYs each override straight from the build context:docker build then fails:
Hit in a real CI pipeline (Buildkite, demo-weather agent) using
--local-bin forge=…— the documented path for baking a vnext-prerelease forge runtime into the image, since the snapshot binary's self-reported version is not a real release tag the Dockerfile could pin.Fix
Append a
!.local-bins/<name>negation per override after the blanket.local-bins/exclusion — dockerignore is last-match-wins, so each override re-enters the context while any unrelated stash content stays excluded. The config+CLI override union is extracted intolocalBins(), shared withcopyLocalBinsso the two can't drift.No behavior change for builds without local bin overrides (pinned by a new test).
Testing
TestDockerignore_ReincludesLocalBinOverrides— negations present for both config (bin_overrides) and CLI (--local-bin) overrides, ordered after the exclusionTestDockerignore_NoLocalBins_KeepsPlainExclusion— no negations in the default pathforge-clisuite passes;golangci-lintclean; gofmt clean