From 871debc2331991592d0c81936ff4257d02a5899a Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 14:06:45 +0000 Subject: [PATCH 01/22] feat(secrets): retire a store whose key is gone, and fail doctor only where the store is read (HACK-2177) A machine with an old global encrypted store whose key file is gone made hack doctor fail in every project, with no way forward but restoring the key. - hack secrets retire-store moves the store aside to .retired- in the same directory (a hard link, then the old name is removed, under the store lock). It never deletes, reads or prints an entry, leaves the key file alone, prints how to put the store back, and needs --yes when it cannot prompt. It refuses a store that opens, is absent, or is unreadable for another reason. - The secret integrity finding for a store with a missing or wrong key is an error only in a project whose runtime reads the store (a store-backed legacy contract value, or plaintext kept in the backend), and a warning elsewhere. It says the entries cannot be counted without the key, names the registered projects that read the store, and gives both choices. - Retired files are reported as information (the check stays ok), with an mv command using absolute paths. - The recovery workflow gets a "Choose one" step naming both choices. - Docs: env guide, upgrade guide section, CLI reference. Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 10 +- docs/reference/cli.md | 28 ++ docs/versions/v5/guides/upgrade-from-v4.md | 27 ++ src/commands/doctor.ts | 12 +- src/commands/recovery-guidance.ts | 42 +++ src/commands/secrets.ts | 89 ++++++ src/lib/doctor-secret-integrity.ts | 128 ++++++-- src/lib/secret-store-retire.ts | 158 ++++++++++ src/lib/secret-store.ts | 113 ++++++- tests/missing-project-error-code.test.ts | 5 + tests/secret-store-retire.test.ts | 350 +++++++++++++++++++++ 11 files changed, 935 insertions(+), 27 deletions(-) create mode 100644 src/lib/secret-store-retire.ts create mode 100644 tests/secret-store-retire.test.ts diff --git a/docs/env.md b/docs/env.md index f8d50dc28..bf11103b9 100644 --- a/docs/env.md +++ b/docs/env.md @@ -1655,7 +1655,15 @@ Some older surfaces are still compatibility-oriented: before it lands, so a concurrent writer's key is kept, never overwritten. The process-wide key cache holds only keychain material, so one store's key file never stands in for another store's key. `hack doctor` reports an unreadable or refused - store, a missing or refused store key, and a key that does not open the store. Explicit key + store, a missing or refused store key, and a key that does not open the store. + A store whose key is missing or wrong is an error only in a project that reads + it (its legacy env contract declares a store-backed value, or plaintext is kept + in the backend), and a warning elsewhere. The finding names the registered + projects that read the store, and offers two ways forward: restore the key, or + run `hack secrets retire-store`. That command renames the store to + `.retired-` beside itself, deletes and prints nothing, and says + how to move it back. It needs `--yes` without a terminal. Doctor then lists + retired files as information. Explicit key provisioning also uses an atomic mode-`0600` replacement and refuses symlinked key paths or immediate parent directories. A key generated on first use is published only when no key file exists, so concurrent first users adopt the key diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 818c116ab..d435c68e7 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -2338,6 +2338,7 @@ hack secrets [options] | `hack secrets set [name]` | Store a secret | | `hack secrets get [name]` | Print a secret (exit 1 if missing) | | `hack secrets delete [name]` | Delete a stored secret | +| `hack secrets retire-store` | Move an encrypted secret store whose key is gone aside (reversible) | ### Options @@ -2427,6 +2428,33 @@ the deletion is refused with E_INTERACTIVE_REQUIRED unless --yes is passed. | `--help, -h` | Show help | | `--version, -v` | Show version | +## `hack secrets retire-store` + +Move an encrypted secret store whose key is gone aside (reversible) + +### Usage + +```bash +hack secrets retire-store [options] +``` + +For an encrypted_file (or cloud shim) store that hack doctor reports with a missing or wrong key. +Renames the store to .retired- in the same directory: nothing is deleted, read or printed, +and the key file is left alone. It prints how to put the store back. The store is the one this project +selects (--path or the current directory), or the global one outside a project. Refused for a store that +opens with the key in use, or that is unreadable for another reason. Without a TTY (or with +HACK_NO_INTERACTIVE / --no-interactive) it is refused with E_INTERACTIVE_REQUIRED unless --yes is passed. + +### Options + +| Option | Description | +| --- | --- | +| `--path, -p ` | Run a project command against a repo path (overrides cwd search) | +| `--yes` | Confirm moving the store aside without prompting (required without a TTY or with HACK_NO_INTERACTIVE) | +| `--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 | + ## Integrations diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index a28c33f4e..290d0c381 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -374,6 +374,33 @@ an explicit `env get KEY` is a deliberate plaintext read. Follow that PR's migration notes when installing a release that contains it. Do not use the new output contract or assume the protection command exists in next.16. +#### An old global secret store whose key is gone + +A machine upgraded from v4 may still have an encrypted store such as +`~/.hack/secrets.enc.json` whose key file was lost. From next.17 (HACK-2169, +HACK-2177), Doctor reports that store as a `secret integrity` finding: + +- an **error** in a project whose legacy env contract reads the store, and a + **warning** in any other project; +- the finding names the registered projects that read the store. Without the + key, its entries cannot be counted or read. + +There are two ways forward. Do not provision a new key: it cannot open these +entries. + +1. **Restore the key.** Put the key file back where the finding says (mode + 600), from a backup or another machine, or set `HACK_SECRETS_FILE_KEY` to the + same key. +2. **Retire the store** when the key is gone for good: + `hack secrets retire-store`, run in a project that selects the store or + outside any project for the global one. Without a terminal, add `--yes`. + - It renames the store to `.retired-` in the same + directory. It deletes nothing, reads and prints no entry, and leaves the + key file alone. + - It prints the `mv` command that puts the store back. + - Afterwards Doctor lists the retired file as information, and the project + check no longer fails. + Sources for next.16 behavior: [env list and get](https://github.com/hack-dance/hack/blob/b21c3b189841c1a3cbb8af2b76414fa347dc7057/src/commands/env.ts), [project key checks](https://github.com/hack-dance/hack/blob/b21c3b189841c1a3cbb8af2b76414fa347dc7057/src/lib/doctor-env-key.ts). diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 2e73f5175..a90fd6731 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -3281,11 +3281,16 @@ async function checkSecretIntegrity({ message: "Every stored secret opens with the key in use", }; } + // Information alone (a retired store kept beside its old name) is ok. + let status: CheckResult["status"] = "ok"; + if (findings.some((finding) => finding.status === "error")) { + status = "error"; + } else if (findings.some((finding) => finding.status === "warn")) { + status = "warn"; + } return { name: SECRET_INTEGRITY_CHECK, - status: findings.some((finding) => finding.status === "error") - ? "error" - : "warn", + status, message: findings.map((finding) => finding.message).join("\n"), }; } @@ -5948,6 +5953,7 @@ async function renderRecoveryGuidance( const hasActions = guidance.temporaryBreakage.length > 0 || guidance.configurationRepair.length > 0 || + (guidance.choices?.length ?? 0) > 0 || guidance.followUp.length > 0; if (!hasActions) { diff --git a/src/commands/recovery-guidance.ts b/src/commands/recovery-guidance.ts index ec931a40d..e9c291e67 100644 --- a/src/commands/recovery-guidance.ts +++ b/src/commands/recovery-guidance.ts @@ -9,12 +9,25 @@ export type RecoveryCheckResult = { export type DoctorRecoveryGuidance = { readonly temporaryBreakage: readonly string[]; readonly configurationRepair: readonly string[]; + /** + * Alternatives the operator chooses between (one is enough), as plain + * text; for example the two ways forward for a secret store whose key is + * missing (HACK-2177). + */ + readonly choices?: readonly string[]; readonly followUp: readonly string[]; readonly verify: readonly string[]; readonly capture: readonly string[]; }; const FIX_CHECKS = new Set(["caddy local ca", "dns:hack", "dns:hack.gy"]); +/** The secret integrity finding for a store whose key is missing names this command. */ +const RETIRE_STORE_COMMAND = "hack secrets retire-store"; +/** The two ways forward for such a store, shown as one choice (HACK-2177). */ +const SECRET_STORE_CHOICES = [ + "Restore the encrypted secret store's key: put the key file back where the secret integrity check says (mode 600), or set HACK_SECRETS_FILE_KEY to the same key.", + `Or, if the key is gone for good, retire the store: \`${RETIRE_STORE_COMMAND}\` moves it aside to a timestamped name beside it, deletes nothing, and prints how to put it back.`, +] as const; const SAFE_SHELL_ARG_PATTERN = /^[A-Za-z0-9_./:-]+$/u; const PROJECTS_PRUNE_COMMAND_PATTERN = /hack projects prune(?: --project [A-Za-z0-9_.-]+)?/u; @@ -61,6 +74,7 @@ export function buildDoctorRecoveryGuidance(input: { }): DoctorRecoveryGuidance { const temporaryBreakage: string[] = []; const configurationRepair: string[] = []; + const choices: string[] = []; const followUp: string[] = []; for (const result of input.results) { @@ -72,6 +86,18 @@ export function buildDoctorRecoveryGuidance(input: { continue; } + if ( + result.name === "secret integrity" && + result.message.includes(RETIRE_STORE_COMMAND) + ) { + for (const choice of SECRET_STORE_CHOICES) { + pushUnique(choices, choice); + } + // Any other finding in the same check still needs a look. + followUp.push(`${result.name}: ${result.message}`); + continue; + } + if (result.message.includes("hack global up")) { pushUnique(temporaryBreakage, "hack global up"); continue; @@ -133,6 +159,7 @@ export function buildDoctorRecoveryGuidance(input: { return { temporaryBreakage, configurationRepair, + choices, followUp, verify: ["hack doctor"], capture: ["hack crash-capture --path "], @@ -150,6 +177,7 @@ export function scopeDoctorRecoveryGuidance(input: { configurationRepair: input.guidance.configurationRepair.map((command) => scopeRecoveryCommand({ command, projectRoot: input.projectRoot }) ), + choices: [...(input.guidance.choices ?? [])], followUp: [...input.guidance.followUp], verify: input.guidance.verify.map((command) => scopeRecoveryCommand({ command, projectRoot: input.projectRoot }) @@ -188,6 +216,11 @@ export function buildRecoveryNextSteps(input: { nextSteps.push(`Configuration repair: \`${command}\`.`); } + const choices = scoped.choices ?? []; + if (choices.length > 0) { + nextSteps.push(`Choose one: ${choices.join(" ")}`); + } + for (const item of scoped.followUp) { nextSteps.push(`Manual follow-up: ${item}`); } @@ -244,6 +277,15 @@ export function buildRecoveryWorkflowLines(input: { stepNumber += 1; } + const choices = scoped.choices ?? []; + if (choices.length > 0) { + lines.push(`${stepNumber}. Choose one:`); + for (const choice of choices) { + lines.push(` - ${choice}`); + } + stepNumber += 1; + } + if (scoped.followUp.length > 0) { lines.push(`${stepNumber}. Investigate:`); for (const item of scoped.followUp) { diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index 9379b0c12..c00697929 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -1,12 +1,17 @@ +import { resolve } from "node:path"; + import { isCancel, password, text } from "@clack/prompts"; import type { CliContext, CommandArgs } from "../cli/command.ts"; import { defineCommand, defineOption, withHandler } from "../cli/command.ts"; +import { optPath } from "../cli/options.ts"; import { canPrompt, confirmSafe, requireInteractive, } from "../lib/interactivity.ts"; import { keychain } from "../lib/keychain.ts"; +import { findProjectContext, readProjectConfig } from "../lib/project.ts"; +import { retireSecretStore } from "../lib/secret-store-retire.ts"; import { logger } from "../ui/logger.ts"; const DEFAULT_SECRETS_SERVICE = "hack-cli" as const; @@ -28,8 +33,18 @@ const optYes = defineOption({ "Confirm the deletion without prompting (required without a TTY or with HACK_NO_INTERACTIVE)", } as const); +const optRetireYes = defineOption({ + name: "yes", + type: "boolean", + long: "--yes", + description: + "Confirm moving the store aside without prompting (required without a TTY or with HACK_NO_INTERACTIVE)", +} as const); + const secretsOptions = [optService] as const; const deleteOptions = [optService, optYes] as const; +const retireStoreOptions = [optPath, optRetireYes] as const; +type RetireStoreArgs = CommandArgs; const secretsPositionals = [{ name: "name", required: false }] as const; type SecretsArgs = CommandArgs< @@ -72,6 +87,24 @@ const deleteSpec = defineCommand({ subcommands: [], } as const); +const retireStoreSpec = defineCommand({ + name: "retire-store", + summary: + "Move an encrypted secret store whose key is gone aside (reversible)", + group: "Secrets", + description: [ + "For an encrypted_file (or cloud shim) store that hack doctor reports with a missing or wrong key.", + "Renames the store to .retired- in the same directory: nothing is deleted, read or printed,", + "and the key file is left alone. It prints how to put the store back. The store is the one this project", + "selects (--path or the current directory), or the global one outside a project. Refused for a store that", + "opens with the key in use, or that is unreadable for another reason. Without a TTY (or with", + "HACK_NO_INTERACTIVE / --no-interactive) it is refused with E_INTERACTIVE_REQUIRED unless --yes is passed.", + ].join("\n"), + options: retireStoreOptions, + positionals: [], + subcommands: [], +} as const); + export const secretsCommand = defineCommand({ name: "secrets", summary: "Manage secrets in OS keychain (Bun.secrets)", @@ -83,6 +116,7 @@ export const secretsCommand = defineCommand({ withHandler(setSpec, handleSecretsSet), withHandler(getSpec, handleSecretsGet), withHandler(deleteSpec, handleSecretsDelete), + withHandler(retireStoreSpec, handleSecretsRetireStore), ], } as const); @@ -216,3 +250,58 @@ async function handleSecretsDelete({ logger.success({ message: `Deleted secret "${name}" (${service})` }); return 0; } + +/** + * `hack secrets retire-store` (HACK-2177): the reversible way forward for an + * encrypted secret store whose key is gone. Confirmation works as for + * `delete`: `--yes`, a prompt (default no), or a closed refusal when the run + * cannot prompt, before anything is moved. + */ +async function handleSecretsRetireStore({ + args, +}: { + readonly ctx: CliContext; + readonly args: RetireStoreArgs; +}): Promise { + const startDir = args.options.path + ? resolve(process.cwd(), args.options.path) + : process.cwd(); + const project = await findProjectContext(startDir); + let projectName = "project"; + if (project) { + const cfg = await readProjectConfig(project); + projectName = ( + cfg.name ?? + project.projectRoot.split("/").at(-1) ?? + "project" + ).trim(); + } + const ok = + args.options.yes === true || + (await confirmSafe({ + message: + "Move the encrypted secret store aside? Its entries stay in the moved file, which can be put back.", + initialValue: false, + nonInteractive: "fail", + hint: "Pass --yes to confirm (`hack secrets retire-store --yes`), or run it in a terminal.", + })); + if (!ok) { + return 0; + } + const outcome = await retireSecretStore({ + projectName, + projectDir: project?.projectDir, + }); + if (outcome.kind === "refused") { + logger.error({ message: outcome.message }); + return 1; + } + logger.success({ + message: [ + `Moved the encrypted secret store ${outcome.from} to ${outcome.to}.`, + "Nothing was deleted, and no entry was read or printed. The key file was left alone.", + `To put it back (while no store is at ${outcome.from}): mv '${outcome.to}' '${outcome.from}'`, + ].join("\n"), + }); + return 0; +} diff --git a/src/lib/doctor-secret-integrity.ts b/src/lib/doctor-secret-integrity.ts index 014873a8e..f80cb9f07 100644 --- a/src/lib/doctor-secret-integrity.ts +++ b/src/lib/doctor-secret-integrity.ts @@ -1,14 +1,21 @@ -import { homedir } from "node:os"; import { isAbsolute, relative, sep } from "node:path"; import { PROJECT_ENV_KEY_FILENAME } from "../constants.ts"; +import { resolveUserHomeDir } from "./home-dir.ts"; import { inspectProjectEnvLayerKeys, PROJECT_ENV_SECRET_KEY_ENV, type ProjectEnvLayerInspectionFailure, type ProjectEnvLayerKeyReport, } from "./project-env-config.ts"; -import { listFileSecretStoreEntries } from "./secret-store.ts"; +import { + listFileSecretStoreEntries, + listRetiredEncryptedStores, +} from "./secret-store.ts"; +import { + listProjectsUsingSecretStore, + projectUsesSecretStore, +} from "./secret-store-retire.ts"; /** * `hack doctor`'s secret integrity findings (HACK-2169): stores and env @@ -16,10 +23,14 @@ import { listFileSecretStoreEntries } from "./secret-store.ts"; * value-free: paths, counts and fixed text only, never a value or key. */ export type SecretIntegrityFinding = { - readonly status: "warn" | "error"; + /** `info` is not a problem: it is shown with an `ok` result. */ + readonly status: "info" | "warn" | "error"; readonly message: string; }; +/** The command that moves a store whose key is gone aside (HACK-2177). */ +export const RETIRE_STORE_COMMAND = "hack secrets retire-store"; + /** A path for display: project-relative inside it, `~/...` under home. */ function displayPath(opts: { readonly projectRoot: string; @@ -34,7 +45,7 @@ function displayPath(opts: { ) { return inside; } - const home = homedir(); + const home = resolveUserHomeDir(); const underHome = relative(home, opts.path); if (underHome && !underHome.startsWith("..") && !isAbsolute(underHome)) { return `~/${underHome}`; @@ -52,12 +63,101 @@ async function inspectSecretStore(opts: { projectName: opts.projectName, projectDir: opts.projectDir, }); + const retired = + listing.kind === "inactive" + ? [] + : await retiredStoreFindings({ ...opts, storePath: listing.storePath }); if (listing.kind !== "unreadable") { - return []; + return retired; } const store = displayPath({ ...opts, path: listing.storePath }); const key = displayPath({ ...opts, path: listing.keyPath }); - switch (listing.reason) { + if ( + listing.reason === "key_missing" || + listing.reason === "wrong_key_or_corrupt" + ) { + return [ + await keylessStoreFinding({ + ...opts, + storePath: listing.storePath, + store, + key, + missing: listing.reason === "key_missing", + }), + ...retired, + ]; + } + return [...storeFindings({ reason: listing.reason, store, key }), ...retired]; +} + +/** + * A store with entries that no available key opens (HACK-2177). An error + * only for a project whose runtime reads the store; for any other project + * the same machine-wide store is a warning. It says what is known without + * the key (entries cannot be counted), which registered projects read it, + * and the two ways forward: restore the key, or retire the store. + */ +async function keylessStoreFinding(opts: { + readonly projectRoot: string; + readonly projectDir: string; + readonly storePath: string; + readonly store: string; + readonly key: string; + readonly missing: boolean; +}): Promise { + const usedHere = await projectUsesSecretStore({ + projectDir: opts.projectDir, + }); + const users = await listProjectsUsingSecretStore({ + storePath: opts.storePath, + }); + const state = opts.missing + ? `The encrypted secret store ${opts.store} has entries, but its key ${opts.key} is missing.` + : `The available key does not open the encrypted secret store ${opts.store}, or the store is damaged.`; + const usage = [ + usedHere + ? "This project's env contract reads it." + : "This project does not read it.", + users.length > 0 + ? `Registered projects that read it: ${users.join(", ")}.` + : "No registered project reads it.", + ].join(" "); + return { + status: usedHere ? "error" : "warn", + message: `${state} Its entries cannot be counted or read without that key. ${usage} Choose one: restore the key (put the key file back at ${opts.key} with mode 600 from a backup or another machine, or set HACK_SECRETS_FILE_KEY to the same key); or, if the key is gone for good, run ${RETIRE_STORE_COMMAND}, which moves the store aside to a timestamped name beside it, deletes nothing, and prints how to put it back. Do not provision a new key: it cannot open these entries.`, + }; +} + +/** Retired copies kept beside the store: information, not a problem. */ +async function retiredStoreFindings(opts: { + readonly projectRoot: string; + readonly storePath: string; +}): Promise { + const store = displayPath({ ...opts, path: opts.storePath }); + return (await listRetiredEncryptedStores(opts.storePath)).map((path) => { + const shown = displayPath({ ...opts, path }); + // The command quotes absolute paths: a quoted `~` is not expanded. + return { + status: "info", + message: `Retired secret store kept at ${shown}; Hack no longer reads it. To put it back while no store is at ${store}: mv '${path}' '${opts.storePath}'.`, + }; + }); +} + +/** Findings for the remaining store states (unchanged from HACK-2169). */ +function storeFindings(opts: { + readonly reason: Exclude< + Extract< + Awaited>, + { kind: "unreadable" } + >["reason"], + "key_missing" | "wrong_key_or_corrupt" + >; + readonly store: string; + readonly key: string; +}): SecretIntegrityFinding[] { + const { store, key } = opts; + switch (opts.reason) { case "store_unreadable": return [ { @@ -72,13 +172,6 @@ async function inspectSecretStore(opts: { message: `The encrypted secret store ${store} is a symlink, not a regular file, or too large, so Hack refuses to read or change it. Replace it with a regular file this user owns (for a symlink: copy the target's contents to ${store} with mode 600), then re-run hack doctor.`, }, ]; - case "key_missing": - return [ - { - status: "error", - message: `The encrypted secret store ${store} has entries, but its key ${key} is missing. Restore that key file from a backup or another machine (mode 600), or set HACK_SECRETS_FILE_KEY to the same key. Do not provision a new key: it cannot open these entries.`, - }, - ]; case "key_refused": return [ { @@ -86,13 +179,6 @@ async function inspectSecretStore(opts: { message: `The key ${key} for ${store} is refused: it is a symlink, other users can read or write it, or git tracks it. Replace it with a regular, untracked file this user owns (chmod 600 ${key}; git rm --cached if tracked, then rotate it).`, }, ]; - case "wrong_key_or_corrupt": - return [ - { - status: "error", - message: `The available key does not open the encrypted secret store ${store}, or the store is damaged. Restore the key that encrypted it (or the store from a backup); do not provision a replacement key over it.`, - }, - ]; case "incomplete_config": return [ { @@ -215,6 +301,6 @@ export async function inspectSecretIntegrity(opts: { ...(await inspectEnvLayers(opts)), ]; const rank = (finding: SecretIntegrityFinding) => - finding.status === "error" ? 0 : 1; + ({ error: 0, warn: 1, info: 2 })[finding.status]; return findings.sort((a, b) => rank(a) - rank(b)); } diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts new file mode 100644 index 000000000..368544d81 --- /dev/null +++ b/src/lib/secret-store-retire.ts @@ -0,0 +1,158 @@ +import { resolve } from "node:path"; + +import { readHackEnvContract, readHackEnvRuntimeConfig } from "./hack-env.ts"; +import { readProjectsRegistry } from "./projects-registry.ts"; +import { + type FileSecretStoreListing, + listFileSecretStoreEntries, + resolveFileSecretStorePaths, + retireEncryptedStoreFile, + SecretStoreRetireError, +} from "./secret-store.ts"; + +/** + * Retiring an encrypted secret store whose key is gone (HACK-2177): a + * deliberate, reversible way forward after `hack doctor` reports a store it + * cannot open. Retiring only renames the store file beside itself; it never + * deletes, decrypts or prints an entry. + */ + +/** Store states retiring is for: entries exist and the key that opens them does not. */ +const RETIRABLE_REASONS = new Set(["key_missing", "wrong_key_or_corrupt"]); + +/** + * Whether a project's runtime reads its secret store: its legacy env + * contract declares a store-backed (`source: "keychain"`) value, or its + * configuration keeps plaintext in the backend. The same condition the + * runtime uses before it opens the store. A contract that cannot be read + * counts as using it, since what it declares is unknown. + */ +export async function projectUsesSecretStore(opts: { + readonly projectDir: string; +}): Promise { + try { + const runtime = await readHackEnvRuntimeConfig(opts); + if (runtime.storePlaintextInBackend) { + return true; + } + const contract = await readHackEnvContract(opts); + if (contract.parseError !== undefined) { + return true; + } + return contract.contract.vars.some( + (contractVar) => contractVar.source === "keychain" + ); + } catch { + return true; + } +} + +/** + * Names (only) of registered projects whose configuration selects the store + * at `storePath` and whose runtime reads it, from configuration only: no + * store is opened. Projects whose configuration cannot be read are skipped; + * a registry that cannot be read yields none. + */ +export async function listProjectsUsingSecretStore(opts: { + readonly storePath: string; +}): Promise { + let projects: readonly { + readonly name: string; + readonly projectDir: string; + }[]; + try { + projects = (await readProjectsRegistry()).projects; + } catch { + return []; + } + const names = new Set(); + for (const project of projects) { + let location: Awaited>; + try { + location = await resolveFileSecretStorePaths({ + projectDir: project.projectDir, + }); + } catch { + continue; + } + if ( + location.active && + resolve(location.storePath) === resolve(opts.storePath) && + (await projectUsesSecretStore({ projectDir: project.projectDir })) + ) { + names.add(project.name); + } + } + return [...names].sort(); +} + +/** + * Where a project's store is, and whether its backend is active. The + * listing reads the store only to classify it, and never provisions a key. + */ +async function storeLocation(opts: { + readonly projectName: string; + readonly projectDir?: string; +}): Promise { + return await listFileSecretStoreEntries(opts); +} + +/** The outcome of `hack secrets retire-store` (paths only, never values). */ +export type SecretStoreRetireOutcome = + | { + readonly kind: "retired"; + readonly from: string; + readonly to: string; + readonly keyPath: string; + } + | { readonly kind: "refused"; readonly message: string }; + +/** + * Checks that the store selected for this context (a project, or the global + * configuration when `projectDir` is omitted) is one retiring is for, then + * moves it aside ({@link retireEncryptedStoreFile}). Refused, with the + * reason and nothing moved, when no file store is configured, the store is + * absent or empty, it opens with the key in use, or it cannot be read for + * another reason (its key file refused, the store unreadable or a link). + */ +export async function retireSecretStore(opts: { + readonly projectName: string; + readonly projectDir?: string; + readonly now?: Date; +}): Promise { + const listing = await storeLocation(opts); + if (listing.kind === "inactive") { + return { + kind: "refused", + message: + "No encrypted file secret store is configured here (the backend is the OS keychain); nothing was moved.", + }; + } + if (listing.kind === "listed") { + return { + kind: "refused", + message: + listing.ownEntries.size + listing.foreignEntryCount === 0 + ? `There is no encrypted secret store with entries at ${listing.storePath}; nothing was moved.` + : `The encrypted secret store at ${listing.storePath} opens with the key in use, so there is nothing to retire; nothing was moved.`, + }; + } + if (!RETIRABLE_REASONS.has(listing.reason)) { + return { + kind: "refused", + message: `The encrypted secret store at ${listing.storePath} is not retired: it is not a missing or wrong key (${listing.reason.replaceAll("_", " ")}). Run hack doctor for what to fix; nothing was moved.`, + }; + } + try { + const moved = await retireEncryptedStoreFile({ + storePath: listing.storePath, + now: opts.now, + }); + return { kind: "retired", ...moved, keyPath: listing.keyPath }; + } catch (error: unknown) { + if (error instanceof SecretStoreRetireError) { + return { kind: "refused", message: error.message }; + } + throw error; + } +} diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index 5e7b46db7..0e78352ac 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -4,7 +4,7 @@ import { createHash, randomBytes, } from "node:crypto"; -import { lstat, realpath, stat } from "node:fs/promises"; +import { link, lstat, readdir, realpath, stat, unlink } from "node:fs/promises"; import { basename, dirname, resolve } from "node:path"; import { CliUsageError } from "../cli/command.ts"; import type { @@ -198,6 +198,114 @@ export type FileSecretStoreListing = { } ); +/** + * Where the encrypted file store (also the cloud shim's) for this context + * lives, from configuration only: nothing is read, opened or provisioned. + * `active` is false when the backend is the OS keychain. + */ +export async function resolveFileSecretStorePaths(input: { + /** Omitted: only the global configuration applies (no project). */ + readonly projectDir?: string; +}): Promise<{ + readonly active: boolean; + readonly storePath: string; + readonly keyPath: string; +}> { + const secretsConfig = ( + await readControlPlaneConfig({ projectDir: input.projectDir }) + ).config.secrets; + return { + active: + secretsConfig.backend === "encrypted_file" || + secretsConfig.backend === "cloud", + storePath: resolveConfiguredPath({ + path: secretsConfig.encryptedFile.path, + projectDir: input.projectDir, + }), + keyPath: resolveConfiguredPath({ + path: + secretsConfig.encryptedFile.keyPath ?? DEFAULT_ENCRYPTED_FILE_KEY_PATH, + projectDir: input.projectDir, + }), + }; +} + +/** Retired stores are kept beside the store as ``. */ +export const RETIRED_STORE_INFIX = ".retired-"; +const STAMP_SEPARATORS = /[-:]/g; +const STAMP_MILLISECONDS = /\.\d{3}Z$/; + +/** Thrown when a store cannot be moved aside; the message says why and what to do. */ +export class SecretStoreRetireError extends Error { + constructor(message: string) { + super(message); + this.name = "SecretStoreRetireError"; + } +} + +/** + * Moves an encrypted store file aside (HACK-2177), never deleting it: under + * the store's mutation lock, the regular file at `storePath` gets a second + * name `.retired-` in the same directory (a hard link, + * which never replaces an existing file), and only then is its original + * name removed. A link, a non-regular file or an absent store is refused. + * Nothing is read or decrypted; the key file is not touched. + */ +export async function retireEncryptedStoreFile(input: { + readonly storePath: string; + readonly now?: Date; +}): Promise<{ readonly from: string; readonly to: string }> { + const storePath = input.storePath; + return await withEncryptedStoreMutation({ + filePath: storePath, + run: async () => { + const info = await lstat(storePath).catch(() => null); + if (info === null) { + throw new SecretStoreRetireError( + `There is no encrypted secret store at ${storePath}; nothing was moved.` + ); + } + if (!info.isFile()) { + throw new SecretStoreRetireError( + `The encrypted secret store at ${storePath} is a symlink or not a regular file; nothing was moved. Inspect it with ls -l, then move it aside by hand if you mean to.` + ); + } + const stamp = (input.now ?? new Date()) + .toISOString() + .replace(STAMP_SEPARATORS, "") + .replace(STAMP_MILLISECONDS, "Z"); + const to = `${storePath}${RETIRED_STORE_INFIX}${stamp}`; + try { + await link(storePath, to); + } catch (error: unknown) { + const code = + typeof error === "object" && error !== null && "code" in error + ? String((error as { readonly code?: unknown }).code) + : "unknown"; + throw new SecretStoreRetireError( + code === "EEXIST" + ? `${to} already exists; nothing was moved. Retry in a second.` + : `Could not give ${storePath} the name ${to} (${code}); nothing was moved. To retire it by hand: mv '${storePath}' '${to}'` + ); + } + await unlink(storePath); + return { from: storePath, to }; + }, + }); +} + +/** Retired copies of `storePath` in its directory, oldest first. */ +export async function listRetiredEncryptedStores( + storePath: string +): Promise { + const prefix = `${basename(storePath)}${RETIRED_STORE_INFIX}`; + const entries = await readdir(dirname(storePath)).catch(() => [] as string[]); + return entries + .filter((name) => name.startsWith(prefix)) + .sort() + .map((name) => resolve(dirname(storePath), name)); +} + /** The listing's reason for each way a store file read can fail. */ const STORE_READ_FAILURE_REASON = { refused: "store_refused", @@ -212,7 +320,8 @@ const STORE_READ_FAILURE_REASON = { */ export async function listFileSecretStoreEntries(input: { readonly projectName: string; - readonly projectDir: string; + /** Omitted: only the global configuration applies (no project). */ + readonly projectDir?: string; }): Promise { const controlPlane = await readControlPlaneConfig({ projectDir: input.projectDir, diff --git a/tests/missing-project-error-code.test.ts b/tests/missing-project-error-code.test.ts index d5e96e630..7580eb3af 100644 --- a/tests/missing-project-error-code.test.ts +++ b/tests/missing-project-error-code.test.ts @@ -221,6 +221,11 @@ const COMMANDS: Readonly> = { "setup cursor": { kind: "not-a-project-command", reason: NOT_REQUIRED }, "setup claude": { kind: "not-a-project-command", reason: NOT_REQUIRED }, "setup codex": { kind: "not-a-project-command", reason: NOT_REQUIRED }, + // Outside a project it retires the global store (HACK-2177). + "secrets retire-store": { + kind: "not-a-project-command", + reason: NOT_REQUIRED, + }, "setup agents": { kind: "not-a-project-command", reason: NOT_REQUIRED }, "setup sync": { kind: "not-a-project-command", reason: NOT_REQUIRED }, "setup migrate": { kind: "not-a-project-command", reason: NOT_REQUIRED }, diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts new file mode 100644 index 000000000..03712fadb --- /dev/null +++ b/tests/secret-store-retire.test.ts @@ -0,0 +1,350 @@ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { + chmod, + lstat, + mkdir, + mkdtemp, + readdir, + readFile, + realpath, + rename, + rm, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; + +import { + buildDoctorRecoveryGuidance, + buildRecoveryWorkflowLines, +} from "../src/commands/recovery-guidance.ts"; +import { + PROJECT_COMPOSE_FILENAME, + PROJECT_CONFIG_FILENAME, + PROJECT_ENV_CONTRACT_FILENAME, +} from "../src/constants.ts"; +import { inspectSecretIntegrity } from "../src/lib/doctor-secret-integrity.ts"; +import { findProjectContext } from "../src/lib/project.ts"; +import { upsertProjectRegistration } from "../src/lib/projects-registry.ts"; +import { resolveSecretStore } from "../src/lib/secret-store.ts"; +import { retireSecretStore } from "../src/lib/secret-store-retire.ts"; +import { restoreEnv } from "./helpers/env.ts"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * HACK-2177: a global encrypted secret store whose key file is gone. Every + * case runs in a fake home (HOME, HACK_HOME and the global config inside a + * temp root); no real store, key, keychain or registry is touched, and + * nothing is deleted except the temp root itself. Values are synthetic. + */ + +const ENTRYPOINT = resolve(import.meta.dir, "..", "index.ts"); +const STORE_VALUE = ["synthetic", "store", "value", "6d1f"].join("-"); +const STORE_KEY = ["synthetic", "store", "key", "92ab"].join("-"); +const ISOLATED_ENV = [ + "HOME", + "HACK_HOME", + "HACK_GLOBAL_CONFIG_PATH", + "HACK_SECRETS_FILE_KEY", + "HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK", + "HACK_ENV_SECRET_KEY", +] as const; +const saved = new Map(); +let root = ""; + +type Fixture = { + readonly home: string; + readonly hackHome: string; + readonly storePath: string; + readonly keyPath: string; + readonly reader: { + readonly projectRoot: string; + readonly projectDir: string; + }; + readonly bystander: { + readonly projectRoot: string; + readonly projectDir: string; + }; +}; + +beforeEach(async () => { + for (const key of ISOLATED_ENV) { + saved.set(key, process.env[key]); + } + root = await realpath(await mkdtemp(join(tmpdir(), "hack-retire-store-"))); +}); + +afterEach(async () => { + for (const key of ISOLATED_ENV) { + restoreEnv(key, saved.get(key)); + } + await rm(root, { recursive: true, force: true }); +}); + +async function createProject(opts: { + readonly name: string; + readonly readsStore: boolean; +}) { + const projectRoot = join(root, "repos", opts.name); + const projectDir = join(projectRoot, ".hack"); + await mkdir(projectDir, { recursive: true }); + await writeFile( + join(projectDir, PROJECT_COMPOSE_FILENAME), + "services:\n api:\n image: alpine:3.20\n" + ); + await writeFile( + join(projectDir, PROJECT_CONFIG_FILENAME), + `${JSON.stringify({ name: opts.name, dev_host: `${opts.name}.hack` })}\n` + ); + if (opts.readsStore) { + // A legacy env contract with a store-backed value: runtime reads the store. + await writeFile( + join(projectDir, PROJECT_ENV_CONTRACT_FILENAME), + `${JSON.stringify({ version: 1, vars: [{ key: "TOKEN", source: "keychain" }] })}\n` + ); + } + const project = await findProjectContext(projectRoot); + if (!project) { + throw new Error(`fixture project ${opts.name} not found`); + } + await upsertProjectRegistration({ project }); + return { projectRoot, projectDir }; +} + +/** A fake home with a global store whose key file has been removed. */ +async function setUp(): Promise { + const home = join(root, "user-home"); + const hackHome = join(home, ".hack"); + await mkdir(hackHome, { recursive: true }); + const storePath = join(hackHome, "secrets.enc.json"); + const keyPath = join(hackHome, "secrets-file.key"); + process.env.HOME = home; + process.env.HACK_HOME = hackHome; + process.env.HACK_GLOBAL_CONFIG_PATH = join(hackHome, "hack.config.json"); + process.env.HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK = "1"; + Reflect.deleteProperty(process.env, "HACK_SECRETS_FILE_KEY"); + Reflect.deleteProperty(process.env, "HACK_ENV_SECRET_KEY"); + await writeFile( + process.env.HACK_GLOBAL_CONFIG_PATH, + `${JSON.stringify({ + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { path: storePath, keyPath }, + }, + }, + })}\n` + ); + const reader = await createProject({ name: "app-a", readsStore: true }); + const bystander = await createProject({ name: "app-b", readsStore: false }); + await writeFile(keyPath, `${STORE_KEY}\n`, { mode: 0o600 }); + await chmod(keyPath, 0o600); + await ( + await resolveSecretStore({ + projectName: "app-a", + projectDir: reader.projectDir, + }) + ).set({ key: "TOKEN", value: STORE_VALUE }); + await rm(keyPath); + return { home, hackHome, storePath, keyPath, reader, bystander }; +} + +async function inspect( + project: Fixture["reader"], + projectName: string +): Promise>> { + return await inspectSecretIntegrity({ ...project, projectName }); +} + +function expectValueFree(text: string) { + expect(text).not.toContain(STORE_VALUE); + expect(text).not.toContain(STORE_KEY); +} + +async function runRetire(opts: { + readonly fixture: Fixture; + readonly path: string; + readonly yes: boolean; +}) { + const child = Bun.spawn( + [ + process.execPath, + ENTRYPOINT, + "secrets", + "retire-store", + "--path", + opts.path, + ...(opts.yes ? ["--yes"] : []), + ], + { + cwd: root, + env: privateCliEnv({ + root, + home: opts.fixture.home, + env: { + HACK_HOME: opts.fixture.hackHome, + HACK_GLOBAL_CONFIG_PATH: join( + opts.fixture.hackHome, + "hack.config.json" + ), + HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK: "1", + HACK_NO_INTERACTIVE: "1", + }, + }), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + } + ); + const [stdout, stderr, code] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + return { code, output: `${stdout}\n${stderr}` }; +} + +test("a store whose key is missing is an error only for a project that reads it", async () => { + const fixture = await setUp(); + const reader = await inspect(fixture.reader, "app-a"); + expect(reader).toHaveLength(1); + expect(reader[0]?.status).toBe("error"); + expect(reader[0]?.message).toContain( + "its key ~/.hack/secrets-file.key is missing" + ); + expect(reader[0]?.message).toContain("This project's env contract reads it."); + + const bystander = await inspect(fixture.bystander, "app-b"); + expect(bystander).toHaveLength(1); + expect(bystander[0]?.status).toBe("warn"); + const message = bystander[0]?.message ?? ""; + expect(message).toContain("This project does not read it."); + expect(message).toContain("Registered projects that read it: app-a."); + expect(message).toContain("cannot be counted or read without that key"); + expect(message).toContain("restore the key"); + expect(message).toContain("hack secrets retire-store"); + expect(message).toContain("Do not provision a new key"); + expectValueFree(message); + + // The recovery block names the two choices. + const lines = buildRecoveryWorkflowLines({ + guidance: buildDoctorRecoveryGuidance({ + results: [{ name: "secret integrity", status: "warn", message }], + }), + projectRoot: null, + }).join("\n"); + expect(lines).toContain("Choose one:"); + expect(lines).toContain("Restore the encrypted secret store's key"); + expect(lines).toContain("`hack secrets retire-store`"); +}); + +test("retire-store refuses without --yes when it cannot prompt, then retires, and moving the file back restores it", async () => { + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + + // Non-interactive and no --yes: refused, nothing moved. + const refused = await runRetire({ + fixture, + path: fixture.bystander.projectRoot, + yes: false, + }); + expect(refused.code).not.toBe(0); + expect(refused.output).toContain("E_INTERACTIVE_REQUIRED"); + expect(refused.output).toContain("--yes"); + expect(await readFile(fixture.storePath)).toEqual(before); + + // With --yes: moved aside, never deleted or read. + const retired = await runRetire({ + fixture, + path: fixture.bystander.projectRoot, + yes: true, + }); + expect(retired.code).toBe(0); + expectValueFree(retired.output); + expect(await lstat(fixture.storePath).catch(() => null)).toBeNull(); + const kept = (await readdir(fixture.hackHome)).filter((name) => + name.startsWith("secrets.enc.json.retired-") + ); + expect(kept).toHaveLength(1); + const retiredPath = join(fixture.hackHome, kept[0] ?? ""); + expect(await readFile(retiredPath)).toEqual(before); + expect(retired.output).toContain(`to ${retiredPath}`); + expect(retired.output).toContain( + `mv '${retiredPath}' '${fixture.storePath}'` + ); + expect(retired.output).toContain("Nothing was deleted"); + + // doctor afterwards: the retired file is information, not a problem. + for (const [project, name] of [ + [fixture.bystander, "app-b"], + [fixture.reader, "app-a"], + ] as const) { + const after = await inspect(project, name); + expect(after).toHaveLength(1); + expect(after[0]?.status).toBe("info"); + expect(after[0]?.message).toContain( + `Retired secret store kept at ~/.hack/${kept[0]}` + ); + expect(after[0]?.message).toContain( + `mv '${retiredPath}' '${fixture.storePath}'` + ); + } + + // Putting it back by moving the file restores the original state. + await rename(retiredPath, fixture.storePath); + expect(await readFile(fixture.storePath)).toEqual(before); + const restored = await inspect(fixture.bystander, "app-b"); + expect(restored).toHaveLength(1); + expect(restored[0]?.status).toBe("warn"); + expect(restored[0]?.message).toContain("is missing"); + // With the key back, the store opens again: nothing was lost. + await writeFile(fixture.keyPath, `${STORE_KEY}\n`, { mode: 0o600 }); + await chmod(fixture.keyPath, 0o600); + expect(await inspect(fixture.reader, "app-a")).toEqual([]); + expect( + await ( + await resolveSecretStore({ + projectName: "app-a", + projectDir: fixture.reader.projectDir, + }) + ).get({ key: "TOKEN" }) + ).toBe(STORE_VALUE); +}, 60_000); + +test("retire-store refuses a store that opens, an absent store, and a key that is refused", async () => { + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + await writeFile(fixture.keyPath, `${STORE_KEY}\n`, { mode: 0o600 }); + await chmod(fixture.keyPath, 0o600); + const opens = await retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + expect(opens).toMatchObject({ kind: "refused" }); + expect(opens.kind === "refused" ? opens.message : "").toContain( + "opens with the key in use" + ); + + // A refused key (others can read it) is not a missing key: fix it instead. + await chmod(fixture.keyPath, 0o644); + const refusedKey = await retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + expect(refusedKey.kind === "refused" ? refusedKey.message : "").toContain( + "key refused" + ); + expect(await readFile(fixture.storePath)).toEqual(before); + + // No store at all: nothing to move. + await rm(fixture.keyPath); + await rename(fixture.storePath, join(root, "elsewhere.enc.json")); + const absent = await retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + expect(absent.kind === "refused" ? absent.message : "").toContain( + "There is no encrypted secret store with entries" + ); + expect(await readFile(join(root, "elsewhere.enc.json"))).toEqual(before); +}); From 3bc26d7cccf5a4b22dc0820dc01e0eca5f3d9a92 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 14:20:03 +0000 Subject: [PATCH 02/22] fix(secrets): re-check a store under its lock before retiring, and scope the retire command to the diagnosed project (HACK-2177) - retireEncryptedStoreFile takes a recheck run under the store lock just before the move; retireSecretStore classifies the store again there, so a store whose key was restored, or that was replaced, is not retired. - The doctor finding names hack secrets retire-store --path , the recovery block carries that exact command, and scoped guidance adds --path to an unscoped one. Co-Authored-By: Claude Opus 5.5 --- src/commands/recovery-guidance.ts | 31 +++++++++--- src/lib/doctor-secret-integrity.ts | 15 +++++- src/lib/secret-store-retire.ts | 55 ++++++++++++-------- src/lib/secret-store.ts | 10 ++++ tests/secret-store-retire.test.ts | 80 +++++++++++++++++++++++++++++- 5 files changed, 162 insertions(+), 29 deletions(-) diff --git a/src/commands/recovery-guidance.ts b/src/commands/recovery-guidance.ts index e9c291e67..43deac762 100644 --- a/src/commands/recovery-guidance.ts +++ b/src/commands/recovery-guidance.ts @@ -23,11 +23,17 @@ export type DoctorRecoveryGuidance = { const FIX_CHECKS = new Set(["caddy local ca", "dns:hack", "dns:hack.gy"]); /** The secret integrity finding for a store whose key is missing names this command. */ const RETIRE_STORE_COMMAND = "hack secrets retire-store"; +/** The command as the finding names it, with the diagnosed project's `--path`. */ +const RETIRE_STORE_COMMAND_PATTERN = + /hack secrets retire-store(?: --path (?:'(?:[^']|'"'"')*'|[A-Za-z0-9_./:-]+))?/u; + /** The two ways forward for such a store, shown as one choice (HACK-2177). */ -const SECRET_STORE_CHOICES = [ - "Restore the encrypted secret store's key: put the key file back where the secret integrity check says (mode 600), or set HACK_SECRETS_FILE_KEY to the same key.", - `Or, if the key is gone for good, retire the store: \`${RETIRE_STORE_COMMAND}\` moves it aside to a timestamped name beside it, deletes nothing, and prints how to put it back.`, -] as const; +function secretStoreChoices(command: string): readonly string[] { + return [ + "Restore the encrypted secret store's key: put the key file back where the secret integrity check says (mode 600), or set HACK_SECRETS_FILE_KEY to the same key.", + `Or, if the key is gone for good, retire the store: \`${command}\` moves it aside to a timestamped name beside it, deletes nothing, and prints how to put it back.`, + ]; +} const SAFE_SHELL_ARG_PATTERN = /^[A-Za-z0-9_./:-]+$/u; const PROJECTS_PRUNE_COMMAND_PATTERN = /hack projects prune(?: --project [A-Za-z0-9_.-]+)?/u; @@ -90,7 +96,10 @@ export function buildDoctorRecoveryGuidance(input: { result.name === "secret integrity" && result.message.includes(RETIRE_STORE_COMMAND) ) { - for (const choice of SECRET_STORE_CHOICES) { + const command = + result.message.match(RETIRE_STORE_COMMAND_PATTERN)?.[0] ?? + RETIRE_STORE_COMMAND; + for (const choice of secretStoreChoices(command)) { pushUnique(choices, choice); } // Any other finding in the same check still needs a look. @@ -177,7 +186,17 @@ export function scopeDoctorRecoveryGuidance(input: { configurationRepair: input.guidance.configurationRepair.map((command) => scopeRecoveryCommand({ command, projectRoot: input.projectRoot }) ), - choices: [...(input.guidance.choices ?? [])], + // A retire command without --path acts on the current directory: scope + // it to the diagnosed project, as the other project commands are. + choices: (input.guidance.choices ?? []).map((choice) => + input.projectRoot?.trim() && + choice.includes(`\`${RETIRE_STORE_COMMAND}\``) + ? choice.replace( + `\`${RETIRE_STORE_COMMAND}\``, + `\`${RETIRE_STORE_COMMAND} --path ${quoteShellArg(input.projectRoot.trim())}\`` + ) + : choice + ), followUp: [...input.guidance.followUp], verify: input.guidance.verify.map((command) => scopeRecoveryCommand({ command, projectRoot: input.projectRoot }) diff --git a/src/lib/doctor-secret-integrity.ts b/src/lib/doctor-secret-integrity.ts index f80cb9f07..9eae12679 100644 --- a/src/lib/doctor-secret-integrity.ts +++ b/src/lib/doctor-secret-integrity.ts @@ -31,6 +31,19 @@ export type SecretIntegrityFinding = { /** The command that moves a store whose key is gone aside (HACK-2177). */ export const RETIRE_STORE_COMMAND = "hack secrets retire-store"; +const SAFE_SHELL_ARG = /^[A-Za-z0-9_./:-]+$/u; + +/** + * The retire command for the store this project selects: with `--path`, + * since it otherwise acts on the current directory's project. + */ +function retireStoreCommand(projectRoot: string): string { + const quoted = SAFE_SHELL_ARG.test(projectRoot) + ? projectRoot + : `'${projectRoot.replaceAll("'", `'"'"'`)}'`; + return `${RETIRE_STORE_COMMAND} --path ${quoted}`; +} + /** A path for display: project-relative inside it, `~/...` under home. */ function displayPath(opts: { readonly projectRoot: string; @@ -124,7 +137,7 @@ async function keylessStoreFinding(opts: { ].join(" "); return { status: usedHere ? "error" : "warn", - message: `${state} Its entries cannot be counted or read without that key. ${usage} Choose one: restore the key (put the key file back at ${opts.key} with mode 600 from a backup or another machine, or set HACK_SECRETS_FILE_KEY to the same key); or, if the key is gone for good, run ${RETIRE_STORE_COMMAND}, which moves the store aside to a timestamped name beside it, deletes nothing, and prints how to put it back. Do not provision a new key: it cannot open these entries.`, + message: `${state} Its entries cannot be counted or read without that key. ${usage} Choose one: restore the key (put the key file back at ${opts.key} with mode 600 from a backup or another machine, or set HACK_SECRETS_FILE_KEY to the same key); or, if the key is gone for good, run ${retireStoreCommand(opts.projectRoot)}, which moves the store aside to a timestamped name beside it, deletes nothing, and prints how to put it back. Do not provision a new key: it cannot open these entries.`, }; } diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index 368544d81..95e0d7883 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -121,32 +121,26 @@ export async function retireSecretStore(opts: { readonly now?: Date; }): Promise { const listing = await storeLocation(opts); - if (listing.kind === "inactive") { - return { - kind: "refused", - message: - "No encrypted file secret store is configured here (the backend is the OS keychain); nothing was moved.", - }; - } - if (listing.kind === "listed") { - return { - kind: "refused", - message: - listing.ownEntries.size + listing.foreignEntryCount === 0 - ? `There is no encrypted secret store with entries at ${listing.storePath}; nothing was moved.` - : `The encrypted secret store at ${listing.storePath} opens with the key in use, so there is nothing to retire; nothing was moved.`, - }; - } - if (!RETIRABLE_REASONS.has(listing.reason)) { - return { - kind: "refused", - message: `The encrypted secret store at ${listing.storePath} is not retired: it is not a missing or wrong key (${listing.reason.replaceAll("_", " ")}). Run hack doctor for what to fix; nothing was moved.`, - }; + const refusal = retireRefusal(listing); + if (refusal !== null) { + return { kind: "refused", message: refusal }; } try { const moved = await retireEncryptedStoreFile({ storePath: listing.storePath, now: opts.now, + // Classified before the lock: classify again under it, so a store + // whose key came back, or that was replaced, is not retired. + recheck: async () => { + const current = await storeLocation(opts); + if (current.storePath !== listing.storePath) { + return "The secret store configuration changed while retiring; nothing was moved. Run hack doctor, then retry."; + } + const changed = retireRefusal(current); + return changed === null + ? null + : `${changed} (It changed while retiring.)`; + }, }); return { kind: "retired", ...moved, keyPath: listing.keyPath }; } catch (error: unknown) { @@ -156,3 +150,22 @@ export async function retireSecretStore(opts: { throw error; } } + +/** + * Why a store is not one to retire, or null when it is (entries exist and + * the key that opens them is missing or wrong). + */ +function retireRefusal(listing: FileSecretStoreListing): string | null { + if (listing.kind === "inactive") { + return "No encrypted file secret store is configured here (the backend is the OS keychain); nothing was moved."; + } + if (listing.kind === "listed") { + return listing.ownEntries.size + listing.foreignEntryCount === 0 + ? `There is no encrypted secret store with entries at ${listing.storePath}; nothing was moved.` + : `The encrypted secret store at ${listing.storePath} opens with the key in use, so there is nothing to retire; nothing was moved.`; + } + if (!RETIRABLE_REASONS.has(listing.reason)) { + return `The encrypted secret store at ${listing.storePath} is not retired: it is not a missing or wrong key (${listing.reason.replaceAll("_", " ")}). Run hack doctor for what to fix; nothing was moved.`; + } + return null; +} diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index 0e78352ac..468b8f7a4 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -254,6 +254,12 @@ export class SecretStoreRetireError extends Error { export async function retireEncryptedStoreFile(input: { readonly storePath: string; readonly now?: Date; + /** + * Runs under the store's lock just before the move: a refusal message + * when the store is no longer one to retire (its key was restored, or it + * was replaced meanwhile), null to go on. + */ + readonly recheck?: () => Promise; }): Promise<{ readonly from: string; readonly to: string }> { const storePath = input.storePath; return await withEncryptedStoreMutation({ @@ -270,6 +276,10 @@ export async function retireEncryptedStoreFile(input: { `The encrypted secret store at ${storePath} is a symlink or not a regular file; nothing was moved. Inspect it with ls -l, then move it aside by hand if you mean to.` ); } + const changed = (await input.recheck?.()) ?? null; + if (changed !== null) { + throw new SecretStoreRetireError(changed); + } const stamp = (input.now ?? new Date()) .toISOString() .replace(STAMP_SEPARATORS, "") diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 03712fadb..94722a0d4 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -26,6 +26,7 @@ import { import { inspectSecretIntegrity } from "../src/lib/doctor-secret-integrity.ts"; import { findProjectContext } from "../src/lib/project.ts"; import { upsertProjectRegistration } from "../src/lib/projects-registry.ts"; +import { withProjectsRegistryLock } from "../src/lib/projects-registry-lock.ts"; import { resolveSecretStore } from "../src/lib/secret-store.ts"; import { retireSecretStore } from "../src/lib/secret-store-retire.ts"; import { restoreEnv } from "./helpers/env.ts"; @@ -235,7 +236,9 @@ test("a store whose key is missing is an error only for a project that reads it" }).join("\n"); expect(lines).toContain("Choose one:"); expect(lines).toContain("Restore the encrypted secret store's key"); - expect(lines).toContain("`hack secrets retire-store`"); + expect(lines).toContain( + `\`hack secrets retire-store --path ${fixture.bystander.projectRoot}\`` + ); }); test("retire-store refuses without --yes when it cannot prompt, then retires, and moving the file back restores it", async () => { @@ -348,3 +351,78 @@ test("retire-store refuses a store that opens, an absent store, and a key that i ); expect(await readFile(join(root, "elsewhere.enc.json"))).toEqual(before); }); + +test("retire re-checks the store under its lock: a key restored meanwhile is not retired", async () => { + // Codex review: eligibility was decided before the store's lock, so a + // store whose key came back in between was still moved aside. + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + let retire: ReturnType | undefined; + await withProjectsRegistryLock({ + lockPath: `${fixture.storePath}.lock`, + label: "test store holder", + run: async () => { + retire = retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + await Bun.sleep(300); + // Still waiting for the lock: the key comes back now. + await writeFile(fixture.keyPath, `${STORE_KEY}\n`, { mode: 0o600 }); + await chmod(fixture.keyPath, 0o600); + }, + }); + if (!retire) { + throw new Error("retire not started"); + } + const outcome = await retire; + expect(outcome.kind).toBe("refused"); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "opens with the key in use" + ); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "It changed while retiring" + ); + expect(await readFile(fixture.storePath)).toEqual(before); + const retired = (await readdir(fixture.hackHome)).filter((name) => + name.includes(".retired-") + ); + expect(retired).toEqual([]); +}); + +test("the retire command doctor and the recovery block name targets the diagnosed project", async () => { + // Codex review: an unscoped command acts on the current directory, not + // the project doctor (or crash capture) diagnosed with --path. + const fixture = await setUp(); + const finding = (await inspect(fixture.bystander, "app-b"))[0]; + const command = `hack secrets retire-store --path ${fixture.bystander.projectRoot}`; + expect(finding?.message).toContain(command); + const guidance = buildDoctorRecoveryGuidance({ + results: [ + { + name: "secret integrity", + status: "warn", + message: finding?.message ?? "", + }, + ], + }); + expect( + buildRecoveryWorkflowLines({ guidance, projectRoot: null }).join("\n") + ).toContain(`\`${command}\``); + // A finding without --path (an older wording) is scoped to the project. + const unscoped = buildDoctorRecoveryGuidance({ + results: [ + { + name: "secret integrity", + status: "warn", + message: "Run hack secrets retire-store to move it aside.", + }, + ], + }); + expect( + buildRecoveryWorkflowLines({ + guidance: unscoped, + projectRoot: "/srv/some project", + }).join("\n") + ).toContain("`hack secrets retire-store --path '/srv/some project'`"); +}); From f913a591d1ef25523dfccd145d7ac03b44026841 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 14:26:46 +0000 Subject: [PATCH 03/22] fix(secrets): shell-quote every printed restore and retire command (HACK-2177) A configured store path is project input and may hold a quote; the mv restore commands doctor and retire-store print, and the --path in the retire command, now use quoteShellWord instead of bare single quotes. The guidance extracts the backticked command whole. Co-Authored-By: Claude Opus 5.5 --- src/commands/recovery-guidance.ts | 2 +- src/commands/secrets.ts | 3 +- src/lib/doctor-secret-integrity.ts | 15 +++---- src/lib/secret-store.ts | 3 +- tests/secret-store-retire.test.ts | 66 ++++++++++++++++++++++++++++-- 5 files changed, 73 insertions(+), 16 deletions(-) diff --git a/src/commands/recovery-guidance.ts b/src/commands/recovery-guidance.ts index 43deac762..a9d44c299 100644 --- a/src/commands/recovery-guidance.ts +++ b/src/commands/recovery-guidance.ts @@ -25,7 +25,7 @@ const FIX_CHECKS = new Set(["caddy local ca", "dns:hack", "dns:hack.gy"]); const RETIRE_STORE_COMMAND = "hack secrets retire-store"; /** The command as the finding names it, with the diagnosed project's `--path`. */ const RETIRE_STORE_COMMAND_PATTERN = - /hack secrets retire-store(?: --path (?:'(?:[^']|'"'"')*'|[A-Za-z0-9_./:-]+))?/u; + /hack secrets retire-store(?: --path (?:'(?:[^']|'\\'')*'|[A-Za-z0-9_@%+=:,./-]+))?/u; /** The two ways forward for such a store, shown as one choice (HACK-2177). */ function secretStoreChoices(command: string): readonly string[] { diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index c00697929..21765aed6 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -10,6 +10,7 @@ import { requireInteractive, } from "../lib/interactivity.ts"; import { keychain } from "../lib/keychain.ts"; +import { quoteShellWord } from "../lib/logs.ts"; import { findProjectContext, readProjectConfig } from "../lib/project.ts"; import { retireSecretStore } from "../lib/secret-store-retire.ts"; import { logger } from "../ui/logger.ts"; @@ -300,7 +301,7 @@ async function handleSecretsRetireStore({ message: [ `Moved the encrypted secret store ${outcome.from} to ${outcome.to}.`, "Nothing was deleted, and no entry was read or printed. The key file was left alone.", - `To put it back (while no store is at ${outcome.from}): mv '${outcome.to}' '${outcome.from}'`, + `To put it back (while no store is at ${outcome.from}): mv ${quoteShellWord(outcome.to)} ${quoteShellWord(outcome.from)}`, ].join("\n"), }); return 0; diff --git a/src/lib/doctor-secret-integrity.ts b/src/lib/doctor-secret-integrity.ts index 9eae12679..dd5d26a19 100644 --- a/src/lib/doctor-secret-integrity.ts +++ b/src/lib/doctor-secret-integrity.ts @@ -2,6 +2,7 @@ import { isAbsolute, relative, sep } from "node:path"; import { PROJECT_ENV_KEY_FILENAME } from "../constants.ts"; import { resolveUserHomeDir } from "./home-dir.ts"; +import { quoteShellWord } from "./logs.ts"; import { inspectProjectEnvLayerKeys, PROJECT_ENV_SECRET_KEY_ENV, @@ -31,17 +32,12 @@ export type SecretIntegrityFinding = { /** The command that moves a store whose key is gone aside (HACK-2177). */ export const RETIRE_STORE_COMMAND = "hack secrets retire-store"; -const SAFE_SHELL_ARG = /^[A-Za-z0-9_./:-]+$/u; - /** * The retire command for the store this project selects: with `--path`, * since it otherwise acts on the current directory's project. */ function retireStoreCommand(projectRoot: string): string { - const quoted = SAFE_SHELL_ARG.test(projectRoot) - ? projectRoot - : `'${projectRoot.replaceAll("'", `'"'"'`)}'`; - return `${RETIRE_STORE_COMMAND} --path ${quoted}`; + return `${RETIRE_STORE_COMMAND} --path ${quoteShellWord(projectRoot)}`; } /** A path for display: project-relative inside it, `~/...` under home. */ @@ -137,7 +133,7 @@ async function keylessStoreFinding(opts: { ].join(" "); return { status: usedHere ? "error" : "warn", - message: `${state} Its entries cannot be counted or read without that key. ${usage} Choose one: restore the key (put the key file back at ${opts.key} with mode 600 from a backup or another machine, or set HACK_SECRETS_FILE_KEY to the same key); or, if the key is gone for good, run ${retireStoreCommand(opts.projectRoot)}, which moves the store aside to a timestamped name beside it, deletes nothing, and prints how to put it back. Do not provision a new key: it cannot open these entries.`, + message: `${state} Its entries cannot be counted or read without that key. ${usage} Choose one: restore the key (put the key file back at ${opts.key} with mode 600 from a backup or another machine, or set HACK_SECRETS_FILE_KEY to the same key); or, if the key is gone for good, run \`${retireStoreCommand(opts.projectRoot)}\`, which moves the store aside to a timestamped name beside it, deletes nothing, and prints how to put it back. Do not provision a new key: it cannot open these entries.`, }; } @@ -149,10 +145,11 @@ async function retiredStoreFindings(opts: { const store = displayPath({ ...opts, path: opts.storePath }); return (await listRetiredEncryptedStores(opts.storePath)).map((path) => { const shown = displayPath({ ...opts, path }); - // The command quotes absolute paths: a quoted `~` is not expanded. + // Absolute paths (a quoted `~` is not expanded), shell-quoted: a + // configured store path is project input and may hold a quote. return { status: "info", - message: `Retired secret store kept at ${shown}; Hack no longer reads it. To put it back while no store is at ${store}: mv '${path}' '${opts.storePath}'.`, + message: `Retired secret store kept at ${shown}; Hack no longer reads it. To put it back while no store is at ${store}: mv ${quoteShellWord(path)} ${quoteShellWord(opts.storePath)}`, }; }); } diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index 468b8f7a4..3da3fd428 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -18,6 +18,7 @@ import { envParseDiagnostic } from "./env-parse-diagnostic.ts"; import { ensureDir } from "./fs.ts"; import { resolveUserHomeDir } from "./home-dir.ts"; import { isKeychainDisabled, keychain } from "./keychain.ts"; +import { quoteShellWord } from "./logs.ts"; import { NativeKeychainTimeoutError } from "./native-keychain.ts"; import { decodeCanonicalBase64, @@ -295,7 +296,7 @@ export async function retireEncryptedStoreFile(input: { throw new SecretStoreRetireError( code === "EEXIST" ? `${to} already exists; nothing was moved. Retry in a second.` - : `Could not give ${storePath} the name ${to} (${code}); nothing was moved. To retire it by hand: mv '${storePath}' '${to}'` + : `Could not give ${storePath} the name ${to} (${code}); nothing was moved. To retire it by hand: mv ${quoteShellWord(storePath)} ${quoteShellWord(to)}` ); } await unlink(storePath); diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 94722a0d4..c6b276197 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -272,9 +272,7 @@ test("retire-store refuses without --yes when it cannot prompt, then retires, an const retiredPath = join(fixture.hackHome, kept[0] ?? ""); expect(await readFile(retiredPath)).toEqual(before); expect(retired.output).toContain(`to ${retiredPath}`); - expect(retired.output).toContain( - `mv '${retiredPath}' '${fixture.storePath}'` - ); + expect(retired.output).toContain(`mv ${retiredPath} ${fixture.storePath}`); expect(retired.output).toContain("Nothing was deleted"); // doctor afterwards: the retired file is information, not a problem. @@ -289,7 +287,7 @@ test("retire-store refuses without --yes when it cannot prompt, then retires, an `Retired secret store kept at ~/.hack/${kept[0]}` ); expect(after[0]?.message).toContain( - `mv '${retiredPath}' '${fixture.storePath}'` + `mv ${retiredPath} ${fixture.storePath}` ); } @@ -426,3 +424,63 @@ test("the retire command doctor and the recovery block name targets the diagnose }).join("\n") ).toContain("`hack secrets retire-store --path '/srv/some project'`"); }); + +test("printed restore commands quote a store path that holds a quote", async () => { + // Codex security review: a configured store path is project input; a + // quote in it must not end the shell literal of the printed mv command. + const fixture = await setUp(); + const hostile = join(fixture.hackHome, "store'; touch PWNED; #'.enc.json"); + await rename(fixture.storePath, hostile); + const configPath = join(fixture.hackHome, "hack.config.json"); + await writeFile( + configPath, + `${JSON.stringify({ + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { path: hostile, keyPath: fixture.keyPath }, + }, + }, + })}\n` + ); + const before = await readFile(hostile); + const outcome = await retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind).toBe("retired"); + const info = (await inspect(fixture.bystander, "app-b")).find( + (finding) => finding.status === "info" + ); + const command = info?.message.split(": ").at(-1) ?? ""; + expect(command.startsWith("mv ")).toBe(true); + // Run exactly what was printed, in the fixture root. + const shell = Bun.spawn(["sh", "-c", command], { + cwd: root, + stdout: "ignore", + stderr: "pipe", + }); + expect(await shell.exited).toBe(0); + expect(await readFile(hostile)).toEqual(before); + expect(await lstat(join(root, "PWNED")).catch(() => null)).toBeNull(); + expect( + await lstat(join(fixture.hackHome, "PWNED")).catch(() => null) + ).toBeNull(); + // The guidance extracts the scoped retire command whole. + const lines = buildRecoveryWorkflowLines({ + guidance: buildDoctorRecoveryGuidance({ + results: [ + { + name: "secret integrity", + status: "warn", + message: + "Run hack secrets retire-store --path '/srv/it'\\''s here' now.", + }, + ], + }), + projectRoot: null, + }).join("\n"); + expect(lines).toContain( + "`hack secrets retire-store --path '/srv/it'\\''s here'`" + ); +}); From 580e91d69a5f4d57ef92bdb40c18dc799070fdb4 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 14:32:50 +0000 Subject: [PATCH 04/22] fix(secrets): a key file that cannot be read is not missing, and its store is not retired (HACK-2177) The listing classified every key-resolution failure other than a refused key as missing, so retire-store accepted a store whose key file exists but is unreadable (mode 000). It is now key_unreadable: doctor says to fix the key's owner and permissions, and retire refuses it. Co-Authored-By: Claude Opus 5.5 --- src/lib/doctor-secret-integrity.ts | 7 +++++ src/lib/secret-store.ts | 44 ++++++++++++++++++++++++++---- tests/secret-store-retire.test.ts | 32 ++++++++++++++++++++++ 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/src/lib/doctor-secret-integrity.ts b/src/lib/doctor-secret-integrity.ts index dd5d26a19..9508a6c3b 100644 --- a/src/lib/doctor-secret-integrity.ts +++ b/src/lib/doctor-secret-integrity.ts @@ -182,6 +182,13 @@ function storeFindings(opts: { message: `The encrypted secret store ${store} is a symlink, not a regular file, or too large, so Hack refuses to read or change it. Replace it with a regular file this user owns (for a symlink: copy the target's contents to ${store} with mode 600), then re-run hack doctor.`, }, ]; + case "key_unreadable": + return [ + { + status: "error", + message: `The key ${key} for ${store} exists but cannot be read by this user. Fix its owner and permissions (chmod 600 ${key}, owned by this user), then re-run hack doctor. Do not retire the store or provision a new key: the existing key may still open it.`, + }, + ]; case "key_refused": return [ { diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index 3da3fd428..16310481d 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -4,7 +4,15 @@ import { createHash, randomBytes, } from "node:crypto"; -import { link, lstat, readdir, realpath, stat, unlink } from "node:fs/promises"; +import { + link, + lstat, + readdir, + readFile, + realpath, + stat, + unlink, +} from "node:fs/promises"; import { basename, dirname, resolve } from "node:path"; import { CliUsageError } from "../cli/command.ts"; import type { @@ -194,6 +202,7 @@ export type FileSecretStoreListing = { | "store_refused" | "key_missing" | "key_refused" + | "key_unreadable" | "wrong_key_or_corrupt" | "incomplete_config"; } @@ -422,7 +431,11 @@ async function openListedStore(input: { }): Promise< | { readonly entries: Record } | { - readonly reason: "key_missing" | "key_refused" | "wrong_key_or_corrupt"; + readonly reason: + | "key_missing" + | "key_refused" + | "key_unreadable" + | "wrong_key_or_corrupt"; } > { let key: string; @@ -435,11 +448,16 @@ async function openListedStore(input: { }) ).key; } catch (error: unknown) { + if (error instanceof SecretStoreKeyRefusedError) { + return { reason: "key_refused" }; + } + // Missing only when there is no key file, or it is empty. A key file + // that exists but cannot be read is not missing (HACK-2177): fixing its + // permissions may recover the store, which must not be retired. return { - reason: - error instanceof SecretStoreKeyRefusedError - ? "key_refused" - : "key_missing", + reason: (await keyFileUnreadable(input.keyPath)) + ? "key_unreadable" + : "key_missing", }; } try { @@ -455,6 +473,20 @@ async function openListedStore(input: { } } +/** True when the key file exists but this user cannot read it. */ +async function keyFileUnreadable(keyPath: string): Promise { + try { + await readFile(keyPath); + return false; + } catch (error: unknown) { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { readonly code?: unknown }).code + : undefined; + return code !== "ENOENT" && code !== "ENOTDIR"; + } +} + function decodeScopedKey(encoded: string): string | null { try { return decodeURIComponent(encoded); diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index c6b276197..98d6e6825 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -484,3 +484,35 @@ test("printed restore commands quote a store path that holds a quote", async () "`hack secrets retire-store --path '/srv/it'\\''s here'`" ); }); + +test("a key file that exists but cannot be read is not missing: the store is not retired", async () => { + // Codex review: an unreadable key file was classified as missing, so a + // store that fixing the key's permissions would recover was retired. + if (process.getuid?.() === 0) { + return; + } + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + await writeFile(fixture.keyPath, `${STORE_KEY}\n`, { mode: 0o600 }); + await chmod(fixture.keyPath, 0o000); + try { + const findings = await inspect(fixture.bystander, "app-b"); + expect(findings).toHaveLength(1); + expect(findings[0]?.status).toBe("error"); + expect(findings[0]?.message).toContain("exists but cannot be read"); + expect(findings[0]?.message).not.toContain("hack secrets retire-store"); + const outcome = await retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind).toBe("refused"); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "key unreadable" + ); + expect(await readFile(fixture.storePath)).toEqual(before); + } finally { + await chmod(fixture.keyPath, 0o600); + } + // Readable again: the store opens; nothing was lost. + expect(await inspect(fixture.bystander, "app-b")).toEqual([]); +}); From 938212ec4169a2365bc040ada8facf6ff2f6bc0e Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 14:43:58 +0000 Subject: [PATCH 05/22] fix(secrets): retire by rename over an exclusive placeholder, refuse key aliases and unsure config, and report incomplete reader scans (HACK-2177) Second-family review of #779: - The move never unlinks: the archive name is created exclusively as an empty placeholder, checked to still be that empty file, and the store is renamed over it. A replacement put at the store path by a writer outside the lock is archived, not deleted; an existing archive name is refused, never replaced. - A store that is its own key file (same device and inode, so aliases and hard links too) is refused. - Retire refuses, before and under the lock, when the global or project config that selects the store exists but cannot be read or does not parse, instead of falling back to another store. - The reader scan reports an unreadable registry and projects it could not check, compares store files by device and inode, and doctor says when the list may be incomplete instead of "no registered project reads it". Co-Authored-By: Claude Opus 5.5 --- src/lib/doctor-secret-integrity.ts | 26 ++++- src/lib/secret-store-retire.ts | 103 +++++++++++++++-- src/lib/secret-store.ts | 178 +++++++++++++++++++++++------ tests/secret-store-retire.test.ts | 127 +++++++++++++++++++- 4 files changed, 390 insertions(+), 44 deletions(-) diff --git a/src/lib/doctor-secret-integrity.ts b/src/lib/doctor-secret-integrity.ts index 9508a6c3b..a0d7bc6f6 100644 --- a/src/lib/doctor-secret-integrity.ts +++ b/src/lib/doctor-secret-integrity.ts @@ -16,6 +16,7 @@ import { import { listProjectsUsingSecretStore, projectUsesSecretStore, + type SecretStoreUsers, } from "./secret-store-retire.ts"; /** @@ -127,9 +128,7 @@ async function keylessStoreFinding(opts: { usedHere ? "This project's env contract reads it." : "This project does not read it.", - users.length > 0 - ? `Registered projects that read it: ${users.join(", ")}.` - : "No registered project reads it.", + describeStoreUsers(users), ].join(" "); return { status: usedHere ? "error" : "warn", @@ -137,6 +136,27 @@ async function keylessStoreFinding(opts: { }; } +/** + * Which registered projects read the store, saying plainly when the scan + * could not check every one (never "no project reads it" on a guess). + */ +function describeStoreUsers(users: SecretStoreUsers): string { + const named = + users.names.length > 0 + ? `Registered projects that read it: ${users.names.join(", ")}.` + : null; + if (!users.registryRead) { + return "The projects registry could not be read, so which registered projects read it is unknown."; + } + if (users.unchecked > 0) { + const gap = `${users.unchecked} registered project(s) could not be checked (their Hack config cannot be read), so the list may be incomplete.`; + return named === null + ? `No checked registered project reads it, but ${gap}` + : `${named} ${gap}`; + } + return named ?? "No registered project reads it."; +} + /** Retired copies kept beside the store: information, not a problem. */ async function retiredStoreFindings(opts: { readonly projectRoot: string; diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index 95e0d7883..aa1d8f553 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -1,5 +1,10 @@ +import { stat } from "node:fs/promises"; import { resolve } from "node:path"; +import { PROJECT_CONFIG_FILENAME } from "../constants.ts"; +import { readControlPlaneConfig } from "../control-plane/sdk/config.ts"; +import { resolveGlobalConfigPath } from "./config-paths.ts"; +import { readOptionalTextFile } from "./fs.ts"; import { readHackEnvContract, readHackEnvRuntimeConfig } from "./hack-env.ts"; import { readProjectsRegistry } from "./projects-registry.ts"; import { @@ -48,14 +53,53 @@ export async function projectUsesSecretStore(opts: { } /** - * Names (only) of registered projects whose configuration selects the store - * at `storePath` and whose runtime reads it, from configuration only: no - * store is opened. Projects whose configuration cannot be read are skipped; - * a registry that cannot be read yields none. + * Why the configuration that selects the store (the global config and, for + * a project, its `hack.config.json`) cannot be trusted, or null when it can. + * A file that exists but cannot be read is not absent, and one that does + * not parse contributes nothing, so either could silently select another + * store (for example the global one). Value-free: paths only. + */ +export async function secretStoreSelectionProblem(opts: { + readonly projectDir?: string; +}): Promise { + const paths = [ + resolveGlobalConfigPath(), + ...(opts.projectDir === undefined + ? [] + : [resolve(opts.projectDir, PROJECT_CONFIG_FILENAME)]), + ]; + for (const path of paths) { + if ((await readOptionalTextFile(path)).kind === "unreadable") { + return `The Hack config ${path} exists but cannot be read, so which secret store is selected is unknown.`; + } + } + const config = await readControlPlaneConfig({ projectDir: opts.projectDir }); + return config.parseError === undefined + ? null + : "The Hack config that selects the secret store does not parse, so which secret store is selected is unknown."; +} + +/** Registered projects that read a store, and whether every one was checked. */ +export type SecretStoreUsers = { + /** Names only, sorted. */ + readonly names: readonly string[]; + /** False when the registry could not be read. */ + readonly registryRead: boolean; + /** Registered projects whose store selection could not be checked. */ + readonly unchecked: number; +}; + +/** + * Registered projects whose configuration selects the store at `storePath` + * and whose runtime reads it, from configuration only: no store is opened. + * A project selects the same store when its configured path is the same, + * or names the same file (device and inode, so aliases through symlinked + * directories match). What could not be checked is counted, never taken as + * "does not read it". */ export async function listProjectsUsingSecretStore(opts: { readonly storePath: string; -}): Promise { +}): Promise { let projects: readonly { readonly name: string; readonly projectDir: string; @@ -63,27 +107,58 @@ export async function listProjectsUsingSecretStore(opts: { try { projects = (await readProjectsRegistry()).projects; } catch { - return []; + return { names: [], registryRead: false, unchecked: 0 }; } + const target = await stat(opts.storePath).catch(() => null); const names = new Set(); + let unchecked = 0; for (const project of projects) { let location: Awaited>; try { + if ( + (await secretStoreSelectionProblem({ + projectDir: project.projectDir, + })) !== null + ) { + unchecked += 1; + continue; + } location = await resolveFileSecretStorePaths({ projectDir: project.projectDir, }); } catch { + unchecked += 1; continue; } if ( location.active && - resolve(location.storePath) === resolve(opts.storePath) && + (await sameStoreFile({ path: location.storePath, opts, target })) && (await projectUsesSecretStore({ projectDir: project.projectDir })) ) { names.add(project.name); } } - return [...names].sort(); + return { names: [...names].sort(), registryRead: true, unchecked }; +} + +/** The same configured path, or the same existing file (device and inode). */ +async function sameStoreFile(input: { + readonly path: string; + readonly opts: { readonly storePath: string }; + readonly target: { readonly dev: number; readonly ino: number } | null; +}): Promise { + if (resolve(input.path) === resolve(input.opts.storePath)) { + return true; + } + if (input.target === null) { + return false; + } + const candidate = await stat(input.path).catch(() => null); + return ( + candidate !== null && + candidate.dev === input.target.dev && + candidate.ino === input.target.ino + ); } /** @@ -120,6 +195,13 @@ export async function retireSecretStore(opts: { readonly projectDir?: string; readonly now?: Date; }): Promise { + const selection = await secretStoreSelectionProblem(opts); + if (selection !== null) { + return { + kind: "refused", + message: `${selection} Nothing was moved. Fix that config, then run hack doctor and retry.`, + }; + } const listing = await storeLocation(opts); const refusal = retireRefusal(listing); if (refusal !== null) { @@ -128,10 +210,15 @@ export async function retireSecretStore(opts: { try { const moved = await retireEncryptedStoreFile({ storePath: listing.storePath, + keyPath: listing.keyPath, now: opts.now, // Classified before the lock: classify again under it, so a store // whose key came back, or that was replaced, is not retired. recheck: async () => { + const unsure = await secretStoreSelectionProblem(opts); + if (unsure !== null) { + return `${unsure} Nothing was moved. (It changed while retiring.)`; + } const current = await storeLocation(opts); if (current.storePath !== listing.storePath) { return "The secret store configuration changed while retiring; nothing was moved. Run hack doctor, then retry."; diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index 16310481d..3cf5fd5ba 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -5,11 +5,12 @@ import { randomBytes, } from "node:crypto"; import { - link, lstat, + open, readdir, readFile, realpath, + rename, stat, unlink, } from "node:fs/promises"; @@ -254,15 +255,27 @@ export class SecretStoreRetireError extends Error { } /** - * Moves an encrypted store file aside (HACK-2177), never deleting it: under - * the store's mutation lock, the regular file at `storePath` gets a second - * name `.retired-` in the same directory (a hard link, - * which never replaces an existing file), and only then is its original - * name removed. A link, a non-regular file or an absent store is refused. - * Nothing is read or decrypted; the key file is not touched. + * Moves an encrypted store file aside (HACK-2177), never deleting it. Under + * the store's mutation lock: + * + * 1. the store must be a regular file, and not the key file itself (same + * device and inode, so an alias or hard link of the key is refused); + * 2. `recheck` runs (eligibility, classified again under the lock); + * 3. the archive name `.retired-` is created exclusively + * as an empty placeholder (refused if that name exists); + * 4. `rename` moves whatever file is at the store path at that instant over + * the placeholder, after checking the placeholder is still the same + * empty file. + * + * Nothing is unlinked: if a writer outside the lock replaced the store in + * between, the replacement is what gets archived, and the only file ever + * replaced is the empty placeholder this call created. Nothing is read or + * decrypted; the key file is not touched. */ export async function retireEncryptedStoreFile(input: { readonly storePath: string; + /** The store's key file: a store that is the key file is not retired. */ + readonly keyPath?: string; readonly now?: Date; /** * Runs under the store's lock just before the move: a refusal message @@ -270,22 +283,17 @@ export async function retireEncryptedStoreFile(input: { * was replaced meanwhile), null to go on. */ readonly recheck?: () => Promise; + /** + * Test seam: runs just before the rename, after the placeholder check, + * so a test can replace the store as a writer outside the lock would. + */ + readonly beforeMove?: () => Promise; }): Promise<{ readonly from: string; readonly to: string }> { const storePath = input.storePath; return await withEncryptedStoreMutation({ filePath: storePath, run: async () => { - const info = await lstat(storePath).catch(() => null); - if (info === null) { - throw new SecretStoreRetireError( - `There is no encrypted secret store at ${storePath}; nothing was moved.` - ); - } - if (!info.isFile()) { - throw new SecretStoreRetireError( - `The encrypted secret store at ${storePath} is a symlink or not a regular file; nothing was moved. Inspect it with ls -l, then move it aside by hand if you mean to.` - ); - } + await assertRetirableStoreFile(input); const changed = (await input.recheck?.()) ?? null; if (changed !== null) { throw new SecretStoreRetireError(changed); @@ -295,25 +303,131 @@ export async function retireEncryptedStoreFile(input: { .replace(STAMP_SEPARATORS, "") .replace(STAMP_MILLISECONDS, "Z"); const to = `${storePath}${RETIRED_STORE_INFIX}${stamp}`; - try { - await link(storePath, to); - } catch (error: unknown) { - const code = - typeof error === "object" && error !== null && "code" in error - ? String((error as { readonly code?: unknown }).code) - : "unknown"; - throw new SecretStoreRetireError( - code === "EEXIST" - ? `${to} already exists; nothing was moved. Retry in a second.` - : `Could not give ${storePath} the name ${to} (${code}); nothing was moved. To retire it by hand: mv ${quoteShellWord(storePath)} ${quoteShellWord(to)}` - ); - } - await unlink(storePath); + const placeholder = await createRetirePlaceholder({ storePath, to }); + await moveStoreOverPlaceholder({ + storePath, + to, + placeholder, + beforeMove: input.beforeMove, + }); return { from: storePath, to }; }, }); } +/** The store is a regular file, and not its own key file (same device and inode). */ +async function assertRetirableStoreFile(input: { + readonly storePath: string; + readonly keyPath?: string; +}): Promise { + const info = await stat(input.storePath).catch(() => null); + const linkInfo = await lstat(input.storePath).catch(() => null); + if (info === null || linkInfo === null) { + throw new SecretStoreRetireError( + `There is no encrypted secret store at ${input.storePath}; nothing was moved.` + ); + } + if (!linkInfo.isFile()) { + throw new SecretStoreRetireError( + `The encrypted secret store at ${input.storePath} is a symlink or not a regular file; nothing was moved. Inspect it with ls -l, then move it aside by hand if you mean to.` + ); + } + const keyInfo = input.keyPath + ? await stat(input.keyPath).catch(() => null) + : null; + if ( + keyInfo !== null && + keyInfo.dev === info.dev && + keyInfo.ino === info.ino + ) { + throw new SecretStoreRetireError( + `The encrypted secret store at ${input.storePath} is its own key file (${input.keyPath}); retiring it would remove the key. Nothing was moved. Fix controlPlane.secrets.encryptedFile so the store and its key are different files.` + ); + } +} + +/** + * Renames the store over the placeholder, after checking the placeholder is + * still the same empty file; on any failure the placeholder (only if still + * ours) is removed and nothing has moved. + */ +async function moveStoreOverPlaceholder(input: { + readonly storePath: string; + readonly to: string; + readonly placeholder: { readonly dev: number; readonly ino: number }; + readonly beforeMove?: () => Promise; +}): Promise { + try { + const current = await lstat(input.to).catch(() => null); + if ( + current === null || + !current.isFile() || + current.dev !== input.placeholder.dev || + current.ino !== input.placeholder.ino || + current.size !== 0 + ) { + throw new SecretStoreRetireError( + `${input.to} changed while retiring; nothing was moved and it was left as it is.` + ); + } + await input.beforeMove?.(); + await rename(input.storePath, input.to); + } catch (error: unknown) { + await removeOwnPlaceholder(input); + if (error instanceof SecretStoreRetireError) { + throw error; + } + throw new SecretStoreRetireError( + `Could not move ${input.storePath} to ${input.to} (${errorCode(error)}); nothing was moved. To retire it by hand: mv ${quoteShellWord(input.storePath)} ${quoteShellWord(input.to)}` + ); + } +} + +/** Creates the archive name exclusively (an empty 0600 file); refuses if it exists. */ +async function createRetirePlaceholder(input: { + readonly storePath: string; + readonly to: string; +}): Promise<{ readonly dev: number; readonly ino: number }> { + let handle: Awaited>; + try { + handle = await open(input.to, "wx", 0o600); + } catch (error: unknown) { + throw new SecretStoreRetireError( + errorCode(error) === "EEXIST" + ? `${input.to} already exists; nothing was moved. Retry in a second.` + : `Could not create ${input.to} (${errorCode(error)}); nothing was moved. To retire it by hand: mv ${quoteShellWord(input.storePath)} ${quoteShellWord(input.to)}` + ); + } + try { + const created = await handle.stat(); + return { dev: created.dev, ino: created.ino }; + } finally { + await handle.close(); + } +} + +/** Removes the placeholder only while it is still the empty file this call made. */ +async function removeOwnPlaceholder(input: { + readonly to: string; + readonly placeholder: { readonly dev: number; readonly ino: number }; +}): Promise { + const current = await lstat(input.to).catch(() => null); + if ( + current?.isFile() === true && + current.dev === input.placeholder.dev && + current.ino === input.placeholder.ino && + current.size === 0 + ) { + await unlink(input.to).catch(() => undefined); + } +} + +function errorCode(error: unknown): string { + return typeof error === "object" && error !== null && "code" in error + ? String((error as { readonly code?: unknown }).code) + : "unknown"; +} + /** Retired copies of `storePath` in its directory, oldest first. */ export async function listRetiredEncryptedStores( storePath: string diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 98d6e6825..18d7a376d 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -9,6 +9,7 @@ import { realpath, rename, rm, + symlink, writeFile, } from "node:fs/promises"; import { tmpdir } from "node:os"; @@ -27,7 +28,10 @@ import { inspectSecretIntegrity } from "../src/lib/doctor-secret-integrity.ts"; import { findProjectContext } from "../src/lib/project.ts"; import { upsertProjectRegistration } from "../src/lib/projects-registry.ts"; import { withProjectsRegistryLock } from "../src/lib/projects-registry-lock.ts"; -import { resolveSecretStore } from "../src/lib/secret-store.ts"; +import { + resolveSecretStore, + retireEncryptedStoreFile, +} from "../src/lib/secret-store.ts"; import { retireSecretStore } from "../src/lib/secret-store-retire.ts"; import { restoreEnv } from "./helpers/env.ts"; import { privateCliEnv } from "./helpers/private-cli-env.ts"; @@ -516,3 +520,124 @@ test("a key file that exists but cannot be read is not missing: the store is not // Readable again: the store opens; nothing was lost. expect(await inspect(fixture.bystander, "app-b")).toEqual([]); }); + +test("a store replaced at the moment of the move is archived, never deleted, and an existing archive is never replaced", async () => { + // Second-family review blocker: link then unlink removed a replacement a + // writer outside the lock put at the store path between the two steps. + const fixture = await setUp(); + const replacement = Buffer.from("synthetic-replacement-store-bytes\n"); + const outcome = await retireEncryptedStoreFile({ + storePath: fixture.storePath, + keyPath: fixture.keyPath, + now: new Date("2026-11-13T12:00:00Z"), + beforeMove: async () => { + // A writer that ignores the lock swaps in a new file (new inode). + const staged = `${fixture.storePath}.staged`; + await writeFile(staged, replacement); + await rename(staged, fixture.storePath); + }, + }); + expect(outcome.to).toBe(`${fixture.storePath}.retired-20261113T120000Z`); + // The replacement is what was archived; nothing at either name was lost. + expect(await readFile(outcome.to)).toEqual(replacement); + expect(await lstat(fixture.storePath).catch(() => null)).toBeNull(); + + // An archive name that already exists is never replaced. + const second = join(fixture.hackHome, "secrets.enc.json"); + await writeFile(second, "synthetic-second-store\n"); + await expect( + retireEncryptedStoreFile({ + storePath: second, + now: new Date("2026-11-13T12:00:00Z"), + }) + ).rejects.toThrow("already exists; nothing was moved"); + expect(await readFile(outcome.to)).toEqual(replacement); + expect(await readFile(second, "utf8")).toBe("synthetic-second-store\n"); +}); + +test("a store that is its own key file is not retired, through any spelling", async () => { + // Second-family review: path and keyPath naming the same file (directly + // or through a symlinked directory) made the key file retirable. + const fixture = await setUp(); + await writeFile(fixture.keyPath, `${STORE_KEY}\n`, { mode: 0o600 }); + await chmod(fixture.keyPath, 0o600); + const alias = join(fixture.home, "key-alias"); + await symlink(fixture.hackHome, alias); + for (const path of [fixture.keyPath, join(alias, "secrets-file.key")]) { + await expect( + retireEncryptedStoreFile({ storePath: path, keyPath: fixture.keyPath }) + ).rejects.toThrow("is its own key file"); + expect(await readFile(fixture.keyPath, "utf8")).toBe(`${STORE_KEY}\n`); + } +}); + +test("retire refuses when the config that selects the store cannot be read or does not parse", async () => { + // Second-family review: an unreadable or malformed project config was + // taken as absent, so the global store could be retired instead. + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + const projectConfig = join(fixture.bystander.projectDir, "hack.config.json"); + if (process.getuid?.() !== 0) { + await chmod(projectConfig, 0o000); + try { + const outcome = await retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "exists but cannot be read" + ); + } finally { + await chmod(projectConfig, 0o644); + } + } + const projectText = await readFile(projectConfig, "utf8"); + await writeFile(projectConfig, "{ not json"); + const malformed = await retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + expect(malformed.kind === "refused" ? malformed.message : "").toContain( + "does not parse" + ); + await writeFile(projectConfig, projectText); + expect(await readFile(fixture.storePath)).toEqual(before); +}); + +test("doctor says when the reader scan is incomplete, and finds readers through an alias", async () => { + // Second-family review: scan failures became "no registered project + // reads it", and a reader reaching the store through a symlinked + // directory was missed. + const fixture = await setUp(); + // app-a reaches the store through a symlinked directory instead. + const alias = join(fixture.home, "hack-alias"); + await symlink(fixture.hackHome, alias); + await writeFile( + join(fixture.reader.projectDir, "hack.config.json"), + `${JSON.stringify({ + name: "app-a", + dev_host: "app-a.hack", + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { + path: join(alias, "secrets.enc.json"), + keyPath: fixture.keyPath, + }, + }, + }, + })}\n` + ); + const aliased = await inspect(fixture.bystander, "app-b"); + expect(aliased[0]?.message).toContain( + "Registered projects that read it: app-a." + ); + + // A registered project whose config cannot be parsed is not "does not read it". + await writeFile(join(fixture.reader.projectDir, "hack.config.json"), "{ bad"); + const partial = await inspect(fixture.bystander, "app-b"); + expect(partial[0]?.message).toContain( + "No checked registered project reads it, but 1 registered project(s) could not be checked" + ); + expect(partial[0]?.message).not.toContain("No registered project reads it."); +}); From 177610c50dee2f12a016d11bd95b4ca268ee53c7 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 14:52:35 +0000 Subject: [PATCH 06/22] fix(secrets): a keychain that cannot be asked is not a missing key, and store findings are errors only where the store is read (HACK-2177) With no key file, a keychain that is locked, denies access, times out or is in its failure cooldown left the key's existence unknown, yet the listing called it key_missing and retire-store accepted it. Keychain failures now carry SecretStoreKeyAccessError and classify as key_access_failed (as does any unexpected failure); retire refuses it and doctor says to unlock the keychain. Missing now means: no key file, and the keychain answered no item or is turned off. Every store-level finding (not only the missing key) is now an error only in a project whose runtime reads the store, a warning elsewhere. Co-Authored-By: Claude Opus 5.5 --- src/lib/doctor-secret-integrity.ts | 25 +++++++++++++- src/lib/secret-store.ts | 40 +++++++++++++++++----- tests/secret-store-retire.test.ts | 55 ++++++++++++++++++++++++++++-- 3 files changed, 108 insertions(+), 12 deletions(-) diff --git a/src/lib/doctor-secret-integrity.ts b/src/lib/doctor-secret-integrity.ts index a0d7bc6f6..d7274a197 100644 --- a/src/lib/doctor-secret-integrity.ts +++ b/src/lib/doctor-secret-integrity.ts @@ -97,7 +97,23 @@ async function inspectSecretStore(opts: { ...retired, ]; } - return [...storeFindings({ reason: listing.reason, store, key }), ...retired]; + // The store is machine-wide: an error only in a project whose runtime + // reads it, a warning elsewhere (HACK-2177). + const usedHere = await projectUsesSecretStore({ + projectDir: opts.projectDir, + }); + return [ + ...storeFindings({ reason: listing.reason, store, key }).map( + (finding): SecretIntegrityFinding => + usedHere || finding.status !== "error" + ? finding + : { + status: "warn", + message: `${finding.message} This project does not read it.`, + } + ), + ...retired, + ]; } /** @@ -202,6 +218,13 @@ function storeFindings(opts: { message: `The encrypted secret store ${store} is a symlink, not a regular file, or too large, so Hack refuses to read or change it. Replace it with a regular file this user owns (for a symlink: copy the target's contents to ${store} with mode 600), then re-run hack doctor.`, }, ]; + case "key_access_failed": + return [ + { + status: "error", + message: `The key for ${store} could not be looked up: there is no key file at ${key}, and the OS keychain could not be asked (it is locked, denied access, or timed out). Unlock the keychain or allow access, then re-run hack doctor. Do not retire the store or provision a new key: the keychain may still hold its key.`, + }, + ]; case "key_unreadable": return [ { diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index 3cf5fd5ba..63ba5b7ad 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -119,6 +119,18 @@ export class SecretStoreKeyUnavailableError extends Error { } } +/** + * The OS keychain could not be asked for the encrypted_file key (access + * denied, locked, timed out, or the failure cooldown): whether the key + * exists is unknown, which is not the same as missing (HACK-2177). + */ +export class SecretStoreKeyAccessError extends SecretStoreKeyUnavailableError { + constructor(message: string) { + super(message); + this.name = "SecretStoreKeyAccessError"; + } +} + /** * The key file exists but is refused (HACK-2036): a link or non-regular * file, readable or writable by other users, or tracked by git. @@ -204,6 +216,7 @@ export type FileSecretStoreListing = { | "key_missing" | "key_refused" | "key_unreadable" + | "key_access_failed" | "wrong_key_or_corrupt" | "incomplete_config"; } @@ -549,6 +562,7 @@ async function openListedStore(input: { | "key_missing" | "key_refused" | "key_unreadable" + | "key_access_failed" | "wrong_key_or_corrupt"; } > { @@ -565,14 +579,22 @@ async function openListedStore(input: { if (error instanceof SecretStoreKeyRefusedError) { return { reason: "key_refused" }; } - // Missing only when there is no key file, or it is empty. A key file - // that exists but cannot be read is not missing (HACK-2177): fixing its + // A key file that exists but cannot be read is not missing: fixing its // permissions may recover the store, which must not be retired. - return { - reason: (await keyFileUnreadable(input.keyPath)) - ? "key_unreadable" - : "key_missing", - }; + if (await keyFileUnreadable(input.keyPath)) { + return { reason: "key_unreadable" }; + } + // Missing only when every place the key could be has answered "none": + // no (or an empty) key file, and the keychain says no item or is turned + // off. A keychain that could not be asked, or any other failure, leaves + // it unknown (HACK-2177). + if ( + !(error instanceof SecretStoreKeyUnavailableError) || + error instanceof SecretStoreKeyAccessError + ) { + return { reason: "key_access_failed" }; + } + return { reason: "key_missing" }; } try { return { @@ -1331,7 +1353,7 @@ async function resolveEncryptedFileKeyMaterial(input?: { cachedKeychainEncryptedFileKey === null && nowMs < encryptedFileKeyFailureCooldownUntilMs ) { - throw new SecretStoreKeyUnavailableError( + throw new SecretStoreKeyAccessError( buildEncryptedBackendKeyRecoveryMessage({ keyPath: configuredKeyPath, storePath: input?.storePath, @@ -1427,7 +1449,7 @@ function keychainKeyAccessError(input: { if (input.error instanceof NativeKeychainTimeoutError) { return input.error; } - return new SecretStoreKeyUnavailableError( + return new SecretStoreKeyAccessError( buildEncryptedBackendKeyRecoveryMessage({ keyPath: input.keyPath, storePath: input.storePath, diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 18d7a376d..cbfd34e2c 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, expect, test } from "bun:test"; +import { afterEach, beforeEach, expect, spyOn, test } from "bun:test"; import { chmod, lstat, @@ -25,6 +25,7 @@ import { PROJECT_ENV_CONTRACT_FILENAME, } from "../src/constants.ts"; import { inspectSecretIntegrity } from "../src/lib/doctor-secret-integrity.ts"; +import { keychain } from "../src/lib/keychain.ts"; import { findProjectContext } from "../src/lib/project.ts"; import { upsertProjectRegistration } from "../src/lib/projects-registry.ts"; import { withProjectsRegistryLock } from "../src/lib/projects-registry-lock.ts"; @@ -502,8 +503,11 @@ test("a key file that exists but cannot be read is not missing: the store is not try { const findings = await inspect(fixture.bystander, "app-b"); expect(findings).toHaveLength(1); - expect(findings[0]?.status).toBe("error"); + // Machine-wide: a warning here, an error where the store is read. + expect(findings[0]?.status).toBe("warn"); expect(findings[0]?.message).toContain("exists but cannot be read"); + expect(findings[0]?.message).toContain("This project does not read it."); + expect((await inspect(fixture.reader, "app-a"))[0]?.status).toBe("error"); expect(findings[0]?.message).not.toContain("hack secrets retire-store"); const outcome = await retireSecretStore({ projectName: "app-b", @@ -641,3 +645,50 @@ test("doctor says when the reader scan is incomplete, and finds readers through ); expect(partial[0]?.message).not.toContain("No registered project reads it."); }); + +test("a keychain that cannot be asked is not a missing key: the store is not retired", async () => { + // Codex review: with no key file and the keychain locked, denied or + // timed out, the key's existence is unknown, but it was classified as + // missing and the store could be retired. + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + process.env.HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK = "0"; + // The keychain answers "no such item": the key is missing (retirable). + const empty = spyOn(keychain, "get").mockImplementation(async () => null); + try { + const missing = await inspect(fixture.bystander, "app-b"); + expect(missing[0]?.message).toContain("is missing"); + expect(missing[0]?.message).toContain("hack secrets retire-store"); + } finally { + empty.mockRestore(); + } + // The keychain cannot be asked: unknown, not retirable. + const denied = spyOn(keychain, "get").mockImplementation(async () => { + throw new Error("synthetic keychain access denied"); + }); + try { + const unknown = await inspect(fixture.bystander, "app-b"); + expect(unknown).toHaveLength(1); + expect(unknown[0]?.message).toContain("could not be looked up"); + expect(unknown[0]?.message).not.toContain("hack secrets retire-store"); + // The failure cooldown that follows is unknown too, never missing. + const outcome = await retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "key access failed" + ); + expect(await readFile(fixture.storePath)).toEqual(before); + } finally { + denied.mockRestore(); + // A successful resolution ends the module's failure cooldown, so no + // later test inherits it. + process.env.HACK_SECRETS_FILE_KEY = STORE_KEY; + await resolveSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + Reflect.deleteProperty(process.env, "HACK_SECRETS_FILE_KEY"); + } +}); From f4bc8140e738314343397f69dddb701a3affeb2a Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 14:58:35 +0000 Subject: [PATCH 07/22] fix(secrets): only a Hack store envelope is retirable, and printed chmod guidance is quoted (HACK-2177) - The configured store path is project input: retire-store now refuses, before and under the lock, a file that does not hold the encrypted store envelope, and doctor never offers to retire such a file. The confirmation prompt names the resolved store path. - The chmod and git rm guidance in the store and key findings uses absolute, shell-quoted paths. Co-Authored-By: Claude Opus 5.5 --- src/commands/secrets.ts | 8 +++- src/lib/doctor-secret-integrity.ts | 28 +++++++++-- src/lib/secret-store-retire.ts | 17 ++++++- src/lib/secret-store.ts | 21 ++++++++ tests/secret-store-retire.test.ts | 77 ++++++++++++++++++++++++++++++ 5 files changed, 143 insertions(+), 8 deletions(-) diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index 21765aed6..2db228bba 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -12,6 +12,7 @@ import { import { keychain } from "../lib/keychain.ts"; import { quoteShellWord } from "../lib/logs.ts"; import { findProjectContext, readProjectConfig } from "../lib/project.ts"; +import { resolveFileSecretStorePaths } from "../lib/secret-store.ts"; import { retireSecretStore } from "../lib/secret-store-retire.ts"; import { logger } from "../ui/logger.ts"; @@ -277,11 +278,14 @@ async function handleSecretsRetireStore({ "project" ).trim(); } + // Name the resolved file in the prompt: the configured path is project input. + const target = await resolveFileSecretStorePaths({ + projectDir: project?.projectDir, + }).catch(() => null); const ok = args.options.yes === true || (await confirmSafe({ - message: - "Move the encrypted secret store aside? Its entries stay in the moved file, which can be put back.", + message: `Move the encrypted secret store ${target?.storePath ?? "(unknown path)"} aside? Its entries stay in the moved file, which can be put back.`, initialValue: false, nonInteractive: "fail", hint: "Pass --yes to confirm (`hack secrets retire-store --yes`), or run it in a terminal.", diff --git a/src/lib/doctor-secret-integrity.ts b/src/lib/doctor-secret-integrity.ts index d7274a197..3a696a856 100644 --- a/src/lib/doctor-secret-integrity.ts +++ b/src/lib/doctor-secret-integrity.ts @@ -10,6 +10,7 @@ import { type ProjectEnvLayerKeyReport, } from "./project-env-config.ts"; import { + isEncryptedStoreFile, listFileSecretStoreEntries, listRetiredEncryptedStores, } from "./secret-store.ts"; @@ -103,7 +104,13 @@ async function inspectSecretStore(opts: { projectDir: opts.projectDir, }); return [ - ...storeFindings({ reason: listing.reason, store, key }).map( + ...storeFindings({ + reason: listing.reason, + store, + key, + storePath: listing.storePath, + keyPath: listing.keyPath, + }).map( (finding): SecretIntegrityFinding => usedHere || finding.status !== "error" ? finding @@ -134,6 +141,14 @@ async function keylessStoreFinding(opts: { const usedHere = await projectUsesSecretStore({ projectDir: opts.projectDir, }); + if (!(await isEncryptedStoreFile(opts.storePath))) { + // The configured path is project input and may name any file: never + // suggest retiring something that is not a Hack store. + return { + status: usedHere ? "error" : "warn", + message: `The configured encrypted secret store path ${opts.store} does not hold a Hack encrypted secret store (no store envelope), so Hack does not read, change or retire it. Check controlPlane.secrets.encryptedFile.path in the Hack config.`, + }; + } const users = await listProjectsUsingSecretStore({ storePath: opts.storePath, }); @@ -201,14 +216,19 @@ function storeFindings(opts: { >; readonly store: string; readonly key: string; + /** Absolute paths, shell-quoted wherever a command is printed. */ + readonly storePath: string; + readonly keyPath: string; }): SecretIntegrityFinding[] { const { store, key } = opts; + const storeArg = quoteShellWord(opts.storePath); + const keyArg = quoteShellWord(opts.keyPath); switch (opts.reason) { case "store_unreadable": return [ { status: "error", - message: `The encrypted secret store ${store} exists but cannot be read; Hack will not read or change it. Make it a regular file this user owns and can read and write (chmod 600 ${store}), then re-run hack doctor.`, + message: `The encrypted secret store ${store} exists but cannot be read; Hack will not read or change it. Make it a regular file this user owns and can read and write (chmod 600 ${storeArg}), then re-run hack doctor.`, }, ]; case "store_refused": @@ -229,14 +249,14 @@ function storeFindings(opts: { return [ { status: "error", - message: `The key ${key} for ${store} exists but cannot be read by this user. Fix its owner and permissions (chmod 600 ${key}, owned by this user), then re-run hack doctor. Do not retire the store or provision a new key: the existing key may still open it.`, + message: `The key ${key} for ${store} exists but cannot be read by this user. Fix its owner and permissions (chmod 600 ${keyArg}, owned by this user), then re-run hack doctor. Do not retire the store or provision a new key: the existing key may still open it.`, }, ]; case "key_refused": return [ { status: "error", - message: `The key ${key} for ${store} is refused: it is a symlink, other users can read or write it, or git tracks it. Replace it with a regular, untracked file this user owns (chmod 600 ${key}; git rm --cached if tracked, then rotate it).`, + message: `The key ${key} for ${store} is refused: it is a symlink, other users can read or write it, or git tracks it. Replace it with a regular, untracked file this user owns (chmod 600 ${keyArg}; git rm --cached ${keyArg} if tracked, then rotate it).`, }, ]; case "incomplete_config": diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index aa1d8f553..becab931d 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -9,6 +9,7 @@ import { readHackEnvContract, readHackEnvRuntimeConfig } from "./hack-env.ts"; import { readProjectsRegistry } from "./projects-registry.ts"; import { type FileSecretStoreListing, + isEncryptedStoreFile, listFileSecretStoreEntries, resolveFileSecretStorePaths, retireEncryptedStoreFile, @@ -203,7 +204,8 @@ export async function retireSecretStore(opts: { }; } const listing = await storeLocation(opts); - const refusal = retireRefusal(listing); + const refusal = + retireRefusal(listing) ?? (await notAStoreRefusal(listing.storePath)); if (refusal !== null) { return { kind: "refused", message: refusal }; } @@ -223,7 +225,8 @@ export async function retireSecretStore(opts: { if (current.storePath !== listing.storePath) { return "The secret store configuration changed while retiring; nothing was moved. Run hack doctor, then retry."; } - const changed = retireRefusal(current); + const changed = + retireRefusal(current) ?? (await notAStoreRefusal(current.storePath)); return changed === null ? null : `${changed} (It changed while retiring.)`; @@ -238,6 +241,16 @@ export async function retireSecretStore(opts: { } } +/** + * Refuses a configured store path that does not hold a Hack encrypted + * store envelope: the path is project input and may name any file. + */ +async function notAStoreRefusal(storePath: string): Promise { + return (await isEncryptedStoreFile(storePath)) + ? null + : `${storePath} is not a Hack encrypted secret store (it does not hold the store's encrypted envelope), so it is not retired; nothing was moved. Check controlPlane.secrets.encryptedFile.path in the Hack config.`; +} + /** * Why a store is not one to retire, or null when it is (entries exist and * the key that opens them is missing or wrong). diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index 63ba5b7ad..6057b603b 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -441,6 +441,27 @@ function errorCode(error: unknown): string { : "unknown"; } +/** + * Whether the file at `storePath` is a Hack encrypted secret store: a + * regular file (read without following a link) holding the store's JSON + * envelope. Nothing is decrypted. A configured store path is project + * input, so anything else (any other file) is never treated as a store to + * retire (HACK-2177). + */ +export async function isEncryptedStoreFile( + storePath: string +): Promise { + const read = await readEncryptedStoreText(storePath); + if (read.kind !== "text") { + return false; + } + try { + return parseEncryptedEnvelope(JSON.parse(read.text)) !== null; + } catch { + return false; + } +} + /** Retired copies of `storePath` in its directory, oldest first. */ export async function listRetiredEncryptedStores( storePath: string diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index cbfd34e2c..6f8546483 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -692,3 +692,80 @@ test("a keychain that cannot be asked is not a missing key: the store is not ret Reflect.deleteProperty(process.env, "HACK_SECRETS_FILE_KEY"); } }); + +test("a configured store path that is not a Hack store is never offered for retirement or retired", async () => { + // Codex security review: the path is project input; any non-empty file + // with no key counted as a keyless store, so a project could point it at + // a host file (such as an authorized_keys file) and have it moved aside. + const fixture = await setUp(); + const hostFile = join(fixture.home, ".ssh", "authorized_keys"); + await mkdir(join(fixture.home, ".ssh"), { recursive: true }); + await writeFile(hostFile, "synthetic-authorized-key-line\n"); + await writeFile( + join(fixture.hackHome, "hack.config.json"), + `${JSON.stringify({ + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { path: hostFile, keyPath: fixture.keyPath }, + }, + }, + })}\n` + ); + const findings = await inspect(fixture.bystander, "app-b"); + expect(findings).toHaveLength(1); + expect(findings[0]?.message).toContain( + "does not hold a Hack encrypted secret store" + ); + expect(findings[0]?.message).not.toContain("hack secrets retire-store"); + const outcome = await retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "is not a Hack encrypted secret store" + ); + expect(await readFile(hostFile, "utf8")).toBe( + "synthetic-authorized-key-line\n" + ); + expect( + (await readdir(join(fixture.home, ".ssh"))).filter((name) => + name.includes(".retired-") + ) + ).toEqual([]); +}); + +test("printed chmod and git rm guidance quotes a hostile key path", async () => { + // Codex security review: the key path is project input; the new + // key_unreadable guidance printed `chmod 600 ` unquoted. + if (process.getuid?.() === 0) { + return; + } + const fixture = await setUp(); + const hostileKey = join(fixture.hackHome, "key'; touch PWNED; #'"); + await writeFile(hostileKey, `${STORE_KEY}\n`, { mode: 0o600 }); + await writeFile( + join(fixture.hackHome, "hack.config.json"), + `${JSON.stringify({ + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { path: fixture.storePath, keyPath: hostileKey }, + }, + }, + })}\n` + ); + const quoted = `'${hostileKey.replaceAll("'", "'\\''")}'`; + await chmod(hostileKey, 0o000); + try { + const unreadable = (await inspect(fixture.reader, "app-a"))[0]; + expect(unreadable?.message).toContain(`chmod 600 ${quoted}`); + } finally { + await chmod(hostileKey, 0o600); + } + // Open to others: refused, with quoted chmod and git rm. + await chmod(hostileKey, 0o644); + const refused = (await inspect(fixture.reader, "app-a"))[0]; + expect(refused?.message).toContain(`chmod 600 ${quoted}`); + expect(refused?.message).toContain(`git rm --cached ${quoted}`); +}); From 5a689148b19e42cbccbd8d713af0f84abf5f8c0d Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 15:12:25 +0000 Subject: [PATCH 08/22] fix(secrets): retire into an exclusive directory so nothing outside it is replaced or removed, and keep scan uncertainty (HACK-2177) Second-family review of #779 at 177610c5: - Retire creates .retired-/ exclusively (0700), hard-links the store into it (a link never replaces a file), then renames whatever is at the store path into that directory under an unpredictable, checked-free name, removing it only when it is the archived file. A replacement written meanwhile is kept beside the archive. Every rename target and the only name ever removed are inside the new directory; failure cleanup is rmdir, which refuses unless empty. - The reader scan reads the registry strictly (an invalid registry is unknown, not empty) and treats a store identity it cannot establish as unchecked, never "different". - Docs and the CLI reference describe the retired directory. Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 4 +- docs/reference/cli.md | 2 +- docs/versions/v5/guides/upgrade-from-v4.md | 7 +- src/commands/secrets.ts | 7 +- src/lib/doctor-secret-integrity.ts | 4 +- src/lib/projects-registry.ts | 26 +++ src/lib/secret-store-retire.ts | 78 ++++++-- src/lib/secret-store.ts | 205 +++++++++++++-------- tests/secret-store-retire.test.ts | 100 +++++++++- 9 files changed, 319 insertions(+), 114 deletions(-) diff --git a/docs/env.md b/docs/env.md index bf11103b9..10e185996 100644 --- a/docs/env.md +++ b/docs/env.md @@ -1660,8 +1660,8 @@ Some older surfaces are still compatibility-oriented: it (its legacy env contract declares a store-backed value, or plaintext is kept in the backend), and a warning elsewhere. The finding names the registered projects that read the store, and offers two ways forward: restore the key, or - run `hack secrets retire-store`. That command renames the store to - `.retired-` beside itself, deletes and prints nothing, and says + run `hack secrets retire-store`. That command moves the store into a new + directory `.retired-/` beside it, deletes and prints nothing, and says how to move it back. It needs `--yes` without a terminal. Doctor then lists retired files as information. Explicit key provisioning also uses an atomic mode-`0600` replacement and refuses symlinked diff --git a/docs/reference/cli.md b/docs/reference/cli.md index d435c68e7..00541f623 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -2439,7 +2439,7 @@ hack secrets retire-store [options] ``` For an encrypted_file (or cloud shim) store that hack doctor reports with a missing or wrong key. -Renames the store to .retired- in the same directory: nothing is deleted, read or printed, +Moves the store into a new directory .retired-/ beside it: nothing is deleted, read or printed, and the key file is left alone. It prints how to put the store back. The store is the one this project selects (--path or the current directory), or the global one outside a project. Refused for a store that opens with the key in use, or that is unreadable for another reason. Without a TTY (or with diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index 290d0c381..bfb669164 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -394,9 +394,10 @@ entries. 2. **Retire the store** when the key is gone for good: `hack secrets retire-store`, run in a project that selects the store or outside any project for the global one. Without a terminal, add `--yes`. - - It renames the store to `.retired-` in the same - directory. It deletes nothing, reads and prints no entry, and leaves the - key file alone. + - It moves the store into a new directory `.retired-/` + beside it, keeping its name. It deletes nothing, reads and prints no + entry, and leaves the key file alone. It only retires a file that is a + Hack encrypted store. - It prints the `mv` command that puts the store back. - Afterwards Doctor lists the retired file as information, and the project check no longer fails. diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index 2db228bba..395bb88e6 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -96,7 +96,7 @@ const retireStoreSpec = defineCommand({ group: "Secrets", description: [ "For an encrypted_file (or cloud shim) store that hack doctor reports with a missing or wrong key.", - "Renames the store to .retired- in the same directory: nothing is deleted, read or printed,", + "Moves the store into a new directory .retired-/ beside it: nothing is deleted, read or printed,", "and the key file is left alone. It prints how to put the store back. The store is the one this project", "selects (--path or the current directory), or the global one outside a project. Refused for a store that", "opens with the key in use, or that is unreadable for another reason. Without a TTY (or with", @@ -306,6 +306,11 @@ async function handleSecretsRetireStore({ `Moved the encrypted secret store ${outcome.from} to ${outcome.to}.`, "Nothing was deleted, and no entry was read or printed. The key file was left alone.", `To put it back (while no store is at ${outcome.from}): mv ${quoteShellWord(outcome.to)} ${quoteShellWord(outcome.from)}`, + ...(outcome.replacement === null + ? [] + : [ + `A different file appeared at ${outcome.from} while moving; it was kept at ${outcome.replacement}.`, + ]), ].join("\n"), }); return 0; diff --git a/src/lib/doctor-secret-integrity.ts b/src/lib/doctor-secret-integrity.ts index 3a696a856..c27ed0620 100644 --- a/src/lib/doctor-secret-integrity.ts +++ b/src/lib/doctor-secret-integrity.ts @@ -177,10 +177,10 @@ function describeStoreUsers(users: SecretStoreUsers): string { ? `Registered projects that read it: ${users.names.join(", ")}.` : null; if (!users.registryRead) { - return "The projects registry could not be read, so which registered projects read it is unknown."; + return "The projects registry could not be read or is not valid, so which registered projects read it is unknown."; } if (users.unchecked > 0) { - const gap = `${users.unchecked} registered project(s) could not be checked (their Hack config cannot be read), so the list may be incomplete.`; + const gap = `${users.unchecked} registered project(s) could not be checked (their Hack config or store file could not be read), so the list may be incomplete.`; return named === null ? `No checked registered project reads it, but ${gap}` : `${named} ${gap}`; diff --git a/src/lib/projects-registry.ts b/src/lib/projects-registry.ts index efda30b62..c1581764a 100644 --- a/src/lib/projects-registry.ts +++ b/src/lib/projects-registry.ts @@ -172,6 +172,32 @@ export async function readProjectsRegistry(opts?: { return out ?? { version: REGISTRY_VERSION, projects: [] }; } +/** + * Strict read for callers that must not take an unreadable or invalid + * registry for an empty one (HACK-2177): `absent` (no file), `valid`, or + * `invalid` (not JSON, or not a registry). A file that cannot be read + * throws {@link ProjectsRegistryReadError}, as {@link readProjectsRegistry}. + */ +export async function readProjectsRegistryStrict(opts?: { + readonly registryPath?: string; +}): Promise< + | { readonly kind: "absent" | "invalid" } + | { readonly kind: "valid"; readonly registry: ProjectsRegistry } +> { + const text = await readRegistryText(opts?.registryPath ?? getRegistryPath()); + if (text === null) { + return { kind: "absent" }; + } + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + return { kind: "invalid" }; + } + const registry = parseRegistry(parsed); + return registry === null ? { kind: "invalid" } : { kind: "valid", registry }; +} + type RegistrationUpsertOptions = { readonly project: ProjectContext; readonly nowIso?: string; diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index becab931d..621d9d104 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -6,7 +6,7 @@ import { readControlPlaneConfig } from "../control-plane/sdk/config.ts"; import { resolveGlobalConfigPath } from "./config-paths.ts"; import { readOptionalTextFile } from "./fs.ts"; import { readHackEnvContract, readHackEnvRuntimeConfig } from "./hack-env.ts"; -import { readProjectsRegistry } from "./projects-registry.ts"; +import { readProjectsRegistryStrict } from "./projects-registry.ts"; import { type FileSecretStoreListing, isEncryptedStoreFile, @@ -84,7 +84,7 @@ export async function secretStoreSelectionProblem(opts: { export type SecretStoreUsers = { /** Names only, sorted. */ readonly names: readonly string[]; - /** False when the registry could not be read. */ + /** False when the registry could not be read or is not a valid registry. */ readonly registryRead: boolean; /** Registered projects whose store selection could not be checked. */ readonly unchecked: number; @@ -106,11 +106,15 @@ export async function listProjectsUsingSecretStore(opts: { readonly projectDir: string; }[]; try { - projects = (await readProjectsRegistry()).projects; + const read = await readProjectsRegistryStrict(); + if (read.kind === "invalid") { + return { names: [], registryRead: false, unchecked: 0 }; + } + projects = read.kind === "valid" ? read.registry.projects : []; } catch { return { names: [], registryRead: false, unchecked: 0 }; } - const target = await stat(opts.storePath).catch(() => null); + const target = await identity(opts.storePath); const names = new Set(); let unchecked = 0; for (const project of projects) { @@ -131,9 +135,18 @@ export async function listProjectsUsingSecretStore(opts: { unchecked += 1; continue; } - if ( - location.active && - (await sameStoreFile({ path: location.storePath, opts, target })) && + if (!location.active) { + continue; + } + const same = await sameStoreFile({ + path: location.storePath, + opts, + target, + }); + if (same === "unknown") { + unchecked += 1; + } else if ( + same === "same" && (await projectUsesSecretStore({ projectDir: project.projectDir })) ) { names.add(project.name); @@ -142,24 +155,51 @@ export async function listProjectsUsingSecretStore(opts: { return { names: [...names].sort(), registryRead: true, unchecked }; } -/** The same configured path, or the same existing file (device and inode). */ +/** + * A file's device and inode: `absent` when nothing is there, `unknown` when + * that cannot be established (for example EACCES or EIO). + */ +async function identity( + path: string +): Promise< + { readonly dev: number; readonly ino: number } | "absent" | "unknown" +> { + try { + const info = await stat(path); + return { dev: info.dev, ino: info.ino }; + } catch (error: unknown) { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { readonly code?: unknown }).code + : undefined; + return code === "ENOENT" || code === "ENOTDIR" ? "absent" : "unknown"; + } +} + +/** + * Whether a configured store path is the store: the same resolved path, or + * the same existing file (device and inode). `unknown` when a file's + * identity cannot be established, never taken as "different". + */ async function sameStoreFile(input: { readonly path: string; readonly opts: { readonly storePath: string }; - readonly target: { readonly dev: number; readonly ino: number } | null; -}): Promise { + readonly target: Awaited>; +}): Promise<"same" | "different" | "unknown"> { if (resolve(input.path) === resolve(input.opts.storePath)) { - return true; + return "same"; + } + const candidate = await identity(input.path); + if (candidate === "absent" || input.target === "absent") { + return "different"; } - if (input.target === null) { - return false; + if (candidate === "unknown" || input.target === "unknown") { + return "unknown"; } - const candidate = await stat(input.path).catch(() => null); - return ( - candidate !== null && - candidate.dev === input.target.dev && + return candidate.dev === input.target.dev && candidate.ino === input.target.ino - ); + ? "same" + : "different"; } /** @@ -179,6 +219,8 @@ export type SecretStoreRetireOutcome = readonly kind: "retired"; readonly from: string; readonly to: string; + /** A replacement found at the store path while moving, kept too. */ + readonly replacement: string | null; readonly keyPath: string; } | { readonly kind: "refused"; readonly message: string }; diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index 6057b603b..f84fc9714 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -3,18 +3,21 @@ import { createDecipheriv, createHash, randomBytes, + randomUUID, } from "node:crypto"; import { + link, lstat, - open, + mkdir, readdir, readFile, realpath, rename, + rmdir, stat, unlink, } from "node:fs/promises"; -import { basename, dirname, resolve } from "node:path"; +import { basename, dirname, join, resolve } from "node:path"; import { CliUsageError } from "../cli/command.ts"; import type { CloudSecretProvider, @@ -268,22 +271,30 @@ export class SecretStoreRetireError extends Error { } /** - * Moves an encrypted store file aside (HACK-2177), never deleting it. Under - * the store's mutation lock: + * Moves an encrypted store file aside (HACK-2177), never deleting it or + * replacing a file this call did not create. Under the store's lock: * * 1. the store must be a regular file, and not the key file itself (same * device and inode, so an alias or hard link of the key is refused); * 2. `recheck` runs (eligibility, classified again under the lock); - * 3. the archive name `.retired-` is created exclusively - * as an empty placeholder (refused if that name exists); - * 4. `rename` moves whatever file is at the store path at that instant over - * the placeholder, after checking the placeholder is still the same - * empty file. + * 3. a new directory `.retired-/` is created exclusively + * (mode 0700; refused if the name exists); + * 4. the store gets a second name `/` with a hard link, + * which never replaces an existing file: that is the archive; + * 5. whatever is at the store path at that instant is renamed into the + * new directory under an unpredictable, checked-free `.source-` + * name. When it is the archived file (same + * device and inode) that extra name is removed; when a writer outside + * the lock had replaced the store, the replacement stays there too. * - * Nothing is unlinked: if a writer outside the lock replaced the store in - * between, the replacement is what gets archived, and the only file ever - * replaced is the empty placeholder this call created. Nothing is read or - * decrypted; the key file is not touched. + * Every rename target, and the only name ever removed, is inside the + * directory this call created; nothing outside it is replaced or removed. + * The one window left is a file planted at the unpredictable source name + * between its check and the rename, by a process writing into that + * private (0700) directory. + * On a failure before step 5 the directory is removed with rmdir, which + * refuses unless it is empty. Nothing is read or decrypted; the key file + * is not touched. */ export async function retireEncryptedStoreFile(input: { readonly storePath: string; @@ -297,11 +308,19 @@ export async function retireEncryptedStoreFile(input: { */ readonly recheck?: () => Promise; /** - * Test seam: runs just before the rename, after the placeholder check, - * so a test can replace the store as a writer outside the lock would. + * Test seam: runs after the archive link, just before the store path is + * renamed, so a test can replace the store as a writer outside the lock + * would. */ readonly beforeMove?: () => Promise; -}): Promise<{ readonly from: string; readonly to: string }> { + /** Test seam: runs right after the archive directory is created. */ + readonly afterDirectory?: (dir: string) => Promise; +}): Promise<{ + readonly from: string; + readonly to: string; + /** Set when a replacement was found at the store path and kept too. */ + readonly replacement: string | null; +}> { const storePath = input.storePath; return await withEncryptedStoreMutation({ filePath: storePath, @@ -315,15 +334,18 @@ export async function retireEncryptedStoreFile(input: { .toISOString() .replace(STAMP_SEPARATORS, "") .replace(STAMP_MILLISECONDS, "Z"); - const to = `${storePath}${RETIRED_STORE_INFIX}${stamp}`; - const placeholder = await createRetirePlaceholder({ storePath, to }); - await moveStoreOverPlaceholder({ + const dir = `${storePath}${RETIRED_STORE_INFIX}${stamp}`; + const to = join(dir, basename(storePath)); + await createRetireDirectory({ storePath, dir }); + await input.afterDirectory?.(dir); + const archived = await linkIntoRetireDirectory({ storePath, dir, to }); + await input.beforeMove?.(); + const replacement = await moveSourceIntoRetireDirectory({ storePath, - to, - placeholder, - beforeMove: input.beforeMove, + dir, + archived, }); - return { from: storePath, to }; + return { from: storePath, to, replacement }; }, }); } @@ -359,80 +381,84 @@ async function assertRetirableStoreFile(input: { } } -/** - * Renames the store over the placeholder, after checking the placeholder is - * still the same empty file; on any failure the placeholder (only if still - * ours) is removed and nothing has moved. - */ -async function moveStoreOverPlaceholder(input: { +/** Creates the archive directory exclusively (0700); refuses if the name exists. */ +async function createRetireDirectory(input: { readonly storePath: string; - readonly to: string; - readonly placeholder: { readonly dev: number; readonly ino: number }; - readonly beforeMove?: () => Promise; + readonly dir: string; }): Promise { try { - const current = await lstat(input.to).catch(() => null); - if ( - current === null || - !current.isFile() || - current.dev !== input.placeholder.dev || - current.ino !== input.placeholder.ino || - current.size !== 0 - ) { - throw new SecretStoreRetireError( - `${input.to} changed while retiring; nothing was moved and it was left as it is.` - ); - } - await input.beforeMove?.(); - await rename(input.storePath, input.to); + await mkdir(input.dir, { mode: 0o700 }); } catch (error: unknown) { - await removeOwnPlaceholder(input); - if (error instanceof SecretStoreRetireError) { - throw error; - } throw new SecretStoreRetireError( - `Could not move ${input.storePath} to ${input.to} (${errorCode(error)}); nothing was moved. To retire it by hand: mv ${quoteShellWord(input.storePath)} ${quoteShellWord(input.to)}` + errorCode(error) === "EEXIST" + ? `${input.dir} already exists; nothing was moved. Retry in a second.` + : `Could not create ${input.dir} (${errorCode(error)}); nothing was moved. To retire the store by hand: mkdir ${quoteShellWord(input.dir)} && mv ${quoteShellWord(input.storePath)} ${quoteShellWord(input.dir)}/` ); } } -/** Creates the archive name exclusively (an empty 0600 file); refuses if it exists. */ -async function createRetirePlaceholder(input: { +/** + * Links the store into the archive directory (a hard link never replaces an + * existing file) and returns the archived file's identity. On failure the + * directory is removed only if it is still empty (rmdir refuses otherwise). + */ +async function linkIntoRetireDirectory(input: { readonly storePath: string; + readonly dir: string; readonly to: string; }): Promise<{ readonly dev: number; readonly ino: number }> { - let handle: Awaited>; try { - handle = await open(input.to, "wx", 0o600); + await link(input.storePath, input.to); + const archived = await lstat(input.to); + return { dev: archived.dev, ino: archived.ino }; } catch (error: unknown) { + await rmdir(input.dir).catch(() => undefined); throw new SecretStoreRetireError( - errorCode(error) === "EEXIST" - ? `${input.to} already exists; nothing was moved. Retry in a second.` - : `Could not create ${input.to} (${errorCode(error)}); nothing was moved. To retire it by hand: mv ${quoteShellWord(input.storePath)} ${quoteShellWord(input.to)}` + `Could not link ${input.storePath} into ${input.dir} (${errorCode(error)}); nothing was moved. To retire it by hand: mv ${quoteShellWord(input.storePath)} ${quoteShellWord(input.dir)}/` ); } - try { - const created = await handle.stat(); - return { dev: created.dev, ino: created.ino }; - } finally { - await handle.close(); - } } -/** Removes the placeholder only while it is still the empty file this call made. */ -async function removeOwnPlaceholder(input: { - readonly to: string; - readonly placeholder: { readonly dev: number; readonly ino: number }; -}): Promise { - const current = await lstat(input.to).catch(() => null); +/** + * Renames whatever is at the store path into the archive directory, and + * removes that extra name only when it is the archived file itself. A + * replacement written meanwhile by a writer outside the lock is kept, and + * its path returned. Both names are inside the directory this call made. + */ +async function moveSourceIntoRetireDirectory(input: { + readonly storePath: string; + readonly dir: string; + readonly archived: { readonly dev: number; readonly ino: number }; +}): Promise { + // An unpredictable name in the directory this call created, checked to + // be free first: the rename never replaces a file planted there. + const source = join(input.dir, `.source-${randomUUID()}`); + if ((await lstat(source).catch(() => null)) !== null) { + throw new SecretStoreRetireError( + `The store was archived at ${join(input.dir, basename(input.storePath))}, but ${source} appeared meanwhile, so ${input.storePath} was left in place. Nothing was deleted; remove ${quoteShellWord(input.storePath)} by hand once you have checked it.` + ); + } + try { + await rename(input.storePath, source); + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") { + // Already gone: the archive holds the store; nothing else to keep. + return null; + } + throw new SecretStoreRetireError( + `The store was archived at ${join(input.dir, basename(input.storePath))}, but ${input.storePath} could not be moved (${errorCode(error)}), so both names hold it. Nothing was deleted. Remove ${quoteShellWord(input.storePath)} by hand once you have checked it, or keep the archive.` + ); + } + const moved = await lstat(source).catch(() => null); if ( - current?.isFile() === true && - current.dev === input.placeholder.dev && - current.ino === input.placeholder.ino && - current.size === 0 + moved?.isFile() === true && + moved.dev === input.archived.dev && + moved.ino === input.archived.ino ) { - await unlink(input.to).catch(() => undefined); + await unlink(source); + return null; } + return source; } function errorCode(error: unknown): string { @@ -462,16 +488,33 @@ export async function isEncryptedStoreFile( } } -/** Retired copies of `storePath` in its directory, oldest first. */ +/** + * Retired copies of `storePath`, oldest first: `/` for + * each `.retired-/` directory (and a bare file of that name, + * as an earlier build of this command left). + */ export async function listRetiredEncryptedStores( storePath: string ): Promise { const prefix = `${basename(storePath)}${RETIRED_STORE_INFIX}`; - const entries = await readdir(dirname(storePath)).catch(() => [] as string[]); - return entries - .filter((name) => name.startsWith(prefix)) - .sort() - .map((name) => resolve(dirname(storePath), name)); + const entries = await readdir(dirname(storePath), { + withFileTypes: true, + }).catch(() => []); + const candidates = entries + .filter((entry) => entry.name.startsWith(prefix)) + .sort((a, b) => a.name.localeCompare(b.name)) + .map((entry) => { + const path = resolve(dirname(storePath), entry.name); + return entry.isDirectory() ? join(path, basename(storePath)) : path; + }); + // Only archives still there: a directory emptied by a restore is not one. + const present: string[] = []; + for (const path of candidates) { + if ((await lstat(path).catch(() => null))?.isFile() === true) { + present.push(path); + } + } + return present; } /** The listing's reason for each way a store file read can fail. */ diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 6f8546483..32623d025 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -274,7 +274,8 @@ test("retire-store refuses without --yes when it cannot prompt, then retires, an name.startsWith("secrets.enc.json.retired-") ); expect(kept).toHaveLength(1); - const retiredPath = join(fixture.hackHome, kept[0] ?? ""); + // The archive is the store, under its own name, in a new directory. + const retiredPath = join(fixture.hackHome, kept[0] ?? "", "secrets.enc.json"); expect(await readFile(retiredPath)).toEqual(before); expect(retired.output).toContain(`to ${retiredPath}`); expect(retired.output).toContain(`mv ${retiredPath} ${fixture.storePath}`); @@ -289,7 +290,7 @@ test("retire-store refuses without --yes when it cannot prompt, then retires, an expect(after).toHaveLength(1); expect(after[0]?.status).toBe("info"); expect(after[0]?.message).toContain( - `Retired secret store kept at ~/.hack/${kept[0]}` + `Retired secret store kept at ~/.hack/${kept[0]}/secrets.enc.json` ); expect(after[0]?.message).toContain( `mv ${retiredPath} ${fixture.storePath}` @@ -529,6 +530,7 @@ test("a store replaced at the moment of the move is archived, never deleted, and // Second-family review blocker: link then unlink removed a replacement a // writer outside the lock put at the store path between the two steps. const fixture = await setUp(); + const before = await readFile(fixture.storePath); const replacement = Buffer.from("synthetic-replacement-store-bytes\n"); const outcome = await retireEncryptedStoreFile({ storePath: fixture.storePath, @@ -541,11 +543,27 @@ test("a store replaced at the moment of the move is archived, never deleted, and await rename(staged, fixture.storePath); }, }); - expect(outcome.to).toBe(`${fixture.storePath}.retired-20261113T120000Z`); - // The replacement is what was archived; nothing at either name was lost. - expect(await readFile(outcome.to)).toEqual(replacement); + const dir = `${fixture.storePath}.retired-20261113T120000Z`; + expect(outcome.to).toBe(join(dir, "secrets.enc.json")); + // Neither file is lost: the archive holds the store that was linked, and + // the replacement put at the store path meanwhile is kept beside it. + expect(await readFile(outcome.to)).toEqual(before); + expect(outcome.replacement?.startsWith(join(dir, ".source-"))).toBe(true); + expect(await readFile(outcome.replacement ?? "")).toEqual(replacement); expect(await lstat(fixture.storePath).catch(() => null)).toBeNull(); + // Without a replacement, the extra name is removed: only the archive. + const plain = join(fixture.hackHome, "plain.enc.json"); + await writeFile(plain, "synthetic-plain-store\n"); + const plainOutcome = await retireEncryptedStoreFile({ + storePath: plain, + now: new Date("2026-11-13T12:00:00Z"), + }); + expect(plainOutcome.replacement).toBeNull(); + expect(await readdir(`${plain}.retired-20261113T120000Z`)).toEqual([ + "plain.enc.json", + ]); + // An archive name that already exists is never replaced. const second = join(fixture.hackHome, "secrets.enc.json"); await writeFile(second, "synthetic-second-store\n"); @@ -555,8 +573,27 @@ test("a store replaced at the moment of the move is archived, never deleted, and now: new Date("2026-11-13T12:00:00Z"), }) ).rejects.toThrow("already exists; nothing was moved"); - expect(await readFile(outcome.to)).toEqual(replacement); + expect(await readFile(outcome.to)).toEqual(before); + expect(await readFile(outcome.replacement ?? "")).toEqual(replacement); expect(await readFile(second, "utf8")).toBe("synthetic-second-store\n"); + + // A file planted at the archive name, inside the new directory, before + // the link: the link refuses, and nothing is removed or replaced. + const third = join(fixture.hackHome, "third.enc.json"); + await writeFile(third, "synthetic-third-store\n"); + let planted = ""; + await expect( + retireEncryptedStoreFile({ + storePath: third, + now: new Date("2026-11-13T12:00:00Z"), + afterDirectory: async (made) => { + planted = join(made, "third.enc.json"); + await writeFile(planted, "synthetic-planted\n"); + }, + }) + ).rejects.toThrow("nothing was moved"); + expect(await readFile(planted, "utf8")).toBe("synthetic-planted\n"); + expect(await readFile(third, "utf8")).toBe("synthetic-third-store\n"); }); test("a store that is its own key file is not retired, through any spelling", async () => { @@ -769,3 +806,54 @@ test("printed chmod and git rm guidance quotes a hostile key path", async () => expect(refused?.message).toContain(`chmod 600 ${quoted}`); expect(refused?.message).toContain(`git rm --cached ${quoted}`); }); + +test('a malformed registry, or a store identity that cannot be read, is never "no registered project reads it"', async () => { + // Second-family review: an invalid registry read as empty, and an + // identity lookup failure counted as "different", both produced the + // definitive no-readers wording. + const fixture = await setUp(); + const registry = join(fixture.hackHome, "projects.json"); + const text = await readFile(registry, "utf8"); + await writeFile(registry, "{ not a registry"); + try { + const malformed = (await inspect(fixture.bystander, "app-b"))[0]; + expect(malformed?.message).toContain( + "The projects registry could not be read or is not valid" + ); + expect(malformed?.message).not.toContain("No registered project reads it."); + } finally { + await writeFile(registry, text); + } + if (process.getuid?.() === 0) { + return; + } + // app-a reaches the store through an alias whose directory cannot be + // searched: its identity is unknown, so it counts as unchecked. + const hidden = join(fixture.home, "hidden"); + await mkdir(hidden); + await symlink(fixture.hackHome, join(hidden, "alias")); + await writeFile( + join(fixture.reader.projectDir, "hack.config.json"), + `${JSON.stringify({ + name: "app-a", + dev_host: "app-a.hack", + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { + path: join(hidden, "alias", "secrets.enc.json"), + keyPath: fixture.keyPath, + }, + }, + }, + })}\n` + ); + await chmod(hidden, 0o000); + try { + const unknown = (await inspect(fixture.bystander, "app-b"))[0]; + expect(unknown?.message).toContain("could not be checked"); + expect(unknown?.message).not.toContain("No registered project reads it."); + } finally { + await chmod(hidden, 0o755); + } +}); From 484c874ea82e6d424de1925f1a7fbcdd88c19fd9 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 15:29:02 +0000 Subject: [PATCH 09/22] fix(secrets): recovery advice that never overwrites, and count invalid registry entries as unchecked (HACK-2177) - A link refused because a file appeared at the archive name leaves both files and suggests no move. Any other link failure removes the empty directory and suggests a fresh mkdir plus mv -n (no overwrite); when the directory is no longer empty it is left alone. - readProjectsRegistryStrict reports how many project entries it had to skip as invalid; the reader scan counts them as unchecked. - The retire comment states the private-directory assumption precisely. Co-Authored-By: Claude Opus 5.5 --- src/lib/projects-registry.ts | 20 +++++++++-- src/lib/secret-store-retire.ts | 5 ++- src/lib/secret-store.ts | 27 ++++++++++++--- tests/secret-store-retire.test.ts | 57 ++++++++++++++++++++++++++++++- 4 files changed, 100 insertions(+), 9 deletions(-) diff --git a/src/lib/projects-registry.ts b/src/lib/projects-registry.ts index c1581764a..9c8dd8851 100644 --- a/src/lib/projects-registry.ts +++ b/src/lib/projects-registry.ts @@ -182,7 +182,12 @@ export async function readProjectsRegistryStrict(opts?: { readonly registryPath?: string; }): Promise< | { readonly kind: "absent" | "invalid" } - | { readonly kind: "valid"; readonly registry: ProjectsRegistry } + | { + readonly kind: "valid"; + readonly registry: ProjectsRegistry; + /** Project entries the registry holds that are not valid (skipped). */ + readonly rejected: number; + } > { const text = await readRegistryText(opts?.registryPath ?? getRegistryPath()); if (text === null) { @@ -195,7 +200,18 @@ export async function readProjectsRegistryStrict(opts?: { return { kind: "invalid" }; } const registry = parseRegistry(parsed); - return registry === null ? { kind: "invalid" } : { kind: "valid", registry }; + if (registry === null) { + return { kind: "invalid" }; + } + // The best-effort parser skips invalid entries; count them, so a caller + // never presents a filtered list as complete. + const raw = isRecord(parsed) ? parsed.projects : undefined; + const total = Array.isArray(raw) ? raw.length : registry.projects.length; + return { + kind: "valid", + registry, + rejected: total - registry.projects.length, + }; } type RegistrationUpsertOptions = { diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index 621d9d104..9188b7fb0 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -105,18 +105,21 @@ export async function listProjectsUsingSecretStore(opts: { readonly name: string; readonly projectDir: string; }[]; + // Registry entries that are not valid projects: unknown, so unchecked. + let rejected = 0; try { const read = await readProjectsRegistryStrict(); if (read.kind === "invalid") { return { names: [], registryRead: false, unchecked: 0 }; } projects = read.kind === "valid" ? read.registry.projects : []; + rejected = read.kind === "valid" ? read.rejected : 0; } catch { return { names: [], registryRead: false, unchecked: 0 }; } const target = await identity(opts.storePath); const names = new Set(); - let unchecked = 0; + let unchecked = rejected; for (const project of projects) { let location: Awaited>; try { diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index f84fc9714..daa66bc54 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -289,9 +289,11 @@ export class SecretStoreRetireError extends Error { * * Every rename target, and the only name ever removed, is inside the * directory this call created; nothing outside it is replaced or removed. - * The one window left is a file planted at the unpredictable source name - * between its check and the rename, by a process writing into that - * private (0700) directory. + * This assumes no other process writes into that fresh private (0700) + * directory: one that does could plant a file at the unpredictable source + * name between its check and the rename, or swap that name between the + * same-file check and its removal. It is not a guarantee against such a + * process. * On a failure before step 5 the directory is removed with rmdir, which * refuses unless it is empty. Nothing is read or decrypted; the key file * is not touched. @@ -412,9 +414,24 @@ async function linkIntoRetireDirectory(input: { const archived = await lstat(input.to); return { dev: archived.dev, ino: archived.ino }; } catch (error: unknown) { - await rmdir(input.dir).catch(() => undefined); + const code = errorCode(error); + if (code === "EEXIST") { + // Something appeared at the archive name: leave it and the directory + // as they are, and suggest no move that could overwrite it. + throw new SecretStoreRetireError( + `A file appeared at ${input.to} while retiring; it and ${input.storePath} were left as they are, and nothing was moved. Inspect ${quoteShellWord(input.dir)}, then retry (a new directory name is used).` + ); + } + // The directory is still empty unless something else wrote into it; + // rmdir refuses otherwise. + const removed = await rmdir(input.dir).then( + () => true, + () => false + ); throw new SecretStoreRetireError( - `Could not link ${input.storePath} into ${input.dir} (${errorCode(error)}); nothing was moved. To retire it by hand: mv ${quoteShellWord(input.storePath)} ${quoteShellWord(input.dir)}/` + removed + ? `Could not link ${input.storePath} into ${input.dir} (${code}; this filesystem may not support hard links); nothing was moved. To retire it by hand without overwriting anything: mkdir ${quoteShellWord(input.dir)} && mv -n ${quoteShellWord(input.storePath)} ${quoteShellWord(input.dir)}/` + : `Could not link ${input.storePath} into ${input.dir} (${code}), and ${input.dir} is no longer empty, so it was left as it is; nothing was moved. Inspect it before retrying.` ); } } diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 32623d025..a01a858f6 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -591,9 +591,36 @@ test("a store replaced at the moment of the move is archived, never deleted, and await writeFile(planted, "synthetic-planted\n"); }, }) - ).rejects.toThrow("nothing was moved"); + ).rejects.toThrow("left as they are"); expect(await readFile(planted, "utf8")).toBe("synthetic-planted\n"); expect(await readFile(third, "utf8")).toBe("synthetic-third-store\n"); + // That refusal suggests no move that could overwrite the planted file. + const collision = await retireEncryptedStoreFile({ + storePath: third, + now: new Date("2026-11-13T12:00:01Z"), + afterDirectory: async (made) => { + await writeFile(join(made, "third.enc.json"), "synthetic-planted\n"); + }, + }).catch((error: unknown) => String(error)); + expect(collision).not.toContain("mv "); + + // Any other link failure: the empty directory is removed, and the manual + // step makes a fresh one and moves without overwriting (mv -n). + const fourth = join(fixture.hackHome, "fourth.enc.json"); + await writeFile(fourth, "synthetic-fourth-store\n"); + const fourthDir = `${fourth}.retired-20261113T120000Z`; + const other = await retireEncryptedStoreFile({ + storePath: fourth, + now: new Date("2026-11-13T12:00:00Z"), + afterDirectory: async () => { + // The store vanishes, so the link fails with ENOENT, not EEXIST. + await rename(fourth, `${fourth}.away`); + }, + }).catch((error: unknown) => String(error)); + expect(other).toContain( + `mkdir ${fourthDir} && mv -n ${fourth} ${fourthDir}/` + ); + expect(await lstat(fourthDir).catch(() => null)).toBeNull(); }); test("a store that is its own key file is not retired, through any spelling", async () => { @@ -857,3 +884,31 @@ test('a malformed registry, or a store identity that cannot be read, is never "n await chmod(hidden, 0o755); } }); + +test("registry entries that are not valid projects make the reader scan incomplete", async () => { + // Second-family review: the strict read still skipped invalid project + // entries silently, so a filtered list read as complete. + const fixture = await setUp(); + const registry = join(fixture.hackHome, "projects.json"); + const text = await readFile(registry, "utf8"); + const parsed = JSON.parse(text) as { projects: unknown[] }; + // Mixed: the valid entries plus one that is not a project. + await writeFile( + registry, + JSON.stringify({ ...parsed, projects: [...parsed.projects, { id: 5 }] }) + ); + const mixed = (await inspect(fixture.bystander, "app-b"))[0]; + expect(mixed?.message).toContain("Registered projects that read it: app-a."); + expect(mixed?.message).toContain( + "1 registered project(s) could not be checked" + ); + // Only an invalid entry: never "No registered project reads it." + await writeFile( + registry, + JSON.stringify({ ...parsed, projects: [{ id: 5 }] }) + ); + const only = (await inspect(fixture.bystander, "app-b"))[0]; + expect(only?.message).toContain("could not be checked"); + expect(only?.message).not.toContain("No registered project reads it."); + await writeFile(registry, text); +}); From 072955eb9a234a2888213f764bfe71e587255f6a Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 16:01:46 +0000 Subject: [PATCH 10/22] fix(secrets): pin a store reached through a symlinked directory to its real path before retiring (HACK-2177) The configured store path is project input; a symlinked directory on the way could lead to another store while the prompt showed the alias. Retire now resolves the store's real path once, acts only on it, and checks under the lock that the configured path still leads there. The confirmation and the result name the real path, with the configured alias when it differs. Co-Authored-By: Claude Opus 5.5 --- src/commands/secrets.ts | 19 +++++++----- src/lib/secret-store-retire.ts | 49 +++++++++++++++++++++++++++++-- tests/secret-store-retire.test.ts | 49 ++++++++++++++++++++++++++++++- 3 files changed, 106 insertions(+), 11 deletions(-) diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index 395bb88e6..1212cc8a4 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -12,8 +12,10 @@ import { import { keychain } from "../lib/keychain.ts"; import { quoteShellWord } from "../lib/logs.ts"; import { findProjectContext, readProjectConfig } from "../lib/project.ts"; -import { resolveFileSecretStorePaths } from "../lib/secret-store.ts"; -import { retireSecretStore } from "../lib/secret-store-retire.ts"; +import { + describeRetireTarget, + retireSecretStore, +} from "../lib/secret-store-retire.ts"; import { logger } from "../ui/logger.ts"; const DEFAULT_SECRETS_SERVICE = "hack-cli" as const; @@ -278,14 +280,15 @@ async function handleSecretsRetireStore({ "project" ).trim(); } - // Name the resolved file in the prompt: the configured path is project input. - const target = await resolveFileSecretStorePaths({ + // Name the real file in the prompt: the configured path is project input + // and may reach another store through a symlinked directory. + const target = await describeRetireTarget({ projectDir: project?.projectDir, - }).catch(() => null); + }); const ok = args.options.yes === true || (await confirmSafe({ - message: `Move the encrypted secret store ${target?.storePath ?? "(unknown path)"} aside? Its entries stay in the moved file, which can be put back.`, + message: `Move the encrypted secret store ${target ?? "(unknown path)"} aside? Its entries stay in the moved file, which can be put back.`, initialValue: false, nonInteractive: "fail", hint: "Pass --yes to confirm (`hack secrets retire-store --yes`), or run it in a terminal.", @@ -303,7 +306,9 @@ async function handleSecretsRetireStore({ } logger.success({ message: [ - `Moved the encrypted secret store ${outcome.from} to ${outcome.to}.`, + outcome.configured === outcome.from + ? `Moved the encrypted secret store ${outcome.from} to ${outcome.to}.` + : `Moved the encrypted secret store ${outcome.from} (configured as ${outcome.configured}) to ${outcome.to}.`, "Nothing was deleted, and no entry was read or printed. The key file was left alone.", `To put it back (while no store is at ${outcome.from}): mv ${quoteShellWord(outcome.to)} ${quoteShellWord(outcome.from)}`, ...(outcome.replacement === null diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index 9188b7fb0..b4185fe00 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -1,4 +1,4 @@ -import { stat } from "node:fs/promises"; +import { realpath, stat } from "node:fs/promises"; import { resolve } from "node:path"; import { PROJECT_CONFIG_FILENAME } from "../constants.ts"; @@ -224,6 +224,8 @@ export type SecretStoreRetireOutcome = readonly to: string; /** A replacement found at the store path while moving, kept too. */ readonly replacement: string | null; + /** The configured store path; `from` is its real path. */ + readonly configured: string; readonly keyPath: string; } | { readonly kind: "refused"; readonly message: string }; @@ -254,14 +256,29 @@ export async function retireSecretStore(opts: { if (refusal !== null) { return { kind: "refused", message: refusal }; } + // The configured path is project input and may reach the store through a + // symlinked directory: pin the real path, act only on it, and check under + // the lock that the configured path still leads there. + const canonical = await realpath(listing.storePath).catch(() => null); + if (canonical === null) { + return { + kind: "refused", + message: `The real path of ${listing.storePath} could not be resolved; nothing was moved.`, + }; + } try { const moved = await retireEncryptedStoreFile({ - storePath: listing.storePath, + storePath: canonical, keyPath: listing.keyPath, now: opts.now, // Classified before the lock: classify again under it, so a store // whose key came back, or that was replaced, is not retired. recheck: async () => { + if ( + (await realpath(listing.storePath).catch(() => null)) !== canonical + ) { + return `${listing.storePath} no longer leads to ${canonical}; nothing was moved. (It changed while retiring.)`; + } const unsure = await secretStoreSelectionProblem(opts); if (unsure !== null) { return `${unsure} Nothing was moved. (It changed while retiring.)`; @@ -277,7 +294,12 @@ export async function retireSecretStore(opts: { : `${changed} (It changed while retiring.)`; }, }); - return { kind: "retired", ...moved, keyPath: listing.keyPath }; + return { + kind: "retired", + ...moved, + configured: listing.storePath, + keyPath: listing.keyPath, + }; } catch (error: unknown) { if (error instanceof SecretStoreRetireError) { return { kind: "refused", message: error.message }; @@ -286,6 +308,27 @@ export async function retireSecretStore(opts: { } } +/** + * What `hack secrets retire-store` would move, for its confirmation: the + * store's real path, and the configured path when that differs (a symlinked + * directory on the way). Null when no file store is configured here. + */ +export async function describeRetireTarget(opts: { + readonly projectDir?: string; +}): Promise { + const location = await resolveFileSecretStorePaths(opts).catch(() => null); + if (location === null || !location.active) { + return null; + } + const canonical = await realpath(location.storePath).catch(() => null); + if (canonical === null) { + return location.storePath; + } + return canonical === resolve(location.storePath) + ? canonical + : `${canonical} (configured as ${location.storePath})`; +} + /** * Refuses a configured store path that does not hold a Hack encrypted * store envelope: the path is project input and may name any file. diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index a01a858f6..9d4226be9 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -33,7 +33,10 @@ import { resolveSecretStore, retireEncryptedStoreFile, } from "../src/lib/secret-store.ts"; -import { retireSecretStore } from "../src/lib/secret-store-retire.ts"; +import { + describeRetireTarget, + retireSecretStore, +} from "../src/lib/secret-store-retire.ts"; import { restoreEnv } from "./helpers/env.ts"; import { privateCliEnv } from "./helpers/private-cli-env.ts"; @@ -912,3 +915,47 @@ test("registry entries that are not valid projects make the reader scan incomple expect(only?.message).not.toContain("No registered project reads it."); await writeFile(registry, text); }); + +test("a store reached through a symlinked directory is named and retired by its real path", async () => { + // Codex security review: lstat checked only the last component, so a + // repo-controlled symlinked directory could lead to another store while + // the prompt showed the alias. + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + const alias = join(fixture.bystander.projectRoot, "linked"); + await symlink(fixture.hackHome, alias); + const configured = join(alias, "secrets.enc.json"); + await writeFile( + join(fixture.bystander.projectDir, "hack.config.json"), + `${JSON.stringify({ + name: "app-b", + dev_host: "app-b.hack", + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { path: configured, keyPath: fixture.keyPath }, + }, + }, + })}\n` + ); + // The confirmation names the real file, and the configured alias. + expect( + await describeRetireTarget({ projectDir: fixture.bystander.projectDir }) + ).toBe(`${fixture.storePath} (configured as ${configured})`); + const outcome = await retireSecretStore({ + projectName: "app-b", + projectDir: fixture.bystander.projectDir, + now: new Date("2026-11-13T12:00:00Z"), + }); + expect(outcome.kind).toBe("retired"); + if (outcome.kind !== "retired") { + return; + } + // It acted on the real path: the archive sits beside the real store. + expect(outcome.from).toBe(fixture.storePath); + expect(outcome.configured).toBe(configured); + expect(outcome.to).toBe( + join(`${fixture.storePath}.retired-20261113T120000Z`, "secrets.enc.json") + ); + expect(await readFile(outcome.to)).toEqual(before); +}); From 31f64f91a84c84ee6a49509e1d0b98158fd052d5 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 16:12:33 +0000 Subject: [PATCH 11/22] fix(secrets): bind the confirmed retire target and check eligibility on the pinned store (HACK-2177) - planSecretStoreRetire resolves the target once (configured path, real path, key path) and classifies the store at its real path; the CLI confirms that plan and passes it to retireSecretStore, which refuses when a fresh plan differs, so a target retargeted while the prompt waits is not moved. - Under the lock, the recheck confirms the same config and paths and that the configured path still leads to the pinned real path, then classifies the store at that real path (classifyEncryptedStoreAt), never through the alias. Co-Authored-By: Claude Opus 5.5 --- src/commands/secrets.ts | 28 ++-- src/lib/secret-store-retire.ts | 246 +++++++++++++++++++----------- src/lib/secret-store.ts | 35 +++++ tests/secret-store-retire.test.ts | 102 +++++++++++++ 4 files changed, 310 insertions(+), 101 deletions(-) diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index 1212cc8a4..e1becd9c2 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -11,9 +11,9 @@ import { } from "../lib/interactivity.ts"; import { keychain } from "../lib/keychain.ts"; import { quoteShellWord } from "../lib/logs.ts"; -import { findProjectContext, readProjectConfig } from "../lib/project.ts"; +import { findProjectContext } from "../lib/project.ts"; import { - describeRetireTarget, + planSecretStoreRetire, retireSecretStore, } from "../lib/secret-store-retire.ts"; import { logger } from "../ui/logger.ts"; @@ -271,24 +271,20 @@ async function handleSecretsRetireStore({ ? resolve(process.cwd(), args.options.path) : process.cwd(); const project = await findProjectContext(startDir); - let projectName = "project"; - if (project) { - const cfg = await readProjectConfig(project); - projectName = ( - cfg.name ?? - project.projectRoot.split("/").at(-1) ?? - "project" - ).trim(); - } - // Name the real file in the prompt: the configured path is project input - // and may reach another store through a symlinked directory. - const target = await describeRetireTarget({ + // Plan once: the confirmation names this exact target (its real path; the + // configured path is project input and may go through a symlinked + // directory), and the retirement refuses if the target changed since. + const planned = await planSecretStoreRetire({ projectDir: project?.projectDir, }); + if (planned.kind === "refused") { + logger.error({ message: planned.message }); + return 1; + } const ok = args.options.yes === true || (await confirmSafe({ - message: `Move the encrypted secret store ${target ?? "(unknown path)"} aside? Its entries stay in the moved file, which can be put back.`, + message: `Move the encrypted secret store ${planned.plan.display} aside? Its entries stay in the moved file, which can be put back.`, initialValue: false, nonInteractive: "fail", hint: "Pass --yes to confirm (`hack secrets retire-store --yes`), or run it in a terminal.", @@ -297,8 +293,8 @@ async function handleSecretsRetireStore({ return 0; } const outcome = await retireSecretStore({ - projectName, projectDir: project?.projectDir, + approved: planned.plan, }); if (outcome.kind === "refused") { logger.error({ message: outcome.message }); diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index b4185fe00..d2fabe8c1 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -8,9 +8,8 @@ import { readOptionalTextFile } from "./fs.ts"; import { readHackEnvContract, readHackEnvRuntimeConfig } from "./hack-env.ts"; import { readProjectsRegistryStrict } from "./projects-registry.ts"; import { - type FileSecretStoreListing, + classifyEncryptedStoreAt, isEncryptedStoreFile, - listFileSecretStoreEntries, resolveFileSecretStorePaths, retireEncryptedStoreFile, SecretStoreRetireError, @@ -205,17 +204,6 @@ async function sameStoreFile(input: { : "different"; } -/** - * Where a project's store is, and whether its backend is active. The - * listing reads the store only to classify it, and never provisions a key. - */ -async function storeLocation(opts: { - readonly projectName: string; - readonly projectDir?: string; -}): Promise { - return await listFileSecretStoreEntries(opts); -} - /** The outcome of `hack secrets retire-store` (paths only, never values). */ export type SecretStoreRetireOutcome = | { @@ -231,18 +219,34 @@ export type SecretStoreRetireOutcome = | { readonly kind: "refused"; readonly message: string }; /** - * Checks that the store selected for this context (a project, or the global - * configuration when `projectDir` is omitted) is one retiring is for, then - * moves it aside ({@link retireEncryptedStoreFile}). Refused, with the - * reason and nothing moved, when no file store is configured, the store is - * absent or empty, it opens with the key in use, or it cannot be read for - * another reason (its key file refused, the store unreadable or a link). + * What retiring would act on, resolved once (HACK-2177): the configured + * store path (project input, possibly through a symlinked directory), its + * real path, and the key path. Shown before confirmation, and carried into + * {@link retireSecretStore}, which refuses if a fresh plan differs. */ -export async function retireSecretStore(opts: { - readonly projectName: string; +export type SecretStoreRetirePlan = { + readonly configured: string; + readonly canonical: string; + readonly keyPath: string; + /** For the confirmation: the real path, and the configured one if it differs. */ + readonly display: string; +}; + +/** + * Plans retiring the store this context selects (a project, or the global + * configuration when `projectDir` is omitted). Refused, with the reason, + * when the selecting config is unsure, no file store is configured, its + * real path cannot be resolved, or the store at that real path is not one + * to retire (absent or empty, opens with the key in use, unreadable for a + * reason other than a missing or wrong key, or not a store envelope). + * Every store read uses the real path, never the configured alias. + */ +export async function planSecretStoreRetire(opts: { readonly projectDir?: string; - readonly now?: Date; -}): Promise { +}): Promise< + | { readonly kind: "plan"; readonly plan: SecretStoreRetirePlan } + | { readonly kind: "refused"; readonly message: string } +> { const selection = await secretStoreSelectionProblem(opts); if (selection !== null) { return { @@ -250,45 +254,78 @@ export async function retireSecretStore(opts: { message: `${selection} Nothing was moved. Fix that config, then run hack doctor and retry.`, }; } - const listing = await storeLocation(opts); - const refusal = - retireRefusal(listing) ?? (await notAStoreRefusal(listing.storePath)); + const location = await resolveFileSecretStorePaths(opts).catch(() => null); + if (location === null || !location.active) { + return { + kind: "refused", + message: + "No encrypted file secret store is configured here (the backend is the OS keychain); nothing was moved.", + }; + } + const canonical = await realpath(location.storePath).catch(() => null); + if (canonical === null) { + return { + kind: "refused", + message: `There is no encrypted secret store with entries at ${location.storePath}; nothing was moved.`, + }; + } + const refusal = await eligibilityRefusal({ + storePath: canonical, + keyPath: location.keyPath, + }); if (refusal !== null) { return { kind: "refused", message: refusal }; } - // The configured path is project input and may reach the store through a - // symlinked directory: pin the real path, act only on it, and check under - // the lock that the configured path still leads there. - const canonical = await realpath(listing.storePath).catch(() => null); - if (canonical === null) { + return { + kind: "plan", + plan: { + configured: location.storePath, + canonical, + keyPath: location.keyPath, + display: + canonical === resolve(location.storePath) + ? canonical + : `${canonical} (configured as ${location.storePath})`, + }, + }; +} + +/** + * Retires the store this context selects ({@link retireEncryptedStoreFile} + * on its real path). With `approved` (the plan shown before confirmation), + * refuses unless a fresh plan names the same configured path, real path + * and key, so a target retargeted while the confirmation waited is not + * moved. Under the store's lock it checks again: the same config and + * paths, the configured path still leading to the same real path, and the + * store at that real path still one to retire. + */ +export async function retireSecretStore(opts: { + /** Unused: the store is the file, whichever project scope it holds. */ + readonly projectName?: string; + readonly projectDir?: string; + readonly now?: Date; + readonly approved?: SecretStoreRetirePlan; + /** Test seam: runs under the lock after the path checks, before eligibility. */ + readonly beforeEligibility?: () => Promise; +}): Promise { + const planned = await planSecretStoreRetire(opts); + if (planned.kind === "refused") { + return planned; + } + const { plan } = planned; + if (opts.approved !== undefined && !samePlan(opts.approved, plan)) { return { kind: "refused", - message: `The real path of ${listing.storePath} could not be resolved; nothing was moved.`, + message: `The store to retire changed after it was confirmed (confirmed: ${opts.approved.display}; now: ${plan.display}); nothing was moved. Run the command again to confirm the current target.`, }; } try { const moved = await retireEncryptedStoreFile({ - storePath: canonical, - keyPath: listing.keyPath, + storePath: plan.canonical, + keyPath: plan.keyPath, now: opts.now, - // Classified before the lock: classify again under it, so a store - // whose key came back, or that was replaced, is not retired. recheck: async () => { - if ( - (await realpath(listing.storePath).catch(() => null)) !== canonical - ) { - return `${listing.storePath} no longer leads to ${canonical}; nothing was moved. (It changed while retiring.)`; - } - const unsure = await secretStoreSelectionProblem(opts); - if (unsure !== null) { - return `${unsure} Nothing was moved. (It changed while retiring.)`; - } - const current = await storeLocation(opts); - if (current.storePath !== listing.storePath) { - return "The secret store configuration changed while retiring; nothing was moved. Run hack doctor, then retry."; - } - const changed = - retireRefusal(current) ?? (await notAStoreRefusal(current.storePath)); + const changed = await lockedRecheck({ opts, plan }); return changed === null ? null : `${changed} (It changed while retiring.)`; @@ -297,8 +334,8 @@ export async function retireSecretStore(opts: { return { kind: "retired", ...moved, - configured: listing.storePath, - keyPath: listing.keyPath, + configured: plan.configured, + keyPath: plan.keyPath, }; } catch (error: unknown) { if (error instanceof SecretStoreRetireError) { @@ -308,25 +345,83 @@ export async function retireSecretStore(opts: { } } +function samePlan(a: SecretStoreRetirePlan, b: SecretStoreRetirePlan): boolean { + return ( + a.configured === b.configured && + a.canonical === b.canonical && + a.keyPath === b.keyPath + ); +} + /** - * What `hack secrets retire-store` would move, for its confirmation: the - * store's real path, and the configured path when that differs (a symlinked - * directory on the way). Null when no file store is configured here. + * Under the store's lock: the selecting config is still sure, still names + * the same store and key paths, the configured path still leads to the + * pinned real path, and the store at that real path (never the alias) is + * still one to retire. A refusal message, or null to go on. */ -export async function describeRetireTarget(opts: { - readonly projectDir?: string; +async function lockedRecheck(input: { + readonly opts: { + readonly projectDir?: string; + readonly beforeEligibility?: () => Promise; + }; + readonly plan: SecretStoreRetirePlan; }): Promise { + const { opts, plan } = input; + const unsure = await secretStoreSelectionProblem(opts); + if (unsure !== null) { + return `${unsure} Nothing was moved.`; + } const location = await resolveFileSecretStorePaths(opts).catch(() => null); - if (location === null || !location.active) { - return null; + if ( + location === null || + !location.active || + location.storePath !== plan.configured || + location.keyPath !== plan.keyPath + ) { + return "The secret store configuration changed; nothing was moved. Run hack doctor, then retry."; } - const canonical = await realpath(location.storePath).catch(() => null); - if (canonical === null) { - return location.storePath; + if ((await realpath(plan.configured).catch(() => null)) !== plan.canonical) { + return `${plan.configured} no longer leads to ${plan.canonical}; nothing was moved.`; } - return canonical === resolve(location.storePath) - ? canonical - : `${canonical} (configured as ${location.storePath})`; + await opts.beforeEligibility?.(); + return await eligibilityRefusal({ + storePath: plan.canonical, + keyPath: plan.keyPath, + }); +} + +/** + * Why the store at this (real) path is not one to retire, or null when it + * is: entries exist, the key that opens them is missing or wrong, and the + * file holds a store envelope. + */ +async function eligibilityRefusal(input: { + readonly storePath: string; + readonly keyPath: string; +}): Promise { + const state = await classifyEncryptedStoreAt(input); + if (state.kind === "empty") { + return `There is no encrypted secret store with entries at ${input.storePath}; nothing was moved.`; + } + if (state.kind === "opens") { + return `The encrypted secret store at ${input.storePath} opens with the key in use, so there is nothing to retire; nothing was moved.`; + } + if (!RETIRABLE_REASONS.has(state.reason)) { + return `The encrypted secret store at ${input.storePath} is not retired: it is not a missing or wrong key (${state.reason.replaceAll("_", " ")}). Run hack doctor for what to fix; nothing was moved.`; + } + return await notAStoreRefusal(input.storePath); +} + +/** + * What `hack secrets retire-store` would move, for its confirmation; null + * when there is nothing to retire here. {@link planSecretStoreRetire} + * carries the same target into the retirement. + */ +export async function describeRetireTarget(opts: { + readonly projectDir?: string; +}): Promise { + const planned = await planSecretStoreRetire(opts); + return planned.kind === "plan" ? planned.plan.display : null; } /** @@ -338,22 +433,3 @@ async function notAStoreRefusal(storePath: string): Promise { ? null : `${storePath} is not a Hack encrypted secret store (it does not hold the store's encrypted envelope), so it is not retired; nothing was moved. Check controlPlane.secrets.encryptedFile.path in the Hack config.`; } - -/** - * Why a store is not one to retire, or null when it is (entries exist and - * the key that opens them is missing or wrong). - */ -function retireRefusal(listing: FileSecretStoreListing): string | null { - if (listing.kind === "inactive") { - return "No encrypted file secret store is configured here (the backend is the OS keychain); nothing was moved."; - } - if (listing.kind === "listed") { - return listing.ownEntries.size + listing.foreignEntryCount === 0 - ? `There is no encrypted secret store with entries at ${listing.storePath}; nothing was moved.` - : `The encrypted secret store at ${listing.storePath} opens with the key in use, so there is nothing to retire; nothing was moved.`; - } - if (!RETIRABLE_REASONS.has(listing.reason)) { - return `The encrypted secret store at ${listing.storePath} is not retired: it is not a missing or wrong key (${listing.reason.replaceAll("_", " ")}). Run hack doctor for what to fix; nothing was moved.`; - } - return null; -} diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index daa66bc54..f9beec540 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -484,6 +484,41 @@ function errorCode(error: unknown): string { : "unknown"; } +/** + * Classifies the store file at an explicit `storePath` with an explicit + * `keyPath`, read-only (no key is provisioned): `empty` (absent or empty), + * `opens` (the available key decrypts it), or `unreadable` with the same + * reasons as {@link listFileSecretStoreEntries}. For a caller that has + * pinned a real path and must not follow a configured alias again + * (HACK-2177). + */ +export async function classifyEncryptedStoreAt(input: { + readonly storePath: string; + readonly keyPath: string; +}): Promise< + | { readonly kind: "empty" } + | { readonly kind: "opens" } + | { + readonly kind: "unreadable"; + readonly reason: Extract< + FileSecretStoreListing, + { kind: "unreadable" } + >["reason"]; + } +> { + const read = await readEncryptedStoreText(input.storePath); + if (read.kind === "refused" || read.kind === "unreadable") { + return { kind: "unreadable", reason: STORE_READ_FAILURE_REASON[read.kind] }; + } + if (read.kind === "absent" || read.text.length === 0) { + return { kind: "empty" }; + } + const opened = await openListedStore({ ...input, text: read.text }); + return "reason" in opened + ? { kind: "unreadable", reason: opened.reason } + : { kind: "opens" }; +} + /** * Whether the file at `storePath` is a Hack encrypted secret store: a * regular file (read without following a link) holding the store's JSON diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 9d4226be9..114787050 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -35,6 +35,7 @@ import { } from "../src/lib/secret-store.ts"; import { describeRetireTarget, + planSecretStoreRetire, retireSecretStore, } from "../src/lib/secret-store-retire.ts"; import { restoreEnv } from "./helpers/env.ts"; @@ -959,3 +960,104 @@ test("a store reached through a symlinked directory is named and retired by its ); expect(await readFile(outcome.to)).toEqual(before); }); + +/** + * Two keyless stores, A (the global store) and B (a copy elsewhere), and + * app-b configured through a repo-local symlinked directory that leads to + * A. Returns how to retarget that directory to B. + */ +async function aliasedStores(fixture: Fixture) { + const before = await readFile(fixture.storePath); + const otherDir = join(root, "other-store-dir"); + await mkdir(otherDir); + const storeB = join(otherDir, "secrets.enc.json"); + await writeFile(storeB, before); + const alias = join(fixture.bystander.projectRoot, "linked"); + await symlink(fixture.hackHome, alias); + await writeFile( + join(fixture.bystander.projectDir, "hack.config.json"), + `${JSON.stringify({ + name: "app-b", + dev_host: "app-b.hack", + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { + path: join(alias, "secrets.enc.json"), + keyPath: fixture.keyPath, + }, + }, + }, + })}\n` + ); + return { + before, + storeB, + retargetToB: async () => { + await rm(alias); + await symlink(otherDir, alias); + }, + }; +} + +test("a target retargeted after confirmation is not retired", async () => { + // Second-family review: the prompt resolved the real path for display, + // and the retirement resolved it again; a symlink retargeted while the + // prompt waited made it retire a store the user had not approved. + const fixture = await setUp(); + const { before, storeB, retargetToB } = await aliasedStores(fixture); + const planned = await planSecretStoreRetire({ + projectDir: fixture.bystander.projectDir, + }); + expect(planned.kind).toBe("plan"); + if (planned.kind !== "plan") { + return; + } + expect(planned.plan.canonical).toBe(fixture.storePath); + // While the confirmation waits, the repo's symlink is pointed at B. + await retargetToB(); + const outcome = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + approved: planned.plan, + }); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "changed after it was confirmed" + ); + expect(await readFile(fixture.storePath)).toEqual(before); + expect(await readFile(storeB)).toEqual(before); +}); + +test("the locked eligibility check reads the pinned store, not the alias", async () => { + // Second-family review: under the lock, eligibility was read through the + // alias after the path check; a retarget then validated B while A (whose + // key had just come back) was moved. + const fixture = await setUp(); + const { before, storeB, retargetToB } = await aliasedStores(fixture); + const planned = await planSecretStoreRetire({ + projectDir: fixture.bystander.projectDir, + }); + if (planned.kind !== "plan") { + throw new Error("expected a plan"); + } + const outcome = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + approved: planned.plan, + beforeEligibility: async () => { + // After the path check: A's key comes back, and the alias moves to B. + await writeFile(fixture.keyPath, `${STORE_KEY}\n`, { mode: 0o600 }); + await chmod(fixture.keyPath, 0o600); + await retargetToB(); + }, + }); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "opens with the key in use" + ); + // A, now recoverable, stays active; B is untouched. + expect(await readFile(fixture.storePath)).toEqual(before); + expect(await readFile(storeB)).toEqual(before); + expect( + (await readdir(fixture.hackHome)).filter((name) => + name.includes(".retired-") + ) + ).toEqual([]); +}); From 598c1ed87a3accacb35ed695fae3e42d59d92c34 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 16:29:19 +0000 Subject: [PATCH 12/22] fix(secrets): show control characters in configured paths instead of sending them to the terminal (HACK-2177) Store paths are project input; a symlinked directory named with a carriage return and an erase-line sequence could rewrite the retire confirmation on screen. A new escapeTerminalControls shows every C0/C1 control character as \xNN; retire-store applies it to its refusal, prompt and each result line, and the secret integrity findings apply it to every message. Co-Authored-By: Claude Opus 5.5 --- src/commands/secrets.ts | 13 ++++++--- src/lib/doctor-secret-integrity.ts | 10 ++++++- src/lib/terminal-text.ts | 17 ++++++++++++ tests/secret-store-retire.test.ts | 43 ++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 5 deletions(-) create mode 100644 src/lib/terminal-text.ts diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index e1becd9c2..4da4093ad 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -16,6 +16,7 @@ import { planSecretStoreRetire, retireSecretStore, } from "../lib/secret-store-retire.ts"; +import { escapeTerminalControls } from "../lib/terminal-text.ts"; import { logger } from "../ui/logger.ts"; const DEFAULT_SECRETS_SERVICE = "hack-cli" as const; @@ -278,13 +279,14 @@ async function handleSecretsRetireStore({ projectDir: project?.projectDir, }); if (planned.kind === "refused") { - logger.error({ message: planned.message }); + logger.error({ message: escapeTerminalControls(planned.message) }); return 1; } const ok = args.options.yes === true || (await confirmSafe({ - message: `Move the encrypted secret store ${planned.plan.display} aside? Its entries stay in the moved file, which can be put back.`, + // Paths are project input: show control characters, never send them. + message: `Move the encrypted secret store ${escapeTerminalControls(planned.plan.display)} aside? Its entries stay in the moved file, which can be put back.`, initialValue: false, nonInteractive: "fail", hint: "Pass --yes to confirm (`hack secrets retire-store --yes`), or run it in a terminal.", @@ -297,10 +299,11 @@ async function handleSecretsRetireStore({ approved: planned.plan, }); if (outcome.kind === "refused") { - logger.error({ message: outcome.message }); + logger.error({ message: escapeTerminalControls(outcome.message) }); return 1; } logger.success({ + // Each line escaped on its own: the newlines between them are intended. message: [ outcome.configured === outcome.from ? `Moved the encrypted secret store ${outcome.from} to ${outcome.to}.` @@ -312,7 +315,9 @@ async function handleSecretsRetireStore({ : [ `A different file appeared at ${outcome.from} while moving; it was kept at ${outcome.replacement}.`, ]), - ].join("\n"), + ] + .map(escapeTerminalControls) + .join("\n"), }); return 0; } diff --git a/src/lib/doctor-secret-integrity.ts b/src/lib/doctor-secret-integrity.ts index c27ed0620..d9cee9490 100644 --- a/src/lib/doctor-secret-integrity.ts +++ b/src/lib/doctor-secret-integrity.ts @@ -19,6 +19,7 @@ import { projectUsesSecretStore, type SecretStoreUsers, } from "./secret-store-retire.ts"; +import { escapeTerminalControls } from "./terminal-text.ts"; /** * `hack doctor`'s secret integrity findings (HACK-2169): stores and env @@ -382,5 +383,12 @@ export async function inspectSecretIntegrity(opts: { ]; const rank = (finding: SecretIntegrityFinding) => ({ error: 0, warn: 1, info: 2 })[finding.status]; - return findings.sort((a, b) => rank(a) - rank(b)); + // Messages carry project-configured paths: control characters are shown, + // never sent to the terminal (HACK-2177). + return findings + .sort((a, b) => rank(a) - rank(b)) + .map((finding) => ({ + ...finding, + message: escapeTerminalControls(finding.message), + })); } diff --git a/src/lib/terminal-text.ts b/src/lib/terminal-text.ts new file mode 100644 index 000000000..010db94dd --- /dev/null +++ b/src/lib/terminal-text.ts @@ -0,0 +1,17 @@ +/** + * Terminal-safe text for one line (HACK-2177). Paths from project + * configuration are input: a carriage return, a newline or an escape + * sequence in one could rewrite or forge what the terminal shows (for + * example erase the real target from a confirmation prompt). Every C0 + * control character (newline and tab included), DEL, and every C1 control + * character is shown visibly as `\xNN`. Multi-line output escapes each line + * on its own, then joins them. + */ +const TERMINAL_CONTROL = /[\u0000-\u001f\u007f-\u009f]/g; + +export function escapeTerminalControls(text: string): string { + return text.replace( + TERMINAL_CONTROL, + (char) => `\\x${char.charCodeAt(0).toString(16).padStart(2, "0")}` + ); +} diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 114787050..ebb88d543 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -38,6 +38,7 @@ import { planSecretStoreRetire, retireSecretStore, } from "../src/lib/secret-store-retire.ts"; +import { escapeTerminalControls } from "../src/lib/terminal-text.ts"; import { restoreEnv } from "./helpers/env.ts"; import { privateCliEnv } from "./helpers/private-cli-env.ts"; @@ -1061,3 +1062,45 @@ test("the locked eligibility check reads the pinned store, not the alias", async ) ).toEqual([]); }); + +test("control characters in a configured store path are shown, never sent to the terminal", async () => { + // Codex security review: a symlinked directory named with CR and an + // erase-line sequence could rewrite the confirmation on screen. + expect(escapeTerminalControls("a\r\u001b[2Kb\nc\td\u009b")).toBe( + "a\\x0d\\x1b[2Kb\\x0ac\\x09d\\x9b" + ); + const fixture = await setUp(); + const alias = join(fixture.bystander.projectRoot, "lin\r\u001b[2Kked"); + await symlink(fixture.hackHome, alias); + await writeFile( + join(fixture.bystander.projectDir, "hack.config.json"), + `${JSON.stringify({ + name: "app-b", + dev_host: "app-b.hack", + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { + path: join(alias, "secrets.enc.json"), + keyPath: fixture.keyPath, + }, + }, + }, + })}\n` + ); + const raw = /[\u0000-\u0008\u000b-\u001f\u007f-\u009f]/; + // Doctor's finding names the path with its controls escaped. + const findings = await inspect(fixture.bystander, "app-b"); + for (const finding of findings) { + expect(raw.test(finding.message)).toBe(false); + } + // The real command's output (the moved paths) is escaped too. + const retired = await runRetire({ + fixture, + path: fixture.bystander.projectRoot, + yes: true, + }); + expect(retired.code).toBe(0); + expect(retired.output).toContain("lin\\x0d\\x1b[2Kked"); + expect(raw.test(retired.output.replaceAll("\u001b[", "ESC["))).toBe(false); +}); From 3b0c8a0c573ff42e10a1366a7cf5b9076597d8b1 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 16:38:02 +0000 Subject: [PATCH 13/22] fix(secrets): refuse a configured store path that is itself a symlink before resolving it (HACK-2177) Resolving a final-component symlink retired its target and left a dangling link at the configured path, which runtime and doctor refuse as a store. Retire now refuses a final-component symlink in the plan and again under the lock; a symlinked directory on the way is still pinned to its real path. Co-Authored-By: Claude Opus 5.5 --- src/lib/secret-store-retire.ts | 25 +++++++++++++++++++++- tests/secret-store-retire.test.ts | 35 +++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index d2fabe8c1..ad8a5c194 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -1,4 +1,4 @@ -import { realpath, stat } from "node:fs/promises"; +import { lstat, realpath, stat } from "node:fs/promises"; import { resolve } from "node:path"; import { PROJECT_CONFIG_FILENAME } from "../constants.ts"; @@ -262,6 +262,10 @@ export async function planSecretStoreRetire(opts: { "No encrypted file secret store is configured here (the backend is the OS keychain); nothing was moved.", }; } + const leafRefusal = await symlinkedStoreRefusal(location.storePath); + if (leafRefusal !== null) { + return { kind: "refused", message: leafRefusal }; + } const canonical = await realpath(location.storePath).catch(() => null); if (canonical === null) { return { @@ -380,6 +384,10 @@ async function lockedRecheck(input: { ) { return "The secret store configuration changed; nothing was moved. Run hack doctor, then retry."; } + const leafRefusal = await symlinkedStoreRefusal(plan.configured); + if (leafRefusal !== null) { + return leafRefusal; + } if ((await realpath(plan.configured).catch(() => null)) !== plan.canonical) { return `${plan.configured} no longer leads to ${plan.canonical}; nothing was moved.`; } @@ -390,6 +398,21 @@ async function lockedRecheck(input: { }); } +/** + * Refuses a configured store path whose last component is a symlink (a + * symlinked directory on the way is fine). Runtime, doctor and the store's + * own reads refuse such a store; retiring its target would leave a + * dangling link at the configured path. + */ +async function symlinkedStoreRefusal( + configured: string +): Promise { + const leaf = await lstat(configured).catch(() => null); + return leaf?.isSymbolicLink() === true + ? `The configured store path ${configured} is itself a symlink, which Hack refuses as a store; nothing was moved. Replace it with the regular store file it points to, or point controlPlane.secrets.encryptedFile.path at that file.` + : null; +} + /** * Why the store at this (real) path is not one to retire, or null when it * is: entries exist, the key that opens them is missing or wrong, and the diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index ebb88d543..98d168715 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -1104,3 +1104,38 @@ test("control characters in a configured store path are shown, never sent to the expect(retired.output).toContain("lin\\x0d\\x1b[2Kked"); expect(raw.test(retired.output.replaceAll("\u001b[", "ESC["))).toBe(false); }); + +test("a configured store path that is itself a symlink is not retired", async () => { + // Codex review: resolving a final-component symlink moved its target + // and left a dangling link at the configured path, which runtime and + // doctor refuse as a store. + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + const link = join(fixture.bystander.projectRoot, "store-link.enc.json"); + await symlink(fixture.storePath, link); + await writeFile( + join(fixture.bystander.projectDir, "hack.config.json"), + `${JSON.stringify({ + name: "app-b", + dev_host: "app-b.hack", + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { path: link, keyPath: fixture.keyPath }, + }, + }, + })}\n` + ); + const planned = await planSecretStoreRetire({ + projectDir: fixture.bystander.projectDir, + }); + expect(planned.kind === "refused" ? planned.message : "").toContain( + "is itself a symlink" + ); + const outcome = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind).toBe("refused"); + expect(await readFile(fixture.storePath)).toEqual(before); + expect((await lstat(link)).isSymbolicLink()).toBe(true); +}); From 45f998de6e447b70d881d62b19aca093aea505dd Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 16:46:46 +0000 Subject: [PATCH 14/22] fix(secrets): include registered worktrees in the store-reader scan (HACK-2177) Linked worktrees are registered under their project, not as projects, and each can have its own env contract and store configuration. The reader scan now checks every registered checkout (the primary and each worktree, as host ps enumerates them), names a worktree reader as " (worktree )", and counts a worktree it cannot check as unchecked. Co-Authored-By: Claude Opus 5.5 --- src/lib/secret-store-retire.ts | 30 ++++++++++++++++----- tests/secret-store-retire.test.ts | 45 +++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index ad8a5c194..8aee496ef 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -1,12 +1,15 @@ import { lstat, realpath, stat } from "node:fs/promises"; -import { resolve } from "node:path"; +import { basename, resolve } from "node:path"; import { PROJECT_CONFIG_FILENAME } from "../constants.ts"; import { readControlPlaneConfig } from "../control-plane/sdk/config.ts"; import { resolveGlobalConfigPath } from "./config-paths.ts"; import { readOptionalTextFile } from "./fs.ts"; import { readHackEnvContract, readHackEnvRuntimeConfig } from "./hack-env.ts"; -import { readProjectsRegistryStrict } from "./projects-registry.ts"; +import { + type RegisteredProject, + readProjectsRegistryStrict, +} from "./projects-registry.ts"; import { classifyEncryptedStoreAt, isEncryptedStoreFile, @@ -100,10 +103,7 @@ export type SecretStoreUsers = { export async function listProjectsUsingSecretStore(opts: { readonly storePath: string; }): Promise { - let projects: readonly { - readonly name: string; - readonly projectDir: string; - }[]; + let projects: readonly RegisteredProject[]; // Registry entries that are not valid projects: unknown, so unchecked. let rejected = 0; try { @@ -119,7 +119,10 @@ export async function listProjectsUsingSecretStore(opts: { const target = await identity(opts.storePath); const names = new Set(); let unchecked = rejected; - for (const project of projects) { + // A project's linked worktrees are registered under it, not as projects + // of their own, and each can have its own env contract and store config: + // every checkout is a context of its own. + for (const project of projects.flatMap(registeredCheckouts)) { let location: Awaited>; try { if ( @@ -157,6 +160,19 @@ export async function listProjectsUsingSecretStore(opts: { return { names: [...names].sort(), registryRead: true, unchecked }; } +/** A registered project's primary checkout and each of its linked worktrees. */ +function registeredCheckouts( + project: RegisteredProject +): { readonly name: string; readonly projectDir: string }[] { + return [ + { name: project.name, projectDir: project.projectDir }, + ...(project.worktrees ?? []).map((worktree) => ({ + name: `${project.name} (worktree ${basename(worktree.path)})`, + projectDir: resolve(worktree.path, project.projectDirName), + })), + ]; +} + /** * A file's device and inode: `absent` when nothing is there, `unknown` when * that cannot be established (for example EACCES or EIO). diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 98d168715..d542bc359 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -1139,3 +1139,48 @@ test("a configured store path that is itself a symlink is not retired", async () expect(await readFile(fixture.storePath)).toEqual(before); expect((await lstat(link)).isSymbolicLink()).toBe(true); }); + +test("a registered worktree that reads the store is named, even when its primary checkout does not", async () => { + // Codex review: linked worktrees are registered under their project, not + // as projects; the scan checked only the primary checkout. + const fixture = await setUp(); + // A worktree of app-b whose env contract reads the store. + const worktreeRoot = join(root, "repos", "app-b-feature"); + const worktreeDir = join(worktreeRoot, ".hack"); + await mkdir(worktreeDir, { recursive: true }); + await writeFile( + join(worktreeDir, "hack.config.json"), + `${JSON.stringify({ name: "app-b", dev_host: "app-b.hack" })}\n` + ); + await writeFile( + join(worktreeDir, "hack.env.json"), + `${JSON.stringify({ version: 1, vars: [{ key: "TOKEN", source: "keychain" }] })}\n` + ); + const registry = join(fixture.hackHome, "projects.json"); + const parsed = JSON.parse(await readFile(registry, "utf8")) as { + projects: { name: string; worktrees?: unknown[] }[]; + }; + for (const project of parsed.projects) { + if (project.name === "app-b") { + project.worktrees = [ + { + path: worktreeRoot, + branch: "feature", + lastSeenAt: "2026-11-13T12:00:00.000Z", + }, + ]; + } + } + await writeFile(registry, JSON.stringify(parsed)); + // Only app-a's primary checkout and app-b's worktree read the store. + const finding = (await inspect(fixture.bystander, "app-b"))[0]; + expect(finding?.message).toContain( + "Registered projects that read it: app-a, app-b (worktree app-b-feature)." + ); + // A worktree whose config cannot be read is unchecked, not "no reader". + await writeFile(join(worktreeDir, "hack.config.json"), "{ bad"); + const partial = (await inspect(fixture.bystander, "app-b"))[0]; + expect(partial?.message).toContain( + "1 registered project(s) could not be checked" + ); +}); From 967739b49b40f45d48e6c216ff6460f389e5d2c8 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 16:56:39 +0000 Subject: [PATCH 15/22] fix(secrets): count an unavailable registered checkout as unchecked (HACK-2177) A registered checkout whose project directory is not there (an unmounted volume, a moved repo) has no config or contract to read; the reader scan now counts it as unchecked instead of a non-reader. Co-Authored-By: Claude Opus 5.5 --- src/lib/secret-store-retire.ts | 6 ++++++ tests/secret-store-retire.test.ts | 29 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index 8aee496ef..5ca989666 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -123,6 +123,12 @@ export async function listProjectsUsingSecretStore(opts: { // of their own, and each can have its own env contract and store config: // every checkout is a context of its own. for (const project of projects.flatMap(registeredCheckouts)) { + // A checkout that is not there (an unmounted volume, a moved repo) has + // no config or contract to read, which is not "does not read it". + if (!(await stat(project.projectDir).catch(() => null))?.isDirectory()) { + unchecked += 1; + continue; + } let location: Awaited>; try { if ( diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index d542bc359..3c05ff2d4 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -1184,3 +1184,32 @@ test("a registered worktree that reads the store is named, even when its primary "1 registered project(s) could not be checked" ); }); + +test("a registered checkout that is not available counts as unchecked", async () => { + // Codex review: a missing project directory (an unmounted volume) read + // as a checkout with no config and no contract, so a non-reader. + const fixture = await setUp(); + const registry = join(fixture.hackHome, "projects.json"); + const parsed = JSON.parse(await readFile(registry, "utf8")) as { + projects: { name: string; worktrees?: unknown[] }[]; + }; + for (const project of parsed.projects) { + if (project.name === "app-a") { + project.worktrees = [ + { + path: join(root, "unmounted", "app-a-feature"), + branch: "feature", + lastSeenAt: "2026-11-13T12:00:00.000Z", + }, + ]; + } + } + await writeFile(registry, JSON.stringify(parsed)); + const finding = (await inspect(fixture.bystander, "app-b"))[0]; + expect(finding?.message).toContain( + "Registered projects that read it: app-a." + ); + expect(finding?.message).toContain( + "1 registered project(s) could not be checked" + ); +}); From baee881506b045a208f150ae2fb5594bca21d667 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 17:03:20 +0000 Subject: [PATCH 16/22] fix(secrets): refuse retiring a store another registered project's key still opens (HACK-2177) Retire asked only whether this project's key opens the store. It now asks every registered checkout that selects the same store file with a different key file whether that key opens it (read-only, at the pinned real path), before and under the lock, and refuses naming them. When some registered checkouts could not be checked it refuses unless --allow-unchecked. Doctor's reader list and this check share one scan of registered checkouts. Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 4 +- docs/reference/cli.md | 4 +- docs/versions/v5/guides/upgrade-from-v4.md | 5 +- src/commands/secrets.ts | 16 +- src/lib/secret-store-retire.ts | 197 ++++++++++++++++----- tests/secret-store-retire.test.ts | 64 +++++++ 6 files changed, 236 insertions(+), 54 deletions(-) diff --git a/docs/env.md b/docs/env.md index 10e185996..cab7bcdc7 100644 --- a/docs/env.md +++ b/docs/env.md @@ -1662,7 +1662,9 @@ Some older surfaces are still compatibility-oriented: projects that read the store, and offers two ways forward: restore the key, or run `hack secrets retire-store`. That command moves the store into a new directory `.retired-/` beside it, deletes and prints nothing, and says - how to move it back. It needs `--yes` without a terminal. Doctor then lists + how to move it back. It needs `--yes` without a terminal. It refuses a store that + another registered project's key still opens, and, unless `--allow-unchecked` is + passed, a store some registered projects could not be checked for. Doctor then lists retired files as information. Explicit key provisioning also uses an atomic mode-`0600` replacement and refuses symlinked key paths or immediate parent directories. A key generated on first use is diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 00541f623..ab5c8e77c 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -2442,7 +2442,8 @@ For an encrypted_file (or cloud shim) store that hack doctor reports with a miss Moves the store into a new directory .retired-/ beside it: nothing is deleted, read or printed, and the key file is left alone. It prints how to put the store back. The store is the one this project selects (--path or the current directory), or the global one outside a project. Refused for a store that -opens with the key in use, or that is unreadable for another reason. Without a TTY (or with +opens with the key in use, that another registered project's key still opens, or that is unreadable for +another reason; when some registered projects could not be checked, pass --allow-unchecked. Without a TTY (or with HACK_NO_INTERACTIVE / --no-interactive) it is refused with E_INTERACTIVE_REQUIRED unless --yes is passed. ### Options @@ -2451,6 +2452,7 @@ HACK_NO_INTERACTIVE / --no-interactive) it is refused with E_INTERACTIVE_REQUIRE | --- | --- | | `--path, -p ` | Run a project command against a repo path (overrides cwd search) | | `--yes` | Confirm moving the store aside without prompting (required without a TTY or with HACK_NO_INTERACTIVE) | +| `--allow-unchecked` | Retire even when some registered projects could not be checked for another key that opens the store | | `--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 | diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index bfb669164..134cc67c0 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -397,7 +397,10 @@ entries. - It moves the store into a new directory `.retired-/` beside it, keeping its name. It deletes nothing, reads and prints no entry, and leaves the key file alone. It only retires a file that is a - Hack encrypted store. + Hack encrypted store, and refuses one that another registered project's + key still opens. If some registered projects could not be checked (for + example a worktree on an unmounted volume), it refuses unless you pass + `--allow-unchecked`. - It prints the `mv` command that puts the store back. - Afterwards Doctor lists the retired file as information, and the project check no longer fails. diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index 4da4093ad..286c35ca6 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -46,9 +46,17 @@ const optRetireYes = defineOption({ "Confirm moving the store aside without prompting (required without a TTY or with HACK_NO_INTERACTIVE)", } as const); +const optAllowUnchecked = defineOption({ + name: "allowUnchecked", + type: "boolean", + long: "--allow-unchecked", + description: + "Retire even when some registered projects could not be checked for another key that opens the store", +} as const); + const secretsOptions = [optService] as const; const deleteOptions = [optService, optYes] as const; -const retireStoreOptions = [optPath, optRetireYes] as const; +const retireStoreOptions = [optPath, optRetireYes, optAllowUnchecked] as const; type RetireStoreArgs = CommandArgs; const secretsPositionals = [{ name: "name", required: false }] as const; @@ -102,7 +110,8 @@ const retireStoreSpec = defineCommand({ "Moves the store into a new directory .retired-/ beside it: nothing is deleted, read or printed,", "and the key file is left alone. It prints how to put the store back. The store is the one this project", "selects (--path or the current directory), or the global one outside a project. Refused for a store that", - "opens with the key in use, or that is unreadable for another reason. Without a TTY (or with", + "opens with the key in use, that another registered project's key still opens, or that is unreadable for", + "another reason; when some registered projects could not be checked, pass --allow-unchecked. Without a TTY (or with", "HACK_NO_INTERACTIVE / --no-interactive) it is refused with E_INTERACTIVE_REQUIRED unless --yes is passed.", ].join("\n"), options: retireStoreOptions, @@ -275,8 +284,10 @@ async function handleSecretsRetireStore({ // Plan once: the confirmation names this exact target (its real path; the // configured path is project input and may go through a symlinked // directory), and the retirement refuses if the target changed since. + const allowUnchecked = args.options.allowUnchecked === true; const planned = await planSecretStoreRetire({ projectDir: project?.projectDir, + allowUnchecked, }); if (planned.kind === "refused") { logger.error({ message: escapeTerminalControls(planned.message) }); @@ -297,6 +308,7 @@ async function handleSecretsRetireStore({ const outcome = await retireSecretStore({ projectDir: project?.projectDir, approved: planned.plan, + allowUnchecked, }); if (outcome.kind === "refused") { logger.error({ message: escapeTerminalControls(outcome.message) }); diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index 5ca989666..90a961286 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -92,57 +92,48 @@ export type SecretStoreUsers = { readonly unchecked: number; }; +/** Registered checkouts that select one store file, and what could not be checked. */ +type StoreContexts = { + readonly contexts: readonly { + readonly name: string; + readonly projectDir: string; + readonly keyPath: string; + }[]; + readonly registryRead: boolean; + readonly unchecked: number; +}; + /** - * Registered projects whose configuration selects the store at `storePath` - * and whose runtime reads it, from configuration only: no store is opened. - * A project selects the same store when its configured path is the same, - * or names the same file (device and inode, so aliases through symlinked - * directories match). What could not be checked is counted, never taken as - * "does not read it". + * Every registered checkout (each project's primary checkout and each of + * its linked worktrees) whose configuration selects the store file at + * `storePath`: the same configured path, or the same file (device and + * inode, so aliases through symlinked directories match). From + * configuration only. A checkout that is not there, whose config is unsure, + * or whose store identity cannot be read is counted as unchecked, never + * taken as "does not select it". */ -export async function listProjectsUsingSecretStore(opts: { +async function storeContexts(opts: { readonly storePath: string; -}): Promise { +}): Promise { let projects: readonly RegisteredProject[]; // Registry entries that are not valid projects: unknown, so unchecked. let rejected = 0; try { const read = await readProjectsRegistryStrict(); if (read.kind === "invalid") { - return { names: [], registryRead: false, unchecked: 0 }; + return { contexts: [], registryRead: false, unchecked: 0 }; } projects = read.kind === "valid" ? read.registry.projects : []; rejected = read.kind === "valid" ? read.rejected : 0; } catch { - return { names: [], registryRead: false, unchecked: 0 }; + return { contexts: [], registryRead: false, unchecked: 0 }; } const target = await identity(opts.storePath); - const names = new Set(); + const contexts: StoreContexts["contexts"][number][] = []; let unchecked = rejected; - // A project's linked worktrees are registered under it, not as projects - // of their own, and each can have its own env contract and store config: - // every checkout is a context of its own. - for (const project of projects.flatMap(registeredCheckouts)) { - // A checkout that is not there (an unmounted volume, a moved repo) has - // no config or contract to read, which is not "does not read it". - if (!(await stat(project.projectDir).catch(() => null))?.isDirectory()) { - unchecked += 1; - continue; - } - let location: Awaited>; - try { - if ( - (await secretStoreSelectionProblem({ - projectDir: project.projectDir, - })) !== null - ) { - unchecked += 1; - continue; - } - location = await resolveFileSecretStorePaths({ - projectDir: project.projectDir, - }); - } catch { + for (const checkout of projects.flatMap(registeredCheckouts)) { + const location = await checkoutStoreLocation(checkout.projectDir); + if (location === null) { unchecked += 1; continue; } @@ -156,14 +147,104 @@ export async function listProjectsUsingSecretStore(opts: { }); if (same === "unknown") { unchecked += 1; - } else if ( - same === "same" && - (await projectUsesSecretStore({ projectDir: project.projectDir })) - ) { - names.add(project.name); + } else if (same === "same") { + contexts.push({ ...checkout, keyPath: location.keyPath }); } } - return { names: [...names].sort(), registryRead: true, unchecked }; + return { contexts, registryRead: true, unchecked }; +} + +/** + * A checkout's store location from its configuration, or null when that + * cannot be established: the checkout is not there (an unmounted volume, a + * moved repo; absent files there are not "unconfigured"), or its selecting + * config is unsure. + */ +async function checkoutStoreLocation( + projectDir: string +): Promise> | null> { + if (!(await stat(projectDir).catch(() => null))?.isDirectory()) { + return null; + } + try { + if ((await secretStoreSelectionProblem({ projectDir })) !== null) { + return null; + } + return await resolveFileSecretStorePaths({ projectDir }); + } catch { + return null; + } +} + +/** + * Registered checkouts whose configuration selects the store at + * `storePath` and whose runtime reads it ({@link projectUsesSecretStore}). + * No store is opened. What could not be checked is counted, never taken as + * "does not read it". + */ +export async function listProjectsUsingSecretStore(opts: { + readonly storePath: string; +}): Promise { + const found = await storeContexts(opts); + const names = new Set(); + for (const context of found.contexts) { + if (await projectUsesSecretStore({ projectDir: context.projectDir })) { + names.add(context.name); + } + } + return { + names: [...names].sort(), + registryRead: found.registryRead, + unchecked: found.unchecked, + }; +} + +/** + * Why retiring this store would take it from another registered checkout + * that can still open it, or null when none can (HACK-2177). A checkout + * selecting the same store file with a different key file is asked whether + * its key opens the store (read-only, at the pinned real path). Incomplete + * coverage refuses unless `allowUnchecked`: the store may be another + * checkout's that could not be checked. + */ +async function otherOpenersRefusal(input: { + readonly canonical: string; + readonly keyPath: string; + readonly allowUnchecked: boolean; +}): Promise { + const found = await storeContexts({ storePath: input.canonical }); + const openers: string[] = []; + const otherKeys = new Map(); + for (const context of found.contexts) { + if (resolve(context.keyPath) !== resolve(input.keyPath)) { + otherKeys.set(context.keyPath, [ + ...(otherKeys.get(context.keyPath) ?? []), + context.name, + ]); + } + } + for (const [keyPath, names] of otherKeys) { + const state = await classifyEncryptedStoreAt({ + storePath: input.canonical, + keyPath, + }); + if (state.kind === "opens") { + openers.push(...names); + } + } + if (openers.length > 0) { + return `The encrypted secret store at ${input.canonical} still opens with another registered project's key (${[...new Set(openers)].sort().join(", ")}), so it is not retired; nothing was moved. Fix this project's key configuration instead.`; + } + if (input.allowUnchecked) { + return null; + } + if (!found.registryRead) { + return `The projects registry could not be read or is not valid, so whether another project still opens ${input.canonical} is unknown; nothing was moved. Fix the registry, or pass --allow-unchecked to retire it anyway.`; + } + if (found.unchecked > 0) { + return `${found.unchecked} registered project(s) could not be checked (not available, or their Hack config or store file could not be read), so whether one still opens ${input.canonical} is unknown; nothing was moved. Run hack doctor to see them, or pass --allow-unchecked to retire it anyway.`; + } + return null; } /** A registered project's primary checkout and each of its linked worktrees. */ @@ -265,6 +346,8 @@ export type SecretStoreRetirePlan = { */ export async function planSecretStoreRetire(opts: { readonly projectDir?: string; + /** Retire even when some registered checkouts could not be checked. */ + readonly allowUnchecked?: boolean; }): Promise< | { readonly kind: "plan"; readonly plan: SecretStoreRetirePlan } | { readonly kind: "refused"; readonly message: string } @@ -295,10 +378,16 @@ export async function planSecretStoreRetire(opts: { message: `There is no encrypted secret store with entries at ${location.storePath}; nothing was moved.`, }; } - const refusal = await eligibilityRefusal({ - storePath: canonical, - keyPath: location.keyPath, - }); + const refusal = + (await eligibilityRefusal({ + storePath: canonical, + keyPath: location.keyPath, + })) ?? + (await otherOpenersRefusal({ + canonical, + keyPath: location.keyPath, + allowUnchecked: opts.allowUnchecked === true, + })); if (refusal !== null) { return { kind: "refused", message: refusal }; } @@ -331,6 +420,8 @@ export async function retireSecretStore(opts: { readonly projectDir?: string; readonly now?: Date; readonly approved?: SecretStoreRetirePlan; + /** Retire even when some registered checkouts could not be checked. */ + readonly allowUnchecked?: boolean; /** Test seam: runs under the lock after the path checks, before eligibility. */ readonly beforeEligibility?: () => Promise; }): Promise { @@ -388,6 +479,7 @@ function samePlan(a: SecretStoreRetirePlan, b: SecretStoreRetirePlan): boolean { async function lockedRecheck(input: { readonly opts: { readonly projectDir?: string; + readonly allowUnchecked?: boolean; readonly beforeEligibility?: () => Promise; }; readonly plan: SecretStoreRetirePlan; @@ -414,10 +506,17 @@ async function lockedRecheck(input: { return `${plan.configured} no longer leads to ${plan.canonical}; nothing was moved.`; } await opts.beforeEligibility?.(); - return await eligibilityRefusal({ - storePath: plan.canonical, - keyPath: plan.keyPath, - }); + return ( + (await eligibilityRefusal({ + storePath: plan.canonical, + keyPath: plan.keyPath, + })) ?? + (await otherOpenersRefusal({ + canonical: plan.canonical, + keyPath: plan.keyPath, + allowUnchecked: opts.allowUnchecked === true, + })) + ); } /** diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 3c05ff2d4..1f01af1e4 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -1213,3 +1213,67 @@ test("a registered checkout that is not available counts as unchecked", async () "1 registered project(s) could not be checked" ); }); + +test("a store another registered project's key still opens is not retired", async () => { + // Codex security review: retire asked only whether this project's key + // opens the store; another project with its own key still used it. + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + const appAKey = join(fixture.reader.projectRoot, ".hack-store.key"); + await writeFile(appAKey, `${STORE_KEY}\n`, { mode: 0o600 }); + await chmod(appAKey, 0o600); + await writeFile( + join(fixture.reader.projectDir, "hack.config.json"), + `${JSON.stringify({ + name: "app-a", + dev_host: "app-a.hack", + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { path: fixture.storePath, keyPath: appAKey }, + }, + }, + })}\n` + ); + const outcome = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "still opens with another registered project's key (app-a)" + ); + expect(await readFile(fixture.storePath)).toEqual(before); +}); + +test("retire refuses when registered projects could not be checked, unless --allow-unchecked", async () => { + // The store may be another checkout's that could not be checked. + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + const registry = join(fixture.hackHome, "projects.json"); + const parsed = JSON.parse(await readFile(registry, "utf8")) as { + projects: { name: string; worktrees?: unknown[] }[]; + }; + for (const project of parsed.projects) { + if (project.name === "app-a") { + project.worktrees = [ + { + path: join(root, "unmounted", "app-a-feature"), + branch: "feature", + lastSeenAt: "2026-11-13T12:00:00.000Z", + }, + ]; + } + } + await writeFile(registry, JSON.stringify(parsed)); + const refused = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + }); + expect(refused.kind === "refused" ? refused.message : "").toContain( + "--allow-unchecked" + ); + expect(await readFile(fixture.storePath)).toEqual(before); + const allowed = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + allowUnchecked: true, + }); + expect(allowed.kind).toBe("retired"); +}); From ef48a226bca367805169de3af7cac7cdc7ae86c5 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 17:13:33 +0000 Subject: [PATCH 17/22] fix(secrets): test every configured key file directly before retiring, and count unreadable ones as unchecked (HACK-2177) - keyFileOpensStore reads a key file directly, without the HACK_SECRETS_FILE_KEY override or the keychain, so a wrong override in the retiring shell cannot hide a key file that works. Retire checks every key file that selects the store, this project's own included. - A key file that exists but cannot be read was not tested: it counts as incomplete coverage and needs --allow-unchecked. Co-Authored-By: Claude Opus 5.5 --- src/lib/secret-store-retire.ts | 31 +++++++++++++--------- src/lib/secret-store.ts | 44 +++++++++++++++++++++++++++++++ tests/secret-store-retire.test.ts | 37 +++++++++++++++++++++++++- 3 files changed, 99 insertions(+), 13 deletions(-) diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index 90a961286..8d63ca681 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -13,6 +13,7 @@ import { import { classifyEncryptedStoreAt, isEncryptedStoreFile, + keyFileOpensStore, resolveFileSecretStorePaths, retireEncryptedStoreFile, SecretStoreRetireError, @@ -214,26 +215,29 @@ async function otherOpenersRefusal(input: { }): Promise { const found = await storeContexts({ storePath: input.canonical }); const openers: string[] = []; - const otherKeys = new Map(); + // Every key file that selects this store, this project's own included, + // read directly (no HACK_SECRETS_FILE_KEY override, no keychain), so a + // wrong override in this shell cannot hide a key file that works. + const keyFiles = new Map([[resolve(input.keyPath), []]]); for (const context of found.contexts) { - if (resolve(context.keyPath) !== resolve(input.keyPath)) { - otherKeys.set(context.keyPath, [ - ...(otherKeys.get(context.keyPath) ?? []), - context.name, - ]); - } + const keyPath = resolve(context.keyPath); + keyFiles.set(keyPath, [...(keyFiles.get(keyPath) ?? []), context.name]); } - for (const [keyPath, names] of otherKeys) { - const state = await classifyEncryptedStoreAt({ + let unknownKeys = 0; + for (const [keyPath, names] of keyFiles) { + const state = await keyFileOpensStore({ storePath: input.canonical, keyPath, }); - if (state.kind === "opens") { - openers.push(...names); + if (state === "opens") { + openers.push(...(names.length > 0 ? names : [`its key file ${keyPath}`])); + } else if (state === "unknown") { + // A key file that cannot be read was not tested: not "does not open". + unknownKeys += 1; } } if (openers.length > 0) { - return `The encrypted secret store at ${input.canonical} still opens with another registered project's key (${[...new Set(openers)].sort().join(", ")}), so it is not retired; nothing was moved. Fix this project's key configuration instead.`; + return `The encrypted secret store at ${input.canonical} still opens with a configured key file (${[...new Set(openers)].sort().join(", ")}), so it is not retired; nothing was moved. Check HACK_SECRETS_FILE_KEY and this project's key configuration instead.`; } if (input.allowUnchecked) { return null; @@ -241,6 +245,9 @@ async function otherOpenersRefusal(input: { if (!found.registryRead) { return `The projects registry could not be read or is not valid, so whether another project still opens ${input.canonical} is unknown; nothing was moved. Fix the registry, or pass --allow-unchecked to retire it anyway.`; } + if (unknownKeys > 0) { + return `${unknownKeys} configured key file(s) for ${input.canonical} could not be read, so whether one still opens it is unknown; nothing was moved. Fix their permissions and run hack doctor, or pass --allow-unchecked to retire it anyway.`; + } if (found.unchecked > 0) { return `${found.unchecked} registered project(s) could not be checked (not available, or their Hack config or store file could not be read), so whether one still opens ${input.canonical} is unknown; nothing was moved. Run hack doctor to see them, or pass --allow-unchecked to retire it anyway.`; } diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index f9beec540..09244cea3 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -519,6 +519,50 @@ export async function classifyEncryptedStoreAt(input: { : { kind: "opens" }; } +/** + * Whether the key in the file at `keyPath` alone opens the store at + * `storePath` (HACK-2177): the key file is read directly, without the + * `HACK_SECRETS_FILE_KEY` override or the keychain, so a wrong override in + * the calling shell cannot hide a key file that works. `unknown` when the + * key file or the store exists but cannot be read; an absent or empty key + * file opens nothing. + */ +export async function keyFileOpensStore(input: { + readonly storePath: string; + readonly keyPath: string; +}): Promise<"opens" | "does_not_open" | "unknown"> { + const store = await readEncryptedStoreText(input.storePath); + if (store.kind !== "text" || store.text.length === 0) { + return store.kind === "absent" ? "does_not_open" : "unknown"; + } + let key: string; + try { + key = + ( + await readReplaceablePrivateFile({ + path: input.keyPath, + directories: [], + refusal: () => new Error("not a regular key file"), + }) + )?.text.trim() ?? ""; + } catch { + return "unknown"; + } + if (key.length === 0) { + return "does_not_open"; + } + try { + decryptEncryptedEntriesText({ + filePath: input.storePath, + text: store.text, + encryptionKey: key, + }); + return "opens"; + } catch { + return "does_not_open"; + } +} + /** * Whether the file at `storePath` is a Hack encrypted secret store: a * regular file (read without following a link) holding the store's JSON diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 1f01af1e4..5ab9875ce 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -1239,9 +1239,44 @@ test("a store another registered project's key still opens is not retired", asyn projectDir: fixture.bystander.projectDir, }); expect(outcome.kind === "refused" ? outcome.message : "").toContain( - "still opens with another registered project's key (app-a)" + "still opens with a configured key file (app-a)" ); expect(await readFile(fixture.storePath)).toEqual(before); + + // A wrong HACK_SECRETS_FILE_KEY in this shell does not hide app-a's key + // file: key files are read directly, not through the override. + process.env.HACK_SECRETS_FILE_KEY = "synthetic-wrong-override"; + try { + const masked = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + }); + expect(masked.kind === "refused" ? masked.message : "").toContain( + "still opens with a configured key file (app-a)" + ); + } finally { + Reflect.deleteProperty(process.env, "HACK_SECRETS_FILE_KEY"); + } + expect(await readFile(fixture.storePath)).toEqual(before); + + // app-a's key file cannot be read: not tested, so --allow-unchecked is + // needed rather than taking it as "does not open". + if (process.getuid?.() !== 0) { + await chmod(appAKey, 0o000); + try { + const unknown = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + }); + expect(unknown.kind === "refused" ? unknown.message : "").toContain( + "configured key file(s)" + ); + expect(unknown.kind === "refused" ? unknown.message : "").toContain( + "--allow-unchecked" + ); + } finally { + await chmod(appAKey, 0o600); + } + expect(await readFile(fixture.storePath)).toEqual(before); + } }); test("retire refuses when registered projects could not be checked, unless --allow-unchecked", async () => { From 4af05a108dae53289aad8885ee3414fba5006d44 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 17:24:37 +0000 Subject: [PATCH 18/22] fix(secrets): check the keychain fallback for projects without a key file before retiring (HACK-2177) A project whose key file is absent opens the store through the OS keychain at runtime. When any checkout selecting the store has no key file, retire now asks the keychain directly (no resolution cooldown or cache): a key that opens the store refuses, naming the project, and a keychain that cannot be asked counts as unchecked. Co-Authored-By: Claude Opus 5.5 --- src/lib/secret-store-retire.ts | 68 ++++++++++++++++++++++++------- src/lib/secret-store.ts | 57 ++++++++++++++++++++++++-- tests/secret-store-retire.test.ts | 55 ++++++++++++++++++++++++- 3 files changed, 162 insertions(+), 18 deletions(-) diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index 8d63ca681..e5d06a3d4 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -13,6 +13,7 @@ import { import { classifyEncryptedStoreAt, isEncryptedStoreFile, + keychainKeyOpensStore, keyFileOpensStore, resolveFileSecretStorePaths, retireEncryptedStoreFile, @@ -223,19 +224,12 @@ async function otherOpenersRefusal(input: { const keyPath = resolve(context.keyPath); keyFiles.set(keyPath, [...(keyFiles.get(keyPath) ?? []), context.name]); } - let unknownKeys = 0; - for (const [keyPath, names] of keyFiles) { - const state = await keyFileOpensStore({ - storePath: input.canonical, - keyPath, - }); - if (state === "opens") { - openers.push(...(names.length > 0 ? names : [`its key file ${keyPath}`])); - } else if (state === "unknown") { - // A key file that cannot be read was not tested: not "does not open". - unknownKeys += 1; - } - } + const evidence = await keyEvidence({ + storePath: input.canonical, + keyFiles, + }); + openers.push(...evidence.openers); + const unknownKeys = evidence.unknown; if (openers.length > 0) { return `The encrypted secret store at ${input.canonical} still opens with a configured key file (${[...new Set(openers)].sort().join(", ")}), so it is not retired; nothing was moved. Check HACK_SECRETS_FILE_KEY and this project's key configuration instead.`; } @@ -246,7 +240,7 @@ async function otherOpenersRefusal(input: { return `The projects registry could not be read or is not valid, so whether another project still opens ${input.canonical} is unknown; nothing was moved. Fix the registry, or pass --allow-unchecked to retire it anyway.`; } if (unknownKeys > 0) { - return `${unknownKeys} configured key file(s) for ${input.canonical} could not be read, so whether one still opens it is unknown; nothing was moved. Fix their permissions and run hack doctor, or pass --allow-unchecked to retire it anyway.`; + return `${unknownKeys} configured key(s) for ${input.canonical} (a key file, or the OS keychain) could not be read, so whether one still opens it is unknown; nothing was moved. Fix their permissions and run hack doctor, or pass --allow-unchecked to retire it anyway.`; } if (found.unchecked > 0) { return `${found.unchecked} registered project(s) could not be checked (not available, or their Hack config or store file could not be read), so whether one still opens ${input.canonical} is unknown; nothing was moved. Run hack doctor to see them, or pass --allow-unchecked to retire it anyway.`; @@ -254,6 +248,52 @@ async function otherOpenersRefusal(input: { return null; } +/** + * Which configured keys open the store, read independently of this shell: + * each key file directly, and the OS keychain (the runtime's fallback) once + * when any checkout has no key file. Returns who opens it, and how many + * keys could not be read (never taken as "does not open"). + */ +async function keyEvidence(input: { + readonly storePath: string; + readonly keyFiles: ReadonlyMap; +}): Promise<{ readonly openers: readonly string[]; readonly unknown: number }> { + const openers: string[] = []; + const keychainUsers: string[] = []; + let keychainReader = false; + let unknown = 0; + for (const [keyPath, names] of input.keyFiles) { + const state = await keyFileOpensStore({ + storePath: input.storePath, + keyPath, + }); + if (state === "opens") { + openers.push(...(names.length > 0 ? names : [`its key file ${keyPath}`])); + } else if (state === "unknown") { + unknown += 1; + } else if (state === "absent") { + keychainReader = true; + keychainUsers.push(...names); + } + } + if (!keychainReader) { + return { openers, unknown }; + } + const keychainState = await keychainKeyOpensStore({ + storePath: input.storePath, + }); + if (keychainState === "opens") { + openers.push( + ...(keychainUsers.length > 0 + ? keychainUsers.map((name) => `${name}, through the OS keychain`) + : ["the OS keychain"]) + ); + } else if (keychainState === "unknown") { + unknown += 1; + } + return { openers, unknown }; +} + /** A registered project's primary checkout and each of its linked worktrees. */ function registeredCheckouts( project: RegisteredProject diff --git a/src/lib/secret-store.ts b/src/lib/secret-store.ts index 09244cea3..1bbf0695e 100644 --- a/src/lib/secret-store.ts +++ b/src/lib/secret-store.ts @@ -530,7 +530,7 @@ export async function classifyEncryptedStoreAt(input: { export async function keyFileOpensStore(input: { readonly storePath: string; readonly keyPath: string; -}): Promise<"opens" | "does_not_open" | "unknown"> { +}): Promise<"opens" | "does_not_open" | "absent" | "unknown"> { const store = await readEncryptedStoreText(input.storePath); if (store.kind !== "text" || store.text.length === 0) { return store.kind === "absent" ? "does_not_open" : "unknown"; @@ -548,14 +548,65 @@ export async function keyFileOpensStore(input: { } catch { return "unknown"; } + if (key.length === 0) { + return "absent"; + } + return storeTextOpensUnder({ + storePath: input.storePath, + text: store.text, + key, + }); +} + +/** + * Whether the encrypted_file key in the OS keychain (the runtime's fallback + * when a project has no key file) opens the store (HACK-2177). Asked + * directly, without the key resolution cooldown or cache. `unknown` when + * the keychain cannot be asked; `does_not_open` when the fallback is turned + * off or holds no key. + */ +export async function keychainKeyOpensStore(input: { + readonly storePath: string; +}): Promise<"opens" | "does_not_open" | "unknown"> { + if (isKeychainDisabled()) { + return "does_not_open"; + } + const store = await readEncryptedStoreText(input.storePath); + if (store.kind !== "text" || store.text.length === 0) { + return store.kind === "absent" ? "does_not_open" : "unknown"; + } + let key: string; + try { + key = + ( + await keychain.get({ + service: ENCRYPTED_FILE_KEY_SERVICE, + name: ENCRYPTED_FILE_KEY_NAME, + }) + )?.trim() ?? ""; + } catch { + return "unknown"; + } if (key.length === 0) { return "does_not_open"; } + return storeTextOpensUnder({ + storePath: input.storePath, + text: store.text, + key, + }); +} + +function storeTextOpensUnder(input: { + readonly storePath: string; + readonly text: string; + readonly key: string; +}): "opens" | "does_not_open" { try { decryptEncryptedEntriesText({ filePath: input.storePath, - text: store.text, - encryptionKey: key, + text: input.text, + encryptionKey: input.key, }); return "opens"; } catch { diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index 5ab9875ce..bb99e0b3e 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -1267,7 +1267,7 @@ test("a store another registered project's key still opens is not retired", asyn projectDir: fixture.bystander.projectDir, }); expect(unknown.kind === "refused" ? unknown.message : "").toContain( - "configured key file(s)" + "configured key(s)" ); expect(unknown.kind === "refused" ? unknown.message : "").toContain( "--allow-unchecked" @@ -1312,3 +1312,56 @@ test("retire refuses when registered projects could not be checked, unless --all }); expect(allowed.kind).toBe("retired"); }); + +test("a store another project opens through the OS keychain is not retired", async () => { + // Codex review: a project with no key file falls back to the keychain; + // an absent key file was taken as "does not open". + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + // app-b has its own, wrong key file; app-a has none (keychain fallback). + const wrongKey = join(fixture.bystander.projectRoot, ".wrong.key"); + await writeFile(wrongKey, "synthetic-wrong-key\n", { mode: 0o600 }); + await chmod(wrongKey, 0o600); + await writeFile( + join(fixture.bystander.projectDir, "hack.config.json"), + `${JSON.stringify({ + name: "app-b", + dev_host: "app-b.hack", + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { path: fixture.storePath, keyPath: wrongKey }, + }, + }, + })}\n` + ); + process.env.HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK = "0"; + const holds = spyOn(keychain, "get").mockImplementation( + async () => STORE_KEY + ); + try { + const outcome = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "app-a, through the OS keychain" + ); + } finally { + holds.mockRestore(); + } + // A keychain that cannot be asked: unchecked, not "does not open". + const locked = spyOn(keychain, "get").mockImplementation(async () => { + throw new Error("synthetic keychain locked"); + }); + try { + const outcome = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "--allow-unchecked" + ); + } finally { + locked.mockRestore(); + } + expect(await readFile(fixture.storePath)).toEqual(before); +}); From 47afaee94c6cce6f20c37ec09d10d1f90625eb72 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 17:32:52 +0000 Subject: [PATCH 19/22] fix(secrets): count rejected worktree entries in the strict registry read (HACK-2177) The best-effort parser also drops invalid worktree entries inside a valid project; readProjectsRegistryStrict now counts them as rejected, so the reader scan reports them as unchecked and retire needs --allow-unchecked. Co-Authored-By: Claude Opus 5.5 --- src/lib/projects-registry.ts | 25 +++++++++++++++++-------- tests/secret-store-retire.test.ts | 27 +++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/lib/projects-registry.ts b/src/lib/projects-registry.ts index 9c8dd8851..cf30dd8ab 100644 --- a/src/lib/projects-registry.ts +++ b/src/lib/projects-registry.ts @@ -203,15 +203,24 @@ export async function readProjectsRegistryStrict(opts?: { if (registry === null) { return { kind: "invalid" }; } - // The best-effort parser skips invalid entries; count them, so a caller - // never presents a filtered list as complete. + // The best-effort parser skips invalid project entries, and invalid + // worktree entries inside a valid project; count both, so a caller never + // presents a filtered list as complete. const raw = isRecord(parsed) ? parsed.projects : undefined; - const total = Array.isArray(raw) ? raw.length : registry.projects.length; - return { - kind: "valid", - registry, - rejected: total - registry.projects.length, - }; + const items: unknown[] = Array.isArray(raw) ? raw : []; + let rejected = 0; + for (const item of items) { + const project = parseProject(item); + if (project === null) { + rejected += 1; + continue; + } + const rawWorktrees = isRecord(item) ? item.worktrees : undefined; + if (Array.isArray(rawWorktrees)) { + rejected += rawWorktrees.length - (project.worktrees?.length ?? 0); + } + } + return { kind: "valid", registry, rejected }; } type RegistrationUpsertOptions = { diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index bb99e0b3e..b9f129bf8 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -1365,3 +1365,30 @@ test("a store another project opens through the OS keychain is not retired", asy } expect(await readFile(fixture.storePath)).toEqual(before); }); + +test("an invalid worktree entry inside a valid project makes the reader scan incomplete", async () => { + // Codex review: the strict read counted rejected project entries but not + // rejected worktree entries, which were dropped silently. + const fixture = await setUp(); + const registry = join(fixture.hackHome, "projects.json"); + const parsed = JSON.parse(await readFile(registry, "utf8")) as { + projects: { name: string; worktrees?: unknown[] }[]; + }; + for (const project of parsed.projects) { + if (project.name === "app-a") { + // No lastSeenAt: the parser skips it. + project.worktrees = [{ path: join(root, "repos", "app-a-feature") }]; + } + } + await writeFile(registry, JSON.stringify(parsed)); + const finding = (await inspect(fixture.bystander, "app-b"))[0]; + expect(finding?.message).toContain( + "1 registered project(s) could not be checked" + ); + const outcome = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "--allow-unchecked" + ); +}); From a49048ffe7d3785b1fe8397aa5aaf98f030a3c13 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 17:41:50 +0000 Subject: [PATCH 20/22] fix(secrets): refuse an uninspectable store path, and count a malformed worktrees field as unchecked (HACK-2177) - An lstat failure on the configured store path other than absent now refuses (whether it is a symlink is unknown) instead of passing. - A registry project whose worktrees field is present but not an array counts as one rejected entry in the strict read, so the reader scan is incomplete and retire needs --allow-unchecked. Co-Authored-By: Claude Opus 5.5 --- src/lib/projects-registry.ts | 4 ++ src/lib/secret-store-retire.ts | 17 ++++++++- tests/secret-store-retire.test.ts | 61 +++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/lib/projects-registry.ts b/src/lib/projects-registry.ts index cf30dd8ab..90e416007 100644 --- a/src/lib/projects-registry.ts +++ b/src/lib/projects-registry.ts @@ -218,6 +218,10 @@ export async function readProjectsRegistryStrict(opts?: { const rawWorktrees = isRecord(item) ? item.worktrees : undefined; if (Array.isArray(rawWorktrees)) { rejected += rawWorktrees.length - (project.worktrees?.length ?? 0); + } else if (rawWorktrees !== undefined) { + // A present worktrees field that is not an array hides any checkouts + // it held: unknown, not "no worktrees". + rejected += 1; } } return { kind: "valid", registry, rejected }; diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index e5d06a3d4..1c8a1c213 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -575,8 +575,21 @@ async function lockedRecheck(input: { async function symlinkedStoreRefusal( configured: string ): Promise { - const leaf = await lstat(configured).catch(() => null); - return leaf?.isSymbolicLink() === true + let leaf: Awaited>; + try { + leaf = await lstat(configured); + } catch (error: unknown) { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { readonly code?: unknown }).code + : undefined; + // Absent is decided later (nothing to retire); anything else is unknown. + if (code === "ENOENT" || code === "ENOTDIR") { + return null; + } + return `The configured store path ${configured} could not be inspected (${String(code ?? "unknown")}), so whether it is a symlink is unknown; nothing was moved.`; + } + return leaf.isSymbolicLink() ? `The configured store path ${configured} is itself a symlink, which Hack refuses as a store; nothing was moved. Replace it with the regular store file it points to, or point controlPlane.secrets.encryptedFile.path at that file.` : null; } diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index b9f129bf8..ea1759480 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -1392,3 +1392,64 @@ test("an invalid worktree entry inside a valid project makes the reader scan inc "--allow-unchecked" ); }); + +test("a configured store path that cannot be inspected is refused, not taken as no symlink", async () => { + // Second-family review: an lstat failure other than "absent" fell + // through as "not a symlink". + const fixture = await setUp(); + const unreadable = join(fixture.hackHome, `${"x".repeat(300)}.enc.json`); + await writeFile( + join(fixture.bystander.projectDir, "hack.config.json"), + `${JSON.stringify({ + name: "app-b", + dev_host: "app-b.hack", + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { path: unreadable, keyPath: fixture.keyPath }, + }, + }, + })}\n` + ); + const planned = await planSecretStoreRetire({ + projectDir: fixture.bystander.projectDir, + }); + expect(planned.kind === "refused" ? planned.message : "").toContain( + "could not be inspected (ENAMETOOLONG)" + ); +}); + +test("a registry worktrees field that is not an array makes the reader scan incomplete", async () => { + // Second-family review: a present but malformed worktrees container was + // read as "no worktrees", hiding any checkout it held. + const fixture = await setUp(); + const registry = join(fixture.hackHome, "projects.json"); + const text = await readFile(registry, "utf8"); + for (const malformed of [ + { + path: join(root, "repos", "app-a-feature"), + lastSeenAt: "2026-11-13T12:00:00.000Z", + }, + "not-a-list", + ]) { + const parsed = JSON.parse(text) as { + projects: { name: string; worktrees?: unknown }[]; + }; + for (const project of parsed.projects) { + if (project.name === "app-a") { + project.worktrees = malformed; + } + } + await writeFile(registry, JSON.stringify(parsed)); + const finding = (await inspect(fixture.bystander, "app-b"))[0]; + expect(finding?.message).toContain( + "1 registered project(s) could not be checked" + ); + const outcome = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + }); + expect(outcome.kind === "refused" ? outcome.message : "").toContain( + "--allow-unchecked" + ); + } +}); From 48bc0a6b136f18307c29bf0a04efe924bc31c89e Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 17:56:17 +0000 Subject: [PATCH 21/22] fix(secrets): retire-store cannot rule out HACK_SECRETS_FILE_KEY in another shell (HACK-2177) HACK_SECRETS_FILE_KEY overrides key files and the keychain at runtime and may be set in the shell of any project that selects the store. A key file or keychain that does not open the store is no proof that project cannot, so retire-store now refuses unless --allow-unchecked confirms none does. Doctor's recovery text and the docs say so. Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 5 +- docs/versions/v5/guides/upgrade-from-v4.md | 5 +- src/lib/doctor-secret-integrity.ts | 2 +- src/lib/secret-store-retire.ts | 36 ++++++++++-- tests/secret-store-retire.test.ts | 66 +++++++++++++++++++++- 5 files changed, 106 insertions(+), 8 deletions(-) diff --git a/docs/env.md b/docs/env.md index a4f081ecd..0a0c0585c 100644 --- a/docs/env.md +++ b/docs/env.md @@ -1686,7 +1686,10 @@ Some older surfaces are still compatibility-oriented: directory `.retired-/` beside it, deletes and prints nothing, and says how to move it back. It needs `--yes` without a terminal. It refuses a store that another registered project's key still opens, and, unless `--allow-unchecked` is - passed, a store some registered projects could not be checked for. Doctor then lists + passed, a store some registered projects could not be checked for. Because + `HACK_SECRETS_FILE_KEY` overrides key files and may be set in another shell for a + project that selects the store, the command cannot rule it out and also needs + `--allow-unchecked` as confirmation that none does. Doctor then lists retired files as information. Explicit key provisioning also uses an atomic mode-`0600` replacement and refuses symlinked key paths or immediate parent directories. A key generated on first use is diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index 134cc67c0..a147619f0 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -400,7 +400,10 @@ entries. Hack encrypted store, and refuses one that another registered project's key still opens. If some registered projects could not be checked (for example a worktree on an unmounted volume), it refuses unless you pass - `--allow-unchecked`. + `--allow-unchecked`. The same flag confirms that no shell running a + project that selects the store sets `HACK_SECRETS_FILE_KEY` to its key. + That variable overrides key files and the keychain and cannot be checked + from the command, so in practice retiring needs `--allow-unchecked`. - It prints the `mv` command that puts the store back. - Afterwards Doctor lists the retired file as information, and the project check no longer fails. diff --git a/src/lib/doctor-secret-integrity.ts b/src/lib/doctor-secret-integrity.ts index d9cee9490..77784d7bc 100644 --- a/src/lib/doctor-secret-integrity.ts +++ b/src/lib/doctor-secret-integrity.ts @@ -164,7 +164,7 @@ async function keylessStoreFinding(opts: { ].join(" "); return { status: usedHere ? "error" : "warn", - message: `${state} Its entries cannot be counted or read without that key. ${usage} Choose one: restore the key (put the key file back at ${opts.key} with mode 600 from a backup or another machine, or set HACK_SECRETS_FILE_KEY to the same key); or, if the key is gone for good, run \`${retireStoreCommand(opts.projectRoot)}\`, which moves the store aside to a timestamped name beside it, deletes nothing, and prints how to put it back. Do not provision a new key: it cannot open these entries.`, + message: `${state} Its entries cannot be counted or read without that key. ${usage} Choose one: restore the key (put the key file back at ${opts.key} with mode 600 from a backup or another machine, or set HACK_SECRETS_FILE_KEY to the same key); or, if the key is gone for good, run \`${retireStoreCommand(opts.projectRoot)}\`, which moves the store aside to a timestamped name beside it, deletes nothing, and prints how to put it back. Add --allow-unchecked once you have confirmed that no shell running a project that selects the store sets HACK_SECRETS_FILE_KEY to its key: that variable overrides key files and cannot be checked from here, so the command refuses without the flag. Do not provision a new key: it cannot open these entries.`, }; } diff --git a/src/lib/secret-store-retire.ts b/src/lib/secret-store-retire.ts index 1c8a1c213..79a405388 100644 --- a/src/lib/secret-store-retire.ts +++ b/src/lib/secret-store-retire.ts @@ -230,6 +230,7 @@ async function otherOpenersRefusal(input: { }); openers.push(...evidence.openers); const unknownKeys = evidence.unknown; + const envOnly = [...new Set(evidence.envOnly)].sort(); if (openers.length > 0) { return `The encrypted secret store at ${input.canonical} still opens with a configured key file (${[...new Set(openers)].sort().join(", ")}), so it is not retired; nothing was moved. Check HACK_SECRETS_FILE_KEY and this project's key configuration instead.`; } @@ -242,6 +243,9 @@ async function otherOpenersRefusal(input: { if (unknownKeys > 0) { return `${unknownKeys} configured key(s) for ${input.canonical} (a key file, or the OS keychain) could not be read, so whether one still opens it is unknown; nothing was moved. Fix their permissions and run hack doctor, or pass --allow-unchecked to retire it anyway.`; } + if (envOnly.length > 0) { + return `No configured key file or keychain entry opens ${input.canonical}, but HACK_SECRETS_FILE_KEY overrides both, and ${envOnly.join(", ")} may still open it with that variable set in another shell, which cannot be checked from here; nothing was moved. If you have confirmed none does, pass --allow-unchecked to retire it.`; + } if (found.unchecked > 0) { return `${found.unchecked} registered project(s) could not be checked (not available, or their Hack config or store file could not be read), so whether one still opens ${input.canonical} is unknown; nothing was moved. Run hack doctor to see them, or pass --allow-unchecked to retire it anyway.`; } @@ -257,9 +261,19 @@ async function otherOpenersRefusal(input: { async function keyEvidence(input: { readonly storePath: string; readonly keyFiles: ReadonlyMap; -}): Promise<{ readonly openers: readonly string[]; readonly unknown: number }> { +}): Promise<{ + readonly openers: readonly string[]; + readonly unknown: number; + /** + * Checkouts whose key file and keychain do not open the store. At + * runtime HACK_SECRETS_FILE_KEY overrides both, and it may be set in + * their own shell, which cannot be checked from here. + */ + readonly envOnly: readonly string[]; +}> { const openers: string[] = []; const keychainUsers: string[] = []; + const wrongKeyFile: string[] = []; let keychainReader = false; let unknown = 0; for (const [keyPath, names] of input.keyFiles) { @@ -274,10 +288,12 @@ async function keyEvidence(input: { } else if (state === "absent") { keychainReader = true; keychainUsers.push(...names); + } else { + wrongKeyFile.push(...names); } } if (!keychainReader) { - return { openers, unknown }; + return { openers, unknown, envOnly: envOnlyNames(wrongKeyFile) }; } const keychainState = await keychainKeyOpensStore({ storePath: input.storePath, @@ -288,10 +304,21 @@ async function keyEvidence(input: { ? keychainUsers.map((name) => `${name}, through the OS keychain`) : ["the OS keychain"]) ); - } else if (keychainState === "unknown") { + return { openers, unknown, envOnly: [] }; + } + if (keychainState === "unknown") { unknown += 1; } - return { openers, unknown }; + return { + openers, + unknown, + envOnly: envOnlyNames([...wrongKeyFile, ...keychainUsers]), + }; +} + +/** Names for the env-override refusal; "this project" when none is registered. */ +function envOnlyNames(names: readonly string[]): readonly string[] { + return names.length > 0 ? names : ["this project"]; } /** A registered project's primary checkout and each of its linked worktrees. */ @@ -623,6 +650,7 @@ async function eligibilityRefusal(input: { */ export async function describeRetireTarget(opts: { readonly projectDir?: string; + readonly allowUnchecked?: boolean; }): Promise { const planned = await planSecretStoreRetire(opts); return planned.kind === "plan" ? planned.plan.display : null; diff --git a/tests/secret-store-retire.test.ts b/tests/secret-store-retire.test.ts index ea1759480..03e2025cc 100644 --- a/tests/secret-store-retire.test.ts +++ b/tests/secret-store-retire.test.ts @@ -176,6 +176,7 @@ async function runRetire(opts: { readonly fixture: Fixture; readonly path: string; readonly yes: boolean; + readonly allowUnchecked?: boolean; }) { const child = Bun.spawn( [ @@ -186,6 +187,7 @@ async function runRetire(opts: { "--path", opts.path, ...(opts.yes ? ["--yes"] : []), + ...(opts.allowUnchecked ? ["--allow-unchecked"] : []), ], { cwd: root, @@ -255,11 +257,24 @@ test("retire-store refuses without --yes when it cannot prompt, then retires, an const fixture = await setUp(); const before = await readFile(fixture.storePath); + // No project has a key file: any may set HACK_SECRETS_FILE_KEY in another + // shell, which cannot be checked, so this refuses without the flag. + const envOnly = await runRetire({ + fixture, + path: fixture.bystander.projectRoot, + yes: true, + }); + expect(envOnly.code).not.toBe(0); + expect(envOnly.output).toContain("HACK_SECRETS_FILE_KEY"); + expect(envOnly.output).toContain("--allow-unchecked"); + expect(await readFile(fixture.storePath)).toEqual(before); + // Non-interactive and no --yes: refused, nothing moved. const refused = await runRetire({ fixture, path: fixture.bystander.projectRoot, yes: false, + allowUnchecked: true, }); expect(refused.code).not.toBe(0); expect(refused.output).toContain("E_INTERACTIVE_REQUIRED"); @@ -271,6 +286,7 @@ test("retire-store refuses without --yes when it cannot prompt, then retires, an fixture, path: fixture.bystander.projectRoot, yes: true, + allowUnchecked: true, }); expect(retired.code).toBe(0); expectValueFree(retired.output); @@ -374,6 +390,7 @@ test("retire re-checks the store under its lock: a key restored meanwhile is not retire = retireSecretStore({ projectName: "app-b", projectDir: fixture.bystander.projectDir, + allowUnchecked: true, }); await Bun.sleep(300); // Still waiting for the lock: the key comes back now. @@ -458,6 +475,7 @@ test("printed restore commands quote a store path that holds a quote", async () const outcome = await retireSecretStore({ projectName: "app-b", projectDir: fixture.bystander.projectDir, + allowUnchecked: true, }); expect(outcome.kind).toBe("retired"); const info = (await inspect(fixture.bystander, "app-b")).find( @@ -942,11 +960,15 @@ test("a store reached through a symlinked directory is named and retired by its ); // The confirmation names the real file, and the configured alias. expect( - await describeRetireTarget({ projectDir: fixture.bystander.projectDir }) + await describeRetireTarget({ + projectDir: fixture.bystander.projectDir, + allowUnchecked: true, + }) ).toBe(`${fixture.storePath} (configured as ${configured})`); const outcome = await retireSecretStore({ projectName: "app-b", projectDir: fixture.bystander.projectDir, + allowUnchecked: true, now: new Date("2026-11-13T12:00:00Z"), }); expect(outcome.kind).toBe("retired"); @@ -1009,6 +1031,7 @@ test("a target retargeted after confirmation is not retired", async () => { const { before, storeB, retargetToB } = await aliasedStores(fixture); const planned = await planSecretStoreRetire({ projectDir: fixture.bystander.projectDir, + allowUnchecked: true, }); expect(planned.kind).toBe("plan"); if (planned.kind !== "plan") { @@ -1020,6 +1043,7 @@ test("a target retargeted after confirmation is not retired", async () => { const outcome = await retireSecretStore({ projectDir: fixture.bystander.projectDir, approved: planned.plan, + allowUnchecked: true, }); expect(outcome.kind === "refused" ? outcome.message : "").toContain( "changed after it was confirmed" @@ -1036,6 +1060,7 @@ test("the locked eligibility check reads the pinned store, not the alias", async const { before, storeB, retargetToB } = await aliasedStores(fixture); const planned = await planSecretStoreRetire({ projectDir: fixture.bystander.projectDir, + allowUnchecked: true, }); if (planned.kind !== "plan") { throw new Error("expected a plan"); @@ -1043,6 +1068,7 @@ test("the locked eligibility check reads the pinned store, not the alias", async const outcome = await retireSecretStore({ projectDir: fixture.bystander.projectDir, approved: planned.plan, + allowUnchecked: true, beforeEligibility: async () => { // After the path check: A's key comes back, and the alias moves to B. await writeFile(fixture.keyPath, `${STORE_KEY}\n`, { mode: 0o600 }); @@ -1099,6 +1125,7 @@ test("control characters in a configured store path are shown, never sent to the fixture, path: fixture.bystander.projectRoot, yes: true, + allowUnchecked: true, }); expect(retired.code).toBe(0); expect(retired.output).toContain("lin\\x0d\\x1b[2Kked"); @@ -1279,6 +1306,43 @@ test("a store another registered project's key still opens is not retired", asyn } }); +test("a project with a wrong key file may still open the store through HACK_SECRETS_FILE_KEY: not retired without --allow-unchecked", async () => { + // Codex security review: HACK_SECRETS_FILE_KEY overrides key files and the + // keychain at runtime, and may be set in another project's shell. A key + // file that does not open the store is no proof that project cannot. + const fixture = await setUp(); + const before = await readFile(fixture.storePath); + const appAKey = join(fixture.reader.projectRoot, ".hack-store.key"); + await writeFile(appAKey, "synthetic-wrong-key-file\n", { mode: 0o600 }); + await chmod(appAKey, 0o600); + await writeFile( + join(fixture.reader.projectDir, "hack.config.json"), + `${JSON.stringify({ + name: "app-a", + dev_host: "app-a.hack", + controlPlane: { + secrets: { + backend: "encrypted_file", + encryptedFile: { path: fixture.storePath, keyPath: appAKey }, + }, + }, + })}\n` + ); + const refused = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + }); + const message = refused.kind === "refused" ? refused.message : ""; + expect(message).toContain("HACK_SECRETS_FILE_KEY overrides both"); + expect(message).toContain("app-a"); + expect(message).toContain("--allow-unchecked"); + expect(await readFile(fixture.storePath)).toEqual(before); + const allowed = await retireSecretStore({ + projectDir: fixture.bystander.projectDir, + allowUnchecked: true, + }); + expect(allowed.kind).toBe("retired"); +}); + test("retire refuses when registered projects could not be checked, unless --allow-unchecked", async () => { // The store may be another checkout's that could not be checked. const fixture = await setUp(); From e1fe59b88eddcbae8d7635ddf8b61b7c32417242 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Fri, 13 Nov 2026 18:09:33 +0000 Subject: [PATCH 22/22] docs(secrets): say retire-store needs --allow-unchecked in help and recovery (HACK-2177) The CLI help, the generated reference and the doctor recovery choice now say that retiring needs --allow-unchecked: it confirms that no shell for a project that selects the store sets HACK_SECRETS_FILE_KEY, which Hack cannot inspect. A key known to open the store still refuses. Co-Authored-By: Claude Opus 5.5 --- docs/reference/cli.md | 6 ++++-- src/commands/recovery-guidance.ts | 2 +- src/commands/secrets.ts | 6 ++++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/docs/reference/cli.md b/docs/reference/cli.md index ab5c8e77c..46880255c 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -2443,7 +2443,9 @@ Moves the store into a new directory .retired-/ beside it: noth and the key file is left alone. It prints how to put the store back. The store is the one this project selects (--path or the current directory), or the global one outside a project. Refused for a store that opens with the key in use, that another registered project's key still opens, or that is unreadable for -another reason; when some registered projects could not be checked, pass --allow-unchecked. Without a TTY (or with +another reason. HACK_SECRETS_FILE_KEY set in another shell cannot be inspected, so retiring needs --allow-unchecked: +pass it once you have confirmed no shell for a project that selects the store sets it (it also covers registered +projects that could not be checked). Without a TTY (or with HACK_NO_INTERACTIVE / --no-interactive) it is refused with E_INTERACTIVE_REQUIRED unless --yes is passed. ### Options @@ -2452,7 +2454,7 @@ HACK_NO_INTERACTIVE / --no-interactive) it is refused with E_INTERACTIVE_REQUIRE | --- | --- | | `--path, -p ` | Run a project command against a repo path (overrides cwd search) | | `--yes` | Confirm moving the store aside without prompting (required without a TTY or with HACK_NO_INTERACTIVE) | -| `--allow-unchecked` | Retire even when some registered projects could not be checked for another key that opens the store | +| `--allow-unchecked` | Accept what cannot be checked: a HACK_SECRETS_FILE_KEY set in another shell for a project that selects the store, and registered projects that could not be checked. Retiring needs it; a key known to open the store still refuses | | `--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 | diff --git a/src/commands/recovery-guidance.ts b/src/commands/recovery-guidance.ts index a9d44c299..ff34bce11 100644 --- a/src/commands/recovery-guidance.ts +++ b/src/commands/recovery-guidance.ts @@ -31,7 +31,7 @@ const RETIRE_STORE_COMMAND_PATTERN = function secretStoreChoices(command: string): readonly string[] { return [ "Restore the encrypted secret store's key: put the key file back where the secret integrity check says (mode 600), or set HACK_SECRETS_FILE_KEY to the same key.", - `Or, if the key is gone for good, retire the store: \`${command}\` moves it aside to a timestamped name beside it, deletes nothing, and prints how to put it back.`, + `Or, if the key is gone for good, retire the store: \`${command}\` moves it aside to a timestamped name beside it, deletes nothing, and prints how to put it back. It needs --allow-unchecked, your confirmation that no shell for a project that selects the store sets HACK_SECRETS_FILE_KEY to its key, which Hack cannot inspect.`, ]; } const SAFE_SHELL_ARG_PATTERN = /^[A-Za-z0-9_./:-]+$/u; diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index 286c35ca6..18f190a5a 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -51,7 +51,7 @@ const optAllowUnchecked = defineOption({ type: "boolean", long: "--allow-unchecked", description: - "Retire even when some registered projects could not be checked for another key that opens the store", + "Accept what cannot be checked: a HACK_SECRETS_FILE_KEY set in another shell for a project that selects the store, and registered projects that could not be checked. Retiring needs it; a key known to open the store still refuses", } as const); const secretsOptions = [optService] as const; @@ -111,7 +111,9 @@ const retireStoreSpec = defineCommand({ "and the key file is left alone. It prints how to put the store back. The store is the one this project", "selects (--path or the current directory), or the global one outside a project. Refused for a store that", "opens with the key in use, that another registered project's key still opens, or that is unreadable for", - "another reason; when some registered projects could not be checked, pass --allow-unchecked. Without a TTY (or with", + "another reason. HACK_SECRETS_FILE_KEY set in another shell cannot be inspected, so retiring needs --allow-unchecked:", + "pass it once you have confirmed no shell for a project that selects the store sets it (it also covers registered", + "projects that could not be checked). Without a TTY (or with", "HACK_NO_INTERACTIVE / --no-interactive) it is refused with E_INTERACTIVE_REQUIRED unless --yes is passed.", ].join("\n"), options: retireStoreOptions,