Skip to content

Fix literal path handling in volume copies - #161

Merged
den-sq merged 1 commit into
mainfrom
codex/154-literal-volume-paths
Oct 19, 2026
Merged

den-sq merged 1 commit into
mainfrom
codex/154-literal-volume-paths

Conversation

@den-sq

@den-sq den-sq commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

The volume server now preserves paths containing apostrophes, quotes, dollar signs, backticks, and repeated spaces in both copy directions. Docker receives an argument array, and the container runs a fixed script with quoted positional parameters. A filename such as Bob's stack.tif is treated as a filename throughout the copy.

Fixes #154. This is a standalone PR against main, independent of the status-presentation stacks. Existing callers and JSON request schemas are unchanged.

Shared-basis audit

  • Copy construction: volume-copy.mjs now owns both directions through its shared Docker-argument builder. The private ingress builder in volume-server-script.mjs and both interpolated shell-command strings are removed, including whitespace normalization that altered valid names. Host-native dirname/basename parsing and POSIX container paths retain their existing roles; path strings become positional data only at the Docker boundary.
  • Execution: the existing runCommand remains the sole Docker execution facility and now calls execFile('docker', args). Its cleanup callers migrate to the same argument representation, so no legacy shell runner or competing command form remains. The container shell parses only fixed operation scripts; request paths never enter those scripts.
  • Existing policies: volume-cleanup.mjs still owns cleanup scope, including hidden-entry preservation and whole-volume retention of main-statistics. Host export still replaces the destination before copying, and only private-report exports apply host-derived UID:GID ownership. Copy ordering, concurrency, HTTP status/schema, and client behavior are unchanged. Launch failures with empty stderr retain the native error message (for example, missing Docker), preserving actionable diagnostics after the runner change. No dependency, validation policy, or retry mechanism is added.

Validation

  • All 150 release tests pass, including literal file/directory round trips, destination replacement, private permissions/ownership, cleanup exclusions, and actual route/runner argument checks for POSIX, Windows drive, and UNC path forms.
  • Both TypeScript checks, changed-file ESLint, Prettier, and git diff --check pass.
  • Real Docker/Alpine file and directory round trips passed with metacharacters and repeated spaces in host folders, volume folders, and filenames. The temporary bind directory and dedicated test volume were removed afterward.
  • Native Docker execution was tested on Linux; Windows path handling is covered by the route fixtures, not a Windows Docker Desktop run.

@tavateva tavateva left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at 5f5b6d23dfc90229bd33c47e77dba37b887b4c8d.

This fixes #154 at the right layer.

  • runCommand now calls execFile('docker', args), so no host shell parses request data.
  • Each container sh -c receives a fixed script: mkdir -p "$1" && cp -r "$2" "$3", rm -rf "$1" && cp -r "$2" "$1" (with optional chown "$3" "$1"), or rm -rf "$1"/*. The paths follow as positional parameters after a sh $0.
  • Container paths are always absolute under /host/ or /volume/, so a name beginning with - can't be read as an option.
  • The duplicated private ingress builder is removed, and so is the whitespace normalisation that silently collapsed repeated spaces.
  • Cleanup keeps its semantics: the glob stays outside the quotes and hidden entries survive.
  • reject(stderr || error.message) keeps diagnostics such as spawn docker ENOENT.
  • Every caller of the three builders is updated, so no string-command path remains.

Verification at the head:

  • npm run test:release: 150 passed, 0 failed. npm run typecheck: clean. ESLint: clean on the changed files; the one warning is a pre-existing directive in an untouched file.

  • Independent round trip. No Docker daemon is available here, so I put a docker shim on PATH. It parses the real -v mappings, maps /host and /volume onto temp directories, and runs the generated sh -c script with the generated positional arguments. Driving the real copyFileToVolumeCommand, copyFileToHostCommand and deleteFilesFromVolumeFolder with:

    • the host folder host $(echo PWNED) it's;
    • the file Bob's stack.tif;
    • the file a "quoted" $HOME `id` two spaces.tif

    both directions copied the exact bytes, with stale destinations replaced. Nothing was expanded or executed, and cleanup left .keep in place. These are the cases that failed on 58b29a2.

  • Validate release inputs and the ubuntu/macOS memory collectors are green. coverage and the Windows collectors were still queued when I posted, during GitHub's ongoing Actions incident. I did not exercise Windows Docker Desktop. On Windows, libuv resolves docker to docker.exe without a shell, and Windows filenames can't contain ", so the route-fixture coverage is the relevant evidence.

Residual (outside #154, no change requested): a host folder whose name contains : still breaks the -v src:/host spec, as it did before. POSIX allows that character; Windows forbids it. --mount would trade that for comma parsing, so it's a separate decision if it ever matters.

@den-sq
den-sq merged commit 5e27339 into main Oct 19, 2026
9 checks passed
@den-sq
den-sq deleted the codex/154-literal-volume-paths branch October 19, 2026 16:47
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.

Volume server interpolates host paths into shell commands; apostrophes break copies

2 participants