Files
gbrain/SECURITY.md
T
cb02932388 v0.26.9 fix(oauth): RFC 6749 hardening + close HTTP MCP shell-job RCE (#628)
* fix(mcp): close HTTP MCP shell-job RCE + tighten remote contract

The HTTP MCP transport in serve-http.ts inlined its own OperationContext
literal and forgot to set `remote: true`. With the field undefined at the
operations.ts protected-job-name guard (line 1391), an HTTP MCP caller
holding a write-scoped OAuth token could submit `submit_job {name: "shell"}`
and execute arbitrary commands on the gbrain host (RCE-class).

Two-layer fix:

1. F7 — explicit `remote: true` on the inlined /mcp OperationContext.
   Stdio MCP at src/mcp/dispatch.ts:61 already set this; the HTTP path
   was the regression.

2. F7b — fail-closed contract on the four ctx.remote consumer sites in
   operations.ts (auto-link skip, telemetry x2, protected-job guard).
   The protected-job guard flips from `if (ctx.remote && ...)` to
   `if (ctx.remote !== false && ...)` and the trusted-marker site flips
   from `!ctx.remote && ...` to `ctx.remote === false && ...`. Anything
   that isn't strictly `false` now treats the caller as remote/untrusted.

3. D12 — `OperationContext.remote` becomes REQUIRED in the TypeScript
   type. The compiler now catches future transports that forget the field.
   The runtime fail-closed defaults are belt+suspenders for any caller
   that bypasses the type via `as` cast or `Partial<>` spread.

Tests:

- New `test/trust-boundary-contract.test.ts` (4 cases) pins the
  fail-closed semantics: undefined-via-cast rejects, remote=true rejects,
  remote=false allowed (only path that escalates protected-name jobs).

- `test/e2e/serve-http-oauth.test.ts` adds 2 cases asserting HTTP MCP
  cannot submit `shell` or `subagent` jobs even with read+write scope.

- `test/e2e/graph-quality.test.ts` adds the now-required `remote: false`
  to its fixture (e2e graph quality simulates local-CLI writes).

Verification: bun test -> 3742 pass / 0 fail. typecheck clean.

Thanks to @ElectricSheepIO on X for the security review that surfaced
this trust-boundary regression.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(oauth): RFC 6749 hardening + serve-http defense in depth

OAuth provider hardening pass that brings the provider into RFC compliance
on auth code, refresh token, and revocation flows, and tightens the
serve-http surface around request logging and admin cookies.

Provider (src/core/oauth-provider.ts):

- F1: bind client_id atomically into the auth code DELETE WHERE clause for
  exchangeAuthorizationCode + challengeForAuthorizationCode. Previous
  pattern (DELETE...RETURNING then post-hoc client compare) burned codes
  on the wrong-client path so the legitimate client could not retry.
  RFC 6749 §10.5.

- F2: same atomic predicate on exchangeRefreshToken. The pre-fix shape
  defeated RFC 6749 §10.4's stolen-token detection by letting attacker +
  victim both succeed.

- F3: refresh token rejects requested scopes that are not a subset of the
  ORIGINAL grant on the row. Codex C9: subset is checked against the
  recorded grant, not the client's currently-allowed scopes (which can
  expand later); omitted scope inherits the original verbatim and stays
  distinct from explicit-empty. RFC 6749 §6.

- F4: revokeToken adds AND client_id to the DELETE so a client cannot
  revoke another client's tokens by guessing the hash. RFC 7009 §2.1.

- F5: deleted_at and token_ttl column probes use a new
  isUndefinedColumnError helper (extracted to src/core/utils.ts per D14)
  that matches SQLSTATE 42703 or column-name-in-message. Bare catch{}
  used to swallow lock timeouts, network blips, and auth failures as
  "column missing" — fail-open posture in a security path.

- F6: sweepExpiredTokens uses RETURNING 1 + array length. Pre-fix
  (result as any).count returned 0 on at least one engine even when
  rows were deleted, and codes were never counted.

- F7c: NEW finding eva-brain missed. exchangeAuthorizationCode now folds
  redirect_uri into the atomic DELETE predicate when the parameter is
  provided. Stored on /authorize, never compared on /token before this
  commit. RFC 6749 §4.1.3 violation. Back-compat: when caller omits the
  parameter the predicate is skipped, preserving SDK consumers that
  haven't adopted the parameter yet.

