fix(transfer): honor bind-mount semantics in filesystem transfers

[ilopezluna]

Supersedes #260.

This PR contains #260's two commits unchanged, preserving their authorship, plus the read-only enforcement and file-import hardening added during follow-up review. It is intended to merge directly into main as the complete mount-aware transfer change.

Problem

The container-FS transferrer anchors imports and exports at the bundle's rootfs. That directory only backs paths not covered by a mount: when the OCI spec declares a bind mount, its source shadows the corresponding rootfs path inside the container.

As a result:

• importing to a bind-mounted path can report success after writing a file the container never sees;
• exporting from that path can archive stale or unrelated rootfs content; and
• resolving directly to a bind source without honoring mount options can bypass the container's read-only view and modify host-backed content exposed as ro.

Changes

Resolve paths against bind mounts

resolveMountRoot reads the bundle spec and maps a container-view path to the directory that backs it. The last matching bind mount in spec order wins, so a later parent mount can hide an earlier child mount. Both transfer directions use the resolved path.

The resolver also:

• interprets relative mount sources against the bundle directory;
• interprets deprecated relative mount destinations against /, as required by OCI on Linux;
• handles single-file bind mounts by anchoring os.Root at the source's parent;
• preserves the container-view name when exporting, even if the source basename differs; and
• updates an existing file destination in place so a single-file bind mount keeps the same inode.

Enforce the container's read-only view

Resolution happens outside the mount namespace where MS_RDONLY is enforced, so the resolver now reports whether the destination is write-protected:

• the matched mount's options use last-option-wins ro/rw and recursive rro/rrw semantics; or
• when no mount covers the path, the OCI spec's root.readonly flag applies.

Copy-to rejects a read-only destination with ErrPermissionDenied before opening the input stream. A writable mount inside a read-only root remains writable, matching Docker semantics. Exports remain allowed.

Validate file-destination imports before mutation

An import over an existing file must contain exactly one regular file. The payload is staged in a temporary sibling and the complete archive is validated before the destination is truncated in place. Empty, truncated, non-regular, or multi-entry archives therefore fail without changing the original file or leaving staging debris.

Bundle-spec behavior

A missing config.json falls back to the rootfs. A config that exists but cannot be read or parsed returns an error: resolving blindly could write to a shadowed path or bypass a read-only mount.

Known limitations

Issue #164 continues to track paths whose subtree crosses into a deeper mount and non-bind mount types, such as tmpfs, whose contents only exist in the container's mount namespace.

Tests

• Mount selection, relative sources and destinations, single-file mounts, and container-view archive names.
• Import and export against bind sources.
• Last-option-wins ro/rw and rro/rrw behavior and the read-only-root fallback.
• End-to-end rejection of imports to read-only mounts and root filesystems before the stream is opened.
• Missing-config fallback and malformed-config rejection.
• Global PAX metadata plus empty, malformed, and multi-entry archives over file destinations, including preservation of the original bytes and inode.

[Copilot]

Pull request overview

This PR hardens the container filesystem transferrer so imports (copy-to) do not bypass the container’s read-only view when paths are backed by bind mounts or a read-only root, by resolving container paths against bundle config.json mounts and refusing writes to destinations marked read-only.

Changes:

• Extend resolveMountRoot to resolve container paths to their backing directory and report whether the container-view path is write-protected (mount ro/rw options + root.readonly fallback).
• Update copy-to to reject imports to read-only destinations with ErrPermissionDenied before consuming the input stream; keep exports allowed.
• Adjust export tar top-level naming to reflect the container-view path (even when mount source basenames differ) and add file-destination import support (extractOverFile) for single-file bind mounts.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
internal/transfer/containerfs.go	Adds mount-aware path resolution (including readonly flag), enforces read-only import refusal, and improves export/import behavior around mounts and file destinations.
internal/transfer/containerfs_test.go	Adds/updates unit and end-to-end tests covering mount resolution, readonly semantics, and copy-to refusal behavior.

Suppressed comments (1)

internal/transfer/containerfs.go:438

• extractOverFile can modify the target file and still return an error when the tar contains multiple entries: the first entry is applied, then the second header triggers "cannot extract multiple entries". This violates the expectation that a rejected archive leaves the destination untouched; consider validating single-entry-ness (e.g., stream to a temp file / buffer, ensure EOF, then truncate+write) before touching the target.

		if header.Typeflag != tar.TypeReg {
			return fmt.Errorf("cannot extract %q over file %s: not a regular file", header.Name, target)
		}
		if written {
			return fmt.Errorf("cannot extract multiple entries over file %s", target)
		}

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

internal/transfer/containerfs.go:369

• In readPath, the parameters and error message still refer to "rootfs", but callers now pass the resolved backing directory (rootfs or a bind-mount source). The inline comment here also says "file in the rootfs", which is no longer always true and can confuse debugging when OpenRoot fails on a mount source path.

		// A destination naming an existing non-directory — a plain
		// file in the rootfs, or the source of a single-file bind
		// mount after resolution — receives the archived file's bytes

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

