feat(mcp): add bounded remote metadata and ingress policy by nicksavov · Pull Request #65 · draftyak/draftyak · GitHub
Skip to content

feat(mcp): add bounded remote metadata and ingress policy - #65

Draft
nicksavov wants to merge 15 commits into
m9from
m9-12-assembly-expiry
Draft

nicksavov wants to merge 15 commits into
m9from
m9-12-assembly-expiry

Conversation

@nicksavov

@nicksavov nicksavov commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor
  • Delivers M9.4 bounded untrusted metadata transport/cache and M9.5 pure native ingress helpers on current m9. No protocol changes or unmerged prerequisites. Backlog and acceptance commands: docs/roadmap/m9.md; lasting contracts: docs/roadmap/m9-boundaries.md.
  • Shares one address classifier and checked TLS connection under src/net for M2b to import. Uses PublicDnsHost, all-answer c-ares resolution, own/native-port denials, original SNI/Host and checked peer before request bytes. Internal resolver/dial/CA hooks are absent from the package entry and owner configuration.
  • Shared fetches retain bounded native jobs and removable caller waiters; callers receive independent bytes/cancellation. Cache reuse requires explicit eligible public freshness. CIMD stays within 5,120 bytes/2.5 seconds and JWKS within 1 MiB/5 seconds; stricter limits win. Native ingress requires an origin-form target before Host and trusted-proxy checks.
  • Final source: 3f4be25. Windows acceptance on this exact source: ordinary and frozen installs passed; all 2,885 workspace unit tests passed, including 256 hub tests in nine files; serial typecheck/test/build warmups passed; unchanged pnpm verify passed 7/7 in 30.8 seconds, including strict Clippy and 15 Rust tests. File and commit guards reported 0 findings in 799 files and 15 commits. The successful sequence retained at least 7.45 GiB free on two CPU cores/two Cargo jobs. Earlier pre-admission resource interruptions remain recorded separately.
  • Actual TLS tests generate certificates at runtime and check the production pin, later DNS changes, invalid certificates before HTTP bytes, redirects, oversized/chunked/gzip/truncated/stalled responses. Socket ingress cases exercise direct TLS and trusted proxies. Synthetic fixtures grant no client, document, device or session authority.
  • Public listener/issuer, document validation, consent/token lifecycle, activation/kill switch and owner-run deployment/client/platform acceptance remain open, including ADR 0006. No vendor calls or remote full-suite runs. PR62 lands first; when its scaffold reaches m9 this PR will rebase onto that actual base and preserve both export sets.

@nicksavov nicksavov left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested (3 major, 6 minor). Reviewed at a216225.