- F12 (cleanup, not security): dcrDisabled constructor option replaces
  the prior monkey-patch of _clientsStore in serve-http.ts. The SDK's
  mcpAuthRouter only wires up /register when the store exposes
  registerClient, so omitting the method via the constructor is
  sufficient. Reframed as cleanup per codex C10 — the monkey-patch
  happened before mcpAuthRouter ran, so the prior shape did not have
  a real security regression to claim.

Dispatch (src/mcp/dispatch.ts):

- F8: new summarizeMcpParams(opName, params) intersects submitted keys
  against the operation's declared params allow-list. Returns
  {redacted, kind, declared_keys, unknown_key_count, approx_bytes}.
  Closes the codex C8 leak: a naive "dump all submitted keys" summary
  still echoed attacker-controlled key names like
  put_page {"wiki/people/sensitive_name": "..."} into mcp_request_log
  + the SSE feed. Allow-list pattern keeps debug visibility on declared
  keys while counting unknowns without naming them.

Serve-http (src/commands/serve-http.ts) + serve (src/commands/serve.ts):

- F8 wiring: mcp_request_log + SSE broadcast routed through
  summarizeMcpParams by default. New --log-full-params flag bypasses
  redaction with a loud stderr warning at startup. Default privacy-
  positive; flag is the documented escape hatch for self-hosted
  operators debugging on their own laptop.

- F9: admin cookies set Secure when req.secure OR issuerUrl.protocol
  is https. Cloudflare-tunnel + reverse-proxy deployments where the
  inside-tunnel hop looks like http but the public URL is https now
  tag cookies correctly.

- F10: bound magicLinkNonces with NONCE_LRU_CAP. Previously only the
  consumed-nonces map was capped; an attacker (or misbehaving agent)
  with the bootstrap token could mint nonces faster than they expired
  and grow the live store unbounded.

- F12: dcrDisabled flows through to the provider constructor instead of
  monkey-patching _clientsStore after construction.

- F14: try/catch wraps StreamableHTTPServerTransport setup +
  handleRequest. SDK-level throws no longer fall through to express's
  default HTML error page; clients expecting JSON-RPC envelopes get a
  JSON 500 instead.

- F15: error envelope unified via buildError + serializeError from
  src/core/errors.ts. OperationError and unexpected exceptions both
  emit the same {class, code, message, hint} shape so clients can
  pattern-match a single envelope.

Tests:

