From 5129aeb6cc30dbacf0505de39df8b437fdc49244 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 12 Oct 2026 22:27:07 -0400 Subject: [PATCH 1/2] test(dns): cover the native DNS activate/deactivate administrator prompt and add the env secrets threat model The `hack global dns activate|deactivate` authorize closures move into createNativeDnsAuthorizer (src/lib/native-domain-dns-authorization.ts) with injectable interactivity, confirmation and administrator prompt. A failed sudo -v now throws NativeDnsAdministratorAuthorizationError (closed outcome "authorization-failed" and failure class; message unchanged). Tests wire the authorizer into the real activation/deactivation flows and host effect boundary and prove: sudo -v runs before every privileged and file effect, a failed prompt is authorization-failed (not denied) and changes nothing, and a decline or a non-interactive run never reaches sudo. Adds docs/security/env-secrets-threat-model.md (HACK-1127), linked from docs/env.md. Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 3 + docs/security/env-secrets-threat-model.md | 157 ++++++ src/commands/global.ts | 54 +-- src/lib/native-domain-dns-authorization.ts | 115 +++++ tests/native-domain-dns-authorization.test.ts | 459 ++++++++++++++++++ 5 files changed, 740 insertions(+), 48 deletions(-) create mode 100644 docs/security/env-secrets-threat-model.md create mode 100644 src/lib/native-domain-dns-authorization.ts create mode 100644 tests/native-domain-dns-authorization.test.ts diff --git a/docs/env.md b/docs/env.md index 0729a7fb..584a41bc 100644 --- a/docs/env.md +++ b/docs/env.md @@ -482,6 +482,8 @@ When you pass `--env` or `--service`, Hack creates a stable scoped workspace nam ## Secret key handling Secret values in the YAML files are encrypted with the project key. +The [env and secrets threat model](security/env-secrets-threat-model.md) lists what +this protects against and the gaps that are still open. Default behavior: @@ -640,6 +642,7 @@ If you are writing new docs or new project setup flows, document the YAML overla - [Initialize a project](guides/init-project.md) - [Sessions](sessions.md) - [CLI reference](cli.md) +- [Env and secrets threat model](security/env-secrets-threat-model.md): assets, trust boundaries, current mitigations and open gaps - [Pulumi-style env config design](plans/2026-03-27-pulumi-style-env-config-design.md) diff --git a/docs/security/env-secrets-threat-model.md b/docs/security/env-secrets-threat-model.md new file mode 100644 index 00000000..dea4e214 --- /dev/null +++ b/docs/security/env-secrets-threat-model.md @@ -0,0 +1,157 @@ +# Env and secrets threat model + +This document describes how Hack protects project env values and secret keys on a +developer machine. It lists the assets, the trust boundaries, the threats, the +mitigations that are merged on `next`, and the gaps that are still open. It tracks +Linear issue HACK-1127. + +For day-to-day usage, see [Env & secrets](../env.md). This page does not change any +behavior. Where it names code, the code is the source of truth. + +## Scope + +In scope: + +- the modern YAML env model (`secretsprovider: project_key`) and its project key +- the legacy secret stores still reachable through `hack env backend`: `keychain`, + `encrypted_file`, and the `cloud` shim that uses `encrypted_file` +- the paths that deliver values: runtime injection into Compose and host commands, + `hack env materialize`, `hack env get`, sessions, and the MCP server +- secret values that come from an external manager such as 1Password + +Out of scope: the retired hosted account, dashboard and ticket flows, and the +unsupported remote, gateway, node and dispatch surfaces. One residual risk in retained +node code is listed under [open gaps](#open-gaps) so that it is not lost. + +## Assets + +| Asset | Where it lives | What it protects | +| --- | --- | --- | +| Project env key | `.hack.secret.key` in the checkout, a shared key under the git common dir, the primary checkout's key (linked worktrees), or `HACK_ENV_SECRET_KEY` | Every `secure: v1:...` value in `.hack/hack.env.*.yaml`. The AES-256-GCM key is the SHA-256 of the key text. | +| Encrypted YAML values (`project_key`) | `.hack/hack.env..yaml`, usually committed | Secret values at rest in git. Plaintext scalars in the same files are not secret. | +| `encrypted_file` store and its key | Store file chosen in config; key from `HACK_SECRETS_FILE_KEY`, `~/.hack/secrets-file.key`, or the keychain service `hack-secrets-backend` | Legacy backend values, including the `cloud` shim. | +| `keychain` store | macOS keychain through `Bun.secrets`, one service per project | Legacy backend values. | +| Values from an external manager (for example 1Password) | The manager's vault, until the user pipes a value into Hack or injects `HACK_ENV_SECRET_KEY` into a process | Hack has no first-party 1Password integration. Custody passes to Hack's stores at the moment of the pipe or injection. | +| Decrypted values in flight | Process environments of Compose containers, host commands, lifecycle processes and session shells; `.hack/.env`; `.hack/.internal/compose.env.override.yml` | The resolved plaintext, after decryption. | + +## Trust boundaries + +1. **Host user account.** Hack runs as the developer. Anything else that runs as the + same user (editors, language servers, project dependencies, agents) can read the + same files and keychain items the user can. Hack does not try to defend against + code that already runs as the user. It does try to keep secrets out of places + that other users, other machines, or later readers can reach: argv, logs, shared + files, git, and published build output. +2. **Containers.** Compose containers receive values as process environment. Anyone + who can reach the Docker engine can read them with `docker inspect`. Container + images and layers must never contain the project key. +3. **Agents and MCP.** An agent acts with the user's rights, but its prompts, + transcripts and tool output may leave the machine. Hack treats the agent context + as a sink that must not receive secret values unless the user asks for one. +4. **CI and managed containers.** CI receives the key only as `HACK_ENV_SECRET_KEY` + from the CI secret store. The key file is never baked into an image. +5. **Repository inputs.** A cloned repository is untrusted input. It may contain + symlinks, malformed ciphertext, oversized files or a committed key. + +## Threats and current mitigations + +Each row names the merged change and the file that carries the control. PR numbers +refer to `hack-dance/hack`. + +### Leakage through argv, logs and process listings + +| Threat | Mitigation on `next` | +| --- | --- | +| A secret passed as a command argument lands in shell history and in `ps` output that other local users can read. | `--value-stdin` on `hack env add` and `hack env set` reads the value from stdin (at most 64 KiB, fixed refusals that never repeat the input). See `src/lib/env-value-stdin.ts` (`readEnvValueFromStdin`, `assertEnvValueStdinUsage`), #308. The hidden prompt stays available in a terminal. | +| A mistyped `KEY=value` argument is echoed back in an error, or turned into a file name or overlay name. | Key, scope and `--env` arguments are validated before any file access, and the refusal text never contains the argument: `src/cli/env-arguments.ts`, `src/cli/env-selection.ts`, `parseEnvConfigSelection` in `src/lib/project.ts`, the shared check in `src/cli/run.ts` (#308). `--project` and `--target` follow the same rule through `src/cli/project-selector.ts` (`parseProjectSelectorOption`, `unknownProjectMessage`, `resolveRegisteredProjectOption`) and the `runCli` check (#318). Proven by `tests/env-key-argument-redaction-cli.test.ts` and `tests/project-target-argument-redaction-cli.test.ts`. | +| Diagnostics print key bytes or absolute paths. | The `hack doctor` env key check uses closed codes and project-relative subjects only: `src/lib/doctor-env-key.ts` (#309). Envelope and key-file failures use one fixed message: `src/lib/private-file.ts`, `src/lib/project-env-config.ts`, `src/lib/secret-store.ts` (#288). | +| Host command records keep arguments or env values. | `hack host exec` and `hack env exec` records are payload-free (see [host command lifetime](../env.md#host-command-lifetime-and-diagnostics)). | +| Tests leak the contributor's real `HOME`, keychain, Docker config or `HACK_*` variables into a spawned CLI. | `tests/helpers/private-cli-env.ts` (`privateCliEnv`) builds the whole child environment, and `tests/cli-spawn-env-isolation.test.ts` fails any source-CLI spawn that copies the runner environment (#319). | + +### Symlinks, TOCTOU and file modes + +| Threat | Mitigation on `next` | +| --- | --- | +| A repository plants `.hack.secret.key` as a symlink (dangling or not) so that Hack reads or writes a file it should not. | Key reads go through `readBoundedPrivateFile` (`lstat`, `O_NOFOLLOW`, descriptor and path identity checks, 1 MiB bound, fatal UTF-8). New keys are created with `createPrivateFileExclusive` (`O_CREAT|O_EXCL|O_NOFOLLOW`, mode 0600, `fchmod`, `fsync`). Both are in `src/lib/private-file.ts`, used by `resolveProjectEnvKey`, `ensureProjectEnvSecretKey` and `resolveInheritedProjectEnvKey` in `src/lib/project-env-config.ts` (#288). | +| A symlinked `.hack/.env`, overlay file or `.hack` directory redirects a write, or a reader sees a partial file. | `src/lib/private-env-file.ts` (`readPrivateEnvFile`, `writePrivateEnvFileIfChanged`) and `replacePrivateFileAtomic` in `src/lib/private-file.ts`: a private sibling is written and fsynced, then renamed over the target; symlinks and non-regular entries are refused with one fixed message (#302). Used by `src/lib/hack-env.ts` and `materializeProjectEnv`. | +| A world-readable env file or store. | `.hack/.env`, overlays and the `encrypted_file` store are written 0600, and a same-content file with group or other access is tightened (#302). The `hack doctor` env key check reports `env-key-weak-permissions` and `env-key-wrong-owner` (#309). | +| Two writers of the `encrypted_file` store lose an update. | An in-process queue plus a cooperative `.lock` using the owner protocol from `src/lib/projects-registry-lock.ts`; a symlinked store path is refused for writes (`src/lib/secret-store.ts`, #302). | + +### Malformed or hostile ciphertext + +| Threat | Mitigation on `next` | +| --- | --- | +| Truncated tags, oversized ciphertext or non-canonical base64 cause a wrong decision or a crash. | `decryptProjectEnvValue` (`src/lib/project-env-config.ts`) and `parseEncryptedEnvelope`/`decryptStore` (`src/lib/secret-store.ts`) require canonical base64 (`decodeCanonicalBase64` in `src/lib/private-file.ts`), a 12-byte IV and a 16-byte tag, pin `authTagLength: 16`, bound the size before decoding, and fail with one fixed error (#288). | +| Ciphertext is moved to another variable, scope or file and still decrypts. | Not prevented. The current v1 format authenticates no associated data. `tests/aes-gcm-v1-context-binding.test.ts` pins this behavior so that any change is deliberate (#302). See [open gaps](#open-gaps). | + +### Public-artifact and git leakage + +| Threat | Mitigation on `next` | +| --- | --- | +| The key is committed. | The root `.gitignore` gets a `.hack.secret.key` entry when the key is created. Encryption refuses a git-tracked key, including an inherited primary key, before it is copied (#288). `hack doctor` reports `env-key-tracked` at any depth (#309) and the "generated files" check can untrack a root key with `--fix`. | +| A key copy ships in build output or a static site. | `hack doctor` reports `env-key-public-path` for copies under `public/`, `static/`, `dist/`, `build/`, `out/`, `www/`, `_site/`, `.next/` and `.output/` (#309). | +| A weak, short or repetitive key. | `env-key-wrong-length` and `env-key-low-entropy` (#309). Generated keys are 32 random bytes in base64url. | +| Materialized `.hack/.env` or local overrides are committed. | Hack owns a committed `.hack/.gitignore` block, and `hack doctor` flags tracked generated files (see [ignore rules](../env.md#ignore-rules-committed-hackgitignore)). | +| A dependency vulnerability ships in Hack. | `bun install --frozen-lockfile`, and the read-only `scripts/dependabot-inventory.ts` lists open alerts (#309). Hack does not sandbox project dependencies that run under `hack host exec`; they receive the injected env by design. | + +### Agent and MCP exfiltration + +| Threat | Mitigation on `next` | +| --- | --- | +| An agent puts a secret in a prompt, an argument or a log. | Canonical agent guidance in `src/agents/instruction-source.ts` says to pipe secrets with `--value-stdin` or use the hidden prompt, to treat `hack env get` and `--show-secrets` as plaintext sinks, and never to repeat their output (#308). The generated plugin bundle and checked-in examples render the same text. | +| An MCP tool returns secret values. | `hack.project.env.keys` and `hack.project.env.explain` in `src/mcp/server.ts` return names and provenance only; `buildEnvKeysArgs` never emits `--show-secrets`, and child stdout and stderr are withheld. | +| A compromised dependency or agent reads the key file. | Out of reach for Hack inside the same user account (boundary 1). File modes and the symlink rules above only stop other users and redirected writes. | + +## Open gaps + +These gaps are known and not closed on `next`. Each one needs its own change. + +1. **Crash-safe key rotation.** Rotation is manual: move the key aside and re-add each + secret. A crash or a partial run leaves files encrypted under two different keys, + and Hack has no journal or resume step for this. There is also no command that + rotates the `encrypted_file` store key. +2. **Crypto decision record pending independent review.** The KDF (a single SHA-256 + of the key text), the missing AAD binding, and the v1 envelope format are + characterized in tests but have no written decision record. #288 was flagged for an + independent crypto review (WU14) that has not happened yet. Any format change must + keep v1 values readable. +3. **Live keychain and 1Password custody receipts.** No test or receipt proves, on a + real macOS host, that values stored through the `keychain` backend or piped from + 1Password stay only in the expected custody: the keychain item, the encrypted YAML + or store, and the target process environment. The CLI tests disable the keychain + fallback (`HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK=1`) and never call 1Password. +4. **Plaintext Compose override.** For the Compose runtime, `src/commands/project.ts` + writes resolved values, including decrypted secrets, to + `.hack/.internal/compose.env.override.yml` with `writeTextFileIfChanged` from + `src/lib/fs.ts`. That call uses `Bun.write`, so the file gets the process umask + mode (often 0644) and a symlink at that path is followed. The #302 rules do not + cover it. +5. **YAML env file writes.** `writeProjectEnvConfigFile` in + `src/lib/project-env-config.ts` also uses `writeTextFileIfChanged`, so + `.hack/hack.env.*.yaml` writes follow a symlink at the file. These files hold + ciphertext and non-secret plaintext, but a hostile repository could redirect the + write. +6. **`encrypted_file` key file hygiene.** `readEncryptedFileKey` and + `writeEncryptedFileKey` in `src/lib/secret-store.ts` read with `readTextFile` and + write with `writeFile` plus `chmod`. They do not use the `src/lib/private-file.ts` + helpers, so they follow symlinks and do not create the key exclusively. +7. **Directory TOCTOU.** The #302 directory checks are path-based `lstat` with + identity rechecks, not `openat`/`renameat`. A swap of `.hack` between the recheck + and `rename(2)` is not fully closed. The store lock is cooperative, and v4 writers + do not honor it. +8. **MCP command output.** `hack.project.run`, `hack.project.exec` and the log tools + return child output. A command such as `printenv`, or an app that logs its env, + puts injected values into the agent context. The only gates are the client's + approval for destructive tools and the agent guidance. +9. **Container env visibility.** Values in container environments are visible to + anyone with Docker engine access (`docker inspect`). This is inherent to env + injection. +10. **Retained node code (unsupported).** `src/lib/nodes-registry.ts` stores node + tokens with `security add-generic-password ... -w `, which puts the value in + argv for the life of that process. The node surface is unsupported; fix or remove + it before that surface returns. + +## Related + +- [Env & secrets](../env.md) +- [CLI reference](../cli.md) diff --git a/src/commands/global.ts b/src/commands/global.ts index 26fd8207..d85f00fb 100644 --- a/src/commands/global.ts +++ b/src/commands/global.ts @@ -57,11 +57,6 @@ import { getLaunchdServiceStatus } from "../daemon/launchd.ts"; import { resolveDaemonPaths } from "../daemon/paths.ts"; import { isProcessRunning } from "../daemon/process.ts"; import { readDaemonStatus } from "../daemon/status.ts"; -import { - ADMINISTRATOR_PROMPT_HINT, - describeAdministratorAuthorizationFailure, - requestAdministratorAuthorization, -} from "../lib/admin-authorization.ts"; import { resolveGlobalConfigPath, resolveGlobalHackDir, @@ -106,6 +101,7 @@ import { } from "../lib/mutagen.ts"; import type { NativeDnsActivationInspection } from "../lib/native-domain-dns-activate.ts"; import { activateNativeDomainDns } from "../lib/native-domain-dns-activate.ts"; +import { createNativeDnsAuthorizer } from "../lib/native-domain-dns-authorization.ts"; import { deactivateNativeDomainDns } from "../lib/native-domain-dns-deactivate.ts"; import { createNativeDnsHostDependencies } from "../lib/native-domain-dns-host.ts"; import { prepareNativeDnsLaunchdRestart } from "../lib/native-domain-dns-launchd.ts"; @@ -536,20 +532,7 @@ async function handleGlobalDnsActivate({ } return current.inspection; }; - const authorize = async () => { - if (!isInteractiveTerminal()) { - return false; - } - const accepted = await confirmSafe({ - message: `Activate only *.${domain} at 127.0.0.1 via a new dnsmasq claim and /etc/resolver file? This restarts dnsmasq; existing v4 suffixes remain unchanged.`, - initialValue: false, - nonInteractive: "decline", - }); - if (!accepted) { - return false; - } - return await requireNativeAdministratorAuthorization(); - }; + const authorize = createNativeDnsAuthorizer({ action: "activate", domain }); const deps = createNativeDnsHostDependencies({ receiptPath: initial.receiptPath, dnsmasqPath: initial.inspection.plan.dnsmasqPath, @@ -655,20 +638,10 @@ async function handleGlobalDnsDeactivate({ } return current.inspection; }; - const authorize = async () => { - if (!isInteractiveTerminal()) { - return false; - } - const accepted = await confirmSafe({ - message: `Deactivate only the owned *.${domain} DNS claim? This restarts dnsmasq; the existing v4 parent remains unchanged.`, - initialValue: false, - nonInteractive: "decline", - }); - if (!accepted) { - return false; - } - return await requireNativeAdministratorAuthorization(); - }; + const authorize = createNativeDnsAuthorizer({ + action: "deactivate", + domain, + }); const deps = createNativeDnsHostDependencies({ receiptPath: initial.receiptPath, dnsmasqPath: initial.inspection.plan.dnsmasqPath, @@ -2338,21 +2311,6 @@ async function runMacPrivilegedCommand(opts: { }); } -/** - * Native administrator prompt after an approved DNS change. A failed prompt - * throws, so it is reported as an authorization failure and never as the - * user's decline. - */ -async function requireNativeAdministratorAuthorization(): Promise { - const result = await requestAdministratorAuthorization(); - if (result.status === "failed") { - throw new Error( - `macOS administrator authorization did not complete (${describeAdministratorAuthorizationFailure(result)}); nothing changed. ${ADMINISTRATOR_PROMPT_HINT}` - ); - } - return true; -} - function isInteractiveTerminal(): boolean { return process.stdin.isTTY === true; } diff --git a/src/lib/native-domain-dns-authorization.ts b/src/lib/native-domain-dns-authorization.ts new file mode 100644 index 00000000..3c67906e --- /dev/null +++ b/src/lib/native-domain-dns-authorization.ts @@ -0,0 +1,115 @@ +import { + ADMINISTRATOR_PROMPT_HINT, + type AdministratorAuthorization, + type AdministratorAuthorizationFailure, + describeAdministratorAuthorizationFailure, + requestAdministratorAuthorization, +} from "./admin-authorization.ts"; +import { confirmSafe } from "./interactivity.ts"; +import type { NativeDnsActivationDependencies } from "./native-domain-dns-activate.ts"; + +/** The `hack global dns` change a native DNS authorizer approves. */ +export type NativeDnsAuthorizationAction = "activate" | "deactivate"; + +/** + * The native administrator prompt did not complete after the user approved the + * change. Carries only the closed failure class, never sudo output. + */ +export class NativeDnsAdministratorAuthorizationError extends Error { + readonly outcome = "authorization-failed" as const; + readonly failure: AdministratorAuthorizationFailure; + + constructor( + result: Extract + ) { + super( + `macOS administrator authorization did not complete (${describeAdministratorAuthorizationFailure(result)}); nothing changed. ${ADMINISTRATOR_PROMPT_HINT}` + ); + this.name = "NativeDnsAdministratorAuthorizationError"; + this.failure = result.failure; + } +} + +/** + * Native administrator prompt after an approved DNS change. A failed prompt + * throws, so it is reported as an authorization failure and never as the + * user's decline. + */ +export async function requireNativeAdministratorAuthorization( + opts: { + readonly requestAdministrator?: () => Promise; + } = {} +): Promise { + const request = + opts.requestAdministrator ?? + (async () => await requestAdministratorAuthorization()); + const result = await request(); + if (result.status === "failed") { + throw new NativeDnsAdministratorAuthorizationError(result); + } + return true; +} + +export interface NativeDnsAuthorizerDependencies { + /** True only when a human can answer the confirmation and the sudo prompt. */ + readonly isInteractive: () => boolean; + readonly confirm: (message: string) => Promise; + /** Shows the native `sudo -v` prompt; later privileged steps use `sudo -n`. */ + readonly requestAdministrator: () => Promise; +} + +const DEFAULTS: NativeDnsAuthorizerDependencies = { + isInteractive: () => process.stdin.isTTY === true, + confirm: async (message) => + await confirmSafe({ + message, + initialValue: false, + nonInteractive: "decline", + }), + requestAdministrator: async () => await requestAdministratorAuthorization(), +}; + +/** Confirmation text naming exactly the owned claim the change touches. */ +export function nativeDnsConfirmationMessage(opts: { + readonly action: NativeDnsAuthorizationAction; + readonly domain: string; +}): string { + if (opts.action === "activate") { + return `Activate only *.${opts.domain} at 127.0.0.1 via a new dnsmasq claim and /etc/resolver file? This restarts dnsmasq; existing v4 suffixes remain unchanged.`; + } + return `Deactivate only the owned *.${opts.domain} DNS claim? This restarts dnsmasq; the existing v4 parent remains unchanged.`; +} + +/** + * Authorizer for `hack global dns activate|deactivate`. + * + * Contract: + * - Without an interactive terminal, or when the user declines, it returns + * false (a decline) and never reaches sudo. + * - After an approval it shows the native administrator prompt. A failed + * prompt throws NativeDnsAdministratorAuthorizationError instead of + * returning false, so the caller reports authorization-failed, not denied. + * - The activation and deactivation flows call it before any privileged step, + * so no `sudo -n` step runs before the prompt succeeds. + */ +export function createNativeDnsAuthorizer(opts: { + readonly action: NativeDnsAuthorizationAction; + readonly domain: string; + readonly dependencies?: Partial; +}): NativeDnsActivationDependencies["authorize"] { + const deps = { ...DEFAULTS, ...opts.dependencies }; + return async () => { + if (!deps.isInteractive()) { + return false; + } + const accepted = await deps.confirm( + nativeDnsConfirmationMessage({ action: opts.action, domain: opts.domain }) + ); + if (!accepted) { + return false; + } + return await requireNativeAdministratorAuthorization({ + requestAdministrator: deps.requestAdministrator, + }); + }; +} diff --git a/tests/native-domain-dns-authorization.test.ts b/tests/native-domain-dns-authorization.test.ts new file mode 100644 index 00000000..644a3fc2 --- /dev/null +++ b/tests/native-domain-dns-authorization.test.ts @@ -0,0 +1,459 @@ +import { afterEach, expect, test } from "bun:test"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { AdministratorAuthorization } from "../src/lib/admin-authorization.ts"; +import { + activateNativeDomainDns, + NativeDnsActivationError, +} from "../src/lib/native-domain-dns-activate.ts"; +import { + createNativeDnsAuthorizer, + NativeDnsAdministratorAuthorizationError, + type NativeDnsAuthorizationAction, + requireNativeAdministratorAuthorization, +} from "../src/lib/native-domain-dns-authorization.ts"; +import { + deactivateNativeDomainDns, + NativeDnsDeactivationError, +} from "../src/lib/native-domain-dns-deactivate.ts"; +import { + createNativeDnsHostDependencies, + type NativeDnsCommandRunner, + type NativeDnsHostDependencies, +} from "../src/lib/native-domain-dns-host.ts"; +import { + type NativeDnsPlan, + planNativeDomainDns, +} from "../src/lib/native-domain-dns-plan.ts"; + +/* + * `hack global dns activate|deactivate` authorizer wired into the real + * activation/deactivation flows and host effect boundary. Every effect is + * recorded in one ordered log; sudo, dnsmasq and system DNS are fakes and all + * files live in a private temporary directory. + */ + +const DOMAIN = "project.example.test"; +const SUDO_V = "sudo -v"; +const roots: string[] = []; + +afterEach(async () => { + for (const root of roots.splice(0)) { + await rm(root, { recursive: true, force: true }); + } +}); + +const AUTHORIZED: AdministratorAuthorization = { status: "authorized" }; +const REJECTED: AdministratorAuthorization = { + status: "failed", + failure: "rejected", + exitCode: 1, +}; + +function recordingAuthorizer(opts: { + readonly action: NativeDnsAuthorizationAction; + readonly log: string[]; + readonly interactive?: boolean; + readonly accept?: boolean; + readonly administrator?: AdministratorAuthorization; +}) { + const prompts: string[] = []; + const authorize = createNativeDnsAuthorizer({ + action: opts.action, + domain: DOMAIN, + dependencies: { + isInteractive: () => opts.interactive ?? true, + confirm: async (message) => { + prompts.push(message); + opts.log.push("confirm"); + return opts.accept ?? true; + }, + requestAdministrator: async () => { + opts.log.push(SUDO_V); + return opts.administrator ?? AUTHORIZED; + }, + }, + }); + return { authorize, prompts }; +} + +async function workspace() { + const root = await mkdtemp(join(tmpdir(), "native-dns-authorization-")); + roots.push(root); + const includeDir = join(root, "dnsmasq.d"); + const resolverDir = join(root, "resolver"); + const privateDir = join(root, "private"); + await Promise.all([ + mkdir(includeDir), + mkdir(resolverDir), + mkdir(privateDir, { mode: 0o700 }), + ]); + return { + root, + includeDir, + resolverDir, + mainPath: join(root, "dnsmasq.conf"), + hostsPath: join(root, "hosts"), + receiptPath: join(privateDir, "native-dns.json"), + }; +} + +type Workspace = Awaited>; + +function planFor( + ws: Workspace, + active?: { readonly plan: NativeDnsPlan } +): NativeDnsPlan { + return planNativeDomainDns({ + domain: DOMAIN, + mainConfig: { + path: ws.mainPath, + content: `conf-dir=${ws.includeDir},*.conf\n`, + }, + includeDir: ws.includeDir, + includeFiles: active + ? [ + { + path: active.plan.dnsmasqPath, + content: active.plan.dnsmasqContent, + }, + ] + : [], + resolverDir: ws.resolverDir, + resolverFiles: active + ? [ + { + path: active.plan.resolverPath, + content: active.plan.resolverContent, + }, + ] + : [], + hosts: { path: ws.hostsPath, content: "127.0.0.1 localhost\n" }, + dnsmasqArgs: ["dnsmasq", "-7", `${ws.includeDir},*.conf`], + receipt: active ? active.plan.activeReceipt : null, + }); +} + +/** + * Host effect boundary whose privileged runner, service restart and cache + * flush (all `sudo -n` in production) and file effects append to `log`. + */ +function recordingHost(opts: { + readonly ws: Workspace; + readonly plan: NativeDnsPlan; + readonly authorize: NativeDnsHostDependencies["authorize"]; + readonly log: string[]; + readonly loopbackAnswer: boolean; +}): NativeDnsHostDependencies { + const runPrivileged: NativeDnsCommandRunner = async (command) => { + const action = command[7] ?? ""; + opts.log.push(`sudo -n resolver ${action}`); + if (action === "remove") { + await rm(command[8] ?? "", { force: true }); + return { exitCode: 0, stdout: "1\n", stderr: "" }; + } + return { exitCode: 0, stdout: "42:73\n", stderr: "" }; + }; + const runCommand: NativeDnsCommandRunner = async (command) => { + if (!opts.loopbackAnswer) { + return { exitCode: 0, stdout: "", stderr: "" }; + } + const stdout = + command[0] === "/usr/bin/dscacheutil" + ? "name: probe\nip_address: 127.0.0.1\n" + : "127.0.0.1\n"; + return { exitCode: 0, stdout, stderr: "" }; + }; + const host = createNativeDnsHostDependencies({ + receiptPath: opts.ws.receiptPath, + dnsmasqPath: opts.plan.dnsmasqPath, + resolverPath: opts.plan.resolverPath, + dnsmasqBinary: join(opts.ws.root, "dnsmasq"), + dnsmasqMainConfigPath: opts.ws.mainPath, + dnsmasqIncludeDir: opts.ws.includeDir, + inspectPlan: async () => ({ plan: opts.plan, fingerprint: "snapshot" }), + authorize: opts.authorize, + restartDnsmasq: async () => { + opts.log.push("sudo -n restart dnsmasq"); + }, + flushDnsCache: async () => { + opts.log.push("sudo -n flush dns cache"); + }, + runCommand, + runPrivileged, + }); + return { + ...host, + async writeReceipt(receipt) { + opts.log.push(`write receipt ${receipt.state}`); + await host.writeReceipt(receipt); + }, + async createExclusive(file) { + opts.log.push(`create ${file.path}`); + return await host.createExclusive(file); + }, + async removeIfOwned(file) { + opts.log.push(`remove ${file.path}`); + return await host.removeIfOwned(file); + }, + }; +} + +/** Publish an active claim on disk without touching the recorded log. */ +async function activeClaim(ws: Workspace) { + const available = planFor(ws); + const setup = createNativeDnsHostDependencies({ + receiptPath: ws.receiptPath, + dnsmasqPath: available.dnsmasqPath, + resolverPath: available.resolverPath, + dnsmasqBinary: join(ws.root, "dnsmasq"), + dnsmasqMainConfigPath: ws.mainPath, + dnsmasqIncludeDir: ws.includeDir, + inspectPlan: async () => ({ plan: available, fingerprint: "setup" }), + authorize: async () => { + throw new Error("setup never authorizes"); + }, + restartDnsmasq: async () => {}, + flushDnsCache: async () => {}, + runCommand: async () => ({ exitCode: 1, stdout: "", stderr: "" }), + runPrivileged: async () => ({ exitCode: 1, stdout: "", stderr: "" }), + }); + await setup.writeReceipt(available.pendingReceipt); + await setup.writeReceipt(available.activeReceipt); + await writeFile(available.dnsmasqPath, available.dnsmasqContent); + await writeFile(available.resolverPath, available.resolverContent); + return planFor(ws, { plan: available }); +} + +function assertAuthorizedFirst(log: readonly string[]): void { + expect(log.slice(0, 2)).toEqual(["confirm", SUDO_V]); + expect(log.filter((entry) => entry === SUDO_V)).toHaveLength(1); + // Non-vacuous: privileged and file effects did run, all after sudo -v. + expect(log.some((entry) => entry.startsWith("sudo -n "))).toBe(true); + expect(log.some((entry) => entry.startsWith("write receipt "))).toBe(true); +} + +test("activate: native administrator authorization precedes every privileged and file effect", async () => { + const ws = await workspace(); + const plan = planFor(ws); + const log: string[] = []; + const { authorize, prompts } = recordingAuthorizer({ + action: "activate", + log, + }); + const active = await activateNativeDomainDns({ + dependencies: recordingHost({ + ws, + plan, + authorize, + log, + loopbackAnswer: true, + }), + }); + expect(active).toEqual(plan.activeReceipt); + assertAuthorizedFirst(log); + expect(prompts).toEqual([ + `Activate only *.${DOMAIN} at 127.0.0.1 via a new dnsmasq claim and /etc/resolver file? This restarts dnsmasq; existing v4 suffixes remain unchanged.`, + ]); +}); + +test("activate: a failed administrator prompt is authorization-failed, not denied, and changes nothing", async () => { + const ws = await workspace(); + const plan = planFor(ws); + const log: string[] = []; + const { authorize } = recordingAuthorizer({ + action: "activate", + log, + administrator: REJECTED, + }); + const failure = await activateNativeDomainDns({ + dependencies: recordingHost({ + ws, + plan, + authorize, + log, + loopbackAnswer: true, + }), + }).catch((error: unknown) => error); + expect(failure).toBeInstanceOf(NativeDnsActivationError); + const error = failure as NativeDnsActivationError; + expect(error.phase).toBe("authorization"); + expect(error.rollbackUncertain).toBe(false); + expect(error.originalCause).toBeInstanceOf( + NativeDnsAdministratorAuthorizationError + ); + expect(error.originalCause).toMatchObject({ + outcome: "authorization-failed", + failure: "rejected", + }); + expect(error.message).toBe( + 'Native DNS activation failed during authorization: macOS administrator authorization did not complete (sudo -v did not accept the administrator password (exit 1)); nothing changed. Run "sudo -v" in this terminal first, then retry.' + ); + expect(log).toEqual(["confirm", SUDO_V]); + await expect(readFile(ws.receiptPath)).rejects.toMatchObject({ + code: "ENOENT", + }); + await expect(readFile(plan.dnsmasqPath)).rejects.toMatchObject({ + code: "ENOENT", + }); +}); + +test("activate: a declined confirmation is denied and never reaches sudo", async () => { + const ws = await workspace(); + const plan = planFor(ws); + const log: string[] = []; + const { authorize } = recordingAuthorizer({ + action: "activate", + log, + accept: false, + }); + await expect( + activateNativeDomainDns({ + dependencies: recordingHost({ + ws, + plan, + authorize, + log, + loopbackAnswer: true, + }), + }) + ).rejects.toMatchObject({ + phase: "authorization", + message: + "Native DNS activation failed during authorization: Native DNS activation was denied", + }); + expect(log).toEqual(["confirm"]); +}); + +test("deactivate: native administrator authorization precedes every privileged and file effect", async () => { + const ws = await workspace(); + const plan = await activeClaim(ws); + const log: string[] = []; + const { authorize, prompts } = recordingAuthorizer({ + action: "deactivate", + log, + }); + const inactive = await deactivateNativeDomainDns({ + dependencies: recordingHost({ + ws, + plan, + authorize, + log, + loopbackAnswer: false, + }), + receiptPath: ws.receiptPath, + }); + expect(inactive).toEqual(plan.inactiveReceipt); + assertAuthorizedFirst(log); + expect(log).toContain("sudo -n resolver remove"); + expect(prompts).toEqual([ + `Deactivate only the owned *.${DOMAIN} DNS claim? This restarts dnsmasq; the existing v4 parent remains unchanged.`, + ]); +}); + +test("deactivate: a failed administrator prompt is authorization-failed, not denied, and changes nothing", async () => { + const ws = await workspace(); + const plan = await activeClaim(ws); + const log: string[] = []; + const { authorize } = recordingAuthorizer({ + action: "deactivate", + log, + administrator: { status: "failed", failure: "timed-out", exitCode: 124 }, + }); + const failure = await deactivateNativeDomainDns({ + dependencies: recordingHost({ + ws, + plan, + authorize, + log, + loopbackAnswer: false, + }), + receiptPath: ws.receiptPath, + }).catch((error: unknown) => error); + expect(failure).toBeInstanceOf(NativeDnsDeactivationError); + const error = failure as NativeDnsDeactivationError; + expect(error.phase).toBe("authorization"); + expect(error.originalCause).toMatchObject({ + outcome: "authorization-failed", + failure: "timed-out", + }); + expect(error.message).not.toContain("denied"); + expect(error.message).toContain( + "the macOS administrator prompt timed out after 120 seconds" + ); + expect(log).toEqual(["confirm", SUDO_V]); + expect(await readFile(ws.receiptPath, "utf8")).toBe( + `${JSON.stringify(plan.activeReceipt)}\n` + ); + expect(await readFile(plan.resolverPath, "utf8")).toBe(plan.resolverContent); + expect(await readFile(plan.dnsmasqPath, "utf8")).toBe(plan.dnsmasqContent); +}); + +test("deactivate: a declined confirmation is denied and never reaches sudo", async () => { + const ws = await workspace(); + const plan = await activeClaim(ws); + const log: string[] = []; + const { authorize } = recordingAuthorizer({ + action: "deactivate", + log, + accept: false, + }); + await expect( + deactivateNativeDomainDns({ + dependencies: recordingHost({ + ws, + plan, + authorize, + log, + loopbackAnswer: false, + }), + receiptPath: ws.receiptPath, + }) + ).rejects.toMatchObject({ + phase: "authorization", + message: + "Native DNS deactivation failed during authorization: Native DNS deactivation was denied", + }); + expect(log).toEqual(["confirm"]); + expect(await readFile(plan.resolverPath, "utf8")).toBe(plan.resolverContent); +}); + +test("without an interactive terminal neither action prompts nor reaches sudo", async () => { + for (const action of ["activate", "deactivate"] as const) { + const log: string[] = []; + const { authorize, prompts } = recordingAuthorizer({ + action, + log, + interactive: false, + }); + expect(await authorize(planFor(await workspace()))).toBe(false); + expect(log).toEqual([]); + expect(prompts).toEqual([]); + } +}); + +test("every administrator failure class throws instead of returning false", async () => { + const failures = [ + { status: "failed", failure: "not-foreground", exitCode: null }, + { status: "failed", failure: "timed-out", exitCode: 124 }, + { status: "failed", failure: "interrupted", exitCode: 130 }, + { status: "failed", failure: "rejected", exitCode: 1 }, + ] as const; + for (const result of failures) { + await expect( + requireNativeAdministratorAuthorization({ + requestAdministrator: async () => result, + }) + ).rejects.toMatchObject({ + name: "NativeDnsAdministratorAuthorizationError", + outcome: "authorization-failed", + failure: result.failure, + }); + } + expect( + await requireNativeAdministratorAuthorization({ + requestAdministrator: async () => AUTHORIZED, + }) + ).toBe(true); +}); From 62a9d124d1146eb62bdcfac6434820eddc352794 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Mon, 12 Oct 2026 22:34:48 -0400 Subject: [PATCH 2/2] test(dns): guard the global dns handlers' authorizer wiring and its defaults tests/global-dns-authorizer-wiring.test.ts checks, on the TypeScript syntax tree of src/commands/global.ts, that handleGlobalDnsActivate and handleGlobalDnsDeactivate each build exactly one authorizer with createNativeDnsAuthorizer({ action: , domain }), pass it to createNativeDnsHostDependencies, and run their flow with those dependencies. The only other authorizer allowed is the decline-only `async () => false` on the deactivate verify-only path. tests/native-domain-dns-authorization.test.ts now also covers the production defaults: the default interactivity check requires a stdin TTY before confirming, and the default confirmation declines an explicitly non-interactive run before sudo. The administrator prompt stays a fake, so no test reaches real sudo. Co-Authored-By: Claude Opus 5.5 --- tests/global-dns-authorizer-wiring.test.ts | 232 ++++++++++++++++++ tests/native-domain-dns-authorization.test.ts | 92 +++++++ 2 files changed, 324 insertions(+) create mode 100644 tests/global-dns-authorizer-wiring.test.ts diff --git a/tests/global-dns-authorizer-wiring.test.ts b/tests/global-dns-authorizer-wiring.test.ts new file mode 100644 index 00000000..e5dfafc6 --- /dev/null +++ b/tests/global-dns-authorizer-wiring.test.ts @@ -0,0 +1,232 @@ +import { expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import ts from "typescript"; + +/* + * Source guard for `hack global dns activate|deactivate`. The handlers need a + * real macOS host, Homebrew dnsmasq and sudo, so no test runs them. This guard + * checks on the TypeScript syntax tree of src/commands/global.ts that each + * handler installs createNativeDnsAuthorizer with its own action, and that the + * flow it runs receives exactly that authorizer. The authorizer and the flows + * are proven behaviorally in tests/native-domain-dns-authorization.test.ts. + * + * Limits: this is a syntax check of one file, not data flow. It trusts the + * imported createNativeDnsAuthorizer and the flow functions by name. + */ + +const GLOBAL_SOURCE = join( + import.meta.dir, + "..", + "src", + "commands", + "global.ts" +); + +const HANDLERS = [ + { + handler: "handleGlobalDnsActivate", + action: "activate", + flow: "activateNativeDomainDns", + }, + { + handler: "handleGlobalDnsDeactivate", + action: "deactivate", + flow: "deactivateNativeDomainDns", + }, +] as const; + +function parseGlobalSource(): ts.SourceFile { + return ts.createSourceFile( + GLOBAL_SOURCE, + readFileSync(GLOBAL_SOURCE, "utf8"), + ts.ScriptTarget.Latest, + true, + ts.ScriptKind.TS + ); +} + +function findFunction( + source: ts.SourceFile, + name: string +): ts.FunctionDeclaration { + const found = source.statements.find( + (statement): statement is ts.FunctionDeclaration => + ts.isFunctionDeclaration(statement) && statement.name?.text === name + ); + if (!found?.body) { + throw new Error(`${name} not found in src/commands/global.ts`); + } + return found; +} + +function collect( + root: ts.Node, + match: (node: ts.Node) => node is T +): T[] { + const out: T[] = []; + const visit = (node: ts.Node): void => { + if (match(node)) { + out.push(node); + } + ts.forEachChild(node, visit); + }; + visit(root); + return out; +} + +function callsNamed(root: ts.Node, name: string): ts.CallExpression[] { + return collect( + root, + (node): node is ts.CallExpression => + ts.isCallExpression(node) && + ts.isIdentifier(node.expression) && + node.expression.text === name + ); +} + +function objectArgument(call: ts.CallExpression): ts.ObjectLiteralExpression { + const [arg] = call.arguments; + if (!(arg && ts.isObjectLiteralExpression(arg))) { + throw new Error("expected an object literal argument"); + } + return arg; +} + +/** The value of `name` in an object literal; shorthand yields the identifier. */ +function propertyValue( + object: ts.ObjectLiteralExpression, + name: string +): ts.Expression | undefined { + for (const property of object.properties) { + if (ts.isShorthandPropertyAssignment(property)) { + if (property.name.text === name) { + return property.name; + } + } else if ( + ts.isPropertyAssignment(property) && + ts.isIdentifier(property.name) && + property.name.text === name + ) { + return property.initializer; + } + } + return; +} + +/** The `const = ` binding a call result is stored in. */ +function constBindingName(call: ts.CallExpression): string | undefined { + const parent = call.parent; + if ( + ts.isVariableDeclaration(parent) && + parent.initializer === call && + ts.isIdentifier(parent.name) && + ts.isVariableDeclarationList(parent.parent) && + (parent.parent.flags & ts.NodeFlags.Const) !== 0 + ) { + return parent.name.text; + } + return; +} + +function isIdentifierNamed( + node: ts.Node | undefined, + name: string | undefined +): boolean { + return ( + node !== undefined && + name !== undefined && + ts.isIdentifier(node) && + node.text === name + ); +} + +/** `async () => false`: the only other authorizer a handler may pass. */ +function isDecliningAuthorizer(node: ts.Node | undefined): boolean { + return ( + node !== undefined && + ts.isArrowFunction(node) && + node.parameters.length === 0 && + node.body.kind === ts.SyntaxKind.FalseKeyword + ); +} + +interface WiringReport { + readonly authorizerActions: readonly string[]; + readonly authorizerDomains: readonly string[]; + readonly authorizerBinding: string | undefined; + readonly flowCalls: number; + readonly flowUsesAuthorizer: boolean; + readonly otherAuthorizers: readonly string[]; +} + +function inspectHandler( + source: ts.SourceFile, + spec: (typeof HANDLERS)[number] +): WiringReport { + const body = findFunction(source, spec.handler).body as ts.Block; + const factoryCalls = callsNamed(body, "createNativeDnsAuthorizer"); + const authorizerActions = factoryCalls.map((call) => { + const action = propertyValue(objectArgument(call), "action"); + return action && ts.isStringLiteral(action) ? action.text : ""; + }); + const authorizerDomains = factoryCalls.map((call) => { + const domain = propertyValue(objectArgument(call), "domain"); + return domain?.getText(source) ?? ""; + }); + const [factoryCall] = factoryCalls; + const authorizerBinding = + factoryCalls.length === 1 && factoryCall + ? constBindingName(factoryCall) + : undefined; + + const hostCalls = callsNamed(body, "createNativeDnsHostDependencies"); + const otherAuthorizers: string[] = []; + const hostBindingsWithAuthorizer = new Set(); + for (const call of hostCalls) { + const authorize = propertyValue(objectArgument(call), "authorize"); + if (isIdentifierNamed(authorize, authorizerBinding)) { + const binding = constBindingName(call); + if (binding) { + hostBindingsWithAuthorizer.add(binding); + } + } else if (!isDecliningAuthorizer(authorize)) { + otherAuthorizers.push(authorize?.getText(source) ?? ""); + } + } + + const flowCalls = callsNamed(body, spec.flow); + const flowUsesAuthorizer = + flowCalls.length > 0 && + flowCalls.every((call) => { + const deps = propertyValue(objectArgument(call), "dependencies"); + return ( + deps !== undefined && + ts.isIdentifier(deps) && + hostBindingsWithAuthorizer.has(deps.text) + ); + }); + + return { + authorizerActions, + authorizerDomains, + authorizerBinding, + flowCalls: flowCalls.length, + flowUsesAuthorizer, + otherAuthorizers, + }; +} + +for (const spec of HANDLERS) { + test(`${spec.handler} runs ${spec.flow} with the native ${spec.action} authorizer`, () => { + const report = inspectHandler(parseGlobalSource(), spec); + expect(report).toEqual({ + authorizerActions: [spec.action], + authorizerDomains: ["domain"], + authorizerBinding: expect.any(String), + flowCalls: 1, + flowUsesAuthorizer: true, + otherAuthorizers: [], + }); + }); +} diff --git a/tests/native-domain-dns-authorization.test.ts b/tests/native-domain-dns-authorization.test.ts index 644a3fc2..d4c974d3 100644 --- a/tests/native-domain-dns-authorization.test.ts +++ b/tests/native-domain-dns-authorization.test.ts @@ -3,6 +3,7 @@ import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { AdministratorAuthorization } from "../src/lib/admin-authorization.ts"; +import { resetNoInteractiveFlagForTests } from "../src/lib/interactivity.ts"; import { activateNativeDomainDns, NativeDnsActivationError, @@ -457,3 +458,94 @@ test("every administrator failure class throws instead of returning false", asyn }) ).toBe(true); }); + +/* + * Production defaults. Only `isInteractive` and `confirm` fall back to the + * defaults here; the administrator prompt stays a fake, so no test reaches + * the real `sudo -v`. + */ + +async function withStdinTty( + isTTY: boolean | undefined, + run: () => Promise +): Promise { + const saved = Object.getOwnPropertyDescriptor(process.stdin, "isTTY"); + Object.defineProperty(process.stdin, "isTTY", { + value: isTTY, + configurable: true, + writable: true, + }); + try { + return await run(); + } finally { + if (saved) { + Object.defineProperty(process.stdin, "isTTY", saved); + } else { + Reflect.deleteProperty(process.stdin, "isTTY"); + } + } +} + +async function withExplicitNonInteractive(run: () => Promise) { + const saved = process.env.HACK_NO_INTERACTIVE; + process.env.HACK_NO_INTERACTIVE = "1"; + resetNoInteractiveFlagForTests(); + try { + return await run(); + } finally { + if (saved === undefined) { + Reflect.deleteProperty(process.env, "HACK_NO_INTERACTIVE"); + } else { + process.env.HACK_NO_INTERACTIVE = saved; + } + } +} + +test("default interactivity check requires a stdin TTY before confirming", async () => { + const plan = planFor(await workspace()); + for (const action of ["activate", "deactivate"] as const) { + for (const isTTY of [undefined, false, true] as const) { + const log: string[] = []; + const authorize = createNativeDnsAuthorizer({ + action, + domain: DOMAIN, + dependencies: { + confirm: async () => { + log.push("confirm"); + return false; + }, + requestAdministrator: async () => { + log.push(SUDO_V); + return AUTHORIZED; + }, + }, + }); + expect(await withStdinTty(isTTY, async () => await authorize(plan))).toBe( + false + ); + expect(log).toEqual(isTTY === true ? ["confirm"] : []); + } + } +}); + +test("default confirmation declines an explicitly non-interactive run before sudo", async () => { + const plan = planFor(await workspace()); + for (const action of ["activate", "deactivate"] as const) { + const log: string[] = []; + const authorize = createNativeDnsAuthorizer({ + action, + domain: DOMAIN, + dependencies: { + isInteractive: () => true, + requestAdministrator: async () => { + log.push(SUDO_V); + return AUTHORIZED; + }, + }, + }); + expect( + await withExplicitNonInteractive(async () => await authorize(plan)) + ).toBe(false); + expect(log).toEqual([]); + } +});