cibuild: build with --no-cache like script/docker (closes #89)

A bare `docker build .` served the Dockerfile's check steps from the
layer cache on an unchanged tree and exited 0 without running them.
script/cibuild now computes the version on its own line and runs the
same build command as script/docker and the shared policy, --no-cache
included, so every CI build runs the checks. README.md describes
script/cibuild accordingly.

Model: opus-5-5

Dockerfile

@@ -11,10 +11,6 @@ COPY . .
 # Touch .pb.go so make does not try to regenerate via protoc (file is committed)
 RUN touch mfer/mf.pb.go
 
-# Changing value from script/cibuild; forces the check steps below to
-# re-run instead of being served from a stale layer cache.
-ARG CHECK_EPOCH
-
 # Go half of fmt-check only: this image has no node, so no prettier. The
 # markdown half runs in the mdfmt stage below.
 RUN make fmt-check-go
@@ -31,10 +27,6 @@ RUN yarn install --frozen-lockfile
 
 COPY . .
 
-# Changing value from script/cibuild; forces the check step below to
-# re-run instead of being served from a stale layer cache.
-ARG CHECK_EPOCH
-
 # No make in this image; call the script entrypoint directly.
 RUN script/prettier --check
 
@@ -55,10 +47,6 @@ COPY . .
 # Touch .pb.go so make does not try to regenerate via protoc (file is committed)
 RUN touch mfer/mf.pb.go
 
-# Changing value from script/cibuild; forces the check steps below to
-# re-run instead of being served from a stale layer cache.
-ARG CHECK_EPOCH
-
 RUN make test
 
 # A build context sent as a tar archive, as upaas sends it, keeps its files'

README.md

@@ -23,8 +23,9 @@ javascript library is planned.
 
 # Build Status
 
-CI runs via `script/cibuild` (`docker build .`), which executes `make check`
-(formatting, linting, tests). The `main` branch must always be green.
+CI runs `script/cibuild`, which builds the Docker image with `--no-cache`, so
+the formatting, lint and test steps in the `Dockerfile` run on every build. The
+`main` branch must always be green.
 
 # Entrypoints
 
@@ -56,8 +57,8 @@ provide:
   Docker lint stage, whose image has no node
 - `script/check` — run `script/test`, `script/lint`, and `script/fmt-check`
 - `script/docker` — build the Docker image tagged with the project name
-- `script/cibuild` — CI entrypoint: `docker build .` (the Dockerfile runs the
-  checks)
+- `script/cibuild` — CI entrypoint: builds the image with the same command as
+  `script/docker`, uncached, so the checks in the Dockerfile run every time
 - `script/precommit` — pre-commit checks: `go mod tidy` verification, then
   `script/check`
 - `script/install-precommit` — install the git pre-commit hook that runs

TODO.md

@@ -24,8 +24,13 @@ only thing left of the `chore/align-repo-policies` branch is the list below.
 
 # Completed Steps
 
-- 2026-09-21: made `script/cibuild` re-run the checks on an unchanged tree via a
-  changing `CHECK_EPOCH` build arg in each Dockerfile check stage (#89)
+- 2026-10-03: `script/cibuild` builds the image with the same command as
+  `script/docker`, `--no-cache` included, so the checks in the Dockerfile run on
+  every build, also on an unchanged tree (#89)
+- 2026-10-03: `fetch` refuses any manifest path that runs through a symlink
+  already in the destination directory, checked before each of its writes
+  (directories, temp file, rename), so such a symlink cannot send a write
+  outside it (#86)
 - 2026-10-02: a plain `docker build .` of a clone now stamps the tag or short
   commit into `mfer version` instead of nothing: `.dockerignore` sends `.git`
   (not `.git/config`), and the build stage takes the `VERSION` build argument,

internal/cli/fetch.go

@@ -53,6 +53,9 @@ var (
 	// errPathTraversal indicates a manifest path escaping the target
 	// directory.
 	errPathTraversal = errors.New("path traversal not allowed")
+	// errSymlinkInPath indicates a manifest path running through a
+	// symlink that already exists in the target directory.
+	errSymlinkInPath = errors.New("symlink in path not allowed")
 	// errSizeMismatch indicates a downloaded file with an unexpected
 	// size.
 	errSizeMismatch = errors.New("size mismatch")
@@ -274,6 +277,35 @@ func sanitizePath(p string) (string, error) {
 	return cleaned, nil
 }
 
+// checkNoSymlinks returns an error if any part of the relative path p
+// already exists as a symlink. sanitizePath checks p only as text, so
+// without this a symlink inside the target directory could send a write
+// to p outside of it. Parts that do not exist yet are fine: fetch creates
+// them as plain directories and files. Call it immediately before each
+// write: a symlink created after it returns is not caught.
+func checkNoSymlinks(p string) error {
+	current := ""
+
+	for _, part := range strings.Split(p, string(filepath.Separator)) {
+		current = filepath.Join(current, part)
+
+		info, err := os.Lstat(current)
+		if errors.Is(err, os.ErrNotExist) {
+			return nil
+		}
+
+		if err != nil {
+			return fmt.Errorf("failed to check %s for a symlink: %w", current, err)
+		}
+
+		if info.Mode()&os.ModeSymlink != 0 {
+			return fmt.Errorf("%w: %s", errSymlinkInPath, current)
+		}
+	}
+
+	return nil
+}
+
 // resolveManifestURL takes a URL and returns the manifest URL.
 // If the URL already ends with .mf, it's returned as-is.
 // Otherwise, index.mf is appended.
@@ -419,7 +451,12 @@ func downloadFile(
 	// Create parent directories if needed
 	dir := filepath.Dir(localPath)
 	if dir != "" && dir != "." {
-		err := os.MkdirAll(dir, dirPerms)
+		err = checkNoSymlinks(dir)
+		if err != nil {
+			return err
+		}
+
+		err = os.MkdirAll(dir, dirPerms)
 		if err != nil {
 			return fmt.Errorf("failed to create directory %s: %w", dir, err)
 		}
@@ -447,14 +484,16 @@ func downloadFile(
 		totalBytes = expectedSize
 	}
 
+	err = checkNoSymlinks(tmpPath)
+	if err != nil {
+		return err
+	}
+
 	// Create temp file.
 	//
-	// G304: tmpPath is derived from localPath, which sanitizePath above
-	// constrains lexically to a relative path that does not escape the
-	// destination directory. That is a purely lexical guarantee: it does
-	// not resolve symlinks, so a pre-existing symlink inside the
-	// destination tree can still redirect this write outside of it
-	// (tracked in issue #86).
+	// G304: tmpPath is a relative path that sanitizePath keeps inside the
+	// target directory as text, and checkNoSymlinks just found no symlink
+	// in it.
 	out, err := os.Create(tmpPath) //nolint:gosec // G304: see comment above
 	if err != nil {
 		return fmt.Errorf("failed to create temp file: %w", err)
@@ -519,6 +558,11 @@ func finishDownload(
 		return err
 	}
 
+	err = checkNoSymlinks(localPath)
+	if err != nil {
+		return err
+	}
+
 	// Rename temp file to final path
 	err = os.Rename(tmpPath, localPath)
 	if err != nil {

internal/cli/fetch_test.go

@@ -440,3 +440,52 @@ func TestFetchProgress(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, content, downloaded)
 }
+
+// TestFetchRefusesSymlinks runs fetch into a destination directory that
+// holds a symlink pointing outside it, in each of the three places fetch
+// writes: a parent directory, the temp file, and the file itself, which
+// the temp file is renamed onto; and once as a directory inside a plain
+// directory. The fetch must fail and nothing outside may change.
+//
+//nolint:paralleltest // changes the process-global working directory
+func TestFetchRefusesSymlinks(t *testing.T) {
+	tests := []struct {
+		name   string
+		entry  string // the manifest's only file
+		link   string // symlink placed in the destination directory
+		target string // what link points to, relative to the outside directory
+	}{
+		{"parent directory", "sub/deeper/file.txt", "sub", "."},
+		{"directory inside a plain directory", "docs/data/passwd", "docs/data", "."},
+		{"temp file", testFileTxt, ".file.txt.tmp", "new.txt"},
+		{"file", testFileTxt, testFileTxt, "new.txt"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			content := []byte("fetched")
+			sourceFs := afero.NewMemMapFs()
+			require.NoError(t, sourceFs.MkdirAll(filepath.Dir("/"+tt.entry), 0o755))
+			require.NoError(t, afero.WriteFile(sourceFs, "/"+tt.entry, content, 0o644))
+
+			server := httptest.NewServer(fetchTestHandler(
+				scanToManifest(t, sourceFs), map[string][]byte{tt.entry: content}))
+			defer server.Close()
+
+			outside := t.TempDir()
+
+			chdirTemp(t)
+			require.NoError(t, os.MkdirAll(filepath.Dir(tt.link), 0o750))
+			require.NoError(t, os.Symlink(filepath.Join(outside, tt.target), tt.link))
+
+			opts := testOpts([]string{testApp, "fetch", "-q", server.URL}, afero.NewOsFs())
+			assert.Equal(t, 1, runCLI(opts))
+			assert.Contains(t, testStderr(t, opts), "failed to download "+tt.entry+
+				": symlink in path not allowed: "+tt.link)
+
+			written, err := os.ReadDir(outside)
+			require.NoError(t, err)
+			assert.Empty(t, written, "fetch wrote outside the destination")
+		})
+	}
+}

script/cibuild

@@ -1,19 +1,24 @@
 #!/bin/sh
-# script/cibuild: run the CI build. The Dockerfile runs make fmt-check-go,
-# make lint, the prettier check, and make test as build steps. A bare
-# `docker build .` would serve those steps from Docker's layer cache on an
-# unchanged tree, exiting 0 without running them. CHECK_EPOCH is a build
-# arg placed above the check steps in every check stage; passing a fresh
-# value each run busts the cache for those steps (and nothing above them),
-# so a green build means the checks actually ran. Dependency layers above
-# the arg stay cached. The Gitea workflow runs this on push.
+# script/cibuild: run the CI build; the Gitea workflow runs this on push.
+# It builds the image with the same command as script/docker. --no-cache
+# because the checks the final stage depends on are RUN steps, and a
+# cached one is a check that did not run.
 set -eu
 
-ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
+ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)"
 
 main() {
     cd "$ROOT"
-    docker build --build-arg CHECK_EPOCH="$(date +%s)" .
+    # Own line: a failing command substitution inside an argument does
+    # not trip `set -e`, so the inline form degrades silently to an
+    # empty constant. The VERSION build argument takes precedence over
+    # the version a build stage derives from the .git in the context.
+    version="$(git describe --tags --always --dirty 2>/dev/null || true)"
+    [ -n "$version" ] || version="unknown"
+    docker build --no-cache \
+        --build-arg VERSION="$version" \
+        -t "$("$SCRIPT_DIR/projectname")" .
 }
 
 main "$@"