- test/oauth.test.ts adds 11 cases:
  * F1+F2 wrong-client cannot consume / read PKCE / burn refresh,
    paired with owner-still-redeems atomically afterward (codex D6 —
    proves the predicate doesn't burn the row on attacker attempts).
  * F3 refresh scope subset enforced.
  * F4 wrong-client cannot revoke.
  * F5 non-schema SQL not swallowed by client_credentials soft-delete probe.
  * F6 sweepExpiredTokens returns count > 0 after deleting rows.
  * F7c redirect_uri match succeeds, mismatch rejects, omitted preserves
    back-compat for callers that don't pass the parameter.
  * F12 dcrDisabled constructor option exposes only getClient,
    registerClientManual still works.

- test/mcp-dispatch-summarize.test.ts (NEW, 6 cases): pins the F8
  privacy invariants. The codex-C8 attacker-key-name probe asserts that
  a sensitive name submitted as a key never appears anywhere in the
  redactor's output.

Verification: bun run typecheck clean. test/oauth.test.ts 55/55,
test/mcp-dispatch-summarize.test.ts 6/6,
test/trust-boundary-contract.test.ts 4/4 from commit A. The one
unrelated unit failure surfaces on master too — environment-sensitive
test that expects ~/.gbrain/config.json to be absent in the test env.

Out of scope: F11 (auth register-client --redirect-uri flag) and F13
(serve --http argv positive-int validator) per codex C11 — operator
UX gaps, not trust-boundary fixes. Filed as follow-up TODOs.

Thanks to @ElectricSheepIO on X for the security review that surfaced
this hardening pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore: file F11 + F13 as OAuth hardening follow-up TODOs

Codex C11 flagged these as scope creep on the v0.26.7 OAuth hardening
PR (operator UX, not trust-boundary). Capturing them here so the
context survives — eva-brain has both implementations and the lift is
mechanical when we want to do them.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(oauth): close adversarial-review findings on F7c + F8

Two bugs surfaced by an adversarial subagent during /ship's pre-landing
review pass that the codex + plan-eng-review didn't catch.

D15 / F7c: `exchangeAuthorizationCode` used `redirectUri ? ...` ternary
to choose the with-redirect vs no-redirect SQL. Empty string fell
through to the no-redirect branch, so a caller submitting
`redirect_uri=""` at /token bypassed the binding entirely. RFC 6749
§4.1.3 spec violation. Switch to `redirectUri !== undefined`. Test:
empty-string redirect_uri must reject when /authorize stored a real URI.

D16 / F8: `summarizeMcpParams` published exact byte length via
`approx_bytes = JSON.stringify(params).length`. Submitting put_page with
a known prefix and observing the resulting log entry across repeated
probes lets an attacker binary-search the size of secret suffix content.
Bucket to 1KB resolution. The redacted summary keeps a coarse
"roughly how big" signal for operators while making size-based
side-channel attacks useless.

Test count: 65 → 67 across the three new test files.
Typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore: bump version and changelog (v0.26.9)

OAuth 2.1 hardening + HTTP MCP shell-job RCE fix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: update project documentation for v0.26.9

Annotate CLAUDE.md key-files entries with v0.26.9 OAuth/MCP hardening pass:
- src/core/operations.ts: D12 (OperationContext.remote required) + F7b
  (4-site fail-closed flip), HTTP MCP shell-job RCE close
- src/core/utils.ts: D14 isUndefinedColumnError extracted helper
- src/mcp/dispatch.ts: F8 summarizeMcpParams privacy redactor with
  declared-keys allow-list + 1KB byte bucketing
- src/commands/serve-http.ts: F7+F8+F9+F10+F12+F14+F15 hardening
- src/core/oauth-provider.ts: F1+F2+F3+F4+F5+F6+F7c+F12 RFC 6749/7009
  hardening pass

Add new test-file entries for test/mcp-dispatch-summarize.test.ts
(7 cases) and test/trust-boundary-contract.test.ts (4 cases). Extend
test/oauth.test.ts (+14 cases) and test/e2e/serve-http-oauth.test.ts
(+2 RCE-close regressions) entries with v0.26.9 case counts.

README.md: added --log-full-params to gbrain serve --http surface.

SECURITY.md: documented mcp_request_log.params redaction default
({redacted, kind, declared_keys, unknown_key_count, approx_bytes}) +
--log-full-params opt-in.

docs/mcp/DEPLOY.md: operator-facing note on SSE feed + audit log
redaction default and when to flip --log-full-params on.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-04 21:11:15 -07:00

180 lines
6.6 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Security
## Reporting Vulnerabilities
If you discover a security issue in GBrain, please report it privately by opening
a [private security advisory](https://github.com/garrytan/gbrain/security/advisories/new)
on GitHub.
Do not open a public issue for security vulnerabilities.
## Remote MCP Security
### ⚠️ Do NOT use open OAuth client registration for remote MCP
If you deploy GBrain's MCP server behind an HTTP wrapper with OAuth 2.1
support, **never allow unauthenticated client registration**. An attacker
who discovers your server URL can:
1. Register a new OAuth client via `POST /register`
2. Use `client_credentials` grant to obtain a bearer token
3. Access all brain data via the MCP tools
### Recommended: `gbrain serve --http`
As of v0.22.7, GBrain ships a built-in HTTP transport that uses the
existing `access_tokens` table for authentication:
```bash
# Create a token
gbrain auth create "my-client"
# Start the HTTP server
gbrain serve --http --port 8787
# Connect via ngrok, Tailscale, or any tunnel
ngrok http 8787 --url your-brain.ngrok.app
```
This is the recommended way to expose GBrain remotely. No OAuth, no
registration endpoint, no self-service tokens. Tokens are managed
exclusively via `gbrain auth create/list/revoke`.
### If you must use a custom HTTP wrapper
1. **Require a secret for client registration** — check a header or body
parameter before creating new OAuth clients
2. **Disable `client_credentials` grant** — only allow `authorization_code`
with browser-based approval
3. **Restrict scopes** — never issue tokens with unlimited scope
4. **Log all token issuance** — alert on unexpected registrations
5. **Rate-limit registration and token endpoints**
### Token Management
```bash
gbrain auth create "claude-desktop" # Create a new token
gbrain auth list # List all tokens
gbrain auth revoke "claude-desktop" # Revoke a token
gbrain auth test <url> --token <tok> # Smoke-test a remote server
```
Tokens are stored as SHA-256 hashes in the `access_tokens` table. The
plaintext token is shown once at creation and never stored.
## `gbrain serve --http` hardening (v0.22.7+)
The built-in HTTP transport ships with several layers of hardening on by
default. All env vars below are optional; the defaults are intentionally
conservative.
### Postgres-only
`gbrain serve --http` requires a Postgres engine. PGLite is local-only by
design and the `access_tokens` / `mcp_request_log` tables don't exist in
the PGLite schema. Local agents continue to use stdio (`gbrain serve`).
Running `--http` against a PGLite-backed install fails fast with a clear
error message at startup.
### CORS
Default-deny: no `Access-Control-Allow-Origin` header is sent unless an
allowlist is configured. To allow browser-based MCP clients:
```bash
GBRAIN_HTTP_CORS_ORIGIN=https://claude.ai gbrain serve --http --port 8787
# Multiple origins: comma-separated
GBRAIN_HTTP_CORS_ORIGIN=https://claude.ai,https://your.app gbrain serve --http
```
When the request `Origin` matches the allowlist, the server echoes it
back in `Access-Control-Allow-Origin` (with `Vary: Origin`). Otherwise no
CORS header is sent and the browser blocks the request.
### Rate limiting
Two buckets, both stored in a bounded LRU map (default 10K keys, evicts
least-recently-used on overflow, prunes entries older than 2× the
window):
| Bucket | When it fires | Default | Env var |
|---|---|---|---|
| Pre-auth IP | Before the DB lookup, on every `/mcp` request | 30 req / 60s | `GBRAIN_HTTP_RATE_LIMIT_IP` |
| Post-auth token | After a valid token is resolved | 60 req / 60s | `GBRAIN_HTTP_RATE_LIMIT_TOKEN` |
| LRU cap | Maximum distinct keys across both buckets | 10000 | `GBRAIN_HTTP_RATE_LIMIT_LRU` |
On exhaustion the server returns `429 Too Many Requests` with a
`Retry-After` header.
**Caveat for tunneled deployments (ngrok, Tailscale Funnel, Cloudflare
Tunnel):** all requests share one egress IP, so the pre-auth IP bucket
becomes effectively shared by all clients on that tunnel. The
post-auth token-id bucket is the load-bearing limiter for tunnel-fronted
deployments.
### Reverse-proxy trust
Disabled by default. To honor `X-Forwarded-For` (or `X-Real-IP`) when
gbrain runs behind a trusted reverse proxy:
```bash
GBRAIN_HTTP_TRUST_PROXY=1 gbrain serve --http --port 8787
```
**Critical safety contract:** only set `GBRAIN_HTTP_TRUST_PROXY=1` when
**both** of these are true:
1. gbrain is reachable only via a trusted reverse proxy (not directly
exposed to the internet on the configured port). The simplest
guarantee is to bind gbrain to `127.0.0.1` or a private interface
and have the proxy forward to it.
2. The proxy strips any client-supplied `X-Forwarded-For` and `X-Real-IP`
headers, then sets them itself. (nginx with `proxy_set_header
X-Forwarded-For $remote_addr` does this; Cloudflare and most cloud
load balancers handle it automatically.)
If gbrain is reachable directly AND `GBRAIN_HTTP_TRUST_PROXY=1` is set,
clients can spoof their IP by sending arbitrary `X-Forwarded-For`
headers, defeating the pre-auth IP rate limit. Without the flag, gbrain
ignores all forwarded-for headers and uses the socket peer address,
which is the safe default for direct-exposure deployments.
### Body size cap
Default 1 MiB, stream-counted (chunked transfers without
`Content-Length` are still capped). Override:
```bash
GBRAIN_HTTP_MAX_BODY_BYTES=2097152 gbrain serve --http # 2 MiB
```
Over-cap requests get `413 Payload Too Large` immediately, before any
body is materialized in memory.
### Audit log
Every `/mcp` request writes one row to `mcp_request_log`:
```bash
psql "$DATABASE_URL" -c \
"SELECT created_at, token_name, operation, status, latency_ms
FROM mcp_request_log
ORDER BY created_at DESC LIMIT 100"
```
`status` is one of: `success`, `error`, `auth_failed`, `rate_limited`,
`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.