v0.26.9 fix(oauth): RFC 6749 hardening + close HTTP MCP shell-job RCE#628
Merged
Conversation
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>
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>
f39e7df to
136db94
Compare
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OAuth 2.1 hardening pass + closes an HTTP MCP shell-job RCE. Anyone running
gbrain serve --httpshould upgrade — the same write-scoped token an agent uses to callput_pagecould submitshelljobs and execute commands on the gbrain host.Trust-boundary fix (F7 + F7b + D12):
src/commands/serve-http.ts—/mcprequest handler now setsremote: trueexplicitly. Stdio MCP atsrc/mcp/dispatch.ts:61already 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 toctx.remote !== false/=== false. Anything that isn't strictlyfalsenow treats the caller as remote/untrusted.OperationContext.remotebecomes REQUIRED in the TypeScript type so the compiler is the first line of defense; runtime fail-closed defaults are belt+suspenders forascasts.test/e2e/serve-http-oauth.test.ts— two new regression tests assert HTTP MCP cannot submitshellorsubagentjobs.OAuth provider RFC compliance (F1–F6, F7c, F12):
client_idbinding on auth code DELETE + refresh DELETE (RFC 6749 §10.5 + §10.4). Wrong-client paths no longer burn the row, owner can still redeem.revokeTokenaddsAND client_idso a client can only revoke its own tokens (RFC 7009 §2.1).catch {}swapped forisUndefinedColumnErrorSQLSTATE 42703 probes — lock timeouts and network errors no longer ride through as "column missing."sweepExpiredTokensreturns the actual count viaRETURNING 1+ array length.redirect_urivalidation on/tokenagainst 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).Privacy + serve-http hardening (F8, F9, F10, F14, F15):
summarizeMcpParamshelper atsrc/mcp/dispatch.tsintersects submitted keys against the operation's declaredparamsallow-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-paramsopt-in flag ongbrain serve --httpfor operators on their own laptops who want raw payloads (loud stderr warning at startup).Secureon HTTPS or behind a public-URL proxy./mcpwrapstransport.handleRequestin try/catch; SDK throws return JSON-RPC 500 instead of express's HTML error page.buildError/serializeErrorfor unified envelope.Test Coverage
Pre-Landing Review
e51bbd24before 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 --httpargv positive-int validator) filed as P3 follow-ups in TODOS.md per D10. Both are operator UX gaps, not trust-boundary fixes.Documentation
src/core/operations.ts(D12 + F7b trust-boundary),src/core/utils.ts(D14isUndefinedColumnError),src/mcp/dispatch.ts(F8summarizeMcpParams),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.--log-full-paramsto thegbrain serve --httpcommand-line surface.mcp_request_log.paramsredaction default +--log-full-paramsopt-in.--log-full-paramson.Test plan
bun run typecheckcleanbun run test— 3763 pass, 0 failtest/brain-registry.serial.test.tssurfaces on master too — environment-sensitive (test expects~/.gbrain/config.jsonabsent), not introduced by this PRtest/e2e/serve-http-oauth.test.ts) passes — runs in CIThanks to @ElectricSheepIO on X for the security review that surfaced this hardening pass.
🤖 Generated with Claude Code