diff --git a/CHANGELOG.md b/CHANGELOG.md index 65ac72ee3..293a150a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,57 @@ All notable changes to GBrain will be documented in this file. +## [0.26.9] - 2026-05-04 + +## **OAuth 2.1 hardening + closes an HTTP MCP shell-job RCE.** +## **Anyone running `gbrain serve --http` should upgrade. The same write-scoped token your agent uses to call `put_page` could submit `shell` jobs and execute commands on the gbrain host.** + +The HTTP MCP transport's request-handler context literal was missing one field — `remote: true` — and that one field was the difference between a trusted local CLI write and an untrusted agent-facing write. With it absent, `operations.ts:1391`'s protected-job-name guard saw a falsy undefined and skipped. An HTTP MCP caller with a `read+write` OAuth token could submit `submit_job {name: "shell", params: {cmd: "id"}}` and own the host. Stdio MCP set the field correctly via `src/mcp/dispatch.ts:61`; HTTP inlined a parallel context-builder for several releases and lost it. + +Fix is two-layered. **F7** (`serve-http.ts`) sets `remote: true` explicitly. **F7b** flips the four trust-boundary call sites in `operations.ts` from falsy-default to `ctx.remote !== false` — anything that isn't strictly `false` now treats the caller as remote/untrusted, so a future transport that forgets the field fails closed instead of fails open. **D12** makes `OperationContext.remote` REQUIRED in the TypeScript type so the compiler is the first line of defense; the runtime fail-closed defaults are belt+suspenders for `as` casts and `Partial<>` spreads. + +The OAuth provider in `src/core/oauth-provider.ts` got a parallel hardening pass on the same release. **F1+F2** fold `client_id` atomically into the auth-code and refresh-token DELETE WHERE clauses (RFC 6749 §10.5 + §10.4 — pre-fix the post-hoc client compare burned the row on wrong-client paths so the legitimate client couldn't retry). **F3** enforces the refresh-scope-subset rule against the original grant on the row, not the client's currently-allowed scopes (RFC 6749 §6). **F4** binds `client_id` on `revokeToken` so a client can only revoke its own tokens (RFC 7009 §2.1). **F7c** validates `redirect_uri` on `/token` against the value stored at `/authorize` (RFC 6749 §4.1.3 — eva-brain missed this; codex caught it). **F5** swaps bare `catch {}` for SQLSTATE 42703 column-existence probes so lock timeouts and network blips no longer ride through as "column missing." **F6** fixes `sweepExpiredTokens` to actually return a count. + +`mcp_request_log` and the admin SSE feed now redact request payloads by default. The new `summarizeMcpParams` helper at `src/mcp/dispatch.ts` intersects submitted keys against the operation's declared `params` allow-list — declared keys preserve for debug visibility, unknown keys are counted but never named (closes the attacker-controlled-key-name leak surface). Byte counts get bucketed to 1KB so attackers can't binary-search secret-content sizes via repeated probes. Operators on their own laptops who want full payload visibility can flip on `--log-full-params` with a loud stderr warning. + +Smaller hardening: admin cookies set `Secure` when behind HTTPS or a public-URL proxy (F9), magic-link nonces are bounded by an LRU cap (F10), `/mcp` wraps `transport.handleRequest` in try/catch so SDK throws hit a JSON-RPC 500 instead of express's default HTML error page (F14), and OperationError + unexpected exceptions both route through the unified `buildError`/`serializeError` envelope (F15). DCR disable became a constructor option on the provider rather than a serve-http monkey-patch (F12 — cleanup, not security). + +To take advantage of v0.26.9 +============================ + +`gbrain upgrade` is a one-step upgrade. There is no migration; all changes are application-layer. + +1. **Upgrade.** `gbrain upgrade`. Confirm `gbrain --version` shows `0.26.9`. + +2. **If you run `gbrain serve --http`,** restart the process. The trust-boundary fix is in the request handler, so it takes effect on the next listen. + +3. **If you run a multi-tenant deployment** (operators other than you have admin / register-client access), audit existing OAuth clients for unexpected redirect_uris with `gbrain auth list-clients` (when you wire the helper) or by inspecting `oauth_clients.redirect_uris` directly. The hardening doesn't touch existing clients; it only constrains future redemptions. + +4. **If your dashboard or scripts read `mcp_request_log.params`,** note the schema shift. The default shape is now `{redacted: true, kind, declared_keys, unknown_key_count, approx_bytes}` (declared_keys is sorted; approx_bytes rounds up to nearest KB). Pass `--log-full-params` to `gbrain serve --http` to opt back into raw payloads with a startup warning. + +5. **If anything breaks,** please file an issue: https://github.com/garrytan/gbrain/issues + +Thanks to @ElectricSheepIO on X for the security review that surfaced this hardening pass. + +### Itemized changes + +- `src/core/oauth-provider.ts` — atomic client_id binding (F1, F2, F4), redirect_uri validation on exchange (F7c), refresh scope subset (F3), `isUndefinedColumnError` column probes (F5), `sweepExpiredTokens` correct count via `RETURNING 1` (F6), `dcrDisabled` constructor option (F12). +- `src/core/operations.ts` — `OperationContext.remote` becomes required (D12). Four sites flipped to `ctx.remote !== false` / `=== false` for fail-closed semantics (F7b). +- `src/commands/serve-http.ts` — explicit `remote: true` on /mcp context (F7), `summarizeMcpParams` wired into `mcp_request_log` + SSE feed by default (F8), `--log-full-params` opt-in, cookie `secure` flag (F9), bounded magic-link nonce LRU (F10), try/catch wrap on transport.handleRequest (F14), unified error envelope via `buildError`/`serializeError` (F15), DCR disable via provider constructor (F12). +- `src/commands/serve.ts` — `--log-full-params` argv flag with stderr warning at startup. +- `src/mcp/dispatch.ts` — new `summarizeMcpParams(opName, params)` helper. Returns `{redacted, kind, declared_keys, unknown_key_count, approx_bytes}` with allow-list intersection and 1KB bucketing. +- `src/core/utils.ts` — extracted `isUndefinedColumnError` predicate (D14, reusable). +- `test/oauth.test.ts` — 14 new cases pinning F1/F2/F3/F4/F5/F6/F7c/F12 invariants, including the empty-string redirect_uri bypass test from the adversarial-review pass. +- `test/mcp-dispatch-summarize.test.ts` — 7 cases pinning F8 redaction invariants including the attacker-key-name probe and the 1KB bucket assertion. +- `test/trust-boundary-contract.test.ts` — 4 cases pinning F7b fail-closed semantics under cast bypass. +- `test/e2e/serve-http-oauth.test.ts` — 2 new E2E regressions for shell-job and subagent-job submission rejection over HTTP MCP. +- `test/e2e/graph-quality.test.ts` — adds explicit `remote: false` to the test fixture (D13 audit follow-through). + +### For contributors + +- `test/oauth.test.ts` now uses the F1/F4 cross-client isolation pattern: a wrong-client attempt must reject AND the rightful owner must still succeed atomically afterward. Apply the same shape when adding tests around the OAuth provider's predicate-bound DELETEs. +- `summarizeMcpParams` is the canonical privacy-preserving redactor. New code paths that log MCP request shapes should route through it rather than inlining `JSON.stringify(params)`. + ## [0.26.8] - 2026-05-04 ## **Every gbrain brain becomes secure by default on upgrade. No public table without RLS, ever.** diff --git a/CLAUDE.md b/CLAUDE.md index e79ce5ab7..24172bfdc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,13 +40,13 @@ strict behavior when unset. ## Key files -- `src/core/operations.ts` — Contract-first operation definitions (the foundation). Also exports upload validators: `validateUploadPath`, `validatePageSlug`, `validateFilename`, plus `matchesSlugAllowList(slug, prefixes)` (v0.23 glob matcher: `/*` matches recursive children; bare `` matches exact only). `OperationContext.remote` flags untrusted callers; `OperationContext.allowedSlugPrefixes` (v0.23) is the trusted-workspace allow-list set by the dream cycle. `put_page` enforces: when `viaSubagent` and `allowedSlugPrefixes` is set, slug must match the allow-list; else the legacy `wiki/agents//...` namespace check applies. Auto-link enabled for trusted-workspace writes (skipped only when `remote=true && !trustedWorkspace`). As of v0.26.0, every `Operation` also carries `scope?: 'read' | 'write' | 'admin'` + `localOnly?: boolean`. All ops are annotated; `sync_brain`, `file_upload`, `file_list`, and `file_url` are `admin + localOnly` (rejected over HTTP). `OperationContext.auth?: AuthInfo` is threaded through HTTP dispatch for scope enforcement in `serve-http.ts` before the op runs. +- `src/core/operations.ts` — Contract-first operation definitions (the foundation). Also exports upload validators: `validateUploadPath`, `validatePageSlug`, `validateFilename`, plus `matchesSlugAllowList(slug, prefixes)` (v0.23 glob matcher: `/*` matches recursive children; bare `` matches exact only). `OperationContext.remote` flags untrusted callers; `OperationContext.allowedSlugPrefixes` (v0.23) is the trusted-workspace allow-list set by the dream cycle. `put_page` enforces: when `viaSubagent` and `allowedSlugPrefixes` is set, slug must match the allow-list; else the legacy `wiki/agents//...` namespace check applies. Auto-link enabled for trusted-workspace writes (skipped only when `remote=true && !trustedWorkspace`). As of v0.26.0, every `Operation` also carries `scope?: 'read' | 'write' | 'admin'` + `localOnly?: boolean`. All ops are annotated; `sync_brain`, `file_upload`, `file_list`, and `file_url` are `admin + localOnly` (rejected over HTTP). `OperationContext.auth?: AuthInfo` is threaded through HTTP dispatch for scope enforcement in `serve-http.ts` before the op runs. **v0.26.9 (D12 + F7b):** `OperationContext.remote` is now a REQUIRED field in the TypeScript type — the compiler is the first defense against transports that forget to set it. Four trust-boundary call sites (`put_page` allowlist, file_upload trust-narrowing, submit_job protected-name guard, auto-link skip) flipped from falsy-default (`!ctx.remote`) to fail-closed semantics (`ctx.remote === false` for "trusted-only" sites and `ctx.remote !== false` for "untrust unless explicit-false"). Anything that isn't strictly `false` is now treated as remote. Closed an HTTP MCP shell-job RCE: a `read+write`-scoped OAuth token could submit `shell` jobs because the HTTP request handler's literal context skipped `remote: true` and `submit_job`'s protected-name guard saw a falsy undefined. Stdio MCP set the field correctly via dispatch.ts; HTTP inlined a parallel context-builder for several releases and lost it. - `src/core/engine.ts` — Pluggable engine interface (BrainEngine). `clampSearchLimit(limit, default, cap)` takes an explicit cap so per-operation caps can be tighter than `MAX_SEARCH_LIMIT`. Exports `LinkBatchInput` / `TimelineBatchInput` for the v0.12.1 bulk-insert API (`addLinksBatch` / `addTimelineEntriesBatch`). As of v0.13.1, `BrainEngine` has a `readonly kind: 'postgres' | 'pglite'` discriminator so migrations (`src/core/migrate.ts`) and other consumers can branch on engine without `instanceof` + dynamic imports. - `src/core/engine-factory.ts` — Engine factory with dynamic imports (`'pglite'` | `'postgres'`) - `src/core/pglite-engine.ts` — PGLite (embedded Postgres 17.5 via WASM) implementation, all 40 BrainEngine methods. `addLinksBatch` / `addTimelineEntriesBatch` use multi-row `unnest()` with manual `$N` placeholders. As of v0.13.1, `connect()` wraps `PGlite.create()` in a try/catch that emits an actionable error naming the macOS 26.3 WASM bug (#223) and pointing at `gbrain doctor`; the lock is released on failure so the next process can retry cleanly. v0.22.0: `searchKeyword` and `searchKeywordChunks` multiply `ts_rank` by the source-factor CASE expression at the chunk-grain level; `searchVector` becomes a two-stage CTE — inner CTE keeps `ORDER BY cc.embedding <=> vec` so HNSW stays usable, outer SELECT re-ranks by `raw_score * source_factor`. Inner LIMIT scales with offset to preserve pagination contract. As of v0.22.6.1, `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL — probes for the specific forward-referenced state the embedded schema blob needs (`pages.source_id`, `links.link_source`, `links.origin_page_id`, `content_chunks.symbol_name`, `content_chunks.language`, `sources` FK target table) and adds only what's missing. Closes the upgrade-wedge bug class that bit users 10+ times across 6 schema versions over 2 years (#239/#243/#266/#357/#366/#374/#375/#378/#395/#396). No-op on fresh installs and modern brains. - `src/core/pglite-schema.ts` — PGLite-specific DDL (pgvector, pg_trgm, triggers) - `src/core/postgres-engine.ts` — Postgres + pgvector implementation (Supabase / self-hosted). `addLinksBatch` / `addTimelineEntriesBatch` use `INSERT ... SELECT FROM unnest($1::text[], ...) JOIN pages ON CONFLICT DO NOTHING RETURNING 1` — 4-5 array params regardless of batch size, sidesteps the 65535-parameter cap. As of v0.12.3, `searchKeyword` / `searchVector` scope `statement_timeout` via `sql.begin` + `SET LOCAL` so the GUC dies with the transaction instead of leaking across the pooled postgres.js connection (contributed by @garagon). `getEmbeddingsByChunkIds` uses `tryParseEmbedding` so one corrupt row skips+warns instead of killing the query. v0.22.0: `searchKeyword`, `searchKeywordChunks`, and `searchVector` apply source-aware ranking by inlining the source-factor CASE and `NOT (col LIKE …)` hard-exclude clause from `src/core/search/sql-ranking.ts`. `searchVector` switches to a two-stage CTE (HNSW-safe inner ORDER BY, source-boost re-rank in the outer SELECT) and carries `p.source_id` through inner→outer for v0.18 multi-source callers. v0.22.1 (#406): `_savedConfig` retains the connect config; `reconnect()` tears down + recreates the pool from saved config (called by supervisor watchdog after 3 consecutive health-check failures). `executeRaw` is a single-statement passthrough — no per-call retry (D3 dropped that as unsound for non-idempotent statements; recovery is supervisor-driven). v0.22.1 (#363, contributed by @orendi84): `connect()` applies `resolveSessionTimeouts()` from `db.ts` as connection-time startup parameters (`statement_timeout`, `idle_in_transaction_session_timeout`) so orphan pgbouncer backends can't hold locks for hours. v0.22.1 (#409, contributed by @atrevino47): `countStaleChunks()` + `listStaleChunks()` server-side-filter on `embedding IS NULL` for `embed --stale`, eliminating ~76 MB/call client-side pull on a fully-embedded brain; `upsertChunks()` resets both `embedding` AND `embedded_at` to NULL when chunk_text changes without a new embedding (consistency). As of v0.22.6.1, `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL on the same forward-reference probe set as the PGLite engine, so old Postgres brains pinned at v0.13/v0.18/v0.19 walk forward cleanly instead of wedging on `column "..." does not exist`. -- `src/core/utils.ts` — Shared SQL utilities extracted from postgres-engine.ts. Exports `parseEmbedding(value)` (throws on unknown input, used by migration + ingest paths where data integrity matters) and as of v0.12.3 `tryParseEmbedding(value)` (returns `null` + warns once per process, used by search/rescore paths where availability matters more than strictness). +- `src/core/utils.ts` — Shared SQL utilities extracted from postgres-engine.ts. Exports `parseEmbedding(value)` (throws on unknown input, used by migration + ingest paths where data integrity matters) and as of v0.12.3 `tryParseEmbedding(value)` (returns `null` + warns once per process, used by search/rescore paths where availability matters more than strictness). **v0.26.9 (D14):** adds `isUndefinedColumnError(err)` predicate — pattern-matches Postgres SQLSTATE 42703 / "column ... does not exist" with engine-driver shape variation tolerated. Replaces bare `catch {}` blocks in `oauth-provider.ts` so genuine errors (lock timeout, network blip, permission denied) propagate while column-missing falls through to the legacy fallback path. Reusable from any future code that needs the same column-existence probe semantics. - `src/core/db.ts` — Connection management, schema initialization. v0.22.1 (#363, contributed by @orendi84): `resolveSessionTimeouts()` returns `statement_timeout` + `idle_in_transaction_session_timeout` (defaults: 5min each, env-overridable via `GBRAIN_STATEMENT_TIMEOUT` / `GBRAIN_IDLE_TX_TIMEOUT` / `GBRAIN_CLIENT_CHECK_INTERVAL`). Both `connect()` (module singleton) and `PostgresEngine.connect()` (worker pool) consume the result via postgres.js's `connection` option, sending GUCs as startup parameters that survive PgBouncer transaction mode (unlike the prior `setSessionDefaults` post-pool SET, kept as a back-compat no-op shim). - `src/commands/migrate-engine.ts` — Bidirectional engine migration (`gbrain migrate --to supabase/pglite`) - `src/core/import-file.ts` — importFromFile + importFromContent (chunk + embed + tags) @@ -126,10 +126,10 @@ strict behavior when unset. - `src/commands/features.ts` — `gbrain features --json --auto-fix`: usage scan + feature adoption salesman - `src/commands/autopilot.ts` — `gbrain autopilot --install`: self-maintaining brain daemon (sync+extract+embed) - `src/mcp/server.ts` — MCP stdio server (generated from operations). v0.22.7: tool-call handler delegates to `dispatchToolCall` from `src/mcp/dispatch.ts` so stdio + HTTP transports share one validation, context-build, and error-format path. -- `src/mcp/dispatch.ts` (v0.22.7) — Shared tool-call dispatch consumed by both stdio (`server.ts`) and HTTP transports. Exports `dispatchToolCall(engine, name, params, opts)`, `buildOperationContext(engine, params, opts)`, and `validateParams(op, params)`. Single source of truth for `(ctx, params)` handler arg order and the 5-field `OperationContext` shape (engine + config + logger + dryRun + remote). Defaults to `remote: true` (untrusted); local CLI callers pass `remote: false`. Closed F1/F2/F3 drift bugs in the original v0.22.5 HTTP transport. +- `src/mcp/dispatch.ts` (v0.22.7) — Shared tool-call dispatch consumed by both stdio (`server.ts`) and HTTP transports. Exports `dispatchToolCall(engine, name, params, opts)`, `buildOperationContext(engine, params, opts)`, and `validateParams(op, params)`. Single source of truth for `(ctx, params)` handler arg order and the 5-field `OperationContext` shape (engine + config + logger + dryRun + remote). Defaults to `remote: true` (untrusted); local CLI callers pass `remote: false`. Closed F1/F2/F3 drift bugs in the original v0.22.5 HTTP transport. **v0.26.9 (F8):** adds `summarizeMcpParams(opName, params)` — privacy-preserving redactor for `mcp_request_log` and the admin SSE feed. Returns `{redacted, kind, declared_keys, unknown_key_count, approx_bytes}`. Intersects submitted top-level keys against the operation's declared `params` allow-list (declared keys preserved as a sorted array for debug visibility; unknown keys counted but never named, closing the attacker-controlled-key-name leak). Byte counts bucketed up to nearest 1KB so an attacker can't binary-search secret-content sizes via repeated probes. Operators on a personal laptop who want raw payload visibility opt back in with `gbrain serve --http --log-full-params` (loud stderr warning at startup). Canonical helper — new logging code paths route through it rather than `JSON.stringify(params)`. - `src/mcp/rate-limit.ts` (v0.22.7) — Bounded-LRU token-bucket limiter. `buildDefaultLimiters()` returns the two-bucket pipeline: pre-auth IP (30/60s, fires BEFORE the DB lookup so brute-force load against `access_tokens` is actually capped) + post-auth token-id (60/60s). Tracks `lastTouchedMs` separately from `lastRefillMs` so an exhausted key can't be reset by hammering past the TTL. LRU cap bounds memory under attacker-controlled key growth. -- `src/commands/serve-http.ts` (v0.26.0) — Express 5 HTTP MCP server with OAuth 2.1, admin dashboard, and SSE live activity feed. Started via `gbrain serve --http [--port N] [--token-ttl N] [--enable-dcr] [--public-url URL]`. Supersedes the v0.22.7 `src/mcp/http-transport.ts` simple bearer-auth path. Combines MCP SDK's `mcpAuthRouter` (authorize / token / register / revoke endpoints), a custom `client_credentials` handler (SDK's token endpoint throws `UnsupportedGrantTypeError` for CC; the custom handler runs BEFORE the router and falls through for `auth_code` / `refresh_token`), `requireBearerAuth` middleware for `/mcp` with scope enforcement before op dispatch, `localOnly` rejection, and `express-rate-limit` at 50 req / 15 min on `/token`. Serves the built admin SPA from `admin/dist/` with SPA fallback. `/admin/events` SSE endpoint broadcasts every MCP request to connected admin browsers. `cookie-parser` middleware wired (Express 5 has no built-in). Startup logging prints port, engine, configured issuer URL (honors `--public-url`), registered-client count, DCR status, and admin bootstrap token. -- `src/core/oauth-provider.ts` (v0.26.0) — `GBrainOAuthProvider` implementing the MCP SDK's `OAuthServerProvider` + `OAuthRegisteredClientsStore` interfaces. Backed by raw SQL (works on both PGLite and Postgres — OAuth is infrastructure, not a BrainEngine concern). Full OAuth 2.1 spec: `authorize` + `exchangeAuthorizationCode` with PKCE (for ChatGPT), `client_credentials` (for Perplexity / Claude), `refresh_token` with rotation, `revokeToken`, `registerClient` (DCR path validates redirect_uri must be `https://` or loopback per RFC 6749 §3.1.2.1). All tokens + client secrets SHA-256 hashed before storage. Auth codes single-use with 10-minute TTL via atomic `DELETE...RETURNING` (closes RFC 6749 §10.5 TOCTOU race). Refresh rotation also `DELETE...RETURNING` (closes §10.4 stolen-token detection bypass). `pgArray()` escapes commas/quotes/braces in elements so a comma-bearing redirect_uri can't smuggle a second array element. Legacy `access_tokens` fallback in `verifyAccessToken` grandfathers pre-v0.26 bearer tokens as `read+write+admin`. `sweepExpiredTokens()` runs on startup wrapped in try/catch. **v0.26.2:** module-private `coerceTimestamp()` boundary helper at the top of the file normalizes postgres-driver-as-string BIGINT columns to JS numbers at every read site (5 call sites: `getClient` L112+L113 for DCR `/register` RFC 7591 §3.2.1 numeric timestamps, `exchangeRefreshToken` L274 + `verifyAccessToken` L296+L303 for the SDK's `typeof === 'number'` bearerAuth check). Throws on non-finite input (NaN/Infinity) so corrupt rows fail loud at the boundary instead of riding through as `expiresAt: NaN`; returns undefined for SQL NULL so callers decide NULL semantics explicitly (refresh + access token paths treat NULL as expired). Helper intentionally NOT promoted to `src/core/utils.ts` — codex review flagged repo-wide BIGINT precision-loss risk for a generic helper. +- `src/commands/serve-http.ts` (v0.26.0) — Express 5 HTTP MCP server with OAuth 2.1, admin dashboard, and SSE live activity feed. Started via `gbrain serve --http [--port N] [--token-ttl N] [--enable-dcr] [--public-url URL] [--log-full-params]`. Supersedes the v0.22.7 `src/mcp/http-transport.ts` simple bearer-auth path. Combines MCP SDK's `mcpAuthRouter` (authorize / token / register / revoke endpoints), a custom `client_credentials` handler (SDK's token endpoint throws `UnsupportedGrantTypeError` for CC; the custom handler runs BEFORE the router and falls through for `auth_code` / `refresh_token`), `requireBearerAuth` middleware for `/mcp` with scope enforcement before op dispatch, `localOnly` rejection, and `express-rate-limit` at 50 req / 15 min on `/token`. Serves the built admin SPA from `admin/dist/` with SPA fallback. `/admin/events` SSE endpoint broadcasts every MCP request to connected admin browsers. `cookie-parser` middleware wired (Express 5 has no built-in). Startup logging prints port, engine, configured issuer URL (honors `--public-url`), registered-client count, DCR status, and admin bootstrap token. **v0.26.9 hardening pass:** F7 sets `remote: true` explicitly on the `/mcp` request handler's OperationContext literal (closes the HTTP shell-job RCE — without this, `submit_job`'s protected-name guard at `operations.ts:1391` saw a falsy undefined and skipped, letting a `read+write`-scoped OAuth token submit `shell` jobs). F8 wires `summarizeMcpParams` from `src/mcp/dispatch.ts` into both `mcp_request_log` writes and the admin SSE feed by default (raw payloads opt-in via `--log-full-params` with stderr warning). F9 sets cookie `Secure` flag when behind HTTPS or a public-URL proxy. F10 caps the magic-link nonce store with an LRU bound. F12 routes DCR disable through the `GBrainOAuthProvider` constructor's `dcrDisabled` option instead of the prior monkey-patch on the express router. F14 wraps `transport.handleRequest` in try/catch so SDK throws return a JSON-RPC 500 envelope instead of express's default HTML error page. F15 unifies OperationError + unexpected exceptions through `buildError` / `serializeError` so `/mcp` always returns the same envelope shape. +- `src/core/oauth-provider.ts` (v0.26.0) — `GBrainOAuthProvider` implementing the MCP SDK's `OAuthServerProvider` + `OAuthRegisteredClientsStore` interfaces. Backed by raw SQL (works on both PGLite and Postgres — OAuth is infrastructure, not a BrainEngine concern). Full OAuth 2.1 spec: `authorize` + `exchangeAuthorizationCode` with PKCE (for ChatGPT), `client_credentials` (for Perplexity / Claude), `refresh_token` with rotation, `revokeToken`, `registerClient` (DCR path validates redirect_uri must be `https://` or loopback per RFC 6749 §3.1.2.1). All tokens + client secrets SHA-256 hashed before storage. Auth codes single-use with 10-minute TTL via atomic `DELETE...RETURNING` (closes RFC 6749 §10.5 TOCTOU race). Refresh rotation also `DELETE...RETURNING` (closes §10.4 stolen-token detection bypass). `pgArray()` escapes commas/quotes/braces in elements so a comma-bearing redirect_uri can't smuggle a second array element. Legacy `access_tokens` fallback in `verifyAccessToken` grandfathers pre-v0.26 bearer tokens as `read+write+admin`. `sweepExpiredTokens()` runs on startup wrapped in try/catch. **v0.26.9 RFC 6749/7009 hardening pass:** F1+F2 fold `client_id` atomically into the `DELETE WHERE` clauses for both auth-code exchange and refresh rotation — pre-fix the post-hoc client compare burned the row on wrong-client paths so the legitimate client couldn't retry. F3 enforces refresh-scope-subset against the original grant on the row (RFC 6749 §6), not the client's currently-allowed scopes — fixes the case where revoking a scope from a client wouldn't shrink the agent's existing refresh tokens. F4 binds `client_id` on `revokeToken` so a client can only revoke its own tokens (RFC 7009 §2.1). F7c validates the `/token` request's `redirect_uri` against the value stored at `/authorize` (RFC 6749 §4.1.3) — empty-string treated as missing rather than wildcard match (adversarial-review fix). F5 swaps bare `catch {}` blocks in `verifyAccessToken` and `getClient` for `isUndefinedColumnError` from `src/core/utils.ts` — only SQLSTATE 42703 falls through to legacy fallback; lock timeouts and network blips throw and surface. F6 makes `sweepExpiredTokens()` actually return the count via `RETURNING 1` + array length, not a fire-and-forget zero. F12 adds `dcrDisabled` constructor option so `serve-http.ts` can disable the `/register` endpoint without monkey-patching the router. **v0.26.2:** module-private `coerceTimestamp()` boundary helper at the top of the file normalizes postgres-driver-as-string BIGINT columns to JS numbers at every read site (5 call sites: `getClient` L112+L113 for DCR `/register` RFC 7591 §3.2.1 numeric timestamps, `exchangeRefreshToken` L274 + `verifyAccessToken` L296+L303 for the SDK's `typeof === 'number'` bearerAuth check). Throws on non-finite input (NaN/Infinity) so corrupt rows fail loud at the boundary instead of riding through as `expiresAt: NaN`; returns undefined for SQL NULL so callers decide NULL semantics explicitly (refresh + access token paths treat NULL as expired). Helper intentionally NOT promoted to `src/core/utils.ts` — codex review flagged repo-wide BIGINT precision-loss risk for a generic helper. - `admin/` (v0.26.0) — React 19 + Vite + TypeScript admin SPA embedded in the binary via `admin/dist/` served by `serve-http.ts`. 7 screens: Login (bootstrap token → session cookie), Dashboard (metrics + SSE feed + token health), Agents (sortable table + sparklines + Register button), Register (modal with scope checkboxes + grant type selector), Credentials reveal (full-screen modal with Copy + Download JSON + yellow one-time-only warning), Request Log (filterable paginated), Agent Detail drawer (Details / Activity / Config Export tabs + Revoke). Design tokens: `#0a0a0f` bg, Inter for UI, JetBrains Mono for data, 4-32px spacing scale, rounded pill badges. HTTP-only SameSite=Strict cookie auth. 65KB gzip. Build: `cd admin && bun install && bun run build`; output at `admin/dist/` is committed for self-contained binaries. - `src/commands/auth.ts` — Token management. `gbrain auth create/list/revoke/test` for legacy bearer tokens (v0.22.7 wired as a first-class CLI subcommand) plus `gbrain auth register-client` (v0.26.0) and `gbrain auth revoke-client ` (v0.26.2) for OAuth 2.1 client lifecycle. `revoke-client` runs an atomic `DELETE...RETURNING` on `oauth_clients`; FK `ON DELETE CASCADE` on `oauth_tokens.client_id` and `oauth_codes.client_id` purges every active token + authorization code in a single transaction. `process.exit(1)` on no-such-client (idempotent — re-running on the same id produces the same exit-1 message). Legacy tokens stored as SHA-256 hashes in `access_tokens`; OAuth clients in `oauth_clients`. As of v0.26.0, legacy tokens grandfather to `read+write+admin` scopes on the OAuth HTTP server, so pre-v0.26 deployments keep working with no migration. - `src/commands/upgrade.ts` — Self-update CLI. `runPostUpgrade()` enumerates migrations from the TS registry (src/commands/migrations/index.ts) and tail-calls `runApplyMigrations(['--yes', '--non-interactive'])` so the mechanical side of every outstanding migration runs unconditionally. @@ -282,7 +282,7 @@ Key commands added in v0.14.2: - `gbrain doctor` gains two new checks: `sync_failures` (surfaces unacknowledged parse failures with exact paths + fix hints) and `brain_score` (renders the 5-component breakdown when score < 100: embed coverage / 35, link density / 25, timeline coverage / 15, orphans / 15, dead links / 10 — sum equals total). Key commands added in v0.26.0 (OAuth 2.1 + HTTP server + admin dashboard): -- `gbrain serve --http [--port 3131] [--token-ttl 3600] [--enable-dcr]` — HTTP MCP server with OAuth 2.1, admin dashboard at `/admin`, SSE activity feed at `/admin/events`, health check at `/health`. Prints admin bootstrap token on first start. Alongside (not replacing) stdio `gbrain serve`. +- `gbrain serve --http [--port 3131] [--token-ttl 3600] [--enable-dcr] [--log-full-params]` — HTTP MCP server with OAuth 2.1, admin dashboard at `/admin`, SSE activity feed at `/admin/events`, health check at `/health`. Prints admin bootstrap token on first start. Alongside (not replacing) stdio `gbrain serve`. As of v0.26.9, `mcp_request_log.params` and the SSE feed default to a redacted summary (`{redacted, kind, declared_keys, unknown_key_count, approx_bytes}`); pass `--log-full-params` to log raw payloads on a personal laptop with a startup warning. - **OAuth client registration** — three paths: 1. CLI: `gbrain auth register-client --grant-types --scopes ` (wired into `src/commands/auth.ts` as a thin wrapper over `GBrainOAuthProvider.registerClientManual`). Default grant types: `client_credentials`. Default scopes: `read`. 2. Admin dashboard: Register client modal → credential reveal with Copy + Download JSON. @@ -484,7 +484,9 @@ parity), `test/cli.test.ts` (CLI structure), `test/config.test.ts` (config redac `test/doctor.test.ts` (doctor command + v0.12.3 assertions that `jsonb_integrity` scans the four v0.12.0 write sites and `markdown_body_completeness` is present), `test/utils.test.ts` (shared SQL utilities + `tryParseEmbedding` null-return and single-warn semantics), `test/build-llms.test.ts` (llms.txt/llms-full.txt generator: path resolution, idempotence, spec shape, regen-drift guard, content contract, AGENTS.md install-path mirror, size-budget enforcement — 7 cases), -`test/oauth.test.ts` (v0.26.0 OAuth 2.1 provider — 27 cases: register, getClient, `client_credentials` grant exchange, `authorization_code` flow with PKCE challenge / verifier, refresh token rotation, `verifyAccessToken` with both OAuth + legacy `access_tokens` fallback, `revokeToken`, `sweepExpiredTokens`, and a contract test asserting `scope` + `localOnly` annotations are set correctly on all 30 operations; **v0.26.2** adds 5 `coerceTimestamp` unit cases (null/undefined/string/number/throw-on-NaN), NULL-`expires_at`-as-expired contract tests for both refresh + access token paths, and a cascade-delete contract test asserting `revoke-client` purges `oauth_tokens` + `oauth_codes` rows via FK CASCADE), +`test/oauth.test.ts` (v0.26.0 OAuth 2.1 provider — 27 cases: register, getClient, `client_credentials` grant exchange, `authorization_code` flow with PKCE challenge / verifier, refresh token rotation, `verifyAccessToken` with both OAuth + legacy `access_tokens` fallback, `revokeToken`, `sweepExpiredTokens`, and a contract test asserting `scope` + `localOnly` annotations are set correctly on all 30 operations; **v0.26.2** adds 5 `coerceTimestamp` unit cases (null/undefined/string/number/throw-on-NaN), NULL-`expires_at`-as-expired contract tests for both refresh + access token paths, and a cascade-delete contract test asserting `revoke-client` purges `oauth_tokens` + `oauth_codes` rows via FK CASCADE; **v0.26.9** adds 14 cases pinning the F1/F2/F3/F4/F5/F6/F7c/F12 invariants, including the F1/F4 cross-client isolation pattern (wrong-client attempt MUST reject AND rightful owner MUST still succeed atomically afterward) and the empty-string `redirect_uri` bypass guard surfaced during adversarial review), +`test/mcp-dispatch-summarize.test.ts` (v0.26.9 — 7 cases pinning F8 `summarizeMcpParams` invariants: declared-keys allow-list intersection, attacker-key-name leak guard (unknown keys counted not named), 1KB byte bucketing for size-probe defense, missing op falls through to fully-redacted shape, declared-keys sorted for deterministic output), +`test/trust-boundary-contract.test.ts` (v0.26.9 — 4 cases pinning F7b fail-closed semantics under cast bypass: `ctx.remote === undefined` treated as remote/untrusted at every flipped call site, `as any` and `Partial<>` spreads can't downgrade trust by accident), `test/check-resolvable-cli.test.ts` (v0.19 CLI wrapper: exit codes, JSON envelope shape, AGENTS.md fallback chain), `test/regression-v0_16_4.test.ts` (findRepoRoot regression guard — hermetic startDir parameterization), `test/filing-audit.test.ts` (v0.19 Check 6: `writes_pages` / `writes_to` frontmatter, filing-rules JSON validation), @@ -511,7 +513,7 @@ E2E tests (`test/e2e/`): Run against real Postgres+pgvector. Require `DATABASE_U - `test/e2e/engine-parity.test.ts` (v0.22.0) — Postgres ↔ PGLite top-result and result-set parity for `searchKeyword` + `searchVector`. Codex flagged that Postgres ranks pages then picks best chunk while PGLite returns chunks directly — without parity coverage the source-boost fix could pass on PGLite and fail on Postgres. Skips gracefully when `DATABASE_URL` is unset. - `test/e2e/postgres-bootstrap.test.ts` (v0.22.6.1) — exercises `PostgresEngine.initSchema()` directly against a fresh real Postgres database. Asserts the bootstrap path is no-op on fresh installs and that SCHEMA_SQL replays cleanly through the engine path (not via the standalone `db.initSchema` from `src/core/db.ts`, which would have produced false-positive coverage). Codex caught the E2E-shape gap during plan review. - `test/e2e/http-transport.test.ts` (v0.22.7) — 8 cases against real Postgres covering `gbrain serve --http` end-to-end: bearer auth round-trip, `last_used_at` SQL-level debounce semantics, `mcp_request_log` row insertion on success and auth_failed paths, `/health` DB-down → 503 (DB-probing health check), and the F1+F2+F3 dispatch round-trip with a real operation. Skips gracefully when `DATABASE_URL` is unset. -- `test/e2e/serve-http-oauth.test.ts` (v0.26.0, expanded v0.26.2) — real-Postgres E2E against `gbrain serve --http` with full OAuth 2.1. Spawns a subprocess server, registers a client via the CLI, mints `client_credentials` tokens, exercises the `/mcp` JSON-RPC pipeline. **v0.26.2 adds:** real DCR `/register` HTTP-level response-shape test (asserts `typeof body.client_id_issued_at === 'number'` over the wire — RFC 7591 §3.2.1 spec compliance, not just internal-store shape); real CLI subprocess test for `revoke-client` (registers → mints token → revokes via `execSync` → asserts token rejected at `/mcp` → asserts re-run exits 1); server fixture flips on `--enable-dcr` so `/register` is reachable. **bun execSync env-inheritance fix:** bun's `execSync` does NOT inherit env mutations done via `process.env.X = ...`, only OS-level env from before bun started. helpers.ts loads `.env.testing` and sets `DATABASE_URL` via `process.env` mutation, which is invisible to subprocesses unless `env: { ...process.env }` is passed explicitly — every subprocess call in this file passes `env: { ...process.env }` for that reason. Reference fix for the next maintainer hitting the same failure mode in sibling sync/cycle/dream/claw-test E2Es. `afterAll` cleanup is guarded on `clientId` (won't throw if `beforeAll` failed before registration); cleanup errors surface to stderr without throwing so real test failures aren't masked. Tracks DCR-registered clients alongside the manual one. Skips gracefully when `DATABASE_URL` is unset. +- `test/e2e/serve-http-oauth.test.ts` (v0.26.0, expanded v0.26.2, expanded v0.26.9) — real-Postgres E2E against `gbrain serve --http` with full OAuth 2.1. Spawns a subprocess server, registers a client via the CLI, mints `client_credentials` tokens, exercises the `/mcp` JSON-RPC pipeline. **v0.26.2 adds:** real DCR `/register` HTTP-level response-shape test (asserts `typeof body.client_id_issued_at === 'number'` over the wire — RFC 7591 §3.2.1 spec compliance, not just internal-store shape); real CLI subprocess test for `revoke-client` (registers → mints token → revokes via `execSync` → asserts token rejected at `/mcp` → asserts re-run exits 1); server fixture flips on `--enable-dcr` so `/register` is reachable. **bun execSync env-inheritance fix:** bun's `execSync` does NOT inherit env mutations done via `process.env.X = ...`, only OS-level env from before bun started. helpers.ts loads `.env.testing` and sets `DATABASE_URL` via `process.env` mutation, which is invisible to subprocesses unless `env: { ...process.env }` is passed explicitly — every subprocess call in this file passes `env: { ...process.env }` for that reason. Reference fix for the next maintainer hitting the same failure mode in sibling sync/cycle/dream/claw-test E2Es. `afterAll` cleanup is guarded on `clientId` (won't throw if `beforeAll` failed before registration); cleanup errors surface to stderr without throwing so real test failures aren't masked. Tracks DCR-registered clients alongside the manual one. **v0.26.9** adds 2 regressions for the F7 trust-boundary fix: an HTTP MCP `submit_job` for `name: "shell"` MUST reject with a permission error (proving the request handler now sets `remote: true` and `submit_job`'s protected-name guard fires), and the same guard rejects subagent submission. Closes the OAuth-token-to-RCE escalation path. Skips gracefully when `DATABASE_URL` is unset. - `test/e2e/sync-parallel.test.ts` (v0.22.13 PR #490) — DATABASE_URL-gated. T2: 60-file Postgres sync at concurrency=4 imports all + no connection leak (probes `pg_stat_activity` before/after to confirm worker engines disconnected). P4: 120-file serial-vs-parallel benchmark prints `SYNC_PARALLEL_BENCH N files | serial=Xms | parallel(4)=Yms | speedup=Zx` for CHANGELOG quoting. Asserts parallel ≤ serial × 1.5 (CI-noise tolerant; not a strict speedup gate). - Tier 2 (`skills.test.ts`) requires OpenClaw + API keys, runs nightly in CI - If `.env.testing` doesn't exist in this directory, check sibling worktrees for one: diff --git a/README.md b/README.md index af9d958c2..cb4de62aa 100644 --- a/README.md +++ b/README.md @@ -729,7 +729,7 @@ ADMIN gbrain serve MCP server (stdio) gbrain serve --http [--port 3131] HTTP MCP server with OAuth 2.1 + admin dashboard [--token-ttl 3600] [--enable-dcr] - [--public-url URL] + [--public-url URL] [--log-full-params] gbrain auth create|list|revoke|test Legacy bearer token management gbrain auth register-client Register an OAuth 2.1 client --grant-types client_credentials,authorization_code diff --git a/SECURITY.md b/SECURITY.md index 3a251e5db..a271c63ed 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -166,3 +166,14 @@ psql "$DATABASE_URL" -c \ `body_too_large`, `parse_error`, `unknown_method`. Failed-auth rows have `token_name = NULL`. Inserts are fire-and-forget so audit failures never block requests. + +**v0.26.9 redaction default.** The `params` column now stores +`{redacted, kind, declared_keys, unknown_key_count, approx_bytes}` instead +of raw JSON-RPC payloads. Declared keys (intersected against the operation's +spec) preserve for debug visibility; unknown keys are counted but never +named so attackers can't probe key existence; byte sizes bucket to 1KB so +content sizes can't be binary-searched. The same shape is broadcast on the +admin SSE feed at `/admin/events`. Operators on a personal laptop who want +raw payloads back can pass `gbrain serve --http --log-full-params` (loud +stderr warning at startup). Multi-tenant deployments should leave it +on the redacted default. diff --git a/TODOS.md b/TODOS.md index 0b45ed718..272b15375 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1,5 +1,31 @@ # TODOS +## OAuth/MCP hardening (v0.26.7 follow-up) + +### F11 — `auth register-client --redirect-uri` flag +**Priority:** P3 + +**What:** `gbrain auth register-client` always passes `[]` for redirect URIs; there is no CLI flag to set them. Operators who want to register an `authorization_code` client without DCR have to hand-edit the database. + +**Why:** Operator UX gap, not a trust-boundary issue. Codex C11 correctly flagged it as scope creep on the v0.26.7 hardening pass — kept out of that PR but worth doing. + +**Pros:** Closes the operator-experience gap. Validates `https://` or loopback per RFC 6749 §3.1.2.1 at registration time. Repeatable flag. +**Cons:** ~30 lines of argv parsing + URL validation. Adds one more flag to the `auth register-client` surface. Low value relative to the OAuth provider hardening that already shipped. +**Context:** Eva-brain has the implementation under `src/commands/auth.ts:registerClient`. Lift verbatim — the `localhost`/`127.0.0.1`/`::1` exact-match validation is correct; codex spot-check confirmed it does NOT match `localhost.evil.com`. v0.27 candidate. +**Depends on:** Nothing. + +### F13 — `gbrain serve --http` argv positive-int validator +**Priority:** P3 + +**What:** `parseInt(args[idx + 1])` on `--port` and `--token-ttl` accepts the next flag as the value if the argument is missing (e.g., `--port --token-ttl 100` parses port as NaN → fallback 3131). Negative integers like `--port -1` parse to -1, server fails to bind with a confusing error. + +**Why:** Hygiene, not security. Codex C11 flagged as scope creep. Cheap to do later. + +**Pros:** Replaces `parseInt(...) || fallback` with a `parsePositiveIntOption(args, flag, fallback, {max?})` helper that validates the next arg isn't a flag, matches `^[1-9]\d*$`, and clamps to a max. Exits 2 with a clear error. +**Cons:** ~20 lines of helper + threading through `serve.ts`. Behavior change: previously-silent bad input now exits loud. Probably fine; no consumer relies on the silent fallback. +**Context:** Eva-brain has the helper at `src/commands/serve.ts`. v0.27 candidate. +**Depends on:** Nothing. + ## destructive-guard (v0.26.5 follow-up) ### Adjacent 2 — Storage objects orphan on hard purge diff --git a/VERSION b/VERSION index f23e1f8ae..5b60fe05a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.26.8 +0.26.9 diff --git a/docs/mcp/DEPLOY.md b/docs/mcp/DEPLOY.md index 83e1749ac..4f508bf35 100644 --- a/docs/mcp/DEPLOY.md +++ b/docs/mcp/DEPLOY.md @@ -85,6 +85,15 @@ Save this token. Open `http://localhost:3131/admin` and paste it to access the dashboard. The dashboard shows live activity, registered clients, request logs, and per-client config export. +> **v0.26.9+:** `mcp_request_log.params` and the live SSE activity feed default +> to a redacted summary `{redacted, kind, declared_keys, unknown_key_count, approx_bytes}`. +> Declared param keys are kept (intersected against the operation's spec); unknown +> keys are counted but never named, and byte sizes round up to 1KB so size-probe +> attacks can't binary-search secret content. Operators on a personal laptop who +> want raw payloads back can pass `gbrain serve --http --log-full-params` (loud +> stderr warning fires at startup). Multi-tenant deployments should leave it on +> the redacted default. + ### 2. Register OAuth clients Register clients from the **`/admin` dashboard**: diff --git a/llms-full.txt b/llms-full.txt index df2f778fc..8eb2d9375 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -137,13 +137,13 @@ strict behavior when unset. ## Key files -- `src/core/operations.ts` — Contract-first operation definitions (the foundation). Also exports upload validators: `validateUploadPath`, `validatePageSlug`, `validateFilename`, plus `matchesSlugAllowList(slug, prefixes)` (v0.23 glob matcher: `/*` matches recursive children; bare `` matches exact only). `OperationContext.remote` flags untrusted callers; `OperationContext.allowedSlugPrefixes` (v0.23) is the trusted-workspace allow-list set by the dream cycle. `put_page` enforces: when `viaSubagent` and `allowedSlugPrefixes` is set, slug must match the allow-list; else the legacy `wiki/agents//...` namespace check applies. Auto-link enabled for trusted-workspace writes (skipped only when `remote=true && !trustedWorkspace`). As of v0.26.0, every `Operation` also carries `scope?: 'read' | 'write' | 'admin'` + `localOnly?: boolean`. All ops are annotated; `sync_brain`, `file_upload`, `file_list`, and `file_url` are `admin + localOnly` (rejected over HTTP). `OperationContext.auth?: AuthInfo` is threaded through HTTP dispatch for scope enforcement in `serve-http.ts` before the op runs. +- `src/core/operations.ts` — Contract-first operation definitions (the foundation). Also exports upload validators: `validateUploadPath`, `validatePageSlug`, `validateFilename`, plus `matchesSlugAllowList(slug, prefixes)` (v0.23 glob matcher: `/*` matches recursive children; bare `` matches exact only). `OperationContext.remote` flags untrusted callers; `OperationContext.allowedSlugPrefixes` (v0.23) is the trusted-workspace allow-list set by the dream cycle. `put_page` enforces: when `viaSubagent` and `allowedSlugPrefixes` is set, slug must match the allow-list; else the legacy `wiki/agents//...` namespace check applies. Auto-link enabled for trusted-workspace writes (skipped only when `remote=true && !trustedWorkspace`). As of v0.26.0, every `Operation` also carries `scope?: 'read' | 'write' | 'admin'` + `localOnly?: boolean`. All ops are annotated; `sync_brain`, `file_upload`, `file_list`, and `file_url` are `admin + localOnly` (rejected over HTTP). `OperationContext.auth?: AuthInfo` is threaded through HTTP dispatch for scope enforcement in `serve-http.ts` before the op runs. **v0.26.9 (D12 + F7b):** `OperationContext.remote` is now a REQUIRED field in the TypeScript type — the compiler is the first defense against transports that forget to set it. Four trust-boundary call sites (`put_page` allowlist, file_upload trust-narrowing, submit_job protected-name guard, auto-link skip) flipped from falsy-default (`!ctx.remote`) to fail-closed semantics (`ctx.remote === false` for "trusted-only" sites and `ctx.remote !== false` for "untrust unless explicit-false"). Anything that isn't strictly `false` is now treated as remote. Closed an HTTP MCP shell-job RCE: a `read+write`-scoped OAuth token could submit `shell` jobs because the HTTP request handler's literal context skipped `remote: true` and `submit_job`'s protected-name guard saw a falsy undefined. Stdio MCP set the field correctly via dispatch.ts; HTTP inlined a parallel context-builder for several releases and lost it. - `src/core/engine.ts` — Pluggable engine interface (BrainEngine). `clampSearchLimit(limit, default, cap)` takes an explicit cap so per-operation caps can be tighter than `MAX_SEARCH_LIMIT`. Exports `LinkBatchInput` / `TimelineBatchInput` for the v0.12.1 bulk-insert API (`addLinksBatch` / `addTimelineEntriesBatch`). As of v0.13.1, `BrainEngine` has a `readonly kind: 'postgres' | 'pglite'` discriminator so migrations (`src/core/migrate.ts`) and other consumers can branch on engine without `instanceof` + dynamic imports. - `src/core/engine-factory.ts` — Engine factory with dynamic imports (`'pglite'` | `'postgres'`) - `src/core/pglite-engine.ts` — PGLite (embedded Postgres 17.5 via WASM) implementation, all 40 BrainEngine methods. `addLinksBatch` / `addTimelineEntriesBatch` use multi-row `unnest()` with manual `$N` placeholders. As of v0.13.1, `connect()` wraps `PGlite.create()` in a try/catch that emits an actionable error naming the macOS 26.3 WASM bug (#223) and pointing at `gbrain doctor`; the lock is released on failure so the next process can retry cleanly. v0.22.0: `searchKeyword` and `searchKeywordChunks` multiply `ts_rank` by the source-factor CASE expression at the chunk-grain level; `searchVector` becomes a two-stage CTE — inner CTE keeps `ORDER BY cc.embedding <=> vec` so HNSW stays usable, outer SELECT re-ranks by `raw_score * source_factor`. Inner LIMIT scales with offset to preserve pagination contract. As of v0.22.6.1, `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL — probes for the specific forward-referenced state the embedded schema blob needs (`pages.source_id`, `links.link_source`, `links.origin_page_id`, `content_chunks.symbol_name`, `content_chunks.language`, `sources` FK target table) and adds only what's missing. Closes the upgrade-wedge bug class that bit users 10+ times across 6 schema versions over 2 years (#239/#243/#266/#357/#366/#374/#375/#378/#395/#396). No-op on fresh installs and modern brains. - `src/core/pglite-schema.ts` — PGLite-specific DDL (pgvector, pg_trgm, triggers) - `src/core/postgres-engine.ts` — Postgres + pgvector implementation (Supabase / self-hosted). `addLinksBatch` / `addTimelineEntriesBatch` use `INSERT ... SELECT FROM unnest($1::text[], ...) JOIN pages ON CONFLICT DO NOTHING RETURNING 1` — 4-5 array params regardless of batch size, sidesteps the 65535-parameter cap. As of v0.12.3, `searchKeyword` / `searchVector` scope `statement_timeout` via `sql.begin` + `SET LOCAL` so the GUC dies with the transaction instead of leaking across the pooled postgres.js connection (contributed by @garagon). `getEmbeddingsByChunkIds` uses `tryParseEmbedding` so one corrupt row skips+warns instead of killing the query. v0.22.0: `searchKeyword`, `searchKeywordChunks`, and `searchVector` apply source-aware ranking by inlining the source-factor CASE and `NOT (col LIKE …)` hard-exclude clause from `src/core/search/sql-ranking.ts`. `searchVector` switches to a two-stage CTE (HNSW-safe inner ORDER BY, source-boost re-rank in the outer SELECT) and carries `p.source_id` through inner→outer for v0.18 multi-source callers. v0.22.1 (#406): `_savedConfig` retains the connect config; `reconnect()` tears down + recreates the pool from saved config (called by supervisor watchdog after 3 consecutive health-check failures). `executeRaw` is a single-statement passthrough — no per-call retry (D3 dropped that as unsound for non-idempotent statements; recovery is supervisor-driven). v0.22.1 (#363, contributed by @orendi84): `connect()` applies `resolveSessionTimeouts()` from `db.ts` as connection-time startup parameters (`statement_timeout`, `idle_in_transaction_session_timeout`) so orphan pgbouncer backends can't hold locks for hours. v0.22.1 (#409, contributed by @atrevino47): `countStaleChunks()` + `listStaleChunks()` server-side-filter on `embedding IS NULL` for `embed --stale`, eliminating ~76 MB/call client-side pull on a fully-embedded brain; `upsertChunks()` resets both `embedding` AND `embedded_at` to NULL when chunk_text changes without a new embedding (consistency). As of v0.22.6.1, `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL on the same forward-reference probe set as the PGLite engine, so old Postgres brains pinned at v0.13/v0.18/v0.19 walk forward cleanly instead of wedging on `column "..." does not exist`. -- `src/core/utils.ts` — Shared SQL utilities extracted from postgres-engine.ts. Exports `parseEmbedding(value)` (throws on unknown input, used by migration + ingest paths where data integrity matters) and as of v0.12.3 `tryParseEmbedding(value)` (returns `null` + warns once per process, used by search/rescore paths where availability matters more than strictness). +- `src/core/utils.ts` — Shared SQL utilities extracted from postgres-engine.ts. Exports `parseEmbedding(value)` (throws on unknown input, used by migration + ingest paths where data integrity matters) and as of v0.12.3 `tryParseEmbedding(value)` (returns `null` + warns once per process, used by search/rescore paths where availability matters more than strictness). **v0.26.9 (D14):** adds `isUndefinedColumnError(err)` predicate — pattern-matches Postgres SQLSTATE 42703 / "column ... does not exist" with engine-driver shape variation tolerated. Replaces bare `catch {}` blocks in `oauth-provider.ts` so genuine errors (lock timeout, network blip, permission denied) propagate while column-missing falls through to the legacy fallback path. Reusable from any future code that needs the same column-existence probe semantics. - `src/core/db.ts` — Connection management, schema initialization. v0.22.1 (#363, contributed by @orendi84): `resolveSessionTimeouts()` returns `statement_timeout` + `idle_in_transaction_session_timeout` (defaults: 5min each, env-overridable via `GBRAIN_STATEMENT_TIMEOUT` / `GBRAIN_IDLE_TX_TIMEOUT` / `GBRAIN_CLIENT_CHECK_INTERVAL`). Both `connect()` (module singleton) and `PostgresEngine.connect()` (worker pool) consume the result via postgres.js's `connection` option, sending GUCs as startup parameters that survive PgBouncer transaction mode (unlike the prior `setSessionDefaults` post-pool SET, kept as a back-compat no-op shim). - `src/commands/migrate-engine.ts` — Bidirectional engine migration (`gbrain migrate --to supabase/pglite`) - `src/core/import-file.ts` — importFromFile + importFromContent (chunk + embed + tags) @@ -223,10 +223,10 @@ strict behavior when unset. - `src/commands/features.ts` — `gbrain features --json --auto-fix`: usage scan + feature adoption salesman - `src/commands/autopilot.ts` — `gbrain autopilot --install`: self-maintaining brain daemon (sync+extract+embed) - `src/mcp/server.ts` — MCP stdio server (generated from operations). v0.22.7: tool-call handler delegates to `dispatchToolCall` from `src/mcp/dispatch.ts` so stdio + HTTP transports share one validation, context-build, and error-format path. -- `src/mcp/dispatch.ts` (v0.22.7) — Shared tool-call dispatch consumed by both stdio (`server.ts`) and HTTP transports. Exports `dispatchToolCall(engine, name, params, opts)`, `buildOperationContext(engine, params, opts)`, and `validateParams(op, params)`. Single source of truth for `(ctx, params)` handler arg order and the 5-field `OperationContext` shape (engine + config + logger + dryRun + remote). Defaults to `remote: true` (untrusted); local CLI callers pass `remote: false`. Closed F1/F2/F3 drift bugs in the original v0.22.5 HTTP transport. +- `src/mcp/dispatch.ts` (v0.22.7) — Shared tool-call dispatch consumed by both stdio (`server.ts`) and HTTP transports. Exports `dispatchToolCall(engine, name, params, opts)`, `buildOperationContext(engine, params, opts)`, and `validateParams(op, params)`. Single source of truth for `(ctx, params)` handler arg order and the 5-field `OperationContext` shape (engine + config + logger + dryRun + remote). Defaults to `remote: true` (untrusted); local CLI callers pass `remote: false`. Closed F1/F2/F3 drift bugs in the original v0.22.5 HTTP transport. **v0.26.9 (F8):** adds `summarizeMcpParams(opName, params)` — privacy-preserving redactor for `mcp_request_log` and the admin SSE feed. Returns `{redacted, kind, declared_keys, unknown_key_count, approx_bytes}`. Intersects submitted top-level keys against the operation's declared `params` allow-list (declared keys preserved as a sorted array for debug visibility; unknown keys counted but never named, closing the attacker-controlled-key-name leak). Byte counts bucketed up to nearest 1KB so an attacker can't binary-search secret-content sizes via repeated probes. Operators on a personal laptop who want raw payload visibility opt back in with `gbrain serve --http --log-full-params` (loud stderr warning at startup). Canonical helper — new logging code paths route through it rather than `JSON.stringify(params)`. - `src/mcp/rate-limit.ts` (v0.22.7) — Bounded-LRU token-bucket limiter. `buildDefaultLimiters()` returns the two-bucket pipeline: pre-auth IP (30/60s, fires BEFORE the DB lookup so brute-force load against `access_tokens` is actually capped) + post-auth token-id (60/60s). Tracks `lastTouchedMs` separately from `lastRefillMs` so an exhausted key can't be reset by hammering past the TTL. LRU cap bounds memory under attacker-controlled key growth. -- `src/commands/serve-http.ts` (v0.26.0) — Express 5 HTTP MCP server with OAuth 2.1, admin dashboard, and SSE live activity feed. Started via `gbrain serve --http [--port N] [--token-ttl N] [--enable-dcr] [--public-url URL]`. Supersedes the v0.22.7 `src/mcp/http-transport.ts` simple bearer-auth path. Combines MCP SDK's `mcpAuthRouter` (authorize / token / register / revoke endpoints), a custom `client_credentials` handler (SDK's token endpoint throws `UnsupportedGrantTypeError` for CC; the custom handler runs BEFORE the router and falls through for `auth_code` / `refresh_token`), `requireBearerAuth` middleware for `/mcp` with scope enforcement before op dispatch, `localOnly` rejection, and `express-rate-limit` at 50 req / 15 min on `/token`. Serves the built admin SPA from `admin/dist/` with SPA fallback. `/admin/events` SSE endpoint broadcasts every MCP request to connected admin browsers. `cookie-parser` middleware wired (Express 5 has no built-in). Startup logging prints port, engine, configured issuer URL (honors `--public-url`), registered-client count, DCR status, and admin bootstrap token. -- `src/core/oauth-provider.ts` (v0.26.0) — `GBrainOAuthProvider` implementing the MCP SDK's `OAuthServerProvider` + `OAuthRegisteredClientsStore` interfaces. Backed by raw SQL (works on both PGLite and Postgres — OAuth is infrastructure, not a BrainEngine concern). Full OAuth 2.1 spec: `authorize` + `exchangeAuthorizationCode` with PKCE (for ChatGPT), `client_credentials` (for Perplexity / Claude), `refresh_token` with rotation, `revokeToken`, `registerClient` (DCR path validates redirect_uri must be `https://` or loopback per RFC 6749 §3.1.2.1). All tokens + client secrets SHA-256 hashed before storage. Auth codes single-use with 10-minute TTL via atomic `DELETE...RETURNING` (closes RFC 6749 §10.5 TOCTOU race). Refresh rotation also `DELETE...RETURNING` (closes §10.4 stolen-token detection bypass). `pgArray()` escapes commas/quotes/braces in elements so a comma-bearing redirect_uri can't smuggle a second array element. Legacy `access_tokens` fallback in `verifyAccessToken` grandfathers pre-v0.26 bearer tokens as `read+write+admin`. `sweepExpiredTokens()` runs on startup wrapped in try/catch. **v0.26.2:** module-private `coerceTimestamp()` boundary helper at the top of the file normalizes postgres-driver-as-string BIGINT columns to JS numbers at every read site (5 call sites: `getClient` L112+L113 for DCR `/register` RFC 7591 §3.2.1 numeric timestamps, `exchangeRefreshToken` L274 + `verifyAccessToken` L296+L303 for the SDK's `typeof === 'number'` bearerAuth check). Throws on non-finite input (NaN/Infinity) so corrupt rows fail loud at the boundary instead of riding through as `expiresAt: NaN`; returns undefined for SQL NULL so callers decide NULL semantics explicitly (refresh + access token paths treat NULL as expired). Helper intentionally NOT promoted to `src/core/utils.ts` — codex review flagged repo-wide BIGINT precision-loss risk for a generic helper. +- `src/commands/serve-http.ts` (v0.26.0) — Express 5 HTTP MCP server with OAuth 2.1, admin dashboard, and SSE live activity feed. Started via `gbrain serve --http [--port N] [--token-ttl N] [--enable-dcr] [--public-url URL] [--log-full-params]`. Supersedes the v0.22.7 `src/mcp/http-transport.ts` simple bearer-auth path. Combines MCP SDK's `mcpAuthRouter` (authorize / token / register / revoke endpoints), a custom `client_credentials` handler (SDK's token endpoint throws `UnsupportedGrantTypeError` for CC; the custom handler runs BEFORE the router and falls through for `auth_code` / `refresh_token`), `requireBearerAuth` middleware for `/mcp` with scope enforcement before op dispatch, `localOnly` rejection, and `express-rate-limit` at 50 req / 15 min on `/token`. Serves the built admin SPA from `admin/dist/` with SPA fallback. `/admin/events` SSE endpoint broadcasts every MCP request to connected admin browsers. `cookie-parser` middleware wired (Express 5 has no built-in). Startup logging prints port, engine, configured issuer URL (honors `--public-url`), registered-client count, DCR status, and admin bootstrap token. **v0.26.9 hardening pass:** F7 sets `remote: true` explicitly on the `/mcp` request handler's OperationContext literal (closes the HTTP shell-job RCE — without this, `submit_job`'s protected-name guard at `operations.ts:1391` saw a falsy undefined and skipped, letting a `read+write`-scoped OAuth token submit `shell` jobs). F8 wires `summarizeMcpParams` from `src/mcp/dispatch.ts` into both `mcp_request_log` writes and the admin SSE feed by default (raw payloads opt-in via `--log-full-params` with stderr warning). F9 sets cookie `Secure` flag when behind HTTPS or a public-URL proxy. F10 caps the magic-link nonce store with an LRU bound. F12 routes DCR disable through the `GBrainOAuthProvider` constructor's `dcrDisabled` option instead of the prior monkey-patch on the express router. F14 wraps `transport.handleRequest` in try/catch so SDK throws return a JSON-RPC 500 envelope instead of express's default HTML error page. F15 unifies OperationError + unexpected exceptions through `buildError` / `serializeError` so `/mcp` always returns the same envelope shape. +- `src/core/oauth-provider.ts` (v0.26.0) — `GBrainOAuthProvider` implementing the MCP SDK's `OAuthServerProvider` + `OAuthRegisteredClientsStore` interfaces. Backed by raw SQL (works on both PGLite and Postgres — OAuth is infrastructure, not a BrainEngine concern). Full OAuth 2.1 spec: `authorize` + `exchangeAuthorizationCode` with PKCE (for ChatGPT), `client_credentials` (for Perplexity / Claude), `refresh_token` with rotation, `revokeToken`, `registerClient` (DCR path validates redirect_uri must be `https://` or loopback per RFC 6749 §3.1.2.1). All tokens + client secrets SHA-256 hashed before storage. Auth codes single-use with 10-minute TTL via atomic `DELETE...RETURNING` (closes RFC 6749 §10.5 TOCTOU race). Refresh rotation also `DELETE...RETURNING` (closes §10.4 stolen-token detection bypass). `pgArray()` escapes commas/quotes/braces in elements so a comma-bearing redirect_uri can't smuggle a second array element. Legacy `access_tokens` fallback in `verifyAccessToken` grandfathers pre-v0.26 bearer tokens as `read+write+admin`. `sweepExpiredTokens()` runs on startup wrapped in try/catch. **v0.26.9 RFC 6749/7009 hardening pass:** F1+F2 fold `client_id` atomically into the `DELETE WHERE` clauses for both auth-code exchange and refresh rotation — pre-fix the post-hoc client compare burned the row on wrong-client paths so the legitimate client couldn't retry. F3 enforces refresh-scope-subset against the original grant on the row (RFC 6749 §6), not the client's currently-allowed scopes — fixes the case where revoking a scope from a client wouldn't shrink the agent's existing refresh tokens. F4 binds `client_id` on `revokeToken` so a client can only revoke its own tokens (RFC 7009 §2.1). F7c validates the `/token` request's `redirect_uri` against the value stored at `/authorize` (RFC 6749 §4.1.3) — empty-string treated as missing rather than wildcard match (adversarial-review fix). F5 swaps bare `catch {}` blocks in `verifyAccessToken` and `getClient` for `isUndefinedColumnError` from `src/core/utils.ts` — only SQLSTATE 42703 falls through to legacy fallback; lock timeouts and network blips throw and surface. F6 makes `sweepExpiredTokens()` actually return the count via `RETURNING 1` + array length, not a fire-and-forget zero. F12 adds `dcrDisabled` constructor option so `serve-http.ts` can disable the `/register` endpoint without monkey-patching the router. **v0.26.2:** module-private `coerceTimestamp()` boundary helper at the top of the file normalizes postgres-driver-as-string BIGINT columns to JS numbers at every read site (5 call sites: `getClient` L112+L113 for DCR `/register` RFC 7591 §3.2.1 numeric timestamps, `exchangeRefreshToken` L274 + `verifyAccessToken` L296+L303 for the SDK's `typeof === 'number'` bearerAuth check). Throws on non-finite input (NaN/Infinity) so corrupt rows fail loud at the boundary instead of riding through as `expiresAt: NaN`; returns undefined for SQL NULL so callers decide NULL semantics explicitly (refresh + access token paths treat NULL as expired). Helper intentionally NOT promoted to `src/core/utils.ts` — codex review flagged repo-wide BIGINT precision-loss risk for a generic helper. - `admin/` (v0.26.0) — React 19 + Vite + TypeScript admin SPA embedded in the binary via `admin/dist/` served by `serve-http.ts`. 7 screens: Login (bootstrap token → session cookie), Dashboard (metrics + SSE feed + token health), Agents (sortable table + sparklines + Register button), Register (modal with scope checkboxes + grant type selector), Credentials reveal (full-screen modal with Copy + Download JSON + yellow one-time-only warning), Request Log (filterable paginated), Agent Detail drawer (Details / Activity / Config Export tabs + Revoke). Design tokens: `#0a0a0f` bg, Inter for UI, JetBrains Mono for data, 4-32px spacing scale, rounded pill badges. HTTP-only SameSite=Strict cookie auth. 65KB gzip. Build: `cd admin && bun install && bun run build`; output at `admin/dist/` is committed for self-contained binaries. - `src/commands/auth.ts` — Token management. `gbrain auth create/list/revoke/test` for legacy bearer tokens (v0.22.7 wired as a first-class CLI subcommand) plus `gbrain auth register-client` (v0.26.0) and `gbrain auth revoke-client ` (v0.26.2) for OAuth 2.1 client lifecycle. `revoke-client` runs an atomic `DELETE...RETURNING` on `oauth_clients`; FK `ON DELETE CASCADE` on `oauth_tokens.client_id` and `oauth_codes.client_id` purges every active token + authorization code in a single transaction. `process.exit(1)` on no-such-client (idempotent — re-running on the same id produces the same exit-1 message). Legacy tokens stored as SHA-256 hashes in `access_tokens`; OAuth clients in `oauth_clients`. As of v0.26.0, legacy tokens grandfather to `read+write+admin` scopes on the OAuth HTTP server, so pre-v0.26 deployments keep working with no migration. - `src/commands/upgrade.ts` — Self-update CLI. `runPostUpgrade()` enumerates migrations from the TS registry (src/commands/migrations/index.ts) and tail-calls `runApplyMigrations(['--yes', '--non-interactive'])` so the mechanical side of every outstanding migration runs unconditionally. @@ -379,7 +379,7 @@ Key commands added in v0.14.2: - `gbrain doctor` gains two new checks: `sync_failures` (surfaces unacknowledged parse failures with exact paths + fix hints) and `brain_score` (renders the 5-component breakdown when score < 100: embed coverage / 35, link density / 25, timeline coverage / 15, orphans / 15, dead links / 10 — sum equals total). Key commands added in v0.26.0 (OAuth 2.1 + HTTP server + admin dashboard): -- `gbrain serve --http [--port 3131] [--token-ttl 3600] [--enable-dcr]` — HTTP MCP server with OAuth 2.1, admin dashboard at `/admin`, SSE activity feed at `/admin/events`, health check at `/health`. Prints admin bootstrap token on first start. Alongside (not replacing) stdio `gbrain serve`. +- `gbrain serve --http [--port 3131] [--token-ttl 3600] [--enable-dcr] [--log-full-params]` — HTTP MCP server with OAuth 2.1, admin dashboard at `/admin`, SSE activity feed at `/admin/events`, health check at `/health`. Prints admin bootstrap token on first start. Alongside (not replacing) stdio `gbrain serve`. As of v0.26.9, `mcp_request_log.params` and the SSE feed default to a redacted summary (`{redacted, kind, declared_keys, unknown_key_count, approx_bytes}`); pass `--log-full-params` to log raw payloads on a personal laptop with a startup warning. - **OAuth client registration** — three paths: 1. CLI: `gbrain auth register-client --grant-types --scopes ` (wired into `src/commands/auth.ts` as a thin wrapper over `GBrainOAuthProvider.registerClientManual`). Default grant types: `client_credentials`. Default scopes: `read`. 2. Admin dashboard: Register client modal → credential reveal with Copy + Download JSON. @@ -581,7 +581,9 @@ parity), `test/cli.test.ts` (CLI structure), `test/config.test.ts` (config redac `test/doctor.test.ts` (doctor command + v0.12.3 assertions that `jsonb_integrity` scans the four v0.12.0 write sites and `markdown_body_completeness` is present), `test/utils.test.ts` (shared SQL utilities + `tryParseEmbedding` null-return and single-warn semantics), `test/build-llms.test.ts` (llms.txt/llms-full.txt generator: path resolution, idempotence, spec shape, regen-drift guard, content contract, AGENTS.md install-path mirror, size-budget enforcement — 7 cases), -`test/oauth.test.ts` (v0.26.0 OAuth 2.1 provider — 27 cases: register, getClient, `client_credentials` grant exchange, `authorization_code` flow with PKCE challenge / verifier, refresh token rotation, `verifyAccessToken` with both OAuth + legacy `access_tokens` fallback, `revokeToken`, `sweepExpiredTokens`, and a contract test asserting `scope` + `localOnly` annotations are set correctly on all 30 operations; **v0.26.2** adds 5 `coerceTimestamp` unit cases (null/undefined/string/number/throw-on-NaN), NULL-`expires_at`-as-expired contract tests for both refresh + access token paths, and a cascade-delete contract test asserting `revoke-client` purges `oauth_tokens` + `oauth_codes` rows via FK CASCADE), +`test/oauth.test.ts` (v0.26.0 OAuth 2.1 provider — 27 cases: register, getClient, `client_credentials` grant exchange, `authorization_code` flow with PKCE challenge / verifier, refresh token rotation, `verifyAccessToken` with both OAuth + legacy `access_tokens` fallback, `revokeToken`, `sweepExpiredTokens`, and a contract test asserting `scope` + `localOnly` annotations are set correctly on all 30 operations; **v0.26.2** adds 5 `coerceTimestamp` unit cases (null/undefined/string/number/throw-on-NaN), NULL-`expires_at`-as-expired contract tests for both refresh + access token paths, and a cascade-delete contract test asserting `revoke-client` purges `oauth_tokens` + `oauth_codes` rows via FK CASCADE; **v0.26.9** adds 14 cases pinning the F1/F2/F3/F4/F5/F6/F7c/F12 invariants, including the F1/F4 cross-client isolation pattern (wrong-client attempt MUST reject AND rightful owner MUST still succeed atomically afterward) and the empty-string `redirect_uri` bypass guard surfaced during adversarial review), +`test/mcp-dispatch-summarize.test.ts` (v0.26.9 — 7 cases pinning F8 `summarizeMcpParams` invariants: declared-keys allow-list intersection, attacker-key-name leak guard (unknown keys counted not named), 1KB byte bucketing for size-probe defense, missing op falls through to fully-redacted shape, declared-keys sorted for deterministic output), +`test/trust-boundary-contract.test.ts` (v0.26.9 — 4 cases pinning F7b fail-closed semantics under cast bypass: `ctx.remote === undefined` treated as remote/untrusted at every flipped call site, `as any` and `Partial<>` spreads can't downgrade trust by accident), `test/check-resolvable-cli.test.ts` (v0.19 CLI wrapper: exit codes, JSON envelope shape, AGENTS.md fallback chain), `test/regression-v0_16_4.test.ts` (findRepoRoot regression guard — hermetic startDir parameterization), `test/filing-audit.test.ts` (v0.19 Check 6: `writes_pages` / `writes_to` frontmatter, filing-rules JSON validation), @@ -608,7 +610,7 @@ E2E tests (`test/e2e/`): Run against real Postgres+pgvector. Require `DATABASE_U - `test/e2e/engine-parity.test.ts` (v0.22.0) — Postgres ↔ PGLite top-result and result-set parity for `searchKeyword` + `searchVector`. Codex flagged that Postgres ranks pages then picks best chunk while PGLite returns chunks directly — without parity coverage the source-boost fix could pass on PGLite and fail on Postgres. Skips gracefully when `DATABASE_URL` is unset. - `test/e2e/postgres-bootstrap.test.ts` (v0.22.6.1) — exercises `PostgresEngine.initSchema()` directly against a fresh real Postgres database. Asserts the bootstrap path is no-op on fresh installs and that SCHEMA_SQL replays cleanly through the engine path (not via the standalone `db.initSchema` from `src/core/db.ts`, which would have produced false-positive coverage). Codex caught the E2E-shape gap during plan review. - `test/e2e/http-transport.test.ts` (v0.22.7) — 8 cases against real Postgres covering `gbrain serve --http` end-to-end: bearer auth round-trip, `last_used_at` SQL-level debounce semantics, `mcp_request_log` row insertion on success and auth_failed paths, `/health` DB-down → 503 (DB-probing health check), and the F1+F2+F3 dispatch round-trip with a real operation. Skips gracefully when `DATABASE_URL` is unset. -- `test/e2e/serve-http-oauth.test.ts` (v0.26.0, expanded v0.26.2) — real-Postgres E2E against `gbrain serve --http` with full OAuth 2.1. Spawns a subprocess server, registers a client via the CLI, mints `client_credentials` tokens, exercises the `/mcp` JSON-RPC pipeline. **v0.26.2 adds:** real DCR `/register` HTTP-level response-shape test (asserts `typeof body.client_id_issued_at === 'number'` over the wire — RFC 7591 §3.2.1 spec compliance, not just internal-store shape); real CLI subprocess test for `revoke-client` (registers → mints token → revokes via `execSync` → asserts token rejected at `/mcp` → asserts re-run exits 1); server fixture flips on `--enable-dcr` so `/register` is reachable. **bun execSync env-inheritance fix:** bun's `execSync` does NOT inherit env mutations done via `process.env.X = ...`, only OS-level env from before bun started. helpers.ts loads `.env.testing` and sets `DATABASE_URL` via `process.env` mutation, which is invisible to subprocesses unless `env: { ...process.env }` is passed explicitly — every subprocess call in this file passes `env: { ...process.env }` for that reason. Reference fix for the next maintainer hitting the same failure mode in sibling sync/cycle/dream/claw-test E2Es. `afterAll` cleanup is guarded on `clientId` (won't throw if `beforeAll` failed before registration); cleanup errors surface to stderr without throwing so real test failures aren't masked. Tracks DCR-registered clients alongside the manual one. Skips gracefully when `DATABASE_URL` is unset. +- `test/e2e/serve-http-oauth.test.ts` (v0.26.0, expanded v0.26.2, expanded v0.26.9) — real-Postgres E2E against `gbrain serve --http` with full OAuth 2.1. Spawns a subprocess server, registers a client via the CLI, mints `client_credentials` tokens, exercises the `/mcp` JSON-RPC pipeline. **v0.26.2 adds:** real DCR `/register` HTTP-level response-shape test (asserts `typeof body.client_id_issued_at === 'number'` over the wire — RFC 7591 §3.2.1 spec compliance, not just internal-store shape); real CLI subprocess test for `revoke-client` (registers → mints token → revokes via `execSync` → asserts token rejected at `/mcp` → asserts re-run exits 1); server fixture flips on `--enable-dcr` so `/register` is reachable. **bun execSync env-inheritance fix:** bun's `execSync` does NOT inherit env mutations done via `process.env.X = ...`, only OS-level env from before bun started. helpers.ts loads `.env.testing` and sets `DATABASE_URL` via `process.env` mutation, which is invisible to subprocesses unless `env: { ...process.env }` is passed explicitly — every subprocess call in this file passes `env: { ...process.env }` for that reason. Reference fix for the next maintainer hitting the same failure mode in sibling sync/cycle/dream/claw-test E2Es. `afterAll` cleanup is guarded on `clientId` (won't throw if `beforeAll` failed before registration); cleanup errors surface to stderr without throwing so real test failures aren't masked. Tracks DCR-registered clients alongside the manual one. **v0.26.9** adds 2 regressions for the F7 trust-boundary fix: an HTTP MCP `submit_job` for `name: "shell"` MUST reject with a permission error (proving the request handler now sets `remote: true` and `submit_job`'s protected-name guard fires), and the same guard rejects subagent submission. Closes the OAuth-token-to-RCE escalation path. Skips gracefully when `DATABASE_URL` is unset. - `test/e2e/sync-parallel.test.ts` (v0.22.13 PR #490) — DATABASE_URL-gated. T2: 60-file Postgres sync at concurrency=4 imports all + no connection leak (probes `pg_stat_activity` before/after to confirm worker engines disconnected). P4: 120-file serial-vs-parallel benchmark prints `SYNC_PARALLEL_BENCH N files | serial=Xms | parallel(4)=Yms | speedup=Zx` for CHANGELOG quoting. Asserts parallel ≤ serial × 1.5 (CI-noise tolerant; not a strict speedup gate). - Tier 2 (`skills.test.ts`) requires OpenClaw + API keys, runs nightly in CI - If `.env.testing` doesn't exist in this directory, check sibling worktrees for one: @@ -2330,7 +2332,7 @@ ADMIN gbrain serve MCP server (stdio) gbrain serve --http [--port 3131] HTTP MCP server with OAuth 2.1 + admin dashboard [--token-ttl 3600] [--enable-dcr] - [--public-url URL] + [--public-url URL] [--log-full-params] gbrain auth create|list|revoke|test Legacy bearer token management gbrain auth register-client Register an OAuth 2.1 client --grant-types client_credentials,authorization_code @@ -4610,6 +4612,15 @@ Save this token. Open `http://localhost:3131/admin` and paste it to access the dashboard. The dashboard shows live activity, registered clients, request logs, and per-client config export. +> **v0.26.9+:** `mcp_request_log.params` and the live SSE activity feed default +> to a redacted summary `{redacted, kind, declared_keys, unknown_key_count, approx_bytes}`. +> Declared param keys are kept (intersected against the operation's spec); unknown +> keys are counted but never named, and byte sizes round up to 1KB so size-probe +> attacks can't binary-search secret content. Operators on a personal laptop who +> want raw payloads back can pass `gbrain serve --http --log-full-params` (loud +> stderr warning fires at startup). Multi-tenant deployments should leave it on +> the redacted default. + ### 2. Register OAuth clients Register clients from the **`/admin` dashboard**: diff --git a/package.json b/package.json index 1e0ea1937..8da7d7b5d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gbrain", - "version": "0.26.8", + "version": "0.26.9", "description": "Postgres-native personal knowledge brain with hybrid RAG search", "type": "module", "main": "src/core/index.ts", diff --git a/src/commands/serve-http.ts b/src/commands/serve-http.ts index 72a5dbed3..87e7b505b 100644 --- a/src/commands/serve-http.ts +++ b/src/commands/serve-http.ts @@ -25,7 +25,10 @@ import type { BrainEngine } from '../core/engine.ts'; import { operations, OperationError } from '../core/operations.ts'; import type { OperationContext, AuthInfo } from '../core/operations.ts'; import { GBrainOAuthProvider } from '../core/oauth-provider.ts'; +import type { SqlQuery } from '../core/oauth-provider.ts'; +import { summarizeMcpParams } from '../mcp/dispatch.ts'; import { loadConfig } from '../core/config.ts'; +import { buildError, serializeError } from '../core/errors.ts'; import { VERSION } from '../version.ts'; import * as db from '../core/db.ts'; @@ -41,19 +44,39 @@ interface ServeHttpOptions { * issuer claim in tokens MUST match the discovery URL clients hit. */ publicUrl?: string; + /** + * When true, write raw request payloads to mcp_request_log + the admin SSE + * feed. Default false: payloads are summarized via dispatch.summarizeMcpParams + * (declared keys only, no values, no attacker-controlled key names). + * + * Operators running gbrain on their own laptop and debugging agent behavior + * can flip this on with `--log-full-params`. The flag prints a loud warning + * at startup so the privacy posture change is visible. + */ + logFullParams?: boolean; } export async function runServeHttp(engine: BrainEngine, options: ServeHttpOptions) { - const { port, tokenTtl, enableDcr, publicUrl } = options; + const { port, tokenTtl, enableDcr, publicUrl, logFullParams } = options; const config = loadConfig() || { engine: 'pglite' as const }; - // Get raw SQL connection for OAuth provider - const sql = db.getConnection(); + if (logFullParams) { + console.error( + '[serve-http] WARNING: --log-full-params writes raw request payloads to mcp_request_log + SSE feed. Disable for shared dashboards or production.', + ); + } - // Initialize OAuth provider + // Get raw SQL connection for OAuth provider + const sql = db.getConnection() as SqlQuery; + + // Initialize OAuth provider. F12 cleanup: DCR-disable now flips a + // constructor option instead of monkey-patching `_clientsStore` after + // construction. Same outcome (no /register endpoint when --enable-dcr + // is not passed); cleaner shape for tests and future maintainers. const oauthProvider = new GBrainOAuthProvider({ - sql: sql as any, + sql, tokenTtl, + dcrDisabled: !enableDcr, }); // Sweep expired tokens on startup (non-blocking) @@ -152,6 +175,20 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption // reverse proxies / tunnels; default to localhost for dev. const issuerUrl = new URL(publicUrl || `http://localhost:${port}`); + // F9: cookie `secure` flag honors both the request's TLS state (req.secure + // is set when express trust-proxy lands an X-Forwarded-Proto: https) AND + // the operator's declared issuer protocol (so a Cloudflare-tunnel deploy + // where the connection inside the tunnel looks like http but the public + // URL is https still tags cookies Secure). Without this, an attacker on + // the network path could MITM the admin cookie over plaintext. + const adminCookie = (req: Request, maxAge: number) => ({ + httpOnly: true, + sameSite: 'strict' as const, + secure: req.secure || issuerUrl.protocol === 'https:', + maxAge, + path: '/admin', + }); + const authRouterOptions: any = { provider: oauthProvider, issuerUrl, @@ -159,15 +196,10 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption resourceName: 'GBrain MCP Server', }; - // Disable DCR by removing registerClient from the clients store - if (!enableDcr) { - // Override the provider's clientsStore to remove registerClient - const originalStore = oauthProvider.clientsStore; - (oauthProvider as any)._clientsStore = { - getClient: originalStore.getClient.bind(originalStore), - // No registerClient = DCR disabled - }; - } + // F12: DCR disable lives on the provider's constructor option above. The + // SDK's mcpAuthRouter reads provider.clientsStore once and only wires up + // /register when the store exposes registerClient — so passing dcrDisabled + // to the constructor is sufficient. No monkey-patching here. const authRouter = mcpAuthRouter(authRouterOptions); @@ -232,12 +264,7 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption const expiresAt = Date.now() + 24 * 60 * 60 * 1000; // 24 hours adminSessions.set(sessionId, expiresAt); - res.cookie('gbrain_admin', sessionId, { - httpOnly: true, - sameSite: 'strict', - maxAge: 24 * 60 * 60 * 1000, - path: '/admin', - }); + res.cookie('gbrain_admin', sessionId, adminCookie(req, 24 * 60 * 60 * 1000)); res.json({ status: 'authenticated' }); }); @@ -271,6 +298,15 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption for (const [nonce, expiresAt] of magicLinkNonces) { if (expiresAt < now) magicLinkNonces.delete(nonce); } + // F10: bound the live-nonce store too. An attacker with the bootstrap + // token (or a misbehaving agent) could mint nonces faster than they + // expire. Map iteration order is insertion order, so dropping from the + // front gives a simple FIFO eviction matching the consumedNonces pattern. + if (magicLinkNonces.size > NONCE_LRU_CAP) { + const drop = magicLinkNonces.size - NONCE_LRU_CAP; + const it = magicLinkNonces.keys(); + for (let i = 0; i < drop; i++) magicLinkNonces.delete(it.next().value as string); + } // Cap consumedNonces growth — drop oldest entries past the LRU cap. if (consumedNonces.size > NONCE_LRU_CAP) { const drop = consumedNonces.size - NONCE_LRU_CAP; @@ -339,12 +375,7 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption const sessionExpiresAt = Date.now() + 7 * 24 * 60 * 60 * 1000; // 7 days for magic link adminSessions.set(sessionId, sessionExpiresAt); - res.cookie('gbrain_admin', sessionId, { - httpOnly: true, - sameSite: 'strict', - maxAge: 7 * 24 * 60 * 60 * 1000, - path: '/admin', - }); + res.cookie('gbrain_admin', sessionId, adminCookie(req, 7 * 24 * 60 * 60 * 1000)); res.redirect('/admin/'); }); @@ -663,15 +694,31 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption error: (msg: string) => console.error(`[ERROR] ${msg}`), }, dryRun: !!(params?.dry_run), + // F7: HTTP MCP is the untrusted/agent-facing transport. Stdio MCP at + // src/mcp/dispatch.ts:61 sets this; the inlined HTTP context-builder + // forgot it for several releases, which let HTTP MCP callers with a + // read+write token submit `shell` jobs and execute arbitrary commands + // on the host (RCE). The fail-closed contract in operations.ts is the + // belt; this is the suspenders. + remote: true, auth: authInfo, }; + // F8: redact request payload by default (declared keys only via the + // op's `params` allow-list; values + attacker-controlled key names + // never written to mcp_request_log or the SSE feed). --log-full-params + // bypasses this for operators debugging on their own laptop, with the + // startup warning printed earlier. + const safeParamsSummary = summarizeMcpParams(name, params); + const logParams = logFullParams + ? (params ? JSON.stringify(params) : null) + : (safeParamsSummary ? JSON.stringify(safeParamsSummary) : null); + const broadcastParams = logFullParams ? (params || {}) : safeParamsSummary; + try { const result = await op.handler(ctx, (params || {}) as Record); const latency = Date.now() - startTime; - // Log request + broadcast to SSE - const logParams = params ? JSON.stringify(params) : null; try { await sql`INSERT INTO mcp_request_log (token_name, agent_name, operation, latency_ms, status, params) VALUES (${authInfo.clientId}, ${agentName}, ${name}, ${latency}, ${'success'}, ${logParams})`; @@ -680,7 +727,7 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption broadcastEvent({ agent: agentName, operation: name, - params: params || {}, + params: broadcastParams, scopes: authInfo.scopes.join(','), latency_ms: latency, status: 'success', @@ -690,10 +737,22 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption return { content: [{ type: 'text', text: JSON.stringify(result) }] }; } catch (e) { const latency = Date.now() - startTime; - const error = e instanceof OperationError ? e.toJSON() : { error: 'internal_error', message: e instanceof Error ? e.message : 'Unknown error' }; - - const errMsg = e instanceof Error ? e.message : 'Unknown error'; - const logParams = params ? JSON.stringify(params) : null; + // F15: unify error envelope. Both OperationError and unexpected + // exceptions go through src/core/errors.ts so clients see a single + // shape ({class, code, message, hint}). Pre-fix, OperationError + // serialized via e.toJSON() and other exceptions used a hand-rolled + // {error, message} envelope — a client couldn't pattern-match + // reliably across the two. + const errorPayload = e instanceof OperationError + ? buildError({ + class: 'OperationError', + code: e.code, + message: e.message, + hint: e.suggestion, + docs_url: e.docs, + }) + : serializeError(e); + const errMsg = errorPayload.message; try { await sql`INSERT INTO mcp_request_log (token_name, agent_name, operation, latency_ms, status, params, error_message) VALUES (${authInfo.clientId}, ${agentName}, ${name}, ${latency}, ${'error'}, ${logParams}, ${errMsg})`; @@ -702,21 +761,37 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption broadcastEvent({ agent: agentName, operation: name, - params: params || {}, + params: broadcastParams, + scopes: authInfo.scopes.join(','), latency_ms: latency, status: 'error', - error: errMsg, + error: errorPayload, timestamp: new Date().toISOString(), }); - return { content: [{ type: 'text', text: JSON.stringify(error) }], isError: true }; + return { content: [{ type: 'text', text: JSON.stringify({ error: errorPayload }) }], isError: true }; } }); - // Use StreamableHTTPServerTransport for stateless request handling - const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: undefined as any }); - await server.connect(transport); - await transport.handleRequest(req, res, req.body); + // F14: wrap transport setup + handleRequest in try/catch. Without this, + // an SDK-level throw (e.g., schema parse failure on a malformed request) + // propagates to express's default error handler, which renders an HTML + // error page — clients expecting JSON-RPC envelopes break. On + // !res.headersSent we emit a minimal JSON 500 so the client at least + // gets parseable JSON back. + try { + const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: undefined as any }); + await server.connect(transport); + await transport.handleRequest(req, res, req.body); + } catch (e) { + console.error('MCP request handler error:', e instanceof Error ? e.message : e); + if (!res.headersSent) { + res.status(500).json({ + error: 'internal_error', + message: e instanceof Error ? e.message : 'Unknown error', + }); + } + } }); // --------------------------------------------------------------------------- diff --git a/src/commands/serve.ts b/src/commands/serve.ts index bbe080649..257bc372d 100644 --- a/src/commands/serve.ts +++ b/src/commands/serve.ts @@ -22,8 +22,15 @@ export async function runServe(engine: BrainEngine, args: string[] = []) { const publicUrlIdx = args.indexOf('--public-url'); const publicUrl = publicUrlIdx >= 0 ? args[publicUrlIdx + 1] : undefined; + // F8 escape hatch: --log-full-params writes raw payloads to mcp_request_log + // and the admin SSE feed instead of redacted summaries. Off by default + // (privacy-first); operators running gbrain on their own laptop can flip + // it on for debug visibility. Loud startup warning fires in serve-http.ts + // when set so the posture change is visible in stderr. + const logFullParams = args.includes('--log-full-params'); + const { runServeHttp } = await import('./serve-http.ts'); - await runServeHttp(engine, { port, tokenTtl, enableDcr, publicUrl }); + await runServeHttp(engine, { port, tokenTtl, enableDcr, publicUrl, logFullParams }); } else { console.error('Starting GBrain MCP server (stdio)...'); await startMcpServer(engine); diff --git a/src/core/oauth-provider.ts b/src/core/oauth-provider.ts index ec6de1d88..5749b4ba4 100644 --- a/src/core/oauth-provider.ts +++ b/src/core/oauth-provider.ts @@ -22,14 +22,14 @@ import type { import type { OAuthServerProvider, AuthorizationParams } from '@modelcontextprotocol/sdk/server/auth/provider.js'; import type { OAuthRegisteredClientsStore } from '@modelcontextprotocol/sdk/server/auth/clients.js'; import type { AuthInfo } from '@modelcontextprotocol/sdk/server/auth/types.js'; -import { hashToken, generateToken } from './utils.ts'; +import { hashToken, generateToken, isUndefinedColumnError } from './utils.ts'; // --------------------------------------------------------------------------- // Types // --------------------------------------------------------------------------- /** Raw SQL query function — works with both PGLite and postgres tagged templates */ -type SqlQuery = (strings: TemplateStringsArray, ...values: unknown[]) => Promise[]>; +export type SqlQuery = (strings: TemplateStringsArray, ...values: unknown[]) => Promise[]>; /** * Convert a JS array to a PostgreSQL array literal for PGLite compat. @@ -111,6 +111,16 @@ interface GBrainOAuthProviderOptions { tokenTtl?: number; /** Default refresh token TTL in seconds (default: 30 days) */ refreshTtl?: number; + /** + * Disable Dynamic Client Registration (RFC 7591) while keeping the rest of + * the OAuth surface intact. When true, `clientsStore.registerClient` is not + * surfaced to the SDK router, so POST `/register` returns 404 even though + * the underlying provider can still register clients programmatically via + * `registerClientManual`. Replaces the previous monkey-patching pattern in + * serve-http.ts (cleanup, not a security fix — DCR was never reachable + * before mcpAuthRouter ran). + */ + dcrDisabled?: boolean; } // --------------------------------------------------------------------------- @@ -185,17 +195,29 @@ class GBrainClientsStore implements OAuthRegisteredClientsStore { export class GBrainOAuthProvider implements OAuthServerProvider { private sql: SqlQuery; private _clientsStore: GBrainClientsStore; + private readonly dcrDisabled: boolean; private tokenTtl: number; private refreshTtl: number; constructor(options: GBrainOAuthProviderOptions) { this.sql = options.sql; this._clientsStore = new GBrainClientsStore(this.sql); + this.dcrDisabled = options.dcrDisabled === true; this.tokenTtl = options.tokenTtl || 3600; this.refreshTtl = options.refreshTtl || 30 * 24 * 3600; } get clientsStore(): OAuthRegisteredClientsStore { + if (this.dcrDisabled) { + // Surface getClient only — without registerClient the SDK's mcpAuthRouter + // does not wire up the /register DCR endpoint. Replaces the prior + // monkey-patch in serve-http.ts; the outcome is identical (DCR off-by- + // default), but the API expresses intent on the constructor instead of + // requiring callers to mutate `_clientsStore` after construction. + return { + getClient: this._clientsStore.getClient.bind(this._clientsStore), + } as OAuthRegisteredClientsStore; + } return this._clientsStore; } @@ -230,13 +252,18 @@ export class GBrainOAuthProvider implements OAuthServerProvider { } async challengeForAuthorizationCode( - _client: OAuthClientInformationFull, + client: OAuthClientInformationFull, authorizationCode: string, ): Promise { const codeHash = hashToken(authorizationCode); + // F1 hardening: bind client_id atomically so a wrong client cannot read + // another client's PKCE challenge. Pre-fix the SELECT didn't filter on + // client_id at all. const rows = await this.sql` SELECT code_challenge FROM oauth_codes - WHERE code_hash = ${codeHash} AND expires_at > ${Math.floor(Date.now() / 1000)} + WHERE code_hash = ${codeHash} + AND client_id = ${client.client_id} + AND expires_at > ${Math.floor(Date.now() / 1000)} `; if (rows.length === 0) throw new Error('Authorization code not found or expired'); return rows[0].code_challenge as string; @@ -246,27 +273,44 @@ export class GBrainOAuthProvider implements OAuthServerProvider { client: OAuthClientInformationFull, authorizationCode: string, _codeVerifier?: string, - _redirectUri?: string, + redirectUri?: string, resource?: URL, ): Promise { const codeHash = hashToken(authorizationCode); const now = Math.floor(Date.now() / 1000); - // Atomic single-use: DELETE...RETURNING in one statement closes the - // TOCTOU window. RFC 6749 §10.5 requires auth codes be single-use; the - // earlier SELECT-then-DELETE pattern let two concurrent token requests - // both pass the SELECT before either ran the DELETE, issuing two valid - // token pairs from one code. With RETURNING, the second request gets - // zero rows back and fails cleanly. See CSO finding #2. - const rows = await this.sql` - DELETE FROM oauth_codes - WHERE code_hash = ${codeHash} AND expires_at > ${now} - RETURNING client_id, scopes, resource - `; + // F1 + F7c hardening: bind client_id AND redirect_uri atomically into the + // DELETE WHERE clause. RFC 6749 §10.5 requires auth codes be single-use; + // RFC 6749 §4.1.3 requires the token endpoint validate redirect_uri + // matches the value sent at /authorize. The previous SELECT-then-compare + // pattern (a) burned the code on the wrong-client path so the legitimate + // client could not retry, and (b) ignored redirect_uri on exchange + // entirely. With RETURNING, the second request — or any wrong-client / + // wrong-redirect-uri attempt — gets zero rows back and fails cleanly. + // The legitimate client's code stays available for one valid redemption. + // + // Use `redirectUri !== undefined` rather than truthy — an attacker + // submitting `redirect_uri=""` (empty string) at /token would otherwise + // hit the falsy branch and bypass the binding entirely. + const rows = redirectUri !== undefined + ? await this.sql` + DELETE FROM oauth_codes + WHERE code_hash = ${codeHash} + AND client_id = ${client.client_id} + AND redirect_uri = ${redirectUri} + AND expires_at > ${now} + RETURNING client_id, scopes, resource + ` + : await this.sql` + DELETE FROM oauth_codes + WHERE code_hash = ${codeHash} + AND client_id = ${client.client_id} + AND expires_at > ${now} + RETURNING client_id, scopes, resource + `; if (rows.length === 0) throw new Error('Authorization code not found or expired'); const codeRow = rows[0]; - if (codeRow.client_id !== client.client_id) throw new Error('Client mismatch'); // Issue tokens const scopes = (codeRow.scopes as string[]) || []; @@ -286,26 +330,42 @@ export class GBrainOAuthProvider implements OAuthServerProvider { const tokenHash = hashToken(refreshToken); const now = Math.floor(Date.now() / 1000); - // Atomic rotation: DELETE...RETURNING closes the TOCTOU window. RFC 6749 - // §10.4 detection of stolen refresh tokens depends on second-use failure; - // the earlier SELECT-then-DELETE pattern let attacker + victim both - // succeed, defeating that signal. See CSO finding #3. + // F2 hardening: bind client_id atomically into the DELETE WHERE clause. + // RFC 6749 §10.4 detection of stolen refresh tokens depends on second-use + // failure. The previous SELECT-then-DELETE pattern + post-hoc client + // compare let an attacker who guessed/stole a refresh token burn it on + // the wrong-client path, defeating the stolen-token signal for the + // legitimate client. With the predicate in the DELETE, wrong-client + // attempts get zero rows back; the legitimate client retains the row + // for one valid rotation. const rows = await this.sql` DELETE FROM oauth_tokens - WHERE token_hash = ${tokenHash} AND token_type = 'refresh' + WHERE token_hash = ${tokenHash} + AND token_type = 'refresh' + AND client_id = ${client.client_id} RETURNING client_id, scopes, expires_at `; if (rows.length === 0) throw new Error('Refresh token not found'); const row = rows[0]; - if (row.client_id !== client.client_id) throw new Error('Client mismatch'); // NULL expires_at is treated as expired (fail-closed). Schema permits NULL // even though issueTokens always sets it, so a corrupt or hand-modified row // can't ride past validation. const expiresAt = coerceTimestamp(row.expires_at); if (expiresAt === undefined || expiresAt < now) throw new Error('Refresh token expired'); - const tokenScopes = scopes || (row.scopes as string[]) || []; + // F3 hardening: requested scopes on refresh MUST be a subset of the + // original grant on this refresh token's row. RFC 6749 §6: "the scope of + // the access token … MUST NOT include any scope not originally granted by + // the resource owner." Scope is checked against the row's scopes (the + // grant), NOT against the client's currently-allowed scopes (which can + // expand later). Omitted scope (`undefined`) inherits the original grant + // verbatim and stays distinct from an explicit empty array. + const grantedScopes = (row.scopes as string[]) || []; + if (scopes && scopes.some(s => !grantedScopes.includes(s))) { + throw new Error('Requested scope exceeds refresh token grant'); + } + const tokenScopes = scopes ?? grantedScopes; return this.issueTokens(client.client_id, tokenScopes, resource, true); } @@ -378,11 +438,21 @@ export class GBrainOAuthProvider implements OAuthServerProvider { // ------------------------------------------------------------------------- async revokeToken( - _client: OAuthClientInformationFull, + client: OAuthClientInformationFull, request: OAuthTokenRevocationRequest, ): Promise { const tokenHash = hashToken(request.token); - await this.sql`DELETE FROM oauth_tokens WHERE token_hash = ${tokenHash}`; + // F4 hardening: bind client_id so a client can only revoke its own + // tokens. RFC 7009 §2.1: "The authorization server first validates the + // client credentials … and then verifies whether the token was issued + // to the client making the revocation request." Pre-fix, any + // authenticated client that knew (or guessed) another client's token + // hash could revoke it. + await this.sql` + DELETE FROM oauth_tokens + WHERE token_hash = ${tokenHash} + AND client_id = ${client.client_id} + `; } // ------------------------------------------------------------------------- @@ -397,13 +467,19 @@ export class GBrainOAuthProvider implements OAuthServerProvider { const client = await this._clientsStore.getClient(clientId); if (!client) throw new Error('Client not found'); - // Check if client has been revoked (soft-deleted) + // Check if client has been revoked (soft-deleted). The deleted_at column + // is recent — pre-migration brains don't have it, so the probe must + // tolerate that one specific failure mode without swallowing real errors + // (lock timeouts, network blips, auth failures). try { const [revoked] = await this.sql`SELECT deleted_at FROM oauth_clients WHERE client_id = ${clientId} AND deleted_at IS NOT NULL`; if (revoked) throw new Error('Client has been revoked'); } catch (e) { - // deleted_at column may not exist on PGLite/older schemas — skip check + // F5 hardening: surface anything that ISN'T a missing-column error. + // Bare `catch {}` masked DB outages as "client not revoked" — fail-open + // posture in a security-sensitive code path. if (e instanceof Error && e.message === 'Client has been revoked') throw e; + if (!isUndefinedColumnError(e, 'deleted_at')) throw e; } // Check grant type first (before verifying secret) @@ -427,7 +503,11 @@ export class GBrainOAuthProvider implements OAuthServerProvider { try { const ttlRows = await this.sql`SELECT token_ttl FROM oauth_clients WHERE client_id = ${clientId}`; if (ttlRows.length > 0 && ttlRows[0].token_ttl) clientTtl = Number(ttlRows[0].token_ttl); - } catch { /* token_ttl column doesn't exist — use server default */ } + } catch (e) { + // F5 hardening: same posture as the deleted_at probe above. Only the + // "column doesn't exist" path is a non-fatal fall-through. + if (!isUndefinedColumnError(e, 'token_ttl')) throw e; + } // Client credentials: access token only, NO refresh token (RFC 6749 4.4.3) return this.issueTokens(clientId, grantedScopes, undefined, false, clientTtl); @@ -439,13 +519,17 @@ export class GBrainOAuthProvider implements OAuthServerProvider { async sweepExpiredTokens(): Promise { const now = Math.floor(Date.now() / 1000); + // F6 hardening: postgres.js and PGLite expose deleted-row count on + // different shapes; `(result as any).count` returned 0 on at least one + // engine even when rows were deleted, and codes were never counted at + // all. RETURNING 1 + array length is portable across both engines. const result = await this.sql` - DELETE FROM oauth_tokens WHERE expires_at < ${now} + DELETE FROM oauth_tokens WHERE expires_at < ${now} RETURNING 1 `; const deletedCodes = await this.sql` - DELETE FROM oauth_codes WHERE expires_at < ${now} + DELETE FROM oauth_codes WHERE expires_at < ${now} RETURNING 1 `; - return (result as any).count || 0; + return result.length + deletedCodes.length; } // ------------------------------------------------------------------------- diff --git a/src/core/operations.ts b/src/core/operations.ts index 4d93d38f2..cb6e73436 100644 --- a/src/core/operations.ts +++ b/src/core/operations.ts @@ -214,9 +214,12 @@ export interface OperationContext { * confinement when remote=true and allow unrestricted local-filesystem access * when remote=false. * - * When unset, operations MUST default to the stricter (remote=true) behavior. + * REQUIRED as of the F7b hardening — the type system is the first line of defense. + * Every transport (CLI / stdio MCP / HTTP MCP / subagent dispatcher) sets this + * explicitly. Consumers still treat anything that isn't strictly `false` as + * remote/untrusted (defense in depth in case the type is bypassed via cast). */ - remote?: boolean; + remote: boolean; /** * Subagent runtime context (v0.16+). Set by the subagent tool dispatcher when * dispatching an op as a tool call from an LLM loop. Used to enforce per-op @@ -407,7 +410,7 @@ const put_page: Operation = { const trustedWorkspace = ctx.viaSubagent === true && Array.isArray(ctx.allowedSlugPrefixes) && ctx.allowedSlugPrefixes.length > 0; - if (ctx.remote === true && !trustedWorkspace) { + if (ctx.remote !== false && !trustedWorkspace) { autoLinks = { skipped: 'remote' }; autoTimeline = { skipped: 'remote' }; } else if (result.parsedPage) { @@ -1388,16 +1391,19 @@ const submit_job: Operation = { // GBRAIN_ALLOW_SHELL_JOBS env flag — even if that flag is on, MCP callers // cannot submit protected-type jobs. const { isProtectedJobName } = await import('./minions/protected-names.ts'); - if (ctx.remote && isProtectedJobName(name)) { + // F7b fail-closed: anything that is not strictly false (i.e., remote=true OR + // the field somehow leaks in undefined despite the required type) rejects + // protected job submissions. Closes the HTTP MCP shell-job RCE that surfaced + // when the HTTP transport's OperationContext literal forgot to set remote. + if (ctx.remote !== false && isProtectedJobName(name)) { throw new OperationError('permission_denied', `'${name}' jobs cannot be submitted over MCP (CLI-only for security)`); } const { MinionQueue } = await import('./minions/queue.ts'); const queue = new MinionQueue(ctx.engine); - // Trusted flag set only when this is a local (non-remote) submission. When - // remote=true, the guard above has already thrown for protected names, so - // passing undefined here is safe for any non-protected name that slips by. - const trusted = !ctx.remote && isProtectedJobName(name) ? { allowProtectedSubmit: true } : undefined; + // Trusted flag fires ONLY for an explicit local CLI submission of a protected + // name. Strict `=== false` so an untyped/cast context can't escalate. + const trusted = ctx.remote === false && isProtectedJobName(name) ? { allowProtectedSubmit: true } : undefined; return queue.add(name, (p.data as Record) || {}, { queue: (p.queue as string) || 'default', priority: (p.priority as number) || 0, diff --git a/src/core/utils.ts b/src/core/utils.ts index 3efbdbea9..a74935c6e 100644 --- a/src/core/utils.ts +++ b/src/core/utils.ts @@ -109,6 +109,33 @@ export function parseEmbedding(value: unknown): Float32Array | null { return null; } +/** + * Detect a Postgres "undefined column" error (SQLSTATE 42703) without depending + * on the postgres.js driver-specific error class. + * + * Used for forward-compat probes — code that does `SELECT foo FROM bar` against + * schemas where `foo` may not exist yet on legacy installs (column was added in + * a later migration). Bare `try { ... } catch {}` swallows EVERY error + * (network blips, lock timeouts, auth failures) which masks real bugs as + * "column missing." This predicate keeps the probe narrow. + * + * Matches on either: + * - SQLSTATE code `42703` (postgres.js sets this on the error) + * - the column name appearing in the message alongside a "does not exist" / + * "no such column" / "undefined column" clause (PGLite + various driver + * wraps) + * + * Anything else falls through and the caller MUST re-throw. + */ +export function isUndefinedColumnError(error: unknown, column: string): boolean { + const code = typeof error === 'object' && error !== null && 'code' in error + ? String((error as { code?: unknown }).code) + : ''; + if (code === '42703') return true; + const message = error instanceof Error ? error.message : String(error); + return message.includes(column) && /does not exist|no such column|undefined column/i.test(message); +} + let _tryParseEmbeddingWarned = false; /** diff --git a/src/mcp/dispatch.ts b/src/mcp/dispatch.ts index 2ebac0fdb..3a4067bae 100644 --- a/src/mcp/dispatch.ts +++ b/src/mcp/dispatch.ts @@ -23,6 +23,101 @@ export interface DispatchOpts { logger?: OperationContext['logger']; } +/** + * Build a privacy-safe summary of MCP request params for logging + the admin + * SSE feed. + * + * The previous default of `JSON.stringify(params)` wrote raw payloads — + * page bodies, search queries, file paths — into `mcp_request_log` and + * broadcast them to every connected admin browser. For a personal-knowledge + * brain those payloads include private notes about real people / deals / + * companies, retained indefinitely. + * + * The redactor returns the SHAPE of the request (what op was called, which + * declared params were passed, approximate size) without any of the values. + * + * Hardening note (codex C8): a naive "dump all submitted keys" summary still + * leaks via attacker-controlled key names — a caller can submit + * `put_page {"wiki/people/sensitive_name": "..."}` and the key becomes a + * persistent log entry. To prevent this, we intersect submitted keys + * against the operation's declared `params` allow-list (the same definition + * `validateParams` reads). Anything outside the allow-list is counted but + * not named. + * + * Operators who want full payloads for debugging set `--log-full-params` on + * `gbrain serve --http`; that path bypasses this helper and writes the raw + * JSON, with a loud startup warning. + */ +export interface ParamSummary { + redacted: true; + kind: 'array' | 'object' | string; + declared_keys?: string[]; + unknown_key_count?: number; + length?: number; + approx_bytes?: number; +} + +/** + * Round a byte count UP to the nearest 1KB so the redacted summary keeps a + * coarse size signal without enabling a size-based side channel. + * + * Why bucketing matters: the previous shape published `approx_bytes` as the + * exact JSON.stringify(params).length. An attacker who can submit + * `put_page` with a known prefix and observe the resulting log entry + * could binary-search the byte length of secret content (the body the + * legitimate user just wrote) via repeated probes. Bucketing to 1KB + * resolution destroys that channel while preserving the operator-useful + * "roughly how large was the request" signal. + */ +function bucketBytes(n: number | undefined): number | undefined { + if (n === undefined || !Number.isFinite(n)) return undefined; + if (n <= 0) return 0; + const KB = 1024; + return Math.ceil(n / KB) * KB; +} + +export function summarizeMcpParams(opName: string, params: unknown): ParamSummary | null { + if (params == null) return null; + + let approxBytes: number | undefined; + try { approxBytes = bucketBytes(JSON.stringify(params).length); } catch { approxBytes = undefined; } + + if (Array.isArray(params)) { + return { + redacted: true, + kind: 'array', + length: params.length, + ...(approxBytes !== undefined ? { approx_bytes: approxBytes } : {}), + }; + } + + if (typeof params === 'object') { + const submittedKeys = Object.keys(params as Record); + const op = operations.find(o => o.name === opName); + const allowList = op ? new Set(Object.keys(op.params)) : new Set(); + const declared: string[] = []; + let unknown = 0; + for (const k of submittedKeys) { + if (allowList.has(k)) declared.push(k); + else unknown += 1; + } + declared.sort(); + return { + redacted: true, + kind: 'object', + declared_keys: declared, + unknown_key_count: unknown, + ...(approxBytes !== undefined ? { approx_bytes: approxBytes } : {}), + }; + } + + return { + redacted: true, + kind: typeof params, + ...(approxBytes !== undefined ? { approx_bytes: approxBytes } : {}), + }; +} + /** Validate required params exist and have the expected type. Returns null on success, error message on failure. */ export function validateParams(op: Operation, params: Record): string | null { for (const [key, def] of Object.entries(op.params)) { diff --git a/test/e2e/graph-quality.test.ts b/test/e2e/graph-quality.test.ts index 2cb4b8c0c..51aad6a1a 100644 --- a/test/e2e/graph-quality.test.ts +++ b/test/e2e/graph-quality.test.ts @@ -40,6 +40,9 @@ function makeContext(): OperationContext { config: { engine: 'pglite' } as any, logger: { info: () => {}, warn: () => {}, error: () => {} }, dryRun: false, + // E2E graph quality simulates local-CLI writes (auto-link / timeline run). + // After F7b made `remote` required this needs to be explicit. + remote: false, }; } diff --git a/test/e2e/serve-http-oauth.test.ts b/test/e2e/serve-http-oauth.test.ts index 9abf354f7..4a784422c 100644 --- a/test/e2e/serve-http-oauth.test.ts +++ b/test/e2e/serve-http-oauth.test.ts @@ -722,4 +722,62 @@ describeE2E('serve-http OAuth 2.1 E2E (v0.26.1 + v0.26.2 + v0.26.3)', () => { }); expect(res.status).toBe(401); }); + + // ========================================================================= + // F7 + F7b: HTTP MCP shell-job RCE regression + // ========================================================================= + // + // The headline trust-boundary fix. Pre-fix, the inlined OperationContext + // literal in serve-http.ts forgot to set `remote: true`, which meant + // operations.ts:1391's protected-job-name guard (`if (ctx.remote && ...)`) + // saw a falsy undefined and skipped. An HTTP MCP caller with a write-scoped + // token could then submit `{name: "shell", params: {cmd: "id"}}` over /mcp + // and execute arbitrary commands on the gbrain host. + // + // The fix is two-layered: + // 1) F7 — serve-http.ts sets `remote: true` explicitly. + // 2) F7b — operations.ts:1391 + :1400 use `ctx.remote !== false` / + // `ctx.remote === false` so undefined fails closed even if a + // future transport bypasses the type via cast. + // + // Together they close the path even if either layer regresses alone. + + test('F7: HTTP MCP cannot submit shell jobs (RCE regression)', async () => { + const { access_token } = await mintToken('read write'); + const res = await mcpCall(access_token, 'tools/call', { + name: 'submit_job', + arguments: { name: 'shell', data: { cmd: 'id' } }, + }); + + const body = await res.text(); + // Must reject. Either HTTP 4xx, or a JSON-RPC envelope carrying an + // OperationError with code permission_denied. The exact wire shape + // depends on SDK error mapping — assert the negative invariant + // (no command executed) and the positive invariant (rejection signal). + const rejected = + res.status >= 400 || + body.includes('permission_denied') || + body.includes('cannot be submitted over MCP'); + expect(rejected).toBe(true); + + // Negative: response must NOT contain a successful submit_job result + // (which would surface a job_id field). If a job ID came back the + // privesc landed. + expect(body).not.toMatch(/"job_id"\s*:\s*"?\d+/); + }, 15_000); + + test('F7: HTTP MCP cannot submit subagent jobs (protected name)', async () => { + const { access_token } = await mintToken('read write'); + const res = await mcpCall(access_token, 'tools/call', { + name: 'submit_job', + arguments: { name: 'subagent', data: { prompt: 'noop' } }, + }); + const body = await res.text(); + const rejected = + res.status >= 400 || + body.includes('permission_denied') || + body.includes('cannot be submitted over MCP'); + expect(rejected).toBe(true); + expect(body).not.toMatch(/"job_id"\s*:\s*"?\d+/); + }, 15_000); }); diff --git a/test/mcp-dispatch-summarize.test.ts b/test/mcp-dispatch-summarize.test.ts new file mode 100644 index 000000000..496fed62e --- /dev/null +++ b/test/mcp-dispatch-summarize.test.ts @@ -0,0 +1,122 @@ +/** + * Unit tests for `summarizeMcpParams` — the F8/codex-C8 redactor that strips + * raw values AND attacker-controlled key names from `mcp_request_log` + the + * admin SSE feed. + * + * Three invariants pinned: + * 1. Declared keys (the ones the operation accepts per its `params` + * definition) survive into `declared_keys` for debug visibility. + * 2. Unknown keys are counted but NOT named. A caller submitting a key + * like `wiki/people/sensitive_name` cannot leak that string into the + * log via the redactor's output. + * 3. The redactor never echoes raw values for any key. Privacy-positive + * default; --log-full-params is the documented escape hatch and lives + * in serve-http.ts, not here. + */ + +import { describe, expect, test } from 'bun:test'; +import { summarizeMcpParams, type ParamSummary } from '../src/mcp/dispatch.ts'; + +describe('summarizeMcpParams — declared-keys allow-list', () => { + test('declared keys are preserved alphabetically', () => { + // put_page declares params: slug, content (and a few others). The summary + // should list both, sorted, without any value bytes. + const summary = summarizeMcpParams('put_page', { + slug: 'people/alice', + content: '# Alice\n\nA private note.', + }) as ParamSummary; + + expect(summary).not.toBeNull(); + expect(summary.redacted).toBe(true); + expect(summary.kind).toBe('object'); + expect(summary.declared_keys).toEqual(expect.arrayContaining(['slug', 'content'])); + // Sorted property — fixed order across runs makes log diffs reviewable. + const sorted = [...(summary.declared_keys ?? [])].sort(); + expect(summary.declared_keys).toEqual(sorted); + expect(summary.unknown_key_count).toBe(0); + expect(summary.approx_bytes).toBeGreaterThan(0); + }); + + test('unknown keys are counted but never named', () => { + // Codex C8 attacker scenario: attacker controls key names and stuffs + // sensitive data into them. Privacy posture requires we count them + // without echoing the names anywhere in the summary. + const sensitiveKey = 'wiki/people/SENSITIVE_TARGET_NAME'; + const summary = summarizeMcpParams('put_page', { + slug: 'people/alice', + [sensitiveKey]: 'attacker-controlled value', + another_unknown: 'whatever', + }) as ParamSummary; + + // Hard invariant: the sensitive name MUST NOT appear in any field of + // the summary. + const serialized = JSON.stringify(summary); + expect(serialized).not.toContain('SENSITIVE_TARGET_NAME'); + expect(serialized).not.toContain('attacker-controlled value'); + + // unknown_key_count counts the keys we couldn't validate. + expect(summary.unknown_key_count).toBe(2); + // Declared keys still surface — slug is part of put_page's allow-list. + expect(summary.declared_keys).toContain('slug'); + }); + + test('unknown op name produces all-unknown summary (zero declared keys)', () => { + // If the operation name doesn't resolve, the allow-list is empty and + // every submitted key is unknown. Privacy stays intact: no key names + // surface in the output. + const summary = summarizeMcpParams('this_op_does_not_exist', { + foo: 'a', + bar: 'b', + }) as ParamSummary; + expect(summary.declared_keys).toEqual([]); + expect(summary.unknown_key_count).toBe(2); + const serialized = JSON.stringify(summary); + expect(serialized).not.toContain('foo'); + expect(serialized).not.toContain('bar'); + }); + + test('null/undefined params return null (caller writes SQL NULL)', () => { + expect(summarizeMcpParams('put_page', null)).toBeNull(); + expect(summarizeMcpParams('put_page', undefined)).toBeNull(); + }); + + test('array params summarize length without elements', () => { + const summary = summarizeMcpParams('put_page', [1, 2, 3, 'sensitive']) as ParamSummary; + expect(summary.kind).toBe('array'); + expect(summary.length).toBe(4); + const serialized = JSON.stringify(summary); + expect(serialized).not.toContain('sensitive'); + }); + + test('primitive params summarize kind without value', () => { + const summary = summarizeMcpParams('put_page', 'a sensitive string') as ParamSummary; + expect(summary.kind).toBe('string'); + const serialized = JSON.stringify(summary); + expect(serialized).not.toContain('sensitive'); + }); + + test('approx_bytes is bucketed to 1KB to defeat size-based side-channels', () => { + // D16 / adversarial-review fix: the previous shape exposed exact byte + // length of every request, enabling an attacker to binary-search the + // size of secret content via repeated probes (submit put_page with a + // known prefix, observe approx_bytes, narrow the unknown-suffix size). + // Bucketing to 1KB resolution destroys the side-channel while keeping + // the operator-useful "roughly how big" signal. + const tiny = summarizeMcpParams('put_page', { slug: 'a' }) as ParamSummary; + // Tiny payload (~14 bytes) rounds up to the first 1KB bucket. + expect(tiny.approx_bytes).toBe(1024); + + // 2KB payload should fall in either the 2KB or 3KB bucket depending on + // exact serialization length — the invariant is that it's a multiple of + // 1024, NOT the literal byte count. + const medium = summarizeMcpParams('put_page', { + slug: 'people/test', + content: 'x'.repeat(2000), + }) as ParamSummary; + expect(medium.approx_bytes).toBeDefined(); + expect(medium.approx_bytes! % 1024).toBe(0); + // Bucket cannot be less than the actual size and must round UP, so + // a ~2KB payload lands in the 2KB or 3KB bucket. + expect(medium.approx_bytes!).toBeGreaterThanOrEqual(2048); + }); +}); diff --git a/test/oauth.test.ts b/test/oauth.test.ts index 3977f516a..7d4c86096 100644 --- a/test/oauth.test.ts +++ b/test/oauth.test.ts @@ -614,3 +614,350 @@ describe('redirect_uri validation (DCR)', () => { expect(stored!.redirect_uris[0]).toBe(trickyUri); }); }); + +// --------------------------------------------------------------------------- +// F1 / F4 — Wrong-client cross-tenant attempts +// --------------------------------------------------------------------------- +// +// The atomic client_id binding lives in the DELETE WHERE clause for auth +// codes (exchange + challenge), refresh tokens (rotate), and revocations. +// Without it, any authenticated client that knew/guessed another client's +// hash could (a) consume the code/refresh on the wrong-client path, +// burning it for the legitimate client, or (b) revoke another client's +// tokens. These tests pin the negative invariant — wrong client fails — +// AND the positive invariant — owner still succeeds atomically afterward. + +describe('F1/F4 cross-client isolation', () => { + test('wrong client cannot consume another client authorization code', async () => { + const { clientId: ownerId } = await provider.registerClientManual( + 'authcode-owner-test', ['authorization_code'], 'read', + ['http://localhost:3000/callback'], + ); + const { clientId: attackerId } = await provider.registerClientManual( + 'authcode-attacker-test', ['authorization_code'], 'read', + ['http://localhost:3000/callback'], + ); + const owner = (await provider.clientsStore.getClient(ownerId))!; + const attacker = (await provider.clientsStore.getClient(attackerId))!; + + let redirectUrl = ''; + const mockRes = { redirect: (url: string) => { redirectUrl = url; } } as any; + await provider.authorize(owner, { + codeChallenge: 'challenge', + redirectUri: 'http://localhost:3000/callback', + scopes: ['read'], + }, mockRes); + const code = new URL(redirectUrl).searchParams.get('code')!; + + // Attacker holding the same code MUST be rejected. + await expect(provider.exchangeAuthorizationCode(attacker, code)).rejects.toThrow(); + + // The atomic predicate's payoff: the legitimate owner can STILL redeem + // the code afterward. Without it, the attacker would have burned the + // row in the DELETE and the owner's redemption would 404. + const tokens = await provider.exchangeAuthorizationCode(owner, code); + expect(tokens.access_token).toStartWith('gbrain_at_'); + }); + + test('wrong client cannot read another client PKCE challenge', async () => { + const { clientId: ownerId } = await provider.registerClientManual( + 'challenge-owner-test', ['authorization_code'], 'read', + ['http://localhost:3000/callback'], + ); + const { clientId: attackerId } = await provider.registerClientManual( + 'challenge-attacker-test', ['authorization_code'], 'read', + ['http://localhost:3000/callback'], + ); + const owner = (await provider.clientsStore.getClient(ownerId))!; + const attacker = (await provider.clientsStore.getClient(attackerId))!; + + let redirectUrl = ''; + const mockRes = { redirect: (url: string) => { redirectUrl = url; } } as any; + await provider.authorize(owner, { + codeChallenge: 'owner-challenge', + redirectUri: 'http://localhost:3000/callback', + scopes: ['read'], + }, mockRes); + const code = new URL(redirectUrl).searchParams.get('code')!; + + await expect(provider.challengeForAuthorizationCode!(attacker, code)).rejects.toThrow(); + await expect(provider.challengeForAuthorizationCode!(owner, code)).resolves.toBe('owner-challenge'); + }); + + test('wrong client cannot revoke another client token', async () => { + const { clientId: ownerId, clientSecret: ownerSecret } = await provider.registerClientManual( + 'revoke-owner-test', ['client_credentials'], 'read', + ); + const { clientId: attackerId } = await provider.registerClientManual( + 'revoke-attacker-test', ['client_credentials'], 'read', + ); + const tokens = await provider.exchangeClientCredentials(ownerId, ownerSecret, 'read'); + const attacker = (await provider.clientsStore.getClient(attackerId))!; + + // Attacker tries to revoke owner's token. revokeToken returns void + // (silent on no-op), so we assert the token still verifies after. + await provider.revokeToken!(attacker, { token: tokens.access_token }); + const authInfo = await provider.verifyAccessToken(tokens.access_token); + expect(authInfo.clientId).toBe(ownerId); + }); +}); + +// --------------------------------------------------------------------------- +// F2 + F3 — Refresh-token cross-client isolation + scope subset +// --------------------------------------------------------------------------- + +describe('F2/F3 refresh hardening', () => { + test('wrong client cannot burn another client refresh token', async () => { + const { clientId: ownerId } = await provider.registerClientManual( + 'refresh-owner-test', ['authorization_code'], 'read', + ['http://localhost:3000/callback'], + ); + const { clientId: attackerId } = await provider.registerClientManual( + 'refresh-attacker-test', ['authorization_code'], 'read', + ['http://localhost:3000/callback'], + ); + const owner = (await provider.clientsStore.getClient(ownerId))!; + const attacker = (await provider.clientsStore.getClient(attackerId))!; + + let redirectUrl = ''; + const mockRes = { redirect: (url: string) => { redirectUrl = url; } } as any; + await provider.authorize(owner, { + codeChallenge: 'challenge', + redirectUri: 'http://localhost:3000/callback', + scopes: ['read'], + }, mockRes); + const code = new URL(redirectUrl).searchParams.get('code')!; + const tokens = await provider.exchangeAuthorizationCode(owner, code); + + // Attacker rejected. + await expect(provider.exchangeRefreshToken(attacker, tokens.refresh_token!)).rejects.toThrow(); + + // Owner still redeems atomically — the row was not burned by the + // attacker's attempt. + const rotated = await provider.exchangeRefreshToken(owner, tokens.refresh_token!); + expect(rotated.access_token).toStartWith('gbrain_at_'); + expect(rotated.refresh_token).toBeDefined(); + expect(rotated.refresh_token).not.toBe(tokens.refresh_token); + }); + + test('refresh cannot request scopes outside the original grant (F3)', async () => { + // Client allowed scopes 'read write', but the user only authorized 'read'. + // The refresh token row carries the granted scope, NOT the client's + // currently-allowed scopes (codex C9). Requesting 'write' on refresh + // must fail even though the client could mint a fresh write-scoped + // token via a new authorize round trip. + const { clientId } = await provider.registerClientManual( + 'refresh-scope-test', ['authorization_code'], 'read write', + ['http://localhost:3000/callback'], + ); + const client = (await provider.clientsStore.getClient(clientId))!; + + let redirectUrl = ''; + const mockRes = { redirect: (url: string) => { redirectUrl = url; } } as any; + await provider.authorize(client, { + codeChallenge: 'challenge', + redirectUri: 'http://localhost:3000/callback', + scopes: ['read'], + }, mockRes); + const code = new URL(redirectUrl).searchParams.get('code')!; + const tokens = await provider.exchangeAuthorizationCode(client, code); + + // Attempt to escalate to write — must reject. + await expect( + provider.exchangeRefreshToken(client, tokens.refresh_token!, ['read', 'write']), + ).rejects.toThrow(/scope/i); + }); +}); + +// --------------------------------------------------------------------------- +// F5 — fail-loud column probes (was: bare catch{}) +// --------------------------------------------------------------------------- + +describe('F5 verifyAccessToken / client_credentials column probes', () => { + test('non-schema SQL failures are not swallowed by client credentials soft-delete probe', async () => { + // Synthesize a non-schema error (SQLSTATE 57P01 = admin_shutdown) and + // make sure the catch block re-throws instead of silently treating + // the client as not-revoked. Without the predicate this throw used to + // disappear into the void. + const sqlFailure = Object.assign(new Error('database session failed'), { code: '57P01' }); + const fakeSql = async (strings: TemplateStringsArray): Promise[]> => { + const query = strings.join('$'); + if (query.includes('SELECT client_id, client_secret_hash')) { + return [{ + client_id: 'gbrain_cl_fake', + client_secret_hash: hashToken('secret'), + client_name: 'fake', + redirect_uris: [], + grant_types: ['client_credentials'], + scope: 'read', + client_id_issued_at: 1, + }]; + } + if (query.includes('SELECT deleted_at')) throw sqlFailure; + return []; + }; + const failingProvider = new GBrainOAuthProvider({ sql: fakeSql as any }); + + await expect( + failingProvider.exchangeClientCredentials('gbrain_cl_fake', 'secret', 'read'), + ).rejects.toThrow('database session failed'); + }); +}); + +// --------------------------------------------------------------------------- +// F6 — sweepExpiredTokens returns a meaningful count across both engines +// --------------------------------------------------------------------------- + +describe('F6 sweepExpiredTokens count', () => { + test('returns count > 0 after deleting expired rows', async () => { + const firstClient = (await sql`SELECT client_id FROM oauth_clients LIMIT 1`)[0]; + const t1 = hashToken(generateToken('sweep_count_')); + const t2 = hashToken(generateToken('sweep_count_')); + await sql`INSERT INTO oauth_tokens (token_hash, token_type, client_id, scopes, expires_at) + VALUES (${t1}, ${'access'}, ${firstClient.client_id as string}, ${'{read}'}, ${1})`; + await sql`INSERT INTO oauth_tokens (token_hash, token_type, client_id, scopes, expires_at) + VALUES (${t2}, ${'access'}, ${firstClient.client_id as string}, ${'{read}'}, ${2})`; + + const swept = await provider.sweepExpiredTokens(); + + // Pre-fix: returned 0 on PGLite/postgres.js even when rows were deleted + // because (result as any).count was unset on at least one path. With + // RETURNING 1 + result.length, the actual row count flows back. + expect(swept).toBeGreaterThanOrEqual(2); + }); +}); + +// --------------------------------------------------------------------------- +// F7c — auth code redirect_uri validated on /token (RFC 6749 §4.1.3) +// --------------------------------------------------------------------------- + +describe('F7c redirect_uri binding on auth code exchange', () => { + test('matching redirect_uri succeeds', async () => { + const { clientId } = await provider.registerClientManual( + 'redir-match-test', ['authorization_code'], 'read', + ['http://localhost:3000/callback'], + ); + const client = (await provider.clientsStore.getClient(clientId))!; + + let redirectUrl = ''; + const mockRes = { redirect: (url: string) => { redirectUrl = url; } } as any; + await provider.authorize(client, { + codeChallenge: 'challenge', + redirectUri: 'http://localhost:3000/callback', + scopes: ['read'], + }, mockRes); + const code = new URL(redirectUrl).searchParams.get('code')!; + + const tokens = await provider.exchangeAuthorizationCode( + client, code, undefined, 'http://localhost:3000/callback', + ); + expect(tokens.access_token).toStartWith('gbrain_at_'); + }); + + test('mismatched redirect_uri rejects', async () => { + const { clientId } = await provider.registerClientManual( + 'redir-mismatch-test', ['authorization_code'], 'read', + ['http://localhost:3000/callback'], + ); + const client = (await provider.clientsStore.getClient(clientId))!; + + let redirectUrl = ''; + const mockRes = { redirect: (url: string) => { redirectUrl = url; } } as any; + await provider.authorize(client, { + codeChallenge: 'challenge', + redirectUri: 'http://localhost:3000/callback', + scopes: ['read'], + }, mockRes); + const code = new URL(redirectUrl).searchParams.get('code')!; + + // Attacker submitting the auth code with a different redirect_uri (e.g., + // an attacker-controlled callback URL) MUST be rejected. RFC 6749 §4.1.3. + await expect( + provider.exchangeAuthorizationCode( + client, code, undefined, 'https://attacker.example/cb', + ), + ).rejects.toThrow(); + }); + + test('empty-string redirect_uri does NOT bypass the binding', async () => { + // D15 / adversarial-review fix: `redirectUri ? ...` would treat empty string + // as falsy and silently fall through to the no-redirect-uri branch, + // letting an attacker submit `redirect_uri=""` to bypass the predicate. + // The fix uses `redirectUri !== undefined`. This test asserts the bypass + // is closed: an empty-string redirect_uri must reject (zero-row DELETE + // since stored value is the original non-empty URI), not slip through. + const { clientId } = await provider.registerClientManual( + 'redir-empty-test', ['authorization_code'], 'read', + ['http://localhost:3000/callback'], + ); + const client = (await provider.clientsStore.getClient(clientId))!; + + let redirectUrl = ''; + const mockRes = { redirect: (url: string) => { redirectUrl = url; } } as any; + await provider.authorize(client, { + codeChallenge: 'challenge', + redirectUri: 'http://localhost:3000/callback', + scopes: ['read'], + }, mockRes); + const code = new URL(redirectUrl).searchParams.get('code')!; + + await expect( + provider.exchangeAuthorizationCode(client, code, undefined, ''), + ).rejects.toThrow(); + }); + + test('omitted redirect_uri (back-compat) still succeeds', async () => { + // Existing callers that don't pass redirectUri keep working — the + // predicate only fires when redirectUri is provided. This protects + // against breaking SDK consumers that haven't adopted the parameter + // yet, while still hardening the path for those that have. + const { clientId } = await provider.registerClientManual( + 'redir-omitted-test', ['authorization_code'], 'read', + ['http://localhost:3000/callback'], + ); + const client = (await provider.clientsStore.getClient(clientId))!; + + let redirectUrl = ''; + const mockRes = { redirect: (url: string) => { redirectUrl = url; } } as any; + await provider.authorize(client, { + codeChallenge: 'challenge', + redirectUri: 'http://localhost:3000/callback', + scopes: ['read'], + }, mockRes); + const code = new URL(redirectUrl).searchParams.get('code')!; + + const tokens = await provider.exchangeAuthorizationCode(client, code); + expect(tokens.access_token).toStartWith('gbrain_at_'); + }); +}); + +// --------------------------------------------------------------------------- +// F12 — DCR disable via constructor option (cleanup, not security) +// --------------------------------------------------------------------------- + +describe('F12 dcrDisabled constructor option', () => { + test('clientsStore omits registerClient when dcrDisabled=true', () => { + const dcrOff = new GBrainOAuthProvider({ sql, dcrDisabled: true }); + const store = dcrOff.clientsStore; + expect(typeof store.getClient).toBe('function'); + // SDK's mcpAuthRouter checks for registerClient before wiring up the + // /register endpoint. Absence of the method == DCR endpoint not exposed. + expect((store as any).registerClient).toBeUndefined(); + }); + + test('clientsStore exposes registerClient when dcrDisabled is false/unset', () => { + const dcrOn = new GBrainOAuthProvider({ sql }); + expect(typeof dcrOn.clientsStore.registerClient).toBe('function'); + }); + + test('registerClientManual still works on dcrDisabled providers (CLI path)', async () => { + // The CLI code path uses registerClientManual, which is independent of + // the DCR /register endpoint. dcrDisabled must NOT break it. + const dcrOff = new GBrainOAuthProvider({ sql, dcrDisabled: true }); + const result = await dcrOff.registerClientManual( + 'dcr-disabled-cli-test', ['client_credentials'], 'read', + ); + expect(result.clientId).toStartWith('gbrain_cl_'); + expect(result.clientSecret).toStartWith('gbrain_cs_'); + }); +}); diff --git a/test/trust-boundary-contract.test.ts b/test/trust-boundary-contract.test.ts new file mode 100644 index 000000000..f66b20a10 --- /dev/null +++ b/test/trust-boundary-contract.test.ts @@ -0,0 +1,90 @@ +/** + * Trust-boundary contract regression tests (F7b). + * + * Pins the fail-closed semantics on `ctx.remote`. After v0.27 the field is + * REQUIRED in the TypeScript type, but consumer code MUST still treat any + * value that isn't strictly `false` as remote/untrusted. This is the runtime + * defense-in-depth for the case where a context is constructed via `as` cast + * or `Partial<>` spread and `remote` ends up undefined despite the type. + * + * The bug class this guards against: a future transport (HTTP/2, WebSocket, + * a third-party plugin) inlines its own OperationContext literal and forgets + * to set `remote`. Without these tests, the type system's compile-time check + * is the only line of defense, and any `as OperationContext` cast bypasses + * it. + */ + +import { describe, expect, test } from 'bun:test'; +import { + operations, + type Operation, + type OperationContext, +} from '../src/core/operations.ts'; +import type { BrainEngine } from '../src/core/engine.ts'; + +const submit_job = operations.find(o => o.name === 'submit_job') as Operation; +if (!submit_job) throw new Error('submit_job operation missing'); + +// Stub engine — submit_job's protected-name guard fires before any DB call, +// so the engine handle is never read on the rejection path. +const stubEngine = {} as BrainEngine; +const stubLogger = { info: () => {}, warn: () => {}, error: () => {} }; + +/** + * Construct an OperationContext with `remote` deliberately undefined despite + * the type system saying it's required. Mimics what would happen if a future + * transport inlines its own context literal and forgets the field. + */ +function castUndefinedRemoteCtx(): OperationContext { + return { + engine: stubEngine, + config: { engine: 'pglite' } as any, + logger: stubLogger, + dryRun: false, + // remote intentionally omitted; cast through unknown to bypass the type + } as unknown as OperationContext; +} + +describe('F7b — trust-boundary contract fail-closed semantics', () => { + test('protected job submission rejected when remote is undefined (cast bypass)', async () => { + const ctx = castUndefinedRemoteCtx(); + await expect( + submit_job.handler(ctx, { name: 'shell', data: { cmd: 'id' } }) + ).rejects.toMatchObject({ code: 'permission_denied' }); + }); + + test('protected job submission rejected when remote is true', async () => { + const ctx = { ...castUndefinedRemoteCtx(), remote: true } as OperationContext; + await expect( + submit_job.handler(ctx, { name: 'shell', data: { cmd: 'id' } }) + ).rejects.toMatchObject({ code: 'permission_denied' }); + }); + + test('protected job submission ALLOWED only when remote is strictly false', async () => { + const ctx = { ...castUndefinedRemoteCtx(), remote: false } as OperationContext; + // The handler now passes the protected-name guard and continues into the + // queue. We don't actually want to enqueue anything in a unit test, so + // we expect the call to fail at a LATER point (engine.executeRaw on the + // stub). That's fine — what we're proving is that the guard does NOT + // throw `permission_denied`, which is the failure mode we'd see if F7b + // regressed back to a falsy check that treated remote=false as remote. + await expect( + submit_job.handler(ctx, { name: 'shell', data: { cmd: 'id' } }) + ).rejects.not.toMatchObject({ code: 'permission_denied' }); + }); + + test('non-protected job names always allowed regardless of remote', async () => { + // 'default-noop' is not in PROTECTED_JOB_NAMES. The protected-name guard + // skips entirely, so we again get a downstream stub-engine error. + const cases: OperationContext[] = [ + castUndefinedRemoteCtx(), + { ...castUndefinedRemoteCtx(), remote: true } as OperationContext, + { ...castUndefinedRemoteCtx(), remote: false } as OperationContext, + ]; + for (const ctx of cases) { + await expect( + submit_job.handler(ctx, { name: 'noop-job', data: {} }) + ).rejects.not.toMatchObject({ code: 'permission_denied' }); + } + }); +});