diff --git a/CHANGELOG.md b/CHANGELOG.md index bba3b66..8da11e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,16 @@ One source can now push the same cookies and secrets to several sinks. - `agentcookie status` and `agentcookie doctor` report per-sink push state. - Example: `examples/source-multi-sink.yaml`. +### PP CLI config carriage (#118) + +Auto-discovered Printing Press CLIs were pointed at the env-shaped `[secrets.file]` slot, so their `config.toml` went through a strict `KEY=VALUE` parser that cannot read the TOML they actually write. Every one of them failed to sync, and CLIs that were simply never authenticated were reported as errors on every push. + +- A PP CLI's `config.toml` now rides as a `[[files]]` carriage item. Bytes are carried verbatim, so nested tables and comments survive and nothing is parsed. +- Carriage is gated on the manifest declaring at least one sensitive key, so a preference-only config is not swept in. +- A never-authenticated CLI is skipped quietly instead of erroring; a hand-written manifest naming a missing file still errors, because its author chose the path. +- `agentcookie secret link-configs` bridges carried configs into `~/.config//config.toml`, where the installed fleet actually reads them (only binaries built after roughly 2026-07 honor an env pointer). Read-only planning, dry run until `--apply`. It never replaces an existing config, and the link is confined to `~/.config` so no symlink along the destination path can redirect it out of that tree. +- Measured across 59 installed CLIs: `secrets-bus` error lines 40 to 0, CLIs contributing secrets 2 to 9. + ## [1.0.0] - 2026-08-13 ### Featured: Mac to Linux continuous sync diff --git a/docs/audits/2026-05-22-pp-cli-auth-inventory.md b/docs/audits/2026-05-22-pp-cli-auth-inventory.md index c0c33e1..7181ab5 100644 --- a/docs/audits/2026-05-22-pp-cli-auth-inventory.md +++ b/docs/audits/2026-05-22-pp-cli-auth-inventory.md @@ -247,6 +247,8 @@ The format spec at `docs/spec-agentcookie-secrets-bus-v1.md` was written before 1. **Multi-account namespacing.** Superhuman (account email keys) and Slack (team ID keys) hold one set of secrets per account in a single file. The v1 spec assumes one secret set per CLI. v1.1 needs either an `accounts//secrets.env` subdirectory convention or namespaced keys in a single file. Recommend the subdirectory convention so a friend can opt in/out of syncing specific accounts. 2. **Per-file (not just per-key) `local-only` markers.** Tesla's `snowflake-private.pem` is local-only, but its `snowflake-public.pem` half is safe-to-sync. The v1 spec's `[sync.keys]` is per-key inside `secrets.env`; v1.1 needs a `[sync.files]` table for non-env-shaped artifacts like `.pem` files that live alongside the env file. + + > **Partly addressed 2026-08-13.** v2 `[[files]]` carries non-env-shaped artifacts, and the PP adapter now uses it for `config.toml` rather than the env-shaped `[secrets.file]` slot (see spec §7.4). Carriage is gated on the manifest declaring at least one sensitive key, so preference-only configs are not swept in. The per-file `local-only` marker itself is still open: carriage today is whole-file with no per-key or per-field filter, so a config holding both a credential and something the user would rather not replicate ships wholesale. Companion files (`cookies.json`, `browser-session-proof.json`, Linear's `LINEAR_API_KEY`) are also still uncarried. 3. **Third sync-safety classification: `device-bound-but-shippable`.** The browser-session-proof JSON used by eBay, OpenArt, and Suno is technically device-bound (it captures fingerprint timing) but the threat model would tolerate sync to a single trusted second machine. The v1 spec only has two buckets (safe-to-sync, local-only). v1.1 needs the middle category with a `caution` marker that warns but does not block. PII observation: ordertogo's `config.toml` carries customer name, phone, and Stripe customer ID alongside auth tokens. The bus replicates whatever's in the file, not just "secrets." Friends should know that. This is documentation territory, not a spec change; the v1 spec's security boundary statement may want a paragraph about PII-replication once v1.1 lands. diff --git a/docs/plans/2026-08-13-2206-fix-pp-cli-toml-secrets-carriage-plan.md b/docs/plans/2026-08-13-2206-fix-pp-cli-toml-secrets-carriage-plan.md new file mode 100644 index 0000000..49a72c0 --- /dev/null +++ b/docs/plans/2026-08-13-2206-fix-pp-cli-toml-secrets-carriage-plan.md @@ -0,0 +1,317 @@ +--- +artifact_contract: ce-unified-plan/v1 +artifact_readiness: implementation-ready +execution: code +product_contract_source: ce-plan-bootstrap +type: fix +created: 2026-08-13 +--- + +# fix: Carry PP CLI config.toml as a file instead of parsing it as dotenv + +## Summary + +Every auto-discovered Printing Press CLI fails to sync its secrets. `DeriveManifestFromPP` points `[secrets.file]` — a slot the manifest contract defines as env-shaped — at `~/.config//config.toml`, which is real TOML. The strict dotenv reader rejects it, so roughly a dozen configured CLIs silently ship nothing, and roughly two dozen unconfigured CLIs emit a `file missing` error on every push. + +The fix is at the adapter, not the parser: derive a `[[files]]` carriage item (machinery that already exists and is designed for this) instead of an env read-in-place, gate it on the manifest declaring at least one sensitive key, and stop reporting never-configured CLIs as errors. + +--- + +## Problem Frame + +`agentcookie source --once` prints ~40 `secrets-bus` errors, one per discovered project, in two classes: + +- **Parse failures (~13).** `read ~/.config//config.toml: line 1: whitespace around '=' is not allowed`, or `missing '=' (expected KEY=VALUE)` where the file opens with a `[table]` header. These CLIs have real credentials on disk that never reach the sink. This is silent data loss — the push reports the error but exits having synced nothing for that CLI. +- **Missing files (~26).** `read-in-place file missing: ~/.config//config.toml`. Most of these directories do not exist at all; the CLI has simply never been authenticated. An unconfigured CLI is a normal state, not an error. + +Causal chain: + +1. `Discover` auto-detects PP CLIs from `.printing-press.json` manifests. +2. `DeriveManifestFromPP` (`internal/secretsbus/pp_cli_adapter.go:57-63`) synthesizes `Secrets.File.Path = ~/.config//config.toml`, citing `docs/audits/2026-05-22-pp-cli-auth-inventory.md` for the canonical path. +3. `LoadPayloadWithDiscovery` (`internal/secretsbus/discover_merge.go:48`) reads that path with `parseEnvFile` unconditionally, regardless of extension. +4. `parseEnvFile` (`internal/secretsbus/secretsbus.go:204-214`) is strict dotenv: it rejects any whitespace around `=` and any line without `=`. +5. PP CLIs write canonical TOML there (`base_url = "..."` with spaces; espn opens with `[favorites]`). + +The audit the adapter cites as its source records these files as `toml` format throughout, and at line 143 already names the problem — "structured data the dotenv shape cannot express" — recommending a `[sync.files]` table for "non-env-shaped artifacts" (line 249). The adapter adopted the audit's *path* and did not carry across its *format* finding. + +--- + +## Goal Capsule + +Configured PP CLIs sync their credentials to the sink in a form the CLI can actually consume on arrival; unconfigured CLIs are quiet; preference-only CLIs are not swept in. + +--- + +## Requirements + +- **R1.** A PP CLI with a populated `~/.config//config.toml` must have that config reach the sink. No silent no-op. +- **R2.** A PP CLI that has never been configured (no config dir, or no `config.toml`) must not produce an error line on push. It is skipped quietly. +- **R3.** A PP CLI whose config holds no sensitive fields (espn: "no secrets — favorites list only", audit line 52) must not have its config carried at all. +- **R4.** A carried config must be consumable by the CLI on the sink machine — materialized where that CLI's documented resolution order will find it. +- **R5.** No regression for the v1 bus, or for hand-written v2 manifests that legitimately declare an env-shaped `[secrets.file]`. Those keep today's parse-and-filter behavior, including its error reporting. + +--- + +## Key Technical Decisions + +**KTD1. Fix the adapter to emit `[[files]]` carriage; do not teach `parseEnvFile` to read TOML.** +`ManifestV2SecretsFile` is documented as "points at an env-shaped file the agent reads in place" (`internal/secretsbus/manifest_v2.go:99-103`). Teaching the env reader TOML would violate that contract for every consumer, and would require a lossy flattening rule for nested tables (`[favorites] nba = "..."` has no dotenv spelling) plus type coercion for non-string TOML values. `[[files]]` already exists for this exact case — its doc example is a pp-cli `config.toml` (`manifest_v2.go:56-62`), it carries bytes verbatim, and the sink materializes 0600. Governs R1, R4. + +**KTD2. Gate carriage on the manifest declaring at least one sensitive key.** +Whole-file carriage has no per-key filter, so it bypasses the `sync.default=false` + `ShouldShipKey` policy that the `[secrets.file]` path applies. Without a gate, espn's preferences would ship where today they are correctly dropped. Carry only when `Sync.Keys` contains at least one `true`. Governs R3. + +**KTD3. Demote missing-source to a quiet skip for auto-derived manifests only.** +A derived manifest asserts a *conventional* path that may not exist; a hand-written manifest asserts a path its author explicitly chose, where absence is a real misconfiguration. Distinguish by manifest provenance, not by path. Governs R2, R5. + +**KTD4. Carry key naming: `_CONFIG_TOML`, hyphens folded to underscores.** +`CarryFiles` validates keys with `validKeyName`, so `booking-com-pp-cli` must become `BOOKING_COM_PP_CLI_CONFIG_TOML`. Reuse the existing env-name derivation rather than inventing a second one. + +**KTD5. Materialize inside `~/.agentcookie/` as today, and expose the config to the CLI through an explicit, opt-in sink-side link step.** (session-settled: user-directed — chosen over an env pointer and over writing directly into `~/.config/`: only 2 of 59 installed CLIs can follow an env pointer, and a direct `~/.config/` write would widen the sink's manifest-driven write authority.) + +Measured on this machine (method in Sources): PP CLIs only honor `XDG_CONFIG_HOME` / `_CONFIG_DIR` if built after roughly 2026-07-01. ordertogo (2026-06-24) ignores both; human-goat (2026-07-03) and juneoven (2026-07-10) honor both. **2 of 59 installed binaries qualify.** `--config ` works universally but is set by whoever invokes the CLI, which the bus does not control. + +So an env pointer alone leaves 57 CLIs broken, and writing straight into `~/.config//` would break the containment invariant that `validateMaterializeTarget` exists to enforce (`internal/secretsbus/manifest_v2.go:314-338`) — the sink's defense against a manifest naming an arbitrary write path. The link step keeps every manifest-driven write sandboxed while still reaching all 59 CLIs, and moves the one privileged write into an explicit user action that can refuse to clobber a real config. Governs R4. + +Note the env-name derivation, which the earlier draft had wrong: the config *directory* comes from `cli_name` (`juneoven-pp-cli`) while the env *variable* comes from `api_name` (`JUNEOVEN_CONFIG_DIR`). `JUNEOVEN_PP_CLI_CONFIG_DIR` is ignored. + +--- + +## High-Level Technical Design + +```mermaid +flowchart TD + A[".printing-press.json discovered"] --> B["DeriveManifestFromPP"] + + subgraph current ["Current — broken"] + B --> C["[secrets.file]
path = ~/.config/<cli>/config.toml"] + C --> D["parseEnvFile()"] + D --> E["TOML rejected:
whitespace around '='"] + D --> F["file absent:
'read-in-place file missing'"] + E --> G["nothing synced"] + F --> G + end + + subgraph proposed ["Proposed"] + B --> H{"manifest declares
>=1 sensitive key?"} + H -->|no| I["skip quietly
(espn: preferences only)"] + H -->|yes| J["[[files]] item
source/key/target/env"] + J --> K{"source exists?"} + K -->|no| L["skip quietly
(never configured)"] + K -->|yes| M["CarryFiles: base64 verbatim"] + M --> N["sink materializes 0600
~/.agentcookie/<cli>/config.toml"] + N --> O["explicit opt-in link step
~/.config/<cli>/config.toml -> materialized
(dry-run default, refuses to clobber)"] + end +``` + +The carried bytes are never parsed by the bus. Structure, comments, and types survive the trip intact, which is what makes nested tables and non-string values a non-issue. + +--- + +## Scope Boundaries + +**In scope:** the derivation path for auto-detected PP CLIs, the error reporting for absent derived sources, and the directory-pointer env semantics needed for a carried config to be usable. + +**Out of scope (true non-goals):** +- Teaching `parseEnvFile` any non-dotenv format. +- Changing the v1 bus, or hand-written v2 manifest behavior. +- The sink-side network reliability observed while diagnosing this (unrelated: transport, not format). + +### Deferred to Follow-Up Work +- **Companion files.** The audit records that several CLIs keep auth beside `config.toml` — `cookies.json` (airbnb, ordertogo), `browser-session-proof.json` (ebay, openart, suno), and Linear's `LINEAR_API_KEY` env var. Those need their own `[[files]]` items; this plan carries `config.toml` only. +- **Per-file `local-only` markers** (audit line 249) for artifacts that must never ship. +- **Field-level redaction** for account-identity fields the audit flags with caution (ordertogo `customer_phone`). + +--- + +## Implementation Units + +### U1. Derive `[[files]]` carriage instead of env read-in-place + +**Goal:** A discovered PP CLI produces a manifest that carries its `config.toml` as a file, and preference-only CLIs produce nothing. + +**Requirements:** R1, R3 (KTD1, KTD2, KTD4) + +**Dependencies:** none + +**Files:** +- `internal/secretsbus/pp_cli_adapter.go` +- `internal/secretsbus/pp_cli_adapter_test.go` + +**Approach:** +1. Compute the sensitive-key set from `auth_env_var_specs` (or `auth_env_vars` fallback) exactly as today. +2. If no key is sensitive, return a manifest with neither `Secrets.File` nor `Files` — discovery then contributes nothing for that CLI. +3. Otherwise emit a single `ManifestV2File`: `Source: ~/.config//config.toml`, `Key: _CONFIG_TOML`, `Target: /config.toml`, `Optional: false`, `Env` per U3. +4. Stop setting `Secrets.File` on derived manifests. Keep `Sync` populated — it still describes intent and is read elsewhere. + +**Patterns to follow:** the `[[files]]` example at `internal/secretsbus/manifest_v2.go:56-62`; existing derivation and validation flow in `DeriveManifestFromPP`. + +**Test scenarios:** +- A manifest with `auth_env_var_specs` containing a `sensitive: true` entry yields exactly one `Files` item with the expected source, target, and key, and a nil `Secrets.File`. +- A hyphenated `cli_name` (`booking-com-pp-cli`) yields a carry key that passes `validKeyName`. +- A manifest with only `sensitive: false` specs (espn shape) yields no `Files` item and no `Secrets.File`. +- A manifest using the legacy `auth_env_vars` fallback still yields a carry item, since that path treats all keys as shipped. +- An empty or invalid `cli_name` still errors exactly as today. + +**Verification:** derived manifests for a sensitive CLI and for espn differ as specified, with no `[secrets.file]` on either. + +--- + +### U2. Stop reporting never-configured derived sources as errors + +**Goal:** Unconfigured PP CLIs are skipped silently; hand-written manifests still report a missing declared path. + +**Requirements:** R2, R5 (KTD3) + +**Dependencies:** U1 + +**Files:** +- `internal/secretsbus/discover_merge.go` +- `internal/secretsbus/discovery.go` +- `internal/secretsbus/discover_merge_test.go` + +**Approach:** +1. Mark provenance on the registered project when the manifest was synthesized rather than parsed from disk — a field on the registry entry alongside `Kind`, set where `DeriveManifestFromPP` is called. +2. In `LoadPayloadWithDiscovery`, when a source is absent and the manifest is derived, skip without appending an error. Keep the existing error for parsed manifests. +3. Apply the same rule to the `CarryFiles` "source missing" error, which otherwise reintroduces the identical noise through the carriage path. + +**Approach note:** step 3 is the easy miss — moving to carriage relocates the missing-file report from `discover_merge.go:51` to `filecarriage.go:95`, so fixing only the former leaves the noise in place under a new message. + +**Patterns to follow:** the existing non-fatal error accumulation contract described at `internal/secretsbus/discover_merge.go:18-20`. + +**Test scenarios:** +- A derived project whose source does not exist contributes no error and no payload. +- A hand-written manifest whose declared `[secrets.file]` path is absent still produces the `read-in-place file missing` error. +- A derived project whose source exists but is unreadable (permissions) still errors — absence is quiet, failure is not. +- A push with only unconfigured CLIs returns an empty error slice. + +**Verification:** `agentcookie source --once` on this machine prints no line for any CLI lacking a config dir. + +--- + +### U3. Explicit sink-side link step for carried configs + +**Goal:** On the sink, a user can link carried configs into the locations their CLIs actually read, without the bus itself ever writing outside `~/.agentcookie/`. + +**Requirements:** R4 (KTD5) + +**Dependencies:** U1 (needs carried configs to link) + +**Files:** +- `internal/cli/secret.go` +- `internal/secretsbus/filecarriage.go` +- `internal/secretsbus/filecarriage_test.go` + +**Approach:** +1. Add a sink-side subcommand that walks materialized carried configs under `~/.agentcookie/` and, for each, links `~/.config//config.toml` to the materialized file. +2. Default to a dry run that prints what it would link. Require an explicit flag to act — this is the one privileged write, so it should not happen implicitly. +3. Refuse to overwrite an existing regular file. Only an absent path, or a symlink already pointing into `~/.agentcookie/`, is a safe target. Never follow a symlink out and write through it. +4. Re-validate the slug with the existing v2 slug rules before composing any path, so a malformed slug cannot produce a surprising destination. +5. Leave `validateMaterializeTarget` and the bus write path untouched — the invariant is the point. + +**Execution note:** this unit creates the only code path in the change that writes outside `~/.agentcookie/`. Write the refuse-to-clobber and refuse-to-traverse tests first, and make them fail for the right reason before the happy path exists. + +**Patterns to follow:** existing sink-side command structure in `internal/cli/secret.go`; the containment posture of `validateMaterializeTarget`. + +**Test scenarios:** +- Absent destination: linking creates the symlink and reports it. +- Destination is an existing regular file: refused, unchanged on disk, non-zero exit or explicit error line. +- Destination is a symlink already pointing into `~/.agentcookie/`: re-pointed, treated as owned. +- Destination is a symlink pointing outside `~/.agentcookie/`: refused, and the write does not follow it. +- Dry run is the default: no filesystem mutation without the explicit flag. +- Traversal-bearing or malformed slug: rejected before any path is composed. +- A carried file that is not a `config.toml` is not linked by this command. + +**Verification:** on a machine with carried configs, the dry run lists the expected links; running with the flag makes ` doctor` report a `config_path` resolving to the materialized file; an existing real config is never replaced. + +--- + +### U4. End-to-end coverage over real PP CLI shapes + +**Goal:** The three shapes on this machine are proven end to end. + +**Requirements:** R1, R2, R3 + +**Dependencies:** U1, U2, U3 + +**Files:** +- `internal/secretsbus/discover_merge_files_test.go` + +**Approach:** drive `LoadPayloadWithDiscovery` against a temp home holding three fixtures — a canonical scaffold CLI with sensitive keys and a populated TOML, a preference-only CLI (espn shape, including a `[favorites]` table), and a discovered-but-unconfigured CLI with no config dir. Assert on the merged payload and the error slice together. + +**Execution note:** build fixtures from the shapes recorded in `docs/audits/2026-05-22-pp-cli-auth-inventory.md` rather than copying real user configs. + +**Test scenarios:** +- Canonical CLI: payload carries the base64 config, the target key, and the directory pointer; the decoded bytes are byte-identical to the source, `[table]` headers and comments intact. +- espn shape: contributes nothing, and its `[favorites]` table never appears in the payload. +- Unconfigured CLI: contributes nothing and no error. +- All three together: exactly one CLI in the payload, and an empty error slice. +- v1 precedence is unaffected — a v1 key still wins over a v2 contribution for the same slug. + +**Verification:** the suite fails on `main` and passes after U1-U3. + +--- + +### U5. Update the spec and close the audit finding + +**Goal:** The manifest documentation states how derived PP CLI configs are carried, so the next reader does not re-derive the env-shaped assumption. + +**Requirements:** R1, R4 + +**Dependencies:** U1, U3 + +**Files:** +- `docs/spec-agentcookie-secrets-bus-v2-adoption.md` +- `docs/audits/2026-05-22-pp-cli-auth-inventory.md` + +**Approach:** document PP CLI derivation as file carriage, state the sensitive-key gate, document the directory pointer next to the existing `env` field, and mark the audit's line 143 / line 249 finding as addressed for `config.toml` with companion files still open. + +**Test expectation:** none — documentation only. + +**Verification:** the spec describes the shipped behavior, and no doc still describes derived PP CLIs as env read-in-place. + +--- + +## Risks & Dependencies + +- **Whole-file carriage ships more than the per-key policy would.** A canonical scaffold config carries all seven fields plus extras, where `ShouldShipKey` would have filtered to the sensitive set. KTD2's gate is coarse — it decides whether to carry a file, not which fields. Accepted here because the CLI needs its own config shape to function, but it means a config holding both a secret and something the user would not sync ships wholesale. The audit's per-file `local-only` marker is the real answer; it is deferred. +- **U3 introduces the change's only write outside `~/.agentcookie/`.** It is explicit, opt-in, dry-run by default, and refuses to clobber, but it is still the highest-risk unit here and deserves the closest review. +- **The sensitivity gate is only as good as the PP metadata.** Measured after implementation: a CLI declaring no `auth_env_vars` and no `auth_env_var_specs` (booking-com, ebay) is excluded by KTD2 even though its `config.toml` holds an access token. This is not a regression — those CLIs did not sync before either — but the fix does not reach them. The gate cannot be loosened to catch them, because espn also declares nothing, and metadata alone cannot separate "has no secrets" from "did not say." The fix belongs in those CLIs' `.printing-press.json`, in `cli-printing-press`. +- **57 of 59 installed CLIs remain on stale binaries.** The link step works around that rather than fixing it. Rebuilding those CLIs on a newer Printing Press would make the env-pointer path viable and the link step optional; that is a `cli-printing-press` concern, not this repo's. +- **Sink-side materialization is assumed working.** It is exercised by `filecarriage_test.go` but has not been verified end to end against a live sink in this investigation — the sink was unreachable throughout (unrelated transport problem). +- **Behavior change on quieting errors.** Anyone relying on the `file missing` lines to notice an unconfigured CLI loses that signal. A `--verbose` or debug-level line would preserve it; not specified here. + +--- + +## Open Questions + +- ~~Should a carried config materialize into the sink's real `~/.config//`?~~ **Settled 2026-08-13:** no. The bus keeps its containment invariant; an explicit opt-in link step (U3) bridges the gap. See KTD5 for the measurement that decided it. +- Should the sensitive-key gate be per-file rather than per-manifest, so a CLI with both sensitive and preference files carries only the former? +- Do any discovered CLIs legitimately use an env-shaped `[secrets.file]` today that U1 would regress by removing? Discovery of a real case would make the change conditional on file extension rather than unconditional. + +--- + +## Verification Contract + +- `go build ./...` and `go vet ./...` clean. +- `go test ./internal/secretsbus/...` passes, including U4's new end-to-end coverage. +- `agentcookie source --once` on a machine with these configs prints zero `secrets-bus` lines for unconfigured CLIs and zero parse errors for configured ones. +- A configured CLI appears in the push payload with byte-identical config content. +- Sink-side verification of materialization is required before this is called done, and needs a reachable sink. + +## Definition of Done + +R1-R5 hold; U1-U5 landed; the verification contract passes; the spec no longer describes derived PP CLIs as env read-in-place; deferred items are recorded rather than silently dropped. + +--- + +## Sources & Research + +- `internal/secretsbus/pp_cli_adapter.go:57-63` — the derivation that sets the TOML path on the env-shaped slot. +- `internal/secretsbus/discover_merge.go:47-56` — unconditional `parseEnvFile` on the derived path. +- `internal/secretsbus/secretsbus.go:204-214` — the strict dotenv reader. +- `internal/secretsbus/manifest_v2.go:43-103` — `[[files]]` carriage contract and the env-shaped `[secrets.file]` definition. +- `internal/secretsbus/filecarriage.go:73-121` — carriage, opt-in gate, size cap, companion keys. +- `docs/audits/2026-05-22-pp-cli-auth-inventory.md` — per-CLI format inventory; line 52 (espn has no secrets), line 143 (dotenv cannot express structured data), line 249 (per-file markers for non-env-shaped artifacts). +- `cli-printing-press` `internal/generator/templates/skill.md.tmpl:357-358` — documented PP CLI config resolution order and `_CONFIG_DIR`. Note this documents the *current template*, not what installed binaries do; the probe below is what settled KTD5. +- **Empirical config-resolution probe, 2026-08-13.** Method: write a `config.toml` carrying a sentinel `base_url` into a temp dir, invoke ` doctor` (which reports the resolved `config_path`) under each candidate mechanism, and check which path and value it reports. Results: espn (built 2026-05-07), tesla (05-22), prediction-goat (05-22), ordertogo (06-24) ignore `XDG_CONFIG_HOME`, `_CONFIG_DIR`, and `_CONFIG_DIR`; human-goat (07-03) and juneoven (07-10) honor `XDG_CONFIG_HOME` and `_CONFIG_DIR` but not `_CONFIG_DIR`. `--config ` works on every binary tested. Installed-binary age distribution: 55 built 2026-05, 2 in 06, 2 in 07. +- Observed failure: 40 error lines across 4 runs of `agentcookie source --once` on 2026-08-13, in the two classes described in the Problem Frame. diff --git a/docs/spec-agentcookie-secrets-bus-v2-adoption.md b/docs/spec-agentcookie-secrets-bus-v2-adoption.md index 55471db..560757a 100644 --- a/docs/spec-agentcookie-secrets-bus-v2-adoption.md +++ b/docs/spec-agentcookie-secrets-bus-v2-adoption.md @@ -253,7 +253,8 @@ The discovery loop synthesizes an in-memory v2 manifest from `.printing-press.js | `description` | `.printing-press.json` `description` | | `project_kind` | Always `"cli"` | | `homepage` | Omitted (not present in PP metadata) | -| `[secrets.file].path` | `~/.config//config.toml` (PP CLI canonical location per [PP audit](audits/2026-05-22-pp-cli-auth-inventory.md)) | +| `[secrets.file]` | **Never set.** The PP CLI canonical auth location is TOML, and `[secrets.file]` is env-shaped (§5.1), read by the strict `KEY=VALUE` parser. See §7.4 | +| `[[files]]` | One item, but only when at least one key is sensitive (§7.4): `source = ~/.config//config.toml` (PP CLI canonical location per [PP audit](audits/2026-05-22-pp-cli-auth-inventory.md)), `target = /config.toml`, `optional = false`, `key` = the slug upper-cased with non-alphanumerics folded to `_`, suffixed `_CONFIG_TOML` | | `[sync.keys]` (per key) | For each `auth_env_var_specs[i]` entry: if `sensitive = true`, key is default-shipped; if `sensitive = false`, `[sync.keys]. = false` | ### 7.2 Override @@ -262,7 +263,18 @@ A PP CLI may ship an explicit `agentcookie.toml` (recommended for tier-A integra ### 7.3 Adapter authority -The adapter never reads the actual secrets file. It only synthesizes a manifest pointing at where the secrets live. The read-in-place step at push time is identical to any other v2 manifest. +The adapter never reads the actual secrets file. It only synthesizes a manifest pointing at where the secrets live. The carriage step at push time is identical to any other v2 manifest's `[[files]]` item. + +### 7.4 Why carriage, not read-in-place + +A PP CLI's `config.toml` is TOML: values carry whitespace around `=`, and some CLIs open with a `[table]` header. `[secrets.file]` is env-shaped by contract (§5.1) and is read by the strict `KEY=VALUE` parser, which rejects both. Pointing the adapter at that slot meant every discovered PP CLI failed to sync — configured ones on a parse error, unconfigured ones as a missing file. This is the case §5.4 already anticipated: "a TOML `config.toml` cannot ride as a single `KEY=VALUE` value." + +Two consequences follow from carrying the whole file: + +- **Sensitivity gate.** Whole-file carriage has no per-key filter, so `[sync.keys]` cannot drop anything once the file ships. The adapter therefore emits no `[[files]]` item at all unless at least one declared key is `sensitive = true`. A CLI whose config holds only preferences (espn: a `[favorites]` list, no credentials) carries nothing. +- **Consumption is a separate step.** Carried files materialize under `~/.agentcookie/` (§5.4 invariant), but a PP CLI reads `~/.config//config.toml`. Only binaries built after roughly 2026-07 honor `XDG_CONFIG_HOME` or `_CONFIG_DIR`, so an env pointer does not reach the installed fleet. Rather than widen the bus's write authority, `agentcookie secret link-configs` bridges it as an explicit opt-in step: read-only planning, dry run by default, refusing to replace an existing config or to write through a symlink pointing outside `~/.agentcookie/`. The refusal covers the whole destination path, not just its last component: every lookup and both writes go through an `os.Root` anchored at `~/.config`, so a symlinked `~/.config/` directory cannot redirect the link out of that tree while the plan claims otherwise. `~/.config` is in turn opened through a root anchored at the home directory, so the containment decision and the open are one operation rather than a resolve followed by an open that a symlink planted between them could make disagree. A symlinked `~/.config` itself is honored — the CLI reads through it too — but only while it resolves inside the home directory. + +Note the env-name asymmetry if you do rely on the pointer: the config *directory* derives from `cli_name` (`juneoven-pp-cli`), while the env *variable* derives from `api_name` (`JUNEOVEN_CONFIG_DIR`). ## 8. Discovery semantics diff --git a/internal/cli/discover.go b/internal/cli/discover.go index c99cc82..905eacf 100644 --- a/internal/cli/discover.go +++ b/internal/cli/discover.go @@ -98,7 +98,21 @@ func runDiscover(cmd *cobra.Command, _ []string) error { row := projectToRow(rp) readPath := row.ReadInPlacePath if readPath == "" { - readPath = "(legacy bus dir)" + // A project with no env read-in-place is not automatically a + // legacy bus entry: a manifest that ships its secrets as carried + // files (every auto-detected PP CLI does, since config.toml is + // TOML rather than env-shaped) has no read-in-place path at all. + switch { + case rp.Manifest != nil && len(rp.Manifest.Files) > 0: + readPath = rp.Manifest.Files[0].Source + if extra := len(rp.Manifest.Files) - 1; extra > 0 { + readPath = fmt.Sprintf("%s (+%d more)", readPath, extra) + } + case rp.Kind == secretsbus.SourceKindLegacyV1: + readPath = "(legacy bus dir)" + default: + readPath = "(none)" + } } coverage := row.Coverage if row.Coverage == "MISMATCH" { diff --git a/internal/cli/secret.go b/internal/cli/secret.go index 52184ce..1122641 100644 --- a/internal/cli/secret.go +++ b/internal/cli/secret.go @@ -81,9 +81,73 @@ var secretEnvCmd = &cobra.Command{ RunE: runSecretEnv, } +var secretLinkConfigsApply bool + +var secretLinkConfigsCmd = &cobra.Command{ + Use: "link-configs", + Short: "Link carried CLI configs from ~/.agentcookie/ into ~/.config/ (dry run by default)", + Long: `Carried configs materialize under ~/.agentcookie//config.toml, but a +PP CLI reads ~/.config//config.toml. This links the second to the first. + +The bus deliberately never writes outside ~/.agentcookie/, so this is a +separate, explicit step rather than something a manifest can trigger. + +It prints what it would do and changes nothing unless --apply is passed. An +existing config file is never replaced, and a symlink pointing anywhere other +than ~/.agentcookie/ is refused rather than written through. That applies to +the whole destination path: a symlinked ~/.config/ directory would carry +the link out of ~/.config, so it is refused too.`, + Args: cobra.NoArgs, + RunE: runSecretLinkConfigs, +} + func init() { - secretCmd.AddCommand(secretListCmd, secretGetCmd, secretSetCmd, secretRmCmd, secretImportFromCmd, secretEnvCmd, secretAliasCmd) + secretCmd.AddCommand(secretListCmd, secretGetCmd, secretSetCmd, secretRmCmd, secretImportFromCmd, secretEnvCmd, secretAliasCmd, secretLinkConfigsCmd) secretImportFromCmd.Flags().StringVar(&secretImportAs, "as", "", "cli-name to file the imported secrets under (required)") + secretLinkConfigsCmd.Flags().BoolVar(&secretLinkConfigsApply, "apply", false, "actually create the links (default: dry run)") +} + +func runSecretLinkConfigs(cmd *cobra.Command, _ []string) error { + home, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("resolve home: %w", err) + } + plan, err := secretsbus.PlanConfigLinks(home) + if err != nil { + return err + } + out := cmd.OutOrStdout() + if len(plan) == 0 { + fmt.Fprintln(out, "no carried configs materialized yet; nothing to link") + return nil + } + + for _, e := range plan { + switch e.Action { + case secretsbus.LinkActionLink: + verb := "would link" + if secretLinkConfigsApply { + verb = "linking" + } + fmt.Fprintf(out, " %s %s -> %s\n", verb, e.Destination, e.Materialized) + case secretsbus.LinkActionAlreadyLinked: + fmt.Fprintf(out, " ok %s (already linked)\n", e.Destination) + case secretsbus.LinkActionRefuse: + fmt.Fprintf(out, " SKIP %s: %s\n", e.Destination, e.Reason) + } + } + + if !secretLinkConfigsApply { + fmt.Fprintln(out, "\ndry run; re-run with --apply to create the links") + return nil + } + + applied, errs := secretsbus.ApplyConfigLinks(home, plan) + fmt.Fprintf(out, "\nlinked %d config(s)\n", applied) + for _, e := range errs { + fmt.Fprintf(cmd.ErrOrStderr(), " skipped: %v\n", e) + } + return nil } // secretsRoot resolves to the v1 standard path. Kept as a helper for tests. diff --git a/internal/cli/secret_revoke.go b/internal/cli/secret_revoke.go index 9081ac2..f426cd5 100644 --- a/internal/cli/secret_revoke.go +++ b/internal/cli/secret_revoke.go @@ -75,14 +75,12 @@ To silence the auto-detect, drop a manifest with sync.default = false: schema_version = 2 name = "%s" display_name = "%s (silenced)" - [secrets.file] - path = "%s" [sync] default = false EOF Then re-run `+"`agentcookie discover`"+` to confirm. -`, name, rp.SourcePath, home, name, name, name, rp.ReadInPlacePath) +`, name, rp.SourcePath, home, name, name, name) return nil case secretsbus.SourceKindLegacyV1: diff --git a/internal/secretsbus/discover_merge.go b/internal/secretsbus/discover_merge.go index 3c27ba4..8a1e1fd 100644 --- a/internal/secretsbus/discover_merge.go +++ b/internal/secretsbus/discover_merge.go @@ -1,6 +1,7 @@ package secretsbus import ( + "errors" "fmt" "maps" "os" @@ -69,6 +70,13 @@ func LoadPayloadWithDiscovery(homeDir string) (*Payload, []error) { enabled := LoadEnabledFileKeys(homeDir, slug) carried, carryErrs := CarryFiles(rp.Manifest.Files, enabled, homeDir) for _, e := range carryErrs { + // A derived manifest asserts a conventional path, so an absent + // source just means the CLI was never configured -- a normal + // state, not an error. A hand-written manifest names a path its + // author chose, where absence is a real misconfiguration. + if rp.Kind == SourceKindPPCLIDerived && errors.Is(e, ErrCarrySourceMissing) { + continue + } errs = append(errs, fmt.Errorf("discovered project %q: %w", slug, e)) } maps.Copy(filtered, carried) diff --git a/internal/secretsbus/discover_merge_files_test.go b/internal/secretsbus/discover_merge_files_test.go index 37b7159..b7edc34 100644 --- a/internal/secretsbus/discover_merge_files_test.go +++ b/internal/secretsbus/discover_merge_files_test.go @@ -1,8 +1,10 @@ package secretsbus import ( + "encoding/base64" "os" "path/filepath" + "strings" "testing" ) @@ -53,6 +55,223 @@ env = "DEMO_CONFIG_PATH" } } +// writePPProject creates a discoverable PP CLI project under home. When +// cfgBody is empty the config.toml is deliberately not created, modelling a +// CLI that has been installed but never authenticated. +func writePPProject(t *testing.T, home, slug, cfgBody string) { + t.Helper() + lib := filepath.Join(home, "printing-press", "library", slug) + if err := os.MkdirAll(lib, 0o700); err != nil { + t.Fatal(err) + } + meta := `{"cli_name": "` + slug + `", "auth_env_var_specs": [{"name": "ACCESS_TOKEN", "sensitive": true}]}` + if err := os.WriteFile(filepath.Join(lib, ".printing-press.json"), []byte(meta), 0o600); err != nil { + t.Fatal(err) + } + if cfgBody == "" { + return + } + cfgDir := filepath.Join(home, ".config", slug) + if err := os.MkdirAll(cfgDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(cfgDir, "config.toml"), []byte(cfgBody), 0o600); err != nil { + t.Fatal(err) + } +} + +// An installed-but-never-authenticated CLI is a normal state, not an error. +func TestLoadPayloadWithDiscovery_UnconfiguredDerivedProjectIsQuiet(t *testing.T) { + home := t.TempDir() + writePPProject(t, home, "unconfigured-pp-cli", "") + + p, errs := LoadPayloadWithDiscovery(home) + if len(errs) != 0 { + t.Errorf("unconfigured CLI must not report errors, got: %v", errs) + } + if kv := p.CLIs["unconfigured-pp-cli"]; len(kv) != 0 { + t.Errorf("unconfigured CLI must contribute nothing, got: %v", keysOf(kv)) + } +} + +// A configured CLI still carries, and its bytes survive verbatim. +func TestLoadPayloadWithDiscovery_ConfiguredDerivedProjectCarries(t *testing.T) { + home := t.TempDir() + body := "# comment\n[section]\nkey = \"value\"\n" + writePPProject(t, home, "configured-pp-cli", body) + + p, errs := LoadPayloadWithDiscovery(home) + if len(errs) != 0 { + t.Errorf("unexpected errors: %v", errs) + } + kv := p.CLIs["configured-pp-cli"] + enc := kv["CONFIGURED_PP_CLI_CONFIG_TOML"] + if enc == "" { + t.Fatalf("configured CLI did not carry its config; keys: %v", keysOf(kv)) + } + decoded, err := base64.StdEncoding.DecodeString(enc) + if err != nil { + t.Fatalf("payload is not valid base64: %v", err) + } + if string(decoded) != body { + t.Errorf("carried bytes differ from source.\n got: %q\nwant: %q", decoded, body) + } + if kv[CarryFileKey("CONFIGURED_PP_CLI_CONFIG_TOML")] != "configured-pp-cli/config.toml" { + t.Errorf("missing or wrong target companion: %v", keysOf(kv)) + } +} + +// Only absence is quiet. A derived source that exists but cannot be carried is +// still a real failure and must be reported. +func TestLoadPayloadWithDiscovery_DerivedSourceThatIsNotAFileStillErrors(t *testing.T) { + home := t.TempDir() + writePPProject(t, home, "brokencfg-pp-cli", "") + // Put a directory where the config.toml should be: present, but uncarryable. + if err := os.MkdirAll(filepath.Join(home, ".config", "brokencfg-pp-cli", "config.toml"), 0o700); err != nil { + t.Fatal(err) + } + + _, errs := LoadPayloadWithDiscovery(home) + if len(errs) == 0 { + t.Fatal("a present-but-uncarryable derived source must still error") + } +} + +// Absence is quiet only for derived manifests. A hand-written manifest names a +// path its author chose, so a missing source there is a real misconfiguration. +func TestLoadPayloadWithDiscovery_ExplicitManifestMissingSourceStillErrors(t *testing.T) { + home := t.TempDir() + mdir := filepath.Join(home, ".agentcookie", "manifests") + if err := os.MkdirAll(mdir, 0o700); err != nil { + t.Fatal(err) + } + manifest := ` +schema_version = 2 +name = "explicit-cli" +display_name = "Explicit" + +[[files]] +source = "~/.config/explicit-cli/config.toml" +key = "EXPLICIT_CONFIG" +target = "explicit-cli/config.toml" +` + if err := os.WriteFile(filepath.Join(mdir, "explicit-cli.toml"), []byte(manifest), 0o600); err != nil { + t.Fatal(err) + } + + _, errs := LoadPayloadWithDiscovery(home) + if len(errs) == 0 { + t.Fatal("explicit manifest with a missing declared source must still error") + } +} + +// writePPProjectSpecs is writePPProject with control over the declared specs, +// for shapes that are not the canonical "one sensitive credential" case. +func writePPProjectSpecs(t *testing.T, home, slug, specs, cfgBody string) { + t.Helper() + lib := filepath.Join(home, "printing-press", "library", slug) + if err := os.MkdirAll(lib, 0o700); err != nil { + t.Fatal(err) + } + meta := `{"cli_name": "` + slug + `", "auth_env_var_specs": ` + specs + `}` + if err := os.WriteFile(filepath.Join(lib, ".printing-press.json"), []byte(meta), 0o600); err != nil { + t.Fatal(err) + } + if cfgBody == "" { + return + } + cfgDir := filepath.Join(home, ".config", slug) + if err := os.MkdirAll(cfgDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(cfgDir, "config.toml"), []byte(cfgBody), 0o600); err != nil { + t.Fatal(err) + } +} + +// The three shapes that actually exist on disk, per +// docs/audits/2026-05-22-pp-cli-auth-inventory.md, discovered together: +// a canonical credential-bearing scaffold, a preference-only CLI, and one that +// was installed but never authenticated. +func TestLoadPayloadWithDiscovery_RealPPCLIShapes(t *testing.T) { + home := t.TempDir() + + scaffold := "base_url = \"https://api.example.com\"\naccess_token = \"tok\"\n" + writePPProjectSpecs(t, home, "canonical-pp-cli", + `[{"name": "ACCESS_TOKEN", "sensitive": true}, {"name": "BASE_URL", "sensitive": false}]`, + scaffold) + + // espn: preferences only, including a nested table the dotenv grammar + // could never have expressed. + writePPProjectSpecs(t, home, "espn-pp-cli", + `[{"name": "BASE_URL", "sensitive": false}]`, + "[favorites]\nnba = \"lakers\"\n") + + writePPProjectSpecs(t, home, "neverauthed-pp-cli", + `[{"name": "ACCESS_TOKEN", "sensitive": true}]`, "") + + p, errs := LoadPayloadWithDiscovery(home) + if len(errs) != 0 { + t.Errorf("no shape here is an error condition, got: %v", errs) + } + + // Exactly one CLI contributes. + if len(p.CLIs) != 1 { + t.Fatalf("want exactly 1 contributing CLI, got %d: %v", len(p.CLIs), mapKeys(p.CLIs)) + } + kv, ok := p.CLIs["canonical-pp-cli"] + if !ok { + t.Fatalf("canonical CLI missing; got %v", mapKeys(p.CLIs)) + } + + // Its bytes survive verbatim. + decoded, err := base64.StdEncoding.DecodeString(kv["CANONICAL_PP_CLI_CONFIG_TOML"]) + if err != nil { + t.Fatalf("payload not base64: %v", err) + } + if string(decoded) != scaffold { + t.Errorf("carried bytes differ.\n got: %q\nwant: %q", decoded, scaffold) + } + + // espn's preferences never enter the payload in any form. + for cli, m := range p.CLIs { + for k, v := range m { + if strings.Contains(v, "lakers") || strings.Contains(k, "ESPN") { + t.Errorf("preference-only CLI leaked into payload: %s/%s", cli, k) + } + } + } +} + +// v1 precedence is unchanged by carriage: a v1 key still wins per section 10.3. +func TestLoadPayloadWithDiscovery_V1StillWinsOverCarriedKey(t *testing.T) { + home := t.TempDir() + writePPProject(t, home, "dup-pp-cli", "x = 1\n") + + // A v1 bus entry for the same slug, holding the same key name. + v1dir := filepath.Join(SecretsRoot(home), "dup-pp-cli") + if err := os.MkdirAll(v1dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(v1dir, "secrets.env"), + []byte("DUP_PP_CLI_CONFIG_TOML=v1-wins\n"), 0o600); err != nil { + t.Fatal(err) + } + + p, _ := LoadPayloadWithDiscovery(home) + if got := p.CLIs["dup-pp-cli"]["DUP_PP_CLI_CONFIG_TOML"]; got != "v1-wins" { + t.Errorf("v1 must win per section 10.3, got %q", got) + } +} + +func mapKeys(m map[string]map[string]string) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + return out +} + func keysOf(m map[string]string) []string { out := make([]string, 0, len(m)) for k := range m { diff --git a/internal/secretsbus/filecarriage.go b/internal/secretsbus/filecarriage.go index 8f6409f..d99bc4d 100644 --- a/internal/secretsbus/filecarriage.go +++ b/internal/secretsbus/filecarriage.go @@ -3,6 +3,7 @@ package secretsbus import ( "bufio" "encoding/base64" + "errors" "fmt" "os" "path/filepath" @@ -45,6 +46,13 @@ func CarryFileEnvKey(payloadKey string) string { return fileEnvKeyPrefix + payloadKey } +// ErrCarrySourceMissing marks a carry failure caused solely by the source file +// not existing, as distinct from a source that exists but cannot be read. A +// caller that synthesized the source path by convention (rather than reading it +// from a hand-written manifest) can treat this as a quiet skip; see +// LoadPayloadWithDiscovery. +var ErrCarrySourceMissing = errors.New("source missing") + // maxCarriedFileBytes caps a single carried file at 256 KB (decoded), matching // the v1 secrets.env size cap. Oversized payloads are refused, not written, so // a runaway file cannot swamp the sink. @@ -92,7 +100,7 @@ func CarryFiles(files []ManifestV2File, enabled map[string]bool, homeDir string) info, err := os.Stat(src) if err != nil { if os.IsNotExist(err) { - errs = append(errs, fmt.Errorf("file item %q: source missing: %s", f.Key, src)) + errs = append(errs, fmt.Errorf("file item %q: %w: %s", f.Key, ErrCarrySourceMissing, src)) } else { errs = append(errs, fmt.Errorf("file item %q: stat %s: %w", f.Key, src, err)) } diff --git a/internal/secretsbus/linkconfigs.go b/internal/secretsbus/linkconfigs.go new file mode 100644 index 0000000..595d1ad --- /dev/null +++ b/internal/secretsbus/linkconfigs.go @@ -0,0 +1,400 @@ +package secretsbus + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" +) + +// Config linking bridges a gap between where the bus is allowed to write and +// where a PP CLI actually reads. +// +// The bus materializes carried files strictly under ~/.agentcookie/ -- +// validateMaterializeTarget enforces that, and the sink re-applies it, so a +// manifest can never name an arbitrary write path. But a PP CLI reads its +// config from ~/.config//config.toml, and only binaries built after +// roughly 2026-07 honor XDG_CONFIG_HOME or _CONFIG_DIR, so an env pointer +// reaches almost none of the installed fleet. +// +// Rather than widen the bus's write authority, linking is a separate, explicit, +// opt-in step: it plans in read-only mode, and only acts when the caller asks. +// It is the one place in the system that writes outside ~/.agentcookie/, so it +// refuses anything it does not positively recognize as safe. +// +// "Safe" is a statement about the whole destination path, not just its last +// component. Inspecting only ~/.config//config.toml is not enough: that +// leaf reads as absent just as readily when ~/.config/ is a symlink into +// some other tree, and the mkdir and symlink that follow would walk that +// symlink and leave the config there while the plan claimed ~/.config. So +// every lookup and every write goes through an os.Root anchored at ~/.config, +// which refuses to follow a symlink whose target leaves that tree instead of +// quietly redirecting the write. ~/.config is itself opened through a root +// anchored at the home directory, so the same holds one level up. + +// LinkAction is what a plan entry proposes to do about one destination. +type LinkAction string + +const ( + // LinkActionLink means the destination is absent and can be created. + LinkActionLink LinkAction = "link" + // LinkActionAlreadyLinked means the destination is already a symlink into + // ~/.agentcookie/, so it is ours and already correct. + LinkActionAlreadyLinked LinkAction = "already-linked" + // LinkActionRefuse means the destination is occupied by something we did + // not create. Never overwritten. + LinkActionRefuse LinkAction = "refuse" +) + +// configDirName is the home-relative directory PP CLIs read their configs +// from, and configFileName the file within a CLI's directory there. +const ( + configDirName = ".config" + configFileName = "config.toml" +) + +// reservedBusDirs are ~/.agentcookie/ subdirectories owned by the bus itself +// rather than by a carried CLI config. +var reservedBusDirs = map[string]bool{ + "secrets": true, + "manifests": true, + "file-optin": true, + "cookies": true, + "state": true, + "logs": true, + "tmp": true, + "credentials": true, +} + +// LinkPlanEntry is one proposed link, fully resolved and classified. +type LinkPlanEntry struct { + // Slug is the CLI directory name (also the ~/.config/ directory name). + Slug string + // Materialized is the absolute path of the carried config under + // ~/.agentcookie/. + Materialized string + // Destination is the absolute path the CLI reads. + Destination string + // Action is what would happen when applied. + Action LinkAction + // Reason explains a refusal, or is empty when there is nothing to explain. + Reason string +} + +// PlanConfigLinks scans materialized carried configs under ~/.agentcookie/ and +// classifies what linking each into ~/.config//config.toml would do. +// +// It is strictly read-only: it creates no directories and no links, which is +// what makes it safe to run as the default dry run. +func PlanConfigLinks(homeDir string) ([]LinkPlanEntry, error) { + root := agentcookieRoot(homeDir) + entries, err := os.ReadDir(root) + if err != nil { + if os.IsNotExist(err) { + return nil, nil // nothing materialized yet + } + return nil, fmt.Errorf("read %s: %w", root, err) + } + + // Opening the config tree read-only: a nil configRoot with no refusal + // means ~/.config does not exist yet, so every destination under it is + // absent and linkable. Planning must not create it -- that is the dry-run + // guarantee -- so the directory is only made later, by ApplyConfigLinks. + configRoot, refusal := openConfigRoot(homeDir) + if configRoot != nil { + defer configRoot.Close() + } + + var plan []LinkPlanEntry + for _, e := range entries { + if !e.IsDir() { + continue + } + slug := e.Name() + if reservedBusDirs[slug] { + continue + } + // Re-validate the slug before it is used to compose a destination + // path, so a malformed directory name cannot produce a surprising + // write location. + if !validCLIName(slug) { + continue + } + src := filepath.Join(root, slug, configFileName) + info, err := os.Lstat(src) + if err != nil || !info.Mode().IsRegular() { + continue // only a real materialized config is a candidate + } + + dst := filepath.Join(homeDir, configDirName, slug, configFileName) + action, reason := LinkActionLink, "" + switch { + case refusal != "": + action, reason = LinkActionRefuse, refusal + case configRoot != nil: + action, reason = classifyDestination(configRoot, slug, root) + } + plan = append(plan, LinkPlanEntry{ + Slug: slug, + Materialized: src, + Destination: dst, + Action: action, + Reason: reason, + }) + } + return plan, nil +} + +// maxConfigLinkHops bounds the symlink rewriting in openConfigDir so a cycle +// of absolute symlinks cannot spin. +const maxConfigLinkHops = 8 + +// openConfigDir opens ~/.config through homeRoot, so the entire walk is +// confined to the home directory and a symlink leading out of it fails rather +// than redirecting the open. A nil root with an empty refusal means ~/.config +// does not exist. +// +// The alternative -- resolving ~/.config by name and then opening the result +// -- cannot be made safe, because the resolve and the open are separate +// lookups that a symlink planted between them makes disagree. Anchoring at the +// home directory instead means the containment decision and the open are the +// same operation. +// +// os.Root rejects an absolute symlink even when its target is inside the root, +// so `ln -s ~/dotfiles/config ~/.config` -- the normal way to write that +// arrangement, and one the CLI itself reads through -- would otherwise be +// refused. Such a target is rewritten as a home-relative path and the open +// retried, which keeps the resolution inside the same confined walk. +func openConfigDir(homeRoot *os.Root, homeDir string) (*os.Root, string) { + name := configDirName + for range maxConfigLinkHops { + root, err := homeRoot.OpenRoot(name) + if err == nil { + return root, "" + } + if os.IsNotExist(err) { + return nil, "" + } + target, rerr := homeRoot.Readlink(name) + if rerr != nil { + return nil, fmt.Sprintf("cannot open %s: %v", filepath.Join(homeDir, name), err) + } + rel, ok := homeRelative(homeDir, target) + if !ok { + return nil, fmt.Sprintf("%s is a symlink to %s, outside the home directory; not writing through it", filepath.Join(homeDir, name), target) + } + name = rel + } + return nil, fmt.Sprintf("%s resolves through too many symlinks to classify", filepath.Join(homeDir, configDirName)) +} + +// homeRelative rewrites a path inside the home directory as a home-relative +// one. The home directory's own symlinks are resolved as well, because a +// target may be written either way -- on macOS ~ is reached through /var while +// the real path is /private/var. +func homeRelative(homeDir, target string) (string, bool) { + target = filepath.Clean(target) + bases := []string{filepath.Clean(homeDir)} + if resolved, err := filepath.EvalSymlinks(homeDir); err == nil && resolved != bases[0] { + bases = append(bases, resolved) + } + for _, base := range bases { + if target == base || !underRoot(target, base) { + continue + } + rel, err := filepath.Rel(base, target) + if err != nil { + continue + } + return rel, true + } + return "", false +} + +// openConfigRoot opens ~/.config for read-only classification, creating +// nothing. A nil root with an empty refusal means ~/.config is absent, so +// every destination under it is absent too. +func openConfigRoot(homeDir string) (*os.Root, string) { + homeRoot, err := os.OpenRoot(homeDir) + if err != nil { + return nil, fmt.Sprintf("cannot open %s: %v", homeDir, err) + } + defer homeRoot.Close() + return openConfigDir(homeRoot, homeDir) +} + +// openConfigRootForWrite is openConfigRoot plus creating ~/.config when it is +// missing. The mkdir goes through the home root too, so that one directory +// cannot be placed elsewhere either. +// +// Creating is not allowed to become a way around the classification. A mkdir +// that reports the directory already exists means something appeared in the +// window since it was found absent, so the loop classifies ~/.config again +// rather than opening it by name -- opening by name is what would follow a +// symlink planted in exactly that window. +func openConfigRootForWrite(homeDir string) (*os.Root, error) { + homeRoot, err := os.OpenRoot(homeDir) + if err != nil { + return nil, fmt.Errorf("open %s: %w", homeDir, err) + } + defer homeRoot.Close() + + nominal := filepath.Join(homeDir, configDirName) + for created := false; ; created = true { + root, refusal := openConfigDir(homeRoot, homeDir) + if refusal != "" { + return nil, errors.New(refusal) + } + if root != nil { + return root, nil + } + if created { + // Still absent after a mkdir that either succeeded or found + // something there: a dangling symlink, or a racing writer. Refuse + // rather than try harder. + return nil, fmt.Errorf("%s does not resolve to a directory; not linking", nominal) + } + if err := homeRoot.Mkdir(configDirName, 0o700); err != nil && !os.IsExist(err) { + return nil, fmt.Errorf("create %s: %w", nominal, err) + } + } +} + +// classifyDestination decides what may be done with slug's destination without +// touching it. +// +// Every lookup is made through configRoot, so a symlink anywhere in +// /config.toml that leads out of ~/.config produces a containment error +// rather than a path that reads as absent. The leaf itself is inspected with +// Lstat, so a symlink there is read and judged, never followed. +func classifyDestination(configRoot *os.Root, slug, busRoot string) (LinkAction, string) { + rel := filepath.Join(slug, configFileName) + info, err := configRoot.Lstat(rel) + if err != nil { + if os.IsNotExist(err) { + return LinkActionLink, "" + } + return LinkActionRefuse, unreachableDestinationReason(configRoot, slug, err) + } + if info.Mode()&os.ModeSymlink == 0 { + return LinkActionRefuse, "destination is an existing file; not replacing it" + } + target, err := configRoot.Readlink(rel) + if err != nil { + return LinkActionRefuse, fmt.Sprintf("cannot read existing symlink: %v", err) + } + if !filepath.IsAbs(target) { + target = filepath.Join(configRoot.Name(), slug, target) + } + if !underRoot(target, busRoot) { + return LinkActionRefuse, "destination is a symlink to " + target + ", which we did not create" + } + return LinkActionAlreadyLinked, "" +} + +// unreachableDestinationReason explains a containment or lookup error in terms +// the user can act on. os.Root only reports that the path escaped, so the +// offending parent is inspected -- Readlink still works on a symlink whose +// target is outside the root, since reading a link does not follow it. +func unreachableDestinationReason(configRoot *os.Root, slug string, cause error) string { + parent := filepath.Join(configRoot.Name(), slug) + info, err := configRoot.Lstat(slug) + switch { + case err != nil: + return fmt.Sprintf("cannot inspect %s: %v", parent, err) + case info.Mode()&os.ModeSymlink != 0: + target, err := configRoot.Readlink(slug) + if err != nil { + return fmt.Sprintf("%s is a symlink out of %s; not writing through it", parent, configRoot.Name()) + } + return fmt.Sprintf("%s is a symlink to %s, outside %s; not writing through it", parent, target, configRoot.Name()) + case !info.IsDir(): + return fmt.Sprintf("%s is not a directory", parent) + default: + return fmt.Sprintf("cannot inspect destination: %v", cause) + } +} + +// underRoot reports whether p is root or lies beneath it, after cleaning. +func underRoot(p, root string) bool { + cleanP := filepath.Clean(p) + cleanRoot := filepath.Clean(root) + if cleanP == cleanRoot { + return true + } + return strings.HasPrefix(cleanP, cleanRoot+string(filepath.Separator)) +} + +// ApplyConfigLinks acts on a plan, creating only the links its entries marked +// linkable. Entries marked refuse are reported as errors and never written; +// entries already linked are skipped. Returns the number of links created. +// +// A plan entry is treated as a request, not as a decision already made: the +// slug, the materialized source and the destination are all re-checked here. +// A caller can hand over a plan it built itself, and even one produced by +// PlanConfigLinks describes a filesystem that may have changed since. +func ApplyConfigLinks(homeDir string, plan []LinkPlanEntry) (int, []error) { + busRoot := agentcookieRoot(homeDir) + var errs []error + applied := 0 + + // Opened on first use so a plan with nothing to link stays read-only and + // does not create ~/.config as a side effect. + var configRoot *os.Root + defer func() { + if configRoot != nil { + configRoot.Close() + } + }() + + for _, e := range plan { + switch e.Action { + case LinkActionAlreadyLinked: + continue + case LinkActionRefuse: + errs = append(errs, fmt.Errorf("%s: %s (%s)", e.Slug, e.Reason, e.Destination)) + continue + } + if !validCLIName(e.Slug) { + errs = append(errs, fmt.Errorf("%q is not a valid cli name; not linking", e.Slug)) + continue + } + // The link target is the other half of the containment: a symlink is + // only worth creating if it points at a config the bus materialized. + if !underRoot(e.Materialized, busRoot) { + errs = append(errs, fmt.Errorf("%s: source %s is outside %s; not linking", e.Slug, e.Materialized, busRoot)) + continue + } + if configRoot == nil { + root, err := openConfigRootForWrite(homeDir) + if err != nil { + // Nothing can be linked if the config tree itself is not + // usable, so report it once rather than per entry. + return applied, append(errs, err) + } + configRoot = root + } + if action, reason := classifyDestination(configRoot, e.Slug, busRoot); action != LinkActionLink { + if action == LinkActionAlreadyLinked { + continue + } + errs = append(errs, fmt.Errorf("%s: %s (%s)", e.Slug, reason, e.Destination)) + continue + } + // Both writes are confined to ~/.config by configRoot, so the + // classification above is a source of good error messages rather than + // the only thing standing between a swapped-in symlink and a config + // written outside the tree. + if err := configRoot.MkdirAll(e.Slug, 0o700); err != nil { + errs = append(errs, fmt.Errorf("%s: create config dir: %w", e.Slug, err)) + continue + } + if err := configRoot.Symlink(e.Materialized, filepath.Join(e.Slug, configFileName)); err != nil { + errs = append(errs, fmt.Errorf("%s: link: %w", e.Slug, err)) + continue + } + applied++ + } + return applied, errs +} diff --git a/internal/secretsbus/linkconfigs_test.go b/internal/secretsbus/linkconfigs_test.go new file mode 100644 index 0000000..f25262d --- /dev/null +++ b/internal/secretsbus/linkconfigs_test.go @@ -0,0 +1,534 @@ +package secretsbus + +import ( + "os" + "path/filepath" + "strings" + "sync" + "testing" +) + +// materialized writes a carried config where MaterializeFiles would put it. +func materialized(t *testing.T, home, slug string) string { + t.Helper() + dir := filepath.Join(agentcookieRoot(home), slug) + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + p := filepath.Join(dir, "config.toml") + if err := os.WriteFile(p, []byte("carried = true\n"), 0o600); err != nil { + t.Fatal(err) + } + return p +} + +func planFor(t *testing.T, home, slug string) LinkPlanEntry { + t.Helper() + plan, err := PlanConfigLinks(home) + if err != nil { + t.Fatalf("plan: %v", err) + } + for _, e := range plan { + if e.Slug == slug { + return e + } + } + t.Fatalf("no plan entry for %q; plan: %#v", slug, plan) + return LinkPlanEntry{} +} + +func TestPlanConfigLinks_AbsentDestinationIsLinkable(t *testing.T) { + home := t.TempDir() + materialized(t, home, "demo-pp-cli") + + e := planFor(t, home, "demo-pp-cli") + if e.Action != LinkActionLink { + t.Errorf("absent destination should be linkable, got %q (%s)", e.Action, e.Reason) + } +} + +// The user's own config is never replaced. +func TestPlanConfigLinks_ExistingRegularFileRefused(t *testing.T) { + home := t.TempDir() + materialized(t, home, "demo-pp-cli") + dst := filepath.Join(home, ".config", "demo-pp-cli") + if err := os.MkdirAll(dst, 0o700); err != nil { + t.Fatal(err) + } + real := filepath.Join(dst, "config.toml") + if err := os.WriteFile(real, []byte("mine = true\n"), 0o600); err != nil { + t.Fatal(err) + } + + e := planFor(t, home, "demo-pp-cli") + if e.Action != LinkActionRefuse { + t.Fatalf("existing regular file must be refused, got %q", e.Action) + } + + // And applying the plan must leave it byte-identical. + if _, errs := ApplyConfigLinks(home, []LinkPlanEntry{e}); len(errs) == 0 { + t.Error("applying a refused entry should report an error") + } + got, err := os.ReadFile(real) + if err != nil { + t.Fatal(err) + } + if string(got) != "mine = true\n" { + t.Errorf("existing config was modified: %q", got) + } +} + +// A symlink we previously created is ours to re-point. +func TestPlanConfigLinks_OwnedSymlinkIsRelinkable(t *testing.T) { + home := t.TempDir() + src := materialized(t, home, "demo-pp-cli") + dst := filepath.Join(home, ".config", "demo-pp-cli") + if err := os.MkdirAll(dst, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(src, filepath.Join(dst, "config.toml")); err != nil { + t.Fatal(err) + } + + e := planFor(t, home, "demo-pp-cli") + if e.Action != LinkActionAlreadyLinked { + t.Errorf("a symlink into ~/.agentcookie/ is ours, got %q (%s)", e.Action, e.Reason) + } +} + +// A symlink pointing somewhere else is not ours; never write through it. +func TestPlanConfigLinks_ForeignSymlinkRefused(t *testing.T) { + home := t.TempDir() + materialized(t, home, "demo-pp-cli") + outside := filepath.Join(t.TempDir(), "elsewhere.toml") + if err := os.WriteFile(outside, []byte("elsewhere = true\n"), 0o600); err != nil { + t.Fatal(err) + } + dst := filepath.Join(home, ".config", "demo-pp-cli") + if err := os.MkdirAll(dst, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(dst, "config.toml")); err != nil { + t.Fatal(err) + } + + e := planFor(t, home, "demo-pp-cli") + if e.Action != LinkActionRefuse { + t.Fatalf("foreign symlink must be refused, got %q", e.Action) + } + ApplyConfigLinks(home, []LinkPlanEntry{e}) + // The symlink target must not have been written through. + got, err := os.ReadFile(outside) + if err != nil { + t.Fatal(err) + } + if string(got) != "elsewhere = true\n" { + t.Errorf("wrote through a foreign symlink: %q", got) + } +} + +func TestApplyConfigLinks_CreatesWorkingSymlink(t *testing.T) { + home := t.TempDir() + src := materialized(t, home, "demo-pp-cli") + + plan, err := PlanConfigLinks(home) + if err != nil { + t.Fatal(err) + } + applied, errs := ApplyConfigLinks(home, plan) + if len(errs) != 0 { + t.Fatalf("apply: %v", errs) + } + if applied != 1 { + t.Errorf("applied = %d, want 1", applied) + } + dst := filepath.Join(home, ".config", "demo-pp-cli", "config.toml") + got, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("destination not readable: %v", err) + } + if string(got) != "carried = true\n" { + t.Errorf("destination content: %q", got) + } + resolved, err := filepath.EvalSymlinks(dst) + if err != nil { + t.Fatal(err) + } + wantResolved, err := filepath.EvalSymlinks(src) + if err != nil { + t.Fatal(err) + } + if resolved != wantResolved { + t.Errorf("resolved to %q, want %q", resolved, wantResolved) + } +} + +// Planning must never mutate the filesystem -- that is what makes dry-run safe. +func TestPlanConfigLinks_IsReadOnly(t *testing.T) { + home := t.TempDir() + materialized(t, home, "demo-pp-cli") + + if _, err := PlanConfigLinks(home); err != nil { + t.Fatal(err) + } + if _, err := os.Lstat(filepath.Join(home, ".config", "demo-pp-cli", "config.toml")); !os.IsNotExist(err) { + t.Errorf("planning created the destination; it must be read-only (err=%v)", err) + } +} + +// Only carried config.toml files are link candidates. +func TestPlanConfigLinks_IgnoresNonConfigCarriedFiles(t *testing.T) { + home := t.TempDir() + dir := filepath.Join(agentcookieRoot(home), "demo-pp-cli") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "cookies.json"), []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + + plan, err := PlanConfigLinks(home) + if err != nil { + t.Fatal(err) + } + if len(plan) != 0 { + t.Errorf("non-config carried files must not be link candidates: %#v", plan) + } +} + +// A directory whose name is not a valid CLI slug is never turned into a path. +func TestPlanConfigLinks_RejectsInvalidSlug(t *testing.T) { + home := t.TempDir() + dir := filepath.Join(agentcookieRoot(home), "Not A Slug") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "config.toml"), []byte("x = 1\n"), 0o600); err != nil { + t.Fatal(err) + } + + plan, err := PlanConfigLinks(home) + if err != nil { + t.Fatal(err) + } + for _, e := range plan { + if e.Slug == "Not A Slug" { + t.Errorf("invalid slug must not produce a plan entry: %#v", e) + } + } +} + +// configDirSymlink points ~/.config/ at target, creating ~/.config as a +// real directory first so only the CLI's own directory is a symlink. +func configDirSymlink(t *testing.T, home, slug, target string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(home, ".config"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, filepath.Join(home, ".config", slug)); err != nil { + t.Fatal(err) + } +} + +// An absent destination leaf says nothing about its parents. A symlink at +// ~/.config/ would carry the mkdir and the symlink into whatever tree it +// points at, leaving the config outside ~/.config while the plan claimed +// otherwise, so it is refused rather than followed. +func TestPlanConfigLinks_SymlinkedParentDirectoryRefused(t *testing.T) { + home := t.TempDir() + materialized(t, home, "demo-pp-cli") + outside := t.TempDir() + configDirSymlink(t, home, "demo-pp-cli", outside) + + e := planFor(t, home, "demo-pp-cli") + if e.Action != LinkActionRefuse { + t.Fatalf("a symlinked config directory must be refused, got %q (%s)", e.Action, e.Reason) + } + if !strings.Contains(e.Reason, outside) { + t.Errorf("reason should name where the parent points; got %q", e.Reason) + } + + applied, errs := ApplyConfigLinks(home, []LinkPlanEntry{e}) + if applied != 0 { + t.Errorf("applied = %d, want 0", applied) + } + if len(errs) == 0 { + t.Error("applying a refused entry should report an error") + } + if _, err := os.Lstat(filepath.Join(outside, "config.toml")); !os.IsNotExist(err) { + t.Errorf("wrote through the symlinked parent into %s (err=%v)", outside, err) + } +} + +// A relative symlink target is resolved, not pattern-matched: climbing out of +// ~/.config with ../ is the same escape as naming an absolute path. +func TestPlanConfigLinks_ParentSymlinkClimbingOutOfConfigRefused(t *testing.T) { + home := t.TempDir() + materialized(t, home, "demo-pp-cli") + escape := filepath.Join(home, "elsewhere") + if err := os.MkdirAll(escape, 0o700); err != nil { + t.Fatal(err) + } + configDirSymlink(t, home, "demo-pp-cli", filepath.Join("..", "elsewhere")) + + e := planFor(t, home, "demo-pp-cli") + if e.Action != LinkActionRefuse { + t.Fatalf("a parent symlink climbing out of ~/.config must be refused, got %q (%s)", e.Action, e.Reason) + } + ApplyConfigLinks(home, []LinkPlanEntry{e}) + if _, err := os.Lstat(filepath.Join(escape, "config.toml")); !os.IsNotExist(err) { + t.Errorf("wrote into %s (err=%v)", escape, err) + } +} + +// A plan describes a filesystem that may have changed by the time it is +// applied, and the verdict being re-checked here is "absent, safe to create" -- +// precisely the one a symlink swapped in afterwards would exploit. +func TestApplyConfigLinks_RefusesParentSymlinkAppearingAfterPlanning(t *testing.T) { + home := t.TempDir() + materialized(t, home, "demo-pp-cli") + + plan, err := PlanConfigLinks(home) + if err != nil { + t.Fatal(err) + } + if len(plan) != 1 || plan[0].Action != LinkActionLink { + t.Fatalf("expected one linkable entry before the swap, got %#v", plan) + } + + outside := t.TempDir() + configDirSymlink(t, home, "demo-pp-cli", outside) + + applied, errs := ApplyConfigLinks(home, plan) + if applied != 0 { + t.Errorf("applied = %d, want 0", applied) + } + if len(errs) == 0 { + t.Error("expected an error once the parent became a symlink") + } + if _, err := os.Lstat(filepath.Join(outside, "config.toml")); !os.IsNotExist(err) { + t.Errorf("wrote into %s after the parent was swapped for a symlink (err=%v)", outside, err) + } +} + +// ~/.config is an ancestor like any other: pointed out of the home directory, +// it no longer describes where a linked config would land. +func TestPlanConfigLinks_ConfigRootSymlinkedOutsideHomeRefused(t *testing.T) { + home := t.TempDir() + materialized(t, home, "demo-pp-cli") + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(home, ".config")); err != nil { + t.Fatal(err) + } + + e := planFor(t, home, "demo-pp-cli") + if e.Action != LinkActionRefuse { + t.Fatalf("a config root outside the home directory must be refused, got %q (%s)", e.Action, e.Reason) + } + ApplyConfigLinks(home, []LinkPlanEntry{e}) + if _, err := os.Lstat(filepath.Join(outside, "demo-pp-cli")); !os.IsNotExist(err) { + t.Errorf("created a CLI directory in %s (err=%v)", outside, err) + } +} + +// A config root that appears as a symlink only after planning is the same +// escape arriving through the one path that has to create ~/.config. Finding +// it absent must not license opening it by name later. +func TestApplyConfigLinks_RefusesConfigRootSymlinkAppearingAfterPlanning(t *testing.T) { + home := t.TempDir() + materialized(t, home, "demo-pp-cli") + + plan, err := PlanConfigLinks(home) + if err != nil { + t.Fatal(err) + } + if len(plan) != 1 || plan[0].Action != LinkActionLink { + t.Fatalf("expected one linkable entry while ~/.config was absent, got %#v", plan) + } + + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(home, ".config")); err != nil { + t.Fatal(err) + } + + applied, errs := ApplyConfigLinks(home, plan) + if applied != 0 { + t.Errorf("applied = %d, want 0", applied) + } + if len(errs) == 0 { + t.Error("expected an error once ~/.config became a symlink out of the home directory") + } + if entries, err := os.ReadDir(outside); err != nil || len(entries) != 0 { + t.Errorf("wrote into %s: entries=%v err=%v", outside, entries, err) + } +} + +// The window worth covering here -- ~/.config turning into a symlink between +// being found absent and being created -- has no single-threaded state that +// reproduces it, so it is exercised by interleaving instead. The assertion is +// one-directional: nothing correct can ever write outside the home directory, +// so this cannot fail spuriously; it can only fail to catch a regression. +func TestApplyConfigLinks_ConfigRootRaceNeverEscapesHome(t *testing.T) { + const rounds = 2000 + + home := t.TempDir() + src := materialized(t, home, "demo-pp-cli") + outside := t.TempDir() + configDir := filepath.Join(home, ".config") + + plan := []LinkPlanEntry{{ + Slug: "demo-pp-cli", + Materialized: src, + Destination: filepath.Join(configDir, "demo-pp-cli", "config.toml"), + Action: LinkActionLink, + }} + + var flipping sync.WaitGroup + flipping.Go(func() { + for range rounds { + os.RemoveAll(configDir) + os.Symlink(outside, configDir) + os.Remove(configDir) + } + }) + for range rounds { + ApplyConfigLinks(home, plan) + } + flipping.Wait() + + entries, err := os.ReadDir(outside) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Errorf("a link escaped into %s: %v", outside, entries) + } +} + +// Pointing the whole config tree at a dotfiles checkout is a normal +// arrangement, and the CLI reads its config through that symlink too, so +// linking follows it while it stays inside the home directory. Both spellings +// of the symlink have to work: os.Root rejects an absolute target outright, so +// that case is resolved separately from the relative one. +func TestApplyConfigLinks_FollowsConfigRootSymlinkedInsideHome(t *testing.T) { + for _, tc := range []struct { + name string + linkFrom func(home, dotfiles string) string + }{ + {"absolute target", func(_, dotfiles string) string { return dotfiles }}, + {"relative target", func(_, _ string) string { return filepath.Join("dotfiles", "config") }}, + } { + t.Run(tc.name, func(t *testing.T) { + home := t.TempDir() + src := materialized(t, home, "demo-pp-cli") + dotfiles := filepath.Join(home, "dotfiles", "config") + if err := os.MkdirAll(dotfiles, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(tc.linkFrom(home, dotfiles), filepath.Join(home, ".config")); err != nil { + t.Fatal(err) + } + + plan, err := PlanConfigLinks(home) + if err != nil { + t.Fatal(err) + } + applied, errs := ApplyConfigLinks(home, plan) + if len(errs) != 0 { + t.Fatalf("apply: %v", errs) + } + if applied != 1 { + t.Fatalf("applied = %d, want 1", applied) + } + got, err := os.ReadFile(filepath.Join(dotfiles, "demo-pp-cli", "config.toml")) + if err != nil { + t.Fatalf("link not readable through the resolved config root: %v", err) + } + if string(got) != "carried = true\n" { + t.Errorf("linked content: %q", got) + } + resolved, err := filepath.EvalSymlinks(filepath.Join(home, ".config", "demo-pp-cli", "config.toml")) + if err != nil { + t.Fatal(err) + } + wantResolved, err := filepath.EvalSymlinks(src) + if err != nil { + t.Fatal(err) + } + if resolved != wantResolved { + t.Errorf("resolved to %q, want %q", resolved, wantResolved) + } + }) + } +} + +// A plan is a request, not a decision already made: a caller can build one by +// hand, so applying re-checks that the link points at a carried config. +func TestApplyConfigLinks_RefusesSourceOutsideBusRoot(t *testing.T) { + home := t.TempDir() + foreign := filepath.Join(t.TempDir(), "config.toml") + if err := os.WriteFile(foreign, []byte("theirs = true\n"), 0o600); err != nil { + t.Fatal(err) + } + + applied, errs := ApplyConfigLinks(home, []LinkPlanEntry{{ + Slug: "demo-pp-cli", + Materialized: foreign, + Destination: filepath.Join(home, ".config", "demo-pp-cli", "config.toml"), + Action: LinkActionLink, + }}) + if applied != 0 { + t.Errorf("applied = %d, want 0", applied) + } + if len(errs) == 0 { + t.Error("expected an error for a source outside ~/.agentcookie/") + } + if _, err := os.Lstat(filepath.Join(home, ".config", "demo-pp-cli", "config.toml")); !os.IsNotExist(err) { + t.Errorf("linked a source outside ~/.agentcookie/ (err=%v)", err) + } +} + +// The slug composes a write path, so a hand-built plan cannot smuggle +// traversal through it. +func TestApplyConfigLinks_RefusesTraversalSlug(t *testing.T) { + home := t.TempDir() + outside := t.TempDir() + + applied, errs := ApplyConfigLinks(home, []LinkPlanEntry{{ + Slug: filepath.Join("..", "..", filepath.Base(outside)), + Materialized: filepath.Join(agentcookieRoot(home), "x", "config.toml"), + Destination: filepath.Join(outside, "config.toml"), + Action: LinkActionLink, + }}) + if applied != 0 { + t.Errorf("applied = %d, want 0", applied) + } + if len(errs) == 0 { + t.Error("expected an error for a traversal slug") + } + if _, err := os.Lstat(filepath.Join(outside, "config.toml")); !os.IsNotExist(err) { + t.Errorf("wrote into %s (err=%v)", outside, err) + } +} + +// The bus's own directories are not CLI config candidates. +func TestPlanConfigLinks_SkipsReservedBusDirectories(t *testing.T) { + home := t.TempDir() + for _, reserved := range []string{"secrets", "manifests", "file-optin"} { + dir := filepath.Join(agentcookieRoot(home), reserved) + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "config.toml"), []byte("x = 1\n"), 0o600); err != nil { + t.Fatal(err) + } + } + + plan, err := PlanConfigLinks(home) + if err != nil { + t.Fatal(err) + } + if len(plan) != 0 { + t.Errorf("reserved bus directories must be skipped: %#v", plan) + } +} diff --git a/internal/secretsbus/pp_cli_adapter.go b/internal/secretsbus/pp_cli_adapter.go index f5d3c18..6ee2dd0 100644 --- a/internal/secretsbus/pp_cli_adapter.go +++ b/internal/secretsbus/pp_cli_adapter.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "os" + "strings" ) // ppCLIMetadata is the subset of .printing-press.json the adapter cares about. @@ -54,13 +55,10 @@ func DeriveManifestFromPP(jsonPath string) (*ManifestV2, error) { DisplayName: meta.DisplayName, Description: meta.Description, ProjectKind: "cli", - Secrets: ManifestV2Secrets{ - File: &ManifestV2SecretsFile{ - // Canonical PP CLI auth location per the U1 audit: - // docs/audits/2026-05-22-pp-cli-auth-inventory.md. - Path: fmt.Sprintf("~/.config/%s/config.toml", meta.CLIName), - }, - }, + // Deliberately no [secrets.file]: that block is env-shaped and read in + // place by parseEnvFile, but the canonical PP CLI auth location is + // TOML (`key = "value"`, and some CLIs open with a [table] header). + // The config rides as a carried file instead; see below. Sync: ManifestV2Sync{ // Per spec section 7.1: only sensitive=true keys default-ship. // Non-sensitive keys get an explicit false override. @@ -93,5 +91,60 @@ func DeriveManifestFromPP(jsonPath string) (*ManifestV2, error) { } } + // Whole-file carriage has no per-key filter, so the [sync.keys] policy + // cannot drop anything once the file ships. Gate on the manifest declaring + // at least one sensitive key, so a CLI whose config holds only preferences + // (espn: a [favorites] list, no credentials) carries nothing at all. + if !anySensitive(m.Sync.Keys) { + return m, nil + } + carryKey := ppCarryKey(meta.CLIName) + if !validKeyName(carryKey) { + return nil, fmt.Errorf("%s: cli_name %q yields invalid carry key %q", jsonPath, meta.CLIName, carryKey) + } + m.Files = []ManifestV2File{{ + // Canonical PP CLI auth location per the U1 audit: + // docs/audits/2026-05-22-pp-cli-auth-inventory.md. + Source: fmt.Sprintf("~/.config/%s/config.toml", meta.CLIName), + Key: carryKey, + Target: fmt.Sprintf("%s/config.toml", meta.CLIName), + Optional: false, + }} + return m, nil } + +// anySensitive reports whether any key is marked for shipping. +func anySensitive(keys map[string]bool) bool { + for _, ship := range keys { + if ship { + return true + } + } + return false +} + +// ppCarryKey derives the wire key a CLI's config.toml rides under. The slug is +// upper-cased with every non-alphanumeric run folded to a single underscore so +// the result satisfies validKeyName: "booking-com-pp-cli" -> +// "BOOKING_COM_PP_CLI_CONFIG_TOML". This is a bus-internal envelope key; it is +// deliberately not the CLI's own config env var, which derives from api_name +// rather than cli_name and is honored by only a small minority of binaries. +func ppCarryKey(cliName string) string { + var b strings.Builder + prevUnderscore := false + for _, r := range strings.ToUpper(cliName) { + isLetter := r >= 'A' && r <= 'Z' + isDigit := r >= '0' && r <= '9' + if isLetter || isDigit { + b.WriteRune(r) + prevUnderscore = false + continue + } + if !prevUnderscore { + b.WriteByte('_') + prevUnderscore = true + } + } + return strings.Trim(b.String(), "_") + "_CONFIG_TOML" +} diff --git a/internal/secretsbus/pp_cli_adapter_test.go b/internal/secretsbus/pp_cli_adapter_test.go index 0f5271c..eccec1b 100644 --- a/internal/secretsbus/pp_cli_adapter_test.go +++ b/internal/secretsbus/pp_cli_adapter_test.go @@ -38,8 +38,26 @@ func TestDeriveManifestFromPP_TeslaShape(t *testing.T) { if m.ProjectKind != "cli" { t.Errorf("project_kind: %q", m.ProjectKind) } - if m.Secrets.File == nil || m.Secrets.File.Path != "~/.config/tesla-pp-cli/config.toml" { - t.Errorf("[secrets.file].path: %#v", m.Secrets.File) + // config.toml is TOML, not env-shaped, so it rides as a carried file + // rather than a [secrets.file] read-in-place. + if m.Secrets.File != nil { + t.Errorf("[secrets.file] should not be set on a derived manifest: %#v", m.Secrets.File) + } + if len(m.Files) != 1 { + t.Fatalf("want exactly 1 carried file, got %d: %#v", len(m.Files), m.Files) + } + f := m.Files[0] + if f.Source != "~/.config/tesla-pp-cli/config.toml" { + t.Errorf("source: %q", f.Source) + } + if f.Target != "tesla-pp-cli/config.toml" { + t.Errorf("target: %q", f.Target) + } + if f.Key != "TESLA_PP_CLI_CONFIG_TOML" { + t.Errorf("key: %q", f.Key) + } + if f.Optional { + t.Error("derived config carriage must not be opt-in") } v, ok := m.Sync.Keys["TESLA_AUTH_TOKEN"] if !ok || !v { @@ -96,6 +114,74 @@ func TestDeriveManifestFromPP_NoSpecsFallbackToEnvVars(t *testing.T) { } } +func TestDeriveManifestFromPP_HyphenatedSlugYieldsValidCarryKey(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, ".printing-press.json") + body := `{ + "cli_name": "booking-com-pp-cli", + "auth_env_var_specs": [{"name": "ACCESS_TOKEN", "sensitive": true}] + }` + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + m, err := DeriveManifestFromPP(p) + if err != nil { + t.Fatalf("derive: %v", err) + } + if len(m.Files) != 1 { + t.Fatalf("want 1 carried file, got %d", len(m.Files)) + } + if got := m.Files[0].Key; got != "BOOKING_COM_PP_CLI_CONFIG_TOML" { + t.Errorf("key: %q", got) + } + if !validKeyName(m.Files[0].Key) { + t.Errorf("carry key %q must be a valid env key name", m.Files[0].Key) + } +} + +// espn ships preferences only (a [favorites] table, no credentials). The audit +// records it as having no secrets, so nothing should be carried for it. +func TestDeriveManifestFromPP_PreferenceOnlyCarriesNothing(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, ".printing-press.json") + body := `{ + "cli_name": "espn-pp-cli", + "auth_env_var_specs": [ + {"name": "BASE_URL", "sensitive": false}, + {"name": "TIMEOUT", "sensitive": false} + ] + }` + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + m, err := DeriveManifestFromPP(p) + if err != nil { + t.Fatalf("derive: %v", err) + } + if len(m.Files) != 0 { + t.Errorf("preference-only CLI must carry nothing, got %#v", m.Files) + } + if m.Secrets.File != nil { + t.Errorf("preference-only CLI must not declare [secrets.file]: %#v", m.Secrets.File) + } +} + +func TestDeriveManifestFromPP_FallbackEnvVarsStillCarry(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, ".printing-press.json") + body := `{"cli_name": "demo", "auth_env_vars": ["API_KEY"]}` + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + m, err := DeriveManifestFromPP(p) + if err != nil { + t.Fatalf("derive: %v", err) + } + if len(m.Files) != 1 { + t.Fatalf("legacy auth_env_vars treats all keys as shipped, so it must carry: %#v", m.Files) + } +} + func TestDeriveManifestFromPP_MissingCliNameErrors(t *testing.T) { dir := t.TempDir() p := filepath.Join(dir, ".printing-press.json")