From 8847da47ae23fd15496d009f5fbf9b1b65b33bf5 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 11:50:06 -0400 Subject: [PATCH 1/9] fix(env): keep stored ciphertext when a secret value is unchanged setProjectEnvValue now keeps an entry's existing ciphertext when it already decrypts to the requested value under the current key, so repeating `hack env set --secret --value-stdin` with the same value leaves the tracked env file byte-identical and git status clean. A different value, a plain entry becoming secret, or an entry that does not open under the current key is re-encrypted with a fresh IV. Env layer writes now commit by rename from an exclusive, fsynced sibling, so a write interrupted before the commit leaves the previous layer intact and resolvable. Existing permission bits and symlink targets are preserved. HACK-P08 moves from deferred to partial: per-key no-op detection and atomic layer writes exist; a multi-key hydration import remains missing. Refs HACK-1152 Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 9 + .../skills/hack-cli/references/patterns.md | 12 +- src/agents/pattern-catalogue.ts | 10 +- src/lib/fs.ts | 83 +++- src/lib/project-env-config.ts | 68 +++- tests/fixtures/project-env-set-worker.ts | 41 ++ tests/pattern-catalogue.test.ts | 2 +- tests/project-env-stable-ciphertext.test.ts | 369 ++++++++++++++++++ 8 files changed, 574 insertions(+), 20 deletions(-) create mode 100644 tests/fixtures/project-env-set-worker.ts create mode 100644 tests/project-env-stable-ciphertext.test.ts diff --git a/docs/env.md b/docs/env.md index 3452d237e..07e5d57d5 100644 --- a/docs/env.md +++ b/docs/env.md @@ -411,6 +411,15 @@ input and without writing anything: `.hack/hack.env..local.yaml`) instead of the shared repo env file — use it for per-checkout values you don't want to commit. +Repeating `hack env add` or `hack env set` with the value already stored is a no-op: when +the existing secret decrypts to the same value under the current key, its ciphertext is +kept, so the file stays byte-identical and Git shows no change. A different value, a +plain entry becoming secret, or an entry that does not decrypt under the current key gets +new ciphertext with a fresh IV. Each env file write goes to a temporary file in the same +directory and replaces the file by rename, so an interrupted write leaves the previous +file in place. Each command writes one key; a script that sets several keys is not atomic +across them. + If you omit `key`, `value`, or `--service`, `hack env add` prompts for them interactively. Pass `--no-interactive` (or set `HACK_NO_INTERACTIVE=1`) for scripted/agent usage: the command applies documented defaults or fails fast with `E_INTERACTIVE_REQUIRED` instead of diff --git a/plugins/hack/skills/hack-cli/references/patterns.md b/plugins/hack/skills/hack-cli/references/patterns.md index b191f8189..e9b9fb51b 100644 --- a/plugins/hack/skills/hack-cli/references/patterns.md +++ b/plugins/hack/skills/hack-cli/references/patterns.md @@ -14,7 +14,7 @@ missing part with ad hoc scripts. | HACK-P05 | Branch-owned external routing claim | Deferred | WU29 | | HACK-P06 | Opt-in public tunnel with end-to-end readiness | Deferred | WU29 | | HACK-P07 | Laptop and headless-runner capability overlays | Partial | WU27/WU30 | -| HACK-P08 | Tracked defaults plus idempotent machine-local hydration | Deferred | WU27/WU14 | +| HACK-P08 | Tracked defaults plus idempotent machine-local hydration | Partial | WU27/WU14 | | HACK-P09 | Disposable run workspaces with reference-aware retention | Partial | WU12/WU30 | | HACK-P10 | Pinned tool image plus independent dependency caches | Partial | WU30 | | HACK-P11 | Stable agent guidance and typed operation results | Partial | WU19/WU21 | @@ -135,12 +135,12 @@ missing part with ad hoc scripts. ## HACK-P08: Tracked defaults plus idempotent machine-local hydration - Use when: A repository tracks env defaults and each machine adds local values on top. -- Disposition: Deferred. Missing: Hack has no semantic no-op detection or atomic machine-local import contract for env hydration. +- Disposition: Partial. Missing: No-op detection and rename-committed writes cover one key per command. Hack has no multi-key import: an interrupted hydration can leave some keys updated and others not. - Owner: WU27/WU14; tracked in HACK-1152. -- Commands: none yet; do not substitute a hand-written recipe. +- Commands: `hack env explain --json` - Prerequisites: Tracked defaults, a machine-local layer and preserved operator overrides. -- Env ownership: Defaults are tracked; hydration writes only the local layer and keeps values out of argv. -- Readiness: Repeat hydration leaves the tracked tree clean; an interrupted import still leaves a resolvable layer. +- Env ownership: Defaults are tracked; hydration writes one key at a time with the `env set` subcommand, piping each value through `--value-stdin` (with `--secret`, and `--local` for the machine-local layer), so values stay out of argv. +- Readiness: Setting a secret to its current value keeps the stored ciphertext, so repeat hydration leaves the tracked tree clean; a changed value gets new ciphertext with a fresh IV. Each layer write commits by rename, so an interrupted write leaves the previous layer resolvable. - Cleanup: Unchanged values cause no rewrite. - Provenance: single-consumer generalization (HACK-P08 research register). @@ -214,4 +214,4 @@ missing part with ad hoc scripts. --- -Content revision: `091b4e80a9b6`. Generated by Hack; edit the canonical source and regenerate the plugin. +Content revision: `85e011bccf62`. Generated by Hack; edit the canonical source and regenerate the plugin. diff --git a/src/agents/pattern-catalogue.ts b/src/agents/pattern-catalogue.ts index 2aa0b783a..c9259d324 100644 --- a/src/agents/pattern-catalogue.ts +++ b/src/agents/pattern-catalogue.ts @@ -253,20 +253,20 @@ export const PATTERN_CATALOGUE: readonly PatternCard[] = [ title: "Tracked defaults plus idempotent machine-local hydration", useWhen: "A repository tracks env defaults and each machine adds local values on top.", - disposition: "deferred", - supportedCommands: [], + disposition: "partial", + supportedCommands: ["hack env explain --json"], prerequisites: "Tracked defaults, a machine-local layer and preserved operator overrides.", envOwnership: - "Defaults are tracked; hydration writes only the local layer and keeps values out of argv.", + "Defaults are tracked; hydration writes one key at a time with the `env set` subcommand, piping each value through `--value-stdin` (with `--secret`, and `--local` for the machine-local layer), so values stay out of argv.", readiness: - "Repeat hydration leaves the tracked tree clean; an interrupted import still leaves a resolvable layer.", + "Setting a secret to its current value keeps the stored ciphertext, so repeat hydration leaves the tracked tree clean; a changed value gets new ciphertext with a fresh IV. Each layer write commits by rename, so an interrupted write leaves the previous layer resolvable.", cleanup: "Unchanged values cause no rewrite.", negativeControl: "Re-encrypting unchanged entries causes diff and stash churn, and values passed in command arguments are not a secure import mechanism.", domainOwner: "WU27/WU14", missingContract: - "Hack has no semantic no-op detection or atomic machine-local import contract for env hydration.", + "No-op detection and rename-committed writes cover one key per command. Hack has no multi-key import: an interrupted hydration can leave some keys updated and others not.", provenance: { consumers: 1, source: "HACK-P08 research register" }, example: null, }, diff --git a/src/lib/fs.ts b/src/lib/fs.ts index f289e353a..706ea6229 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -1,4 +1,16 @@ -import { lstat, mkdir, stat } from "node:fs/promises"; +import { randomUUID } from "node:crypto"; +import { + constants, + type FileHandle, + lstat, + mkdir, + open, + realpath, + rename, + stat, + unlink, +} from "node:fs/promises"; +import { basename, dirname, join } from "node:path"; export async function pathExists(absolutePath: string): Promise { try { @@ -89,6 +101,75 @@ export async function writeTextFileIfChanged( return { changed: true }; } +/** + * Replaces `absolutePath` with `content` only when the text differs, publishing + * the new contents atomically. + * + * Commit point: a uniquely named sibling is created exclusively (no symlink + * follow), written and fsynced, then renamed over the target. A process that + * dies before the rename leaves the previous complete file in place and at most + * an orphaned `...tmp` sibling that no reader selects; the staging + * file is removed on every failure this process observes. Directory fsync is + * best effort. + * + * An existing target keeps its permission bits, and an existing symlink keeps + * pointing where it did: the replacement lands at the link's resolved path, as + * a plain write through the link would. A new file gets the umask default. + */ +export async function writeTextFileAtomicIfChanged( + absolutePath: string, + content: string +): Promise<{ readonly changed: boolean }> { + const existing = await readTextFile(absolutePath); + if (existing === content) { + return { changed: false }; + } + const target = + existing === null ? absolutePath : await realpath(absolutePath); + const mode = existing === null ? null : (await stat(target)).mode & 0o7777; + const staging = join( + dirname(target), + `.${basename(target)}.${randomUUID()}.tmp` + ); + const file = await open( + staging, + constants.O_CREAT | + constants.O_EXCL | + constants.O_NOFOLLOW | + constants.O_WRONLY, + 0o666 + ); + let committed = false; + try { + if (mode !== null) { + await file.chmod(mode); + } + await file.writeFile(content, "utf8"); + await file.sync(); + await rename(staging, target); + committed = true; + } finally { + await file.close(); + if (!committed) { + await unlink(staging).catch(() => undefined); + } + } + await syncDirectoryBestEffort(dirname(target)); + return { changed: true }; +} + +async function syncDirectoryBestEffort(directory: string): Promise { + let handle: FileHandle | null = null; + try { + handle = await open(directory, constants.O_RDONLY); + await handle.sync(); + } catch { + // Some filesystems refuse directory fsync; the file itself is synced. + } finally { + await handle?.close().catch(() => undefined); + } +} + /** * Ensures a marker-delimited managed block exists in a gitignore-style file. * diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 3a18af37c..2f533b849 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -33,6 +33,7 @@ import { pathExists, readTextFile, writeTextFile, + writeTextFileAtomicIfChanged, writeTextFileIfChanged, } from "./fs.ts"; import { @@ -2259,6 +2260,59 @@ function decryptProjectEnvStoredValue(opts: { }); } +/** + * Secret value to store for `plaintext`, keeping the entry's current ciphertext + * when it already opens to exactly `plaintext` under `keyText`. + * + * Fresh-IV encryption makes every write differ, so re-encrypting an unchanged + * value would churn a tracked file on each repeat. Reuse is semantic, not + * textual: an absent, plain, removed, malformed, wrong-key or different entry + * gets a new ciphertext with a fresh IV. Decryption failures are not reported; + * they only select rotation. + */ +function stableProjectEnvSecretValue(opts: { + readonly previous: ProjectEnvStoredValue | undefined; + readonly plaintext: string; + readonly keyText: string; +}): ProjectEnvSecretValue { + const previous = opts.previous; + if ( + previous !== undefined && + previous !== null && + isProjectEnvSecretValue(previous) && + opensToPlaintext({ + ciphertext: previous.secure, + plaintext: opts.plaintext, + keyText: opts.keyText, + }) + ) { + return { secure: previous.secure }; + } + return { + secure: encryptProjectEnvValue({ + plaintext: opts.plaintext, + keyText: opts.keyText, + }), + }; +} + +function opensToPlaintext(opts: { + readonly ciphertext: string; + readonly plaintext: string; + readonly keyText: string; +}): boolean { + try { + return ( + decryptProjectEnvValue({ + ciphertext: opts.ciphertext, + keyText: opts.keyText, + }) === opts.plaintext + ); + } catch { + return false; + } +} + export async function setProjectEnvValue(opts: { readonly projectRoot: string; readonly projectDir: string; @@ -2311,12 +2365,11 @@ export async function setProjectEnvValue(opts: { projectRoot: opts.projectRoot, }); createdKey = ensuredKey.created; - storedValue = { - secure: encryptProjectEnvValue({ - plaintext: opts.value, - keyText: ensuredKey.keyText, - }), - }; + storedValue = stableProjectEnvSecretValue({ + previous: read.config.values[scope]?.[opts.key], + plaintext: opts.value, + keyText: ensuredKey.keyText, + }); } if (opts.local === true) { await ensureHackDirGitignore({ @@ -2424,7 +2477,8 @@ async function writeProjectEnvConfigFile(opts: { const yaml = YAML.stringify(opts.config, null, 2); const text = yaml.endsWith("\n") ? yaml : `${yaml}\n`; await assertLegacyProjectDirectory({ projectDir: dirname(opts.path) }); - return (await writeTextFileIfChanged(opts.path, text)).changed; + // Rename-committed: an interrupted mutation leaves the previous layer intact. + return (await writeTextFileAtomicIfChanged(opts.path, text)).changed; } export async function materializeProjectEnv(opts: { diff --git a/tests/fixtures/project-env-set-worker.ts b/tests/fixtures/project-env-set-worker.ts new file mode 100644 index 000000000..a77f0ee6a --- /dev/null +++ b/tests/fixtures/project-env-set-worker.ts @@ -0,0 +1,41 @@ +import { mock } from "bun:test"; +import * as fs from "node:fs/promises"; + +/** + * Runs one `setProjectEnvValue` secret write in a disposable child and pauses + * it at the commit rename. Everything before that boundary (reading the layer, + * encrypting, creating, writing and fsyncing the staging file) runs unmocked; + * the parent then SIGKILLs the child to test a real pre-commit interruption. + * Values are synthetic test strings passed on stdin, never in argv. + */ +const [projectRoot, projectDir] = process.argv.slice(2); +if (!(projectRoot && projectDir)) { + throw new Error("Missing project env worker arguments"); +} + +const rename = fs.rename; +mock.module("node:fs/promises", () => ({ + ...fs, + rename: async (from: string, to: string) => { + process.stdout.write("prepared\n"); + // Hold the staged write open until the parent kills this process. + await Bun.sleep(120_000); + return await rename(from, to); + }, +})); + +const value = await Bun.stdin.text(); + +const { setProjectEnvValue } = await import( + "../../src/lib/project-env-config.ts" +); +await setProjectEnvValue({ + projectRoot, + projectDir, + envName: null, + scope: "global", + key: "API_TOKEN", + value, + secret: true, +}); +process.stdout.write("committed\n"); diff --git a/tests/pattern-catalogue.test.ts b/tests/pattern-catalogue.test.ts index 4aca9e395..a836df669 100644 --- a/tests/pattern-catalogue.test.ts +++ b/tests/pattern-catalogue.test.ts @@ -68,7 +68,7 @@ test("starting dispositions match the recorded decisions", () => { "HACK-P05": "deferred", "HACK-P06": "deferred", "HACK-P07": "partial", - "HACK-P08": "deferred", + "HACK-P08": "partial", "HACK-P09": "partial", "HACK-P10": "partial", "HACK-P11": "partial", diff --git a/tests/project-env-stable-ciphertext.test.ts b/tests/project-env-stable-ciphertext.test.ts new file mode 100644 index 000000000..c6ba1ffb6 --- /dev/null +++ b/tests/project-env-stable-ciphertext.test.ts @@ -0,0 +1,369 @@ +import { afterEach, expect, test } from "bun:test"; +import { + chmod, + mkdir, + mkdtemp, + readdir, + readFile, + rm, + stat, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { YAML } from "bun"; + +import { + PROJECT_COMPOSE_FILENAME, + PROJECT_CONFIG_FILENAME, + PROJECT_ENV_KEY_FILENAME, +} from "../src/constants.ts"; +import { + resolveProjectEnvConfig, + resolveProjectEnvConfigPath, + setProjectEnvValue, +} from "../src/lib/project-env-config.ts"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * HACK-1152 (pattern HACK-P08): repeating an unchanged secret write keeps the + * stored ciphertext, a changed value rotates it with a fresh IV, and an + * interrupted write leaves the previous layer resolvable. Every value here is a + * synthetic test string; every child gets a private HOME. + */ + +const REPO_ROOT = resolve(import.meta.dir, ".."); +const ENTRYPOINT = resolve(REPO_ROOT, "index.ts"); +const WORKER = resolve(import.meta.dir, "fixtures/project-env-set-worker.ts"); +const FIRST = "synthetic-first-6f1c"; +const SECOND = "synthetic-second-92ad"; + +const tempDirs = new Set(); + +afterEach(async () => { + for (const tempDir of tempDirs) { + await rm(tempDir, { recursive: true, force: true }); + } + tempDirs.clear(); +}); + +type Repo = { + readonly root: string; + readonly projectRoot: string; + readonly projectDir: string; + readonly envFile: string; +}; + +async function createRepo(): Promise { + const root = await mkdtemp(join(tmpdir(), "hack-env-stable-")); + tempDirs.add(root); + const projectRoot = resolve(root, "repo"); + const projectDir = resolve(projectRoot, ".hack"); + await mkdir(projectDir, { recursive: true }); + await writeFile( + resolve(projectDir, PROJECT_COMPOSE_FILENAME), + "services:\n api:\n image: alpine:3.20\n" + ); + await writeFile( + resolve(projectDir, PROJECT_CONFIG_FILENAME), + `${JSON.stringify({ name: "env-stable", dev_host: "env-stable.hack" }, null, 2)}\n` + ); + await writeFile( + resolve(projectRoot, ".gitignore"), + `${PROJECT_ENV_KEY_FILENAME}\n` + ); + return { + root, + projectRoot, + projectDir, + envFile: resolveProjectEnvConfigPath({ projectDir, envName: null }), + }; +} + +async function setSecret(repo: Repo, value: string) { + return await setProjectEnvValue({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + scope: "global", + key: "API_TOKEN", + value, + secret: true, + }); +} + +async function resolvedToken(repo: Repo): Promise { + const resolved = await resolveProjectEnvConfig({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + serviceNames: ["api"], + }); + return resolved?.globalEnv.API_TOKEN; +} + +function storedCiphertext(text: string): string { + const parsed: unknown = YAML.parse(text); + const secure = ( + parsed as { values?: { global?: { API_TOKEN?: { secure?: unknown } } } } + ).values?.global?.API_TOKEN?.secure; + if (typeof secure !== "string") { + throw new Error("Expected a stored secure value"); + } + return secure; +} + +/** v1 values are `v1:::`. */ +function ivSegment(ciphertext: string): string { + return ciphertext.split(":")[1] ?? ""; +} + +async function git(repo: Repo, args: readonly string[]): Promise { + const proc = Bun.spawn(["git", ...args], { + cwd: repo.projectRoot, + env: privateCliEnv({ + root: repo.root, + env: { GIT_CONFIG_NOSYSTEM: "1", GIT_CONFIG_GLOBAL: "/dev/null" }, + }), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + proc.exited, + ]); + if (exitCode !== 0) { + throw new Error(`git ${args[0] ?? ""} failed`); + } + return stdout; +} + +async function commitAll(repo: Repo): Promise { + await git(repo, ["add", "-A"]); + await git(repo, [ + "-c", + "user.name=Hack Test", + "-c", + "user.email=hack@example.com", + "commit", + "-q", + "-m", + "fixture", + ]); +} + +test("repeating an unchanged secret keeps the tracked file byte-identical", async () => { + const repo = await createRepo(); + await git(repo, ["init", "-q", "-b", "main"]); + const first = await setSecret(repo, FIRST); + expect(first.changed).toBe(true); + expect(first.createdKey).toBe(true); + await commitAll(repo); + const committed = await readFile(repo.envFile); + + for (let attempt = 0; attempt < 3; attempt += 1) { + const repeat = await setSecret(repo, FIRST); + expect(repeat.changed).toBe(false); + expect(repeat.createdKey).toBe(false); + } + expect((await readFile(repo.envFile)).equals(committed)).toBe(true); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + expect(await resolvedToken(repo)).toBe(FIRST); +}); + +test("a changed secret rotates ciphertext with a fresh IV and resolves", async () => { + const repo = await createRepo(); + await setSecret(repo, FIRST); + const before = storedCiphertext(await readFile(repo.envFile, "utf8")); + + const changed = await setSecret(repo, SECOND); + expect(changed.changed).toBe(true); + const after = storedCiphertext(await readFile(repo.envFile, "utf8")); + expect(after).not.toBe(before); + expect(ivSegment(after)).not.toBe(ivSegment(before)); + expect(await resolvedToken(repo)).toBe(SECOND); + + // Returning to an earlier value is a change, not a reuse of old ciphertext. + await setSecret(repo, FIRST); + const reverted = storedCiphertext(await readFile(repo.envFile, "utf8")); + expect(reverted).not.toBe(before); + expect(reverted).not.toBe(after); + expect(await resolvedToken(repo)).toBe(FIRST); +}); + +test("an entry that no longer opens under the current key is re-encrypted", async () => { + const repo = await createRepo(); + await setSecret(repo, FIRST); + const sealedUnderOldKey = storedCiphertext( + await readFile(repo.envFile, "utf8") + ); + // Synthetic replacement key: the stored entry cannot open under it. + await writeFile( + resolve(repo.projectRoot, PROJECT_ENV_KEY_FILENAME), + "synthetic-replacement-key-material-0123456789\n" + ); + + const result = await setSecret(repo, FIRST); + expect(result.changed).toBe(true); + const rotated = storedCiphertext(await readFile(repo.envFile, "utf8")); + expect(rotated).not.toBe(sealedUnderOldKey); + expect(await resolvedToken(repo)).toBe(FIRST); +}); + +test("plain and secret forms of the same value are never treated as unchanged", async () => { + const repo = await createRepo(); + await setProjectEnvValue({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + scope: "global", + key: "API_TOKEN", + value: FIRST, + secret: false, + }); + const upgraded = await setSecret(repo, FIRST); + expect(upgraded.changed).toBe(true); + const text = await readFile(repo.envFile, "utf8"); + expect(text).not.toContain(FIRST); + storedCiphertext(text); + expect(await resolvedToken(repo)).toBe(FIRST); +}); + +test("a rewrite keeps the layer's permission bits", async () => { + const repo = await createRepo(); + await setSecret(repo, FIRST); + await chmod(repo.envFile, 0o640); + await setSecret(repo, SECOND); + expect((await stat(repo.envFile)).mode & 0o777).toBe(0o640); +}); + +async function readUntil( + stream: ReadableStream, + marker: string +): Promise { + const reader = stream.getReader(); + const decoder = new TextDecoder(); + let seen = ""; + try { + while (!seen.includes(marker)) { + const chunk = await reader.read(); + if (chunk.done) { + throw new Error(`Worker exited before ${marker.trim()}`); + } + seen += decoder.decode(chunk.value, { stream: true }); + } + } finally { + reader.releaseLock(); + } +} + +test("a write killed before its commit rename leaves the previous layer resolvable", async () => { + const repo = await createRepo(); + await setSecret(repo, FIRST); + const original = await readFile(repo.envFile); + + const worker = Bun.spawn( + [process.execPath, WORKER, repo.projectRoot, repo.projectDir], + { + cwd: repo.projectRoot, + env: privateCliEnv({ root: repo.root }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + } + ); + worker.stdin.write(SECOND); + await worker.stdin.end(); + await readUntil(worker.stdout, "prepared\n"); + // The replacement is fully staged and fsynced, but not yet committed. + expect((await readFile(repo.envFile)).equals(original)).toBe(true); + worker.kill("SIGKILL"); + await worker.exited; + + const abandoned = (await readdir(repo.projectDir)).filter((name) => + name.endsWith(".tmp") + ); + expect(abandoned).toHaveLength(1); + const staged = await readFile( + join(repo.projectDir, abandoned[0] ?? ""), + "utf8" + ); + expect(storedCiphertext(staged)).not.toBe( + storedCiphertext(original.toString("utf8")) + ); + + // The old layer is intact and still resolves; the orphan is not a layer. + expect((await readFile(repo.envFile)).equals(original)).toBe(true); + expect(await resolvedToken(repo)).toBe(FIRST); + + // The next writer commits normally without adopting the orphan. + const retried = await setSecret(repo, SECOND); + expect(retried.changed).toBe(true); + expect(await resolvedToken(repo)).toBe(SECOND); + expect(storedCiphertext(await readFile(repo.envFile, "utf8"))).not.toBe( + storedCiphertext(staged) + ); +}, 30_000); + +async function runCli( + repo: Repo, + args: readonly string[], + stdin: string +): Promise { + const proc = Bun.spawn([process.execPath, ENTRYPOINT, ...args], { + cwd: repo.projectRoot, + env: privateCliEnv({ + root: repo.root, + env: { + HACK_GLOBAL_CONFIG_PATH: join(repo.root, "hack-config.json"), + HACK_LOGGER: "console", + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: "/dev/null", + }, + }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }); + proc.stdin.write(stdin); + await proc.stdin.end(); + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + expect(stdout).not.toContain(FIRST); + expect(stderr).not.toContain(FIRST); + return exitCode; +} + +test("repeat `env set --secret --value-stdin` leaves git status clean", async () => { + const repo = await createRepo(); + await writeFile( + repo.envFile, + "version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: {}\n" + ); + await git(repo, ["init", "-q", "-b", "main"]); + const setArgs = [ + "env", + "set", + "API_TOKEN", + "--secret", + "--value-stdin", + "--path", + repo.projectRoot, + ]; + expect(await runCli(repo, setArgs, `${FIRST}\n`)).toBe(0); + await commitAll(repo); + const committed = await readFile(repo.envFile); + + expect(await runCli(repo, setArgs, `${FIRST}\n`)).toBe(0); + expect((await readFile(repo.envFile)).equals(committed)).toBe(true); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + + // Positive control: a changed value does reach the tracked file. + expect(await runCli(repo, setArgs, `${SECOND}\n`)).toBe(0); + expect(await git(repo, ["status", "--porcelain"])).toBe( + " M .hack/hack.env.default.yaml\n" + ); +}, 60_000); From 77c599ac3ed09f64e67239c99cf19477183b7772 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 12:05:10 -0400 Subject: [PATCH 2/9] fix(env): ignore orphaned env staging files and pin atomic write behavior A `--local` env write killed before its commit rename left a `.hack.env*..tmp` staging file that git did not ignore, so a later `git add -A` could commit machine-local values, and a tracked-layer orphan left the tree dirty. The managed `.hack/.gitignore` now ignores `.hack.env*.tmp`, and `hack doctor` flags a tracked staging file. Tests now cover symlink preservation, staging cleanup after a failed rename, and clean git status after a killed tracked or local write. The argv-env-value recipe skips flags, so the P08 card can name `hack env set --local --value-stdin --secret`. Docs describe the first-repeat normalization of hand-edited files and the rename edge cases. Refs HACK-1152 Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 25 +++-- .../skills/hack-cli/references/patterns.md | 6 +- src/agents/pattern-catalogue.ts | 7 +- src/lib/doctor-generated-files.ts | 1 + src/lib/fs.ts | 8 +- src/templates.ts | 5 + tests/doctor-generated-files.test.ts | 11 ++- tests/fixtures/project-env-set-worker.ts | 10 +- tests/hack-gitignore.test.ts | 2 + tests/helpers/pattern-recipes.ts | 18 +++- tests/pattern-catalogue.test.ts | 20 ++++ tests/project-env-stable-ciphertext.test.ts | 92 +++++++++++++++++-- 12 files changed, 177 insertions(+), 28 deletions(-) diff --git a/docs/env.md b/docs/env.md index 07e5d57d5..d3263b015 100644 --- a/docs/env.md +++ b/docs/env.md @@ -51,6 +51,7 @@ worktrees inherit the rules with zero setup. It covers (patterns relative to - `.env.state.json` - `hack.env.local.yaml` - `hack.env.*.local.yaml` +- `.hack.env*.tmp` (staging files an interrupted env write can leave behind) - `tickets/` (retained only to contain retired machine-local ticket caches during upgrades) How it is maintained: @@ -413,12 +414,24 @@ per-checkout values you don't want to commit. Repeating `hack env add` or `hack env set` with the value already stored is a no-op: when the existing secret decrypts to the same value under the current key, its ciphertext is -kept, so the file stays byte-identical and Git shows no change. A different value, a -plain entry becoming secret, or an entry that does not decrypt under the current key gets -new ciphertext with a fresh IV. Each env file write goes to a temporary file in the same -directory and replaces the file by rename, so an interrupted write leaves the previous -file in place. Each command writes one key; a script that sets several keys is not atomic -across them. +kept, so once Hack has written the file it stays byte-identical and Git shows no change. +A hand-edited file (comments, other formatting, or no `global: {}`) is normalized on the +first repeat and stays byte-identical after that. A different value, a plain entry +becoming secret, or an entry that does not decrypt under the current key gets new +ciphertext with a fresh IV. Each command writes one key; a script that sets several keys +is not atomic across them. + +Each env file write goes to a hidden `...tmp` file in the same directory and +replaces the file by rename, so an interrupted write leaves the previous file in place. +A write killed before the rename can leave that staging file behind. It holds the whole +new layer, so `.hack/.gitignore` ignores `.hack.env*.tmp`, and you can delete it. Hack +does not remove it on the next write, because another write may still be using it. The +rename has these effects: + +- a symlinked env file keeps its link; the new contents land at the file the link points + to, and the staging file is created next to that file +- a symlink whose target does not exist is replaced by a regular file +- the directory must be writable, even when the env file itself is writable If you omit `key`, `value`, or `--service`, `hack env add` prompts for them interactively. Pass `--no-interactive` (or set `HACK_NO_INTERACTIVE=1`) for scripted/agent usage: the diff --git a/plugins/hack/skills/hack-cli/references/patterns.md b/plugins/hack/skills/hack-cli/references/patterns.md index e9b9fb51b..bdc5406d0 100644 --- a/plugins/hack/skills/hack-cli/references/patterns.md +++ b/plugins/hack/skills/hack-cli/references/patterns.md @@ -137,9 +137,9 @@ missing part with ad hoc scripts. - Use when: A repository tracks env defaults and each machine adds local values on top. - Disposition: Partial. Missing: No-op detection and rename-committed writes cover one key per command. Hack has no multi-key import: an interrupted hydration can leave some keys updated and others not. - Owner: WU27/WU14; tracked in HACK-1152. -- Commands: `hack env explain --json` +- Commands: `hack env set --local --value-stdin --secret`, `hack env explain --json` - Prerequisites: Tracked defaults, a machine-local layer and preserved operator overrides. -- Env ownership: Defaults are tracked; hydration writes one key at a time with the `env set` subcommand, piping each value through `--value-stdin` (with `--secret`, and `--local` for the machine-local layer), so values stay out of argv. +- Env ownership: Defaults are tracked; hydration runs `hack env set --local --value-stdin --secret` once per key with the value piped on stdin, so values stay out of argv. Without `--local` the same command writes a tracked default. - Readiness: Setting a secret to its current value keeps the stored ciphertext, so repeat hydration leaves the tracked tree clean; a changed value gets new ciphertext with a fresh IV. Each layer write commits by rename, so an interrupted write leaves the previous layer resolvable. - Cleanup: Unchanged values cause no rewrite. - Provenance: single-consumer generalization (HACK-P08 research register). @@ -214,4 +214,4 @@ missing part with ad hoc scripts. --- -Content revision: `85e011bccf62`. Generated by Hack; edit the canonical source and regenerate the plugin. +Content revision: `27bff041d416`. Generated by Hack; edit the canonical source and regenerate the plugin. diff --git a/src/agents/pattern-catalogue.ts b/src/agents/pattern-catalogue.ts index c9259d324..789138ce4 100644 --- a/src/agents/pattern-catalogue.ts +++ b/src/agents/pattern-catalogue.ts @@ -254,11 +254,14 @@ export const PATTERN_CATALOGUE: readonly PatternCard[] = [ useWhen: "A repository tracks env defaults and each machine adds local values on top.", disposition: "partial", - supportedCommands: ["hack env explain --json"], + supportedCommands: [ + "hack env set --local --value-stdin --secret", + "hack env explain --json", + ], prerequisites: "Tracked defaults, a machine-local layer and preserved operator overrides.", envOwnership: - "Defaults are tracked; hydration writes one key at a time with the `env set` subcommand, piping each value through `--value-stdin` (with `--secret`, and `--local` for the machine-local layer), so values stay out of argv.", + "Defaults are tracked; hydration runs `hack env set --local --value-stdin --secret` once per key with the value piped on stdin, so values stay out of argv. Without `--local` the same command writes a tracked default.", readiness: "Setting a secret to its current value keeps the stored ciphertext, so repeat hydration leaves the tracked tree clean; a changed value gets new ciphertext with a fresh IV. Each layer write commits by rename, so an interrupted write leaves the previous layer resolvable.", cleanup: "Unchanged values cause no rewrite.", diff --git a/src/lib/doctor-generated-files.ts b/src/lib/doctor-generated-files.ts index 0728c784d..e1eb7fbe0 100644 --- a/src/lib/doctor-generated-files.ts +++ b/src/lib/doctor-generated-files.ts @@ -28,6 +28,7 @@ export function buildGeneratedFilePathspecs(opts: { `${dir}/.env.state.json`, `${dir}/tickets`, `${dir}/hack.env.*.local.yaml`, + `${dir}/.hack.env*.tmp`, PROJECT_ENV_KEY_FILENAME, ]; } diff --git a/src/lib/fs.ts b/src/lib/fs.ts index 706ea6229..96558b275 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -109,12 +109,16 @@ export async function writeTextFileIfChanged( * follow), written and fsynced, then renamed over the target. A process that * dies before the rename leaves the previous complete file in place and at most * an orphaned `...tmp` sibling that no reader selects; the staging - * file is removed on every failure this process observes. Directory fsync is - * best effort. + * file is removed on every failure this process observes. Orphans are never + * swept here, because a concurrent writer may still own one; callers that write + * under `.hack/` rely on its managed `.gitignore` to keep them unstaged. + * Directory fsync is best effort. * * An existing target keeps its permission bits, and an existing symlink keeps * pointing where it did: the replacement lands at the link's resolved path, as * a plain write through the link would. A new file gets the umask default. + * Unlike an in-place write, a dangling symlink is replaced by a regular file, + * and the containing directory must be writable. */ export async function writeTextFileAtomicIfChanged( absolutePath: string, diff --git a/src/templates.ts b/src/templates.ts index a7b665d2b..0c7b447be 100644 --- a/src/templates.ts +++ b/src/templates.ts @@ -331,6 +331,10 @@ export const HACK_DIR_GITIGNORE_END_MARKER = "# end managed by hack" as const; * the `.hack/` directory. Root-level `.hack.secret.key` stays in the root * `.gitignore` (see `ensureProjectEnvSecretKey`). The retired `tickets/` * entry remains upgrade-safe so existing machine-local caches cannot be staged. + * `.hack.env*.tmp` covers the `...tmp` staging sibling that + * `writeTextFileAtomicIfChanged` leaves behind when an env layer write is + * killed before its commit rename; it holds a whole layer, including + * machine-local values, so it must never be staged. */ export const HACK_DIR_GITIGNORE_ENTRIES = [ ".internal/", @@ -339,6 +343,7 @@ export const HACK_DIR_GITIGNORE_ENTRIES = [ ".env.state.json", "hack.env.local.yaml", "hack.env.*.local.yaml", + ".hack.env*.tmp", "tickets/", ] as const; diff --git a/tests/doctor-generated-files.test.ts b/tests/doctor-generated-files.test.ts index 2f130e436..337706f5b 100644 --- a/tests/doctor-generated-files.test.ts +++ b/tests/doctor-generated-files.test.ts @@ -39,6 +39,8 @@ async function runGit(args: readonly string[], cwd: string): Promise { return stdout.trim(); } +const STAGING_ORPHAN = ".hack.env.local.yaml.0b88c1d2.tmp"; + /** * Creates a git repo where machine-local generated files were force-added and * committed — the leak scenario the doctor check exists to catch. @@ -69,6 +71,11 @@ async function createLeakedRepo(): Promise { resolve(repoRoot, ".hack", "tickets", "legacy-cache.json"), "{}\n" ); + // Staging file left by an env layer write killed before its commit rename. + await writeFile( + resolve(repoRoot, ".hack", STAGING_ORPHAN), + "version: 1\nvalues:\n global: {}\n" + ); await writeFile( resolve(repoRoot, PROJECT_ENV_KEY_FILENAME), "super-secret-key\n" @@ -90,6 +97,7 @@ test("inspectTrackedGeneratedFiles lists tracked generated files and flags the s PROJECT_ENV_KEY_FILENAME, ".hack/.branch/compose.x.override.yml", ".hack/.env.state.json", + `.hack/${STAGING_ORPHAN}`, ".hack/tickets/legacy-cache.json", ]); expect(inspection?.secretKeyTracked).toBe(true); @@ -138,7 +146,7 @@ test("untrackGeneratedFiles removes offenders from the index, keeps files on dis projectRoot: repoRoot, projectDirName: ".hack", }); - expect(inspection?.trackedPaths.length).toBe(4); + expect(inspection?.trackedPaths.length).toBe(5); // Same flow as `hack doctor --fix`: untrack, then ensure the nested ignore. const untracked = await untrackGeneratedFiles({ @@ -152,6 +160,7 @@ test("untrackGeneratedFiles removes offenders from the index, keeps files on dis for (const path of [ ".hack/.branch/compose.x.override.yml", ".hack/.env.state.json", + `.hack/${STAGING_ORPHAN}`, ".hack/tickets/legacy-cache.json", PROJECT_ENV_KEY_FILENAME, ]) { diff --git a/tests/fixtures/project-env-set-worker.ts b/tests/fixtures/project-env-set-worker.ts index a77f0ee6a..154a3393e 100644 --- a/tests/fixtures/project-env-set-worker.ts +++ b/tests/fixtures/project-env-set-worker.ts @@ -6,10 +6,13 @@ import * as fs from "node:fs/promises"; * it at the commit rename. Everything before that boundary (reading the layer, * encrypting, creating, writing and fsyncing the staging file) runs unmocked; * the parent then SIGKILLs the child to test a real pre-commit interruption. - * Values are synthetic test strings passed on stdin, never in argv. + * Values are synthetic test strings passed on stdin, never in argv. The optional + * third argument, `tracked` (default) or `local`, selects the layer written. */ -const [projectRoot, projectDir] = process.argv.slice(2); -if (!(projectRoot && projectDir)) { +const [projectRoot, projectDir, layer = "tracked"] = process.argv.slice(2); +if ( + !(projectRoot && projectDir && (layer === "tracked" || layer === "local")) +) { throw new Error("Missing project env worker arguments"); } @@ -37,5 +40,6 @@ await setProjectEnvValue({ key: "API_TOKEN", value, secret: true, + local: layer === "local", }); process.stdout.write("committed\n"); diff --git a/tests/hack-gitignore.test.ts b/tests/hack-gitignore.test.ts index 92663caa4..3a017d063 100644 --- a/tests/hack-gitignore.test.ts +++ b/tests/hack-gitignore.test.ts @@ -186,6 +186,8 @@ test("nested .hack/.gitignore makes git ignore generated files, including in lin ".hack/.env.state.json", ".hack/hack.env.local.yaml", ".hack/hack.env.qa.local.yaml", + ".hack/.hack.env.local.yaml.0b88c1d2.tmp", + ".hack/.hack.env.default.yaml.0b88c1d2.tmp", ".hack/tickets/state/projection.sqlite", ]) { expect( diff --git a/tests/helpers/pattern-recipes.ts b/tests/helpers/pattern-recipes.ts index fe447a022..4a71ef854 100644 --- a/tests/helpers/pattern-recipes.ts +++ b/tests/helpers/pattern-recipes.ts @@ -8,6 +8,22 @@ import { * Findings are closed codes; the scanned text is never echoed. */ +// One shell word on the same line. A backtick ends a word, because it closes +// an inline code span; a whole backtick command substitution is one operand. +const WORD = "[^\\s`]*"; +const FLAG = `[ \\t]+-${WORD}`; +const OPERAND = `[ \\t]+(?:\`[^\`\\n]*\`|[^\\s\`-]${WORD})`; + +/** + * `hack env set|add` with two operands, so a key and a value in argv. Flags + * between them are skipped, which keeps `hack env set --value-stdin` + * allowed. A flag that takes a value (`--env dev`) leaves that value as an + * operand, so such a form is still flagged. + */ +const ARGV_ENV_VALUE = new RegExp( + `\\bhack[ \\t]+env[ \\t]+(?:set|add)(?:${FLAG})*${OPERAND}(?:${FLAG})*${OPERAND}` +); + /** Recipes that must never appear outside a negative-example block. */ export const BANNED_RECIPES: readonly (readonly [string, RegExp])[] = [ ["broad-process-kill", /\b(?:pkill|killall)\b/], @@ -22,7 +38,7 @@ export const BANNED_RECIPES: readonly (readonly [string, RegExp])[] = [ "argv-secret-assignment", /\b[A-Z][A-Z0-9_]*(?:TOKEN|SECRET|PASSWORD|PASSWD|API_KEY)=\S/, ], - ["argv-env-value", /\bhack\s+env\s+(?:set|add)\s+\S+\s+\S+/], + ["argv-env-value", ARGV_ENV_VALUE], ]; function stripNegativeExamples(text: string): string { diff --git a/tests/pattern-catalogue.test.ts b/tests/pattern-catalogue.test.ts index a836df669..d4bb37be1 100644 --- a/tests/pattern-catalogue.test.ts +++ b/tests/pattern-catalogue.test.ts @@ -161,6 +161,26 @@ test("negative control: the banned-recipe scan flags each recipe outside a negat } }); +test("the argv-env-value scan skips flags but still flags a value operand", () => { + for (const text of [ + "hack env set API_TOKEN --local --value-stdin --secret", + "`hack env set --value-stdin` (pipe the value)", + "run `hack env set API_TOKEN` and pipe the value", + "hack env set API_TOKEN\nsecond line", + ]) { + expect(bannedRecipeFindings(text), text).toEqual([]); + } + for (const text of [ + "hack env set --local API_TOKEN synthetic-value", + "hack env add API_TOKEN --local synthetic-value", + "`hack env set API_TOKEN synthetic-value`", + "hack env set API_TOKEN `cat synthetic-file`", + "hack env set API_TOKEN --env dev", + ]) { + expect(bannedRecipeFindings(text), text).toEqual(["argv-env-value"]); + } +}); + test("the packaged hack-cli reference is the rendered catalogue", async () => { const packaged = await Bun.file( join( diff --git a/tests/project-env-stable-ciphertext.test.ts b/tests/project-env-stable-ciphertext.test.ts index c6ba1ffb6..8a0b69d31 100644 --- a/tests/project-env-stable-ciphertext.test.ts +++ b/tests/project-env-stable-ciphertext.test.ts @@ -1,12 +1,14 @@ import { afterEach, expect, test } from "bun:test"; import { chmod, + lstat, mkdir, mkdtemp, readdir, readFile, rm, stat, + symlink, writeFile, } from "node:fs/promises"; import { tmpdir } from "node:os"; @@ -18,9 +20,12 @@ import { PROJECT_CONFIG_FILENAME, PROJECT_ENV_KEY_FILENAME, } from "../src/constants.ts"; +import { writeTextFileAtomicIfChanged } from "../src/lib/fs.ts"; import { + ensureHackDirGitignore, resolveProjectEnvConfig, resolveProjectEnvConfigPath, + resolveProjectEnvLocalConfigPath, setProjectEnvValue, } from "../src/lib/project-env-config.ts"; import { privateCliEnv } from "./helpers/private-cli-env.ts"; @@ -80,7 +85,18 @@ async function createRepo(): Promise { }; } -async function setSecret(repo: Repo, value: string) { +type Layer = "tracked" | "local"; + +function layerPath(repo: Repo, layer: Layer): string { + return layer === "local" + ? resolveProjectEnvLocalConfigPath({ + projectDir: repo.projectDir, + envName: null, + }) + : repo.envFile; +} + +async function setSecret(repo: Repo, value: string, layer: Layer = "tracked") { return await setProjectEnvValue({ projectRoot: repo.projectRoot, projectDir: repo.projectDir, @@ -89,6 +105,7 @@ async function setSecret(repo: Repo, value: string) { key: "API_TOKEN", value, secret: true, + local: layer === "local", }); } @@ -257,13 +274,63 @@ async function readUntil( } } -test("a write killed before its commit rename leaves the previous layer resolvable", async () => { +test("a symlinked layer keeps its link and the write lands at the link target", async () => { const repo = await createRepo(); - await setSecret(repo, FIRST); - const original = await readFile(repo.envFile); + const shared = resolve(repo.root, "shared"); + await mkdir(shared); + const linkTarget = resolve(shared, "hack.env.default.yaml"); + await writeFile( + linkTarget, + "version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: {}\n" + ); + await symlink(linkTarget, repo.envFile); + + expect((await setSecret(repo, FIRST)).changed).toBe(true); + expect((await lstat(repo.envFile)).isSymbolicLink()).toBe(true); + storedCiphertext(await readFile(linkTarget, "utf8")); + expect(await resolvedToken(repo)).toBe(FIRST); + // Neither directory keeps a staging file after a committed write. + for (const dir of [repo.projectDir, shared]) { + expect( + (await readdir(dir)).filter((name) => name.endsWith(".tmp")) + ).toEqual([]); + } +}); + +test("a write that fails after staging removes its staging file", async () => { + const repo = await createRepo(); + // A directory at the target path: staging succeeds, the commit rename fails. + const target = resolve(repo.projectDir, "occupied.yaml"); + await mkdir(target); + + await expect( + writeTextFileAtomicIfChanged(target, "synthetic: content\n") + ).rejects.toThrow(); + expect((await stat(target)).isDirectory()).toBe(true); + expect( + (await readdir(repo.projectDir)).filter((name) => name.endsWith(".tmp")) + ).toEqual([]); +}); + +for (const layer of ["tracked", "local"] as const) { + test(`a ${layer} write killed before its commit rename leaves the layer resolvable and git clean`, async () => { + await assertKilledWriteIsRecoverable(layer); + }, 30_000); +} + +async function assertKilledWriteIsRecoverable(layer: Layer): Promise { + const repo = await createRepo(); + const layerFile = layerPath(repo, layer); + await git(repo, ["init", "-q", "-b", "main"]); + // `hack init` writes the managed `.hack/.gitignore`; `--local` writes heal it. + await ensureHackDirGitignore({ projectDir: repo.projectDir }); + await setSecret(repo, FIRST, layer); + await commitAll(repo); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + const original = await readFile(layerFile); const worker = Bun.spawn( - [process.execPath, WORKER, repo.projectRoot, repo.projectDir], + [process.execPath, WORKER, repo.projectRoot, repo.projectDir, layer], { cwd: repo.projectRoot, env: privateCliEnv({ root: repo.root }), @@ -276,7 +343,7 @@ test("a write killed before its commit rename leaves the previous layer resolvab await worker.stdin.end(); await readUntil(worker.stdout, "prepared\n"); // The replacement is fully staged and fsynced, but not yet committed. - expect((await readFile(repo.envFile)).equals(original)).toBe(true); + expect((await readFile(layerFile)).equals(original)).toBe(true); worker.kill("SIGKILL"); await worker.exited; @@ -293,17 +360,22 @@ test("a write killed before its commit rename leaves the previous layer resolvab ); // The old layer is intact and still resolves; the orphan is not a layer. - expect((await readFile(repo.envFile)).equals(original)).toBe(true); + expect((await readFile(layerFile)).equals(original)).toBe(true); expect(await resolvedToken(repo)).toBe(FIRST); + // The orphan holds the whole layer, so it must be ignored: `git add -A` + // would otherwise commit machine-local values, and the tree would be dirty. + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + await git(repo, ["add", "-A"]); + expect(await git(repo, ["diff", "--cached", "--name-only"])).toBe(""); // The next writer commits normally without adopting the orphan. - const retried = await setSecret(repo, SECOND); + const retried = await setSecret(repo, SECOND, layer); expect(retried.changed).toBe(true); expect(await resolvedToken(repo)).toBe(SECOND); - expect(storedCiphertext(await readFile(repo.envFile, "utf8"))).not.toBe( + expect(storedCiphertext(await readFile(layerFile, "utf8"))).not.toBe( storedCiphertext(staged) ); -}, 30_000); +} async function runCli( repo: Repo, From ad65a7ea2dd0682c2709f0168c5e57c1b6a808ca Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 12:09:41 -0400 Subject: [PATCH 3/9] chore(hack): refresh the managed .hack/.gitignore block Refs HACK-1152 Co-Authored-By: Claude Opus 5.5 --- .hack/.gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.hack/.gitignore b/.hack/.gitignore index d6aeb28b5..1dcac9688 100644 --- a/.hack/.gitignore +++ b/.hack/.gitignore @@ -5,5 +5,6 @@ .env.state.json hack.env.local.yaml hack.env.*.local.yaml +.hack.env*.tmp tickets/ # end managed by hack From 06f262309db3111e65c8a23324456ef73ac57e83 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 12:48:25 -0400 Subject: [PATCH 4/9] feat(env): idempotent batch hydration from stdin or file Add `hack env hydrate`, which reads KEY=value entries (or one JSON object with --format json) from piped stdin or --file and writes them to the machine-local env layer in one rename-committed write. - Only the effective local layer is written; a git-tracked one is refused. - An entry that already holds its value is kept (secret ciphertext and scalar YAML type), and a no-op import does not rewrite the file. A changed value gets new ciphertext with a fresh IV. - Local entries outside the input, other scopes and removal markers for other keys are kept. - A secret in the layer that does not open under the reader key, a key-file mint that would shadow HACK_ENV_SECRET_KEY, a held or stale hydration lock, and a layer changed by another writer (compare-and-swap recheck before the rename) all refuse without changing the layer. - Input is bounded (1 MiB), strict and all-or-nothing; refusals are closed and value-free and name at most a line number. - Unexpected positional arguments are now reported by count, not echoed, so `hack env hydrate KEY=value` cannot print the value. Refs HACK-1158. Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 69 +- docs/reference/cli.md | 29 + .../skills/hack-cli/references/patterns.md | 12 +- src/agents/pattern-catalogue.ts | 10 +- src/cli/command.ts | 7 +- src/commands/env.ts | 119 +++ src/lib/env-hydrate-input.ts | 417 +++++++++++ src/lib/fs.ts | 28 +- src/lib/project-env-config.ts | 510 ++++++++++++- tests/cli-command.test.ts | 9 + tests/env-hydrate-input.test.ts | 233 ++++++ tests/fixtures/project-env-hydrate-worker.ts | 84 +++ tests/project-env-hydrate.test.ts | 692 ++++++++++++++++++ 13 files changed, 2203 insertions(+), 16 deletions(-) create mode 100644 src/lib/env-hydrate-input.ts create mode 100644 tests/env-hydrate-input.test.ts create mode 100644 tests/fixtures/project-env-hydrate-worker.ts create mode 100644 tests/project-env-hydrate.test.ts diff --git a/docs/env.md b/docs/env.md index d3263b015..7f17db0d5 100644 --- a/docs/env.md +++ b/docs/env.md @@ -255,6 +255,7 @@ into a host command, and `--target` on env and host commands chooses the `host` | `hack env apply` | `--env`, `--service` | Runs `hack restart ` with the same `--env`, recreating one service. | This is the refresh path for one service. | `--service` is required. | | `hack env add` | `--env`, `--service` | Selects the file written (`hack.env..yaml`, or the base file for `base`/`default`/`none`; `--local` writes the local variant). | Running workloads keep old values until `hack env apply` or `hack restart`. | Mutates only env files. | | `hack env set` | `--env`, `--service` | Alias for `hack env add`. | Same as `hack env add`. | Mutates only env files. | +| `hack env hydrate` | `--env`, `--service` | Selects the local file written, as for `hack env add --local`; imports every entry in one commit. | Same as `hack env add`. | Mutates only the machine-local env file; refuses a git-tracked one. | | `hack env unset` | `--env`, `--service` | Selects the file the key is removed from, as for `hack env add`. | Same as `hack env add`. | Mutates only env files. | | `hack env materialize` | `--env`, `--service` | Writes `.hack/.env` once, at invocation. | Rerun `hack env materialize`. | Compatibility output for external tools; runtime injection does not depend on it. | | `hack env exec` | `--env`, `--service`, `--target` | Per invocation, into one host command (host view by default). | Each invocation resolves again. | Runs on the host, not in a container. | @@ -419,7 +420,8 @@ A hand-edited file (comments, other formatting, or no `global: {}`) is normalize first repeat and stays byte-identical after that. A different value, a plain entry becoming secret, or an entry that does not decrypt under the current key gets new ciphertext with a fresh IV. Each command writes one key; a script that sets several keys -is not atomic across them. +is not atomic across them. To write many machine-local values in one commit, use +`hack env hydrate` (below). Each env file write goes to a hidden `...tmp` file in the same directory and replaces the file by rename, so an interrupted write leaves the previous file in place. @@ -448,6 +450,71 @@ hack env unset SERVICE_TOKEN --service api --env qa hack env unset API_BASE_URL --local ``` +### Hydrate machine-local values in one step + +`hack env hydrate` imports many values into the machine-local layer at once, for example +when a new checkout or machine pulls its secrets from a secret manager. Tracked defaults +stay in `hack.env.default.yaml`; hydration writes only the gitignored local layer +(`.hack/hack.env.local.yaml`, or `.hack/hack.env..local.yaml` with `--env`). + +```bash +# Pipe KEY=value lines from a secret manager; values never appear in argv. + | hack env hydrate --secret + +# Or read a file, or a pipe such as <(secret-source). +hack env hydrate --secret --file .secrets.env +hack env hydrate --secret --file secrets.json --format json --env qa --service api +``` + +Input comes only from piped stdin or `--file`, never from arguments, and is at most +1 MiB. The default `env` format is strict, one entry per line: + +- blank lines and lines starting with `#` are skipped, and an `export ` prefix is allowed +- `KEY=value`, where the key matches `^[A-Z_][A-Z0-9_]*$`; an unquoted value is the + rest of the line with surrounding spaces removed, and `#` inside it is part of the value +- `'...'` is literal; `"..."` accepts `\n`, `\r`, `\t`, `\"` and `\\`; a quoted value + must close on the same line, followed only by spaces or a `# comment` +- empty values, repeated keys and `${VAR}` expansion are not supported + +`--format json` takes one object whose values are non-empty strings. One malformed +entry refuses the whole input. The message gives only the line number, never the +line, and nothing is written. + +`--secret` encrypts every imported value; without it the values are stored as plain +entries in the local layer. `--service` picks one scope for all entries (default +`global`). The output and `--json` result list the added, updated and unchanged keys, +never values. + +Hydration is idempotent and all-or-nothing: + +- An entry that already holds its value is left alone: a secret keeps its ciphertext + and a plain number keeps its YAML type. When nothing changes, the file is not + rewritten, so comments and formatting survive and Git shows no change. +- A changed value gets new ciphertext with a fresh IV. +- Local entries that are not in the input, other scopes, and removal markers for other + keys are kept. A key in the input replaces that key's local entry, including a + value you set by hand with `--local`. +- The whole new layer is written to a staging file and published by one rename, so an + interrupted hydration leaves the previous layer, with every key at its old value. + +Hydration refuses, without changing the layer, when: + +- git tracks the target local layer +- a secret already in the local layer does not decrypt under the project env key, or no + key is available +- `--secret` is used, no `.hack.secret.key` exists, and `HACK_ENV_SECRET_KEY` is set + (a new key file would replace that key for every layer) +- another `hack env hydrate` for the same project is running +- the local layer changed between hydration's read and its rename, for example from a + concurrent `hack env set --local` + +Concurrent hydrations are serialized by `.hack/.internal/env-hydrate.lock`, which holds +the owner's process ID. A hydration that is killed (for example with SIGKILL) cannot +remove it; the next hydration then reports the stale lock and its path instead of taking +it over. Remove the file once no hydration is running. `hack env set --local` does not +take this lock. Hydration checks the layer again just before its rename, which narrows +the window in which such a write could be overwritten but cannot close it. + Materialize a compatibility `.hack/.env`: ```bash diff --git a/docs/reference/cli.md b/docs/reference/cli.md index dd1f96c8c..011efee21 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -2205,6 +2205,7 @@ hack env [options] | `hack env apply` | Recreate one service with its resolved env | | `hack env add [key] [value]` | Add or update an env value | | `hack env set [key] [value]` | Alias for env add | +| `hack env hydrate` | Import many values into the machine-local env layer at once | | `hack env materialize` | Write a compatibility .env file from the selected overlay | | `hack env exec [command...]` | Run a host command with project env injected | | `hack env shell` | Open a host shell with project env injected | @@ -2394,6 +2395,34 @@ Same value rules as `hack env add`: pipe secrets with --value-stdin or use the h | `--help, -h` | Show help | | `--version, -v` | Show version | +## `hack env hydrate` + +Import many values into the machine-local env layer at once + +### Usage + +```bash +hack env hydrate [options] +``` + +Reads KEY=value entries from piped stdin or --file (never from arguments) and writes them, in one atomic commit, to the worktree-local env file (`hack.env.local.yaml`, or `hack.env..local.yaml` with --env). Tracked env files are never written. Repeating an import with the same values changes nothing; other local entries are kept. Values are never printed. + +### Options + +| Option | Description | +| --- | --- | +| `--path, -p ` | Run a project command against a repo path (overrides cwd search) | +| `--project ` | Target a registered project by name (from ~/.hack/projects.json) | +| `--env ` | Select an env overlay by name; omit for the configured default overlay, or use 'base' or 'default' for base layers only | +| `--json` | Output JSON (machine-readable) | +| `--secret` | Store value as an encrypted secure entry | +| `--service ` | Target scope (global or a discovered service name) | +| `--file ` | Read entries from this file (or a pipe such as <(producer)) instead of stdin | +| `--format ` | Input format: KEY=value lines (env, default) or one JSON object of string values (json) | +| `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | +| `--help, -h` | Show help | +| `--version, -v` | Show version | + ## `hack env materialize` Write a compatibility .env file from the selected overlay diff --git a/plugins/hack/skills/hack-cli/references/patterns.md b/plugins/hack/skills/hack-cli/references/patterns.md index bdc5406d0..ef39aec19 100644 --- a/plugins/hack/skills/hack-cli/references/patterns.md +++ b/plugins/hack/skills/hack-cli/references/patterns.md @@ -135,13 +135,13 @@ missing part with ad hoc scripts. ## HACK-P08: Tracked defaults plus idempotent machine-local hydration - Use when: A repository tracks env defaults and each machine adds local values on top. -- Disposition: Partial. Missing: No-op detection and rename-committed writes cover one key per command. Hack has no multi-key import: an interrupted hydration can leave some keys updated and others not. +- Disposition: Partial. Missing: Hydration replaces an imported key's local entry; it cannot tell an operator's deliberate local override of that key from a stale hydrated value. `hack env set --local` does not take the hydration lock, so only a recheck just before the rename guards against it. - Owner: WU27/WU14; tracked in HACK-1152. -- Commands: `hack env set --local --value-stdin --secret`, `hack env explain --json` +- Commands: `hack env hydrate --file --secret`, `hack env set --local --value-stdin --secret`, `hack env explain --json` - Prerequisites: Tracked defaults, a machine-local layer and preserved operator overrides. -- Env ownership: Defaults are tracked; hydration runs `hack env set --local --value-stdin --secret` once per key with the value piped on stdin, so values stay out of argv. Without `--local` the same command writes a tracked default. -- Readiness: Setting a secret to its current value keeps the stored ciphertext, so repeat hydration leaves the tracked tree clean; a changed value gets new ciphertext with a fresh IV. Each layer write commits by rename, so an interrupted write leaves the previous layer resolvable. -- Cleanup: Unchanged values cause no rewrite. +- Env ownership: Defaults are tracked; `hack env hydrate --secret` reads `KEY=value` entries from piped stdin or `--file` and writes only the gitignored machine-local layer, so values stay out of argv and out of tracked files. Local entries outside the input are kept. +- Readiness: A repeat import with the same values does not rewrite the layer; a changed value gets new ciphertext with a fresh IV. One rename commits all keys, so an interrupted import leaves every key at its previous value. A wrong key, a tracked local layer or a concurrent hydration refuses without changing the layer. +- Cleanup: Unchanged values cause no rewrite. A killed import can leave a staging file and a stale `.hack/.internal/env-hydrate.lock`; the next import reports the lock instead of taking it over. - Provenance: single-consumer generalization (HACK-P08 research register). @@ -214,4 +214,4 @@ missing part with ad hoc scripts. --- -Content revision: `27bff041d416`. Generated by Hack; edit the canonical source and regenerate the plugin. +Content revision: `d2a336816ac2`. Generated by Hack; edit the canonical source and regenerate the plugin. diff --git a/src/agents/pattern-catalogue.ts b/src/agents/pattern-catalogue.ts index 789138ce4..e6ae909cd 100644 --- a/src/agents/pattern-catalogue.ts +++ b/src/agents/pattern-catalogue.ts @@ -255,21 +255,23 @@ export const PATTERN_CATALOGUE: readonly PatternCard[] = [ "A repository tracks env defaults and each machine adds local values on top.", disposition: "partial", supportedCommands: [ + "hack env hydrate --file --secret", "hack env set --local --value-stdin --secret", "hack env explain --json", ], prerequisites: "Tracked defaults, a machine-local layer and preserved operator overrides.", envOwnership: - "Defaults are tracked; hydration runs `hack env set --local --value-stdin --secret` once per key with the value piped on stdin, so values stay out of argv. Without `--local` the same command writes a tracked default.", + "Defaults are tracked; `hack env hydrate --secret` reads `KEY=value` entries from piped stdin or `--file` and writes only the gitignored machine-local layer, so values stay out of argv and out of tracked files. Local entries outside the input are kept.", readiness: - "Setting a secret to its current value keeps the stored ciphertext, so repeat hydration leaves the tracked tree clean; a changed value gets new ciphertext with a fresh IV. Each layer write commits by rename, so an interrupted write leaves the previous layer resolvable.", - cleanup: "Unchanged values cause no rewrite.", + "A repeat import with the same values does not rewrite the layer; a changed value gets new ciphertext with a fresh IV. One rename commits all keys, so an interrupted import leaves every key at its previous value. A wrong key, a tracked local layer or a concurrent hydration refuses without changing the layer.", + cleanup: + "Unchanged values cause no rewrite. A killed import can leave a staging file and a stale `.hack/.internal/env-hydrate.lock`; the next import reports the lock instead of taking it over.", negativeControl: "Re-encrypting unchanged entries causes diff and stash churn, and values passed in command arguments are not a secure import mechanism.", domainOwner: "WU27/WU14", missingContract: - "No-op detection and rename-committed writes cover one key per command. Hack has no multi-key import: an interrupted hydration can leave some keys updated and others not.", + "Hydration replaces an imported key's local entry; it cannot tell an operator's deliberate local override of that key from a stale hydrated value. `hack env set --local` does not take the hydration lock, so only a recheck just before the rename guards against it.", provenance: { consumers: 1, source: "HACK-P08 research register" }, example: null, }, diff --git a/src/cli/command.ts b/src/cli/command.ts index d12b833f1..7bf585f2e 100644 --- a/src/cli/command.ts +++ b/src/cli/command.ts @@ -448,8 +448,11 @@ export function parsePositionalsForCommand< } if (idx < remaining.length) { - const extra = remaining.slice(idx).join(" "); - throw new CliUsageError(`Unexpected arguments: ${extra}`); + // Count only: an extra argument is often a pasted `KEY=value` or secret. + const extra = remaining.length - idx; + throw new CliUsageError( + `Unexpected arguments: ${extra} more than this command accepts. They are not shown because they may contain values.` + ); } return out as PositionalsValues; diff --git a/src/commands/env.ts b/src/commands/env.ts index bcef1651d..eac7edf24 100644 --- a/src/commands/env.ts +++ b/src/commands/env.ts @@ -22,6 +22,10 @@ import { resolveRegisteredProjectOption } from "../cli/project-selector.ts"; import { readControlPlaneConfig } from "../control-plane/sdk/config.ts"; import { updateGlobalConfig } from "../lib/config.ts"; import { resolveGlobalConfigPath } from "../lib/config-paths.ts"; +import { + parseEnvHydrateInputFormat, + readEnvHydrateInput, +} from "../lib/env-hydrate-input.ts"; import { assertEnvValueStdinUsage, readEnvValueFromStdin, @@ -69,6 +73,7 @@ import { import { discoverComposeServiceNames, ENV_KEY_INVALID_MESSAGE, + hydrateProjectEnvLocalLayer, materializeProjectEnv, migrateLegacyProjectEnv, parseProjectEnvTarget, @@ -289,6 +294,44 @@ const setSpec = defineCommand({ subcommands: [], } as const); +const optHydrateFile = defineOption({ + name: "file", + type: "string", + long: "--file", + valueHint: "", + description: + "Read entries from this file (or a pipe such as <(producer)) instead of stdin", +} as const); + +const optHydrateFormat = defineOption({ + name: "format", + type: "string", + long: "--format", + valueHint: "", + description: + "Input format: KEY=value lines (env, default) or one JSON object of string values (json)", +} as const); + +const hydrateSpec = defineCommand({ + name: "hydrate", + summary: "Import many values into the machine-local env layer at once", + description: + "Reads KEY=value entries from piped stdin or --file (never from arguments) and writes them, in one atomic commit, to the worktree-local env file (`hack.env.local.yaml`, or `hack.env..local.yaml` with --env). Tracked env files are never written. Repeating an import with the same values changes nothing; other local entries are kept. Values are never printed.", + group: "Project", + options: [ + optPath, + optProject, + optEnv, + optJson, + optSecret, + optService, + optHydrateFile, + optHydrateFormat, + ], + positionals: [], + subcommands: [], +} as const); + const materializeSpec = defineCommand({ name: "materialize", summary: "Write a compatibility .env file from the selected overlay", @@ -1919,6 +1962,81 @@ const handleEnvSet: CommandHandlerFor = async ({ return 0; }; +const handleEnvHydrate: CommandHandlerFor = async ({ + ctx, + args, +}): Promise => { + // Refuse bad options, select the project, and read and parse the whole + // input before anything is migrated or written, so a refusal stores nothing. + const format = parseEnvHydrateInputFormat(args.options.format); + const scope = parseEnvScopeOption({ scopeName: args.options.service }); + const envName = resolveRequestedEnvName({ envOption: args.options.env }); + const project = await resolveProjectForEnv({ + ctx, + pathOpt: args.options.path, + projectOpt: args.options.project, + }); + const entries = await readEnvHydrateInput({ + format, + filePath: + args.options.file === undefined + ? null + : resolve(ctx.cwd, args.options.file), + }); + + const projectName = await resolveProjectName(project); + await maybeMigrateLegacyProjectEnv({ + project, + projectName, + reason: "mutation", + }); + + const result = await hydrateProjectEnvLocalLayer({ + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: envName ?? null, + scope, + entries, + secret: args.options.secret === true, + }); + + if (args.options.json === true) { + process.stdout.write( + `${JSON.stringify( + { + file: result.filePath, + local: true, + scope: result.scope, + secret: args.options.secret === true, + changed: result.changed, + created_key: result.createdKey, + added: result.added, + updated: result.updated, + unchanged: result.unchanged, + }, + null, + 2 + )}\n` + ); + return 0; + } + + logger.success({ + message: [ + result.changed + ? `Hydrated ${result.filePath}` + : `No changes needed in ${result.filePath}`, + "worktree-local override", + `scope=${result.scope}`, + `${result.added.length} added, ${result.updated.length} updated, ${result.unchanged.length} unchanged`, + result.createdKey ? "generated project env key" : null, + ] + .filter((value): value is string => typeof value === "string") + .join(" • "), + }); + return 0; +}; + const handleEnvMaterialize: CommandHandlerFor = async ({ ctx, args, @@ -2512,6 +2630,7 @@ export const envCommand = defineCommand({ withHandler(applySpec, handleEnvApply), withHandler(addSpec, handleEnvAdd), withHandler(setSpec, handleEnvSet), + withHandler(hydrateSpec, handleEnvHydrate), withHandler(materializeSpec, handleEnvMaterialize), withHandler(execSpec, handleEnvExec), withHandler(shellSpec, handleEnvShell), diff --git a/src/lib/env-hydrate-input.ts b/src/lib/env-hydrate-input.ts new file mode 100644 index 000000000..8d6516f41 --- /dev/null +++ b/src/lib/env-hydrate-input.ts @@ -0,0 +1,417 @@ +import { constants, type FileHandle, open } from "node:fs/promises"; +import { ClosedCliError } from "./cli-result.ts"; +import type { EnvValueStdinSource } from "./env-value-stdin.ts"; + +/** + * Batch input for `hack env hydrate`: many `KEY=value` entries read from a + * non-TTY stdin or from a file, never from argv. + * + * Contract: + * - Input is bounded by {@link ENV_HYDRATE_INPUT_MAX_BYTES}, the size bound of + * one env layer. Reading stops as soon as the bound is exceeded. + * - A TTY stdin is refused before any byte is read. + * - Parsing is strict and all-or-nothing: one malformed line refuses the + * whole input, so a source that lost a value is never half-imported. + * - Every refusal is a closed, value-free {@link EnvHydrateInputError}. The + * message may carry a 1-based line number; it never carries input bytes, + * keys, values, the file path or an underlying error. + * + * `env` format, one entry per line (`\n` or `\r\n`): + * - Blank lines and lines whose first non-blank character is `#` are skipped. + * - An optional `export ` prefix is accepted. + * - `KEY=value`, where `KEY` matches `^[A-Z_][A-Z0-9_]*$` (spaces around the + * key are ignored). An unquoted value is the rest of the line with + * surrounding blanks removed; `#` inside it is literal. + * - `'...'` is literal. `"..."` accepts the escapes `\n`, `\r`, `\t`, `\"` and + * `\\`. A quoted value must close on the same line; only blanks or a + * `# comment` may follow it. + * - Empty values and repeated keys are refused. No `${VAR}` expansion. + * + * `json` format: one object whose keys match the key pattern and whose values + * are non-empty strings. + */ +export const ENV_HYDRATE_INPUT_MAX_BYTES = 1024 * 1024; + +export const ENV_HYDRATE_INPUT_FORMATS = ["env", "json"] as const; +export type EnvHydrateInputFormat = (typeof ENV_HYDRATE_INPUT_FORMATS)[number]; + +export const ENV_HYDRATE_INPUT_REFUSAL_REASONS = [ + "interactive_tty", + "invalid_format", + "oversize", + "read_failed", + "invalid_utf8", + "nul_byte", + "empty", + "invalid_line", + "invalid_key", + "duplicate_key", + "empty_value", + "unterminated_quote", + "invalid_escape", + "trailing_text", + "invalid_json", + "json_not_object", + "json_value_not_string", +] as const; + +export type EnvHydrateInputRefusalReason = + (typeof ENV_HYDRATE_INPUT_REFUSAL_REASONS)[number]; + +const NOTHING_WRITTEN = "Nothing was written."; +const NOT_SHOWN = "Input is not shown because it may contain values."; + +const REFUSAL_MESSAGES: Readonly> = + { + interactive_tty: + "hack env hydrate reads entries from piped stdin or --file, but stdin is an interactive terminal. Pipe the entries in or pass --file.", + invalid_format: 'Invalid --format value. Expected "env" or "json".', + oversize: `Hydration input exceeds the ${ENV_HYDRATE_INPUT_MAX_BYTES}-byte limit.`, + read_failed: + "Could not read the hydration input. The path is not shown because it may contain a value.", + invalid_utf8: "Hydration input is not valid UTF-8.", + nul_byte: + "Hydration input contains a NUL byte, which env values cannot carry.", + empty: "Hydration input contains no entries.", + invalid_line: "expected KEY=value.", + invalid_key: "keys must match ^[A-Z_][A-Z0-9_]*$.", + duplicate_key: "the key repeats an earlier entry.", + empty_value: "the value is empty.", + unterminated_quote: "the quoted value does not close on this line.", + invalid_escape: + 'unsupported escape in a double-quoted value (use \\n, \\r, \\t, \\" or \\\\).', + trailing_text: "unexpected text after the closing quote.", + invalid_json: "Hydration input is not valid JSON.", + json_not_object: + "JSON hydration input must be one object mapping keys to values.", + json_value_not_string: "JSON hydration values must be strings.", + }; + +/** Closed refusal for hydration input; the message never carries input. */ +export class EnvHydrateInputError extends ClosedCliError { + readonly reason: EnvHydrateInputRefusalReason; + /** 1-based line of an `env`-format refusal, or null. */ + readonly line: number | null; + + constructor( + reason: EnvHydrateInputRefusalReason, + line: number | null = null + ) { + const detail = REFUSAL_MESSAGES[reason]; + super( + line === null + ? `${detail} ${NOT_SHOWN} ${NOTHING_WRITTEN}` + : `Hydration input line ${line}: ${detail} ${NOT_SHOWN} ${NOTHING_WRITTEN}` + ); + this.name = "EnvHydrateInputError"; + this.reason = reason; + this.line = line; + } +} + +/** Fixed refusal for `--format`; the argument is never echoed. */ +export function parseEnvHydrateInputFormat( + format: string | undefined +): EnvHydrateInputFormat { + const requested = format?.trim() || "env"; + for (const candidate of ENV_HYDRATE_INPUT_FORMATS) { + if (candidate === requested) { + return candidate; + } + } + throw new EnvHydrateInputError("invalid_format"); +} + +const ENV_KEY_PATTERN = /^[A-Z_][A-Z0-9_]*$/; + +function processStdinSource(): EnvValueStdinSource { + return { + isTTY: process.stdin.isTTY === true, + open: () => Bun.stdin.stream(), + }; +} + +/** + * Read and parse one hydration input. + * + * @param input.filePath - Absolute path to read instead of stdin. Pipes and + * FIFOs (for example `--file <(producer)`) are accepted; directories are not. + * @param input.stdin - Stdin source; defaults to the process stdin. + * @returns Validated keys to values, in input order. + * @throws EnvHydrateInputError for every refusal (closed, value-free). + */ +export async function readEnvHydrateInput(input: { + readonly format: EnvHydrateInputFormat; + readonly filePath?: string | null; + readonly stdin?: EnvValueStdinSource; + readonly maxBytes?: number; +}): Promise> { + const maxBytes = input.maxBytes ?? ENV_HYDRATE_INPUT_MAX_BYTES; + const bytes = + typeof input.filePath === "string" + ? await readBoundedFile({ path: input.filePath, maxBytes }) + : await readBoundedStdin({ + source: input.stdin ?? processStdinSource(), + maxBytes, + }); + let text: string; + try { + text = new TextDecoder("utf-8", { fatal: true }).decode(bytes); + } catch { + throw new EnvHydrateInputError("invalid_utf8"); + } finally { + bytes.fill(0); + } + if (text.includes("\0")) { + throw new EnvHydrateInputError("nul_byte"); + } + return parseEnvHydrateInput({ text, format: input.format }); +} + +/** Parse decoded hydration input under the module contract. */ +export function parseEnvHydrateInput(input: { + readonly text: string; + readonly format: EnvHydrateInputFormat; +}): ReadonlyMap { + const entries = + input.format === "json" + ? parseJsonEntries(input.text) + : parseEnvEntries(input.text); + if (entries.size === 0) { + throw new EnvHydrateInputError("empty"); + } + return entries; +} + +function parseEnvEntries(text: string): Map { + const entries = new Map(); + const lines = text.split("\n"); + for (let index = 0; index < lines.length; index += 1) { + const lineNumber = index + 1; + const raw = lines[index] ?? ""; + const line = raw.endsWith("\r") ? raw.slice(0, -1) : raw; + const trimmed = line.trim(); + if (trimmed.length === 0 || trimmed.startsWith("#")) { + continue; + } + const body = trimmed.startsWith("export ") + ? trimmed.slice("export ".length).trimStart() + : trimmed; + const equals = body.indexOf("="); + if (equals <= 0) { + throw new EnvHydrateInputError("invalid_line", lineNumber); + } + const key = body.slice(0, equals).trim(); + if (!ENV_KEY_PATTERN.test(key)) { + throw new EnvHydrateInputError("invalid_key", lineNumber); + } + if (entries.has(key)) { + throw new EnvHydrateInputError("duplicate_key", lineNumber); + } + const value = parseEnvValue({ + raw: body.slice(equals + 1).trim(), + lineNumber, + }); + if (value.length === 0) { + throw new EnvHydrateInputError("empty_value", lineNumber); + } + entries.set(key, value); + } + return entries; +} + +function parseEnvValue(input: { + readonly raw: string; + readonly lineNumber: number; +}): string { + const quote = input.raw[0]; + if (quote === "'") { + const close = input.raw.indexOf("'", 1); + if (close < 0) { + throw new EnvHydrateInputError("unterminated_quote", input.lineNumber); + } + assertOnlyCommentAfter({ + rest: input.raw.slice(close + 1), + lineNumber: input.lineNumber, + }); + return input.raw.slice(1, close); + } + if (quote === '"') { + return parseDoubleQuoted(input); + } + return input.raw; +} + +const DOUBLE_QUOTE_ESCAPES: Readonly> = { + n: "\n", + r: "\r", + t: "\t", + '"': '"', + "\\": "\\", +}; + +function parseDoubleQuoted(input: { + readonly raw: string; + readonly lineNumber: number; +}): string { + let value = ""; + for (let index = 1; index < input.raw.length; index += 1) { + const char = input.raw[index]; + if (char === '"') { + assertOnlyCommentAfter({ + rest: input.raw.slice(index + 1), + lineNumber: input.lineNumber, + }); + return value; + } + if (char === "\\") { + const escaped = DOUBLE_QUOTE_ESCAPES[input.raw[index + 1] ?? ""]; + if (escaped === undefined) { + throw new EnvHydrateInputError("invalid_escape", input.lineNumber); + } + value += escaped; + index += 1; + continue; + } + value += char; + } + throw new EnvHydrateInputError("unterminated_quote", input.lineNumber); +} + +function assertOnlyCommentAfter(input: { + readonly rest: string; + readonly lineNumber: number; +}): void { + const rest = input.rest.trim(); + if (rest.length > 0 && !rest.startsWith("#")) { + throw new EnvHydrateInputError("trailing_text", input.lineNumber); + } +} + +function parseJsonEntries(text: string): Map { + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + throw new EnvHydrateInputError("invalid_json"); + } + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new EnvHydrateInputError("json_not_object"); + } + const entries = new Map(); + for (const [key, value] of Object.entries(parsed)) { + if (!ENV_KEY_PATTERN.test(key)) { + throw new EnvHydrateInputError("invalid_key"); + } + if (typeof value !== "string") { + throw new EnvHydrateInputError("json_value_not_string"); + } + if (value.length === 0) { + throw new EnvHydrateInputError("empty_value"); + } + entries.set(key, value); + } + return entries; +} + +async function readBoundedStdin(input: { + readonly source: EnvValueStdinSource; + readonly maxBytes: number; +}): Promise { + if (input.source.isTTY) { + throw new EnvHydrateInputError("interactive_tty"); + } + const chunks: Uint8Array[] = []; + let total = 0; + let reader: ReadableStreamDefaultReader; + try { + reader = input.source.open().getReader(); + } catch { + throw new EnvHydrateInputError("read_failed"); + } + try { + while (true) { + let result: Awaited>; + try { + result = await reader.read(); + } catch { + wipe(chunks); + throw new EnvHydrateInputError("read_failed"); + } + if (result.done) { + break; + } + total += result.value.byteLength; + chunks.push(result.value); + if (total > input.maxBytes) { + wipe(chunks); + await reader.cancel().catch(() => undefined); + throw new EnvHydrateInputError("oversize"); + } + } + } finally { + reader.releaseLock(); + } + return concat(chunks, total); +} + +async function readBoundedFile(input: { + readonly path: string; + readonly maxBytes: number; +}): Promise { + let file: FileHandle; + try { + file = await open(input.path, constants.O_RDONLY); + } catch { + throw new EnvHydrateInputError("read_failed"); + } + const chunks: Uint8Array[] = []; + let total = 0; + try { + const stats = await file.stat().catch(() => null); + if (stats === null || stats.isDirectory()) { + throw new EnvHydrateInputError("read_failed"); + } + while (true) { + const chunk = new Uint8Array(64 * 1024); + let bytesRead: number; + try { + ({ bytesRead } = await file.read(chunk, 0, chunk.byteLength, null)); + } catch { + chunk.fill(0); + wipe(chunks); + throw new EnvHydrateInputError("read_failed"); + } + if (bytesRead === 0) { + break; + } + total += bytesRead; + chunks.push(chunk.subarray(0, bytesRead)); + if (total > input.maxBytes) { + chunk.fill(0); + wipe(chunks); + throw new EnvHydrateInputError("oversize"); + } + } + } finally { + await file.close().catch(() => undefined); + } + return concat(chunks, total); +} + +function concat(chunks: readonly Uint8Array[], total: number): Uint8Array { + const out = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + out.set(chunk, offset); + offset += chunk.byteLength; + } + wipe(chunks); + return out; +} + +function wipe(chunks: readonly Uint8Array[]): void { + for (const chunk of chunks) { + chunk.fill(0); + } +} diff --git a/src/lib/fs.ts b/src/lib/fs.ts index 96558b275..4224aec98 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -101,6 +101,14 @@ export async function writeTextFileIfChanged( return { changed: true }; } +/** A compare-and-swap write found the file changed by another writer. */ +export class AtomicWriteConflictError extends Error { + constructor() { + super("The file changed while it was being replaced; nothing was written."); + this.name = "AtomicWriteConflictError"; + } +} + /** * Replaces `absolutePath` with `content` only when the text differs, publishing * the new contents atomically. @@ -119,12 +127,24 @@ export async function writeTextFileIfChanged( * a plain write through the link would. A new file gets the umask default. * Unlike an in-place write, a dangling symlink is replaced by a regular file, * and the containing directory must be writable. + * + * With `expectedExisting`, the write is a compare-and-swap: the current text + * (null when absent) must equal it both before staging and again after the + * staging file is fsynced, immediately before the rename; otherwise the + * staging file is removed and {@link AtomicWriteConflictError} is thrown. The + * second check narrows, but cannot close, the window in which a writer that + * does not use this check can still be overwritten. */ export async function writeTextFileAtomicIfChanged( absolutePath: string, - content: string + content: string, + options: { readonly expectedExisting?: string | null } = {} ): Promise<{ readonly changed: boolean }> { const existing = await readTextFile(absolutePath); + const expectsExisting = options.expectedExisting !== undefined; + if (expectsExisting && existing !== options.expectedExisting) { + throw new AtomicWriteConflictError(); + } if (existing === content) { return { changed: false }; } @@ -150,6 +170,12 @@ export async function writeTextFileAtomicIfChanged( } await file.writeFile(content, "utf8"); await file.sync(); + if ( + expectsExisting && + (await readTextFile(absolutePath)) !== options.expectedExisting + ) { + throw new AtomicWriteConflictError(); + } await rename(staging, target); committed = true; } finally { diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 2f533b849..566272aa7 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -4,7 +4,7 @@ import { createHash, randomBytes, } from "node:crypto"; -import { lstat, readdir, rm, stat } from "node:fs/promises"; +import { lstat, readdir, rm, stat, unlink } from "node:fs/promises"; import { basename, dirname, relative, resolve } from "node:path"; import { YAML } from "bun"; import { @@ -25,8 +25,10 @@ import { HACK_DIR_GITIGNORE_END_MARKER, HACK_DIR_GITIGNORE_ENTRIES, } from "../templates.ts"; +import { ClosedCliError } from "./cli-result.ts"; import { parseDotEnv, serializeDotEnv } from "./env.ts"; import { + AtomicWriteConflictError, ensureDir, ensureGitignoreEntry, ensureManagedGitignoreBlock, @@ -649,6 +651,19 @@ async function readProjectEnvConfigFile(opts: { readonly strictRead?: boolean; }): Promise { const text = await readProjectEnvLayerText(opts); + return parseProjectEnvConfigText({ + path: opts.path, + environment: opts.environment, + text, + }); +} + +function parseProjectEnvConfigText(opts: { + readonly path: string; + readonly environment: string; + readonly text: string | null; +}): ProjectEnvConfigReadResult { + const text = opts.text; if (text === null) { return { path: opts.path, @@ -2470,15 +2485,506 @@ export async function unsetProjectEnvValue(opts: { return { changed, filePath }; } +/** Closed reasons `hydrateProjectEnvLocalLayer` refuses with; none writes a layer. */ +export const PROJECT_ENV_HYDRATION_REFUSAL_REASONS = [ + "tracked_layer", + "locked", + "stale_lock", + "unreadable_layer", + "wrong_key", + "key_env_only", + "concurrent_write", +] as const; + +export type ProjectEnvHydrationRefusalReason = + (typeof PROJECT_ENV_HYDRATION_REFUSAL_REASONS)[number]; + +const PROJECT_ENV_HYDRATION_LOCK_FILENAME = "env-hydrate.lock"; +const PROJECT_ENV_HYDRATION_NOTHING_WRITTEN = "No env layer was changed."; + +function projectEnvHydrationRefusalMessage(opts: { + readonly reason: ProjectEnvHydrationRefusalReason; + readonly lockPath: string; +}): string { + switch (opts.reason) { + case "tracked_layer": + return "Refusing to hydrate: git tracks the machine-local env layer, so hydrated values would be committed. Untrack it with `git rm --cached` and retry."; + case "locked": + return "Refusing to hydrate: another `hack env hydrate` is writing this project's machine-local env layer. Retry when it finishes."; + case "stale_lock": + return `Refusing to hydrate: an interrupted \`hack env hydrate\` left its lock at ${opts.lockPath}, and the process that held it has exited. Remove that file and retry.`; + case "unreadable_layer": + return "Refusing to hydrate: the machine-local env layer is not a readable, valid env file. Fix or remove it and retry."; + case "wrong_key": + return `Refusing to hydrate: a secret already in the machine-local env layer does not decrypt under the project env key, or no key is available. Restore the matching ${PROJECT_ENV_KEY_FILENAME} and retry.`; + case "key_env_only": + return `Refusing to hydrate secrets: ${PROJECT_ENV_SECRET_KEY_ENV} is set but no ${PROJECT_ENV_KEY_FILENAME} exists, and creating one would replace that key for every layer. Store the same key in ${PROJECT_ENV_KEY_FILENAME} and retry.`; + case "concurrent_write": + return "Refusing to hydrate: the machine-local env layer changed while hydration was preparing it. Re-run hydration."; + default: + return "Refusing to hydrate."; + } +} + +/** + * Closed, value-free refusal from `hydrateProjectEnvLocalLayer`. The message + * names no key, value or input line; it may name Hack's own lock path. + */ +export class ProjectEnvHydrationRefusalError extends ClosedCliError { + readonly reason: ProjectEnvHydrationRefusalReason; + + constructor(opts: { + readonly reason: ProjectEnvHydrationRefusalReason; + readonly lockPath: string; + }) { + super( + `${projectEnvHydrationRefusalMessage(opts)} ${PROJECT_ENV_HYDRATION_NOTHING_WRITTEN}` + ); + this.name = "ProjectEnvHydrationRefusalError"; + this.reason = opts.reason; + } +} + +export type ProjectEnvHydrationResult = { + readonly filePath: string; + readonly scope: string; + readonly changed: boolean; + readonly createdKey: boolean; + /** Imported keys that the layer's scope did not hold. Sorted. */ + readonly added: readonly string[]; + /** Imported keys whose stored entry was replaced. Sorted. */ + readonly updated: readonly string[]; + /** Imported keys whose stored entry already held the value. Sorted. */ + readonly unchanged: readonly string[]; +}; + +/** + * Writes several values into the machine-local env layer in one commit. + * + * Contract: + * - Only the effective machine-local layer for `envName` is written + * (`hack.env.local.yaml` or `hack.env..local.yaml`); a layer that + * git tracks is refused. Tracked defaults are never read for writing. + * - Keys outside `entries`, other scopes and removal markers for other keys + * are kept as they are. An imported key replaces that key's local entry. + * - Idempotent: an entry that already holds its value is kept unchanged (a + * secret keeps its ciphertext, a scalar keeps its YAML type). When no entry + * changes, the file is not rewritten at all. A changed or newly secret value + * gets new ciphertext with a fresh IV. + * - All-or-nothing: the whole new layer is published by one rename. Every + * refusal ({@link ProjectEnvHydrationRefusalError}) happens before that + * rename, so the previous layer stays in place. + * - Concurrency: hydrations of one project are serialized by an exclusive + * lock file under `.internal/`; a second hydration refuses instead of + * waiting. The write is also a compare-and-swap against the bytes read, so a + * different writer that changed the layer meanwhile makes hydration refuse + * rather than overwrite it. A lock left by a killed process is reported, not + * removed. + * - Wrong key: every secret already in the layer must decrypt under the key + * readers would use, or hydration refuses. Secret mode encrypts with the key + * file `hack env add --secret` uses and refuses to create one while only + * `HACK_ENV_SECRET_KEY` provides a key. + * + * Values never appear in errors or results; results name only keys. + */ +export async function hydrateProjectEnvLocalLayer(opts: { + readonly projectRoot: string; + readonly projectDir: string; + readonly envName: string | null; + readonly scope: string; + /** Validated keys to plaintext values. */ + readonly entries: ReadonlyMap; + readonly secret: boolean; +}): Promise { + await assertLegacyProjectDirectory({ projectDir: opts.projectDir }); + await assertLegacyProjectInputFamily({ projectRoot: opts.projectRoot }); + for (const key of opts.entries.keys()) { + if (!PROJECT_ENV_KEY_PATTERN.test(key)) { + throw new Error(ENV_KEY_INVALID_MESSAGE); + } + } + const scope = assertValidProjectEnvScopeName({ scopeName: opts.scope }); + const filePath = await resolveProjectEnvEffectiveLocalConfigPath({ + projectRoot: opts.projectRoot, + projectDir: opts.projectDir, + envName: opts.envName, + }); + const lockPath = resolve( + opts.projectDir, + ".internal", + PROJECT_ENV_HYDRATION_LOCK_FILENAME + ); + const refuse = (reason: ProjectEnvHydrationRefusalReason) => + new ProjectEnvHydrationRefusalError({ reason, lockPath }); + + const tracked = await isGitTrackedProjectEnvPath({ + projectRoot: opts.projectRoot, + path: filePath, + }); + if (tracked === true) { + throw refuse("tracked_layer"); + } + + const release = await acquireProjectEnvHydrationLock({ + projectDir: opts.projectDir, + lockPath, + refuse, + }); + try { + return await hydrateLockedProjectEnvLayer({ + ...opts, + scope, + filePath, + refuse, + }); + } finally { + await release(); + } +} + +type ProjectEnvHydrationRefuse = ( + reason: ProjectEnvHydrationRefusalReason +) => ProjectEnvHydrationRefusalError; + +async function hydrateLockedProjectEnvLayer(opts: { + readonly projectRoot: string; + readonly projectDir: string; + readonly envName: string | null; + readonly scope: string; + readonly entries: ReadonlyMap; + readonly secret: boolean; + readonly filePath: string; + readonly refuse: ProjectEnvHydrationRefuse; +}): Promise { + const environment = opts.envName ?? "default"; + const layer = await readProjectEnvHydrationLayer({ + filePath: opts.filePath, + environment, + refuse: opts.refuse, + }); + const key = await resolveProjectEnvHydrationKey({ + projectRoot: opts.projectRoot, + config: layer.config, + secret: opts.secret, + refuse: opts.refuse, + }); + const plan = planProjectEnvHydration({ + previousScope: layer.config.values[opts.scope] ?? {}, + entries: opts.entries, + keyText: key.keyText, + }); + const result = { + filePath: opts.filePath, + scope: opts.scope, + createdKey: key.createdKey, + added: plan.added, + updated: plan.updated, + unchanged: plan.unchanged, + }; + if (plan.added.length === 0 && plan.updated.length === 0) { + // Nothing to change: leave the layer's bytes, including hand edits, alone. + return { ...result, changed: false }; + } + + const nextValues: ProjectEnvValuesByScope = { + ...layer.config.values, + [opts.scope]: plan.nextScope, + }; + if (!("global" in nextValues)) { + nextValues.global = {}; + } + await ensureHackDirGitignore({ projectDir: opts.projectDir }); + try { + await writeProjectEnvConfigFile({ + path: opts.filePath, + config: { + version: PROJECT_ENV_CONFIG_VERSION, + environment, + secretsprovider: PROJECT_ENV_SECRETS_PROVIDER, + values: nextValues, + }, + expectedExisting: layer.text, + }); + } catch (error: unknown) { + if (error instanceof AtomicWriteConflictError) { + throw opts.refuse("concurrent_write"); + } + throw error; + } + return { ...result, changed: true }; +} + +/** The layer's exact bytes (null when absent) and its parsed config. */ +async function readProjectEnvHydrationLayer(opts: { + readonly filePath: string; + readonly environment: string; + readonly refuse: ProjectEnvHydrationRefuse; +}): Promise<{ + readonly text: string | null; + readonly config: ProjectEnvConfig; +}> { + let text: string | null; + try { + text = await readProjectEnvLayerText({ + path: opts.filePath, + strictRead: true, + }); + } catch { + throw opts.refuse("unreadable_layer"); + } + const read = parseProjectEnvConfigText({ + path: opts.filePath, + environment: opts.environment, + text, + }); + if (read.parseError !== undefined) { + throw opts.refuse("unreadable_layer"); + } + return { text, config: read.config }; +} + +/** + * Checks every secret already in the layer against the key readers use and, + * in secret mode, selects (or first creates) the encryption key. + */ +async function resolveProjectEnvHydrationKey(opts: { + readonly projectRoot: string; + readonly config: ProjectEnvConfig; + readonly secret: boolean; + readonly refuse: ProjectEnvHydrationRefuse; +}): Promise<{ readonly keyText: string | null; readonly createdKey: boolean }> { + const fileKey = await readExistingProjectEnvKeyFileText({ + projectRoot: opts.projectRoot, + }); + const environmentKey = + process.env[PROJECT_ENV_SECRET_KEY_ENV]?.trim() || null; + // The same priority `resolveProjectEnvKey` gives readers of this layer. + const readerKey = fileKey ?? environmentKey; + const existingSecrets = Object.values(opts.config.values).flatMap( + (scopeValues) => + Object.values(scopeValues).filter( + (value): value is ProjectEnvSecretValue => + value !== null && isProjectEnvSecretValue(value) + ) + ); + const opensAll = + readerKey !== null && + existingSecrets.every((secret) => + canDecryptProjectEnvValue({ + ciphertext: secret.secure, + keyText: readerKey, + }) + ); + if (existingSecrets.length > 0 && !opensAll) { + throw opts.refuse("wrong_key"); + } + if (!opts.secret) { + return { keyText: null, createdKey: false }; + } + if (fileKey === null && readerKey !== null) { + throw opts.refuse("key_env_only"); + } + const ensured = await ensureProjectEnvSecretKey({ + projectRoot: opts.projectRoot, + }); + if (readerKey !== null && ensured.keyText !== readerKey) { + throw opts.refuse("wrong_key"); + } + return { keyText: ensured.keyText, createdKey: ensured.created }; +} + +/** Pure merge of imported entries into one scope; keys are reported sorted. */ +function planProjectEnvHydration(opts: { + readonly previousScope: Readonly>; + readonly entries: ReadonlyMap; + /** Encrypts every imported value when set; stores plain values when null. */ + readonly keyText: string | null; +}) { + const nextScope: Record = { + ...opts.previousScope, + }; + const added: string[] = []; + const updated: string[] = []; + const unchanged: string[] = []; + const keys = [...opts.entries.keys()].sort((left, right) => + left.localeCompare(right) + ); + for (const key of keys) { + const plaintext = opts.entries.get(key) ?? ""; + const previous = Object.hasOwn(opts.previousScope, key) + ? opts.previousScope[key] + : undefined; + const stored: ProjectEnvStoredValue = + opts.keyText === null + ? stableProjectEnvPlainValue({ previous, plaintext }) + : stableProjectEnvSecretValue({ + previous, + plaintext, + keyText: opts.keyText, + }); + if (sameProjectEnvStoredValue(previous, stored)) { + unchanged.push(key); + continue; + } + (previous === undefined ? added : updated).push(key); + nextScope[key] = stored; + } + return { nextScope, added, updated, unchanged }; +} + +/** A plain scalar that already prints as `plaintext` keeps its YAML type. */ +function stableProjectEnvPlainValue(opts: { + readonly previous: ProjectEnvStoredValue | undefined; + readonly plaintext: string; +}): ProjectEnvStoredValue { + const previous = opts.previous; + if ( + (typeof previous === "string" || + typeof previous === "number" || + typeof previous === "boolean") && + String(previous) === opts.plaintext + ) { + return previous; + } + return opts.plaintext; +} + +function sameProjectEnvStoredValue( + previous: ProjectEnvStoredValue | undefined, + next: ProjectEnvStoredValue +): boolean { + if (previous === undefined || previous === null || next === null) { + return previous === next; + } + if (isProjectEnvSecretValue(previous) || isProjectEnvSecretValue(next)) { + return ( + isProjectEnvSecretValue(previous) && + isProjectEnvSecretValue(next) && + previous.secure === next.secure + ); + } + return previous === next; +} + +function canDecryptProjectEnvValue(opts: { + readonly ciphertext: string; + readonly keyText: string; +}): boolean { + try { + decryptProjectEnvValue(opts); + return true; + } catch { + return false; + } +} + +/** + * The key file `ensureProjectEnvSecretKey` would select (checkout-local, + * existing shared, then the primary checkout's), read without creating, + * copying or adopting anything. + */ +async function readExistingProjectEnvKeyFileText(opts: { + readonly projectRoot: string; +}): Promise { + const localKey = await readLegacyProjectEnvKeyFile({ + path: resolveProjectEnvKeyPath({ projectRoot: opts.projectRoot }), + }); + if (localKey !== null) { + return localKey; + } + const sharedKeyPath = await resolveProjectEnvSharedKeyPath({ + projectRoot: opts.projectRoot, + }); + const sharedKey = + sharedKeyPath === null + ? null + : await readLegacyProjectEnvKeyFile({ path: sharedKeyPath }); + if (sharedKey !== null) { + return sharedKey; + } + const inherited = await resolveInheritedProjectEnvKey({ + projectRoot: opts.projectRoot, + }); + return inherited?.keyText ?? null; +} + +/** + * Takes the project's hydration lock: `.internal/env-hydrate.lock`, created + * exclusively (no symlink follow, mode 0600) and holding the owner's PID. + * + * An existing lock is never removed or taken over, because a second process + * cannot prove the first one is gone without racing a third. It refuses as + * `locked` while the recorded PID is alive (or unreadable, which includes a + * lock still being written) and as `stale_lock` once that process has exited. + * The returned release removes the lock only while the path still names the + * file this call created. + */ +async function acquireProjectEnvHydrationLock(opts: { + readonly projectDir: string; + readonly lockPath: string; + readonly refuse: ProjectEnvHydrationRefuse; +}): Promise<() => Promise> { + await ensureDir(dirname(opts.lockPath)); + await assertLegacyProjectDirectory({ projectDir: opts.projectDir }); + try { + await createPrivateFileExclusive({ + path: opts.lockPath, + text: `${process.pid}\n`, + }); + } catch (error: unknown) { + if (!(isRecord(error) && error.code === "EEXIST")) { + throw error; + } + const holder = await readTextFile(opts.lockPath); + const pid = Number.parseInt(holder?.trim() ?? "", 10); + throw opts.refuse( + Number.isSafeInteger(pid) && pid > 0 && !isProcessAlive(pid) + ? "stale_lock" + : "locked" + ); + } + const created = await lstat(opts.lockPath); + return async () => { + const current = await lstat(opts.lockPath).catch(() => null); + if ( + current?.isFile() && + current.dev === created.dev && + current.ino === created.ino + ) { + await unlink(opts.lockPath).catch(() => undefined); + } + }; +} + +/** Signal 0 probes existence only; EPERM means a live process of another user. */ +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error: unknown) { + return isRecord(error) && error.code === "EPERM"; + } +} + async function writeProjectEnvConfigFile(opts: { readonly path: string; readonly config: ProjectEnvConfig; + /** Compare-and-swap guard; see `writeTextFileAtomicIfChanged`. */ + readonly expectedExisting?: string | null; }): Promise { const yaml = YAML.stringify(opts.config, null, 2); const text = yaml.endsWith("\n") ? yaml : `${yaml}\n`; await assertLegacyProjectDirectory({ projectDir: dirname(opts.path) }); // Rename-committed: an interrupted mutation leaves the previous layer intact. - return (await writeTextFileAtomicIfChanged(opts.path, text)).changed; + return ( + await writeTextFileAtomicIfChanged( + opts.path, + text, + opts.expectedExisting === undefined + ? {} + : { expectedExisting: opts.expectedExisting } + ) + ).changed; } export async function materializeProjectEnv(opts: { diff --git a/tests/cli-command.test.ts b/tests/cli-command.test.ts index e63a10ae4..32ef8c017 100644 --- a/tests/cli-command.test.ts +++ b/tests/cli-command.test.ts @@ -48,6 +48,15 @@ test("parsePositionalsForCommand throws on extra args", () => { ).toThrow("Unexpected arguments"); }); +test("parsePositionalsForCommand never echoes the extra arguments", () => { + // An extra argument is often a pasted `KEY=value`; only the count is shown. + expect(() => + parsePositionalsForCommand([], ["API_TOKEN=synthetic-extra-4c2d", "more"]) + ).toThrow( + "Unexpected arguments: 2 more than this command accepts. They are not shown because they may contain values." + ); +}); + test("parseOptionsForCommand converts number options", () => { const opts = [ { diff --git a/tests/env-hydrate-input.test.ts b/tests/env-hydrate-input.test.ts new file mode 100644 index 000000000..2eee54328 --- /dev/null +++ b/tests/env-hydrate-input.test.ts @@ -0,0 +1,233 @@ +import { afterEach, expect, test } from "bun:test"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { + EnvHydrateInputError, + type EnvHydrateInputRefusalReason, + parseEnvHydrateInput, + parseEnvHydrateInputFormat, + readEnvHydrateInput, +} from "../src/lib/env-hydrate-input.ts"; + +/** + * HACK-1158: hydration input is parsed strictly and all-or-nothing, and every + * refusal is value-free. CANARY is a synthetic string that must never reach + * an error message. + */ + +const CANARY = "synthetic-canary-7d41"; + +const tempDirs = new Set(); + +afterEach(async () => { + for (const dir of tempDirs) { + await rm(dir, { recursive: true, force: true }); + } + tempDirs.clear(); +}); + +function entriesOf(text: string, format: "env" | "json" = "env") { + return Object.fromEntries(parseEnvHydrateInput({ text, format })); +} + +function refusal(run: () => unknown): { + reason: EnvHydrateInputRefusalReason; + line: number | null; +} { + try { + run(); + } catch (error: unknown) { + if (!(error instanceof EnvHydrateInputError)) { + throw error; + } + expect(error.message).not.toContain(CANARY); + expect(error.message).toContain("Nothing was written."); + return { reason: error.reason, line: error.line }; + } + throw new Error("Expected a hydration input refusal"); +} + +async function asyncRefusal( + run: () => Promise +): Promise { + try { + await run(); + } catch (error: unknown) { + if (!(error instanceof EnvHydrateInputError)) { + throw error; + } + expect(error.message).not.toContain(CANARY); + return error.reason; + } + throw new Error("Expected a hydration input refusal"); +} + +function stdinOf(bytes: Uint8Array, isTTY = false) { + return { + isTTY, + open: () => + new ReadableStream({ + start(controller) { + controller.enqueue(bytes); + controller.close(); + }, + }), + }; +} + +test("env format accepts comments, export, quotes and literal #", () => { + expect( + entriesOf( + [ + "# source: synthetic fixture", + "", + "API_TOKEN=plain-value", + " export DB_PASS = 'single # quoted' # trailing comment", + 'MULTI="line\\none\\t\\"q\\" \\\\"', + "HASH=abc#def", + "SPACED= inner value ", + "CRLF=value\r", + ].join("\n") + ) + ).toEqual({ + API_TOKEN: "plain-value", + DB_PASS: "single # quoted", + MULTI: 'line\none\t"q" \\', + HASH: "abc#def", + SPACED: "inner value", + CRLF: "value", + }); +}); + +test("json format accepts one object of string values", () => { + expect( + entriesOf(JSON.stringify({ API_TOKEN: CANARY, DB_PASS: "b" }), "json") + ).toEqual({ API_TOKEN: CANARY, DB_PASS: "b" }); +}); + +test("each malformed env line refuses the whole input with its line number", () => { + const cases: readonly [string, EnvHydrateInputRefusalReason, number][] = [ + [`OK=1\n${CANARY}`, "invalid_line", 2], + [`OK=1\n=${CANARY}`, "invalid_line", 2], + [`lower_${CANARY}=x`, "invalid_key", 1], + [`API_TOKEN=${CANARY}\nAPI_TOKEN=${CANARY}`, "duplicate_key", 2], + ["API_TOKEN=", "empty_value", 1], + ["API_TOKEN=''", "empty_value", 1], + [`API_TOKEN='${CANARY}`, "unterminated_quote", 1], + [`API_TOKEN="${CANARY}\nrest"`, "unterminated_quote", 1], + [`API_TOKEN="${CANARY}\\x"`, "invalid_escape", 1], + [`API_TOKEN='${CANARY}' ${CANARY}`, "trailing_text", 1], + ]; + for (const [text, reason, line] of cases) { + expect(refusal(() => entriesOf(text))).toEqual({ reason, line }); + } +}); + +test("empty input and malformed json are refused without echo", () => { + expect(refusal(() => entriesOf("# only comments\n\n")).reason).toBe("empty"); + const jsonCases: readonly [string, EnvHydrateInputRefusalReason][] = [ + [`{"API_TOKEN": ${CANARY}}`, "invalid_json"], + [`["${CANARY}"]`, "json_not_object"], + [`"${CANARY}"`, "json_not_object"], + [`{"bad-${CANARY}": "x"}`, "invalid_key"], + ['{"API_TOKEN": 7}', "json_value_not_string"], + ['{"API_TOKEN": ""}', "empty_value"], + ["{}", "empty"], + ]; + for (const [text, reason] of jsonCases) { + expect(refusal(() => entriesOf(text, "json")).reason).toBe(reason); + } +}); + +test("--format accepts only env and json and never echoes the argument", () => { + expect(parseEnvHydrateInputFormat(undefined)).toBe("env"); + expect(parseEnvHydrateInputFormat("json")).toBe("json"); + expect(refusal(() => parseEnvHydrateInputFormat(CANARY)).reason).toBe( + "invalid_format" + ); +}); + +test("stdin input is bounded and refuses TTY, NUL and invalid UTF-8", async () => { + const encoder = new TextEncoder(); + expect( + Object.fromEntries( + await readEnvHydrateInput({ + format: "env", + stdin: stdinOf(encoder.encode(`API_TOKEN=${CANARY}\n`)), + }) + ) + ).toEqual({ API_TOKEN: CANARY }); + let opened = false; + expect( + await asyncRefusal(() => + readEnvHydrateInput({ + format: "env", + stdin: { + isTTY: true, + open: () => { + opened = true; + throw new Error("must not read a TTY"); + }, + }, + }) + ) + ).toBe("interactive_tty"); + expect(opened).toBe(false); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ + format: "env", + stdin: stdinOf(encoder.encode(`API_TOKEN=${CANARY}`)), + maxBytes: 8, + }) + ) + ).toBe("oversize"); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ + format: "env", + stdin: stdinOf(encoder.encode(`API_TOKEN=${CANARY}\0`)), + }) + ) + ).toBe("nul_byte"); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ + format: "env", + stdin: stdinOf(new Uint8Array([0x41, 0x3d, 0xff, 0xfe])), + }) + ) + ).toBe("invalid_utf8"); +}); + +test("file input reads regular files and refuses directories and missing paths", async () => { + const dir = await mkdtemp(join(tmpdir(), "hack-hydrate-input-")); + tempDirs.add(dir); + const file = join(dir, "entries.env"); + await writeFile(file, `API_TOKEN=${CANARY}\n`); + expect( + Object.fromEntries( + await readEnvHydrateInput({ format: "env", filePath: file }) + ) + ).toEqual({ API_TOKEN: CANARY }); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ format: "env", filePath: dir }) + ) + ).toBe("read_failed"); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ + format: "env", + filePath: join(dir, `missing-${CANARY}`), + }) + ) + ).toBe("read_failed"); + expect( + await asyncRefusal(() => + readEnvHydrateInput({ format: "env", filePath: file, maxBytes: 4 }) + ) + ).toBe("oversize"); +}); diff --git a/tests/fixtures/project-env-hydrate-worker.ts b/tests/fixtures/project-env-hydrate-worker.ts new file mode 100644 index 000000000..a8cfd35b2 --- /dev/null +++ b/tests/fixtures/project-env-hydrate-worker.ts @@ -0,0 +1,84 @@ +import { mock } from "bun:test"; +import { existsSync } from "node:fs"; +import * as fs from "node:fs/promises"; + +/** + * Runs one `hydrateProjectEnvLocalLayer` secret import in a disposable child + * and pauses it at a chosen point of the layer write. Everything before the + * pause runs unmocked. + * + * - `rename`: pause at the commit rename, after the staging file is written + * and fsynced. The parent SIGKILLs the child to test a real interruption. + * - `staging`: pause when the staging file is opened, before the + * compare-and-swap recheck, until `/go` exists. The parent + * changes the layer meanwhile to test a concurrent writer. + * + * Entries arrive on stdin as `KEY=value` lines of synthetic test strings, + * never in argv. The child prints `prepared`, then `committed` or + * `refused:`. + */ +const [projectRoot, projectDir, pauseAt] = process.argv.slice(2); +if ( + !( + projectRoot && + projectDir && + (pauseAt === "rename" || pauseAt === "staging") + ) +) { + throw new Error("Missing project env hydrate worker arguments"); +} + +const rename = fs.rename; +const open = fs.open; +mock.module("node:fs/promises", () => ({ + ...fs, + rename: async (from: string, to: string) => { + if (pauseAt === "rename" && from.endsWith(".tmp")) { + process.stdout.write("prepared\n"); + // Hold the staged write open until the parent kills this process. + await Bun.sleep(120_000); + } + return await rename(from, to); + }, + open: async (...args: Parameters) => { + const [path] = args; + if ( + pauseAt === "staging" && + typeof path === "string" && + path.endsWith(".tmp") + ) { + process.stdout.write("prepared\n"); + while (!existsSync(`${projectDir}/go`)) { + await Bun.sleep(10); + } + } + return await open(...args); + }, +})); + +const entries = new Map(); +for (const line of (await Bun.stdin.text()).split("\n")) { + const equals = line.indexOf("="); + if (equals > 0) { + entries.set(line.slice(0, equals), line.slice(equals + 1)); + } +} + +const { hydrateProjectEnvLocalLayer, ProjectEnvHydrationRefusalError } = + await import("../../src/lib/project-env-config.ts"); +try { + await hydrateProjectEnvLocalLayer({ + projectRoot, + projectDir, + envName: null, + scope: "global", + entries, + secret: true, + }); + process.stdout.write("committed\n"); +} catch (error: unknown) { + if (!(error instanceof ProjectEnvHydrationRefusalError)) { + throw error; + } + process.stdout.write(`refused:${error.reason}\n`); +} diff --git a/tests/project-env-hydrate.test.ts b/tests/project-env-hydrate.test.ts new file mode 100644 index 000000000..34a390952 --- /dev/null +++ b/tests/project-env-hydrate.test.ts @@ -0,0 +1,692 @@ +import { afterEach, expect, test } from "bun:test"; +import { + mkdir, + mkdtemp, + readdir, + readFile, + rm, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { YAML } from "bun"; + +import { + PROJECT_COMPOSE_FILENAME, + PROJECT_CONFIG_FILENAME, + PROJECT_ENV_KEY_FILENAME, +} from "../src/constants.ts"; +import { + AtomicWriteConflictError, + writeTextFileAtomicIfChanged, +} from "../src/lib/fs.ts"; +import { + ensureHackDirGitignore, + hydrateProjectEnvLocalLayer, + ProjectEnvHydrationRefusalError, + type ProjectEnvHydrationRefusalReason, + resolveProjectEnvConfig, + resolveProjectEnvConfigPath, + resolveProjectEnvLocalConfigPath, + setProjectEnvValue, +} from "../src/lib/project-env-config.ts"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * HACK-1158: `hack env hydrate` writes many values into the machine-local env + * layer in one commit. Repeats are byte-identical, changed values rotate with + * a fresh IV, other local entries survive, and a wrong key, an interrupted + * write or a concurrent writer leaves the previous layer in place. Every value + * here is a synthetic test string; every child gets a private HOME. + */ + +const REPO_ROOT = resolve(import.meta.dir, ".."); +const ENTRYPOINT = resolve(REPO_ROOT, "index.ts"); +const WORKER = resolve( + import.meta.dir, + "fixtures/project-env-hydrate-worker.ts" +); +const TOKEN = "synthetic-token-3b9e"; +const PASSWORD = "synthetic-password-c51a"; +const CHANGED = "synthetic-changed-80f2"; +const CANARIES = [TOKEN, PASSWORD, CHANGED] as const; + +const tempDirs = new Set(); + +afterEach(async () => { + for (const tempDir of tempDirs) { + await rm(tempDir, { recursive: true, force: true }); + } + tempDirs.clear(); +}); + +type Repo = { + readonly root: string; + readonly projectRoot: string; + readonly projectDir: string; + readonly trackedFile: string; + readonly localFile: string; +}; + +async function createRepo(): Promise { + const root = await mkdtemp(join(tmpdir(), "hack-env-hydrate-")); + tempDirs.add(root); + const projectRoot = resolve(root, "repo"); + const projectDir = resolve(projectRoot, ".hack"); + await mkdir(projectDir, { recursive: true }); + await writeFile( + resolve(projectDir, PROJECT_COMPOSE_FILENAME), + "services:\n api:\n image: alpine:3.20\n" + ); + await writeFile( + resolve(projectDir, PROJECT_CONFIG_FILENAME), + `${JSON.stringify({ name: "env-hydrate", dev_host: "env-hydrate.hack" }, null, 2)}\n` + ); + await writeFile( + resolve(projectRoot, ".gitignore"), + `${PROJECT_ENV_KEY_FILENAME}\n` + ); + return { + root, + projectRoot, + projectDir, + trackedFile: resolveProjectEnvConfigPath({ projectDir, envName: null }), + localFile: resolveProjectEnvLocalConfigPath({ projectDir, envName: null }), + }; +} + +async function hydrate( + repo: Repo, + entries: Record, + secret = true, + envName: string | null = null +) { + return await hydrateProjectEnvLocalLayer({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName, + scope: "global", + entries: new Map(Object.entries(entries)), + secret, + }); +} + +async function refusalOf( + run: () => Promise +): Promise { + try { + await run(); + } catch (error: unknown) { + if (!(error instanceof ProjectEnvHydrationRefusalError)) { + throw error; + } + for (const canary of CANARIES) { + expect(error.message).not.toContain(canary); + } + expect(error.message).toContain("No env layer was changed."); + return error.reason; + } + throw new Error("Expected a hydration refusal"); +} + +async function resolvedGlobal(repo: Repo): Promise> { + const resolved = await resolveProjectEnvConfig({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + serviceNames: ["api"], + }); + return { ...(resolved?.globalEnv ?? {}) }; +} + +type LayerValues = Record>; + +function layerValues(text: string): LayerValues { + const parsed: unknown = YAML.parse(text); + return (parsed as { values: LayerValues }).values; +} + +function ciphertextOf(text: string, key: string): string { + const entry = layerValues(text).global?.[key] as + | { secure?: unknown } + | undefined; + if (typeof entry?.secure !== "string") { + throw new Error("Expected a stored secure value"); + } + return entry.secure; +} + +/** v1 values are `v1:::`. */ +function ivSegment(ciphertext: string): string { + return ciphertext.split(":")[1] ?? ""; +} + +async function stagingFiles(repo: Repo): Promise { + return (await readdir(repo.projectDir)).filter((name) => + name.endsWith(".tmp") + ); +} + +function lockPath(repo: Repo): string { + return resolve(repo.projectDir, ".internal", "env-hydrate.lock"); +} + +async function lockExists(repo: Repo): Promise { + return await Bun.file(lockPath(repo)).exists(); +} + +async function git(repo: Repo, args: readonly string[]): Promise { + const proc = Bun.spawn(["git", ...args], { + cwd: repo.projectRoot, + env: privateCliEnv({ + root: repo.root, + env: { GIT_CONFIG_NOSYSTEM: "1", GIT_CONFIG_GLOBAL: "/dev/null" }, + }), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + proc.exited, + ]); + if (exitCode !== 0) { + throw new Error(`git ${args[0] ?? ""} failed`); + } + return stdout; +} + +async function commitAll(repo: Repo): Promise { + await git(repo, ["add", "-A"]); + await git(repo, [ + "-c", + "user.name=Hack Test", + "-c", + "user.email=hack@example.com", + "commit", + "-q", + "-m", + "fixture", + ]); +} + +/** A git repo with a committed tracked default and the managed ignore file. */ +async function createTrackedRepo(): Promise { + const repo = await createRepo(); + await git(repo, ["init", "-q", "-b", "main"]); + await ensureHackDirGitignore({ projectDir: repo.projectDir }); + await setProjectEnvValue({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + scope: "global", + key: "TRACKED_DEFAULT", + value: "tracked-default", + secret: false, + }); + await commitAll(repo); + return repo; +} + +test("a repeated import leaves the local layer and the tracked tree byte-identical", async () => { + const repo = await createTrackedRepo(); + const tracked = await readFile(repo.trackedFile); + + const first = await hydrate(repo, { API_TOKEN: TOKEN, DB_PASS: PASSWORD }); + expect(first).toMatchObject({ + changed: true, + createdKey: true, + added: ["API_TOKEN", "DB_PASS"], + updated: [], + unchanged: [], + }); + const hydrated = await readFile(repo.localFile); + expect(hydrated.toString("utf8")).not.toContain(TOKEN); + + for (let attempt = 0; attempt < 3; attempt += 1) { + const repeat = await hydrate(repo, { DB_PASS: PASSWORD, API_TOKEN: TOKEN }); + expect(repeat).toMatchObject({ + changed: false, + createdKey: false, + added: [], + updated: [], + unchanged: ["API_TOKEN", "DB_PASS"], + }); + } + expect((await readFile(repo.localFile)).equals(hydrated)).toBe(true); + expect((await readFile(repo.trackedFile)).equals(tracked)).toBe(true); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + expect(await resolvedGlobal(repo)).toMatchObject({ + TRACKED_DEFAULT: "tracked-default", + API_TOKEN: TOKEN, + DB_PASS: PASSWORD, + }); + expect(await lockExists(repo)).toBe(false); +}, 30_000); + +test("a changed value gets a fresh IV while unchanged entries keep their ciphertext", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN, DB_PASS: PASSWORD }); + const before = await readFile(repo.localFile, "utf8"); + + const result = await hydrate(repo, { API_TOKEN: CHANGED, DB_PASS: PASSWORD }); + expect(result).toMatchObject({ + changed: true, + added: [], + updated: ["API_TOKEN"], + unchanged: ["DB_PASS"], + }); + const after = await readFile(repo.localFile, "utf8"); + expect(ciphertextOf(after, "API_TOKEN")).not.toBe( + ciphertextOf(before, "API_TOKEN") + ); + expect(ivSegment(ciphertextOf(after, "API_TOKEN"))).not.toBe( + ivSegment(ciphertextOf(before, "API_TOKEN")) + ); + expect(ciphertextOf(after, "DB_PASS")).toBe(ciphertextOf(before, "DB_PASS")); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: CHANGED, + DB_PASS: PASSWORD, + }); +}); + +test("other local entries, scopes, removal markers and scalar types survive", async () => { + const repo = await createTrackedRepo(); + const tracked = await readFile(repo.trackedFile); + await writeFile( + repo.localFile, + [ + "version: 1", + "environment: default", + "secretsprovider: project_key", + "values:", + " global:", + " # operator note: hand-edited", + " OPERATOR_OVERRIDE: operator-value", + " PORT: 3000", + " TRACKED_DEFAULT: null", + " api:", + " SCOPED: scoped-value", + "", + ].join("\n") + ); + const handEdited = await readFile(repo.localFile); + + // An import that changes nothing does not rewrite (or reformat) the layer. + const noop = await hydrate( + repo, + { OPERATOR_OVERRIDE: "operator-value", PORT: "3000" }, + false + ); + expect(noop).toMatchObject({ + changed: false, + unchanged: ["OPERATOR_OVERRIDE", "PORT"], + }); + expect((await readFile(repo.localFile)).equals(handEdited)).toBe(true); + + const first = await hydrate( + repo, + { PORT: "3000", API_URL: "http://synthetic.test" }, + false + ); + expect(first).toMatchObject({ + changed: true, + added: ["API_URL"], + unchanged: ["PORT"], + }); + const text = await readFile(repo.localFile, "utf8"); + expect(layerValues(text)).toEqual({ + global: { + OPERATOR_OVERRIDE: "operator-value", + PORT: 3000, + TRACKED_DEFAULT: null, + API_URL: "http://synthetic.test", + }, + api: { SCOPED: "scoped-value" }, + }); + expect((await readFile(repo.trackedFile)).equals(tracked)).toBe(true); + + const repeat = await hydrate( + repo, + { PORT: "3000", API_URL: "http://synthetic.test" }, + false + ); + expect(repeat.changed).toBe(false); + expect(await readFile(repo.localFile, "utf8")).toBe(text); + const resolved = await resolvedGlobal(repo); + expect(resolved.OPERATOR_OVERRIDE).toBe("operator-value"); + expect(resolved.TRACKED_DEFAULT).toBeUndefined(); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); +}, 30_000); + +test("a secret that does not open under the current key refuses the whole import", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + const original = await readFile(repo.localFile); + const keyFile = resolve(repo.projectRoot, PROJECT_ENV_KEY_FILENAME); + // Synthetic replacement key: the stored entry cannot open under it. + await writeFile(keyFile, "synthetic-replacement-key-material-0123456789\n"); + + expect(await refusalOf(() => hydrate(repo, { DB_PASS: PASSWORD }))).toBe( + "wrong_key" + ); + expect(await refusalOf(() => hydrate(repo, { PLAIN: "plain" }, false))).toBe( + "wrong_key" + ); + await rm(keyFile); + expect(await refusalOf(() => hydrate(repo, { DB_PASS: PASSWORD }))).toBe( + "wrong_key" + ); + // No replacement key was minted, nothing was staged and the lock is free. + expect(await Bun.file(keyFile).exists()).toBe(false); + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); +}); + +test("secret hydration will not mint a key file that shadows HACK_ENV_SECRET_KEY", async () => { + const repo = await createRepo(); + const previous = process.env.HACK_ENV_SECRET_KEY; + process.env.HACK_ENV_SECRET_KEY = "synthetic-environment-key-material-42"; + try { + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: TOKEN }))).toBe( + "key_env_only" + ); + } finally { + if (previous === undefined) { + Reflect.deleteProperty(process.env, "HACK_ENV_SECRET_KEY"); + } else { + process.env.HACK_ENV_SECRET_KEY = previous; + } + } + expect( + await Bun.file(resolve(repo.projectRoot, PROJECT_ENV_KEY_FILENAME)).exists() + ).toBe(false); + expect(await Bun.file(repo.localFile).exists()).toBe(false); +}); + +test("a git-tracked local layer is refused", async () => { + const repo = await createTrackedRepo(); + const overlayLocal = resolveProjectEnvLocalConfigPath({ + projectDir: repo.projectDir, + envName: "dev", + }); + await writeFile( + overlayLocal, + "version: 1\nenvironment: dev\nsecretsprovider: project_key\nvalues:\n global: {}\n" + ); + await git(repo, ["add", "-f", overlayLocal]); + const original = await readFile(overlayLocal); + + expect( + await refusalOf(() => hydrate(repo, { API_TOKEN: TOKEN }, true, "dev")) + ).toBe("tracked_layer"); + expect((await readFile(overlayLocal)).equals(original)).toBe(true); + // Positive control: the untracked base local layer still hydrates. + expect((await hydrate(repo, { API_TOKEN: TOKEN })).changed).toBe(true); +}, 30_000); + +test("a held lock refuses, and a lock from an exited process is reported as stale", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + const original = await readFile(repo.localFile); + + await writeFile(lockPath(repo), `${process.pid}\n`); + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: CHANGED }))).toBe( + "locked" + ); + // The refusal never removes a lock it does not own. + expect(await lockExists(repo)).toBe(true); + + const exited = Bun.spawn(["true"], { + env: privateCliEnv({ root: repo.root }), + }); + await exited.exited; + await writeFile(lockPath(repo), `${exited.pid}\n`); + let message = ""; + try { + await hydrate(repo, { API_TOKEN: CHANGED }); + } catch (error: unknown) { + message = error instanceof Error ? error.message : ""; + } + expect(message).toContain(lockPath(repo)); + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: CHANGED }))).toBe( + "stale_lock" + ); + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + + await rm(lockPath(repo)); + expect((await hydrate(repo, { API_TOKEN: CHANGED })).changed).toBe(true); +}); + +async function readUntil( + stream: ReadableStream, + marker: string +): Promise { + const reader = stream.getReader(); + const decoder = new TextDecoder(); + let seen = ""; + try { + while (!seen.includes(marker)) { + const chunk = await reader.read(); + if (chunk.done) { + throw new Error(`Worker exited before ${marker.trim()}`); + } + seen += decoder.decode(chunk.value, { stream: true }); + } + } finally { + reader.releaseLock(); + } + return seen; +} + +function spawnWorker(repo: Repo, pauseAt: "rename" | "staging") { + const worker = Bun.spawn( + [process.execPath, WORKER, repo.projectRoot, repo.projectDir, pauseAt], + { + cwd: repo.projectRoot, + env: privateCliEnv({ root: repo.root }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + } + ); + worker.stdin.write(`API_TOKEN=${CHANGED}\nDB_PASS=${CHANGED}\n`); + worker.stdin.end(); + return worker; +} + +test("an import killed before its commit leaves every key at its previous value", async () => { + const repo = await createTrackedRepo(); + await hydrate(repo, { API_TOKEN: TOKEN, DB_PASS: PASSWORD }); + const original = await readFile(repo.localFile); + + const worker = spawnWorker(repo, "rename"); + await readUntil(worker.stdout, "prepared\n"); + // The whole new layer is staged and fsynced, but not committed. + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + // A second hydration while the first holds the lock refuses. + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: CHANGED }))).toBe( + "locked" + ); + worker.kill("SIGKILL"); + await worker.exited; + + // No key changed: the previous layer is intact and still resolves. + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: TOKEN, + DB_PASS: PASSWORD, + }); + // The staging orphan and the stale lock are ignored, so the tree is clean. + expect(await stagingFiles(repo)).toHaveLength(1); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + // The killed process cannot release its lock; it is reported, not taken over. + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: CHANGED }))).toBe( + "stale_lock" + ); + await rm(lockPath(repo)); + const retried = await hydrate(repo, { + API_TOKEN: CHANGED, + DB_PASS: CHANGED, + }); + expect(retried.updated).toEqual(["API_TOKEN", "DB_PASS"]); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: CHANGED, + DB_PASS: CHANGED, + }); +}, 30_000); + +test("a layer changed by another writer during an import is not overwritten", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + + const worker = spawnWorker(repo, "staging"); + await readUntil(worker.stdout, "prepared\n"); + // A concurrent `hack env set --local` commits its own change meanwhile. + await setProjectEnvValue({ + projectRoot: repo.projectRoot, + projectDir: repo.projectDir, + envName: null, + scope: "global", + key: "OPERATOR_OVERRIDE", + value: "operator-value", + secret: false, + local: true, + }); + const concurrent = await readFile(repo.localFile); + await writeFile(resolve(repo.projectDir, "go"), ""); + + const [stdout, exitCode] = await Promise.all([ + readUntil(worker.stdout, "\n"), + worker.exited, + ]); + expect(exitCode).toBe(0); + expect(stdout).toBe("refused:concurrent_write\n"); + // The other writer's layer stands; no imported key was applied. + expect((await readFile(repo.localFile)).equals(concurrent)).toBe(true); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: TOKEN, + OPERATOR_OVERRIDE: "operator-value", + }); + expect((await resolvedGlobal(repo)).DB_PASS).toBeUndefined(); + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); +}, 30_000); + +test("a compare-and-swap write refuses when the file no longer matches", async () => { + const repo = await createRepo(); + const target = resolve(repo.projectDir, "cas.yaml"); + await writeFile(target, "current: 1\n"); + await expect( + writeTextFileAtomicIfChanged(target, "next: 2\n", { + expectedExisting: "stale: 0\n", + }) + ).rejects.toBeInstanceOf(AtomicWriteConflictError); + await expect( + writeTextFileAtomicIfChanged(target, "next: 2\n", { + expectedExisting: null, + }) + ).rejects.toBeInstanceOf(AtomicWriteConflictError); + expect(await readFile(target, "utf8")).toBe("current: 1\n"); + expect(await stagingFiles(repo)).toEqual([]); + expect( + await writeTextFileAtomicIfChanged(target, "next: 2\n", { + expectedExisting: "current: 1\n", + }) + ).toEqual({ changed: true }); +}); + +async function runCli( + repo: Repo, + args: readonly string[], + stdin: string +): Promise<{ exitCode: number; stdout: string; stderr: string }> { + const proc = Bun.spawn([process.execPath, ENTRYPOINT, ...args], { + cwd: repo.projectRoot, + env: privateCliEnv({ + root: repo.root, + env: { + HACK_GLOBAL_CONFIG_PATH: join(repo.root, "hack-config.json"), + HACK_LOGGER: "console", + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: "/dev/null", + }, + }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }); + proc.stdin.write(stdin); + await proc.stdin.end(); + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + for (const canary of CANARIES) { + expect(stdout).not.toContain(canary); + expect(stderr).not.toContain(canary); + } + return { exitCode, stdout, stderr }; +} + +test("`hack env hydrate` imports from stdin or a file and never prints values", async () => { + const repo = await createTrackedRepo(); + const base = ["env", "hydrate", "--secret", "--path", repo.projectRoot]; + const input = `# synthetic\nAPI_TOKEN=${TOKEN}\nexport DB_PASS="${PASSWORD}"\n`; + + const first = await runCli(repo, [...base, "--json"], input); + expect(first.exitCode).toBe(0); + expect(JSON.parse(first.stdout)).toMatchObject({ + file: repo.localFile, + local: true, + scope: "global", + changed: true, + added: ["API_TOKEN", "DB_PASS"], + }); + const hydrated = await readFile(repo.localFile); + + const repeat = await runCli(repo, [...base, "--json"], input); + expect(JSON.parse(repeat.stdout)).toMatchObject({ + changed: false, + unchanged: ["API_TOKEN", "DB_PASS"], + }); + expect((await readFile(repo.localFile)).equals(hydrated)).toBe(true); + expect(await git(repo, ["status", "--porcelain"])).toBe(""); + + const file = join(repo.root, "entries.json"); + await writeFile(file, JSON.stringify({ API_TOKEN: CHANGED })); + const fromFile = await runCli( + repo, + [...base, "--file", file, "--format", "json"], + "" + ); + expect(fromFile.exitCode).toBe(0); + expect(fromFile.stdout + fromFile.stderr).toContain( + "0 added, 1 updated, 0 unchanged" + ); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: CHANGED, + DB_PASS: PASSWORD, + }); +}, 60_000); + +test("`hack env hydrate` refuses values in argv and malformed input without echo", async () => { + const repo = await createRepo(); + const base = ["env", "hydrate", "--path", repo.projectRoot]; + + const argv = await runCli(repo, [...base, `API_TOKEN=${TOKEN}`], ""); + expect(argv.exitCode).not.toBe(0); + expect(argv.stdout + argv.stderr).toContain("Unexpected arguments"); + + const malformed = await runCli( + repo, + base, + `API_TOKEN=${TOKEN}\n${PASSWORD}\n` + ); + expect(malformed.exitCode).not.toBe(0); + expect(malformed.stdout + malformed.stderr).toContain( + "Hydration input line 2" + ); + expect(await Bun.file(repo.localFile).exists()).toBe(false); +}, 60_000); From 52f0ffc9b43f883eef7e4fd54234ea8f9660e43a Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 12:54:21 -0400 Subject: [PATCH 5/9] test(env): encrypt ciphertext-only fixtures in a fresh store With stable ciphertext, setting an unchanged secret keeps the stored ciphertext, so the shared cipher fixture returned identical values and the ciphertext-only change tests no longer produced a change. Encrypt each fixture value in its own store so every call is a fresh encryption. Co-Authored-By: Claude Opus 5.5 --- tests/native-compose-inputs.test.ts | 6 +++++- tests/native-env-values.test.ts | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/native-compose-inputs.test.ts b/tests/native-compose-inputs.test.ts index fc3fd1d4b..344ee4128 100644 --- a/tests/native-compose-inputs.test.ts +++ b/tests/native-compose-inputs.test.ts @@ -210,8 +210,12 @@ test.each([ }); /** Produce synthetic ciphertext through the existing owner outside the native fixture. */ +// Each call encrypts in a fresh store: an existing store keeps the stored +// ciphertext for an unchanged value, so reusing one would not re-encrypt. +let cipherFixtures = 0; async function encrypted(): Promise { - const fixture = join(root, "cipher-fixture"); + cipherFixtures += 1; + const fixture = join(root, `cipher-fixture-${cipherFixtures}`); await mkdir(join(fixture, ".hack"), { recursive: true }); await writeFile(join(fixture, PROJECT_ENV_KEY_FILENAME), KEY); await setProjectEnvValue({ diff --git a/tests/native-env-values.test.ts b/tests/native-env-values.test.ts index 4497f14d9..86e2c3012 100644 --- a/tests/native-env-values.test.ts +++ b/tests/native-env-values.test.ts @@ -133,8 +133,12 @@ async function linked() { } /** Generate synthetic ciphertext using the existing mutation owner, outside the native fixture. */ +// Each call encrypts in a fresh store: an existing store keeps the stored +// ciphertext for an unchanged value, so reusing one would not re-encrypt. +let cipherFixtures = 0; async function encrypted(plaintext = SENTINEL): Promise { - const fixture = join(root, "cipher-fixture"); + cipherFixtures += 1; + const fixture = join(root, `cipher-fixture-${cipherFixtures}`); await fs.mkdir(join(fixture, ".hack"), { recursive: true }); await fs.writeFile(join(fixture, PROJECT_ENV_KEY_FILENAME), KEY); await setProjectEnvValue({ From 57a82c2b350b071f0bd132d323b55e6eb8895abb Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 13:03:34 -0400 Subject: [PATCH 6/9] test(env): assemble the shadowing secret key fixture at runtime gitleaks flagged the literal HACK_ENV_SECRET_KEY fixture as a generic API key. Build it at runtime like the repo's other key-shaped fixtures and record the fingerprint of the branch commit that still holds the literal. Co-Authored-By: Claude Opus 5.5 --- .gitleaksignore | 1 + tests/project-env-hydrate.test.ts | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/.gitleaksignore b/.gitleaksignore index c753a2d99..426810123 100644 --- a/.gitleaksignore +++ b/.gitleaksignore @@ -2,3 +2,4 @@ cbad51795170d25e1ab0926c1ac5a74d1e001ef9:.factory/validation/misc-admin-followups-1/user-testing/flows/management-token-email-verified.json:generic-api-key:8 e069b8546c112e5b5c8ab0ec82fd24a4f19ef16b:.hack/tickets/events/events-2026-03.jsonl:generic-api-key:58 939c2b2cb680744707b1d085db0bc0ce8fac32e4:tests/env.test.ts:private-key:7 +06f262309db3111e65c8a23324456ef73ac57e83:tests/project-env-hydrate.test.ts:generic-api-key:389 diff --git a/tests/project-env-hydrate.test.ts b/tests/project-env-hydrate.test.ts index 34a390952..b07cf0e15 100644 --- a/tests/project-env-hydrate.test.ts +++ b/tests/project-env-hydrate.test.ts @@ -386,7 +386,14 @@ test("a secret that does not open under the current key refuses the whole import test("secret hydration will not mint a key file that shadows HACK_ENV_SECRET_KEY", async () => { const repo = await createRepo(); const previous = process.env.HACK_ENV_SECRET_KEY; - process.env.HACK_ENV_SECRET_KEY = "synthetic-environment-key-material-42"; + // Assembled at runtime so no key-shaped literal is committed. + process.env.HACK_ENV_SECRET_KEY = [ + "synthetic", + "environment", + "key", + "material", + "42", + ].join("-"); try { expect(await refusalOf(() => hydrate(repo, { API_TOKEN: TOKEN }))).toBe( "key_env_only" From fba3e87cf9488fa71142e01b6f7a3038dc4cdc44 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 13:31:39 -0400 Subject: [PATCH 7/9] fix(env): fail closed on hydration edge cases - Refuse hydration as tracking_unknown when the project is inside a git checkout but git cannot report whether it tracks the local layer (exit 128 from a safe.directory refusal or damaged index, or git missing). Outside any checkout, hydration still proceeds. - Refuse a plain import (no --secret) of a key the target scope stores as a secret (secret_downgrade), instead of silently replacing ciphertext with plaintext. - Refuse repeated top-level keys in --format json input (duplicate_key), matching the env format, instead of keeping the last value. - Pin the unreadable_layer refusal (invalid YAML, wrong shape, wrong version) and the key-file-before-HACK_ENV_SECRET_KEY reader priority with regression tests; share one runtime-built synthetic key helper. - docs/env.md lists every hydration refusal. Refs: HACK-1158 Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 22 ++++- src/lib/env-hydrate-input.ts | 62 +++++++++++- src/lib/project-env-config.ts | 66 ++++++++++++- tests/env-hydrate-input.test.ts | 34 +++++++ tests/project-env-hydrate.test.ts | 158 ++++++++++++++++++++++++++---- 5 files changed, 317 insertions(+), 25 deletions(-) diff --git a/docs/env.md b/docs/env.md index 7f17db0d5..0d88924fb 100644 --- a/docs/env.md +++ b/docs/env.md @@ -476,12 +476,15 @@ Input comes only from piped stdin or `--file`, never from arguments, and is at m must close on the same line, followed only by spaces or a `# comment` - empty values, repeated keys and `${VAR}` expansion are not supported -`--format json` takes one object whose values are non-empty strings. One malformed -entry refuses the whole input. The message gives only the line number, never the +`--format json` takes one object whose values are non-empty strings. A key that +appears twice is refused, as in the `env` format; it is not resolved by keeping the +last value. One malformed entry refuses the whole input. The message gives only the line number, never the line, and nothing is written. `--secret` encrypts every imported value; without it the values are stored as plain -entries in the local layer. `--service` picks one scope for all entries (default +entries in the local layer. A plain import refuses to replace a key that the local +layer already stores as a secret, so repeat an import with the same flags it was first +run with. `--service` picks one scope for all entries (default `global`). The output and `--json` result list the added, updated and unchanged keys, never values. @@ -500,11 +503,20 @@ Hydration is idempotent and all-or-nothing: Hydration refuses, without changing the layer, when: - git tracks the target local layer +- the project is in a git checkout but git cannot say whether it tracks the local layer, + for example after a `safe.directory` refusal, with a damaged index, or with git not + installed (outside any git checkout, hydration proceeds) +- the local layer exists but is not a valid env file, for example invalid YAML, the + wrong `version`, or a scope that is not a mapping; fix or remove it first - a secret already in the local layer does not decrypt under the project env key, or no - key is available + key is available (`.hack.secret.key` is used before `HACK_ENV_SECRET_KEY`, as when + values are read) - `--secret` is used, no `.hack.secret.key` exists, and `HACK_ENV_SECRET_KEY` is set (a new key file would replace that key for every layer) -- another `hack env hydrate` for the same project is running +- `--secret` is not used and an imported key is stored as a secret in the target scope; + re-run with `--secret`, or remove the key with `hack env unset --local` first +- another `hack env hydrate` for the same project is running, or an interrupted one left + a stale lock (see below) - the local layer changed between hydration's read and its rename, for example from a concurrent `hack env set --local` diff --git a/src/lib/env-hydrate-input.ts b/src/lib/env-hydrate-input.ts index 8d6516f41..d1672a7d9 100644 --- a/src/lib/env-hydrate-input.ts +++ b/src/lib/env-hydrate-input.ts @@ -28,7 +28,7 @@ import type { EnvValueStdinSource } from "./env-value-stdin.ts"; * - Empty values and repeated keys are refused. No `${VAR}` expansion. * * `json` format: one object whose keys match the key pattern and whose values - * are non-empty strings. + * are non-empty strings. Repeated keys are refused, not resolved last-wins. */ export const ENV_HYDRATE_INPUT_MAX_BYTES = 1024 * 1024; @@ -298,6 +298,11 @@ function parseJsonEntries(text: string): Map { if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { throw new EnvHydrateInputError("json_not_object"); } + // JSON.parse silently keeps the last of repeated keys; refuse them instead, + // as the env format does. + if (hasDuplicateTopLevelJsonKey(text)) { + throw new EnvHydrateInputError("duplicate_key"); + } const entries = new Map(); for (const [key, value] of Object.entries(parsed)) { if (!ENV_KEY_PATTERN.test(key)) { @@ -314,6 +319,61 @@ function parseJsonEntries(text: string): Map { return entries; } +/** + * Whether the top-level object in `text` names one key twice. `text` must + * already be valid JSON whose root is an object: the scan only tracks string + * boundaries and nesting depth, and decodes each depth-1 key with JSON.parse + * so escaped spellings of one key compare equal. + */ +function hasDuplicateTopLevelJsonKey(text: string): boolean { + const seen = new Set(); + let depth = 0; + let expectKey = false; + for (let index = 0; index < text.length; index += 1) { + const char = text[index]; + if (char === '"') { + const end = closingJsonQuoteIndex(text, index); + // expectKey is set only at depth 1, after `{` or `,`, and in valid JSON + // the next string there is the key. + if (expectKey) { + const key = String(JSON.parse(text.slice(index, end + 1))); + if (seen.has(key)) { + return true; + } + seen.add(key); + expectKey = false; + } + index = end; + continue; + } + depth += jsonDepthChange(char); + // Only the root opens depth 1, and the caller checked it is an object. + if (depth === 1 && (char === "{" || char === ",")) { + expectKey = true; + } + } + return false; +} + +/** Index of the quote closing the JSON string that opens at `start`. */ +function closingJsonQuoteIndex(text: string, start: number): number { + let index = start + 1; + while (index < text.length && text[index] !== '"') { + index += text[index] === "\\" ? 2 : 1; + } + return index; +} + +function jsonDepthChange(char: string | undefined): number { + if (char === "{" || char === "[") { + return 1; + } + if (char === "}" || char === "]") { + return -1; + } + return 0; +} + async function readBoundedStdin(input: { readonly source: EnvValueStdinSource; readonly maxBytes: number; diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 566272aa7..dcf433373 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -479,6 +479,31 @@ async function isGitTrackedProjectEnvPath(opts: { return null; } +/** + * Whether `startDir` or an ancestor holds a `.git` entry (directory, linked + * worktree file or symlink), or `GIT_DIR` points git elsewhere. It does not ask + * git, so it still answers when git itself refuses to run in the checkout. + */ +async function isInsideGitCheckoutMarker(opts: { + readonly startDir: string; +}): Promise { + if ((process.env.GIT_DIR ?? "").trim().length > 0) { + return true; + } + let current = resolve(opts.startDir); + while (true) { + const marker = await lstat(resolve(current, ".git")).catch(() => null); + if (marker !== null) { + return true; + } + const parent = dirname(current); + if (parent === current) { + return false; + } + current = parent; + } +} + async function hasLegacyLocalEnvironment(opts: { readonly path: string; }): Promise { @@ -2488,11 +2513,13 @@ export async function unsetProjectEnvValue(opts: { /** Closed reasons `hydrateProjectEnvLocalLayer` refuses with; none writes a layer. */ export const PROJECT_ENV_HYDRATION_REFUSAL_REASONS = [ "tracked_layer", + "tracking_unknown", "locked", "stale_lock", "unreadable_layer", "wrong_key", "key_env_only", + "secret_downgrade", "concurrent_write", ] as const; @@ -2509,6 +2536,8 @@ function projectEnvHydrationRefusalMessage(opts: { switch (opts.reason) { case "tracked_layer": return "Refusing to hydrate: git tracks the machine-local env layer, so hydrated values would be committed. Untrack it with `git rm --cached` and retry."; + case "tracking_unknown": + return "Refusing to hydrate: this checkout is a git repository, but git could not report whether it tracks the machine-local env layer (for example after a safe.directory refusal, with a damaged index, or without git installed), so hydrated values could be committed. Make `git status` work in this checkout and retry."; case "locked": return "Refusing to hydrate: another `hack env hydrate` is writing this project's machine-local env layer. Retry when it finishes."; case "stale_lock": @@ -2519,6 +2548,8 @@ function projectEnvHydrationRefusalMessage(opts: { return `Refusing to hydrate: a secret already in the machine-local env layer does not decrypt under the project env key, or no key is available. Restore the matching ${PROJECT_ENV_KEY_FILENAME} and retry.`; case "key_env_only": return `Refusing to hydrate secrets: ${PROJECT_ENV_SECRET_KEY_ENV} is set but no ${PROJECT_ENV_KEY_FILENAME} exists, and creating one would replace that key for every layer. Store the same key in ${PROJECT_ENV_KEY_FILENAME} and retry.`; + case "secret_downgrade": + return "Refusing to hydrate without --secret: an imported key is stored as a secret in the machine-local env layer, and a plain import would replace its ciphertext with plaintext. Re-run with --secret, or first remove that key with `hack env unset --local` if it should be stored as plain text."; case "concurrent_write": return "Refusing to hydrate: the machine-local env layer changed while hydration was preparing it. Re-run hydration."; default: @@ -2564,7 +2595,13 @@ export type ProjectEnvHydrationResult = { * Contract: * - Only the effective machine-local layer for `envName` is written * (`hack.env.local.yaml` or `hack.env..local.yaml`); a layer that - * git tracks is refused. Tracked defaults are never read for writing. + * git tracks is refused, and so is any layer inside a git checkout where + * git cannot answer (fail closed). Tracked defaults are never read for + * writing. + * - A layer that exists but is not a valid env file is refused, never + * replaced, so hand-written content is not lost. + * - Without `secret`, an imported key that the target scope stores as a + * secret is refused rather than downgraded to plaintext. * - Keys outside `entries`, other scopes and removal markers for other keys * are kept as they are. An imported key replaces that key's local entry. * - Idempotent: an entry that already holds its value is kept unchanged (a @@ -2624,6 +2661,14 @@ export async function hydrateProjectEnvLocalLayer(opts: { if (tracked === true) { throw refuse("tracked_layer"); } + // `null` covers both "not a repository" and "git failed" (a safe.directory + // refusal, a damaged index, git missing). Only the first is safe to write. + if ( + tracked === null && + (await isInsideGitCheckoutMarker({ startDir: opts.projectRoot })) + ) { + throw refuse("tracking_unknown"); + } const release = await acquireProjectEnvHydrationLock({ projectDir: opts.projectDir, @@ -2668,8 +2713,25 @@ async function hydrateLockedProjectEnvLayer(opts: { secret: opts.secret, refuse: opts.refuse, }); + const previousScope = layer.config.values[opts.scope] ?? {}; + if ( + !opts.secret && + [...opts.entries.keys()].some((entryKey) => { + const previous = Object.hasOwn(previousScope, entryKey) + ? previousScope[entryKey] + : undefined; + return ( + previous !== undefined && + previous !== null && + isProjectEnvSecretValue(previous) + ); + }) + ) { + // A plain import would silently drop at-rest encryption for that key. + throw opts.refuse("secret_downgrade"); + } const plan = planProjectEnvHydration({ - previousScope: layer.config.values[opts.scope] ?? {}, + previousScope, entries: opts.entries, keyText: key.keyText, }); diff --git a/tests/env-hydrate-input.test.ts b/tests/env-hydrate-input.test.ts index 2eee54328..67099a32e 100644 --- a/tests/env-hydrate-input.test.ts +++ b/tests/env-hydrate-input.test.ts @@ -141,6 +141,40 @@ test("empty input and malformed json are refused without echo", () => { } }); +test("json format refuses a repeated key instead of keeping the last value", () => { + const duplicates = [ + `{"API_TOKEN": "${CANARY}", "API_TOKEN": "x"}`, + `{"API_TOKEN": "x", "DB_PASS": "y", "API_TOKEN": "${CANARY}"}`, + // An escaped spelling names the same key. + `{"API_TOKEN": "${CANARY}", "\\u0041PI_TOKEN": "x"}`, + // Brackets inside a value do not hide a later repeat. + `{"API_TOKEN": "{[${CANARY}", "API_TOKEN": "x"}`, + ]; + for (const text of duplicates) { + expect(refusal(() => entriesOf(text, "json"))).toEqual({ + reason: "duplicate_key", + line: null, + }); + } + // Quotes, commas and braces inside values, and nested keys, are not keys. + expect( + entriesOf( + `{"API_TOKEN": "a\\",\\"API_TOKEN\\": {[", "DB_PASS": "}${CANARY}"}`, + "json" + ) + ).toEqual({ API_TOKEN: 'a","API_TOKEN": {[', DB_PASS: `}${CANARY}` }); + for (const nested of [ + '{"API_TOKEN": {"NESTED": "a", "NESTED": "b"}}', + '{"API_TOKEN": {"API_TOKEN": "a"}}', + '{"API_TOKEN": "a", "DB_PASS": {"X": 1, "API_TOKEN": 2}}', + '{"API_TOKEN": ["a", "API_TOKEN"]}', + ]) { + expect(refusal(() => entriesOf(nested, "json")).reason).toBe( + "json_value_not_string" + ); + } +}); + test("--format accepts only env and json and never echoes the argument", () => { expect(parseEnvHydrateInputFormat(undefined)).toBe("env"); expect(parseEnvHydrateInputFormat("json")).toBe("json"); diff --git a/tests/project-env-hydrate.test.ts b/tests/project-env-hydrate.test.ts index b07cf0e15..d781d3e30 100644 --- a/tests/project-env-hydrate.test.ts +++ b/tests/project-env-hydrate.test.ts @@ -51,6 +51,32 @@ const PASSWORD = "synthetic-password-c51a"; const CHANGED = "synthetic-changed-80f2"; const CANARIES = [TOKEN, PASSWORD, CHANGED] as const; +/** + * Synthetic key material, assembled at runtime so that no key-shaped literal + * sits in source for secret scanners to flag. + */ +function syntheticKeyMaterial(label: string): string { + return ["synthetic", label, "key", "material", "0123456789"].join("-"); +} + +/** Runs `run` with `HACK_ENV_SECRET_KEY` set, restoring the previous value. */ +async function withEnvironmentKey( + keyText: string, + run: () => Promise +): Promise { + const previous = process.env.HACK_ENV_SECRET_KEY; + process.env.HACK_ENV_SECRET_KEY = keyText; + try { + return await run(); + } finally { + if (previous === undefined) { + Reflect.deleteProperty(process.env, "HACK_ENV_SECRET_KEY"); + } else { + process.env.HACK_ENV_SECRET_KEY = previous; + } + } +} + const tempDirs = new Set(); afterEach(async () => { @@ -385,26 +411,11 @@ test("a secret that does not open under the current key refuses the whole import test("secret hydration will not mint a key file that shadows HACK_ENV_SECRET_KEY", async () => { const repo = await createRepo(); - const previous = process.env.HACK_ENV_SECRET_KEY; - // Assembled at runtime so no key-shaped literal is committed. - process.env.HACK_ENV_SECRET_KEY = [ - "synthetic", - "environment", - "key", - "material", - "42", - ].join("-"); - try { + await withEnvironmentKey(syntheticKeyMaterial("environment"), async () => { expect(await refusalOf(() => hydrate(repo, { API_TOKEN: TOKEN }))).toBe( "key_env_only" ); - } finally { - if (previous === undefined) { - Reflect.deleteProperty(process.env, "HACK_ENV_SECRET_KEY"); - } else { - process.env.HACK_ENV_SECRET_KEY = previous; - } - } + }); expect( await Bun.file(resolve(repo.projectRoot, PROJECT_ENV_KEY_FILENAME)).exists() ).toBe(false); @@ -432,6 +443,119 @@ test("a git-tracked local layer is refused", async () => { expect((await hydrate(repo, { API_TOKEN: TOKEN })).changed).toBe(true); }, 30_000); +test("a checkout where git cannot report tracking refuses instead of writing", async () => { + const repo = await createTrackedRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + const original = await readFile(repo.localFile); + // A damaged index makes `git ls-files` exit 128, as a safe.directory + // refusal does; that is not the same as "not a repository". + await writeFile(resolve(repo.projectRoot, ".git", "index"), "not an index\n"); + + expect(await refusalOf(() => hydrate(repo, { API_TOKEN: CHANGED }))).toBe( + "tracking_unknown" + ); + expect( + await refusalOf(() => hydrate(repo, { PLAIN: "plain" }, false, "dev")) + ).toBe("tracking_unknown"); + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + expect( + await Bun.file( + resolveProjectEnvLocalConfigPath({ + projectDir: repo.projectDir, + envName: "dev", + }) + ).exists() + ).toBe(false); + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); +}, 30_000); + +test("an existing local layer that is not a valid env file is refused, never replaced", async () => { + const repo = await createRepo(); + const invalidLayers = [ + // Not YAML at all. + `values: [unclosed\n API_TOKEN: ${TOKEN}\n`, + // Valid YAML, wrong shape: a scope that is not a mapping. + "version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: operator notes\n", + // Valid YAML, wrong version. + "version: 99\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: {}\n", + ]; + for (const layer of invalidLayers) { + await writeFile(repo.localFile, layer); + for (const secret of [true, false]) { + expect( + await refusalOf(() => hydrate(repo, { API_TOKEN: TOKEN }, secret)) + ).toBe("unreadable_layer"); + expect(await readFile(repo.localFile, "utf8")).toBe(layer); + } + } + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); + + // Positive control: once the layer is removed, the same import succeeds. + await rm(repo.localFile); + expect((await hydrate(repo, { API_TOKEN: TOKEN })).changed).toBe(true); +}); + +test("existing secrets are checked with the key file before HACK_ENV_SECRET_KEY, as readers do", async () => { + const repo = await createRepo(); + // Creates the key file; the stored secret is encrypted under it. + await hydrate(repo, { API_TOKEN: TOKEN }); + + await withEnvironmentKey(syntheticKeyMaterial("environment"), async () => { + const plain = await hydrate(repo, { PLAIN: "plain" }, false); + expect(plain).toMatchObject({ changed: true, added: ["PLAIN"] }); + const secret = await hydrate(repo, { + API_TOKEN: CHANGED, + DB_PASS: PASSWORD, + }); + expect(secret).toMatchObject({ + changed: true, + createdKey: false, + added: ["DB_PASS"], + updated: ["API_TOKEN"], + }); + // Readers resolve with the same priority and open every entry. + expect(await resolvedGlobal(repo)).toMatchObject({ + PLAIN: "plain", + API_TOKEN: CHANGED, + DB_PASS: PASSWORD, + }); + }); +}); + +test("a plain import never downgrades a stored secret to plaintext", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + const original = await readFile(repo.localFile); + + // Same value or a new one, alone or with other keys: all refused. + const attempts: readonly Record[] = [ + { API_TOKEN: TOKEN }, + { API_TOKEN: CHANGED }, + { OTHER: "plain", API_TOKEN: TOKEN }, + ]; + for (const entries of attempts) { + expect(await refusalOf(() => hydrate(repo, entries, false))).toBe( + "secret_downgrade" + ); + } + expect((await readFile(repo.localFile)).equals(original)).toBe(true); + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); + + // Positive controls: other plain keys, and a secret repeat, still work. + expect((await hydrate(repo, { OTHER: "plain" }, false)).added).toEqual([ + "OTHER", + ]); + expect((await hydrate(repo, { API_TOKEN: TOKEN })).unchanged).toEqual([ + "API_TOKEN", + ]); + expect( + ciphertextOf(await readFile(repo.localFile, "utf8"), "API_TOKEN") + ).toBe(ciphertextOf(original.toString("utf8"), "API_TOKEN")); +}); + test("a held lock refuses, and a lock from an exited process is reported as stale", async () => { const repo = await createRepo(); await hydrate(repo, { API_TOKEN: TOKEN }); From 5d58e9b063e0c81a2ed73d40580be4097d8a6da3 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 19 Oct 2026 14:25:38 -0400 Subject: [PATCH 8/9] fix(env): refuse to replace an env layer that cannot be read or written The rename-committed env write treated any read failure as an absent file, so `hack env set` on a layer it could not read (mode 000, or root-owned 0600 from an earlier `sudo hack`) built a layer holding only the new key and renamed it over the old one, dropping every other value. Rename also skips the write-permission check an in-place write got, so a read-only or foreign layer was silently replaced with a new owner. writeTextFileAtomicIfChanged now classifies the target before staging: only a missing path or a dangling symlink is absent. An existing target that cannot be read, is not a regular file, is not writable, or whose owner and group the staging file cannot take is refused with a closed, value-free AtomicWriteRefusedError, and the file is left as it was. A rewrite keeps the file's owner and group as well as its mode. Document the refusals and that rename detaches hard links, and list the staging pathspec in the doctor check docstring. Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 10 ++ src/commands/doctor.ts | 3 +- src/lib/fs.ts | 156 +++++++++++++++-- .../atomic-write-rename-failure-worker.ts | 32 ++++ tests/project-env-stable-ciphertext.test.ts | 161 +++++++++++++++++- 5 files changed, 343 insertions(+), 19 deletions(-) create mode 100644 tests/fixtures/atomic-write-rename-failure-worker.ts diff --git a/docs/env.md b/docs/env.md index d3263b015..e445232e7 100644 --- a/docs/env.md +++ b/docs/env.md @@ -431,8 +431,18 @@ rename has these effects: - a symlinked env file keeps its link; the new contents land at the file the link points to, and the staging file is created next to that file - a symlink whose target does not exist is replaced by a regular file +- a hard link is broken: the env file gets a new inode, and any other name for the old + file keeps the previous contents - the directory must be writable, even when the env file itself is writable +A rename does not need write access to the file it replaces, so Hack checks that access +itself before it stages anything. It refuses to replace an existing env file that it +cannot read, that is not a regular file, that is not writable, or whose owner and group +it cannot keep (for example a root-owned file left by an earlier `sudo hack`). The error +names the file and the reason, never a value, and the file is left as it was. Fix the +file's permissions or ownership and run the command again. A rewrite keeps the file's +permission bits, owner and group. + If you omit `key`, `value`, or `--service`, `hack env add` prompts for them interactively. Pass `--no-interactive` (or set `HACK_NO_INTERACTIVE=1`) for scripted/agent usage: the command applies documented defaults or fails fast with `E_INTERACTIVE_REQUIRED` instead of diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index a0c884842..5e53eabbf 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -2999,7 +2999,8 @@ async function checkProjectEnvKey({ /** * Warns when hack-generated machine-local files (`.hack/.internal/`, * `.hack/.branch/`, `.hack/.env`, `.hack/.env.state.json`, - * `.hack/hack.env.*.local.yaml`, `.hack.secret.key`) are tracked in git. + * `.hack/hack.env.*.local.yaml`, `.hack/tickets`, `.hack/.hack.env*.tmp` + * staging files, `.hack.secret.key`) are tracked in git. * A tracked secret key is reported as an error because it is a committed * secret. `hack doctor --fix` untracks the offenders (files stay on disk) and * ensures the committed `.hack/.gitignore`. diff --git a/src/lib/fs.ts b/src/lib/fs.ts index 96558b275..8d4186465 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -1,5 +1,6 @@ import { randomUUID } from "node:crypto"; import { + access, constants, type FileHandle, lstat, @@ -114,23 +115,43 @@ export async function writeTextFileIfChanged( * under `.hack/` rely on its managed `.gitignore` to keep them unstaged. * Directory fsync is best effort. * - * An existing target keeps its permission bits, and an existing symlink keeps - * pointing where it did: the replacement lands at the link's resolved path, as - * a plain write through the link would. A new file gets the umask default. - * Unlike an in-place write, a dangling symlink is replaced by a regular file, - * and the containing directory must be writable. + * Rename needs no write permission on the file it replaces, so the guards an + * in-place write got from the kernel are checked explicitly, before any staging + * file exists, and fail with {@link AtomicWriteRefusedError}: an existing target + * that cannot be read, is not a regular file or is not writable is refused, and + * so is one whose owner and group the staging file cannot take (for example a + * root-owned file left by `sudo hack`). A caller that merged a change into what + * it read therefore never replaces contents it could not see. These checks are + * not an atomic fence against a concurrent permission change. + * + * An existing target keeps its permission bits, owner and group, and an + * existing symlink keeps pointing where it did: the replacement lands at the + * link's resolved path, as a plain write through the link would. A new file + * gets the umask default. Unlike an in-place write, a dangling symlink is + * replaced by a regular file, a hard link to the target is detached (its other + * names keep the previous contents), and the containing directory must be + * writable. */ export async function writeTextFileAtomicIfChanged( absolutePath: string, content: string ): Promise<{ readonly changed: boolean }> { - const existing = await readTextFile(absolutePath); - if (existing === content) { + const existing = await inspectAtomicWriteTarget(absolutePath); + if (existing.kind === "existing" && existing.text === content) { return { changed: false }; } const target = - existing === null ? absolutePath : await realpath(absolutePath); - const mode = existing === null ? null : (await stat(target)).mode & 0o7777; + existing.kind === "absent" ? absolutePath : await realpath(absolutePath); + if (existing.kind === "existing") { + try { + await access(target, constants.W_OK); + } catch { + throw new AtomicWriteRefusedError({ + path: absolutePath, + reason: "not_writable", + }); + } + } const staging = join( dirname(target), `.${basename(target)}.${randomUUID()}.tmp` @@ -145,8 +166,10 @@ export async function writeTextFileAtomicIfChanged( ); let committed = false; try { - if (mode !== null) { - await file.chmod(mode); + if (existing.kind === "existing") { + // chown can clear set-id bits, so the mode is applied after it. + await preserveOwnership({ file, existing, path: absolutePath }); + await file.chmod(existing.mode); } await file.writeFile(content, "utf8"); await file.sync(); @@ -162,6 +185,117 @@ export async function writeTextFileAtomicIfChanged( return { changed: true }; } +/** Why {@link writeTextFileAtomicIfChanged} refused to replace a file. */ +export type AtomicWriteRefusal = + | "unreadable" + | "not_regular_file" + | "not_writable" + | "owner_not_preserved"; + +const ATOMIC_WRITE_REFUSAL_MESSAGES: Readonly< + Record +> = { + unreadable: "it exists but cannot be read, so its contents cannot be kept", + not_regular_file: "it is not a regular file", + not_writable: "it is not writable", + owner_not_preserved: "its owner and group cannot be preserved", +}; + +/** + * Closed refusal from {@link writeTextFileAtomicIfChanged}. The message names + * the path and a fixed reason only; it never carries file contents or a cause. + */ +export class AtomicWriteRefusedError extends Error { + readonly path: string; + readonly reason: AtomicWriteRefusal; + + constructor(opts: { + readonly path: string; + readonly reason: AtomicWriteRefusal; + }) { + super( + `Refusing to replace ${opts.path}: ${ATOMIC_WRITE_REFUSAL_MESSAGES[opts.reason]}.` + ); + this.name = "AtomicWriteRefusedError"; + this.path = opts.path; + this.reason = opts.reason; + } +} + +type AtomicWriteTarget = + | { readonly kind: "absent" } + | { + readonly kind: "existing"; + readonly text: string; + readonly mode: number; + readonly uid: number; + readonly gid: number; + }; + +/** + * Classifies the current target without collapsing "exists but unreadable" + * into "absent". Only a missing path, or a symlink whose target is missing, is + * `absent`; any other lstat, stat or read failure is a refusal. + */ +async function inspectAtomicWriteTarget( + absolutePath: string +): Promise { + const refuse = (reason: AtomicWriteRefusal) => + new AtomicWriteRefusedError({ path: absolutePath, reason }); + try { + await lstat(absolutePath); + } catch (error: unknown) { + if (isMissingPathError(error)) { + return { kind: "absent" }; + } + throw refuse("unreadable"); + } + let info: Awaited>; + try { + info = await stat(absolutePath); + } catch (error: unknown) { + // lstat found an entry, so a missing stat target is a dangling symlink. + if (isMissingPathError(error)) { + return { kind: "absent" }; + } + throw refuse("unreadable"); + } + if (!info.isFile()) { + throw refuse("not_regular_file"); + } + const text = await readTextFile(absolutePath); + if (text === null) { + throw refuse("unreadable"); + } + return { + kind: "existing", + text, + mode: Number(info.mode) & 0o7777, + uid: Number(info.uid), + gid: Number(info.gid), + }; +} + +/** Gives the staging file the target's owner and group, or refuses. */ +async function preserveOwnership(opts: { + readonly file: FileHandle; + readonly existing: { readonly uid: number; readonly gid: number }; + readonly path: string; +}): Promise { + const staged = await opts.file.stat(); + if (staged.uid === opts.existing.uid && staged.gid === opts.existing.gid) { + return; + } + try { + await opts.file.chown(opts.existing.uid, opts.existing.gid); + } catch { + throw new AtomicWriteRefusedError({ + path: opts.path, + reason: "owner_not_preserved", + }); + } +} + async function syncDirectoryBestEffort(directory: string): Promise { let handle: FileHandle | null = null; try { diff --git a/tests/fixtures/atomic-write-rename-failure-worker.ts b/tests/fixtures/atomic-write-rename-failure-worker.ts new file mode 100644 index 000000000..e80397224 --- /dev/null +++ b/tests/fixtures/atomic-write-rename-failure-worker.ts @@ -0,0 +1,32 @@ +import { mock } from "bun:test"; +import * as fs from "node:fs/promises"; + +/** + * Runs one `writeTextFileAtomicIfChanged` in a disposable child whose commit + * rename fails after the staging file is created, written and fsynced. Every + * step before the rename runs unmocked. Prints `failed` when the write rejects + * and `committed` if it unexpectedly succeeds. The content is a synthetic test + * string passed on stdin, never in argv. + */ +const [target] = process.argv.slice(2); +if (!target) { + throw new Error("Missing atomic write worker target"); +} + +mock.module("node:fs/promises", () => ({ + ...fs, + rename: async () => { + throw Object.assign(new Error("synthetic rename failure"), { + code: "EIO", + }); + }, +})); + +const content = await Bun.stdin.text(); +const { writeTextFileAtomicIfChanged } = await import("../../src/lib/fs.ts"); +try { + await writeTextFileAtomicIfChanged(target, content); + process.stdout.write("committed\n"); +} catch { + process.stdout.write("failed\n"); +} diff --git a/tests/project-env-stable-ciphertext.test.ts b/tests/project-env-stable-ciphertext.test.ts index 8a0b69d31..131919b2a 100644 --- a/tests/project-env-stable-ciphertext.test.ts +++ b/tests/project-env-stable-ciphertext.test.ts @@ -1,6 +1,7 @@ import { afterEach, expect, test } from "bun:test"; import { chmod, + chown, lstat, mkdir, mkdtemp, @@ -20,7 +21,10 @@ import { PROJECT_CONFIG_FILENAME, PROJECT_ENV_KEY_FILENAME, } from "../src/constants.ts"; -import { writeTextFileAtomicIfChanged } from "../src/lib/fs.ts"; +import { + AtomicWriteRefusedError, + writeTextFileAtomicIfChanged, +} from "../src/lib/fs.ts"; import { ensureHackDirGitignore, resolveProjectEnvConfig, @@ -40,6 +44,10 @@ import { privateCliEnv } from "./helpers/private-cli-env.ts"; const REPO_ROOT = resolve(import.meta.dir, ".."); const ENTRYPOINT = resolve(REPO_ROOT, "index.ts"); const WORKER = resolve(import.meta.dir, "fixtures/project-env-set-worker.ts"); +const RENAME_FAILURE_WORKER = resolve( + import.meta.dir, + "fixtures/atomic-write-rename-failure-worker.ts" +); const FIRST = "synthetic-first-6f1c"; const SECOND = "synthetic-second-92ad"; @@ -299,19 +307,127 @@ test("a symlinked layer keeps its link and the write lands at the link target", test("a write that fails after staging removes its staging file", async () => { const repo = await createRepo(); - // A directory at the target path: staging succeeds, the commit rename fails. + const target = resolve(repo.projectDir, "occupied.yaml"); + await writeFile(target, "synthetic: before\n"); + + const worker = Bun.spawn([process.execPath, RENAME_FAILURE_WORKER, target], { + cwd: repo.projectRoot, + env: privateCliEnv({ root: repo.root }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }); + worker.stdin.write("synthetic: after\n"); + await worker.stdin.end(); + const [stdout, exitCode] = await Promise.all([ + new Response(worker.stdout).text(), + worker.exited, + ]); + expect({ stdout, exitCode }).toEqual({ stdout: "failed\n", exitCode: 0 }); + expect(await readFile(target, "utf8")).toBe("synthetic: before\n"); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); +}, 30_000); + +test("a directory at the target is refused before anything is staged", async () => { + const repo = await createRepo(); const target = resolve(repo.projectDir, "occupied.yaml"); await mkdir(target); - await expect( + const refusal = await refusalOf( writeTextFileAtomicIfChanged(target, "synthetic: content\n") - ).rejects.toThrow(); + ); + expect(refusal.reason).toBe("not_regular_file"); expect((await stat(target)).isDirectory()).toBe(true); - expect( - (await readdir(repo.projectDir)).filter((name) => name.endsWith(".tmp")) - ).toEqual([]); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); +}); + +const RUNS_AS_ROOT = + typeof process.getuid === "function" && process.getuid() === 0; + +/** Synthetic layer holding a key the failed write must not drop. */ +const OTHER_LAYER = + "version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global:\n OTHER_SETTING: synthetic-other-31b7\n"; + +// Root bypasses permission bits, so these refusals only apply to other users. +test.skipIf(RUNS_AS_ROOT)( + "an existing layer that cannot be read is refused, not replaced", + async () => { + const repo = await createRepo(); + await writeFile(repo.envFile, OTHER_LAYER); + await chmod(repo.envFile, 0o000); + try { + const refusal = await refusalOf(setSecret(repo, FIRST)); + expect(refusal.reason).toBe("unreadable"); + expect(refusal.message).not.toContain(FIRST); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); + } finally { + await chmod(repo.envFile, 0o600); + } + expect(await readFile(repo.envFile, "utf8")).toBe(OTHER_LAYER); + } +); + +test.skipIf(RUNS_AS_ROOT)( + "a readable layer that is not writable is refused, not replaced", + async () => { + const repo = await createRepo(); + await writeFile(repo.envFile, OTHER_LAYER); + await chmod(repo.envFile, 0o444); + const before = await stat(repo.envFile); + + const refusal = await refusalOf(setSecret(repo, FIRST)); + expect(refusal.reason).toBe("not_writable"); + expect(await readFile(repo.envFile, "utf8")).toBe(OTHER_LAYER); + const after = await stat(repo.envFile); + expect(after.ino).toBe(before.ino); + expect(after.mode & 0o777).toBe(0o444); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); + } +); + +test("a rewrite keeps the layer's group", async () => { + const repo = await createRepo(); + await setSecret(repo, FIRST); + const current = (await stat(repo.envFile)).gid; + // Any other group this process belongs to can be assigned without root. + const other = (process.getgroups?.() ?? []).find((gid) => gid !== current); + if (other === undefined) { + return; + } + await chown(repo.envFile, (await stat(repo.envFile)).uid, other); + + expect((await setSecret(repo, SECOND)).changed).toBe(true); + expect((await stat(repo.envFile)).gid).toBe(other); + expect(await resolvedToken(repo)).toBe(SECOND); +}); + +test("a dangling symlink layer is replaced by a regular file", async () => { + const repo = await createRepo(); + await symlink(resolve(repo.root, "missing.yaml"), repo.envFile); + + expect((await setSecret(repo, FIRST)).changed).toBe(true); + expect((await lstat(repo.envFile)).isFile()).toBe(true); + expect(await resolvedToken(repo)).toBe(FIRST); }); +async function refusalOf( + pending: Promise +): Promise { + try { + await pending; + } catch (error: unknown) { + if (error instanceof AtomicWriteRefusedError) { + return error; + } + throw error; + } + throw new Error("Expected the write to be refused"); +} + +async function tmpSiblings(directory: string): Promise { + return (await readdir(directory)).filter((name) => name.endsWith(".tmp")); +} + for (const layer of ["tracked", "local"] as const) { test(`a ${layer} write killed before its commit rename leaves the layer resolvable and git clean`, async () => { await assertKilledWriteIsRecoverable(layer); @@ -439,3 +555,34 @@ test("repeat `env set --secret --value-stdin` leaves git status clean", async () " M .hack/hack.env.default.yaml\n" ); }, 60_000); + +test.skipIf(RUNS_AS_ROOT)( + "`env set` on an unreadable layer fails and keeps the layer", + async () => { + const repo = await createRepo(); + await writeFile(repo.envFile, OTHER_LAYER); + await chmod(repo.envFile, 0o000); + let exitCode: number; + try { + exitCode = await runCli( + repo, + [ + "env", + "set", + "API_TOKEN", + "--secret", + "--value-stdin", + "--path", + repo.projectRoot, + ], + `${FIRST}\n` + ); + } finally { + await chmod(repo.envFile, 0o600); + } + expect(exitCode).not.toBe(0); + expect(await readFile(repo.envFile, "utf8")).toBe(OTHER_LAYER); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); + }, + 60_000 +); From e99f68c64a97b7991954f2291e1b356a93ffdff3 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Tue, 20 Oct 2026 09:14:37 -0400 Subject: [PATCH 9/9] test(env): cover the refusal when a layer's owner cannot be kept The owner_not_preserved refusal in writeTextFileAtomicIfChanged had no test, because a non-root user cannot create a file owned by someone else. A disposable child now reports the target as owned by another uid, so the staging file's fchown really fails with EPERM. The test expects the closed refusal, the old bytes and inode kept, and no staging file left. Co-Authored-By: Claude Opus 5.5 --- .../atomic-write-foreign-owner-worker.ts | 48 +++++++++++++++++++ tests/project-env-stable-ciphertext.test.ts | 39 +++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 tests/fixtures/atomic-write-foreign-owner-worker.ts diff --git a/tests/fixtures/atomic-write-foreign-owner-worker.ts b/tests/fixtures/atomic-write-foreign-owner-worker.ts new file mode 100644 index 000000000..7b7f907c7 --- /dev/null +++ b/tests/fixtures/atomic-write-foreign-owner-worker.ts @@ -0,0 +1,48 @@ +import { mock } from "bun:test"; +import * as fs from "node:fs/promises"; + +/** + * Runs one `writeTextFileAtomicIfChanged` in a disposable child where the + * target appears to belong to another user: `stat` of the target reports the + * caller's uid plus one, so the staging file must be `fchown`ed to a user the + * caller is not. Run as a non-root user, that `fchown` really fails with EPERM; + * nothing else is mocked. Prints `refused:` for an + * `AtomicWriteRefusedError`, `failed` for any other rejection and `committed` + * if the write succeeds. The content is a synthetic test string passed on + * stdin, never in argv. + */ +const [target] = process.argv.slice(2); +if (!target) { + throw new Error("Missing atomic write worker target"); +} + +// Captured before mocking: the namespace binding then points at the mock. +const realStat = fs.stat; + +mock.module("node:fs/promises", () => ({ + ...fs, + stat: async (...args: Parameters) => { + const info = await realStat(...args); + if (args[0] !== target) { + return info; + } + return Object.assign(Object.create(Object.getPrototypeOf(info)), info, { + uid: Number(info.uid) + 1, + }); + }, +})); + +const content = await Bun.stdin.text(); +const { AtomicWriteRefusedError, writeTextFileAtomicIfChanged } = await import( + "../../src/lib/fs.ts" +); +try { + await writeTextFileAtomicIfChanged(target, content); + process.stdout.write("committed\n"); +} catch (error: unknown) { + process.stdout.write( + error instanceof AtomicWriteRefusedError + ? `refused:${error.reason}\n` + : "failed\n" + ); +} diff --git a/tests/project-env-stable-ciphertext.test.ts b/tests/project-env-stable-ciphertext.test.ts index 131919b2a..46b5442ee 100644 --- a/tests/project-env-stable-ciphertext.test.ts +++ b/tests/project-env-stable-ciphertext.test.ts @@ -48,6 +48,10 @@ const RENAME_FAILURE_WORKER = resolve( import.meta.dir, "fixtures/atomic-write-rename-failure-worker.ts" ); +const FOREIGN_OWNER_WORKER = resolve( + import.meta.dir, + "fixtures/atomic-write-foreign-owner-worker.ts" +); const FIRST = "synthetic-first-6f1c"; const SECOND = "synthetic-second-92ad"; @@ -401,6 +405,41 @@ test("a rewrite keeps the layer's group", async () => { expect(await resolvedToken(repo)).toBe(SECOND); }); +// Root can chown to any user, so the refusal only applies to other users. +test.skipIf(RUNS_AS_ROOT)( + "a layer whose owner cannot be kept is refused, not replaced", + async () => { + const repo = await createRepo(); + await writeFile(repo.envFile, OTHER_LAYER); + const before = await stat(repo.envFile); + + const worker = Bun.spawn( + [process.execPath, FOREIGN_OWNER_WORKER, repo.envFile], + { + cwd: repo.projectRoot, + env: privateCliEnv({ root: repo.root }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + } + ); + worker.stdin.write("synthetic: after\n"); + await worker.stdin.end(); + const [stdout, exitCode] = await Promise.all([ + new Response(worker.stdout).text(), + worker.exited, + ]); + expect({ stdout, exitCode }).toEqual({ + stdout: "refused:owner_not_preserved\n", + exitCode: 0, + }); + expect(await readFile(repo.envFile, "utf8")).toBe(OTHER_LAYER); + expect((await stat(repo.envFile)).ino).toBe(before.ino); + expect(await tmpSiblings(repo.projectDir)).toEqual([]); + }, + 30_000 +); + test("a dangling symlink layer is replaced by a regular file", async () => { const repo = await createRepo(); await symlink(resolve(repo.root, "missing.yaml"), repo.envFile);