diff --git a/.dot-agent-deck.toml b/.dot-agent-deck.toml index c9f9fb44..daf1548b 100644 --- a/.dot-agent-deck.toml +++ b/.dot-agent-deck.toml @@ -30,14 +30,16 @@ Workflow: - After implementation lands: delegate to reviewer and auditor in parallel. Re-delegate findings you agree with to coder (implementation-side) or tester (test-side). - For docs-only changes: delegate to documenter. - Resolve any blocking review/audit findings before moving on. -- Final integration-test gate: before release, delegate the FULL `npm run test:integration` (no pattern, whole suite) to the tester — run once here, not per task during TDD. CLAUDE.md makes this mandatory before any work is complete, and it spins up a Kind cluster. Proceed only if it passes; on failure, re-delegate the fix to coder/tester and re-run until green. -- Before release: summarize what changed end-to-end and STOP until the user confirms. Then delegate to release. +- Final integration-test gate: the full `npm run test:integration` suite runs as the PR's CI (it spins up a Kind cluster; CLAUDE.md makes it mandatory before any work is complete). It runs once, on the PR — not per task during TDD, and not as a separate pre-PR run. The release worker watches that CI and reports the result back. +- Pre-release: summarize what changed end-to-end and STOP until the user confirms. Then delegate /prd-done to release. Release opens the PR, waits for the PR's CI and automated reviews (e.g. CodeRabbit) to settle, and reports back — it does NOT merge. Route any CI failures or review findings it reports to coder (implementation-side) or tester (test-side), then re-engage release to re-check. +- Merge: only on the user's explicit go-ahead do you re-delegate to release to finish /prd-done (merge the PR, close the issue). Never merge without that go-ahead. PRD-driven work: this project tracks active work in prds/. When the user references a PRD by number or name, paste the path (e.g. prds/581-per-request-user-prompts-repo.md) in --task to whichever worker you delegate to. Coordination slash commands you may run yourself (do NOT delegate these): - /prd-next, /prd-update-progress, /prds-get — progress tracking and PRD navigation -- /changelog-fragment — required before any commit with user-facing changes + +The changelog fragment is created inside /prd-done (the release worker's job) — you do not run /changelog-fragment yourself. Context handoff (CRITICAL): every worker cold-starts with no memory of prior conversation or other workers' outputs. Whatever you write in --task is the entire context they have. Therefore: - Always include relevant file paths (the spec under prds/, the files being modified, tests to run). @@ -101,5 +103,11 @@ command = "devbox run agent-medium" description = "Runs the project's release/PR/merge workflow; never modifies code" clear = false prompt_template = """ -Your job is to run the project's release flow using the project's slash commands: /prd-done (branch + PR + merge + issue close), and /changelog-fragment (when a fragment is missing). Do NOT modify source code. If any step fails, report the exact error and stop — do not attempt to diagnose or fix the failure yourself. If the task is missing context (PR title, target branch, release notes path), report that via work-done rather than improvising. +Your job is to run /prd-done to complete the PRD workflow (branch, push, PR, merge, close issue). Do NOT modify source code, and do NOT delegate to other roles. + +If any step fails (failing CI, merge conflict, or a CHANGES_REQUESTED review state), report the exact error and stop — do not diagnose or fix it; the orchestrator will re-delegate to coder/tester. + +After opening the PR, WAIT for all PR processes to finish — CI / GitHub Actions and automated reviews (e.g. CodeRabbit; fall back to `gh` if its MCP is unauthenticated) — then report back: the PR URL, per-check CI conclusions (for any failure, the job/test name + a `gh run view --log-failed` excerpt and a real-regression-vs-flake read; you MAY re-run one clearly-transient job once), and a categorised summary of automated-review findings (severity, file:line, one-line description). Route any decision about findings back through the orchestrator, never the end-user. + +Once the PR is open, CI is green, and reviews have settled, STOP — do NOT merge. Report back via work-done with the PR URL so the orchestrator can pause for user review. Only merge the PR and close the issue when the orchestrator re-delegates with an explicit instruction to continue. """ diff --git a/changelog.d/621.feature.md b/changelog.d/621.feature.md new file mode 100644 index 00000000..a57652ae --- /dev/null +++ b/changelog.d/621.feature.md @@ -0,0 +1,9 @@ +## Per-Request Path, Branch, and Credential for the Prompts Repo Override + +The per-request prompts-repo override now carries a subdirectory, a branch, and a credential, so a secondary skills source no longer has to live at the repository root on `main` behind the server's single git token. Previously the override (`?repo=`) cloned only the repository root of the default branch using the server's `DOT_AI_GIT_TOKEN`, which left repositories that keep skills under a `skills/`-style subdirectory, on a non-default branch, or in a different authentication realm unusable as a source. + +`GET /api/v1/prompts` and `POST /api/v1/prompts/:name` now accept `?path=` and `?branch=` query parameters, and `POST /api/v1/prompts/refresh` accepts `path` and `branch` body fields, so the override can target a subdirectory on any branch. A new `X-Dot-AI-Git-Token` request header authenticates the override clone against its own host and takes precedence over the server's `DOT_AI_GIT_TOKEN` for that request only — letting you pull from a private repository in a separate auth realm without reconfiguring the server. The forwarded token is scoped to the source host (never forwarded across a redirect), never written to logs, error messages, the `source` field, or the cache key, and token-bearing requests are cloned in isolation so a private source is never served from or into the shared cache. + +All three additions are optional and additive: omit `path` for the repository root, omit `branch` for `main`, and omit the header to use the server's environment credential. A request that sends none of them behaves identically to before, so existing deployments see no change. + +See the [Prompts REST API reference](https://devopstoolkit.ai/docs/mcp/ai-engine/api/rest-api) and the [multi-source skills override guide](https://devopstoolkit.ai/docs/mcp/ai-engine/tools/prompts) for parameters, defaults, and precedence. diff --git a/docs/ai-engine/api/rest-api.md b/docs/ai-engine/api/rest-api.md index 3c9292dc..9ab5d7a2 100644 --- a/docs/ai-engine/api/rest-api.md +++ b/docs/ai-engine/api/rest-api.md @@ -247,15 +247,21 @@ curl http://your-ingress-url/api/v1/openapi | jq '.paths' Three REST endpoints expose the shared prompt library. Each one accepts an optional `repo` parameter that, when supplied, fetches prompts from that repository instead of the one configured via `DOT_AI_USER_PROMPTS_REPO`. When `repo` is omitted, behavior is identical to the env-var-configured setup. +The override can additionally carry three **optional, additive** qualifiers — a subdirectory (`path`), a `branch`, and a per-request git credential (the `X-Dot-AI-Git-Token` header). They only apply to a request that already supplies `repo`, and each one defaults to today's behavior when omitted: `path` defaults to the repo root, `branch` defaults to `main`, and the credential defaults to the server's `DOT_AI_GIT_TOKEN`. + +> **Unchanged by default.** A request that supplies no `path`, no `branch`, and no `X-Dot-AI-Git-Token` header behaves byte-identically to v1.21.0 — same clone target (repo root on `main`), same credential (`DOT_AI_GIT_TOKEN`), same response — whether it uses `?repo=` or the env-var-configured repo (no `?repo=`). All three additions are opt-in per request; existing callers see zero change. + See the [Shared Prompt Library](../tools/prompts.md) for the user-facing tool guide. ### Endpoint Summary -| Endpoint | `repo` parameter | -|----------|------------------| -| `POST /api/v1/prompts/refresh` | JSON body: `{ "repo": "" }` | -| `GET /api/v1/prompts` | Query string: `?repo=` | -| `POST /api/v1/prompts/:promptName` | Query string: `?repo=` | +| Endpoint | `repo` / `path` / `branch` placement | Credential | +|----------|--------------------------------------|------------| +| `POST /api/v1/prompts/refresh` | JSON body: `{ "repo": "", "path": "", "branch": "" }` | `X-Dot-AI-Git-Token` header | +| `GET /api/v1/prompts` | Query string: `?repo=&path=&branch=` | `X-Dot-AI-Git-Token` header | +| `POST /api/v1/prompts/:promptName` | Query string: `?repo=&path=&branch=` | `X-Dot-AI-Git-Token` header | + +`path` and `branch` follow the same placement as `repo` on each endpoint (query string for `GET` and `POST /:promptName`, JSON body for `refresh`). The credential **always** travels as the `X-Dot-AI-Git-Token` request header — never in the query string or body. See [Per-request `path`, `branch`, and credential](#per-request-path-branch-and-credential) below. ### The `source` field @@ -271,6 +277,24 @@ Every response from these endpoints includes a `source` field identifying which **Credential scrubbing**: URLs with embedded credentials are scrubbed before being echoed back. `https://user:tok@host/repo` becomes `https://***:***@host/repo`. The transform is deterministic, so the same credentialed URL always produces the same `source` value across requests. +> **`source` is keyed on the repo URL only.** Adding `path`, `branch`, or the `X-Dot-AI-Git-Token` header does **not** change the `source` value for a given repo — it still echoes the (scrubbed) override URL and stays stable per repo. This lets a caller use `source` as a stable skill-tagging key regardless of which subdirectory, branch, or credential it pulled with. + +### Per-request `path`, `branch`, and credential + +These three qualifiers extend a `repo` override so a secondary source can live under a `skills/`-style subdirectory, on a non-default branch, and in a different authentication realm than the server's env-var repo. All three are optional and additive. + +| Qualifier | Placement | Default when omitted | +|-----------|-----------|----------------------| +| `path` | `?path=` (query) or `"path"` (JSON body) | Repo root | +| `branch` | `?branch=` (query) or `"branch"` (JSON body) | `main` | +| Credential | `X-Dot-AI-Git-Token` request header (never query/body) | Server's `DOT_AI_GIT_TOKEN` env credential | + +**Credential precedence.** When the `X-Dot-AI-Git-Token` header is present, the server clones the override repo with that token **for that request only**; it takes precedence over `DOT_AI_GIT_TOKEN`. When the header is absent, the override clone uses the server's `DOT_AI_GIT_TOKEN` exactly as before. The forwarded token is scoped to the host in `repo` (it is not forwarded across a cross-host redirect) and never appears in logs, error messages, the `source` field, or the cache key. + +> **The credential header is inert without `repo`.** A request that sends `X-Dot-AI-Git-Token` but no `?repo=` is unaffected by the header — it is read only to authenticate an override clone. The env-var-configured path never changes behavior based on the header. + +The `path` and `branch` values map onto the same layout an env-var repo uses via `DOT_AI_USER_PROMPTS_PATH` / `DOT_AI_USER_PROMPTS_BRANCH` — they are simply supplied per request instead of via deployment configuration. + ### `POST /api/v1/prompts/refresh` Force-refreshes the prompts cache. Use this when you've pushed new prompts to the repository and don't want to wait for `DOT_AI_USER_PROMPTS_CACHE_TTL` to expire. @@ -280,6 +304,10 @@ Force-refreshes the prompts cache. Use this when you've pushed new prompts to th | Field | Type | Description | |-------|------|-------------| | `repo` | string | Override repository URL (HTTPS). When supplied, bypasses `DOT_AI_USER_PROMPTS_REPO` for this request. | +| `path` | string | Subdirectory within the override repo to load prompts from. Omit for the repo root. Only applies when `repo` is supplied. | +| `branch` | string | Branch of the override repo to clone. Omit for `main`. Only applies when `repo` is supplied. | + +The override credential travels as the `X-Dot-AI-Git-Token` header (see [above](#per-request-path-branch-and-credential)), never as a body field. **Built-in case** (no env-var repo, no override): @@ -348,6 +376,19 @@ curl -s -X POST http://localhost:3456/api/v1/prompts/refresh \ } ``` +> **Illustrative example** (placeholder token, non-runnable as written). The response shape is shown for reference, not captured from a live run. Replace `` with a real token — it is scrubbed from logs and never appears in the response. + +**Subdirectory + branch + per-request credential**: load prompts from a `skills/` subdirectory on a non-default branch of a private repo, authenticating with a request-supplied token: + +```bash +curl -s -X POST http://localhost:3456/api/v1/prompts/refresh \ + -H "Content-Type: application/json" \ + -H "X-Dot-AI-Git-Token: " \ + -d '{"repo":"https://forgejo.example.com/team/skills","path":"skills","branch":"team-skills"}' +``` + +The response has the same shape as the per-request override case above. `source` still echoes the override repo URL (`"https://forgejo.example.com/team/skills"`, credentials scrubbed) — unchanged by `path`, `branch`, or the header — while `promptsLoaded` reflects the prompts found under `skills/` on the `team-skills` branch. The token is used only to clone that host and never appears in the response. + ### `GET /api/v1/prompts` Lists all available prompts (built-in plus user-defined). Returns the prompt names, descriptions, and any declared arguments. @@ -357,6 +398,10 @@ Lists all available prompts (built-in plus user-defined). Returns the prompt nam | Parameter | Description | |-----------|-------------| | `repo` | Override repository URL (HTTPS). When supplied, bypasses `DOT_AI_USER_PROMPTS_REPO` for this request. | +| `path` | Subdirectory within the override repo to load prompts from. Omit for the repo root. Only applies when `repo` is supplied. | +| `branch` | Branch of the override repo to clone. Omit for `main`. Only applies when `repo` is supplied. | + +The override credential travels as the `X-Dot-AI-Git-Token` header (see [above](#per-request-path-branch-and-credential)), never as a query parameter. **Built-in case**: @@ -398,6 +443,17 @@ The `source` field echoes the override URL (credentials scrubbed): } ``` +> **Illustrative example** (placeholder token, non-runnable as written). The response shape is shown for reference, not captured from a live run. Replace `` with a real token — it is scrubbed from logs and never appears in the response. + +**Subdirectory + branch + per-request credential**: list the prompts a private cross-realm source publishes under `skills/` on a non-default branch: + +```bash +curl -s "http://localhost:3456/api/v1/prompts?repo=https://forgejo.example.com/team/skills&path=skills&branch=team-skills" \ + -H "X-Dot-AI-Git-Token: " +``` + +The response shape is identical to the per-request override case above — the `prompts` array carries whatever lives under `skills/` on the `team-skills` branch, and `source` still echoes `"https://forgejo.example.com/team/skills"` (unaffected by `path`, `branch`, or the header). + ### `POST /api/v1/prompts/:promptName` Returns the rendered content (`messages`, optional `files`) of a single prompt by name. @@ -407,6 +463,10 @@ Returns the rendered content (`messages`, optional `files`) of a single prompt b | Parameter | Description | |-----------|-------------| | `repo` | Override repository URL (HTTPS). When supplied, bypasses `DOT_AI_USER_PROMPTS_REPO` for this request. | +| `path` | Subdirectory within the override repo to load prompts from. Omit for the repo root. Only applies when `repo` is supplied. | +| `branch` | Branch of the override repo to clone. Omit for `main`. Only applies when `repo` is supplied. | + +The override credential travels as the `X-Dot-AI-Git-Token` header (see [above](#per-request-path-branch-and-credential)), never as a query parameter or body field. **Request body** (all fields optional): @@ -432,16 +492,30 @@ curl -s -X POST "http://localhost:3456/api/v1/prompts/prd-create?repo=https://gi } ``` -### Validation Rules for `repo` +> **Illustrative example** (placeholder token, non-runnable as written). The response shape is shown for reference, not captured from a live run. Replace `` with a real token — it is scrubbed from logs and never appears in the response. -The server validates the `repo` parameter before performing any clone or pull. Invalid values return HTTP 400 with the standard error envelope. +**Subdirectory + branch + per-request credential**: render a prompt that lives under `skills/` on a non-default branch of a private cross-realm repo: + +```bash +curl -s -X POST "http://localhost:3456/api/v1/prompts/deploy-app?repo=https://forgejo.example.com/team/skills&path=skills&branch=team-skills" \ + -H "Content-Type: application/json" \ + -H "X-Dot-AI-Git-Token: " \ + -d '{}' +``` + +The response carries the rendered `messages` for the prompt resolved under `skills/` on the `team-skills` branch; `source` still echoes `"https://forgejo.example.com/team/skills"`, unchanged by `path`, `branch`, or the header. + +### Validation Rules for `repo`, `path`, and `branch` + +The server validates the `repo`, `path`, and `branch` inputs before performing any clone or pull. Invalid values return HTTP 400 with the standard error envelope. Validation runs **before** the loader is touched, so a rejected override can never corrupt the env-var-configured cache. | Rule | Behavior | |------|----------| -| Scheme must be `http` or `https` | Other schemes (`file://`, `ssh://`, `git://`) are rejected. | -| `subPath` must be a safe relative path | `..` segments, absolute paths, and null bytes are rejected. (subPath is not exposed via REST in this release — the env-var defaults apply.) | -| `branch` must match the git-safe character set | Validated when supplied programmatically. (Not exposed via REST in this release.) | +| Scheme must be `http` or `https` | Other schemes (`file://`, `ssh://`, `git://`) are rejected: `Invalid override repoUrl scheme: ssh: (only http and https are allowed) for ssh://bad`. | +| `path` must be a safe relative path | `..` segments, absolute paths, and null bytes are rejected (`Invalid override subPath: Relative path cannot escape target directory`, `Invalid override subPath: Relative path cannot be absolute`, `Invalid override subPath: contains null byte`). | +| `branch` must match the git-safe character set | Only `[A-Za-z0-9_.\-/]` is allowed; other characters are rejected: `Invalid override branch name: `. | | Credentials in the URL | Never echoed in error messages — scrubbed via `sanitizeUrlForLogging`. | +| `X-Dot-AI-Git-Token` | Never echoed in error messages, logs, the `source` field, or the cache key. A bad/unauthorized token surfaces as a request-scoped clone error, with the token scrubbed. | **Validation-error envelope** (`HTTP 400`): @@ -466,14 +540,32 @@ curl -s "http://localhost:3456/api/v1/prompts?repo=ssh://bad" \ HTTP: 400 ``` +An invalid `path` (or `branch`) fails the same way — a request-scoped `HTTP 400` with the deterministic `VALIDATION_ERROR` message from the table above and the same `meta` envelope shape: + +```bash +curl -s "http://localhost:3456/api/v1/prompts?repo=https://github.com/org/skills&path=../etc" \ + -w "\nHTTP: %{http_code}\n" +``` + +```json +{ + "success": false, + "error": { + "code": "VALIDATION_ERROR", + "message": "Invalid override subPath: Relative path cannot escape target directory" + } +} +HTTP: 400 +``` + A request that fails validation never touches the loader, so the env-var-configured cache cannot be corrupted by a malformed override. ### Server-side caveats for this release The `repo` parameter is the server's contract surface for composing prompts from multiple repositories. Each request still serves exactly one repo; how (and whether) callers compose responses from multiple requests is the caller's concern — see the [DevOps AI Toolkit CLI docs](https://devopstoolkit.ai/docs/cli) for the CLI-side composition flow. -- **Single shared token**: All overrides authenticate with the same `DOT_AI_GIT_TOKEN`. Repos that live on different providers (e.g., GitHub + private GitLab) can't both be authenticated. Per-repo tokens are deferred. -- **Single-slot cache**: The loader caches one repo at a time. Sequential requests against different repos re-clone each time (acceptable cost with `--depth 1` clones, but observable when alternating between repos within the TTL window). +- **Per-request credentials**: Each request may carry its own `X-Dot-AI-Git-Token`, so repos on different providers (e.g., GitHub + private GitLab) can each authenticate with their own token. The header takes precedence over `DOT_AI_GIT_TOKEN` for that request only; absent the header, the server env credential is used as before. (This lifts the single-shared-token limitation from the previous release.) +- **Single-slot cache**: The loader caches one repo at a time. Sequential requests against different repos re-clone each time (acceptable cost with `--depth 1` clones, but observable when alternating between repos within the TTL window). Token-bearing override requests are additionally isolated from the shared unauthenticated cache slot, so an authenticated private clone is never served to a different caller. - **No URL allowlist / SSRF gate**: The endpoint assumes the caller is trusted. Don't expose the override surface to untrusted clients without an upstream gate. For the user-facing summary, see [Shared Prompt Library § Multi-source skills](../tools/prompts.md#multi-source-skills-via-the-per-request-repo-override). diff --git a/docs/ai-engine/tools/prompts.md b/docs/ai-engine/tools/prompts.md index f5a7e7e0..1867a5c4 100644 --- a/docs/ai-engine/tools/prompts.md +++ b/docs/ai-engine/tools/prompts.md @@ -421,16 +421,27 @@ The feature is designed for graceful degradation: When a single `DOT_AI_USER_PROMPTS_REPO` isn't enough — for example, you want org-wide public skills from one repository plus per-team private skills from another — run `dot-ai skills generate --repo ` (see the [CLI docs](https://devopstoolkit.ai/docs/cli) for the canonical reference) to fetch prompts from a specified repository for that invocation only, overriding the env-var default. Run the command multiple times — typically wired up as separate agent hooks, one per source — and the CLI tags each set of generated skills with its source so subsequent runs only wipe their own slice. -Under the hood, each invocation talks to the server once and the server still serves exactly one repository per request; composition lives in the CLI, not the server. +The override carries more than just the repo URL. A secondary source can live wherever it actually is, via three **optional, additive** qualifiers: -When the override is not used, behavior is unchanged: the server falls back to `DOT_AI_USER_PROMPTS_REPO`, or to the built-in prompts when no env-var repo is configured. +| Qualifier | What it does | Default when omitted | +|-----------|--------------|----------------------| +| `path` (subdirectory) | Load prompts from a `skills/`-style subdirectory instead of the repo root — the same layout an env-var repo selects with `DOT_AI_USER_PROMPTS_PATH`. | Repo root | +| `branch` | Pull the source from a non-default branch. | `main` | +| Per-request credential | Authenticate the override clone with a request-supplied git token (the `X-Dot-AI-Git-Token` header), so a second repo in a **different auth realm** (another Forgejo, a private GitHub or GitLab) can be reached without sharing one server-wide token. | Server's `DOT_AI_GIT_TOKEN` | + +**Token precedence**: when a request supplies the credential header, it authenticates that override clone and takes precedence over the server's `DOT_AI_GIT_TOKEN` — but only for that request. Absent the header, the override clone falls back to `DOT_AI_GIT_TOKEN` exactly as before. The token always travels as a request header — never in a URL or body — and never appears in logs, error messages, or the `source` tag. + +Put together, a second source like *"the platform team's private skills, kept under `skills/` on the `team-skills` branch of a self-hosted Forgejo"* is reachable as a single override — a subdirectory, on a non-default branch, in a separate auth realm — alongside your org-wide public source. The CLI tags each source by its repo URL (the `source` value), which is **not** affected by `path`, `branch`, or the credential, so the per-source skill slices stay stable across runs. + +Under the hood, each invocation talks to the server once and the server still serves exactly one repository per request; composition lives in the CLI, not the server. The exact wire placement of each qualifier — `path`/`branch` as query params or JSON body fields, the credential as the `X-Dot-AI-Git-Token` header — is in the [REST API reference](../api/rest-api.md#per-request-path-branch-and-credential). + +> **Unchanged by default.** The `path`, `branch`, and credential qualifiers are all opt-in per request. A request that supplies none of them behaves byte-identically to the previous release — same clone target (repo root on `main`), same `DOT_AI_GIT_TOKEN` credential, same response. And when the override itself is not used, behavior is unchanged: the server falls back to `DOT_AI_USER_PROMPTS_REPO`, or to the built-in prompts when no env-var repo is configured. **Server-side caveats** for this release (the contract is additive, so these can be lifted later without breaking changes): | Caveat | Impact | |--------|--------| -| Single shared `DOT_AI_GIT_TOKEN` | All overrides authenticate with the same token. Repos that live on different providers (e.g., GitHub + private GitLab) can't both be authenticated. | -| Single-slot loader cache | Sequential invocations against different repos re-clone each time. Clones are `--depth 1`, so the cost is small per call, but it's noticeable when alternating between repos within the TTL window. | +| Single-slot loader cache | Sequential invocations against different repos re-clone each time. Clones are `--depth 1`, so the cost is small per call, but it's noticeable when alternating between repos within the TTL window. Token-bearing override requests are additionally isolated from the shared cache slot, so a private authenticated clone is never served to another caller. | | No URL allowlist | The server trusts the override URL. Don't expose this surface to untrusted callers without an upstream gate. | **When NOT to use the override**: diff --git a/mock-server/fixtures/prompts/get-override-path-branch.json b/mock-server/fixtures/prompts/get-override-path-branch.json new file mode 100644 index 00000000..318cf12c --- /dev/null +++ b/mock-server/fixtures/prompts/get-override-path-branch.json @@ -0,0 +1,20 @@ +{ + "success": true, + "data": { + "description": "Skill that lives only under a non-default subdirectory on a non-default branch. Reachable only when BOTH ?path= and ?branch= are supplied with the repo override (PRD #621).", + "messages": [ + { + "role": "user", + "content": { + "type": "text", + "text": "This prompt resolves ONLY when both ?path= and ?branch= are threaded into the repo override (PRD #621). Its presence proves the path/branch parameters were honored by the clone." + } + } + ], + "source": "built-in" + }, + "meta": { + "timestamp": "2024-01-15T10:30:00.000Z", + "version": "1.0.0" + } +} diff --git a/mock-server/fixtures/prompts/list-override-path-branch.json b/mock-server/fixtures/prompts/list-override-path-branch.json new file mode 100644 index 00000000..2f053095 --- /dev/null +++ b/mock-server/fixtures/prompts/list-override-path-branch.json @@ -0,0 +1,23 @@ +{ + "success": true, + "data": { + "prompts": [ + { + "name": "skill-on-branch", + "description": "Skill that lives only under a non-default subdirectory on a non-default branch. Reachable only when BOTH ?path= and ?branch= are supplied with the repo override (PRD #621).", + "arguments": [ + { + "name": "target", + "description": "What to apply the skill to", + "required": false + } + ] + } + ], + "source": "built-in" + }, + "meta": { + "timestamp": "2024-01-15T10:30:00.000Z", + "version": "1.0.0" + } +} diff --git a/mock-server/fixtures/prompts/refresh-override-path-branch.json b/mock-server/fixtures/prompts/refresh-override-path-branch.json new file mode 100644 index 00000000..d196eddb --- /dev/null +++ b/mock-server/fixtures/prompts/refresh-override-path-branch.json @@ -0,0 +1,12 @@ +{ + "success": true, + "data": { + "refreshed": true, + "promptsLoaded": 5, + "source": "built-in" + }, + "meta": { + "timestamp": "2024-01-15T10:30:00.000Z", + "version": "1.0.0" + } +} diff --git a/mock-server/prompts-override.ts b/mock-server/prompts-override.ts index c26d98e2..6becdc95 100644 --- a/mock-server/prompts-override.ts +++ b/mock-server/prompts-override.ts @@ -1,15 +1,32 @@ /** - * Mock-server helpers for PRD #581: per-request user prompts repo override. + * Mock-server helpers for the per-request user prompts repo override. * * The real server validates and clones the override repo. The mock-server only - * needs to mirror the wire contract — specifically, when a `repo` parameter is - * supplied the response's `data.source` echoes that URL verbatim so CLI tagging - * tests can verify the round-trip. + * needs to mirror the wire CONTRACT so the companion CLI (vfarcic/dot-ai-cli) + * can test against it: + * + * PRD #581 — `repo`: + * when a `repo` parameter is supplied the response's `data.source` echoes + * that URL (credentials scrubbed) so CLI tagging tests can verify the + * round-trip. + * + * PRD #621 M5 — `path` / `branch` / token: + * - `?path=` / `?branch=` (query on GET list & POST get-by-name) and `path` + * / `branch` (JSON body on POST /refresh) select a DISTINCT prompt set + * when BOTH are supplied alongside a repo override — mirroring the real + * server resolving a subdir on a non-default branch. + * - invalid `path` (traversal/absolute/null-byte) or `branch` (illegal + * chars) → request-scoped 400 with credentials scrubbed. + * - the credential travels ONLY as the X-Dot-AI-Git-Token header (never + * query/body) and is accepted but NEVER echoed; `source` is unaffected by + * path/branch/token. * * Extracted from server.ts so it can be unit-tested without starting the HTTP * listener. */ +import { posix } from 'node:path'; + export function isPromptsRoutePath(path: string): boolean { return ( path === '/api/v1/prompts' || @@ -18,6 +35,171 @@ export function isPromptsRoutePath(path: string): boolean { ); } +/** + * Query-param names whose values look credential-bearing. Mirrors the real + * server's scrubSourceUrl allowlist (case-insensitive substring match). + */ +const CREDENTIAL_PARAM_RE = /token|key|secret|password|auth|credential/i; + +/** + * Scrub userinfo (https://user:pass@host) from a URL. Mirrors the real + * server's sanitizeUrlForLogging. + */ +function scrubUserinfo(url: string): string { + try { + const parsed = new URL(url); + if (parsed.username) parsed.username = '***'; + if (parsed.password) parsed.password = '***'; + return parsed.toString(); + } catch { + return url.replace(/\/\/[^@]+@/, '//***@'); + } +} + +/** + * Scrub a repo URL before it is echoed into `data.source`. Removes BOTH + * userinfo credentials and credential-bearing query-param values (deterministic + * `***` placeholder, so the scrubbed value is stable per repo — the CLI relies + * on a stable source for skill tagging). Mirrors the real server's + * scrubSourceUrl. A credential-free URL is returned unchanged. + */ +export function scrubRepoUrl(url: string): string { + const userinfoScrubbed = scrubUserinfo(url); + try { + const parsed = new URL(userinfoScrubbed); + let mutated = false; + for (const name of [...parsed.searchParams.keys()]) { + if (CREDENTIAL_PARAM_RE.test(name)) { + parsed.searchParams.set(name, '***'); + mutated = true; + } + } + return mutated ? parsed.toString() : userinfoScrubbed; + } catch { + return userinfoScrubbed; + } +} + +/** + * Scrub credentials embedded in a free-text message. Mirrors the real server's + * git-utils scrubCredentials (used on validation-error messages as + * defense-in-depth). + */ +export function scrubCredentials(message: string): string { + return message + .replace(/\/\/x-access-token:[^@]+@/g, '//***@') + .replace(/\/\/[^/:][^@]*:[^@]+@/g, '//***@'); +} + +/** + * Coerce an override param (repo/path/branch) supplied via query string or JSON + * body: non-strings and empty/whitespace-only values become `undefined` + * (treated as not supplied); otherwise the trimmed value. Mirrors the real + * server's coerceOverrideStringParam emptiness handling. + */ +export function coerceOverrideParam(value: unknown): string | undefined { + if (typeof value !== 'string') return undefined; + const trimmed = value.trim(); + return trimmed.length > 0 ? trimmed : undefined; +} + +/** Branch character set accepted by the real server (isValidGitBranch). */ +const VALID_BRANCH_RE = /^[a-zA-Z0-9_.\-/]+$/; + +/** + * Reject a subPath that escapes the repo root, is absolute, or contains a null + * byte. Mirrors the real server's sanitizeRelativePath. + */ +function assertSafeSubPath(subPath: string): void { + if (subPath.includes('\0')) { + throw new Error('contains null byte'); + } + if (subPath.startsWith('/')) { + throw new Error('Relative path cannot be absolute'); + } + const normalized = posix.normalize(subPath); + if (normalized.startsWith('..') || posix.isAbsolute(normalized)) { + throw new Error('Relative path cannot escape target directory'); + } +} + +export interface PromptsOverrideParams { + repo?: string; + path?: string; + branch?: string; +} + +export type OverrideValidation = + | { ok: true } + | { ok: false; message: string }; + +/** + * Validate the override path/branch. Path/branch only qualify a repo override, + * so without a `repo` they are ignored (no 400) — exactly as the real server's + * extractPromptsOverride drops them when no repo is present. With a repo, an + * invalid path or branch yields a request-scoped 400 message (credentials + * scrubbed). Mirrors getUserPromptsConfigFromOverride's validation. + */ +export function validatePromptsOverride( + params: PromptsOverrideParams +): OverrideValidation { + if (!params.repo) { + return { ok: true }; + } + if (params.path !== undefined) { + try { + assertSafeSubPath(params.path); + } catch (error) { + const detail = error instanceof Error ? error.message : 'invalid'; + return { + ok: false, + message: scrubCredentials(`Invalid override subPath: ${detail}`), + }; + } + } + if (params.branch !== undefined && !VALID_BRANCH_RE.test(params.branch)) { + return { + ok: false, + message: scrubCredentials( + `Invalid override branch name: ${params.branch}` + ), + }; + } + return { ok: true }; +} + +/** + * Fixture serving the DISTINCT prompt set for an override that carries BOTH a + * path and a branch (keyed by route path). Mirrors the real server resolving a + * subdir on a non-default branch — the marker prompt is reachable only when + * both flow through. + */ +const OVERRIDE_PATH_BRANCH_FIXTURES: Record = { + '/api/v1/prompts': 'prompts/list-override-path-branch.json', + '/api/v1/prompts/refresh': 'prompts/refresh-override-path-branch.json', + '/api/v1/prompts/:promptName': 'prompts/get-override-path-branch.json', +}; + +/** + * Return the path+branch override fixture for a prompts route, or undefined if + * the route has no distinct override fixture. + */ +export function getOverridePathBranchFixture( + routePath: string +): string | undefined { + return OVERRIDE_PATH_BRANCH_FIXTURES[routePath]; +} + +/** + * Whether a request resolves the DISTINCT path+branch prompt set: a repo + * override carrying BOTH a (valid) path and branch. + */ +export function selectsOverridePathBranchSet( + params: PromptsOverrideParams +): boolean { + return Boolean(params.repo && params.path && params.branch); +} + export function applyPromptsRepoOverride( fixture: unknown, repo: string | undefined @@ -30,7 +212,9 @@ export function applyPromptsRepoOverride( ...f, data: { ...(f.data as Record), - source: repo, + // `source` echoes the override repo with credentials scrubbed, and is + // unaffected by path/branch/token (PRD #621 M5 contract). + source: scrubRepoUrl(repo), }, }; } diff --git a/mock-server/server.ts b/mock-server/server.ts index b1cbf8d5..d1cccf09 100644 --- a/mock-server/server.ts +++ b/mock-server/server.ts @@ -14,7 +14,11 @@ import { URL, fileURLToPath } from 'node:url'; import { matchRoute, getAllRoutes } from './routes.js'; import { applyPromptsRepoOverride, + coerceOverrideParam, + getOverridePathBranchFixture, isPromptsRoutePath, + selectsOverridePathBranchSet, + validatePromptsOverride, } from './prompts-override.js'; import { isSingleResourceRoutePath, @@ -39,7 +43,12 @@ function setCorsHeaders(res: ServerResponse): void { 'Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS' ); - res.setHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization'); + // PRD #621 M2/M5: advertise the X-Dot-AI-Git-Token credential header so the + // CLI's preflight succeeds, mirroring the real server's CORS allowlist. + res.setHeader( + 'Access-Control-Allow-Headers', + 'Content-Type, Authorization, X-Dot-AI-Git-Token' + ); } /** @@ -177,27 +186,63 @@ async function handleRequest( return; } - // Load and return fixture. For prompts routes, apply the optional `repo` - // override (PRD #581) so the response's `source` field echoes the supplied - // URL — matching the real server's contract that the CLI relies on for - // skill tagging. + // Load and return fixture. For prompts routes, mirror the real server's + // per-request override contract (PRD #581 `repo`; PRD #621 `path`/`branch` + // + X-Dot-AI-Git-Token header). try { const fixture = await loadFixture(route.fixture); let payload: unknown = fixture; if (isPromptsRoutePath(route.path)) { - let repo: string | undefined = url.searchParams.get('repo') || undefined; - if ( - !repo && - method === 'POST' && - route.path === '/api/v1/prompts/refresh' - ) { + // Mirror the real server's override-source contract exactly: + // - POST /api/v1/prompts/refresh → repo/path/branch from the JSON BODY + // ONLY (the real handler reads bodyObj.repo/path/branch; query is not + // consulted for /refresh). + // - GET /api/v1/prompts and POST /api/v1/prompts/:name → from the QUERY. + let repo: string | undefined; + let pathParam: string | undefined; + let branchParam: string | undefined; + if (method === 'POST' && route.path === '/api/v1/prompts/refresh') { const body = await readJsonBody(req); - const bodyRepo = body?.['repo']; - if (typeof bodyRepo === 'string' && bodyRepo.trim()) { - repo = bodyRepo.trim(); + repo = coerceOverrideParam(body?.['repo']); + pathParam = coerceOverrideParam(body?.['path']); + branchParam = coerceOverrideParam(body?.['branch']); + } else { + repo = coerceOverrideParam(url.searchParams.get('repo')); + pathParam = coerceOverrideParam(url.searchParams.get('path')); + branchParam = coerceOverrideParam(url.searchParams.get('branch')); + } + + // The credential travels ONLY as the X-Dot-AI-Git-Token header. The mock + // advertises it via CORS and tolerates it here, but it is intentionally + // never read into — nor reflected by — the response, `source`, or logs. + + // Invalid path/branch (only meaningful with a repo override) → 400 with + // credentials scrubbed, exactly as the real server. + const validation = validatePromptsOverride({ + repo, + path: pathParam, + branch: branchParam, + }); + if (!validation.ok) { + sendJson(res, 400, { + success: false, + error: { code: 'VALIDATION_ERROR', message: validation.message }, + }); + return; + } + + // A repo override carrying BOTH a path and a branch resolves a DISTINCT + // prompt set (mirrors the real server resolving a subdir on a non-default + // branch). Otherwise the default (root@main) fixture is used. + let baseFixture: unknown = fixture; + if (selectsOverridePathBranchSet({ repo, path: pathParam, branch: branchParam })) { + const overrideFixture = getOverridePathBranchFixture(route.path); + if (overrideFixture) { + baseFixture = await loadFixture(overrideFixture); } } - payload = applyPromptsRepoOverride(fixture, repo); + + payload = applyPromptsRepoOverride(baseFixture, repo); } sendJson(res, 200, payload); } catch (error) { diff --git a/prds/621-prompts-override-path-branch-credential.md b/prds/621-prompts-override-path-branch-credential.md index b0faea09..44022ce4 100644 --- a/prds/621-prompts-override-path-branch-credential.md +++ b/prds/621-prompts-override-path-branch-credential.md @@ -1,6 +1,6 @@ # PRD #621: Per-Request Path, Branch, and Credential for the Prompts Repo Override -**Status**: Draft +**Status**: Implementation complete — CI-verified green (run 27431461358; commits 33e3716, 06907a8). M1–M6 done; M7 (companion CLI PRD) drafted, cross-repo filing pending. Mock-image republish pending at release. **Priority**: Important (workarounds exist but are painful) **Related Issues**: #621 (this PRD); #607 / #581 (the override this extends); #575 (multi-realm auth discussion); CLI companion `vfarcic/dot-ai-cli#15` @@ -120,13 +120,13 @@ scan + load (unchanged) Sequenced so the low-risk, high-value path lands first; the credential work (which carries the cache/redirect decisions) follows. -- [ ] **M1 — Path + branch wiring.** `extractPromptsOverride` reads `?path=`/`?branch=` (GET) and `path`/`branch` body fields (POST) into `candidate.subPath`/`candidate.branch`. Existing downstream validation (`sanitizeRelativePath`, `isValidGitBranch`) and cache-key tracking are reused unchanged. Credential scrubbing extended to the new params. -- [ ] **M2 — Credential header.** Read `X-Dot-AI-Git-Token` in the REST handlers; add `gitToken?` to `UserPromptsOverride`; `getUserPromptsConfigFromOverride` sources the token from the override when present, else env. Add the header to both CORS allowlists. -- [ ] **M3 — Clone auth + cache isolation.** `cloneRepo` / auth path accepts a per-call token that overrides `getGitAuthConfigFromEnv()`; token scoped to the source host with no cross-host redirect forwarding (decision 3). Cache isolation for token-bearing override requests (decision 2) — finalize the concrete mechanism here. -- [ ] **M4 — Tests.** Including the **backward-compat parity test** (no-new-param requests unchanged, for both no-`repo` and `?repo=`-only paths); path/branch honored + invalid-value 400s with scrubbing; token precedence over env; token absent from logs/errors/`source`/cache key; cross-host redirect does not leak the token; private authenticated clone not served cross-caller from cache. `npm run test:integration` green. -- [ ] **M5 — Mock-server parity.** `mock-server/routes.ts` and `mock-server/fixtures/prompts/` accept `path`/`branch` (query + body) and the `X-Dot-AI-Git-Token` header, and reflect behavior consistently. Mock image republished via `publish-mock-server` at release time. -- [ ] **M6 — Documentation.** Update `docs/ai-engine/api/rest-api.md` (Prompts Endpoints) and `docs/ai-engine/tools/prompts.md` (multi-source override section) with the new params, the header, defaults, token precedence, and the explicit unchanged-by-default guarantee. Changelog fragment in `changelog.d/` (`621-*.feature.md`). -- [ ] **M7 — Companion CLI PRD handoff.** Create a PRD in `vfarcic/dot-ai-cli` (building on `vfarcic/dot-ai-cli#15`) for the CLI-side `--repo-path`, `--repo-branch`, and `DOT_AI_GIT_TOKEN`-forwarding work. Package the complete server-side contract the CLI needs (see below) so the CLI team can implement and verify against the mock without re-deriving it. +- [x] **M1 — Path + branch wiring.** `extractPromptsOverride` reads `?path=`/`?branch=` (GET) and `path`/`branch` body fields (POST) into `candidate.subPath`/`candidate.branch`. Existing downstream validation (`sanitizeRelativePath`, `isValidGitBranch`) and cache-key tracking are reused unchanged. Credential scrubbing extended to the new params. +- [x] **M2 — Credential header.** Read `X-Dot-AI-Git-Token` in the REST handlers; add `gitToken?` to `UserPromptsOverride`; `getUserPromptsConfigFromOverride` sources the token from the override when present, else env. Add the header to both CORS allowlists. +- [x] **M3 — Clone auth + cache isolation.** `cloneRepo` / auth path accepts a per-call token that overrides `getGitAuthConfigFromEnv()`; token scoped to the source host with no cross-host redirect forwarding (decision 3). Cache isolation for token-bearing override requests (decision 2) — finalize the concrete mechanism here. +- [x] **M4 — Tests.** Including the **backward-compat parity test** (no-new-param requests unchanged, for both no-`repo` and `?repo=`-only paths); path/branch honored + invalid-value 400s with scrubbing; token precedence over env; token absent from logs/errors/`source`/cache key; cross-host redirect does not leak the token; private authenticated clone not served cross-caller from cache. `npm run test:integration` green. +- [x] **M5 — Mock-server parity.** (Code done; mock image republish via `publish-mock-server` pending at release.) `mock-server/routes.ts` and `mock-server/fixtures/prompts/` accept `path`/`branch` (query + body) and the `X-Dot-AI-Git-Token` header, and reflect behavior consistently. Mock image republished via `publish-mock-server` at release time. +- [x] **M6 — Documentation.** (Docs + changelog fragment `changelog.d/621.feature.md` done; optional live-validation of example `promptsLoaded` counts pending.) Update `docs/ai-engine/api/rest-api.md` (Prompts Endpoints) and `docs/ai-engine/tools/prompts.md` (multi-source override section) with the new params, the header, defaults, token precedence, and the explicit unchanged-by-default guarantee. Changelog fragment in `changelog.d/` (`621-*.feature.md`). +- [ ] **M7 — Companion CLI PRD handoff.** (Drafted at `tmp/621-cli-companion-prd.md` with the full server-side contract; cross-repo filing in `vfarcic/dot-ai-cli` pending user confirmation.) Create a PRD in `vfarcic/dot-ai-cli` (building on `vfarcic/dot-ai-cli#15`) for the CLI-side `--repo-path`, `--repo-branch`, and `DOT_AI_GIT_TOKEN`-forwarding work. Package the complete server-side contract the CLI needs (see below) so the CLI team can implement and verify against the mock without re-deriving it. ### M7 — Contract the CLI PRD must carry diff --git a/src/core/git-utils.ts b/src/core/git-utils.ts index fa05bcc0..3351635d 100644 --- a/src/core/git-utils.ts +++ b/src/core/git-utils.ts @@ -13,13 +13,30 @@ */ import simpleGit, { SimpleGitOptions } from 'simple-git'; +import { spawn } from 'node:child_process'; import * as jwt from 'jsonwebtoken'; import * as fs from 'fs'; import * as path from 'path'; +import * as os from 'os'; const FETCH_TIMEOUT_MS = 30000; const GIT_TIMEOUT_MS = 120000; // 2 minutes for git operations +/** + * Environment variable name through which a per-request override credential + * (PRD #621 M3) is handed to the GIT_ASKPASS helper. The token travels in the + * git child process's ENVIRONMENT — never on its argv (ps/proc) and never + * embedded in the clone URL written to `.git/config`. + */ +export const ASKPASS_TOKEN_ENV = 'DOT_AI_GIT_ASKPASS_TOKEN'; + +/** + * Environment variable naming the host the override token is bound to. The + * GIT_ASKPASS helper emits the token ONLY when git's credential prompt names + * this host, so a cross-host HTTP redirect can never obtain it (Decision 3). + */ +export const ASKPASS_HOST_ENV = 'DOT_AI_GIT_ASKPASS_HOST'; + // ─── Auth types ─── export interface GitAuthConfig { @@ -200,6 +217,190 @@ export function sanitizeRelativePath(relativePath: string): string { export interface CloneOptions { branch?: string; depth?: number; + /** + * Per-call git credential (PRD #621 M3). When supplied it OVERRIDES the + * env/GitHub-App auth (`getGitAuthConfigFromEnv`) for this clone only + * (Decision 4) and is scoped to the host in `repoUrl` with cross-host + * redirect forwarding disabled (Decision 3 — see buildOverrideCloneAuth). + * When omitted, the clone uses env auth exactly as before. + */ + token?: string; +} + +/** + * PRD #621 M3 / Decision 3: build the clone URL + intended host for a + * per-request override credential. + * + * The credential itself is NOT in the returned URL — it is the bare + * `x-access-token` username only (the token is passed via a HOST-BOUND + * GIT_ASKPASS helper, see cloneRepo / createAskpassScript). So the token never + * lands on the git argv or in the cloned `.git/config` remote URL (MEDIUM-2/3). + * + * No `-c` git config is returned: the earlier `-c credential.helper=` was + * REJECTED by simple-git's safety guard (allowUnsafeCredentialHelper), which + * aborted the clone entirely; and `-c http.followRedirects=false` is dropped + * per review finding R-1 (it blocked legitimate same-host redirects too). The + * host-bound askpass makes following redirects provably safe — the token is + * emitted ONLY for `host`, and libcurl already strips credentials on a + * cross-host redirect by default. + * + * Returned as plain data so the auth decision is unit-testable without spawning + * git. The token is intentionally NOT a parameter — it never influences this + * (URL/argv) surface. + */ +export function buildOverrideCloneAuth(repoUrl: string): { + cloneUrl: string; + host: string; +} { + const url = new URL(repoUrl); + const host = url.host; + url.username = 'x-access-token'; + url.password = ''; + return { cloneUrl: url.toString(), host }; +} + +/** + * Create a throwaway, HOST-BOUND GIT_ASKPASS helper script. The script holds NO + * secret — it echoes the token from the environment (ASKPASS_TOKEN_ENV) ONLY + * when git's credential prompt (passed as $1) names the intended host + * (ASKPASS_HOST_ENV), delimited by `@`/`//` before and a closing quote after. + * For any other host — e.g. after an HTTP redirect, or a look-alike like + * `github.com.evil.test` — it emits nothing, so the token can never reach a + * different host (Decision 3). The token never touches disk. The script lives + * in its own 0700 temp dir; `cleanup` removes it. + */ +function createAskpassScript(): { scriptPath: string; cleanup: () => void } { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'dot-ai-askpass-')); + try { + fs.chmodSync(dir, 0o700); + } catch { + /* best-effort hardening */ + } + const scriptPath = path.join(dir, 'askpass.sh'); + // Host-bound match: require the intended host immediately after `@` or `//` + // and immediately before the closing `'` git puts around the URL, so neither + // a different redirect host nor a look-alike suffix matches. + const script = [ + '#!/bin/sh', + 'case "$1" in', + ` *"@$${ASKPASS_HOST_ENV}'"*|*"//$${ASKPASS_HOST_ENV}'"*)`, + ` printf '%s\\n' "$${ASKPASS_TOKEN_ENV}"`, + ' ;;', + 'esac', + '', + ].join('\n'); + fs.writeFileSync(scriptPath, script, { mode: 0o700 }); + return { + scriptPath, + cleanup: () => { + try { + fs.rmSync(dir, { recursive: true, force: true }); + } catch { + /* best-effort cleanup; the script holds no secret */ + } + }, + }; +} + +/** + * PRD #621 M3: clone an OVERRIDE repo using a per-request token, via a + * HOST-BOUND GIT_ASKPASS helper. + * + * This deliberately spawns `git` DIRECTLY rather than going through simple-git: + * simple-git's safety scanner rejects the env vars this approach relies on + * (GIT_ASKPASS → allowUnsafeAskPass) and even flags inherited vars like EDITOR + * / PAGER, which aborts the clone before it starts. A direct spawn lets us pass + * the full process.env (PATH/HOME/proxy/TLS) plus the askpass wiring with no + * argument/env guard interference, while still keeping: + * - the token OFF the argv (the URL carries only the `x-access-token` + * username) and OUT of .git/config (MEDIUM-2/MEDIUM-3); + * - the token bound to the source host so a cross-host redirect can't obtain + * it (Decision 3 — host-bound askpass + libcurl's default cross-host + * credential stripping). Redirects are NOT disabled (review finding R-1), + * so legitimate same-host redirects still work. + */ +async function cloneWithOverrideToken( + repoUrl: string, + targetDir: string, + opts: CloneOptions & { token: string } +): Promise<{ localPath: string; branch: string }> { + const { cloneUrl, host } = buildOverrideCloneAuth(repoUrl); + const askpass = createAskpassScript(); + + const args = ['clone']; + if (opts.branch) { + args.push('--branch', opts.branch); + } + if (opts.depth) { + args.push('--depth', String(opts.depth)); + } + // `--` terminates option parsing so the URL/dir can never be read as flags. + args.push('--', cloneUrl, targetDir); + + const env: NodeJS.ProcessEnv = { + ...process.env, + GIT_ASKPASS: askpass.scriptPath, + // Never fall back to an interactive terminal prompt if askpass yields nothing. + GIT_TERMINAL_PROMPT: '0', + [ASKPASS_TOKEN_ENV]: opts.token, + [ASKPASS_HOST_ENV]: host, + }; + + try { + await new Promise((resolve, reject) => { + const child = spawn('git', args, { + env, + stdio: ['ignore', 'ignore', 'pipe'], + }); + let stderr = ''; + // The 'error', 'close', and timeout handlers can each race to settle this + // promise (e.g. 'close' still fires after a kill or a spawn 'error'). A + // promise only settles once, but the LATER handlers would still run their + // logic on an already-settled promise. Guard so the FIRST settle wins and + // every subsequent handler is a no-op, and clear the timeout on settle so + // no dangling timer fires afterwards. + let settled = false; + const timerRef: { id?: ReturnType } = {}; + const settle = (action: () => void): void => { + if (settled) return; + settled = true; + if (timerRef.id) clearTimeout(timerRef.id); + action(); + }; + + timerRef.id = setTimeout(() => { + settle(() => { + child.kill('SIGKILL'); + reject(new Error(`git clone timed out after ${GIT_TIMEOUT_MS}ms`)); + }); + }, GIT_TIMEOUT_MS); + child.stderr?.on('data', chunk => { + stderr += chunk.toString(); + }); + child.on('error', err => { + settle(() => reject(err)); + }); + child.on('close', code => { + settle(() => { + if (code === 0) { + resolve(); + } else { + // stderr carries only the username-only URL (no token), so it is + // safe to surface; the caller scrubs it again as defense-in-depth. + reject( + new Error(`git clone exited with code ${code}: ${stderr.trim()}`) + ); + } + }); + }); + }); + } finally { + // Remove the askpass helper as soon as the clone finishes (success or + // failure). It holds no secret, but leaving temp files around is untidy. + askpass.cleanup(); + } + + return { localPath: targetDir, branch: opts.branch || 'main' }; } export async function cloneRepo( @@ -207,10 +408,19 @@ export async function cloneRepo( targetDir: string, opts?: CloneOptions ): Promise<{ localPath: string; branch: string }> { + // PRD #621 M3 / Decision 4: a per-request override credential takes precedence + // over env auth for THIS clone only and uses the host-bound GIT_ASKPASS path. + if (opts?.token) { + return cloneWithOverrideToken(repoUrl, targetDir, { + ...opts, + token: opts.token, + }); + } + + // Env/GitHub-App auth path (unchanged): credentials come from + // getGitAuthConfigFromEnv and are embedded in the URL as before. const authConfig = getGitAuthConfigFromEnv(); let cloneUrl: string; - - // Use authenticated URL if credentials are available, otherwise clone unauthenticated (public repos) if (authConfig.pat || authConfig.githubApp) { const token = await getAuthToken(authConfig); cloneUrl = getAuthenticatedUrl(repoUrl, token); diff --git a/src/core/user-prompts-loader.ts b/src/core/user-prompts-loader.ts index 1ce606d3..8e68a524 100644 --- a/src/core/user-prompts-loader.ts +++ b/src/core/user-prompts-loader.ts @@ -15,6 +15,7 @@ import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; +import * as crypto from 'crypto'; import { Logger } from './error-handling'; import { cloneRepo, @@ -44,6 +45,15 @@ export interface UserPromptsOverride { repoUrl: string; branch?: string; subPath?: string; + /** + * Per-request git credential forwarded by the caller (PRD #621 M2/M3, via the + * X-Dot-AI-Git-Token header). When present it takes precedence over + * DOT_AI_GIT_TOKEN for THIS request only (Decision 4) and triggers per-request + * cache isolation so a private authenticated clone is never served to/from the + * shared unauthenticated cache slot (Decision 2). Never enters the cache key + * or any log/error/source surface. + */ + gitToken?: string; } /** @@ -181,7 +191,10 @@ export function getUserPromptsConfigFromOverride( repoUrl: override.repoUrl, branch, subPath: normalizedSubPath, - gitToken: process.env.DOT_AI_GIT_TOKEN, + // PRD #621 M2 / Decision 4: a request-supplied token (override.gitToken) + // takes precedence over the server env credential for this request only; + // the env credential remains the fallback when no header is present. + gitToken: override.gitToken ?? process.env.DOT_AI_GIT_TOKEN, cacheTtlSeconds, }; } @@ -279,12 +292,42 @@ function isValidGitBranch(branch: string): boolean { } /** - * Clone the user prompts repository + * Create a unique, throwaway ROOT directory for a token-bearing override + * request (PRD #621 M3 / Decision 2). It lives alongside the shared cache + * directory but is NOT the shared slot, so an authenticated private clone is + * never written to (or served from) the unauthenticated cache. The caller + * removes it after reading. + * + * Hardening (LOW-4): created atomically via fs.mkdtempSync with a CSPRNG + * (crypto.randomUUID) name component and mode 0700, so the authenticated clone + * cannot land in a predictable, world-readable location. + */ +function createIsolatedCloneRoot(): string { + const parent = path.dirname(getCacheDirectory()); + const root = fs.mkdtempSync( + path.join(parent, `user-prompts-override-${crypto.randomUUID()}-`) + ); + try { + fs.chmodSync(root, 0o700); + } catch { + /* best-effort hardening (mkdtempSync already creates with 0700) */ + } + return root; +} + +/** + * Clone the user prompts repository. + * + * `overrideToken` (PRD #621 M3) is a per-request credential that, when present, + * overrides env auth for this clone only and is scoped to the source host with + * no cross-host redirect forwarding (handled in cloneRepo). When omitted, the + * clone authenticates via env exactly as before. */ async function cloneRepository( config: UserPromptsConfig, localPath: string, - logger: Logger + logger: Logger, + overrideToken?: string ): Promise { // Validate branch name as defense-in-depth if (!isValidGitBranch(config.branch)) { @@ -314,6 +357,9 @@ async function cloneRepository( await cloneRepo(config.repoUrl, localPath, { branch: config.branch, depth: 1, + // Per-request override credential (PRD #621 M3). undefined → cloneRepo + // falls back to env auth, i.e. today's behavior unchanged. + token: overrideToken, }); logger.info('Successfully cloned user prompts repository', { @@ -321,13 +367,13 @@ async function cloneRepository( branch: config.branch, }); } catch (error) { + const scrub = (raw: string): string => + scrubCredentials( + config.gitToken ? raw.replaceAll(config.gitToken, '***') : raw + ); const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - const sanitizedError = scrubCredentials( - config.gitToken - ? errorMessage.replaceAll(config.gitToken, '***') - : errorMessage - ); + const sanitizedError = scrub(errorMessage); logger.error( 'Failed to clone user prompts repository', @@ -337,6 +383,17 @@ async function cloneRepository( branch: config.branch, } ); + // LOW-5: scrub the caught error IN PLACE (message + stack) before attaching + // it as `cause`, so a serialized `.cause` cannot leak the token. (With the + // GIT_ASKPASS rework the override token is no longer on the git argv/URL, so + // it cannot appear here in the first place — this is defense-in-depth, esp. + // for the env-credential path which still embeds its token in the URL.) + if (error instanceof Error) { + error.message = scrub(error.message); + if (error.stack) { + error.stack = scrub(error.stack); + } + } throw new Error( `Failed to clone user prompts repository: ${sanitizedError}`, { cause: error } @@ -386,14 +443,64 @@ async function pullRepository( } /** - * Ensure the repository is cloned and up-to-date - * Returns the path to the prompts directory within the repository + * Ensure the repository is cloned and up-to-date. + * + * Returns the path to the prompts directory within the repository, plus an + * optional `isolatedRoot` the caller must remove after reading (set only for + * the token-bearing isolation path below). + * + * PRD #621 M3 / Decision 2 (cache isolation): when `overrideToken` is present + * (a request forwarded an X-Dot-AI-Git-Token), the clone is performed into a + * unique throwaway directory and the shared `cacheState` is neither read nor + * written. This guarantees an authenticated private clone is never served from + * — nor written into — the shared unauthenticated cache slot for the same + * (repoUrl, branch, subPath) coordinate, and that the token never enters the + * cache key. Token-less requests use the shared cache exactly as before. */ async function ensureRepository( config: UserPromptsConfig, logger: Logger, - forceRefresh: boolean = false -): Promise { + forceRefresh: boolean = false, + overrideToken?: string +): Promise<{ promptsDir: string; isolatedRoot?: string }> { + if (overrideToken) { + const isolatedRoot = createIsolatedCloneRoot(); + // Clone into a subdirectory of the 0700 root so the root's restrictive + // permissions cover the authenticated clone (git creates `cloneDir` itself). + const cloneDir = path.join(isolatedRoot, 'repo'); + logger.debug('Token-bearing override: cloning in isolation', { + url: sanitizeUrlForLogging(config.repoUrl), + branch: config.branch, + }); + try { + await cloneRepository(config, cloneDir, logger, overrideToken); + } catch (error) { + // Remove any partial clone before propagating so a failed authenticated + // request leaves no isolated directory behind. With GIT_ASKPASS the token + // is never written to disk, so a cleanup failure cannot leave a PAT + // behind — but warn (don't swallow) for observability. + try { + fs.rmSync(isolatedRoot, { recursive: true, force: true }); + } catch (cleanupError) { + logger.warn( + 'Failed to remove isolated clone directory after clone failure', + { + path: isolatedRoot, + error: + cleanupError instanceof Error + ? cleanupError.message + : String(cleanupError), + } + ); + } + throw error; + } + const promptsDir = config.subPath + ? path.join(cloneDir, config.subPath) + : cloneDir; + return { promptsDir, isolatedRoot }; + } + const localPath = getCacheDirectory(); const now = Date.now(); const ttlMs = config.cacheTtlSeconds * 1000; @@ -443,7 +550,11 @@ async function ensureRepository( } // Return path to prompts directory (with optional subPath) - return config.subPath ? path.join(localPath, config.subPath) : localPath; + return { + promptsDir: config.subPath + ? path.join(localPath, config.subPath) + : localPath, + }; } const SKILL_FILE_MAX_BYTES = 5 * 1024 * 1024; // 5 MB per file (before base64 encoding) @@ -556,8 +667,22 @@ export async function loadUserPrompts( return []; } + // PRD #621 M3 / Decision 2: a request-forwarded credential (override.gitToken) + // triggers per-request cache isolation. Track the throwaway clone directory so + // it is removed after the read (success-path cleanup; the failure path is + // cleaned up inside ensureRepository). + const overrideToken = override?.gitToken; + let isolatedRoot: string | undefined; + try { - const promptsDir = await ensureRepository(config, logger, forceRefresh); + const ensured = await ensureRepository( + config, + logger, + forceRefresh, + overrideToken + ); + const promptsDir = ensured.promptsDir; + isolatedRoot = ensured.isolatedRoot; if (!fs.existsSync(promptsDir)) { logger.warn('User prompts directory not found in repository', { @@ -651,6 +776,24 @@ export async function loadUserPrompts( new Error(safeMessage) ); return []; + } finally { + // PRD #621 M3 / Decision 2: remove the per-request isolated clone (if any) + // so token-bearing override clones leave no on-disk residue. With + // GIT_ASKPASS the token is never written to disk, so a failed cleanup + // cannot leave a PAT behind — but warn (don't swallow) for observability. + if (isolatedRoot) { + try { + fs.rmSync(isolatedRoot, { recursive: true, force: true }); + } catch (cleanupError) { + logger.warn('Failed to remove isolated clone directory', { + path: isolatedRoot, + error: + cleanupError instanceof Error + ? cleanupError.message + : String(cleanupError), + }); + } + } } } diff --git a/src/interfaces/cors-headers.ts b/src/interfaces/cors-headers.ts new file mode 100644 index 00000000..f3634f7d --- /dev/null +++ b/src/interfaces/cors-headers.ts @@ -0,0 +1,44 @@ +/** + * Shared CORS allow-header lists (PRD #621 M2, Decision 1). + * + * The REST (src/interfaces/rest-api.ts) and front HTTP (src/interfaces/mcp.ts) + * layers each answer CORS preflight with their own `Access-Control-Allow-Headers` + * value, and the two were historically OUT OF SYNC. Centralizing the credential + * header name here guarantees the new `X-Dot-AI-Git-Token` header is advertised + * by BOTH preflight responses (Decision 1) and that the two lists can never + * silently drift apart on this header again. + * + * The two lists intentionally differ on the OTHER headers (mcp.ts also allows + * X-Session-Id / X-Dot-AI-Authorization), which is preserved. + */ + +/** + * Per-request git credential header (PRD #621 M2/M3, Decision 1). The CLI + * forwards its `DOT_AI_GIT_TOKEN` here so the server can authenticate an + * overridden (`?repo=`) clone against a second auth realm. Always a request + * header — never a query param or body field. + */ +export const GIT_TOKEN_HEADER = 'X-Dot-AI-Git-Token'; + +/** + * Lowercased form for reading the header off Node's `req.headers` (Node + * lowercases all incoming header names). + */ +export const GIT_TOKEN_HEADER_LC = GIT_TOKEN_HEADER.toLowerCase(); + +/** + * `Access-Control-Allow-Headers` value for the REST API layer + * (rest-api.ts `setCorsHeaders`). + */ +export const REST_CORS_ALLOW_HEADERS = `Content-Type, Authorization, ${GIT_TOKEN_HEADER}`; + +/** + * `Access-Control-Allow-Headers` value for the front HTTP layer (mcp.ts). + * Retains X-Session-Id and X-Dot-AI-Authorization from the pre-existing list. + * + * Includes `Mcp-Session-Id` because the MCP session router in mcp.ts routes + * requests by `req.headers['mcp-session-id']` (the Streamable HTTP transport's + * session header); without it a browser preflight requesting that header would + * fail (CodeRabbit Finding 3). `X-Session-Id` is kept for backward compat. + */ +export const MCP_CORS_ALLOW_HEADERS = `Content-Type, X-Session-Id, Mcp-Session-Id, Authorization, X-Dot-AI-Authorization, ${GIT_TOKEN_HEADER}`; diff --git a/src/interfaces/header-redaction.ts b/src/interfaces/header-redaction.ts new file mode 100644 index 00000000..bb646f5f --- /dev/null +++ b/src/interfaces/header-redaction.ts @@ -0,0 +1,49 @@ +/** + * Redaction of credential-bearing request headers before they are logged. + * + * The front HTTP layer (mcp.ts) logs incoming requests at debug level. Logging + * `req.headers` verbatim would leak credentials — `Authorization`, + * `X-Dot-AI-Authorization`, and (PRD #621) the per-request `X-Dot-AI-Git-Token` + * — bypassing the loader-level scrubbing. PRD #621 requires the forwarded token + * NEVER appear in logs, so these header values are replaced with a fixed + * placeholder before logging. + * + * Kept in its own module so the redaction is unit-testable without importing + * the heavy mcp.ts server module. + */ + +import { GIT_TOKEN_HEADER_LC } from './cors-headers'; + +/** Fixed placeholder substituted for a redacted header value. */ +export const REDACTED_PLACEHOLDER = '***REDACTED***'; + +/** + * Lowercased names of headers whose values are credential-bearing and must be + * redacted before logging. Node lowercases all incoming header names, so the + * comparison is done in lowercase. + */ +export const SENSITIVE_HEADER_NAMES: ReadonlySet = new Set([ + 'authorization', + 'x-dot-ai-authorization', + GIT_TOKEN_HEADER_LC, + 'cookie', + 'proxy-authorization', +]); + +/** + * Return a shallow copy of `headers` with the value of every credential-bearing + * header replaced by REDACTED_PLACEHOLDER. Non-sensitive headers are preserved + * verbatim. The input object is never mutated. + */ +export function redactSensitiveHeaders( + headers: Record | undefined +): Record { + if (!headers) return {}; + const redacted: Record = {}; + for (const [name, value] of Object.entries(headers)) { + redacted[name] = SENSITIVE_HEADER_NAMES.has(name.toLowerCase()) + ? REDACTED_PLACEHOLDER + : value; + } + return redacted; +} diff --git a/src/interfaces/mcp.ts b/src/interfaces/mcp.ts index 8446faaf..06025e7a 100644 --- a/src/interfaces/mcp.ts +++ b/src/interfaces/mcp.ts @@ -80,7 +80,9 @@ import { type PromptsListArgs, } from '../tools/prompts'; import { RestToolRegistry } from './rest-registry'; -import { RestApiRouter } from './rest-api'; +import { RestApiRouter, sanitizeRequestUrlForLogging } from './rest-api'; +import { MCP_CORS_ALLOW_HEADERS } from './cors-headers'; +import { redactSensitiveHeaders } from './header-redaction'; import { checkBearerAuth, DotAIOAuthProvider, @@ -651,10 +653,16 @@ export class MCPServer { // Execute entire request within the span's context for proper propagation await context.with(trace.setSpan(context.active(), span), async () => { try { + // PRD #621: the forwarded token must never appear in logs. Redact + // credential-bearing headers (HIGH-1) AND scrub credentials from the + // request URL — a ?repo=https://user:token@host or a + // credential-bearing query param would otherwise leak verbatim + // (CodeRabbit Finding 1). sanitizeRequestUrlForLogging is the shared + // helper already used by the REST layer. this.logger.debug('HTTP request received', { method: req.method, - url: req.url, - headers: req.headers, + url: sanitizeRequestUrlForLogging(req.url), + headers: redactSensitiveHeaders(req.headers), }); // Handle CORS for browser-based clients @@ -663,9 +671,11 @@ export class MCPServer { 'Access-Control-Allow-Methods', 'GET, POST, DELETE, OPTIONS' ); + // PRD #621 M2 / Decision 1: includes X-Dot-AI-Git-Token, kept in + // sync with the REST allowlist via cors-headers.ts. res.setHeader( 'Access-Control-Allow-Headers', - 'Content-Type, X-Session-Id, Authorization, X-Dot-AI-Authorization' + MCP_CORS_ALLOW_HEADERS ); if (req.method === 'OPTIONS') { diff --git a/src/interfaces/rest-api.ts b/src/interfaces/rest-api.ts index 391e1a23..5bc442d3 100644 --- a/src/interfaces/rest-api.ts +++ b/src/interfaces/rest-api.ts @@ -27,6 +27,10 @@ import { UserPromptsOverride, } from '../core/user-prompts-loader'; import { scrubCredentials } from '../core/git-utils'; +import { + GIT_TOKEN_HEADER_LC, + REST_CORS_ALLOW_HEADERS, +} from './cors-headers'; import { GenericSessionManager } from '../core/generic-session-manager'; import { getSessionEventBus, @@ -120,6 +124,159 @@ export function sanitizeRequestUrlForLogging( } } +/** + * Coerce an optional override string param (path/branch) supplied via query + * string or JSON body. Mirrors the `repo` guard in extractPromptsOverride: + * - non-string (array, number, object, boolean) → 400 (avoids a 500 from a + * malformed body reaching downstream code). + * - absent (null/undefined) or empty/whitespace-only → `undefined`, i.e. + * treated as not supplied so the downstream default (subPath '' / branch + * 'main') is preserved and an empty-string branch never reaches + * isValidGitBranch (which would otherwise reject it). + * - otherwise → the trimmed value. + */ +function coerceOverrideStringParam( + value: unknown, + name: 'path' | 'branch' +): + | { ok: true; value: string | undefined } + | { ok: false; message: string } { + if (value === undefined || value === null) { + return { ok: true, value: undefined }; + } + if (typeof value !== 'string') { + return { + ok: false, + message: `${name} must be a string (got ${Array.isArray(value) ? 'array' : typeof value})`, + }; + } + const trimmed = value.trim(); + return { ok: true, value: trimmed.length > 0 ? trimmed : undefined }; +} + +/** + * Read the per-request git credential from the X-Dot-AI-Git-Token header + * (PRD #621 M2). Node lowercases incoming header names and may present a + * repeated header as an array; normalize to a single non-empty string or + * undefined. The value is a secret, so it is never logged here. + */ +function readGitTokenHeader(req: IncomingMessage): string | undefined { + const raw = req.headers[GIT_TOKEN_HEADER_LC]; + const value = Array.isArray(raw) ? raw[0] : raw; + if (typeof value !== 'string') { + return undefined; + } + const trimmed = value.trim(); + return trimmed.length > 0 ? trimmed : undefined; +} + +/** + * Extract and validate the per-request prompts override. + * + * Threads three optional, additive inputs from the request into a + * UserPromptsOverride (PRD #581 introduced `repo`; PRD #621 M1 adds `path` + * and `branch`): + * - repoParam → override.repoUrl (GET ?repo= / POST body `repo`) + * - pathParam → override.subPath (GET ?path= / POST body `path`) + * - branchParam → override.branch (GET ?branch= / POST body `branch`) + * - gitToken → override.gitToken (X-Dot-AI-Git-Token request header; M2) + * + * Returns: + * - { ok: true, override } when no `repo` is supplied (override undefined; + * any path/branch/token are ignored, since they only qualify an override — + * this keeps the no-`repo` / env-var-configured path unchanged). + * - { ok: true, override } when a syntactically valid override is supplied. + * - { ok: false, message } when the override fails validation (HTTP 400). + * + * The validation message is run through scrubCredentials so embedded tokens + * never reach the wire response. + * + * Backward compatibility (PRD #621, non-negotiable): when path/branch are + * absent or empty, the override carries `repoUrl` only — byte-identical to + * the PRD #581 behavior (same clone target: repo root, `main`). subPath and + * branch are populated ONLY for a non-empty value, so downstream defaults are + * untouched. The credential header is INERT unless a `?repo=` override is + * present: without a repo this returns `override: undefined` before the token + * is ever read, so the env-var path is unaffected by a forwarded header. + * + * Validation is delegated to getUserPromptsConfigFromOverride (scheme, + * sanitizeRelativePath for subPath, isValidGitBranch for branch) and happens + * BEFORE any clone or shared-cache mutation, so a rejected override can never + * corrupt the env-var-configured cache. + */ +export function extractPromptsOverride( + repoParam: unknown, + pathParam?: unknown, + branchParam?: unknown, + gitToken?: string +): + | { + ok: true; + override?: UserPromptsOverride; + } + | { + ok: false; + message: string; + } { + // Treat absent (null/undefined) repo as no override. path/branch/token only + // qualify an override, so without a repo they are ignored (the credential + // header is inert on the env-var path). + if (repoParam === undefined || repoParam === null) { + return { ok: true, override: undefined }; + } + // The wire contract says `repo` is a string. Anything else (array, + // number, object, boolean) is a 400 — otherwise downstream code + // would crash to a 500. + if (typeof repoParam !== 'string') { + return { + ok: false, + message: `repo must be a string (got ${Array.isArray(repoParam) ? 'array' : typeof repoParam})`, + }; + } + const trimmed = repoParam.trim(); + if (!trimmed) { + return { ok: true, override: undefined }; + } + const candidate: UserPromptsOverride = { repoUrl: trimmed }; + + // PRD #621 M1: thread ?path= / body `path` into subPath (validated + // downstream by sanitizeRelativePath). + const pathResult = coerceOverrideStringParam(pathParam, 'path'); + if (!pathResult.ok) { + return pathResult; + } + if (pathResult.value !== undefined) { + candidate.subPath = pathResult.value; + } + + // PRD #621 M1: thread ?branch= / body `branch` into branch (validated + // downstream by isValidGitBranch). + const branchResult = coerceOverrideStringParam(branchParam, 'branch'); + if (!branchResult.ok) { + return branchResult; + } + if (branchResult.value !== undefined) { + candidate.branch = branchResult.value; + } + + // PRD #621 M2: the X-Dot-AI-Git-Token header (read by the handler and passed + // in already-normalized to a non-empty string or undefined) authenticates + // THIS override clone. It travels only as a header — never query/body — and + // is never echoed (computePromptsSource uses repoUrl only). + if (gitToken) { + candidate.gitToken = gitToken; + } + + try { + // Throws on invalid scheme / subPath / branch. + getUserPromptsConfigFromOverride(candidate); + return { ok: true, override: candidate }; + } catch (error) { + const raw = error instanceof Error ? error.message : 'Invalid override'; + return { ok: false, message: scrubCredentials(raw) }; + } +} + /** * HTTP status codes for REST responses */ @@ -1860,54 +2017,6 @@ export class RestApiRouter { } } - /** - * Extract and validate the per-request `repo` override (PRD #581). - * - * Returns: - * - { ok: true, override } when no repo param is supplied, override is undefined. - * - { ok: true, override } when a syntactically valid override URL is supplied. - * - { ok: false, message } when the override fails validation (HTTP 400). - * - * The validation message is run through scrubCredentials so embedded tokens - * never reach the wire response. - */ - private extractPromptsOverride(repoParam: unknown): - | { - ok: true; - override?: UserPromptsOverride; - } - | { - ok: false; - message: string; - } { - // Treat absent (null/undefined) as no override. - if (repoParam === undefined || repoParam === null) { - return { ok: true, override: undefined }; - } - // The wire contract says `repo` is a string. Anything else (array, - // number, object, boolean) is a 400 — otherwise downstream code - // would crash to a 500. - if (typeof repoParam !== 'string') { - return { - ok: false, - message: `repo must be a string (got ${Array.isArray(repoParam) ? 'array' : typeof repoParam})`, - }; - } - const trimmed = repoParam.trim(); - if (!trimmed) { - return { ok: true, override: undefined }; - } - const candidate: UserPromptsOverride = { repoUrl: trimmed }; - try { - // Throws on invalid scheme / subPath / branch. - getUserPromptsConfigFromOverride(candidate); - return { ok: true, override: candidate }; - } catch (error) { - const raw = error instanceof Error ? error.message : 'Invalid override'; - return { ok: false, message: scrubCredentials(raw) }; - } - } - /** * Handle prompts list requests */ @@ -1920,8 +2029,15 @@ export class RestApiRouter { try { this.logger.info('Processing prompts list request', { requestId }); - const overrideResult = this.extractPromptsOverride( - searchParams.get('repo') + // PRD #581: ?repo= override. PRD #621 M1: ?path= / ?branch= thread into + // candidate.subPath / candidate.branch (absent → unchanged behavior). + // PRD #621 M2: X-Dot-AI-Git-Token header authenticates the override clone + // (inert when no ?repo= override is present). + const overrideResult = extractPromptsOverride( + searchParams.get('repo'), + searchParams.get('path'), + searchParams.get('branch'), + readGitTokenHeader(req) ); if (!overrideResult.ok) { await this.sendErrorResponse( @@ -1993,8 +2109,15 @@ export class RestApiRouter { promptName, }); - const overrideResult = this.extractPromptsOverride( - searchParams.get('repo') + // PRD #581: ?repo= override. PRD #621 M1: ?path= / ?branch= thread into + // candidate.subPath / candidate.branch (absent → unchanged behavior). + // PRD #621 M2: X-Dot-AI-Git-Token header authenticates the override clone + // (inert when no ?repo= override is present). + const overrideResult = extractPromptsOverride( + searchParams.get('repo'), + searchParams.get('path'), + searchParams.get('branch'), + readGitTokenHeader(req) ); if (!overrideResult.ok) { await this.sendErrorResponse( @@ -2077,9 +2200,20 @@ export class RestApiRouter { }); // body.repo type is checked inside extractPromptsOverride (it accepts - // unknown and rejects non-string values with 400 — see F2). - const bodyObj = body as { repo?: unknown } | undefined; - const overrideResult = this.extractPromptsOverride(bodyObj?.repo); + // unknown and rejects non-string values with 400 — see F2). PRD #621 M1: + // body `path` / `branch` thread into candidate.subPath / candidate.branch + // (absent → unchanged behavior); both are likewise type-checked. + const bodyObj = body as + | { repo?: unknown; path?: unknown; branch?: unknown } + | undefined; + // PRD #621 M2: the credential always travels as the X-Dot-AI-Git-Token + // header — never the body — and is inert without a repo override. + const overrideResult = extractPromptsOverride( + bodyObj?.repo, + bodyObj?.path, + bodyObj?.branch, + readGitTokenHeader(req) + ); if (!overrideResult.ok) { await this.sendErrorResponse( res, @@ -2639,7 +2773,9 @@ export class RestApiRouter { if (this.config.enableCors) { headers['Access-Control-Allow-Origin'] = '*'; - headers['Access-Control-Allow-Headers'] = 'Content-Type, Authorization'; + // R-2: use the shared allowlist (single source of truth in cors-headers.ts) + // so the SSE preflight includes X-Dot-AI-Git-Token like every other route. + headers['Access-Control-Allow-Headers'] = REST_CORS_ALLOW_HEADERS; } res.writeHead(HttpStatus.OK, headers); @@ -3588,10 +3724,9 @@ export class RestApiRouter { private setCorsHeaders(res: ServerResponse): void { res.setHeader('Access-Control-Allow-Origin', '*'); res.setHeader('Access-Control-Allow-Methods', 'GET, POST, DELETE, OPTIONS'); - res.setHeader( - 'Access-Control-Allow-Headers', - 'Content-Type, Authorization' - ); + // PRD #621 M2 / Decision 1: advertise the X-Dot-AI-Git-Token credential + // header (shared with the mcp.ts allowlist via cors-headers.ts). + res.setHeader('Access-Control-Allow-Headers', REST_CORS_ALLOW_HEADERS); res.setHeader('Access-Control-Max-Age', '86400'); } diff --git a/tests/integration/tools/prompts.test.ts b/tests/integration/tools/prompts.test.ts index ac841420..7b9e13e8 100644 --- a/tests/integration/tools/prompts.test.ts +++ b/tests/integration/tools/prompts.test.ts @@ -5,10 +5,26 @@ * Validates prompts list and get functionality with exact data validation. */ -import { describe, test, expect, beforeAll } from 'vitest'; +import { describe, test, expect, beforeAll, afterAll } from 'vitest'; import { IntegrationTest } from '../helpers/test-base.js'; -describe.concurrent('Prompts Integration', () => { +// NOTE: this suite is intentionally NOT describe.concurrent. Every test here +// exercises the SAME deployed server's SINGLE shared user-prompts loader cache +// (one on-disk cache directory keyed by repoUrl/branch/subPath, with no locking). +// That is shared state, so per the integration convention ("only run concurrently +// if tests are truly independent — no shared state") these tests must be +// serialized: the non-token override happy-path tests (PRD #621 M1 path/branch +// subdir clones and the ?repo=-only root clone) RE-CLONE that shared directory to +// a different coordinate, and running them concurrently with the env-coordinate +// readers (Prompts List, Prompts Get, Prompts Cache Refresh, the no-?repo source +// tests) raced — a concurrent rmSync+re-clone made a reader observe an incomplete +// env cache (built-in-only 11 instead of 11 + 4 env user prompts = 15). Serial +// execution removes that race entirely. Test FILES still run in parallel via the +// fork pool (maxForks), and no other base-group-2 file touches /api/v1/prompts, +// so cross-file parallelism is unaffected. The token-bearing M3 tests clone into +// their own isolated mkdtemp dir and were never the hazard; serializing is inert +// for them. +describe('Prompts Integration', () => { const integrationTest = new IntegrationTest(); // Detect deployment mode based on MCP_BASE_URL (parse hostname to avoid substring matching issues) @@ -639,4 +655,554 @@ describe.concurrent('Prompts Integration', () => { expect(JSON.stringify(response)).not.toContain(secret); }); }); + + // PRD #621 M1: the override must also carry a subdirectory (?path= / body + // `path`) and a branch (?branch= / body `branch`), threaded into + // candidate.subPath / candidate.branch inside extractPromptsOverride and + // HONORED by the clone. Today extractPromptsOverride builds `{ repoUrl }` + // only (rest-api.ts ~line 1900) and the handlers pass only `repo`, so the + // five tests below are RED until M1 lands. + // + // Cache/race note (extends the PRD #581 comment above): a happy-path override + // re-clones into the SINGLE shared cache directory (getCacheDirectory() is + // NOT keyed by coordinate) and overwrites the loader's + // (repoUrl, branch, subPath) cacheState. The integration server is shared + // across all test files, so a concurrent plain /api/v1/prompts request can + // re-clone the env-var coordinate into the same directory mid-read. To keep + // these deterministic we: + // - observe each override within a SINGLE request (that request's response + // reflects its own clone+read), + // - retry the observation a few times (expectEventually) to absorb a + // transient concurrent re-clone, and + // - restore the env-var cache (refresh with no override) in finally/afterAll. + // The fixture — a uniquely-named prompt committed to a throwaway branch under + // a non-default subdirectory — is created via the GitHub API exactly like the + // 'Prompts Cache Refresh' test above, and only when DOT_AI_GIT_TOKEN is set. + describe('Per-request path + branch override (PRD #621 M1)', () => { + const gitToken = process.env.DOT_AI_GIT_TOKEN; + // Must match DOT_AI_USER_PROMPTS_REPO from the integration infra so the + // override changes only branch/subPath (not repoUrl) vs. the env config. + const promptsRepoUrl = + 'https://github.com/vfarcic/dot-ai-test-prompts.git'; + const ghRepoApi = + 'https://api.github.com/repos/vfarcic/dot-ai-test-prompts'; + + const overrideRunId = Date.now(); + const fixtureBranch = `prd621-fixture-${overrideRunId}`; + const fixtureSubdir = 'prd621-skills'; + const fixturePromptName = `prd621-override-${overrideRunId}`; + const fixtureDescription = + 'PRD 621 fixture prompt: subdir on a non-default branch'; + const fixturePromptPath = `${fixtureSubdir}/${fixturePromptName}.md`; + const fixturePromptContent = [ + '---', + `name: ${fixturePromptName}`, + `description: ${fixtureDescription}`, + '---', + '', + 'This prompt lives ONLY on a non-default branch under a non-default', + 'subdirectory. It is reachable only when BOTH ?path= and ?branch= are', + 'threaded into the override and honored by the clone (PRD #621 M1).', + ].join('\n'); + + // Two-stage tracking so teardown is robust to PARTIAL setup: the branch is + // created before the file is committed, so if the commit (or anything after + // branch creation) throws, afterAll must still delete the branch or it is + // orphaned on the remote. fixtureBranchCreated gates cleanup; fixtureReady + // gates the happy-path tests. + let fixtureBranchCreated = false; + let fixtureReady = false; + + async function ghApi(method: string, apiPath: string, body?: unknown) { + return fetch(`${ghRepoApi}${apiPath}`, { + method, + headers: { + Authorization: `token ${gitToken}`, + Accept: 'application/vnd.github+json', + 'Content-Type': 'application/json', + }, + body: body ? JSON.stringify(body) : undefined, + }); + } + + // Refresh with NO override restores the loader cacheState to the env-var + // coordinate (repoUrl, main, user-prompts) without changing it, undoing any + // coordinate drift left by an override request. + async function restoreEnvCache(): Promise { + await integrationTest.httpClient.post('/api/v1/prompts/refresh', {}); + } + + // Retry an assertion block to absorb a transient concurrent re-clone of the + // shared cache directory (see cache/race note above). In RED the assertion + // never passes, so this just delays the (expected) final failure. + async function expectEventually( + fn: () => Promise, + attempts = 5, + delayMs = 1500 + ): Promise { + let lastError: unknown; + for (let i = 0; i < attempts; i++) { + try { + await fn(); + return; + } catch (error) { + lastError = error; + await new Promise(resolve => setTimeout(resolve, delayMs)); + } + } + throw lastError; + } + + beforeAll(async () => { + if (!gitToken) return; + + // 1. Resolve main's commit SHA. + const refRes = await ghApi('GET', '/git/ref/heads/main'); + expect(refRes.ok).toBe(true); + const refData = (await refRes.json()) as { object: { sha: string } }; + + // 2. Create the throwaway fixture branch off main. + const branchRes = await ghApi('POST', '/git/refs', { + ref: `refs/heads/${fixtureBranch}`, + sha: refData.object.sha, + }); + expect(branchRes.ok).toBe(true); + // The branch now exists on the remote — record this BEFORE the file commit + // so afterAll always cleans it up even if the commit below fails. + fixtureBranchCreated = true; + + // 3. Commit the fixture prompt under the non-default subdirectory on it. + const fileRes = await ghApi('PUT', `/contents/${fixturePromptPath}`, { + message: 'test: add PRD #621 path/branch override fixture', + content: Buffer.from(fixturePromptContent).toString('base64'), + branch: fixtureBranch, + }); + expect(fileRes.ok).toBe(true); + + fixtureReady = true; + }); + + afterAll(async () => { + try { + // Always delete the throwaway branch if it was created — even on a + // PARTIAL setup (branch created but file commit failed) — so it is never + // orphaned on the remote. Deleting the branch removes the fixture file + // with it. Gated on fixtureBranchCreated, NOT fixtureReady. + if (fixtureBranchCreated) { + await ghApi('DELETE', `/git/refs/heads/${fixtureBranch}`); + } + } finally { + // Restore the shared server cache to the env-var coordinate for other + // test files, regardless of whether branch deletion succeeded. Only the + // gitToken path runs override tests that could have drifted the cache. + if (gitToken) { + await restoreEnvCache(); + } + } + }); + + test('GET /api/v1/prompts?path=&branch= resolves a prompt from the subdir on the non-default branch (PRD #621)', async () => { + if (!gitToken) { + console.log( + 'Skipping PRD #621 path/branch GET test: DOT_AI_GIT_TOKEN not set' + ); + return; + } + try { + await expectEventually(async () => { + const response = await integrationTest.httpClient.get( + `/api/v1/prompts?repo=${encodeURIComponent(promptsRepoUrl)}` + + `&path=${encodeURIComponent(fixtureSubdir)}` + + `&branch=${encodeURIComponent(fixtureBranch)}` + ); + expect(response).toMatchObject({ success: true }); + const fixturePrompt = response.data.prompts.find( + (p: { name: string }) => p.name === fixturePromptName + ); + // RED until M1: ?path=/?branch= are dropped, the clone reads repo + // ROOT on MAIN, and this prompt is absent (fixturePrompt undefined). + expect(fixturePrompt).toMatchObject({ + name: fixturePromptName, + description: fixtureDescription, + }); + }); + } finally { + await restoreEnvCache(); + } + // 300000ms: comprehensive test — real git clone + up to 5 expectEventually + // retries (matches the project convention used by the cache-refresh test). + }, 300000); + + // End-to-end exercise of the real git clone-with-token path (PRD #621 M3). + // This is the ONLY e2e coverage of the credential clone: every other + // positive token test mocks simple-git. The token-vs-env auth distinction + // is deliberately NOT asserted (no private/second-realm repo is available in + // CI), so this does NOT prove "the token authenticated where env would not". + // Instead it sends the credential header alongside ?path=/?branch= so M2 + // reads it and M3 threads it into an ISOLATED, per-request token-bearing + // clone (GIT_ASKPASS keeps the token off the URL/argv/.git/config; + // http.followRedirects is scoped to the source host) — then asserts the + // distinct subdir@branch prompt still resolves and the token never leaks. + // The point: prove the credential mechanism (GIT_ASKPASS / redirect config) + // does NOT break cloning the public fixture, end to end. It would catch a + // regression where, e.g., followRedirects=false blocks git's normal + // github.com -> codeload redirect and the clone silently yields no prompts. + test('GET /api/v1/prompts?path=&branch= with X-Dot-AI-Git-Token clones the subdir/branch prompt end-to-end without leaking the token (PRD #621 M3)', async () => { + if (!gitToken) { + console.log( + 'Skipping PRD #621 M3 token-clone e2e test: DOT_AI_GIT_TOKEN not set' + ); + return; + } + try { + await expectEventually(async () => { + const response = await integrationTest.httpClient.get( + `/api/v1/prompts?repo=${encodeURIComponent(promptsRepoUrl)}` + + `&path=${encodeURIComponent(fixtureSubdir)}` + + `&branch=${encodeURIComponent(fixtureBranch)}`, + // Real env token: guarantees the public clone succeeds (so a failure + // means the credential MECHANISM broke the clone, not bad auth). + { 'X-Dot-AI-Git-Token': gitToken } + ); + expect(response).toMatchObject({ success: true }); + // The distinct subdir@branch prompt resolves through the token-bearing + // isolated clone. + const fixturePrompt = response.data.prompts.find( + (p: { name: string }) => p.name === fixturePromptName + ); + expect(fixturePrompt).toMatchObject({ + name: fixturePromptName, + description: fixtureDescription, + }); + // The forwarded credential must never appear in the response surface + // (source, error, or anywhere). + expect(JSON.stringify(response)).not.toContain(gitToken); + }); + } finally { + await restoreEnvCache(); + } + // 300000ms: comprehensive e2e — real isolated token-bearing git clone + up + // to 5 expectEventually retries (matches the cache-refresh test convention). + }, 300000); + + test('POST /api/v1/prompts/:name?path=&branch= returns the subdir/branch prompt content (PRD #621)', async () => { + if (!gitToken) { + console.log( + 'Skipping PRD #621 path/branch get-by-name test: DOT_AI_GIT_TOKEN not set' + ); + return; + } + try { + await expectEventually(async () => { + const response = await integrationTest.httpClient.post( + `/api/v1/prompts/${fixturePromptName}` + + `?repo=${encodeURIComponent(promptsRepoUrl)}` + + `&path=${encodeURIComponent(fixtureSubdir)}` + + `&branch=${encodeURIComponent(fixtureBranch)}`, + {} + ); + // RED until M1: path/branch dropped → prompt not found → 400. + expect(response).toMatchObject({ + success: true, + data: { + description: fixtureDescription, + messages: [ + { + role: 'user', + content: { type: 'text', text: expect.any(String) }, + }, + ], + }, + }); + }); + } finally { + await restoreEnvCache(); + } + // 300000ms: comprehensive test — real git clone + expectEventually retries. + }, 300000); + + test('POST /api/v1/prompts/refresh honors path and branch body fields (PRD #621)', async () => { + if (!gitToken) { + console.log( + 'Skipping PRD #621 refresh body path/branch test: DOT_AI_GIT_TOKEN not set' + ); + return; + } + try { + await expectEventually(async () => { + // Control: same branch, a subdirectory that does NOT exist on it → + // built-in prompts only (no user prompts loaded). + const controlRes = await integrationTest.httpClient.post( + '/api/v1/prompts/refresh', + { + repo: promptsRepoUrl, + path: `${fixtureSubdir}-absent-${overrideRunId}`, + branch: fixtureBranch, + } + ); + expect(controlRes).toMatchObject({ + success: true, + data: { refreshed: true, promptsLoaded: expect.any(Number) }, + }); + + // Fixture: the subdirectory that DOES exist on the branch → + // built-in prompts + exactly the one fixture prompt. + const fixtureRes = await integrationTest.httpClient.post( + '/api/v1/prompts/refresh', + { + repo: promptsRepoUrl, + path: fixtureSubdir, + branch: fixtureBranch, + } + ); + expect(fixtureRes).toMatchObject({ + success: true, + data: { + refreshed: true, + promptsLoaded: expect.any(Number), + source: expect.stringContaining('dot-ai-test-prompts'), + }, + }); + + // RED until M1: body path/branch are dropped, so BOTH refreshes + // clone repo ROOT on MAIN and load the same count → the +1 from the + // fixture subdir never appears. + expect(fixtureRes.data.promptsLoaded).toBe( + controlRes.data.promptsLoaded + 1 + ); + }); + } finally { + await restoreEnvCache(); + } + // 300000ms: comprehensive test — real git clone + expectEventually retries. + }, 300000); + + test('GET /api/v1/prompts?path= returns 400 with scrubbed credentials and leaves the env cache intact (PRD #621)', async () => { + const secret = 'prd621_path_secret_xyz'; + const credUrl = `https://user:${secret}@github.com/vfarcic/dot-ai-test-prompts.git`; + + const before = await integrationTest.httpClient.get('/api/v1/prompts'); + expect(before).toMatchObject({ success: true }); + + // RED until M1: ?path= is dropped, the override validates on repoUrl + // alone, and the traversal subPath is never rejected (no 400). + const response = await integrationTest.httpClient.get( + `/api/v1/prompts?repo=${encodeURIComponent(credUrl)}` + + `&path=${encodeURIComponent('../../etc/passwd')}` + ); + expect(response).toMatchObject({ + success: false, + error: { code: 'VALIDATION_ERROR' }, + }); + // The embedded credential must never reach the response. + expect(JSON.stringify(response)).not.toContain(secret); + + // A request rejected BEFORE any clone must not corrupt the env-var cache: + // the env config still serves the same prompts and source afterwards. + const after = await integrationTest.httpClient.get('/api/v1/prompts'); + expect(after).toMatchObject({ + success: true, + data: { source: before.data.source }, + }); + // Scalar count comparison (toBe) — the env prompt count is unchanged after + // the rejected request; not an object shape, so toMatchObject does not apply. + expect(after.data.prompts.length).toBe(before.data.prompts.length); + // 120000ms (NOT the 300000ms comprehensive convention): this is a + // validation test — the override is rejected BEFORE any clone, so it is + // fast and deterministic and does not need the long comprehensive timeout. + }, 120000); + + test('POST /api/v1/prompts/refresh with invalid branch returns 400 with scrubbed credentials (PRD #621)', async () => { + const secret = 'prd621_branch_secret_xyz'; + const credUrl = `https://user:${secret}@github.com/vfarcic/dot-ai-test-prompts.git`; + + // RED until M1: body `branch` is dropped, so the illegal branch name is + // never validated and no 400 is returned. + const response = await integrationTest.httpClient.post( + '/api/v1/prompts/refresh', + { repo: credUrl, branch: 'bad branch name!!' } + ); + expect(response).toMatchObject({ + success: false, + error: { code: 'VALIDATION_ERROR' }, + }); + // Negative no-leak assertion — intentionally not toMatchObject (no object + // shape to match; we assert the credential is absent from the response). + expect(JSON.stringify(response)).not.toContain(secret); + // 120000ms (NOT the 300000ms comprehensive convention): validation test — + // rejected before any clone, fast and deterministic. + }, 120000); + }); + + // PRD #621 M2/M3/M4: the override gains an optional per-request git CREDENTIAL + // forwarded via the `X-Dot-AI-Git-Token` request header (M2 reads it + adds it + // to both CORS allowlists; M3 uses it for the clone, scoped to the source host, + // with cache isolation). M4 is the non-negotiable backward-compat parity guard. + // + // What IS robustly observable via black-box HTTP — and therefore lives here: + // - M2 CORS allowlist: an OPTIONS preflight advertises the new header + // (RED today — neither allowlist lists it yet). + // - M4 parity: a request with NO path/branch/header behaves like v1.21.0, + // for both the no-?repo= (env-var) path and the ?repo=-only path, and the + // credential header is never echoed back. NOTE: the header is truly inert + // ONLY on the no-?repo= path. When a ?repo= override IS present the token + // is NOT inert — M2 reads it and M3 threads it into an isolated, + // per-request token-bearing clone (GIT_ASKPASS) that bypasses the shared + // cache. It does not change the observed prompt set/source in the + // ?repo=-only test only because the asserted prompt is built-in and the + // public test-repo root carries no user prompts. + // + // What is NOT robustly observable here (flagged for the coder as UNIT tests — + // see the report; the #581 happy-path override coverage was pushed to unit + // tests for the same shared-server/shared-cache reasons): + // - Credential PRECEDENCE / AUTH (override.gitToken ?? env; clone auth + // against the source host): needs a private or second-auth-realm repo — + // a positive auth test is non-distinguishing because the env token can + // already read the env realm, and the only RED-distinguishing signal + // (auth failure => content ABSENT) is retry-unsafe and race-unsafe on the + // shared deployed server + single non-coordinate-keyed cache directory. + // - CACHE ISOLATION (token-bearing private clone not served to/from the + // shared unauthenticated slot; token absent from the cache key): the + // cross-serve window is unobservable/flaky black-box; unit tests can + // control cache state precisely (git boundary mocked). + // - CROSS-HOST REDIRECT non-forwarding (decision 3): needs a controllable + // redirecting git host — not available; unit-test the auth/redirect path. + // - LOG scrubbing (token absent from server logs): logs are not HTTP-observable. + describe('Per-request credential header + backward-compat parity (PRD #621 M2/M3/M4)', () => { + const promptsRepoUrl = + 'https://github.com/vfarcic/dot-ai-test-prompts.git'; + + // Retry an equality/presence assertion to absorb a transient concurrent + // re-clone of the shared cache directory (see the M1 cache/race note). Used + // only for assertions whose GREEN state is the STABLE state, so retrying can + // absorb a race but cannot mask a genuine M2/M3 regression (which would fail + // every attempt). + async function expectEventually( + fn: () => Promise, + attempts = 5, + delayMs = 1500 + ): Promise { + let lastError: unknown; + for (let i = 0; i < attempts; i++) { + try { + await fn(); + return; + } catch (error) { + lastError = error; + await new Promise(resolve => setTimeout(resolve, delayMs)); + } + } + throw lastError; + } + + async function restoreEnvCache(): Promise { + await integrationTest.httpClient.post('/api/v1/prompts/refresh', {}); + } + + // ---- M2: CORS allowlist (the one genuinely RED-today test in this block) ---- + test('OPTIONS /api/v1/prompts preflight advertises the X-Dot-AI-Git-Token header (PRD #621 M2)', async () => { + const baseUrl = process.env.MCP_BASE_URL || 'http://localhost:3456'; + // OPTIONS is answered (204) before auth and carries the CORS allowlist. + const res = await fetch(`${baseUrl}/api/v1/prompts`, { + method: 'OPTIONS', + headers: { + Origin: 'http://localhost', + 'Access-Control-Request-Method': 'GET', + 'Access-Control-Request-Headers': 'x-dot-ai-git-token', + }, + }); + const allowHeaders = res.headers.get('access-control-allow-headers'); + // These assert a single RESPONSE-HEADER string, not an object shape, so + // toMatchObject does not apply: toBeTruthy checks the header is present + // (CORS enabled and surfaced through the ingress), and toContain checks the + // allowlist string advertises the new credential header. + expect(allowHeaders).toBeTruthy(); + // RED until M2: the allowlist is "Content-Type, X-Session-Id, + // Authorization, X-Dot-AI-Authorization" (mcp.ts) / "Content-Type, + // Authorization" (rest-api.ts) — neither lists the new credential header. + expect((allowHeaders || '').toLowerCase()).toContain( + 'x-dot-ai-git-token' + ); + // 60000ms: a single fast OPTIONS preflight — no git clone, no retries — so + // the 300000ms comprehensive-test convention does not apply here. + }, 60000); + + // ---- M4 parity: no-?repo= (env-var) path is inert to the credential header ---- + test('no-override request behaves like v1.21.0 with the credential header present and never echoes it (PRD #621 M4 parity)', async () => { + const secret = 'prd621_norepo_header_secret_zzz'; + await expectEventually(async () => { + const withHeader = await integrationTest.httpClient.get( + '/api/v1/prompts', + { 'X-Dot-AI-Git-Token': secret } + ); + expect(withHeader).toMatchObject({ + success: true, + // Source is the env-var repo, unchanged by the header. + data: { source: expect.stringContaining('dot-ai-test-prompts') }, + }); + const names = withHeader.data.prompts.map( + (p: { name: string }) => p.name + ); + // Built-in AND env user prompts (loaded from the user-prompts/ subdir) + // are present => the env-var path is fully unaffected by the header. + expect(names).toContain('prd-create'); + expect(names).toContain('eval-run'); + // The forwarded credential must never appear in the response surface. + expect(JSON.stringify(withHeader)).not.toContain(secret); + }); + // 300000ms: comprehensive test — expectEventually retries against the + // shared env cache (matches the cache-refresh test convention). + }, 300000); + + // ---- M4 parity: ?repo=-only path matches PRD #581; the header is threaded + // into an isolated token clone but the observed outcome is unchanged + // here (built-in prompt + empty public repo root) ---- + test('?repo=-only request matches PRD #581 (root clone) with and without the credential header (PRD #621 M4 parity)', async () => { + const secret = 'prd621_repoonly_header_secret_zzz'; + try { + await expectEventually(async () => { + const res = await integrationTest.httpClient.get( + `/api/v1/prompts?repo=${encodeURIComponent(promptsRepoUrl)}` + ); + const resH = await integrationTest.httpClient.get( + `/api/v1/prompts?repo=${encodeURIComponent(promptsRepoUrl)}`, + { 'X-Dot-AI-Git-Token': secret } + ); + for (const r of [res, resH]) { + expect(r).toMatchObject({ + success: true, + data: { source: expect.stringContaining('dot-ai-test-prompts') }, + }); + const names = r.data.prompts.map((p: { name: string }) => p.name); + // The response object-shape is asserted with toMatchObject above + // (the convention). The two checks below intentionally use toContain + // because they assert ARRAY MEMBERSHIP, not object shape — toContain + // is the correct matcher here. ?repo=-only clones the repo ROOT on + // main (PRD #581 behavior): built-in prompts present, but env user + // prompts (which live under the user-prompts/ subdir) are NOT loaded. + expect(names).toContain('prd-create'); + expect(names).not.toContain('eval-run'); + } + // The header is NOT inert when a repo override is present: M2 reads it + // and M3 threads it into an isolated, per-request token-bearing clone. + // The prompt SET is identical here only because the asserted prompt is + // built-in and the public repo ROOT has no user prompts — so the + // token-bearing isolated clone and the unauthenticated clone surface + // the same result. This remains a valid parity guard (same observable + // result with or without the header). + const nameSet = (r: { data: { prompts: { name: string }[] } }) => + r.data.prompts.map(p => p.name).sort(); + // toEqual asserts EXACT equality of the two name arrays; toMatchObject + // would be the wrong tool (it does partial matching, not array equality). + expect(nameSet(resH)).toEqual(nameSet(res)); + // Negative no-leak assertion — intentionally NOT toMatchObject (there + // is no object shape to match; we assert the token is absent entirely). + expect(JSON.stringify(resH)).not.toContain(secret); + }); + } finally { + await restoreEnvCache(); + } + // 300000ms: comprehensive test — real git clone + expectEventually retries. + }, 300000); + }); }); diff --git a/tests/unit/core/git-utils-override-auth.test.ts b/tests/unit/core/git-utils-override-auth.test.ts new file mode 100644 index 00000000..522f5189 --- /dev/null +++ b/tests/unit/core/git-utils-override-auth.test.ts @@ -0,0 +1,340 @@ +/** + * Unit Tests: per-request override clone auth (PRD #621 M3, Decisions 3 & 4; + * remediation MEDIUM-2/MEDIUM-3/R-1 + the CI clone-failure fix) + * + * The token-bearing override clone spawns `git` DIRECTLY (not via simple-git, + * whose safety scanner rejected the GIT_ASKPASS / credential.helper wiring and + * aborted the clone). These tests pin: + * - buildOverrideCloneAuth returns a username-only URL (token NOT in the URL) + * scoped to the source host. + * - cloneRepo's token path puts the token NOWHERE on the git argv (it rides a + * HOST-BOUND GIT_ASKPASS helper in the child ENV) and the clone URL (which + * becomes .git/config) carries no token. + * - the GIT_ASKPASS helper is HOST-BOUND: it emits the token ONLY for the + * intended host, and nothing for a different/look-alike host (Decision 3). + * - the helper holds no secret, is executable, and is removed after the clone. + * - the env-credential path is unchanged (simple-git, no spawn, no askpass). + * + * child_process.spawn and simple-git are mocked so the decision is observed + * without performing a real clone. + */ + +import { + describe, + test, + expect, + beforeEach, + afterEach, + vi, +} from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { EventEmitter } from 'node:events'; +import { execFileSync } from 'node:child_process'; + +const { mockSpawn, mockClone, mockStatus } = vi.hoisted(() => ({ + mockSpawn: vi.fn(), + mockClone: vi.fn(), + mockStatus: vi.fn(), +})); + +vi.mock('node:child_process', async importOriginal => { + const actual = await importOriginal(); + return { ...actual, spawn: mockSpawn }; +}); + +vi.mock('simple-git', () => ({ + default: vi.fn(() => ({ clone: mockClone, status: mockStatus })), +})); + +import { + cloneRepo, + buildOverrideCloneAuth, + getAuthenticatedUrl, + ASKPASS_TOKEN_ENV, + ASKPASS_HOST_ENV, +} from '../../../src/core/git-utils'; + +const REPO = 'https://github.com/example-org/private.git'; +const TOKEN = 'per-call-secret-tok'; + +/** Fake child process that closes with `exitCode` on the next tick. */ +function makeChild(exitCode = 0, stderr = ''): EventEmitter { + const child = new EventEmitter() as EventEmitter & { + stderr: EventEmitter; + kill: () => void; + }; + child.stderr = new EventEmitter(); + child.kill = vi.fn(); + setImmediate(() => { + if (stderr) child.stderr.emit('data', Buffer.from(stderr)); + child.emit('close', exitCode); + }); + return child; +} + +/** Fake child the test drives manually (no auto-emit), so multiple settling + * events can be fired in a controlled order. */ +function makeManualChild(): EventEmitter & { + stderr: EventEmitter; + kill: ReturnType; +} { + const child = new EventEmitter() as EventEmitter & { + stderr: EventEmitter; + kill: ReturnType; + }; + child.stderr = new EventEmitter(); + child.kill = vi.fn(); + return child; +} + +describe('buildOverrideCloneAuth (PRD #621 M3 / Decision 3)', () => { + test('returns a username-only URL scoped to the source host (NO token in the URL)', () => { + const { cloneUrl, host } = buildOverrideCloneAuth(REPO); + const parsed = new URL(cloneUrl); + expect(parsed.host).toBe(new URL(REPO).host); + expect(host).toBe('github.com'); + // Only the (non-secret) username is present; the token is supplied via + // GIT_ASKPASS, never embedded here. + expect(parsed.username).toBe('x-access-token'); + expect(parsed.password).toBe(''); + }); + + test('preserves a non-default port in the host', () => { + const { cloneUrl, host } = buildOverrideCloneAuth( + 'https://forgejo.example.com:3000/org/repo.git' + ); + expect(host).toBe('forgejo.example.com:3000'); + expect(cloneUrl).toContain('x-access-token@forgejo.example.com:3000'); + }); +}); + +describe('cloneRepo per-call token (PRD #621 M3 / Decisions 3 & 4)', () => { + const savedToken = process.env.DOT_AI_GIT_TOKEN; + const savedAppEnabled = process.env.GITHUB_APP_ENABLED; + + let lastSpawn: + | { cmd: string; args: string[]; env: NodeJS.ProcessEnv } + | undefined; + let askpass: { scriptPath: string; content: string; mode: number } | undefined; + + beforeEach(() => { + lastSpawn = undefined; + askpass = undefined; + mockSpawn.mockReset(); + mockSpawn.mockImplementation( + (cmd: string, args: string[], options: { env: NodeJS.ProcessEnv }) => { + lastSpawn = { cmd, args, env: options.env }; + const scriptPath = options.env?.GIT_ASKPASS as string | undefined; + if (scriptPath && fs.existsSync(scriptPath)) { + askpass = { + scriptPath, + content: fs.readFileSync(scriptPath, 'utf8'), + mode: fs.statSync(scriptPath).mode, + }; + } + return makeChild(0); + } + ); + mockClone.mockReset(); + mockClone.mockResolvedValue(undefined); + mockStatus.mockReset(); + mockStatus.mockResolvedValue({ current: 'main' }); + delete process.env.DOT_AI_GIT_TOKEN; + delete process.env.GITHUB_APP_ENABLED; + }); + + afterEach(() => { + if (savedToken !== undefined) process.env.DOT_AI_GIT_TOKEN = savedToken; + else delete process.env.DOT_AI_GIT_TOKEN; + if (savedAppEnabled !== undefined) + process.env.GITHUB_APP_ENABLED = savedAppEnabled; + else delete process.env.GITHUB_APP_ENABLED; + }); + + test('spawns git directly (NOT simple-git) for a token clone', async () => { + await cloneRepo(REPO, '/tmp/clone-target', { + token: TOKEN, + branch: 'feature-x', + depth: 1, + }); + expect(mockSpawn).toHaveBeenCalledTimes(1); + expect(mockClone).not.toHaveBeenCalled(); + expect(lastSpawn!.cmd).toBe('git'); + }); + + test('the token is NOT on the git argv, and the clone URL has no token — MEDIUM-2/3', async () => { + await cloneRepo(REPO, '/tmp/clone-target', { + token: TOKEN, + branch: 'feature-x', + depth: 1, + }); + const { args } = lastSpawn!; + // The URL git is given (and persists as origin in .git/config) is + // username-only. + expect(args).toContain( + 'https://x-access-token@github.com/example-org/private.git' + ); + expect(args).toEqual( + expect.arrayContaining(['clone', '--branch', 'feature-x', '--depth', '1']) + ); + // Token appears NOWHERE on the argv. + expect(JSON.stringify(args)).not.toContain(TOKEN); + }); + + test('the token + intended host ride the child ENV via GIT_ASKPASS', async () => { + await cloneRepo(REPO, '/tmp/clone-target', { token: TOKEN }); + const env = lastSpawn!.env; + expect(env[ASKPASS_TOKEN_ENV]).toBe(TOKEN); + expect(env[ASKPASS_HOST_ENV]).toBe('github.com'); + expect(typeof env.GIT_ASKPASS).toBe('string'); + expect(env.GIT_TERMINAL_PROMPT).toBe('0'); + // Full process.env is carried through (PATH etc.). + expect(env.PATH ?? env.Path).toBeDefined(); + }); + + test('the GIT_ASKPASS helper holds NO secret, is executable, and is removed after the clone', async () => { + await cloneRepo(REPO, '/tmp/clone-target', { token: TOKEN }); + expect(askpass).toBeDefined(); + // Reads the token from the env var — does NOT contain the token. + expect(askpass!.content).toContain(ASKPASS_TOKEN_ENV); + expect(askpass!.content).not.toContain(TOKEN); + // Owner-executable. + expect(askpass!.mode & 0o100).toBe(0o100); + // Cleaned up once the clone finished. + expect(fs.existsSync(askpass!.scriptPath)).toBe(false); + }); + + // Decision 3, the core security property: the helper emits the token ONLY for + // the intended host. We execute the EXACT generated script against the + // credential prompts git would produce. + test('the GIT_ASKPASS helper is HOST-BOUND — token only for the intended host (Decision 3)', async () => { + await cloneRepo(REPO, '/tmp/clone-target', { token: TOKEN }); + const host = lastSpawn!.env[ASKPASS_HOST_ENV] as string; + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'askpass-hostbound-')); + const scriptPath = path.join(dir, 'askpass.sh'); + fs.writeFileSync(scriptPath, askpass!.content, { mode: 0o700 }); + const run = (prompt: string): string => + execFileSync(scriptPath, [prompt], { + env: { [ASKPASS_TOKEN_ENV]: TOKEN, [ASKPASS_HOST_ENV]: host }, + encoding: 'utf8', + }).trim(); + + try { + // Intended host → emits the token. + expect(run(`Password for 'https://x-access-token@${host}': `)).toBe( + TOKEN + ); + // Different host (e.g. after a cross-host redirect) → emits nothing. + expect(run(`Password for 'https://x-access-token@evil.test': `)).toBe(''); + // Look-alike suffix host → emits nothing (delimiter-bounded match). + expect( + run(`Password for 'https://x-access-token@${host}.evil.test': `) + ).toBe(''); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + test('the per-call token OVERRIDES the env credential (Decision 4)', async () => { + process.env.DOT_AI_GIT_TOKEN = 'env-tok-should-not-be-used'; + await cloneRepo(REPO, '/tmp/clone-target', { token: TOKEN }); + // Token path used (spawn), env path (simple-git) not used. + expect(mockSpawn).toHaveBeenCalledTimes(1); + expect(mockClone).not.toHaveBeenCalled(); + expect(JSON.stringify(lastSpawn!.args)).not.toContain( + 'env-tok-should-not-be-used' + ); + expect(lastSpawn!.env[ASKPASS_TOKEN_ENV]).toBe(TOKEN); + }); + + test('a non-zero git exit rejects with the (token-free) stderr', async () => { + mockSpawn.mockReset(); + mockSpawn.mockImplementation(() => + makeChild(128, 'fatal: Authentication failed') + ); + await expect( + cloneRepo(REPO, '/tmp/clone-target', { token: TOKEN }) + ).rejects.toThrow(/git clone exited with code 128/); + }); + + // CodeRabbit nitpick: the 'error', 'close', and timeout handlers can each try + // to settle the promise. The settled-guard must ensure the FIRST wins, later + // handlers are no-ops, and the timeout is cleared on settle (no dangling + // timer fires afterwards). + test('settles exactly once and clears the timeout when error then close both fire', async () => { + vi.useFakeTimers(); + try { + const child = makeManualChild(); + mockSpawn.mockReset(); + mockSpawn.mockImplementation(() => child); + + const p = cloneRepo(REPO, '/tmp/clone-target', { token: TOKEN }); + // Capture the outcome up front so the (impossible) double-settle can't + // surface as an unhandled rejection. + const outcome = p.then( + () => ({ status: 'resolved' as const }), + (e: Error) => ({ status: 'rejected' as const, error: e }) + ); + + // First settle: spawn 'error'. A later 'close' (process exit after the + // error) must be a NO-OP — it must NOT flip the result to the close-code + // error. + child.emit('error', new Error('spawn ENOENT')); + child.emit('close', 1); + + const result = await outcome; + expect(result.status).toBe('rejected'); + expect((result as { error: Error }).error.message).toBe('spawn ENOENT'); + + // The timeout was cleared on settle: advancing well past GIT_TIMEOUT_MS + // fires nothing (no SIGKILL from a dangling timer). + vi.advanceTimersByTime(10 * 60 * 1000); + expect(child.kill).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); +}); + +describe('cloneRepo env path — unchanged (no token)', () => { + const savedToken = process.env.DOT_AI_GIT_TOKEN; + const savedAppEnabled = process.env.GITHUB_APP_ENABLED; + + beforeEach(() => { + mockSpawn.mockReset(); + mockClone.mockReset(); + mockClone.mockResolvedValue(undefined); + mockStatus.mockReset(); + mockStatus.mockResolvedValue({ current: 'main' }); + delete process.env.DOT_AI_GIT_TOKEN; + delete process.env.GITHUB_APP_ENABLED; + }); + + afterEach(() => { + if (savedToken !== undefined) process.env.DOT_AI_GIT_TOKEN = savedToken; + else delete process.env.DOT_AI_GIT_TOKEN; + if (savedAppEnabled !== undefined) + process.env.GITHUB_APP_ENABLED = savedAppEnabled; + else delete process.env.GITHUB_APP_ENABLED; + }); + + test('env token → simple-git clone with the env-authenticated URL, NO spawn, NO askpass', async () => { + process.env.DOT_AI_GIT_TOKEN = 'env-tok'; + await cloneRepo(REPO, '/tmp/clone-target', { branch: 'main', depth: 1 }); + expect(mockSpawn).not.toHaveBeenCalled(); + expect(mockClone).toHaveBeenCalledTimes(1); + const [url] = mockClone.mock.calls[0]; + expect(url).toBe(getAuthenticatedUrl(REPO, 'env-tok')); + }); + + test('no token and no env auth clones unauthenticated (public repo, unchanged)', async () => { + await cloneRepo(REPO, '/tmp/clone-target', {}); + expect(mockSpawn).not.toHaveBeenCalled(); + expect(mockClone).toHaveBeenCalledTimes(1); + const [url] = mockClone.mock.calls[0]; + expect(url).toBe(REPO); + }); +}); diff --git a/tests/unit/core/user-prompts-loader.test.ts b/tests/unit/core/user-prompts-loader.test.ts index 470c199a..5cfa0785 100644 --- a/tests/unit/core/user-prompts-loader.test.ts +++ b/tests/unit/core/user-prompts-loader.test.ts @@ -43,11 +43,35 @@ vi.mock('../../../src/core/git-utils.js', async () => { }; }); +// fs is real for every test EXCEPT when fsMockState.failIsolatedRmSync is set, +// in which case rmSync on the per-request isolated dir throws — letting us +// exercise the cleanup-failure-warns path (MEDIUM-2). vitest can't spy on a +// built-in module's ESM named export, so we mock the module and delegate. +const fsMockState = vi.hoisted(() => ({ failIsolatedRmSync: false })); +vi.mock('fs', async () => { + const actual = await vi.importActual('fs'); + return { + ...actual, + default: actual, + rmSync: (p: import('fs').PathLike, opts?: import('fs').RmOptions) => { + if ( + fsMockState.failIsolatedRmSync && + String(p).includes('user-prompts-override-') + ) { + throw new Error('simulated rmSync failure'); + } + return actual.rmSync(p, opts); + }, + }; +}); + import { cloneRepo } from '../../../src/core/git-utils.js'; import { loadUserPrompts, clearUserPromptsCache, getUserPromptsCacheState, + getUserPromptsConfigFromOverride, + getCacheDirectory, } from '../../../src/core/user-prompts-loader.js'; import type { Logger } from '../../../src/core/error-handling.js'; @@ -392,4 +416,170 @@ describe('User Prompts Loader Override', () => { expect(JSON.stringify(outerErrors)).not.toContain(secret); }); }); + + // PRD #621 M2/M3: per-request credential header (override.gitToken) + + // clone-auth precedence + cache isolation (Decisions 2 & 4). + describe('Credential override (PRD #621 M2/M3)', () => { + // --- A: precedence / per-call token to cloneRepo --- + describe('token precedence (Decision 4)', () => { + test('getUserPromptsConfigFromOverride: override.gitToken wins over env', () => { + process.env.DOT_AI_GIT_TOKEN = 'env-token'; + const config = getUserPromptsConfigFromOverride({ + repoUrl: OVERRIDE_REPO, + gitToken: 'override-token', + }); + expect(config.gitToken).toBe('override-token'); + }); + + test('getUserPromptsConfigFromOverride: env used when no override token', () => { + process.env.DOT_AI_GIT_TOKEN = 'env-token'; + const config = getUserPromptsConfigFromOverride({ + repoUrl: OVERRIDE_REPO, + }); + expect(config.gitToken).toBe('env-token'); + }); + + test('the override token is the per-call token cloneRepo receives', async () => { + process.env.DOT_AI_GIT_TOKEN = 'env-token'; + await loadUserPrompts(makeCapturingLogger().logger, false, { + repoUrl: OVERRIDE_REPO, + gitToken: 'override-token', + }); + expect(cloneRepo).toHaveBeenCalledTimes(1); + // 3rd arg to cloneRepo is CloneOptions { branch, depth, token }. + expect(vi.mocked(cloneRepo).mock.calls[0][2]).toMatchObject({ + token: 'override-token', + }); + }); + + test('no override token → cloneRepo receives token undefined (env auth path)', async () => { + process.env.DOT_AI_GIT_TOKEN = 'env-token'; + await loadUserPrompts(makeCapturingLogger().logger, false, { + repoUrl: OVERRIDE_REPO, + }); + expect(vi.mocked(cloneRepo).mock.calls[0][2]).toMatchObject({ + token: undefined, + }); + }); + }); + + // --- B: cache isolation (Decision 2) --- + describe('cache isolation (Decision 2)', () => { + test('a token-bearing request clones in isolation and does NOT populate the shared cache', async () => { + const result = await loadUserPrompts( + makeCapturingLogger().logger, + false, + { repoUrl: OVERRIDE_REPO, gitToken: 'secret-token' } + ); + // The clone still produces prompts... + expect(result).toMatchObject([{ name: 'prd-581-test' }]); + // ...but the shared cacheState was never written (isolated path). + expect(getUserPromptsCacheState()).toBeNull(); + // The clone target was a throwaway dir, NOT the shared cache directory. + const targetDir = vi.mocked(cloneRepo).mock.calls[0][1]; + expect(targetDir).not.toBe(getCacheDirectory()); + expect(targetDir).toContain('user-prompts-override-'); + }); + + test('a later no-token request for the same coordinate is NOT served the token-bearing clone', async () => { + // 1. Token-bearing request (isolated, leaves shared cache untouched). + await loadUserPrompts(makeCapturingLogger().logger, false, { + repoUrl: OVERRIDE_REPO, + gitToken: 'secret-token', + }); + expect(cloneRepo).toHaveBeenCalledTimes(1); + expect(getUserPromptsCacheState()).toBeNull(); + + // 2. No-token request for the SAME coordinate must clone fresh into the + // shared slot (it cannot be served the private isolated clone). + await loadUserPrompts(makeCapturingLogger().logger, false, { + repoUrl: OVERRIDE_REPO, + }); + expect(cloneRepo).toHaveBeenCalledTimes(2); + expect(vi.mocked(cloneRepo).mock.calls[1][1]).toBe(getCacheDirectory()); + expect(vi.mocked(cloneRepo).mock.calls[1][2]).toMatchObject({ + token: undefined, + }); + expect(getUserPromptsCacheState()).toMatchObject({ + repoUrl: OVERRIDE_REPO, + }); + }); + + test('the cache key excludes the token (only coordinate fields are tracked)', async () => { + await loadUserPrompts(makeCapturingLogger().logger, false, { + repoUrl: OVERRIDE_REPO, + }); + const state = getUserPromptsCacheState(); + expect(state).not.toBeNull(); + expect(Object.keys(state!).sort()).toEqual([ + 'branch', + 'lastPullTime', + 'localPath', + 'repoUrl', + 'subPath', + ]); + }); + }); + + // --- D: log/error scrubbing --- + describe('credential scrubbing', () => { + test('a forwarded token is scrubbed from clone-error log output', async () => { + const secret = 'forwarded_tok_secret_99'; + vi.mocked(cloneRepo).mockReset(); + vi.mocked(cloneRepo).mockImplementation( + makeFailingClone( + `fatal: authentication failed for https://x-access-token:${secret}@github.com/example-org/private.git` + ) + ); + + const { logger, calls } = makeCapturingLogger(); + const result = await loadUserPrompts(logger, false, { + repoUrl: OVERRIDE_REPO, + gitToken: secret, + }); + + expect(result).toEqual([]); + expect(JSON.stringify(calls)).not.toContain(secret); + }); + }); + + // MEDIUM-2: a failed cleanup of the isolated clone must WARN (not swallow), + // so an rmSync failure is observable rather than silent. + describe('isolation cleanup (MEDIUM-2)', () => { + test('cleanup failure is warned, not swallowed', async () => { + fsMockState.failIsolatedRmSync = true; + const { logger, calls } = makeCapturingLogger(); + try { + const result = await loadUserPrompts(logger, false, { + repoUrl: OVERRIDE_REPO, + gitToken: 'tok-cleanup', + }); + // The clone+read still succeeded; only the cleanup failed. + expect(result).toMatchObject([{ name: 'prd-581-test' }]); + const warned = calls.some( + c => + c.level === 'warn' && + c.message === 'Failed to remove isolated clone directory' + ); + expect(warned).toBe(true); + } finally { + const cloneTarget = vi.mocked(cloneRepo).mock.calls.at(0)?.[1] as + | string + | undefined; + fsMockState.failIsolatedRmSync = false; + // Remove the dir the simulated failure left behind (rmSync now real). + if (cloneTarget) { + try { + fs.rmSync(path.dirname(cloneTarget), { + recursive: true, + force: true, + }); + } catch { + /* best-effort */ + } + } + } + }); + }); + }); }); diff --git a/tests/unit/interfaces/cors-headers.test.ts b/tests/unit/interfaces/cors-headers.test.ts new file mode 100644 index 00000000..7b709b46 --- /dev/null +++ b/tests/unit/interfaces/cors-headers.test.ts @@ -0,0 +1,64 @@ +/** + * Unit Tests: CORS allow-header parity (PRD #621 M2, Decision 1) + * + * The new X-Dot-AI-Git-Token credential header must be advertised by BOTH CORS + * preflight responses — the REST layer (rest-api.ts setCorsHeaders) and the + * front HTTP layer (mcp.ts). Historically these two allowlists were out of + * sync; cors-headers.ts is now the single source of truth for the credential + * header so they can never silently drift apart on it again. + * + * These constants are the EXACT strings emitted in the Access-Control-Allow- + * Headers response header by each layer (rest-api.ts and mcp.ts both import + * them), so asserting on them here is equivalent to asserting on the wire + * value without needing to stand up either server. + */ + +import { describe, test, expect } from 'vitest'; +import { + GIT_TOKEN_HEADER, + GIT_TOKEN_HEADER_LC, + REST_CORS_ALLOW_HEADERS, + MCP_CORS_ALLOW_HEADERS, +} from '../../../src/interfaces/cors-headers'; + +describe('CORS allow-header parity (PRD #621 M2)', () => { + test('the credential header name is X-Dot-AI-Git-Token', () => { + expect(GIT_TOKEN_HEADER).toBe('X-Dot-AI-Git-Token'); + }); + + test('the lowercased form matches Node req.headers keys', () => { + expect(GIT_TOKEN_HEADER_LC).toBe('x-dot-ai-git-token'); + }); + + test('REST allowlist (rest-api.ts setCorsHeaders) advertises the header', () => { + expect(REST_CORS_ALLOW_HEADERS).toContain(GIT_TOKEN_HEADER); + // Preserves the pre-existing REST headers. + expect(REST_CORS_ALLOW_HEADERS).toContain('Content-Type'); + expect(REST_CORS_ALLOW_HEADERS).toContain('Authorization'); + }); + + test('MCP allowlist (mcp.ts) advertises the header', () => { + expect(MCP_CORS_ALLOW_HEADERS).toContain(GIT_TOKEN_HEADER); + // Preserves the pre-existing MCP headers (which differ from REST by design). + expect(MCP_CORS_ALLOW_HEADERS).toContain('Content-Type'); + expect(MCP_CORS_ALLOW_HEADERS).toContain('X-Session-Id'); + expect(MCP_CORS_ALLOW_HEADERS).toContain('Authorization'); + expect(MCP_CORS_ALLOW_HEADERS).toContain('X-Dot-AI-Authorization'); + }); + + // CodeRabbit Finding 3: the MCP session router routes by the + // `mcp-session-id` request header, so the preflight allowlist must advertise + // it (case-insensitively) or browser MCP calls that send it would fail. + test('MCP allowlist advertises the session-routing header (Mcp-Session-Id)', () => { + expect(MCP_CORS_ALLOW_HEADERS).toContain('Mcp-Session-Id'); + expect(MCP_CORS_ALLOW_HEADERS.toLowerCase()).toContain('mcp-session-id'); + // X-Session-Id is retained for backward compat (additive change). + expect(MCP_CORS_ALLOW_HEADERS).toContain('X-Session-Id'); + }); + + test('BOTH allowlists include the credential header (parity, case-insensitive)', () => { + for (const list of [REST_CORS_ALLOW_HEADERS, MCP_CORS_ALLOW_HEADERS]) { + expect(list.toLowerCase()).toContain('x-dot-ai-git-token'); + } + }); +}); diff --git a/tests/unit/interfaces/header-redaction.test.ts b/tests/unit/interfaces/header-redaction.test.ts new file mode 100644 index 00000000..113e0656 --- /dev/null +++ b/tests/unit/interfaces/header-redaction.test.ts @@ -0,0 +1,75 @@ +/** + * Unit Tests: redactSensitiveHeaders (PRD #621 remediation HIGH-1) + * + * The front HTTP layer (mcp.ts) logs incoming requests at debug level. Logging + * req.headers verbatim leaked credential headers — Authorization, + * X-Dot-AI-Authorization, and (PRD #621) X-Dot-AI-Git-Token — violating the + * "token never appears in logs" success criterion. This pins the redaction. + */ + +import { describe, test, expect } from 'vitest'; +import { + redactSensitiveHeaders, + REDACTED_PLACEHOLDER, + SENSITIVE_HEADER_NAMES, +} from '../../../src/interfaces/header-redaction'; + +describe('redactSensitiveHeaders (PRD #621 HIGH-1)', () => { + test('redacts X-Dot-AI-Git-Token (the per-request credential)', () => { + const out = redactSensitiveHeaders({ + 'x-dot-ai-git-token': 'ghp_supersecret_value', + 'content-type': 'application/json', + }); + expect(out['x-dot-ai-git-token']).toBe(REDACTED_PLACEHOLDER); + // Non-sensitive headers are preserved verbatim. + expect(out['content-type']).toBe('application/json'); + expect(JSON.stringify(out)).not.toContain('ghp_supersecret_value'); + }); + + test('redacts Authorization and X-Dot-AI-Authorization', () => { + const out = redactSensitiveHeaders({ + authorization: 'Bearer secret-jwt-aaa', + 'x-dot-ai-authorization': 'Bearer secret-jwt-bbb', + }); + expect(out.authorization).toBe(REDACTED_PLACEHOLDER); + expect(out['x-dot-ai-authorization']).toBe(REDACTED_PLACEHOLDER); + const serialized = JSON.stringify(out); + expect(serialized).not.toContain('secret-jwt-aaa'); + expect(serialized).not.toContain('secret-jwt-bbb'); + }); + + test('is case-insensitive on header names', () => { + const out = redactSensitiveHeaders({ + 'X-Dot-AI-Git-Token': 'MixedCaseSecret', + Authorization: 'Bearer UpperSecret', + }); + expect(out['X-Dot-AI-Git-Token']).toBe(REDACTED_PLACEHOLDER); + expect(out.Authorization).toBe(REDACTED_PLACEHOLDER); + expect(JSON.stringify(out)).not.toContain('Secret'); + }); + + test('preserves non-sensitive headers and does not mutate the input', () => { + const input = { + 'content-type': 'application/json', + 'x-session-id': 'sess-123', + 'user-agent': 'curl/8', + authorization: 'Bearer keep-out', + }; + const out = redactSensitiveHeaders(input); + expect(out['content-type']).toBe('application/json'); + expect(out['x-session-id']).toBe('sess-123'); + expect(out['user-agent']).toBe('curl/8'); + // Input object is untouched. + expect(input.authorization).toBe('Bearer keep-out'); + }); + + test('handles undefined headers', () => { + expect(redactSensitiveHeaders(undefined)).toEqual({}); + }); + + test('the sensitive set includes the credential headers', () => { + expect(SENSITIVE_HEADER_NAMES.has('x-dot-ai-git-token')).toBe(true); + expect(SENSITIVE_HEADER_NAMES.has('authorization')).toBe(true); + expect(SENSITIVE_HEADER_NAMES.has('x-dot-ai-authorization')).toBe(true); + }); +}); diff --git a/tests/unit/interfaces/rest-api-prompts-override.test.ts b/tests/unit/interfaces/rest-api-prompts-override.test.ts new file mode 100644 index 00000000..26e76c9d --- /dev/null +++ b/tests/unit/interfaces/rest-api-prompts-override.test.ts @@ -0,0 +1,310 @@ +/** + * Unit Tests: extractPromptsOverride (PRD #581 `repo`, PRD #621 M1 `path`/`branch`) + * + * extractPromptsOverride is the single place the REST prompts handlers turn a + * request's `repo` / `path` / `branch` inputs into a UserPromptsOverride. The + * three handlers (GET list, POST :name, POST /refresh) all delegate to it, so + * pinning its behavior here covers the handler wiring without needing a live + * cluster (the integration tests in tests/integration/tools/prompts.test.ts + * exercise the end-to-end clone path). + * + * The contract this file pins: + * - PRD #581: `repo` threads into override.repoUrl; non-string / bad-scheme + * inputs are rejected with a credential-scrubbed 400. + * - PRD #621 M1: `path` → override.subPath, `branch` → override.branch, both + * optional and additive. Omitting them is byte-identical to PRD #581 + * (override carries `repoUrl` only — the non-negotiable backward-compat + * guarantee). Invalid path/branch are rejected with a scrubbed 400 BEFORE + * any clone/cache mutation. + */ + +import { describe, test, expect } from 'vitest'; +import { extractPromptsOverride } from '../../../src/interfaces/rest-api'; + +const REPO = 'https://github.com/vfarcic/dot-ai-test-prompts.git'; + +describe('extractPromptsOverride', () => { + describe('no override (no repo supplied)', () => { + test('undefined repo → ok with undefined override', () => { + const result = extractPromptsOverride(undefined); + expect(result).toEqual({ ok: true, override: undefined }); + }); + + test('null repo (searchParams.get miss) → ok with undefined override', () => { + const result = extractPromptsOverride(null); + expect(result).toEqual({ ok: true, override: undefined }); + }); + + test('empty/whitespace repo → ok with undefined override', () => { + expect(extractPromptsOverride('')).toEqual({ + ok: true, + override: undefined, + }); + expect(extractPromptsOverride(' ')).toEqual({ + ok: true, + override: undefined, + }); + }); + + test('path/branch are ignored when no repo is supplied', () => { + // path/branch only qualify an override; without a repo there is no + // override, so they must not produce one (env-var path stays in charge). + const result = extractPromptsOverride(null, 'skills', 'feature'); + expect(result).toEqual({ ok: true, override: undefined }); + }); + }); + + describe('backward compatibility — repo only (PRD #581 parity)', () => { + test('repo only → override carries repoUrl ONLY (no subPath, no branch)', () => { + const result = extractPromptsOverride(REPO); + // Byte-identical to PRD #581: exactly { repoUrl } and nothing else. + expect(result).toEqual({ ok: true, override: { repoUrl: REPO } }); + }); + + test('repo is trimmed', () => { + const result = extractPromptsOverride(` ${REPO} `); + expect(result).toEqual({ ok: true, override: { repoUrl: REPO } }); + }); + + test('empty path/branch alongside repo do not add keys (still repoUrl only)', () => { + // An explicit empty `?path=`/`?branch=` must be treated as absent so the + // downstream defaults ('' / 'main') are preserved and an empty-string + // branch never reaches isValidGitBranch. + const result = extractPromptsOverride(REPO, '', ''); + expect(result).toEqual({ ok: true, override: { repoUrl: REPO } }); + }); + + test('whitespace-only path/branch are treated as absent', () => { + const result = extractPromptsOverride(REPO, ' ', ' '); + expect(result).toEqual({ ok: true, override: { repoUrl: REPO } }); + }); + + test('null path/branch (searchParams.get miss) → repoUrl only', () => { + const result = extractPromptsOverride(REPO, null, null); + expect(result).toEqual({ ok: true, override: { repoUrl: REPO } }); + }); + }); + + describe('path threading (PRD #621 M1)', () => { + test('path → override.subPath', () => { + const result = extractPromptsOverride(REPO, 'skills'); + expect(result).toEqual({ + ok: true, + override: { repoUrl: REPO, subPath: 'skills' }, + }); + }); + + test('nested path is preserved', () => { + const result = extractPromptsOverride(REPO, 'a/b/c'); + expect(result).toMatchObject({ + ok: true, + override: { repoUrl: REPO, subPath: 'a/b/c' }, + }); + }); + + test('path is trimmed', () => { + const result = extractPromptsOverride(REPO, ' skills '); + expect(result).toMatchObject({ + ok: true, + override: { repoUrl: REPO, subPath: 'skills' }, + }); + }); + }); + + describe('branch threading (PRD #621 M1)', () => { + test('branch → override.branch', () => { + const result = extractPromptsOverride(REPO, undefined, 'my-feature'); + expect(result).toEqual({ + ok: true, + override: { repoUrl: REPO, branch: 'my-feature' }, + }); + }); + + test('branch with slashes/dots is preserved', () => { + const result = extractPromptsOverride( + REPO, + undefined, + 'release/v1.2.3' + ); + expect(result).toMatchObject({ + ok: true, + override: { repoUrl: REPO, branch: 'release/v1.2.3' }, + }); + }); + + test('branch is trimmed', () => { + const result = extractPromptsOverride(REPO, undefined, ' main '); + expect(result).toMatchObject({ + ok: true, + override: { repoUrl: REPO, branch: 'main' }, + }); + }); + }); + + describe('path + branch together', () => { + test('both thread into the override', () => { + const result = extractPromptsOverride(REPO, 'prd621-skills', 'fixture'); + expect(result).toEqual({ + ok: true, + override: { + repoUrl: REPO, + subPath: 'prd621-skills', + branch: 'fixture', + }, + }); + }); + }); + + describe('type validation (avoid 500 on malformed body)', () => { + test('non-string repo (array) → 400', () => { + const result = extractPromptsOverride(['a', 'b']); + expect(result).toEqual({ + ok: false, + message: 'repo must be a string (got array)', + }); + }); + + test('non-string repo (number) → 400', () => { + const result = extractPromptsOverride(42); + expect(result).toEqual({ + ok: false, + message: 'repo must be a string (got number)', + }); + }); + + test('non-string path (array) → 400', () => { + const result = extractPromptsOverride(REPO, ['skills', 'other']); + expect(result).toEqual({ + ok: false, + message: 'path must be a string (got array)', + }); + }); + + test('non-string path (number) → 400', () => { + const result = extractPromptsOverride(REPO, 7); + expect(result).toEqual({ + ok: false, + message: 'path must be a string (got number)', + }); + }); + + test('non-string branch (array) → 400', () => { + const result = extractPromptsOverride(REPO, undefined, ['a', 'b']); + expect(result).toEqual({ + ok: false, + message: 'branch must be a string (got array)', + }); + }); + + test('non-string branch (object) → 400', () => { + const result = extractPromptsOverride(REPO, undefined, { x: 1 }); + expect(result).toEqual({ + ok: false, + message: 'branch must be a string (got object)', + }); + }); + }); + + describe('downstream validation (rejected before any clone)', () => { + test('bad repo scheme → 400 mentioning scheme', () => { + const result = extractPromptsOverride( + 'ssh://git@github.com/example/repo.git' + ); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.message).toContain('scheme'); + } + }); + + test('path traversal → 400 mentioning subPath', () => { + const result = extractPromptsOverride(REPO, '../../etc/passwd'); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.message).toContain('subPath'); + } + }); + + test('invalid branch name → 400 mentioning branch', () => { + const result = extractPromptsOverride( + REPO, + undefined, + 'bad branch name!!' + ); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.message).toContain('branch'); + } + }); + }); + + describe('credential header threading (PRD #621 M2)', () => { + test('gitToken → override.gitToken when a repo override is present', () => { + const result = extractPromptsOverride(REPO, undefined, undefined, 'tok123'); + expect(result).toEqual({ + ok: true, + override: { repoUrl: REPO, gitToken: 'tok123' }, + }); + }); + + test('gitToken threads alongside path and branch', () => { + const result = extractPromptsOverride(REPO, 'skills', 'feat', 'tok123'); + expect(result).toEqual({ + ok: true, + override: { + repoUrl: REPO, + subPath: 'skills', + branch: 'feat', + gitToken: 'tok123', + }, + }); + }); + + test('absent gitToken → no gitToken key (repoUrl only)', () => { + const result = extractPromptsOverride(REPO, undefined, undefined, undefined); + expect(result).toEqual({ ok: true, override: { repoUrl: REPO } }); + }); + + // Header-inert-without-override (PRD #621 M2/M4, test F): a forwarded token + // with NO ?repo= override must NOT create an override — the env-var path is + // untouched and the token is dropped before it is ever read. + test('gitToken with no repo (null) → override undefined (header is inert)', () => { + const result = extractPromptsOverride(null, undefined, undefined, 'tok123'); + expect(result).toEqual({ ok: true, override: undefined }); + }); + + test('gitToken with empty repo → override undefined (header is inert)', () => { + const result = extractPromptsOverride(' ', undefined, undefined, 'tok123'); + expect(result).toEqual({ ok: true, override: undefined }); + }); + }); + + describe('credential scrubbing on validation failure (HARD CONSTRAINT 3)', () => { + const secret = 'unit_secret_token_xyz'; + + test('bad scheme with embedded credential → secret not echoed', () => { + const result = extractPromptsOverride( + `ssh://user:${secret}@github.com/example/repo.git` + ); + expect(result.ok).toBe(false); + expect(JSON.stringify(result)).not.toContain(secret); + }); + + test('invalid path with credential in repoUrl → secret not echoed', () => { + const credUrl = `https://user:${secret}@github.com/vfarcic/dot-ai-test-prompts.git`; + const result = extractPromptsOverride(credUrl, '../../etc/passwd'); + expect(result.ok).toBe(false); + expect(JSON.stringify(result)).not.toContain(secret); + }); + + test('invalid branch with credential in repoUrl → secret not echoed', () => { + const credUrl = `https://user:${secret}@github.com/vfarcic/dot-ai-test-prompts.git`; + const result = extractPromptsOverride( + credUrl, + undefined, + 'bad branch name!!' + ); + expect(result.ok).toBe(false); + expect(JSON.stringify(result)).not.toContain(secret); + }); + }); +}); diff --git a/tests/unit/interfaces/rest-api-url-scrubbing.test.ts b/tests/unit/interfaces/rest-api-url-scrubbing.test.ts index 21bd3aa6..0e03bcb7 100644 --- a/tests/unit/interfaces/rest-api-url-scrubbing.test.ts +++ b/tests/unit/interfaces/rest-api-url-scrubbing.test.ts @@ -84,6 +84,19 @@ describe('sanitizeRequestUrlForLogging (PRD #581 F3)', () => { ); }); + // CodeRabbit Finding 1: mcp.ts logs `req.url` through this helper at debug + // level. A raw (unencoded) credential-bearing `?repo=` value — exactly what + // arrives on `req.url` — must be scrubbed so the embedded token never reaches + // the log. PRD #621: "token never appears in logs". + test('scrubs a raw credential-bearing ?repo= req.url (mcp debug-log path)', () => { + const url = '/api/v1/prompts?repo=https://user:s3cret-tok@host/repo'; + const result = sanitizeRequestUrlForLogging(url); + expect(result).toBeDefined(); + expect(result).not.toContain('s3cret-tok'); + expect(result).not.toContain('user:s3cret-tok'); + expect(decodeURIComponent(result as string)).toContain('***@host/repo'); + }); + test('preserves other query params alongside the `repo` rewrite', () => { const url = '/api/v1/prompts?other=v1&repo=https%3A%2F%2Fu%3At%40h%2Fr&another=v2';