The branch merges cleanly onto m9 and changes no contract paths. Run against a loopback TLS server and DNS stub, the fetcher connects only to the address it checked, and it refuses redirects as well as oversized, compressed, stalled and truncated responses. CI's changes, pr-title, cheap and required checks pass (dco stays red until sign-off). On Windows, these pass: the frozen install, lint, vendor-guard, typecheck, Rust, build, zizmor, pnpm guard (0 findings), the spec and vector drift checks, and all 199 hub tests. There, pnpm verify's test step failed only on two machine-load timeouts, in spikes/kit and tools/verify, which this PR doesn't touch.

  1. major apps/hub/src/mcp/proxy-facts.ts:140 inspect() takes only remoteAddress, encrypted and rawHeaders, so it never sees the request target. Consider GET https://other.example/oauth/token HTTP/1.1, sent through Node's HTTP server from the trusted proxy address with the right Host, X-Forwarded-For and X-Forwarded-Proto. It gets trusted-proxy facts naming the hub's publicOrigin, while req.url names the other origin. Why: the M0.18 handoff records that oidc-provider builds endpoint URLs, the interaction returnTo and the private_key_jwt audiences it accepts from Koa's ctx.href. For an absolute-form request, that is the target itself, so a client assertion minted for another server's token URL would pass. For this reason S4 answers 421 unless the target is origin-form (spikes/s04-oauth-as/src/routes.ts:243-259). Plan §6 L3 hardening ("Host must equal publicBaseUrl") and §9.2's OAuth mix-up row rely on this binding, and an absolute target's authority overrides Host (RFC 9112 §3.2.2). Expected fix: add the native IncomingMessage.url to the input, and refuse any target that doesn't start with / before the Host checks. Add socket tests that send an absolute-form target with a correct Host on both the direct and the trusted-proxy paths.

  2. major apps/hub/test/client-metadata/network.test.ts:181 The DNS pin and the TLS peer check are tested only through mocks that can't fail them. Lines 25-36 replace node:https, node:dns/promises and node:os. The fake request never calls the lookup hook, and the tests set the fake socket's authorized and remoteAddress themselves. The one pin assertion calls the hook with { all: true }. With family set and autoSelectFamily: false, though, Node 26.12 calls it with { family: 4, hints: 0 }. So the branch production uses (src/mcp/client-metadata/https.ts:91) never runs in a test: replacing it with a fresh dns.lookup, or with a fixed 127.0.0.1, still passes the suite. The PR body's "controlled DNS/HTTPS fixtures" and the "rebinding pins" in m9-client-metadata.md:36 claim more than the suite shows. Why: plan §9.2's "SSRF; OAuth mix-up; DNS rebinding" row is gated by "SSRF matrix; rebinding … tests", and M9's acceptance is "The SSRF matrix is blocked". S4 tested the same properties over real TLS (spikes/s04-oauth-as/test/ssrf.test.ts:183 and :236), through the resolve, dial and ca seams that the M0.19 handoff asks M9's unit tests to use. Expected fix: add a real-socket suite for getPinnedMetadata and ClientMetadataFetcher. Run it against a loopback HTTPS server with a certificate made at test time, reached through an internal seam (resolver, dial target, CA) that neither the package entry nor configuration exposes. Assert that:

    • the server sees one connection, on the checked address, while later DNS answers change;
    • a wrong-name or untrusted certificate is refused before any request bytes arrive;
    • a 302, a chunked body one byte over the cap, gzip and a stalled body are each refused.

    In the existing test, also call the hook with { family: 4, hints: 0 } and assert that it returns the pin.

  3. major docs/roadmap/m9.md (missing) There is no M9 backlog. docs/roadmap/m9.md exists neither on m9 nor in this PR, the PR body names no item, and nothing states owned paths or acceptance commands, so the scope can't be checked. The branch name implies an M9.12 that nothing defines. The code also sits under apps/hub/src/mcp/. But fetching CIMD documents and jwks_uri is the authorization server's client authentication (plan §6), and the proxy and Host rules cover every L3 route, not only /mcp. Your earlier draft of this backlog, on the retired m9-01…m9-03 branches, gave this work to M9.4 (apps/hub/src/oauth-as/{cimd,guarded-fetch,…}.ts) and M9.5 (apps/hub/src/listeners/l3/**). Why:

    • plan §12 Work rules: each milestone writes docs/roadmap/mN.md, with inputs, outputs, owning paths and an acceptance test;
    • plan §3 lists mcp and oauth-as as separate modules;
    • AGENTS.md:84 and CONTRIBUTING.md:52: change only the paths your item owns;
    • integration.md: the PR body lists the items it delivers and their acceptance outputs.

    Expected fix: add docs/roadmap/m9.md with the items this PR delivers (ids, inputs, outputs, owned paths and acceptance commands). Reconcile it with plan §6 and §12, and note what waits for ADR 0006 (the CIDR filter and the proxy header rules). Move the modules to the paths those items own, and name the items in the PR body with their acceptance outputs.

  4. minor apps/hub/src/mcp/client-metadata/addresses.ts:60 A deniedAddresses entry written as IPv4-mapped IPv6 never matches the A record it names. canonicalAddress (lines 39-43) keeps ::ffff:8.8.8.8 as ::ffff:808:808. So validateAnswers([{ address: "8.8.8.8", family: 4 }], ["::ffff:8.8.8.8"]) returns the pin, while ["8.8.8.8"] refuses, and the constructor (index.ts:58-63) accepts the mapped spelling silently. Dual-stack Node sockets report IPv4 addresses in that form, so a hub address taken from a socket fails open. Why: plan §4 step 6 always blocks the hub's own addresses. The M0.12 handoff's broker check reads IPv4-mapped and IPv4-compatible IPv6 by their embedded IPv4 and canonicalizes configured addresses, and says M9 must apply the same check. This PR's own ip() in proxy-facts.ts already folds mapped IPv4. Expected fix: when building the deny set (in the constructor and in resolvePublic), fold mapped addresses to dotted IPv4, and fold or refuse IPv4-compatible ones. Alternatively, refuse mapped spellings in deniedAddresses. Add a test with a mapped deny entry against an A answer.

  5. minor apps/hub/src/mcp/client-metadata/addresses.ts:10 The resolve-then-connect guard (address classifier, own-address denial and pinned connect) is private to the metadata fetcher. Your broker branch m5-26-google-payload already holds a second copy (apps/hub/src/plugins/broker/addresses.ts) that has drifted. It has no 168.63.129.16 block, and it resolves with dns.lookup, which reads the hosts file, instead of a c-ares Resolver. The destination-host check (policy.ts:106-114) also re-implements the merged PublicDnsHost (packages/protocol/src/tool-v1/egress.ts:17-20), but without its refusal of names under localhost, local, internal, home.arpa and onion. Why: plan §9.2's SSRF and rebinding row names one resolve-then-connect guard, §4 step 6 defines it for the broker, and §6 sends CIMD and jwks_uri fetches "through the SSRF-guarded fetch". Expected fix: put the classifier and the pinned connect in one hub module that the broker can import, with parameters for the broker's port denials and local-rule exception, and record that in the PR body as a handoff for M2b. Validate destination hosts with PublicDnsHost from @draftyak/protocol/tool-v1.

  6. minor apps/hub/src/mcp/client-metadata/index.ts:102 A fetch for a URL that is already in flight is refused instead of sharing the first fetch. Each ChatGPT token request fetches the same jwks_uri, so when two overlap (a code exchange and a refresh, or two refreshes), one of them fails client authentication. And because keys are fetched before an assertion is verified, anyone's token requests naming that client can keep the URL busy. The cache won't absorb this. metadataCacheTtl returns 0 when any response header is outside its 13 names (cache.ts:10-24, 56), and Cloudflare-fronted hosts such as chatgpt.com and claude.ai add others, cf-ray among them. Why: under plan §6, ChatGPT's client uses private_key_jwt with jwks_uri, and the M0.19 handoff records one JWKS fetch per ChatGPT token request. Expected fix: keep one in-flight promise per key that concurrent callers await, each getting its own copy and honouring its own signal, and count it as one concurrency slot. Decide cache eligibility from Cache-Control, Vary, Set-Cookie and the authentication headers instead of refusing every unlisted header name. Or drop this cache, if oidc-provider's own caching is the intended reuse.

  7. minor apps/hub/src/mcp/client-metadata/limits.ts:7 JWKS has no purpose ceiling below the 10,000 ms configuration maximum (lines 20-29), and getPinnedMetadata gives it no deadline of its own (https.ts:42). The JWKS fetch runs inside the token request, which may also load the CIMD document (up to 2,500 ms). So an accepted configuration lets one token request spend 12.5 s on fetches alone, and network.test.ts:805 pins the 10 s case. Why:

    • plan §6: "Token endpoint. Form-urlencoded, under 10 s";
    • the §13 OAuth AS suite: "under 10 s";
    • the M0.19 handoff records one JWKS fetch per ChatGPT token request;
    • S4's guarded fetch defaulted to 5,000 ms.

    Expected fix: give JWKS a fixed purpose ceiling that keeps the CIMD fetch, the JWKS fetch and the rest of the token request under 10 s (for example 5,000 ms), and add a test that configuration can't raise it.

  8. minor docs/roadmap/m9-integrated-foundations.md:13 (and the other eight docs/roadmap/m9-*.md notes) The nine notes are a work log, and parts are already wrong:

    • They say verification is pending or unrun (this line, m9-client-metadata.md:33-34, m9-peer-policy.md:5-6, m9-final-assembly-deadline.md:30-31), while the PR body reports pnpm verify 7/7.
    • Their test counts disagree: 93, 102, 131, 175, 187, 197 and 199.
    • They start from commit ids that change on rebase-merge: 0348f47, 079c28d, 98af3e1, ab4de94, and 4059414 from a retired branch.
    • Two cite .cache/reports/*.json (m9-metadata-collector.md:40, m9-proxy-socket-contract.md:43), which .gitignore excludes.
    • m9-client-metadata.md:7 and limits.ts:3 credit the 5 KiB cap to "the plan's 5 KB", but the plan sets no CIMD size. 5 KiB is oidc-provider's own cap, which S4 kept (spikes/s04-oauth-as/src/cimd.ts:421).

    Why: per AGENTS.md:16, docs/roadmap holds milestone backlogs and their appendices. Expected fix: move the lasting facts into the M9 backlog or one appendix:

    • each limit and its source;
    • the cache rules;
    • the proxy-facts contract and its dependency on ADR 0006;
    • the composition duties: close the fetcher on a policy, epoch or lock change; apply per-IP limits before any lookup; list the hub's public and tunnel addresses in deniedAddresses.

    Delete the rest, and fix the 5 KiB attribution.

  9. minor apps/hub/test/client-metadata/network.test.ts:609 expect(result.body).toEqual(body) compares a 1 MiB Buffer element by element. In CI run 39139843522, the cheap job's 1,048,576-byte case took 26,701 ms against the 30,000 ms testTimeout (vitest.shared.ts:48), so a slower runner will turn the required check red. Why: cheap feeds the required check on every PR. Expected fix: compare with Buffer.compare(result.body, body) or .equals() (about 3 ms), here and at line 579.

@nicksavov

Copy link
Copy Markdown
Contributor Author

@nicksavov nicksavov left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested (1 major and 6 minor open). Re-reviewed at 3f4be25 (earlier review at a216225).

Findings 1, 2, 4, 6 and 9 are fixed, and 3, 5, 7 and 8 are partly fixed. What remains of 5 is major, because the shared guard now sits on m9, where M2's broker can't import it. On Windows these pass: the frozen install, all 256 hub tests, typecheck, build, pnpm guard (0 findings) and the spec and vector checks. The branch merges cleanly onto m9 (dc123b6), and the hub and protocol tests pass there too. CI's cheap, required, changes and pr-title checks pass (dco stays red until sign-off). pnpm verify failed only on tools/verify's .env.live worktree-scan timeout, which comes from the dev box's 800 worktrees; a fix is landing on m0 and reaches m9 with the next m1 sync.

Earlier findings:

  1. Fixed (ec01f8a).

  2. Fixed (4150227).

  3. Partly fixed (minor): docs/roadmap/m9.md now gives M9.4 and M9.5 inputs, outputs, owned paths and commands, the code sits under oauth-as and listeners/l3, and the ADR 0006 dependency is recorded. The reconciliation with plan §6 and §12 is still missing:

    • No item delivers the static /status page or, if S3 found it conforms, session-pending URL elicitation. No item carries the M9 acceptance line "a scripted 2026 client gets the /status URL, and its retry succeeds only after the desktop approval; an auto-accepting client gains nothing".
    • M9.2 (line 16) promises exact redirect binding, without §6's port-agnostic loopback redirects for allowlisted all-loopback CIMD clients (remote Claude Code). Lines 41-42 enable no vendor host by default, and no item seeds §6's claude.ai and chatgpt.com allowlist entries.
    • M9.3 lacks "the public page shows no accounts". M9.6 doesn't name the plan's four fakes (claude.ai and ChatGPT seeded from the recorded documents, remote Claude Code, Codex), docs/guides/mcp-clients.md, or the S4 fallback (SSH forward plus agent token).
    • M9.1 and M9.6 own no paths, and only M9.4 and M9.5 have an acceptance test (§12 Work rules). No item owns apps/hub/test/helpers/metadata-tls.ts, which both M9.4's and M9.5's tests import, or this PR's edits to the hub manifest, configs and src/index.ts (AGENTS.md:84).

    Expected fix: add the missing deliverables and acceptance lines (a new item for /status and URL elicitation, or an extended M9.1 or M9.5), and give every item owned paths and an acceptance test. Or say plainly that the rows outside M9.4 and M9.5 are placeholders, to be completed before those items start. Either way, list apps/hub/test/helpers/** under M9.4 or M9.5, and mark the scaffold edits as shared with M2.1.

  4. Fixed (4150227).

  5. Partly fixed (major): src/net is one module. It imports no OAuth code, validates hosts with PublicDnsHost, and keeps the hub-port, inspector-port, own-address and metadata-address denials whatever a caller passes. But the broker still can't import it:

    • Plan §12's M2 row gives the broker's DNS guard to M2 stream 2b (#62's M2.17, plugins/broker/**).
    • Milestone branches only receive earlier milestones (m2 is merged into m9, never the reverse), so a module that exists only on m9 can't reach M2.17 before M2 is merged.
    • #62's m2.md:33 now lists "M9 shared network classifier/pinned connector" as an M2.17 input. That leaves M2.17 two choices: copy the guard, which plan §9.2's single resolve-then-connect guard rules out, or wait for M9.
    • apps/hub/src/net isn't a plan §3 module either, so §11's CODEOWNERS rule (the † modules) wouldn't cover it.

    Expected fix, per integration.md's dependency rule (move what an earlier item needs into the earlier PR): submit the guard in a PR into m2 with its own tests (the src/net cases from shared-network.test.ts and a real-TLS case for the pinned connection). Put it under a path M2.17 owns inside plugins† (plan §3), and record it in m2.md. Once m2 is merged into m9, rebase this PR so M9.4 imports it. Change m9.md:18 so M9.4 consumes the guard instead of owning it, and move the handoff at m9-boundaries.md:46-50 into m2.md.

  6. Fixed (628bcea).

  7. Partly fixed (minor): the code is right. JWKS_TIMEOUT_MS is 5,000, configuration can't raise it, getPinnedMetadata applies it on its own, and CIMD's 2.5 s plus one 5 s JWKS fetch fits the 10 s token endpoint. But both JWKS tests take their waits from the constant under test (network.test.ts:824 and :1102), so setting it back to 10,000, the original defect, still passes. The CIMD test uses literal times and does catch the same change. Expected fix: use literal 4,999 and 5,000 ms in both JWKS tests, or assert JWKS_TIMEOUT_MS === 5000 and CIMD_TIMEOUT_MS + JWKS_TIMEOUT_MS < 10_000.

  8. Partly fixed (minor): the nine notes are gone, and the 5,120-byte cap is credited to oidc-provider and S4. But m9-boundaries.md:16 says the 2,500 ms CIMD limit is "required by the remote OAuth design". The plan sets no CIMD time: 2,500 ms is oidc-provider 9.12.2's timeout on every outbound fetch, jwks_uri included (lib/helpers/fetch_request.js:225, AbortSignal.timeout(2500)). Expected fix: credit it to that default. Also note that if M9.2 passes the provider's fetch signal through, JWKS fetches stop at 2.5 s too.

  9. Fixed (08a0851).

New findings:

  1. minor apps/hub/src/net/addresses.ts:206 The guard's local-rule exception can never reach loopback, and port 1455 isn't a mandatory denial.

    • validateEgressAnswers adds every networkInterfaces() address to the own-address set, including the loopback interface's 127.0.0.1 and ::1 (lines 206-209), and allowLocal can't override that set. So only loopback addresses that no interface holds, such as 127.0.0.2, get through; shared-network.test.ts:62-74 pins this.
    • HUB_DENIED_PORTS (line 19) holds only 47613-47615 and 9229, so 1455 is refused only when a caller lists it (shared-network.test.ts:96).

    Why:

    • Plan §4 step 6 blocks loopback "unless the rule is local", and it names loopback separately from the hub's addresses.
    • §5's local presets are Ollama at 127.0.0.1:11434, LM Studio :1234, llama.cpp (8080, 9931) and vLLM :8000.
    • M0.12's merged rule counts hub addresses "besides loopback" and only blocks ports on loopback (spikes/s02-plugin-sandbox/src/broker-stub.ts:29, 109-114).
    • Plan §1's Transient row and the M0.26 handoff have the broker refuse 1455 like 47613-47615.

    As handed to M2b (m9-boundaries.md:46-48), the broker couldn't reach any local preset. Expected fix, in the m2 PR from finding 5 or here: leave loopback (127.0.0.0/8 and ::1) out of the interface-derived own-address set, keep configured and non-loopback interface addresses denied, and add 1455 to HUB_DENIED_PORTS. Test that with allowLocal, 127.0.0.1:11434 and [::1]:11434 pass while 47613-47615, 9229 and 1455 stay refused there, and that without it both addresses are refused. Update m9-boundaries.md:24-34. Metadata fetches don't change, since they never allow local.

  2. minor apps/hub/src/net/addresses.ts:22 The address tables (lines 22-47) leave out four blocks that oidc-provider 9.12.2 refuses itself: 192.31.196.0/24 and 192.175.48.0/24 (AS112), 192.52.193.0/24 (AMT) and 2620:4f:8000::/48 (AS112). So publicAddress accepts, for example, 192.31.196.1 and 2620:4f:8000::1. Why: once M9.2 routes oidc-provider's CIMD and jwks_uri fetches through this guard via configuration.fetch, the provider's own special-use check is bypassed. The M0.19 handoff records that S4's guard covers oidc-provider's whole special-use list for that reason, and S4's merged table includes all four (spikes/s04-oauth-as/src/cimd.ts:33-52). Expected fix: add the four blocks, with a test that refuses one address from each.

  3. minor apps/hub/package.json:30 reflect-metadata 0.2.2 is a new dependency outside the census, and no backlog records it. Its only use is letting test/helpers/metadata-tls.ts:1 load @peculiar/x509, whose tsyringe dependency needs Reflect metadata readers. Why: m0.md's Version pins mark every version outside the census, and m1.md's D12 has the owner check such pins before their first PR. The merged spikes load the same @peculiar/x509 2.2.0 with two no-op readers instead (spikes/s04-oauth-as/test/connectors.ts:90-95, spikes/s12-sms/src/hub-standin.ts:75-82). Expected fix: drop reflect-metadata, and install those two readers before a dynamic import of @peculiar/x509 in the helper. Or record the pin in m9.md as outside the census, for the owner's check.

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