fix(scripts): NUL-delimited compose-flags contract for path-with-spaces safety#1024
fix(scripts): NUL-delimited compose-flags contract for path-with-spaces safety#1024yasinBursali wants to merge 3 commits intoLight-Heart-Labs:mainfrom
Conversation
|
Audit follow-up: needs a stronger compose-flags representation. The array expansion reduces glob risk, but the claimed path-with-spaces fix is not complete: |
validate.sh, dream-preflight.sh, and validate-compose-stack.sh
expanded $COMPOSE_FLAGS (and $ENV_FILE_FLAG) unquoted when invoking
docker compose. Convert to bash-array expansion via `read -ra flags
<<< "$COMPOSE_FLAGS"` followed by `"${flags[@]}"`, matching the
established pattern in dream-cli.
Benefits:
- Eliminates 4 SC2086 shellcheck warnings.
- Prevents accidental glob expansion of flag values.
- Aligns three more consumer scripts with the project-canonical
compose-flags consumption pattern.
Note: resolve-compose-stack.sh emits relative paths only, so
absolute install paths with spaces (the "Jane Smith" scenario
sometimes raised on reports) do not appear in COMPOSE_FLAGS by
design. This refactor does not fix that scenario — it is
shellcheck/glob hardening and convention alignment.
Two sites in validate.sh (L64/65) previously passed a compose
invocation as a string to a check() helper running `bash -c "$cmd"`;
those are inlined as direct if/grep pipelines so array expansion
applies correctly without refactoring check() itself. Other check()
callers are untouched.
…es safety Maintainer audit on PR Light-Heart-Labs#1024 flagged that the original `read -ra COMPOSE_FLAGS_ARR <<< "$COMPOSE_FLAGS"` still splits on whitespace and breaks on `-f /tmp/path with spaces/compose.yml`. The PR's own scope clarification admitted this. The audit asked for a delimiter-safe contract (array, NUL/JSON/newline-delimited). End-to-end fix: - `resolve-compose-stack.sh` adds `--null` (alias `-0`) which emits individual argv tokens NUL-separated with a trailing NUL. Default (string) and `--env` modes are unchanged. - `validate.sh` and `dream-preflight.sh` switch to invoking the resolver with `--null` and reading into an array via the standard `while IFS= read -r -d '' _arg; do ...; done < <(...)` pattern. The broken `read -ra <<<` line is removed. - `validate-compose-stack.sh` adds `--compose-flag` (singular, repeatable) for path-safe input. The legacy `--compose-flags` (plural string) is preserved for the existing installer caller (installers/phases/02-detection.sh) which is on the legacy contract; callers that have access to NUL-safe data should use the singular form. Empirically verified end-to-end on macOS: legacy `--compose-flags "-f /tmp/ds path test.XXX/c.yml"` fails with "open /tmp/ds: file does not exist" (whitespace split); new `--compose-flag -f --compose-flag "/tmp/ds path test.XXX/c.yml"` succeeds. New regression test `tests/test-resolve-compose-null.sh` (8 cases) verifies: - --null produces NUL-separated bytes (inspected via tempfile because bash $() strips NUL silently). - Round-trip via `read -d ''` produces the expected token array. - Synthetic NUL stream containing a path with whitespace round-trips through the consumer pattern intact. - Default mode is unchanged (no NUL bytes; expected legacy string). Wired into `make test`. shellcheck clean on all changed files. Out of scope: `installers/lib/compose-select.sh` builds COMPOSE_FLAGS as a space-delimited string for the installer's own use; updating it to the NUL contract would expand scope into the installer chain. The new `--compose-flag` interface lets that caller migrate independently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…NUL stream CG follow-up: the original 8-case test scaffolded a script-dir whose parent path contained a space, but the resolver emits paths RELATIVE to script-dir, so the NUL stream never actually contained a literal space. The test was effectively only exercising "resolver runs in a spaced parent dir". Add a 9th case that creates `extensions/services/space ext/manifest.json` with `compose_file: compose.yaml`, drops a stub compose.yaml, and runs the resolver. The resolver enumerates the extension and emits `extensions/services/space ext/compose.yaml` as a relative path containing a literal space. The test asserts that exactly one token in the NUL-delimited stream matches that path verbatim — proving the end-to-end producer→consumer round-trip preserves embedded whitespace. Uses manifest.json (not yaml) so the test is independent of PyYAML. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
31d2e67 to
cd89f02
Compare
|
Pushed audit follow-up ( Empirically reproduced the path-with-spaces failure: Implemented an end-to-end NUL-delimited contract:
9 cases in
Local CG returned APPROVED WITH WARNINGS; the warning ("test-honesty: scaffold doesn't actually exercise space-in-path through resolver") is addressed in Out of scope (deferred follow-ups, called out in the PR body): Title updated to reflect the actual fix scope. |
What
Replace the broken
read -ra COMPOSE_FLAGS_ARR <<< "$COMPOSE_FLAGS"whitespace-split with an end-to-end NUL-delimited contract betweenresolve-compose-stack.shand its consumers, addressing the maintainer's audit follow-up thatread -rastill mangles paths containing spaces.Also adds a path-safe singular
--compose-flaginterface tovalidate-compose-stack.sh(legacy--compose-flagsplural string preserved for the existing installer caller).Why
The original PR's scope-clarification admitted: "install directories containing spaces do not appear in COMPOSE_FLAGS as absolute path components in the current code. This PR does not change that behaviour; it is shellcheck hardening and convention alignment only." The maintainer didn't accept that boundary and asked for a delimiter-safe contract.
Empirically reproduced the underlying bug: legacy
--compose-flags "-f /tmp/ds path test.XXX/c.yml"fails with "open /tmp/ds: file does not exist" becauseread -rasplits on whitespace inside the path. The same input via the new--compose-flag -f --compose-flag "/tmp/ds path test.XXX/c.yml"succeeds.How
Producer side:
dream-server/scripts/resolve-compose-stack.sh--null(alias-0) flag.NULL_MODEas an 8th positional argv. When set, it emits each argv token (-f, file-path,-f, file-path, …) as raw bytes viasys.stdout.buffer.write, NUL-separated, with a trailing NUL (suppressed when the resolved set is empty so consumerread -d ''loops terminate cleanly with zero tokens).--envmodes are unchanged.Consumer side:
dream-server/scripts/validate.sh,dream-server/scripts/dream-preflight.shread -ra <<< "$str"pattern with the standard NUL-safe consumer loop:COMPOSE_FLAGS_ARRarray is then passed via"${COMPOSE_FLAGS_ARR[@]}"todocker compose, preserving every embedded character including whitespace.Validator side:
dream-server/scripts/validate-compose-stack.sh--compose-flag(singular, repeatable) for path-safe input. Each invocation appends one already-tokenised argv element.--compose-flags(plural string) for legacy callers (the installer'sinstallers/phases/02-detection.sh:405passes the resolver's string output via this flag; updating that caller is a separate scope).COMPOSE_FLAGS_ARR. Empty-check is now${#COMPOSE_FLAGS_ARR[@]} -eq 0. Diagnostic-only display string is built from the array with[*]join.Testing
New
dream-server/tests/test-resolve-compose-null.sh(9 cases, wired intomake test):--nulloutput contains NUL bytes (inspected via tempfile because bash$()strips NUL silently).read -d ''produces the expected token array.-f docker-compose.base.yml.-f docker-compose.nvidia.yml.extensions/services/space ext/compose.yaml) is enumerated by the resolver, emitted as a single NUL-terminated token with the literal space preserved, and read back correctly by the consumer pattern.Empirical verification on macOS:
xxdinspection:--nulloutput is-f\0docker-compose.base.yml\0-f\0docker-compose.nvidia.yml\0...--compose-flags "-f /tmp/ds path test.XXX/c.yml" --quietexits 1 with "open /tmp/ds: file does not exist"; new--compose-flag -f --compose-flag "/tmp/ds path test.XXX/c.yml" --quietexits 0.make lint,make test,shellcheckall green. The 12 pre-existing Python 3.14 asyncio shim failures in dashboard-api tests are unrelated.Review
Local CG APPROVED WITH WARNINGS:
cd89f028): the original test'sSCRIPT_DIR_WITH_SPACEscaffold was misleading because the resolver emits paths relative to--script-dir, so the literal space never appeared in the NUL stream. The 9th case scaffolds an extension atextensions/services/space ext/so the relative pathextensions/services/space ext/compose.yamlitself contains a literal space — exercising the producer→consumer round-trip end-to-end.Out of scope (deferred follow-ups)
installers/lib/compose-select.shbuildsCOMPOSE_FLAGSas a space-delimited string for the installer's own use. Migrating that producer to NUL would expand scope into the installer chain. The new--compose-flagsingular interface is available when phase 02 wants to migrate independently.installers/phases/02-detection.sh:405still uses legacy--compose-flags "$COMPOSE_FLAGS". The resolver currently emits relative paths and the scriptcd "$INSTALL_DIR"s before invoking compose, so this path is mostly theoretical — but a path-safe migration is a clean follow-up.((FAILED++))underset -einvalidate.shanddream-preflight.sh: pre-existing across upstream/main, NOT introduced by this PR. Recommend separate cleanup PR using((FAILED++)) || trueorFAILED=$((FAILED + 1))."${arr[@]}"empty-array expansion under bash 3.2 +set -u: would crash, but repo posture is bash 4+ (enforced viaservice-registry.sh:18). Not a regression introduced by this PR; the legacy code paths used string variables that didn't have this trap.Platform Impact
--nullPython branch usessys.stdout.buffer.writewhich emits raw bytes correctly under all Python 3 versions.