The MCP resource server, and the signing key out of the database #170
No reviewers
Labels
No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
lz/agent-nexus!170
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/mcp-resource-server"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #154 and #166.
POST /api/mcpplus its discovery documents — the resource-server half of the MCP work. With #151 (the authorization server) and #152 (the CIMD allowlist) already merged, this is the point where a client can complete authorization and call a tool.Ships two read tools (
list_workspaces,list_sessions) so the shell is demonstrably working. The rest are #155–#158.Two decisions that reversed the issue
createMcpHandler, notWebStandardStreamableHTTPServerTransport. The issue prescribed that transport in stateless mode. In@modelcontextprotocol/server@2.0.0that is the legacy leg — taking it would have shipped a 2025-only server.createMcpHandlerserves 2026-07-28 and, with its defaultlegacy: 'stateless', answers 2025-era traffic with exactly thesessionIdGenerator: undefinedwiring the issue described by hand. One factory backs both. 2026-07-28 is the revision that deprecated DCR for CIMD, which is what #152 shipped for, so serving only the older half would have been self-defeating.Access tokens stay JWT. An earlier draft switched them to opaque so the resource server could call
provider.AccessToken.find—formats/jwt.jsexports nogetTokenId, so that lookup cannot resolve a JWT. Reversed after an honest count:josetakesissuer,audience,algorithmsandtypas options, so the one check no SDK helper performs — the audience — is a parameter rather than hand-written code with tests defending it. Opaque's advantage was immediate revocation, and it is smaller than it looks: refresh tokens are always opaque, so revoking a grant already leaves a client unable to mint anything new. The residual is one in-flight access token, andaccessTokenTTLdrops to 300s to bound it.#166
The signing key cannot be sealed under the vault DEK:
loadOrCreateJwksruns at boot while the DEK exists only inside an unlocked operator session, and sessions do not survive a restart (fact #25). Sealing would 503 OAuth after every restart until a human opened a browser — defeating the phone-without-the-UI goal.So the issue's own third option ships:
OAUTH_SIGNING_KEY, base64 of a JWKS. This is a trade, not a strict improvement — anyone who candocker inspectthe Nexus container can read it. It removes a database-file exposure and adds an environment one. DEPLOYMENT.md says so plainly.A malformed value throws rather than falling back to a generated key; the existing boot catch turns that into a 503 with the reason logged while the rest of Nexus starts. Setting it on an instance that already generated one does not delete the old
metarow.Verification
pnpm typecheck5157 files 0 errors ·pnpm vitest run2160 tests / 192 files ·pnpm lintclean ·pnpm buildclean. Baseline before the branch was 2061.Nine checks against
node build/server.jswith a tempDATA_DIR, driven withnode:http(fetchsilently drops an explicitHost), all passing:resourcehttps://nexus.test/api/mcp/api/settings/instance401 empty / 200 with the operator jar)POST /api/mcp→ 401 +WWW-Authenticatenaming the metadata URL — and the same POST carrying a live operator cookie also 401s, which is what admitting the route above the cookie read buysHostwith a valid bearer → 403GET /api/mcp→ 405tools/call list_workspaces→ an actual roster rowpublic_url→ 503 naming HTTPS, not a 500openid-only token → 403 with the challenge; a realnexus:read nexus:writetoken → 200What is NOT verified
Hostallowlist is unverified against the real deployment.hostHeaderValidationResponsereads whateverproxy_set_header Hostgives it; an nginx forwardingHost: 127.0.0.1:3001would 403 every MCP call with all of the above still green. Worth one request througha.lck.shbefore trusting it. Deliberately not dodged by acceptingX-Forwarded-Host, which is client-settable unless nginx overwrites it.Review found nine things the task-level passes did not
Each task was built with mutation testing and every survivor was driven to a fix. A four-agent review across the whole diff still found:
nexus:write-only token was admitted and got a server with notoolscapability, sotools/listanswered-32601— the exact "client reads a broken server rather than missing authorization" outcome the check existed to prevent. Both shipped tools arenexus:read, so the admission rule and the registry filter disagreed. Admission now derives from the filter itself, so drift is structurally impossible.Accept/406 rule and SSE framing belong to the legacy leg only. The modern leg checks noAcceptheader and returns plain JSON. Four comments said otherwise, including an AGENTS.md fact.OAUTH_SIGNING_KEYpassed validation and then failed at token exchange, after consent succeeded, with nothing logged — there is no productionserver_errorlistener. Now refused at boot.isError: trueat status 200 without callingonerror.deps.tswiring, Host-before-Origin order, the host-less request) and two false "measured" claims.Five of the nine were in code or comments written by the lead. Recorded as AGENTS.md facts 33–38.
Follow-ups
oauth_payloadssweep, still open.oidc-providersupports it natively.Two failures that produced no signal anywhere. decodeSigningKey checked only that "keys" was non-empty and every key had a "d", so a keyset the Provider happily constructs still broke later: more than one key 503s /api/mcp forever through publicJwkFor while both discovery documents keep serving 200, and a key that is not EC P-256 throws at the token exchange — after consent has already succeeded — where nothing logs it, since production registers no server_error listener. Both are now boot-time refusals naming the fault. A rejecting tool executor is converted by the SDK into isError content on a 200 and does NOT reach the handler onerror, so listWorkspaces dying on a downed Docker proxy left the Nexus log silent. The executor now logs { err, tool } and rethrows, keeping the SDK's wire answer.Three assertions that could not fail, and one comment that named the wrong mechanism. deps.ts binds listWorkspaces and listSessions to their services and nothing touched runtime.deps — every other suite injects its own ToolDeps — so swapping the two bindings, or dropping the workspace argument, left the suite green. Both the result and the arguments are now asserted, and db/docker carry tags so two bare {} cannot compare deep-equal past a swap. Host-before-Origin was observable only with both headers wrong, which no test sent; swapping the operands passed everything. And the missing-Host case claimed `new Request` always derives a Host from its URL. Measured false — headers.get('host') is null and the header list empty — so the test went to the SDK validator directly and stayed green with the host leg deleted from rebindingRefusal. It now drives rebindingRefusal. mcpRuntime's doc claimed the route and the metadata endpoints both read through it. They do not: it has one caller, and both .well-known routes import mcpMetadataOptions directly. The invariant holds — that shared function is what makes it hold — so the comment now names it, and records why routing discovery through the runtime would be a regression rather than a tidy-up.Verified against the live deployment
PR running on
a.lck.sh. The body's "theHostallowlist is unverified against the real deployment" caveat is now resolved — and the answer is good.The decisive one: an unauthenticated
POST /api/mcpreturns 401, not 403.A 403
Invalid Hostwould have meant nginx forwards something the allowlist doesn't match — and every MCP call would have failed with every gate green. It reached the bearer gate, so nginx passes the clientHostthrough andmcpAllowedHostnamesmatches it. The challenge also carries the correct path-insertedresource_metadata.Both discovery documents serve, unauthenticated, with the right
resource:The rebinding defence is live, and layered more deeply than expected:
The Origin refusal is Nexus's own
rebindingRefusalfiring through nginx, so that module demonstrably runs in production. The rebound Host never reaches Nexus at all — nginx has no server block forattacker.testand 404s it first. So in this deployment the app-level Host check is a second lock rather than the only one. That is strictly better, but it does mean production traffic cannot exercise it; the unit and live-local checks are what cover that path.Still not verified
The modern 2026-07-28 leg is now verified live
The body's second caveat is resolved too. My earlier claim that this could not be tested was wrong — it needed a real token, not production access, and a local built server produces one exactly as check 7 already did.
One token, one running built server, both legs driven for comparison:
Accept200 application/json, plain JSON, both tools200 application/jsonAccept: text/plain200 application/json— no Accept check exists on this legAccept406— same server, same token200 text/event-stream, SSE-framedtools/call200, real roster instructuredContentMcp-Method400 -32020header/body mismatch, not a silent fall back401— the gate runs before the transportChecks 1 and 4 are the finding: identical credential, identical endpoint, 200 vs 406 decided purely by the envelope. That asymmetry is why a suite of claim-less POSTs looked like full coverage while testing only one leg.
A new fact, found by driving it
The first
tools/callattempt was refused:2026-07-28 requires a second header for three methods. Confirmed in the SDK rather than inferred:
With
Mcp-Method: tools/callandMcp-Name: list_workspaces, and no Accept header, it returns the real seeded roster as plain JSON.This is a client obligation the SDK enforces, so nothing in Nexus needed changing — but the only modern-leg test on the branch listed tools rather than invoking one, so the path a client actually uses had live proof and no regression guard.
e980cadadds it (asserting both the 400 without the header and the 200 with it, plus that the executor really ran), and extends AGENTS.md fact 33. Verified the test bites: neutralising the tool filter fails it.Gates after that commit: 2161 tests / 192 files, typecheck 5157 files 0 errors, lint clean.
Remaining
Only "Claude has actually connected". Everything else on the endpoint is now verified against a built server, and the discovery, challenge, Host and Origin behaviour is verified against
a.lck.shitself.Note
OAUTH_SIGNING_KEYis not set in production, which is a supported state — the key loads from the pre-existingmetarow, asGET /oauth/jwksconfirms. Setting it swaps the key and invalidates issued tokens, so before a client connects is the cheapest moment; afterwards it costs one re-authorization. The oldmetarow is not deleted by setting it.Signing key: no database, two spellings — and a simplify pass, verified regression-free
Since the last comment the signing key stopped coming from the database entirely, gained a Docker-secret form, and the branch had a final simplify pass. All of it re-verified against the built server.
The change
OAUTH_SIGNING_KEY(base64 inline) orOAUTH_SIGNING_KEY_FILE(a path to a file holding the same base64). Themetapath is gone, so "the signing key is never in the database" is now true by construction rather than by operator discipline — previously it was closed only if you set the var and manually deleted the row._FILEis the recommended production form, and the docs say why rather than just recommending it: a value inEnv:is readable by anyone who candocker inspector read/proc/1/environ— the rule AGENTS.md already stated for worker containers, applied to the same container it was always true of.Both set is a hard error, keyed on presence rather than emptiness. That is a deliberate deviation from the postgres/mysql entrypoints, which use
[ "${!var:-}" ]and so read an empty value as unset. Here an empty value is already a fault — it is what a secret that failed to mount produces — so treating it as absence would silently hand over to the other spelling.Live verification, against a rebuild of
06d8b00The nine original checks and the seven modern-leg checks re-ran byte-identical to the earlier transcript. The four key-source states are new:
/api/state/oauth/jwks/api/mcpOAUTH_SIGNING_KEYinlineOAUTH_SIGNING_KEY_FILEkidBoth-set and neither-set degrade MCP only —
/api/state, the UI and settings all answer 200, so the control plane boots normally. The refusal is explicit in the log:And the database is genuinely ignored — observed, not assumed. With a valid keyset seeded into
meta.oauth_jwksand no env var:The control that makes that meaningful: the same keyset handed in via
OAUTH_SIGNING_KEYserves 200 and a working/oauth/jwks. So the 503 above is the row being ignored, not the key being bad.Simplify, gated on more than green tests
A green suite proves tests pass, not that they still mean anything — so the simplify pass was gated on a 12-guard mutation harness that re-breaks each security guard and asserts the named test dies: audience, issuer and
typbinding; the tool-surface admission rule; the registry scope filter; Host-before-bearer ordering; the Origin leg; the HTTPS refusal; the both-spellings exclusivity; the single-key and EC P-256 checks; and the gate placement that keeps a vault key off/api/mcp.12/12 before, 12/12 after, no
NOT-APPLIED. The harness reportsNOT-APPLIEDwhen an anchor no longer matches, because a mutation that silently fails to apply is indistinguishable from a test gap.The pass found exactly one thing: the no-signing-key 503 sentence was hand-written byte-for-byte in two files — the only duplicated value literal in the branch. A rename of either variable would have updated one copy and left the two 503 surfaces telling the operator to set different things.
Its rejections were as useful. It declined to make
importJWKeager inverifier.ts, becausedeps.tsbuilds a verifier per request — an eager import would run the key import for requests refused at the Host/Origin stage, the attacker-controlled path the ordering guard exists to keep cheap. And it keptInvalidSigningKeya class rather than a factory because pino's serializer emits the class name; the boot log above showserr.type=InvalidSigningKey, so that judgement was right.Gates
typecheckexit 0 (5157 files, 0 errors) ·vitestexit 0 (192 files, 2169 tests) ·lintexit 0 ·buildexit 0 — each run as its own command with exit codes captured directly.That last detail is not pedantry:
pnpm build 2>&1 | tail -4; echo $?reports tail's status, so several earlier "exit 0" readings in this thread were not evidence.07ba490records that and the chaining race in AGENTS.md.Still the only open item
Claude has not connected. Everything else is verified. One note on ordering: setting the key swaps it and invalidates issued tokens, so wiring the secret before a client connects costs nothing, and afterwards costs one re-authorization.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.