internal/transfer/containerfs.go:90

• The comment for resolveMountRoot says it returns "the path relative" to the resolved root, but the function currently returns values like "/file" (see strings.TrimPrefix at line 174). This mismatch makes the contract unclear for callers; either normalize rel to be truly relative (no leading "/") or adjust the docstring to reflect that rel may be absolute-like and is intended to be passed through rootRel().

// resolveMountRoot maps a path expressed in the container's view onto the
// directory that backs it, returning that directory and the path relative to
// it.

internal/transfer/containerfs.go:440

• extractOverFile returns the raw error from io.CopyN / Close without context; in common failure cases this can surface as a bare EOF/UnexpectedEOF, which is hard to diagnose from a Transfer error. Wrapping these with target/path context would make failures actionable.

	// Copy exactly the size the header declares; the tar reader
	// bounds the entry anyway, and the explicit limit satisfies
	// gosec's decompression-bomb rule (G110).
	if _, err := io.CopyN(f, tr, header.Size); err != nil {
		f.Close()
		return err
	}
	if err := f.Close(); err != nil {
		return err

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Kern--]

I don't think this statement is correct:

The longest matching bind destination wins, so a nested mount takes precedence over its parent.

Mounts are ordered, so last match should win, not longest.

We've got a bunch of unhandled edge cases that are cropping up because we're trying to figure out the filesystem without actually mounting. This is fine for now, but as part of #164 we should consider the moby approach of doing the mounting and copying from the mount point instead.

AI finding:

[P2] Longest destination does not always represent the visible mount

Location: internal/transfer/containerfs.go:158

Description: Mount resolution ignores OCI mount ordering and always selects the most deeply nested destination.

Behavior:

• Setup: The spec lists a bind mount at /data/inner, followed by another bind mount at /data.
• Actual: A transfer involving /data/inner/file accesses the first mount’s source.
• Expected: It must access the later /data mount’s inner/file, because OCI mounts are applied in array order and the later parent mount hides the earlier child mount.

Contract/invariant: Transfers must reflect the container’s actual mounted filesystem view.

Failure path: resolveMountRoot considers every matching bind mount and replaces the candidate solely when len(dest) >= len(bestDest). Thus, /data/inner remains selected even when a
later /data mount shadows it. The transfer then reads or writes a backing source that is invisible inside the container.

Impact: Copy-to can report success while the container sees no change. Copy-from can expose stale or otherwise hidden content from a mount source. Reverse-ordered nested mounts are valid
OCI input, though less common than parent-before-child ordering.

Confidence: High

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

[Copilot]

🟢 Approval recommended

The mount-aware resolution and read-only enforcement are implemented consistently with the stated OCI ordering semantics and are backed by comprehensive unit tests for the key edge cases and failure modes.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new single-file overwrite path (extractOverFile) appears to enforce “exactly one tar entry” rather than “exactly one regular-file payload,” which can reject otherwise-valid single-file archives that include common tar metadata entries.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/transfer/containerfs.go:424

• extractOverFile enforces “exactly one tar entry” (tr.Next() must hit EOF immediately after the first regular file). Many tar writers may include non-payload metadata entries (e.g., PAX xheaders / GNU longname/link records) while still carrying exactly one regular-file payload; those would be rejected here even though they satisfy the stated “exactly one regular file” contract. If compatibility with such archives matters, consider skipping known metadata entry types and enforcing “exactly one regular file payload entry” instead.

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes security-sensitive filesystem/path resolution and extraction behavior around mounts and read-only enforcement, so a final human review is warranted despite strong test coverage.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

[Kern--]

LGTM.

I'm not sure the development history is that useful, but squashing is definitely not blocking.

[austinvazquez]

Single-file mount path building breaks when mount destination is /.

Suggested change

	rel = strings.TrimPrefix(target, mountDest)
	rel = strings.TrimPrefix(target, mountDest)
	if mountDest == "/" && rel != "" {
	rel = "/" + rel
	}

Failing unit test:

func TestResolveMountRootSingleFileMountAtRoot(t *testing.T) {
  bundle, _, _ := makeRootfs(t)
  source := filepath.Join(bundle, "single-file")
  if err := os.WriteFile(source, []byte("data"), 0644); err != nil {
    t.Fatal(err)
  }
  resolvedSource, err := filepath.EvalSymlinks(source)
  if err != nil {
    t.Fatal(err)
  }
  writeBundleSpec(t, bundle, specMount{Destination: "/", Source: source})

  root, rel, _, err := resolveMountRoot(bundle, "/etc/foo")
  if err != nil {
    t.Fatal(err)
  }

  wantRoot := filepath.Dir(resolvedSource)
  wantRel := filepath.Base(resolvedSource) + "/etc/foo"
  if root != wantRoot || rel != wantRel {
    t.Errorf("resolveMountRoot(%q) = (%q, %q), want (%q, %q)", "/etc/foo", root, rel, wantRoot, wantRel)
  }
}

[austinvazquez]

Opening with truncate can destroy the original contents if the copy fails. Is that acceptable or should we consider truncating after copy succeeds?