Skip to content

v0.26.9 fix(oauth): RFC 6749 hardening + close HTTP MCP shell-job RCE#628

Merged
garrytan merged 7 commits into
masterfrom
garrytan/eva-oauth-research
May 5, 2026
Merged

v0.26.9 fix(oauth): RFC 6749 hardening + close HTTP MCP shell-job RCE#628
garrytan merged 7 commits into
masterfrom
garrytan/eva-oauth-research

Conversation

@garrytan

@garrytan garrytan commented May 5, 2026

Copy link
Copy Markdown
Owner

Summary

OAuth 2.1 hardening pass + closes an HTTP MCP shell-job RCE. Anyone running gbrain serve --http should upgrade — the same write-scoped token an agent uses to call put_page could submit shell jobs and execute commands on the gbrain host.

Trust-boundary fix (F7 + F7b + D12):

  • src/commands/serve-http.ts/mcp request handler now sets remote: true explicitly. Stdio MCP at src/mcp/dispatch.ts:61 already had this; the inlined HTTP context-builder lost it for several releases.
  • src/core/operations.ts — four trust-boundary call sites flip from falsy-default to ctx.remote !== false / === false. Anything that isn't strictly false now treats the caller as remote/untrusted.
  • OperationContext.remote becomes REQUIRED in the TypeScript type so the compiler is the first line of defense; runtime fail-closed defaults are belt+suspenders for as casts.
  • test/e2e/serve-http-oauth.test.ts — two new regression tests assert HTTP MCP cannot submit shell or subagent jobs.

OAuth provider RFC compliance (F1–F6, F7c, F12):

  • F1+F2: atomic client_id binding on auth code DELETE + refresh DELETE (RFC 6749 §10.5 + §10.4). Wrong-client paths no longer burn the row, owner can still redeem.
  • F3: refresh-token requested scopes must be a subset of the original grant on the row, not the client's currently-allowed scopes (RFC 6749 §6).
  • F4: revokeToken adds AND client_id so a client can only revoke its own tokens (RFC 7009 §2.1).
  • F5: bare catch {} swapped for isUndefinedColumnError SQLSTATE 42703 probes — lock timeouts and network errors no longer ride through as "column missing."
  • F6: sweepExpiredTokens returns the actual count via RETURNING 1 + array length.
  • F7c: redirect_uri validation on /token against the value stored at /authorize (RFC 6749 §4.1.3 — eva-brain missed this; codex caught it; adversarial review caught the empty-string-bypass case).
  • F12: DCR disable via constructor option instead of monkey-patch (cleanup, not security).

Privacy + serve-http hardening (F8, F9, F10, F14, F15):

  • F8: 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 count without naming. Byte counts bucketed to 1KB to defeat size-based side channels.
  • --log-full-params opt-in flag on gbrain serve --http for operators on their own laptops who want raw payloads (loud stderr warning at startup).
  • F9: admin cookies set Secure on HTTPS or behind a public-URL proxy.
  • F10: magic-link nonces bounded by an LRU cap.
  • F14: /mcp wraps transport.handleRequest in try/catch; SDK throws return JSON-RPC 500 instead of express's HTML error page.
  • F15: OperationError + unexpected exceptions both route through buildError/serializeError for unified envelope.

Test Coverage

21 new test cases across 4 test files. 100% of new code paths covered.

[+] src/core/oauth-provider.ts (F1–F6, F7c, F12)
  ├── exchangeAuthorizationCode + challengeForAuthorizationCode (F1)
  │   ├── [★★★] wrong-client cannot consume + owner-still-redeems
  │   └── [★★★] empty-string redirect_uri does NOT bypass binding (D15)
  ├── exchangeRefreshToken (F2 + F3)
  │   ├── [★★★] wrong-client cannot burn + owner-still-redeems
  │   └── [★★★] scope-superset rejected (RFC §6)
  ├── revokeToken (F4) — [★★★] cross-client cannot revoke
  ├── verifyAccessToken (F5) — [★★★] non-schema SQL errors not swallowed
  ├── sweepExpiredTokens (F6) — [★★★] returns count > 0
  ├── redirect_uri (F7c) — [★★★] match / mismatch / omitted (back-compat)
  └── dcrDisabled (F12) — [★★★] /register endpoint absent + manual still works

[+] src/mcp/dispatch.ts (F8 — 7 cases)
  ├── [★★★] declared keys preserved alphabetically
  ├── [★★★] unknown keys counted without naming (codex C8 attacker probe)
  ├── [★★★] approx_bytes bucketed to 1KB (D16 side-channel)
  └── [★★★] null/undefined/array/primitive shape coverage

