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 diff --git a/docs/env.md b/docs/env.md index 35af7494f..69f7a44e5 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: @@ -412,6 +413,37 @@ 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 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 +- 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/plugins/hack/skills/hack-cli/references/patterns.md b/plugins/hack/skills/hack-cli/references/patterns.md index 6840c5e0d..0c10542cc 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 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 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 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: `03a667cfa6a4`. Generated by Hack; edit the canonical source and regenerate the plugin. +Content revision: `247c8f20c3c8`. 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 b0797f73a..b691a4f1f 100644 --- a/src/agents/pattern-catalogue.ts +++ b/src/agents/pattern-catalogue.ts @@ -270,20 +270,23 @@ 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 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 only the local layer and keeps values 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: - "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, e2eScenario: null, 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/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 f289e353a..8d4186465 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -1,4 +1,17 @@ -import { lstat, mkdir, stat } from "node:fs/promises"; +import { randomUUID } from "node:crypto"; +import { + access, + 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 +102,212 @@ 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. 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. + * + * 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 inspectAtomicWriteTarget(absolutePath); + if (existing.kind === "existing" && existing.text === content) { + return { changed: false }; + } + const target = + 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` + ); + const file = await open( + staging, + constants.O_CREAT | + constants.O_EXCL | + constants.O_NOFOLLOW | + constants.O_WRONLY, + 0o666 + ); + let committed = false; + try { + 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(); + await rename(staging, target); + committed = true; + } finally { + await file.close(); + if (!committed) { + await unlink(staging).catch(() => undefined); + } + } + await syncDirectoryBestEffort(dirname(target)); + 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 { + 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 20537975a..ede60187e 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 { @@ -2260,6 +2261,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; @@ -2312,12 +2366,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({ @@ -2425,7 +2478,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/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/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/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/fixtures/project-env-set-worker.ts b/tests/fixtures/project-env-set-worker.ts new file mode 100644 index 000000000..154a3393e --- /dev/null +++ b/tests/fixtures/project-env-set-worker.ts @@ -0,0 +1,45 @@ +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. The optional + * third argument, `tracked` (default) or `local`, selects the layer written. + */ +const [projectRoot, projectDir, layer = "tracked"] = process.argv.slice(2); +if ( + !(projectRoot && projectDir && (layer === "tracked" || layer === "local")) +) { + 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, + 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/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({ diff --git a/tests/pattern-catalogue.test.ts b/tests/pattern-catalogue.test.ts index 4aca9e395..d4bb37be1 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", @@ -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 new file mode 100644 index 000000000..46b5442ee --- /dev/null +++ b/tests/project-env-stable-ciphertext.test.ts @@ -0,0 +1,627 @@ +import { afterEach, expect, test } from "bun:test"; +import { + chmod, + chown, + lstat, + mkdir, + mkdtemp, + readdir, + readFile, + rm, + stat, + symlink, + 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 { + AtomicWriteRefusedError, + 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"; + +/** + * 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 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"; + +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 }), + }; +} + +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, + envName: null, + scope: "global", + key: "API_TOKEN", + value, + secret: true, + local: layer === "local", + }); +} + +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 symlinked layer keeps its link and the write lands at the link target", async () => { + const repo = await createRepo(); + 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(); + 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); + + const refusal = await refusalOf( + writeTextFileAtomicIfChanged(target, "synthetic: content\n") + ); + expect(refusal.reason).toBe("not_regular_file"); + expect((await stat(target)).isDirectory()).toBe(true); + 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); +}); + +// 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); + + 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); + }, 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, layer], + { + 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(layerFile)).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(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, layer); + expect(retried.changed).toBe(true); + expect(await resolvedToken(repo)).toBe(SECOND); + expect(storedCiphertext(await readFile(layerFile, "utf8"))).not.toBe( + storedCiphertext(staged) + ); +} + +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); + +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 +);