Skip to content

Commit 32c8be7

Browse files
antfubotantfubot
committed
docs: add security remediation plans from /improve audit
Adds 7 self-contained implementation plans (plus index) covering the highest-leverage security findings from a focused audit: unauthenticated MCP HTTP route, prototype-pollution writes in the data inspector, remote asset path traversal, OTP magic-link origin poisoning, symlink escapes in served/managed directories, and unpinned privileged GitHub Actions. No source code changed; plans/ is documentation for follow-up execution.
1 parent 2d978f8 commit 32c8be7

8 files changed

Lines changed: 1000 additions & 0 deletions

‎plans/001-pin-github-actions.md‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
# Plan 001: Pin privileged GitHub Actions dependencies
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 improvising. When complete, update this plan's row in `plans/README.md` unless a reviewer says they own the index.
4+
>
5+
> **Drift check (run first)**: `git diff --stat 2d978f84..HEAD -- .github/workflows/ci.yml .github/workflows/ecosystem-ci.yml .github/workflows/release.yml`
6+
> If an in-scope workflow changed, compare it with the excerpts below. Stop if its jobs or permissions changed materially.
7+
8+
## Status
9+
10+
- **Priority**: P1
11+
- **Effort**: S
12+
- **Risk**: LOW
13+
- **Depends on**: none
14+
- **Category**: security
15+
- **Planned at**: commit `2d978f84`, 2026-09-01
16+
17+
## Why this matters
18+
19+
The release job delegates publishing to `sxzz/workflows@main` while granting repository write and OIDC permissions. CI and ecosystem jobs also execute actions through movable branch or major-version references. Pinning each `uses:` dependency to a reviewed commit makes the executable supply-chain input immutable and auditable.
20+
21+
## Current state
22+
23+
- `.github/workflows/release.yml` owns npm publishing and release creation.
24+
- `.github/workflows/ci.yml` delegates unit checks and runs the E2E job.
25+
- `.github/workflows/ecosystem-ci.yml` executes the downstream compatibility job.
26+
27+
Current privileged reference:
28+
29+
```yaml
30+
# .github/workflows/release.yml:8-15
31+
jobs:
32+
release:
33+
uses: sxzz/workflows/.github/workflows/release.yml@main
34+
permissions:
35+
contents: write
36+
id-token: write
37+
```
38+
39+
Current CI references include `sxzz/workflows/.github/workflows/unit-test.yml@main`, `actions/checkout@v7`, `pnpm/action-setup@v6`, and `actions/setup-node@v7`.
40+
41+
Repository convention: workflow changes use two-space YAML indentation. Commits and PR titles use Conventional Commits, for example `chore: update deps`.
42+
43+
## Commands you will need
44+
45+
| Purpose | Command | Expected on success |
46+
|---|---|---|
47+
| Find mutable refs | `rg -n 'uses:\s*[^#\s]+@(main|master|v[0-9]+)$' .github/workflows` | no output after the edit |
48+
| Whitespace check | `git diff --check` | exit 0 |
49+
| Full verification | `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` | every command exits 0 |
50+
51+
## Scope
52+
53+
**In scope**:
54+
55+
- `.github/workflows/ci.yml`
56+
- `.github/workflows/ecosystem-ci.yml`
57+
- `.github/workflows/release.yml`
58+
59+
**Out of scope**:
60+
61+
- Workflow permissions, triggers, commands, and job structure.
62+
- Dependency version updates unrelated to action pinning.
63+
- Release execution, tags, version bumps, and publishing.
64+
65+
## Git workflow
66+
67+
- Work in the assigned worktree; if a branch must be created, use `fix/pin-github-actions`.
68+
- Use a Conventional Commit such as `fix(ci): pin action dependencies`.
69+
- Do not trigger a release. Push/open a PR only when instructed by the operator.
70+
71+
## Steps
72+
73+
### Step 1: Resolve immutable commits
74+
75+
For every active `uses:` entry in all three workflow files, resolve the currently referenced branch or major tag to its exact commit SHA. For GitHub-hosted actions, use `gh api repos/<owner>/<repo>/commits/<current-ref> --jq .sha` and require one 40-character result. This includes reusable workflows and ordinary actions. Pin the commit selected by the existing ref; do not substitute an unrelated latest release.
76+
77+
Keep the readable release or branch beside the SHA as a comment, for example:
78+
79+
```yaml
80+
uses: actions/checkout@<40-character-sha> # v7
81+
```
82+
83+
**Verify**: `rg -n '^\s*-?\s*uses:' .github/workflows` -> every active line ends in a 40-character hexadecimal SHA before an optional comment.
84+
85+
### Step 2: Confirm the reusable workflow contract
86+
87+
Fetch both pinned reusable workflow files with `gh api repos/sxzz/workflows/contents/.github/workflows/<file>?ref=<sha> --jq .content | base64 -d`. Save neither response. Pipe the release workflow through `rg -n 'workflow_call|publish:'`; the output must contain both keys. Pipe the unit-test workflow through `rg -n 'workflow_call|build:|test:|lint:|build-for-lint:'`; the output must contain all five keys. Do not change this repository's permissions or inputs in this plan.
88+
89+
**Verify**: run both `gh api ... | base64 -d | rg ...` checks above -> every named key is printed; then `git diff --word-diff=porcelain -- .github/workflows` -> only `uses:` revisions and adjacent version comments changed.
90+
91+
### Step 3: Run repository checks
92+
93+
Run the immutable-reference search, whitespace check, and the full repository verification command.
94+
95+
**Verify**: `rg -n 'uses:\s*[^#\s]+@(main|master|v[0-9]+)$' .github/workflows` -> no output.
96+
97+
## Test plan
98+
99+
- No new runtime tests are required; this is declarative workflow hardening.
100+
- Verify every active `uses:` line in all three files, not only the release job.
101+
- Run `git diff --check` and the full repository verification gate.
102+
103+
## Done criteria
104+
105+
- [ ] Every active action and reusable workflow reference is pinned to a reviewed 40-character commit SHA.
106+
- [ ] Each pin has a readable upstream version/branch comment.
107+
- [ ] Release permissions, triggers, and commands are unchanged.
108+
- [ ] `git diff --check` exits 0.
109+
- [ ] `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` exits 0.
110+
- [ ] No files outside the in-scope list and `plans/README.md` changed.
111+
112+
## STOP conditions
113+
114+
- An upstream reusable workflow at the resolved commit does not accept the current inputs.
115+
- GitHub API/network access is unavailable, rate-limited, or does not return one unambiguous 40-character commit SHA for a current ref.
116+
- Resolving a reference requires choosing between materially different upstream implementations.
117+
- A workflow is intentionally expected to follow a branch and no immutable release commit can be identified.
118+
- The full verification command fails twice for a reason caused by this change.
119+
120+
## Maintenance notes
121+
122+
Dependabot or Renovate should update SHA pins together with their version comments. Reviewers should verify both the upstream diff and the displayed version whenever a pin moves.

‎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.

0 commit comments

Comments
 (0)