[+] src/core/operations.ts (F7b — 4 cases)
  ├── [★★★] protected job rejected when remote=undefined (cast bypass)
  ├── [★★★] protected job rejected when remote=true
  ├── [★★★] protected job ALLOWED only when remote strictly false
  └── [★★★] non-protected job names always allowed regardless of remote

[+] test/e2e/serve-http-oauth.test.ts (F7 — 2 E2E regressions)
  ├── [★★★] HTTP MCP submit_job shell rejected (RCE close)
  └── [★★★] HTTP MCP submit_job subagent rejected (protected-name guard)

Tests: 3742 → 3763 (+21 net new this branch)
COVERAGE: 24/24 paths tested (100%)
QUALITY: ★★★ × all

Pre-Landing Review

  • Codex plan review: 11 findings, all incorporated as decisions D3–D10. Three corrected real plan bugs (F7 narrowing, F8 key-name leak, missed F7c). Five added test/scope rigor. Three flagged scope creep — F11+F13 dropped to TODOs.
  • Plan-eng-review: 3 findings — D12 type-tighten, D13 pre-flight test audit, D14 DRY extract. All accepted.
  • Adversarial subagent (this ship): caught D15 (empty-string redirect_uri bypass) and D16 (approx_bytes side-channel). Both fixed in commit e51bbd24 before push.

Plan Completion

14/14 decisions DONE: D1 (--log-full-params), D2 (@ElectricSheepIO credit), D3 (F7 narrowed to submit_job), D4 (F7b 4-site flip), D5 (F7c), D6 (owner-redeems tests), D7 (SDK error compat), D8 (declared-keys allow-list), D9 (F12 reframed), D10 (F11+F13 dropped), D11 (one PR / atomic commits), D12 (type tighten), D13 (pre-flight audit), D14 (DRY extract).

Plus D15 + D16 from this ship's adversarial pass.

TODOS

F11 (auth register-client --redirect-uri) and F13 (serve --http argv positive-int validator) filed as P3 follow-ups in TODOS.md per D10. Both are operator UX gaps, not trust-boundary fixes.

Documentation

  • CLAUDE.md — v0.26.9 annotations on src/core/operations.ts (D12 + F7b trust-boundary), src/core/utils.ts (D14 isUndefinedColumnError), src/mcp/dispatch.ts (F8 summarizeMcpParams), 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 RFC 6749/7009 hardening). New test-file entries.
  • README.md — added --log-full-params to the gbrain serve --http command-line surface.
  • SECURITY.md — documented the v0.26.9 mcp_request_log.params redaction default + --log-full-params opt-in.
  • docs/mcp/DEPLOY.md — operator-facing note on redaction default and when to flip --log-full-params on.

Test plan

  • bun run typecheck clean
  • bun run test — 3763 pass, 0 fail
  • All 21 new tests pass
  • One pre-existing flake in test/brain-registry.serial.test.ts surfaces on master too — environment-sensitive (test expects ~/.gbrain/config.json absent), not introduced by this PR
  • CI passes
  • Real-Postgres E2E (test/e2e/serve-http-oauth.test.ts) passes — runs in CI

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

🤖 Generated with Claude Code

garrytan and others added 4 commits May 4, 2026 16:06
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>
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>
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>
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>
@garrytan garrytan changed the title v0.26.8 fix(oauth): RFC 6749 hardening + close HTTP MCP shell-job RCE v0.26.9 fix(oauth): RFC 6749 hardening + close HTTP MCP shell-job RCE May 5, 2026
garrytan and others added 2 commits May 4, 2026 17:56
OAuth 2.1 hardening + HTTP MCP shell-job RCE fix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
@garrytan garrytan force-pushed the garrytan/eva-oauth-research branch from f39e7df to 136db94 Compare May 5, 2026 00:57
Pull v0.26.8 (auto-RLS event trigger, PR #612) into the OAuth/MCP
hardening branch. Three conflicts resolved:

- VERSION: kept 0.26.9 (this branch claims the next slot above
  master's freshly-shipped 0.26.8).
- package.json: kept 0.26.9 to match VERSION.
- CHANGELOG.md: my v0.26.9 entry stacks on top of master's v0.26.8
  entry, contiguous version sequence preserved (0.26.9 → 0.26.8 →
  0.26.7 → ...).

CLAUDE.md auto-merged cleanly: my v0.26.9 OAuth/MCP-hardening
annotations and master's v0.26.8 RLS-trigger annotations live in
non-overlapping line ranges.

Regenerated llms.txt + llms-full.txt to reflect both passes (the
build-llms test pins the committed bundle against the current
generator output and would otherwise fail).

Verification: bun run typecheck clean, bun test 3773 pass / 0 fail.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@garrytan garrytan merged commit cb02932 into master May 5, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant