Skip to content

Commit 1c9f789

Browse files
antfubotantfu
antfubot
and
antfu
authored
docs: add security remediation plans from /improve audit (#323)
Co-authored-by: Anthony Fu <github@antfu.me>
1 parent d6150dd commit 1c9f789

7 files changed

Lines changed: 878 additions & 0 deletions

‎plans/002-authenticate-mcp-http.md‎

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
# Plan 002: Require authentication on route-based MCP
2+
3+
> **Executor instructions**: Follow this plan step by step. Run every verification command and confirm the expected result before moving on. If a STOP condition occurs, stop and report it instead of weakening authorization. Update this plan's row in `plans/README.md` when complete unless a reviewer owns the index.
4+
>
5+
> **Drift check (run first)**: `git diff --stat 2d978f84..HEAD -- packages/devframe/src/adapters packages/devframe/src/types/devframe.ts packages/devframe/src/cli packages/devframe/src/node/diagnostics.ts packages/hub/src/node packages/next/src packages/next/test packages/vite/test/single.test.ts tests/optional-mcp-bundles.test.ts examples/files-inspector/src/devframe.ts examples/hub-next docs/content/1.guide/14.security.md docs/content/1.guide/18.hub-initiate.md docs/content/2.adapters/7.mcp.md docs/content/3.frameworks/1.vite.md docs/content/3.frameworks/3.next.md docs/content/6.errors tests/__snapshots__/tsnapi`
6+
> Stop if MCP transport or route option interfaces have materially changed.
7+
8+
## Status
9+
10+
- **Priority**: P1
11+
- **Effort**: M
12+
- **Risk**: MED
13+
- **Depends on**: `plans/001-pin-github-actions.md`
14+
- **Category**: security
15+
- **Planned at**: commit `2d978f84`, 2026-09-01
16+
17+
## Why this matters
18+
19+
The MCP HTTP route currently treats a caller-provided `Origin` as authorization. `Origin` is useful for browser DNS-rebinding and cross-site request protection, but native clients can supply any value. A reachable route can therefore invoke privileged agent tools without proving identity; `@devframes/next/hub` enables this route by default.
20+
21+
## Current state
22+
23+
- `packages/devframe/src/adapters/mcp/fetch.ts` is the web-standard HTTP boundary.
24+
- `packages/devframe/src/adapters/mcp/http.ts` mounts that boundary into h3.
25+
- `packages/devframe/src/adapters/initiate.ts`, `packages/hub/src/node/initiate.ts`, and `packages/next/src/host.ts` mount route-based MCP.
26+
- `packages/devframe/src/types/devframe.ts:94-113` defines `McpRouteOptions` with only `path` and `allowedOrigins`.
27+
- `packages/devframe/src/cli/connect.ts:246-272` creates native MCP transports with only an `Origin` header.
28+
- `packages/devframe/src/cli/main.ts:13-24` constructs the native gateway.
29+
30+
The vulnerable boundary is:
31+
32+
```ts
33+
// packages/devframe/src/adapters/mcp/fetch.ts:75-85
34+
const origin = req.headers.get('origin') ?? undefined
35+
if (allowedOrigins !== false && (origin === undefined || !isAllowedOrigin(origin, allowedOrigins ?? [])))
36+
return new Response('Forbidden: origin required', { status: 403 })
37+
return handler.fetch(req)
38+
```
39+
40+
Tool invocation occurs at `packages/devframe/src/adapters/mcp/build-server.ts:287-305`. Keep the origin check as a separate defense; do not replace it with authentication. Node-side failures use coded diagnostics, and public API changes require fresh `tsnapi` snapshots after a build.
41+
42+
## Target authorization contract
43+
44+
Implement this exact, independent MCP authorization model:
45+
46+
- Add `McpRouteOptions.authorization` with three accepted values: a non-empty bearer token string, a callback `(request: Request) => boolean | Promise<boolean>`, or explicit `false` for an origin-only local opt-out.
47+
- `mcp: true` reads its bearer from `DEVFRAME_MCP_AUTH_TOKEN`. Missing/empty configuration fails startup with a new coded diagnostic instead of mounting a route.
48+
- An object MCP config must include `authorization`; omission fails with the same diagnostic.
49+
- The origin gate runs first and authorization second. Missing/invalid bearer credentials return `401` plus `WWW-Authenticate: Bearer`; disallowed origins remain `403`.
50+
- Compare configured token strings in constant time. A callback cannot disable origin checking.
51+
- `devframe connect` reads `DEVFRAME_MCP_AUTH_TOKEN` by default. `ConnectServerOptions.authToken` accepts either one token string or `(record: DevframeInstanceRecord) => string | undefined` for callers connecting to instances with distinct credentials.
52+
- `@devframes/next/hub` changes its omitted MCP default from enabled to disabled. Callers opt in with an explicit authorization policy.
53+
- Never place an MCP token in URLs, connection metadata, instance registry records, logs, diagnostics, tool payloads, or command-line arguments.
54+
55+
## Commands you will need
56+
57+
| Purpose | Command | Expected on success |
58+
|---|---|---|
59+
| MCP tests | `pnpm exec vitest run packages/devframe/src/adapters/mcp/__tests__/mcp-http.test.ts packages/devframe/src/adapters/__tests__/initiate.test.ts` | all tests pass |
60+
| Host tests | `pnpm exec vitest run packages/hub/src/node/__tests__/initiate.test.ts packages/next/test/handler.test.ts` | all tests pass |
61+
| Compatibility tests | `pnpm exec vitest run packages/devframe/src/adapters/__tests__/dev.test.ts packages/vite/test/single.test.ts tests/optional-mcp-bundles.test.ts examples/hub-next/tests/next-devframe-hub.test.ts` | all tests pass |
62+
| Typechecks | `pnpm --filter devframe typecheck && pnpm --filter @devframes/hub typecheck && pnpm --filter @devframes/next typecheck` | exit 0 |
63+
| API snapshots | `pnpm build && pnpm exec vitest run tests/exports.test.ts -u` | only intended public snapshots change |
64+
| Full verification | `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` | every command exits 0 |
65+
66+
## Scope
67+
68+
**In scope**:
69+
70+
- `packages/devframe/src/adapters/mcp/fetch.ts`
71+
- `packages/devframe/src/adapters/mcp/http.ts`
72+
- `packages/devframe/src/adapters/mcp/__tests__/mcp-http.test.ts`
73+
- `packages/devframe/src/adapters/_shared.ts`
74+
- `packages/devframe/src/adapters/cac.ts`
75+
- `packages/devframe/src/adapters/initiate.ts`
76+
- `packages/devframe/src/adapters/__tests__/initiate.test.ts`
77+
- `packages/devframe/src/adapters/__tests__/dev.test.ts`
78+
- `packages/devframe/src/types/devframe.ts`
79+
- `packages/devframe/src/cli/connect.ts`
80+
- `packages/devframe/src/cli/main.ts`
81+
- New `packages/devframe/src/cli/connect.test.ts`
82+
- `packages/devframe/src/node/diagnostics.ts`
83+
- One new `docs/content/6.errors/DFxxxx.md` for missing MCP authorization
84+
- `packages/hub/src/node/initiate.ts`
85+
- `packages/hub/src/node/__tests__/initiate.test.ts`
86+
- `packages/next/src/host.ts`
87+
- `packages/next/src/hub.ts`
88+
- `packages/next/test/handler.test.ts`
89+
- `packages/vite/test/single.test.ts`
90+
- `tests/optional-mcp-bundles.test.ts`
91+
- `examples/files-inspector/src/devframe.ts`
92+
- `examples/hub-next/src/client/devframe/next-devframe-hub.ts`
93+
- `examples/hub-next/tests/next-devframe-hub.test.ts`
94+
- `tests/__snapshots__/tsnapi/devframe/types.snapshot.d.ts`
95+
- `tests/__snapshots__/tsnapi/devframe/adapters/mcp.snapshot.d.ts`
96+
- `tests/__snapshots__/tsnapi/devframe/adapters/dev.snapshot.d.ts`
97+
- `tests/__snapshots__/tsnapi/devframe/initiate.snapshot.d.ts`
98+
- `tests/__snapshots__/tsnapi/devframe/index.snapshot.d.ts`
99+
- `tests/__snapshots__/tsnapi/devframe/internal.snapshot.d.ts`
100+
- `tests/__snapshots__/tsnapi/@devframes/hub/initiate.snapshot.d.ts`
101+
- `tests/__snapshots__/tsnapi/@devframes/next/hub.snapshot.d.ts`
102+
- `docs/content/1.guide/14.security.md`
103+
- `docs/content/1.guide/18.hub-initiate.md`
104+
- `docs/content/2.adapters/7.mcp.md`
105+
- `docs/content/3.frameworks/1.vite.md`
106+
- `docs/content/3.frameworks/3.next.md`
107+
108+
**Out of scope**:
109+
110+
- RPC/browser authentication and remote-dock tokens.
111+
- Shared-state filtering; Plan 003 owns it.
112+
- MCP tool argument validation and safety annotations.
113+
- Stdio MCP's local transport.
114+
- Compatibility code that silently preserves unauthenticated HTTP behavior.
115+
116+
## Git workflow
117+
118+
- Use the assigned worktree; branch if needed: `fix/authenticate-mcp-http`.
119+
- Commit style: `fix(devframe): authenticate HTTP MCP requests`.
120+
- Do not push/open a PR unless instructed by the operator.
121+
122+
## Steps
123+
124+
### Step 1: Add the MCP authorization policy
125+
126+
Add `authorization` to `McpRouteOptions` and matching MCP handler options. Implement one internal authorization function in `fetch.ts`: parse exactly one `Authorization: Bearer <token>` credential for string policies, compare it with the configured value using the existing crypto-token utility, invoke callback policies, and bypass identity only for explicit `false`. Reject malformed, empty, or multiple credentials without logging them.
127+
128+
Define `mcp: true` as shorthand for `authorization: process.env.DEVFRAME_MCP_AUTH_TOKEN`. Add the next sequential `DF` diagnostic and required error page when the shorthand has no token or an object omits authorization.
129+
130+
**Verify**: `pnpm --filter devframe typecheck` -> exit 0.
131+
132+
### Step 2: Enforce both HTTP gates
133+
134+
In `createMcpFetchHandler.handle`, retain origin validation, then authorize before calling `handler.fetch(req)`. Add tests for allowed Origin with no/wrong/correct bearer, disallowed Origin with correct bearer, callback allow/deny, and explicit `authorization: false`.
135+
136+
Use generic response bodies. No response may reveal whether a supplied token was close to correct.
137+
138+
**Verify**: `pnpm exec vitest run packages/devframe/src/adapters/mcp/__tests__/mcp-http.test.ts` -> all tests pass.
139+
140+
### Step 3: Wire every route and disable the Next default
141+
142+
Propagate the MCP authorization policy through `initDevframe`, `initHub`, and the Next host. The behavior matrix is:
143+
144+
| MCP setting | HTTP behavior |
145+
|---|---|
146+
| omitted/`false` | route absent |
147+
| `true` + non-empty environment token | requires that bearer |
148+
| `true` + missing token | coded startup failure; route absent |
149+
| object + token | requires that bearer |
150+
| object + callback | delegates identity to callback |
151+
| object + `authorization: false` | explicit origin-only opt-out |
152+
153+
Change `createNextDevframeHub` from `mcp: options.mcp ?? true` to the secure disabled default. Update existing hub/Next tests that currently expect Origin-only success.
154+
155+
**Verify**: `pnpm exec vitest run packages/devframe/src/adapters/__tests__/initiate.test.ts packages/hub/src/node/__tests__/initiate.test.ts packages/next/test/handler.test.ts` -> all tests pass.
156+
157+
### Step 4: Preserve the native gateway through explicit credentials
158+
159+
Add `ConnectServerOptions.authToken?: string | ((record: DevframeInstanceRecord) => string | undefined)`. `main.ts` passes `process.env.DEVFRAME_MCP_AUTH_TOKEN`; do not add a CLI flag because command-line secrets are process-visible. Resolve the token for each record and pass it into `withInstanceClient`, which sets the Authorization header. An unauthorized instance reports auth-required and never retries without authentication.
160+
161+
Add focused tests with fake SDK transports or the smallest extracted header helper. Prove the token is in request headers but absent from indexed results and formatted errors.
162+
163+
**Verify**: `pnpm exec vitest run packages/devframe/src/cli/connect.test.ts` -> all tests pass.
164+
165+
### Step 5: Update docs and API snapshots
166+
167+
Update the scoped docs to distinguish origin validation from identity, explain `DEVFRAME_MCP_AUTH_TOKEN`, document callback/explicit-false policies, and state that the Next hub no longer enables MCP by default. Update runnable examples: use an explicit environment-backed authorization policy where they demonstrate MCP; use explicit `authorization: false` only in test fixtures that are provably loopback-bound. Follow repository terminology: use “node side”, “RPC client”, and “host framework”; avoid bare “client”, “server”, and “host” in prose.
168+
169+
Run `pnpm build && pnpm exec vitest run tests/exports.test.ts -u`, inspect the diff, and keep only listed snapshots whose public types actually changed.
170+
171+
**Verify**: `pnpm test` -> build, tests, and API snapshots pass.
172+
173+
## Test plan
174+
175+
- Allowed Origin + no/invalid bearer -> 401.
176+
- Disallowed Origin + valid bearer -> 403.
177+
- Valid configured bearer -> initialize/list/call succeeds.
178+
- Callback policy allow/deny -> success/401.
179+
- Explicit `authorization: false` + allowed Origin -> succeeds.
180+
- `mcp: true` without environment token -> coded startup failure.
181+
- Next hub omitted default -> no route.
182+
- Native gateway forwards the selected per-instance bearer and never serializes it.
183+
184+
## Done criteria
185+
186+
- [ ] No route reaches `handler.fetch(req)` without passing both applicable gates.
187+
- [ ] Every route mount uses an explicit MCP authorization policy.
188+
- [ ] `Origin` is documented and tested as request hardening, not identity.
189+
- [ ] The Next hub defaults MCP to disabled.
190+
- [ ] Credentials occur only in configuration and Authorization headers.
191+
- [ ] Targeted tests, listed typechecks, API snapshots, and full verification pass.
192+
- [ ] Only in-scope files and `plans/README.md` changed.
193+
194+
## STOP conditions
195+
196+
- A supported connector can be preserved only by publishing a bearer in metadata, URLs, registry data, logs, or command arguments.
197+
- Route authorization cannot be wired without coupling it to browser/RPC token storage.
198+
- A host framework bypasses `createMcpFetchHandler` and would remain unauthenticated.
199+
- The token resolver would need to expose credentials through MCP tool arguments/results.
200+
- API snapshot changes include unrelated exports.
201+
202+
## Maintenance notes
203+
204+
Every future HTTP transport must keep identity authorization separate from Origin/Host validation. Reviewers should trace all `mountMcpHttp` and `createMcpFetchHandler` call sites and verify credentials never enter diagnostics. Multi-instance callers should use the resolver form rather than sharing one token unless shared configuration is intentional.

‎plans/003-enforce-mcp-state-policy.md‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
# Plan 003: Enforce shared-state exposure policy on direct MCP reads
2+
3+
> **Executor instructions**: Follow this plan step by step and run each verification command. Stop on a listed STOP condition. Update this plan's status row in `plans/README.md` when complete unless a reviewer owns the index.
4+
>
5+
> **Drift check (run first)**: `git diff --stat 2d978f84..HEAD -- packages/devframe/src/adapters/mcp/build-server.ts packages/devframe/src/adapters/mcp/__tests__/mcp-server.test.ts`
6+
> Stop if shared-state resource registration has materially changed.
7+
8+
## Status
9+
10+
- **Priority**: P1
11+
- **Effort**: S
12+
- **Risk**: LOW
13+
- **Depends on**: `plans/002-authenticate-mcp-http.md`
14+
- **Category**: security
15+
- **Planned at**: commit `2d978f84`, 2026-09-01
16+
17+
## Why this matters
18+
19+
MCP resource listing honors `exposeSharedState`, but direct `devframe://state/<key>` reads do not. A caller that knows a filtered key can bypass the policy. One shared predicate must govern listing, the built-in read tool, and direct resource reads.
20+
21+
## Current state
22+
23+
`packages/devframe/src/adapters/mcp/build-server.ts:202-205` already centralizes policy conversion:
24+
25+
```ts
26+
function sharedStateFilter(exposeSharedState: boolean | ((key: string) => boolean)) {
27+
if (exposeSharedState === false)
28+
return undefined
29+
return typeof exposeSharedState === 'function' ? exposeSharedState : () => true
30+
}
31+
```
32+
33+
The list path applies the predicate at lines 343-355, while the direct read at lines 377-385 calls `ctx.rpc.sharedState.get(parsed.key)` without checking it. `readStateResult` at lines 230-241 demonstrates the existing deny behavior and coded diagnostic `DF0048`.
34+
35+
## Commands you will need
36+
37+
| Purpose | Command | Expected on success |
38+
|---|---|---|
39+
| Targeted test | `pnpm exec vitest run packages/devframe/src/adapters/mcp/__tests__/mcp-server.test.ts` | all tests pass |
40+
| Typecheck | `pnpm --filter devframe typecheck` | exit 0 |
41+
| Full verification | `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` | every command exits 0 |
42+
43+
## Scope
44+
45+
**In scope**:
46+
47+
- `packages/devframe/src/adapters/mcp/build-server.ts`
48+
- `packages/devframe/src/adapters/mcp/__tests__/mcp-server.test.ts`
49+
50+
**Out of scope**:
51+
52+
- MCP HTTP authentication from Plan 002.
53+
- Changing default `exposeSharedState` values at adapter call sites.
54+
- Filtering registered agent resources; this finding concerns shared-state projections only.
55+
- New diagnostics unless existing `DF0048` cannot represent the denial.
56+
57+
## Git workflow
58+
59+
- Work in the assigned worktree; branch if needed: `fix/mcp-state-policy`.
60+
- Commit style: `fix(devframe): enforce MCP state exposure policy`.
61+
- Do not push/open a PR unless instructed.
62+
63+
## Steps
64+
65+
### Step 1: Reuse one predicate in resource handlers
66+
67+
Resolve `sharedStateFilter(exposeSharedState)` once inside `registerResourceHandlers`. Use it for both list and read. For `parsed.kind === 'state'`, reject when the predicate is absent or returns false before calling `sharedState.get`. Match the existing `DF0048` denial used by `readStateResult`.
68+
69+
Do not silently return an empty value and do not reveal whether a denied key exists.
70+
71+
**Verify**: `pnpm --filter devframe typecheck` -> exit 0.
72+
73+
### Step 2: Add bypass regression tests
74+
75+
Generalize the `bootPair` test helper so tests can supply `exposeSharedState`. Add cases proving:
76+
77+
- `false` omits state resources and rejects a direct URI read.
78+
- A predicate lists/reads allowed keys and rejects a known denied key by direct URI.
79+
- `true` retains current list/read behavior.
80+
- The built-in state-read tool and resource path agree for the same policy.
81+
82+
Use opaque key names; do not embed sensitive-looking values in tests.
83+
84+
**Verify**: `pnpm exec vitest run packages/devframe/src/adapters/mcp/__tests__/mcp-server.test.ts` -> all tests pass.
85+
86+
## Test plan
87+
88+
Model new tests after `mcp-server.test.ts:234-255`. Assert both listing and direct reads, since testing only the list would miss the vulnerability.
89+
90+
## Done criteria
91+
92+
- [ ] One predicate controls every shared-state MCP projection.
93+
- [ ] Denied direct reads fail before storage access.
94+
- [ ] Tests cover `false`, predicate allow/deny, and `true`.
95+
- [ ] Targeted test and typecheck pass.
96+
- [ ] Full repository verification passes.
97+
- [ ] Only in-scope files and `plans/README.md` changed.
98+
99+
## STOP conditions
100+
101+
- Plan 002 changed the resource registration architecture enough that the excerpts no longer match.
102+
- A denied read cannot use `DF0048` without exposing key existence; report before adding an ad-hoc error.
103+
- Registered non-state resources unexpectedly depend on `exposeSharedState`.
104+
105+
## Maintenance notes
106+
107+
Any future shared-state transport must apply the exposure predicate at the read operation, not only during discovery. Reviewers should search for all `parsed.kind === 'state'` and `sharedState.get` calls in the MCP adapter.

0 commit comments

Comments
 (0)