From b4c5704865adc87a7f29a022e0c07c372edafa83 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 22 Sep 2026 14:43:58 +0200 Subject: [PATCH] Consolidate binary-mode env-strip lists into a shared data file tests/quarto-cmd.ts's kStripEnvVars, run-tests.ps1's $probeStrip, and run-tests.sh's inline env -u chain hand-maintained three copies of the same list, and had already drifted (the shell probes were missing 7 names, including QUARTO_VERSION_REQUIREMENT) at the moment they were introduced in #14706. All three now read tests/binary-mode-strip-env.txt at runtime, with identical parse semantics (trim including CR, skip blanks/#comments, validate ^[A-Za-z_][A-Za-z0-9_]*$, fail closed on missing/empty/malformed). buildBinaryEnv() is split into a pure sanitizeBinaryEnv(ambient, overlay) so the strip/overlay contract is unit-testable without touching Deno.env directly (test-anti-patterns.md forbids Deno.env.set()). The originally planned CI regression guard (setting QUARTO_VERSION_REQUIREMENT on the binary-mode workflow steps) was dropped: verified empirically that both launcher scripts (run-tests.sh's `quarto`, run-tests.ps1's `quarto.cmd`) special-case --version to print a static version and exit without ever invoking Deno, so no environment variable can affect that probe's outcome. The guard would have been dead code. --- .../built-version-testing-architecture.md | 10 ++- tests/README.md | 1 + tests/binary-mode-strip-env.txt | 29 +++++++ tests/quarto-cmd.ts | 59 +++++++++----- tests/run-tests.ps1 | 24 ++++-- tests/run-tests.sh | 26 +++++-- tests/unit/binary-mode-strip-env.test.ts | 78 +++++++++++++++++++ 7 files changed, 192 insertions(+), 35 deletions(-) create mode 100644 tests/binary-mode-strip-env.txt create mode 100644 tests/unit/binary-mode-strip-env.test.ts diff --git a/llm-docs/built-version-testing-architecture.md b/llm-docs/built-version-testing-architecture.md index 2622797bda7..32996a9cb36 100644 --- a/llm-docs/built-version-testing-architecture.md +++ b/llm-docs/built-version-testing-architecture.md @@ -1,8 +1,9 @@ --- -main_commit: 97f222ff3 -analyzed_date: 2026-09-21 +main_commit: 3d32aedb1 +analyzed_date: 2026-09-23 key_files: - tests/quarto-cmd.ts + - tests/binary-mode-strip-env.txt - tests/test.ts - tests/run-tests.sh - tests/run-tests.ps1 @@ -234,9 +235,12 @@ CI extracts artifacts to `RUNNER_TEMP`. ### D4. Child env: inherit ambient + strip dev vars (not clearEnv+allowlist) -Binary-mode spawns inherit the ambient environment minus a strip list (`QUARTO_SHARE_PATH`, `QUARTO_BIN_PATH`, `DENO_DIR`, `QUARTO_DEBUG`, `QUARTO_FORCE_VERSION`, ...), with `TestContext.env` overlaid last. +Binary-mode spawns inherit the ambient environment minus a strip list, with `TestContext.env` overlaid last. +The list (16 names, `QUARTO_SHARE_PATH`, `QUARTO_BIN_PATH`, `DENO_DIR`, `QUARTO_DEBUG`, `QUARTO_FORCE_VERSION`, ...) lives in one tracked file, `tests/binary-mode-strip-env.txt`, read at runtime by `quarto-cmd.ts`'s `buildBinaryEnv()`/`sanitizeBinaryEnv()` and by the `QUARTO_TEST_BIN` preflight probe in both `run-tests.sh` and `run-tests.ps1` — a single source of truth for what must not leak into a built-binary spawn, at either the probe or every subsequent test command. A `clearEnv` allowlist was rejected because the required Windows system variables (`SystemRoot`, `PATHEXT`, and others) are difficult to maintain reliably. The dev-tree exports in `run-tests.[sh|ps1]` are kept in all modes — the *harness* process still needs them; only the *child* is sanitized. +All three readers fail closed (missing/unreadable/empty file, or a malformed entry) rather than silently stripping nothing. +No CI check exercises the strip list's actual *effect*: the `--version` probe in `run-tests.[sh|ps1]` and `assertTestBinary()` is served by a launcher-level shortcut (`package/scripts/common/quarto`, `package/scripts/windows/quarto.cmd`) that prints the version and exits without invoking Deno at all, so no environment variable — including `QUARTO_VERSION_REQUIREMENT` — can ever change that probe's outcome. A CI guard built on setting `QUARTO_VERSION_REQUIREMENT` and expecting the probe to fail (or keep succeeding) was considered and dropped for this reason; it would have been dead code regardless of whether a reader correctly stripped the variable. ### D5. Silent-green guard: synthetic ERROR records diff --git a/tests/README.md b/tests/README.md index 8fb7540b993..f0cbe46bace 100644 --- a/tests/README.md +++ b/tests/README.md @@ -484,6 +484,7 @@ Don't do Tests normally run Quarto in-process from the dev sources. Set `QUARTO_TEST_BIN` to an installed Quarto to run commands against that binary instead. See [Built-Version Testing Architecture](../llm-docs/built-version-testing-architecture.md) for the harness and CI design. +The environment variables stripped from a binary-mode spawn are tracked in `binary-mode-strip-env.txt`, one name per line — shared by `quarto-cmd.ts` and both `run-tests.[sh|ps1]` preflight probes. To run in binary mode locally: diff --git a/tests/binary-mode-strip-env.txt b/tests/binary-mode-strip-env.txt new file mode 100644 index 00000000000..8e3add84e77 --- /dev/null +++ b/tests/binary-mode-strip-env.txt @@ -0,0 +1,29 @@ +# Environment variables removed from the inherited ambient environment before +# invoking a built quarto under test. A caller-supplied overlay is applied +# after removal and may reintroduce a name listed here on purpose. +# One variable name per line. Blank lines and '#' comments are ignored. +# +# Read by tests/quarto-cmd.ts (every binary-mode spawn) and by the +# QUARTO_TEST_BIN preflight probe in tests/run-tests.sh and tests/run-tests.ps1. + +# Dev-tree pointers exported by run-tests.[sh|ps1] and configure. +QUARTO_SHARE_PATH +QUARTO_BIN_PATH +QUARTO_ROOT +QUARTO_SRC_PATH +QUARTO_DENO +QUARTO_DENO_DOM +DENO_DIR + +# Dev-mode behaviour and version overrides. +QUARTO_DEBUG +QUARTO_FORCE_VERSION +QUARTO_VERSION_REQUIREMENT + +# Ambient per-run state that must not leak into a test spawn. +QUARTO_PROJECT_DIR +QUARTO_PROFILE +QUARTO_LOG +QUARTO_LOG_LEVEL +QUARTO_LOG_FORMAT +RSTUDIO diff --git a/tests/quarto-cmd.ts b/tests/quarto-cmd.ts index 62e7d784344..372e155b959 100644 --- a/tests/quarto-cmd.ts +++ b/tests/quarto-cmd.ts @@ -15,24 +15,31 @@ import { join } from "../src/deno_ral/path.ts"; // Strip dev-tree and logging state from built-binary spawns. Other ambient // variables are inherited, then the per-test environment is applied. -const kStripEnvVars = [ - "QUARTO_SHARE_PATH", - "QUARTO_BIN_PATH", - "QUARTO_DEBUG", - "DENO_DIR", - "QUARTO_DENO", - "QUARTO_DENO_DOM", - "QUARTO_ROOT", - "QUARTO_SRC_PATH", - "QUARTO_FORCE_VERSION", - "QUARTO_VERSION_REQUIREMENT", - "QUARTO_PROJECT_DIR", - "QUARTO_PROFILE", - "QUARTO_LOG", - "QUARTO_LOG_LEVEL", - "QUARTO_LOG_FORMAT", - "RSTUDIO", -]; +// The list lives in tests/binary-mode-strip-env.txt, shared with the +// QUARTO_TEST_BIN preflight probe in run-tests.sh and run-tests.ps1. +const kNameRe = /^[A-Za-z_][A-Za-z0-9_]*$/; + +function parseStripEnvVars(text: string, path: string): string[] { + const names = text + .split("\n") + .map((line) => line.replace(/\r$/, "").trim()) + .filter((line) => line.length > 0 && !line.startsWith("#")); + if (names.length === 0) { + throw new Error(`${path} yielded no variable names`); + } + for (const name of names) { + if (!kNameRe.test(name)) { + throw new Error(`${path} contains an invalid variable name: ${name}`); + } + } + return names; +} + +const kStripEnvVarsUrl = new URL("binary-mode-strip-env.txt", import.meta.url); +export const stripEnvVars = parseStripEnvVars( + Deno.readTextFileSync(kStripEnvVarsUrl), + kStripEnvVarsUrl.pathname, +); // std/log LogLevels.ERROR, as expected by readExecuteOutput(). const kErrorLevel = 40; @@ -60,16 +67,26 @@ export function quartoDevBinCmd(): string { return join(binPath, isWindows ? "quarto.cmd" : "quarto"); } -export function buildBinaryEnv( +// Pure core of buildBinaryEnv(): clones ambient/overlay before mutating, +// so neither input is modified. Names on the strip list are removed first; +// overlay is applied afterwards and may reintroduce a stripped name. +export function sanitizeBinaryEnv( + ambient: Record, overlay?: Record, ): Record { - const env = Deno.env.toObject(); - for (const name of kStripEnvVars) { + const env = { ...ambient }; + for (const name of stripEnvVars) { delete env[name]; } return { ...env, ...(overlay ?? {}) }; } +export function buildBinaryEnv( + overlay?: Record, +): Record { + return sanitizeBinaryEnv(Deno.env.toObject(), overlay); +} + // Sanitize direct subprocess spawns in binary mode. Dev-mode spawns inherit // the ambient environment and apply only the requested overlay. export function quartoSpawnEnvOptions( diff --git a/tests/run-tests.ps1 b/tests/run-tests.ps1 index e2edc1a9960..65aa3d3c4a8 100644 --- a/tests/run-tests.ps1 +++ b/tests/run-tests.ps1 @@ -73,12 +73,24 @@ If (-not [string]::IsNullOrEmpty($Env:QUARTO_TEST_BIN)) { Write-Host -ForegroundColor red "ERROR: QUARTO_TEST_BIN ($($Env:QUARTO_TEST_BIN)) does not exist" Exit 1 } - # Strip dev paths while probing the installed binary. - $probeStrip = @( - "QUARTO_SHARE_PATH", "QUARTO_BIN_PATH", "QUARTO_DEBUG", "DENO_DIR", - "QUARTO_DENO", "QUARTO_DENO_DOM", "QUARTO_ROOT", "QUARTO_SRC_PATH", - "QUARTO_FORCE_VERSION" - ) + # Strip dev paths while probing the installed binary. Shared list, see + # tests/binary-mode-strip-env.txt (also read by quarto-cmd.ts and run-tests.sh). + $stripEnvFile = Join-Path $SCRIPT_PATH "binary-mode-strip-env.txt" + If (-not (Test-Path $stripEnvFile)) { + Write-Host -ForegroundColor red "ERROR: strip-env list file not found: $stripEnvFile" + Exit 1 + } + $probeStrip = @(Get-Content $stripEnvFile | ForEach-Object { $_.Trim() } | Where-Object { $_ -ne "" -and -not $_.StartsWith("#") }) + If ($probeStrip.Count -eq 0) { + Write-Host -ForegroundColor red "ERROR: strip-env list file yielded no variable names: $stripEnvFile" + Exit 1 + } + ForEach ($name in $probeStrip) { + If ($name -notmatch "^[A-Za-z_][A-Za-z0-9_]*$") { + Write-Host -ForegroundColor red "ERROR: strip-env list file contains an invalid variable name: $name" + Exit 1 + } + } $probeSaved = @{} ForEach ($name in $probeStrip) { $probeSaved[$name] = [Environment]::GetEnvironmentVariable($name) diff --git a/tests/run-tests.sh b/tests/run-tests.sh index c8fa1902298..96baf1df82d 100755 --- a/tests/run-tests.sh +++ b/tests/run-tests.sh @@ -78,11 +78,27 @@ if [[ -n "$QUARTO_TEST_BIN" ]]; then echo "ERROR: QUARTO_TEST_BIN ($QUARTO_TEST_BIN) does not exist or is not executable" exit 1 fi - # Strip dev paths while probing the installed binary. - QUARTO_TEST_BIN_VERSION="$(env -u QUARTO_SHARE_PATH -u QUARTO_BIN_PATH \ - -u QUARTO_DEBUG -u DENO_DIR -u QUARTO_DENO -u QUARTO_DENO_DOM \ - -u QUARTO_ROOT -u QUARTO_SRC_PATH -u QUARTO_FORCE_VERSION \ - "$QUARTO_TEST_BIN" --version 2>/dev/null)" + # Strip dev paths while probing the installed binary. Shared list, see + # tests/binary-mode-strip-env.txt (also read by quarto-cmd.ts and run-tests.ps1). + strip_env_file="$SCRIPT_PATH/binary-mode-strip-env.txt" + if [[ ! -f "$strip_env_file" ]]; then + echo "ERROR: strip-env list file not found: $strip_env_file" + exit 1 + fi + strip_args=() + while IFS=$' \t\r' read -r name || [[ -n "$name" ]]; do + [[ -z "$name" || "$name" == \#* ]] && continue + if [[ ! "$name" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then + echo "ERROR: strip-env list file contains an invalid variable name: $name" + exit 1 + fi + strip_args+=(-u "$name") + done < "$strip_env_file" + if [[ "${#strip_args[@]}" -eq 0 ]]; then + echo "ERROR: strip-env list file yielded no variable names: $strip_env_file" + exit 1 + fi + QUARTO_TEST_BIN_VERSION="$(env "${strip_args[@]}" "$QUARTO_TEST_BIN" --version 2>/dev/null)" QUARTO_TEST_BIN_PROBE_EXIT=$? if [[ $QUARTO_TEST_BIN_PROBE_EXIT -ne 0 ]]; then echo "ERROR: QUARTO_TEST_BIN ($QUARTO_TEST_BIN) exited with code $QUARTO_TEST_BIN_PROBE_EXIT while reporting its version." diff --git a/tests/unit/binary-mode-strip-env.test.ts b/tests/unit/binary-mode-strip-env.test.ts new file mode 100644 index 00000000000..fe339e85de0 --- /dev/null +++ b/tests/unit/binary-mode-strip-env.test.ts @@ -0,0 +1,78 @@ +/* + * binary-mode-strip-env.test.ts + * + * Copyright (C) 2026 Posit Software, PBC + */ +import { assert, assertEquals } from "testing/asserts"; +import { unitTest } from "../test.ts"; +import { sanitizeBinaryEnv, stripEnvVars } from "../quarto-cmd.ts"; + +const kNameRe = /^[A-Za-z_][A-Za-z0-9_]*$/; + +unitTest( + "binary-mode-strip-env - parsed list is well-formed", + async () => { + assert(stripEnvVars.length > 0, "strip list must not be empty"); + assertEquals( + stripEnvVars.length, + new Set(stripEnvVars).size, + "strip list must not contain duplicates", + ); + for (const name of stripEnvVars) { + assert(kNameRe.test(name), `invalid variable name: ${name}`); + } + return Promise.resolve(); + }, +); + +unitTest( + "binary-mode-strip-env - sanitizeBinaryEnv strips listed names, keeps others", + async () => { + const ambient: Record = { UNRELATED_VAR: "keep" }; + for (const name of stripEnvVars) { + ambient[name] = "leak"; + } + const result = sanitizeBinaryEnv(ambient); + for (const name of stripEnvVars) { + assertEquals(result[name], undefined, `${name} should be stripped`); + } + assertEquals(result.UNRELATED_VAR, "keep"); + return Promise.resolve(); + }, +); + +unitTest( + "binary-mode-strip-env - overlay wins and may reintroduce a stripped name", + async () => { + const [firstStripped] = stripEnvVars; + const ambient: Record = { + [firstStripped]: "leak", + SAME_NAME: "ambient", + }; + const overlay = { [firstStripped]: "reintroduced", SAME_NAME: "overlay" }; + const result = sanitizeBinaryEnv(ambient, overlay); + assertEquals(result[firstStripped], "reintroduced"); + assertEquals(result.SAME_NAME, "overlay"); + return Promise.resolve(); + }, +); + +unitTest( + "binary-mode-strip-env - sanitizeBinaryEnv does not mutate its inputs", + async () => { + const [firstStripped] = stripEnvVars; + const ambient: Record = { + [firstStripped]: "leak", + UNRELATED_VAR: "keep", + }; + const overlay = { OVERLAY_VAR: "value" }; + const ambientBefore = { ...ambient }; + const overlayBefore = { ...overlay }; + + sanitizeBinaryEnv(ambient, overlay); + + assertEquals(ambient, ambientBefore); + assertEquals(overlay, overlayBefore); + return Promise.resolve(); + }, +);