diff --git a/plans/002-authenticate-mcp-http.md b/plans/002-authenticate-mcp-http.md new file mode 100644 index 00000000..ebca15c1 --- /dev/null +++ b/plans/002-authenticate-mcp-http.md @@ -0,0 +1,204 @@ +# Plan 002: Require authentication on route-based MCP + +> **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. +> +> **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` +> Stop if MCP transport or route option interfaces have materially changed. + +## Status + +- **Priority**: P1 +- **Effort**: M +- **Risk**: MED +- **Depends on**: `plans/001-pin-github-actions.md` +- **Category**: security +- **Planned at**: commit `2d978f84`, 2026-09-01 + +## Why this matters + +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. + +## Current state + +- `packages/devframe/src/adapters/mcp/fetch.ts` is the web-standard HTTP boundary. +- `packages/devframe/src/adapters/mcp/http.ts` mounts that boundary into h3. +- `packages/devframe/src/adapters/initiate.ts`, `packages/hub/src/node/initiate.ts`, and `packages/next/src/host.ts` mount route-based MCP. +- `packages/devframe/src/types/devframe.ts:94-113` defines `McpRouteOptions` with only `path` and `allowedOrigins`. +- `packages/devframe/src/cli/connect.ts:246-272` creates native MCP transports with only an `Origin` header. +- `packages/devframe/src/cli/main.ts:13-24` constructs the native gateway. + +The vulnerable boundary is: + +```ts +// packages/devframe/src/adapters/mcp/fetch.ts:75-85 +const origin = req.headers.get('origin') ?? undefined +if (allowedOrigins !== false && (origin === undefined || !isAllowedOrigin(origin, allowedOrigins ?? []))) + return new Response('Forbidden: origin required', { status: 403 }) +return handler.fetch(req) +``` + +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. + +## Target authorization contract + +Implement this exact, independent MCP authorization model: + +- Add `McpRouteOptions.authorization` with three accepted values: a non-empty bearer token string, a callback `(request: Request) => boolean | Promise`, or explicit `false` for an origin-only local opt-out. +- `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. +- An object MCP config must include `authorization`; omission fails with the same diagnostic. +- The origin gate runs first and authorization second. Missing/invalid bearer credentials return `401` plus `WWW-Authenticate: Bearer`; disallowed origins remain `403`. +- Compare configured token strings in constant time. A callback cannot disable origin checking. +- `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. +- `@devframes/next/hub` changes its omitted MCP default from enabled to disabled. Callers opt in with an explicit authorization policy. +- Never place an MCP token in URLs, connection metadata, instance registry records, logs, diagnostics, tool payloads, or command-line arguments. + +## Commands you will need + +| Purpose | Command | Expected on success | +|---|---|---| +| 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 | +| Host tests | `pnpm exec vitest run packages/hub/src/node/__tests__/initiate.test.ts packages/next/test/handler.test.ts` | all tests pass | +| 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 | +| Typechecks | `pnpm --filter devframe typecheck && pnpm --filter @devframes/hub typecheck && pnpm --filter @devframes/next typecheck` | exit 0 | +| API snapshots | `pnpm build && pnpm exec vitest run tests/exports.test.ts -u` | only intended public snapshots change | +| Full verification | `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` | every command exits 0 | + +## Scope + +**In scope**: + +- `packages/devframe/src/adapters/mcp/fetch.ts` +- `packages/devframe/src/adapters/mcp/http.ts` +- `packages/devframe/src/adapters/mcp/__tests__/mcp-http.test.ts` +- `packages/devframe/src/adapters/_shared.ts` +- `packages/devframe/src/adapters/cac.ts` +- `packages/devframe/src/adapters/initiate.ts` +- `packages/devframe/src/adapters/__tests__/initiate.test.ts` +- `packages/devframe/src/adapters/__tests__/dev.test.ts` +- `packages/devframe/src/types/devframe.ts` +- `packages/devframe/src/cli/connect.ts` +- `packages/devframe/src/cli/main.ts` +- New `packages/devframe/src/cli/connect.test.ts` +- `packages/devframe/src/node/diagnostics.ts` +- One new `docs/content/6.errors/DFxxxx.md` for missing MCP authorization +- `packages/hub/src/node/initiate.ts` +- `packages/hub/src/node/__tests__/initiate.test.ts` +- `packages/next/src/host.ts` +- `packages/next/src/hub.ts` +- `packages/next/test/handler.test.ts` +- `packages/vite/test/single.test.ts` +- `tests/optional-mcp-bundles.test.ts` +- `examples/files-inspector/src/devframe.ts` +- `examples/hub-next/src/client/devframe/next-devframe-hub.ts` +- `examples/hub-next/tests/next-devframe-hub.test.ts` +- `tests/__snapshots__/tsnapi/devframe/types.snapshot.d.ts` +- `tests/__snapshots__/tsnapi/devframe/adapters/mcp.snapshot.d.ts` +- `tests/__snapshots__/tsnapi/devframe/adapters/dev.snapshot.d.ts` +- `tests/__snapshots__/tsnapi/devframe/initiate.snapshot.d.ts` +- `tests/__snapshots__/tsnapi/devframe/index.snapshot.d.ts` +- `tests/__snapshots__/tsnapi/devframe/internal.snapshot.d.ts` +- `tests/__snapshots__/tsnapi/@devframes/hub/initiate.snapshot.d.ts` +- `tests/__snapshots__/tsnapi/@devframes/next/hub.snapshot.d.ts` +- `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` + +**Out of scope**: + +- RPC/browser authentication and remote-dock tokens. +- Shared-state filtering; Plan 003 owns it. +- MCP tool argument validation and safety annotations. +- Stdio MCP's local transport. +- Compatibility code that silently preserves unauthenticated HTTP behavior. + +## Git workflow + +- Use the assigned worktree; branch if needed: `fix/authenticate-mcp-http`. +- Commit style: `fix(devframe): authenticate HTTP MCP requests`. +- Do not push/open a PR unless instructed by the operator. + +## Steps + +### Step 1: Add the MCP authorization policy + +Add `authorization` to `McpRouteOptions` and matching MCP handler options. Implement one internal authorization function in `fetch.ts`: parse exactly one `Authorization: Bearer ` 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. + +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. + +**Verify**: `pnpm --filter devframe typecheck` -> exit 0. + +### Step 2: Enforce both HTTP gates + +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`. + +Use generic response bodies. No response may reveal whether a supplied token was close to correct. + +**Verify**: `pnpm exec vitest run packages/devframe/src/adapters/mcp/__tests__/mcp-http.test.ts` -> all tests pass. + +### Step 3: Wire every route and disable the Next default + +Propagate the MCP authorization policy through `initDevframe`, `initHub`, and the Next host. The behavior matrix is: + +| MCP setting | HTTP behavior | +|---|---| +| omitted/`false` | route absent | +| `true` + non-empty environment token | requires that bearer | +| `true` + missing token | coded startup failure; route absent | +| object + token | requires that bearer | +| object + callback | delegates identity to callback | +| object + `authorization: false` | explicit origin-only opt-out | + +Change `createNextDevframeHub` from `mcp: options.mcp ?? true` to the secure disabled default. Update existing hub/Next tests that currently expect Origin-only success. + +**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. + +### Step 4: Preserve the native gateway through explicit credentials + +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. + +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. + +**Verify**: `pnpm exec vitest run packages/devframe/src/cli/connect.test.ts` -> all tests pass. + +### Step 5: Update docs and API snapshots + +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. + +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. + +**Verify**: `pnpm test` -> build, tests, and API snapshots pass. + +## Test plan + +- Allowed Origin + no/invalid bearer -> 401. +- Disallowed Origin + valid bearer -> 403. +- Valid configured bearer -> initialize/list/call succeeds. +- Callback policy allow/deny -> success/401. +- Explicit `authorization: false` + allowed Origin -> succeeds. +- `mcp: true` without environment token -> coded startup failure. +- Next hub omitted default -> no route. +- Native gateway forwards the selected per-instance bearer and never serializes it. + +## Done criteria + +- [ ] No route reaches `handler.fetch(req)` without passing both applicable gates. +- [ ] Every route mount uses an explicit MCP authorization policy. +- [ ] `Origin` is documented and tested as request hardening, not identity. +- [ ] The Next hub defaults MCP to disabled. +- [ ] Credentials occur only in configuration and Authorization headers. +- [ ] Targeted tests, listed typechecks, API snapshots, and full verification pass. +- [ ] Only in-scope files and `plans/README.md` changed. + +## STOP conditions + +- A supported connector can be preserved only by publishing a bearer in metadata, URLs, registry data, logs, or command arguments. +- Route authorization cannot be wired without coupling it to browser/RPC token storage. +- A host framework bypasses `createMcpFetchHandler` and would remain unauthenticated. +- The token resolver would need to expose credentials through MCP tool arguments/results. +- API snapshot changes include unrelated exports. + +## Maintenance notes + +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. diff --git a/plans/003-enforce-mcp-state-policy.md b/plans/003-enforce-mcp-state-policy.md new file mode 100644 index 00000000..cd3c9e13 --- /dev/null +++ b/plans/003-enforce-mcp-state-policy.md @@ -0,0 +1,107 @@ +# Plan 003: Enforce shared-state exposure policy on direct MCP reads + +> **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. +> +> **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` +> Stop if shared-state resource registration has materially changed. + +## Status + +- **Priority**: P1 +- **Effort**: S +- **Risk**: LOW +- **Depends on**: `plans/002-authenticate-mcp-http.md` +- **Category**: security +- **Planned at**: commit `2d978f84`, 2026-09-01 + +## Why this matters + +MCP resource listing honors `exposeSharedState`, but direct `devframe://state/` 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. + +## Current state + +`packages/devframe/src/adapters/mcp/build-server.ts:202-205` already centralizes policy conversion: + +```ts +function sharedStateFilter(exposeSharedState: boolean | ((key: string) => boolean)) { + if (exposeSharedState === false) + return undefined + return typeof exposeSharedState === 'function' ? exposeSharedState : () => true +} +``` + +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`. + +## Commands you will need + +| Purpose | Command | Expected on success | +|---|---|---| +| Targeted test | `pnpm exec vitest run packages/devframe/src/adapters/mcp/__tests__/mcp-server.test.ts` | all tests pass | +| Typecheck | `pnpm --filter devframe typecheck` | exit 0 | +| Full verification | `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` | every command exits 0 | + +## Scope + +**In scope**: + +- `packages/devframe/src/adapters/mcp/build-server.ts` +- `packages/devframe/src/adapters/mcp/__tests__/mcp-server.test.ts` + +**Out of scope**: + +- MCP HTTP authentication from Plan 002. +- Changing default `exposeSharedState` values at adapter call sites. +- Filtering registered agent resources; this finding concerns shared-state projections only. +- New diagnostics unless existing `DF0048` cannot represent the denial. + +## Git workflow + +- Work in the assigned worktree; branch if needed: `fix/mcp-state-policy`. +- Commit style: `fix(devframe): enforce MCP state exposure policy`. +- Do not push/open a PR unless instructed. + +## Steps + +### Step 1: Reuse one predicate in resource handlers + +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`. + +Do not silently return an empty value and do not reveal whether a denied key exists. + +**Verify**: `pnpm --filter devframe typecheck` -> exit 0. + +### Step 2: Add bypass regression tests + +Generalize the `bootPair` test helper so tests can supply `exposeSharedState`. Add cases proving: + +- `false` omits state resources and rejects a direct URI read. +- A predicate lists/reads allowed keys and rejects a known denied key by direct URI. +- `true` retains current list/read behavior. +- The built-in state-read tool and resource path agree for the same policy. + +Use opaque key names; do not embed sensitive-looking values in tests. + +**Verify**: `pnpm exec vitest run packages/devframe/src/adapters/mcp/__tests__/mcp-server.test.ts` -> all tests pass. + +## Test plan + +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. + +## Done criteria + +- [ ] One predicate controls every shared-state MCP projection. +- [ ] Denied direct reads fail before storage access. +- [ ] Tests cover `false`, predicate allow/deny, and `true`. +- [ ] Targeted test and typecheck pass. +- [ ] Full repository verification passes. +- [ ] Only in-scope files and `plans/README.md` changed. + +## STOP conditions + +- Plan 002 changed the resource registration architecture enough that the excerpts no longer match. +- A denied read cannot use `DF0048` without exposing key existence; report before adding an ad-hoc error. +- Registered non-state resources unexpectedly depend on `exposeSharedState`. + +## Maintenance notes + +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. diff --git a/plans/004-contain-remote-assets.md b/plans/004-contain-remote-assets.md new file mode 100644 index 00000000..f59cf957 --- /dev/null +++ b/plans/004-contain-remote-assets.md @@ -0,0 +1,116 @@ +# Plan 004: Contain remote asset materialization inside its target directory + +> **Executor instructions**: Follow this plan step by step and run each verification command. Stop rather than improvising if provider path semantics differ from the assumptions below. Update `plans/README.md` when complete unless a reviewer owns the index. +> +> **Drift check (run first)**: `git diff --stat 2d978f84..HEAD -- packages/devframe/src/utils/remote-assets.ts packages/devframe/src/utils/remote-assets.test.ts` +> If either file changed, compare the materialization loop and successful fixture with the excerpts below; stop on a mismatch. + +## Status + +- **Priority**: P1 +- **Effort**: S +- **Risk**: LOW +- **Depends on**: none +- **Category**: security +- **Planned at**: commit `2d978f84`, 2026-09-01 + +## Why this matters + +`RemoteAssetsStore.materialize()` trusts provider-listed paths after checking only a string prefix. A compromised provider can list a prefixed path whose suffix traverses outside the requested build directory. Build materialization must reject unsafe list entries before fetching or writing them. + +## Current state + +- `packages/devframe/src/utils/remote-assets.ts` implements provider listing, caching, serving, and build materialization. +- `packages/devframe/src/utils/remote-assets.test.ts` has fake jsDelivr/unpkg providers and an existing successful materialization test at lines 182-189. + +Vulnerable loop: + +```ts +// packages/devframe/src/utils/remote-assets.ts:343-356 +for (const filePath of files.filter(f => f.startsWith(prefix))) { + const target = join(targetDir, filePath.slice(prefix.length)) + const url = provider.fileUrl(normalized.package, normalized.version, filePath) + // fetch, mkdir, writeFile(target, ...) +} +``` + +Use existing coded diagnostic `DF0064` through the local `fail()` helper. Do not add raw node-side errors. + +## Commands you will need + +| Purpose | Command | Expected on success | +|---|---|---| +| Targeted test | `pnpm exec vitest run packages/devframe/src/utils/remote-assets.test.ts` | all tests pass | +| Typecheck | `pnpm --filter devframe typecheck` | exit 0 | +| Full verification | `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` | every command exits 0 | + +## Scope + +**In scope**: + +- `packages/devframe/src/utils/remote-assets.ts` +- `packages/devframe/src/utils/remote-assets.test.ts` + +**Out of scope**: + +- CDN integrity/signature verification. +- Cache storage permissions and cache eviction. +- Request-path handling in `serve()`; it already has separate traversal tests. +- Provider API redesign. + +## Git workflow + +- Branch if needed: `fix/remote-assets-traversal`. +- Commit style: `fix(devframe): contain remote asset materialization`. +- Do not push/open a PR unless instructed. + +## Steps + +### Step 1: Validate every listed path + +Before constructing a URL or issuing a fetch, require each listed path to: + +- be a package-relative provider path, not an absolute path or URL; +- contain only `/` separators; reject any backslash rather than normalizing it; +- either lie outside the configured `prefix` and remain ignored, or lie beneath that prefix on a segment boundary and pass the remaining checks; +- have a non-empty relative suffix; +- contain no traversal after normalization. + +Resolve the final destination against `resolve(targetDir)` and require exact containment (`target === root` is not a writable file; descendants must start with `root + sep`). Account for Windows separators by using Node path primitives for filesystem containment rather than string `/` assumptions. + +Continue ignoring ordinary package files outside the selected prefix (`package.json` is present in the existing valid fixture). Reject an entry that claims to be beneath the selected prefix but has an unsafe suffix; do not fetch it. + +**Verify**: `pnpm --filter devframe typecheck` -> exit 0. + +### Step 2: Add malicious-listing regression tests + +Extend the test fake or add a small custom `RemoteAssetsProvider` that returns controlled file names. Cover: + +- a prefixed traversal entry; +- an absolute path entry; +- a backslash traversal entry, which must be rejected on every platform; +- a prefix-confusion entry, which must be ignored as outside the selected prefix; +- an ordinary outside-prefix package file, which must remain ignored without invalidating the manifest; +- a normal nested asset still materializes. + +For each rejection, assert the fetch for that file was not attempted and an outside sentinel file was not created/modified. Do not include an operating-system sensitive path in the fixture. + +**Verify**: `pnpm exec vitest run packages/devframe/src/utils/remote-assets.test.ts` -> all tests pass. + +## Done criteria + +- [ ] Unsafe provider paths fail before network fetch and filesystem mutation. +- [ ] Final destinations are proven descendants of the resolved target directory. +- [ ] Existing jsDelivr and unpkg materialization remains functional. +- [ ] Targeted test, typecheck, and full verification pass. +- [ ] Only in-scope files and `plans/README.md` changed. + +## STOP conditions + +- Provider listings intentionally use absolute URLs rather than package-relative paths. +- Correct containment requires changing the public `RemoteAssetsProvider` contract. +- A platform-specific path behavior cannot be represented by deterministic tests. + +## Maintenance notes + +Keep validation immediately before materialization even if built-in providers sanitize listings; custom providers remain an untrusted boundary. Review future bulk extraction/materialization code for the same prefix-versus-containment mistake. diff --git a/plans/005-block-data-inspector-prototype-writes.md b/plans/005-block-data-inspector-prototype-writes.md new file mode 100644 index 00000000..98e863cf --- /dev/null +++ b/plans/005-block-data-inspector-prototype-writes.md @@ -0,0 +1,112 @@ +# Plan 005: Block prototype-chain traversal and mutation in Data Inspector writes + +> **Executor instructions**: Follow this plan step by step and run every verification command. Stop if protecting object writes would require changing Map semantics. Update `plans/README.md` when complete unless a reviewer owns the index. +> +> **Drift check (run first)**: `git diff --stat 2d978f84..HEAD -- plugins/data-inspector/src/engine/normalize.ts plugins/data-inspector/src/engine/write.ts plugins/data-inspector/test/write.test.ts` +> If any file changed, compare `navigate`, `setAt`, `addTo`, and `renameAt` with the excerpts below; stop on a mismatch. + +## Status + +- **Priority**: P1 +- **Effort**: S +- **Risk**: LOW +- **Depends on**: none +- **Category**: security +- **Planned at**: commit `2d978f84`, 2026-09-01 + +## Why this matters + +Data Inspector re-descends object paths through inherited properties and assigns caller-selected keys directly. A write path can therefore reach shared prototypes and mutate behavior outside the inspected source. Object operations must remain on own properties and reject prototype-sensitive property names while preserving Map keys as data. + +## Current state + +- `plugins/data-inspector/src/engine/normalize.ts` normalizes graphs and provides `navigate()`. +- `plugins/data-inspector/src/engine/write.ts` applies set/delete/add/rename operations. +- `plugins/data-inspector/test/write.test.ts` is the canonical operation matrix. + +Current inherited traversal: + +```ts +// normalize.ts:99-107 +for (const [kind, at] of path) { + // ... + case 'k': + cur = cur instanceof Map ? cur.get(at) : (cur as Record)[at] +} +``` + +Current direct assignments occur at `write.ts:89-94` and `write.ts:194-199`. Delete already uses `Object.hasOwn` at lines 139-144; match that ownership convention. + +## Commands you will need + +| Purpose | Command | Expected on success | +|---|---|---| +| Targeted test | `pnpm exec vitest run plugins/data-inspector/test/write.test.ts` | all tests pass | +| Package typecheck | `pnpm --filter @devframes/plugin-data-inspector typecheck` | exit 0 | +| Full verification | `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` | every command exits 0 | + +## Scope + +**In scope**: + +- `plugins/data-inspector/src/engine/normalize.ts` +- `plugins/data-inspector/src/engine/write.ts` +- `plugins/data-inspector/test/write.test.ts` + +**Out of scope**: + +- Map keys named `constructor`, `prototype`, or `__proto__`; Map keys are data and must keep working. +- Query-language evaluation, RPC authentication, and normalization resource limits. +- Changing the wire shape of `WriteRequest`. +- Broad object cloning or freezing. + +## Git workflow + +- Branch if needed: `fix/data-inspector-prototype-writes`. +- Commit style: `fix(data-inspector): block prototype-chain writes`. +- Do not push/open a PR unless instructed. + +## Steps + +### Step 1: Centralize safe plain-object key checks + +Add one small internal helper for non-Map object property operations. It must reject `__proto__`, `prototype`, and `constructor` consistently with a named `WriteError` (reuse `InvalidKey` unless the current error union requires a dedicated name). Apply it to every plain-object set, add, and rename destination. Do not apply it to Map operations. + +For a set/rename source path that denotes an existing object property, require `Object.hasOwn(parent, key)` before reading or assigning. Preserve existing descriptor checks for readonly/accessor properties. For add and rename destinations, create an own data property with `Object.defineProperty(..., { configurable: true, enumerable: true, writable: true, value })` rather than bracket assignment, so an inherited setter cannot run. Continue rejecting the three prototype-sensitive names even though `defineProperty` could create them as own properties. + +**Verify**: `pnpm --filter @devframes/plugin-data-inspector typecheck` -> exit 0. + +### Step 2: Make navigation own-property-only + +In `navigate()`, retain `Map.get` behavior. For ordinary objects, return `undefined` when the requested key is not an own property before reading it. This aligns live re-navigation with the normalizer, which exposes an object's own graph rather than its prototype chain. + +Do not invoke getters solely to determine ownership. + +**Verify**: `pnpm exec vitest run plugins/data-inspector/test/engine.test.ts plugins/data-inspector/test/write.test.ts` -> all tests pass. + +### Step 3: Add regression coverage for every write shape + +Add tests proving set, add, and rename reject prototype-sensitive keys; nested inherited traversal returns `PathNotFound`; and `Object.prototype` remains unchanged after each attempt. Add a custom-prototype fixture with an inherited setter and prove add/rename creates an own data property without invoking that setter. Use `try/finally` cleanup around any prototype sentinel so a failed assertion cannot contaminate later tests. + +Add a positive test proving a Map can still use the same strings as keys. + +**Verify**: `pnpm exec vitest run plugins/data-inspector/test/write.test.ts` -> all tests pass. + +## Done criteria + +- [ ] Plain-object navigation never follows inherited properties. +- [ ] Plain-object set/add/rename reject all prototype-sensitive names. +- [ ] Maps preserve arbitrary key semantics. +- [ ] Regression tests assert global prototypes remain unchanged. +- [ ] Targeted tests, package typecheck, and full verification pass. +- [ ] Only in-scope files and `plans/README.md` changed. + +## STOP conditions + +- The normalizer deliberately exposes inherited properties elsewhere and tests rely on mutating them. +- The public `WriteError` union cannot represent rejection without a public API decision. +- A proposed fix changes Map behavior. + +## Maintenance notes + +All future write operations must use the same plain-object key helper. Reviewers should search for bracket assignment and `Object.defineProperty` in the engine before approval. diff --git a/plans/006-validate-auth-link-origin.md b/plans/006-validate-auth-link-origin.md new file mode 100644 index 00000000..8d09b31b --- /dev/null +++ b/plans/006-validate-auth-link-origin.md @@ -0,0 +1,145 @@ +# Plan 006: Validate request-derived origins before printing authentication links + +> **Executor instructions**: Follow this plan step by step. Preserve proxy/host-framework use cases only through explicit trusted configuration; never fall back to accepting an arbitrary request authority. Stop on any listed condition. Update `plans/README.md` when complete unless a reviewer owns the index. +> +> **Drift check (run first)**: `git diff --stat 2d978f84..HEAD -- packages/devframe/src/node/instance-shell.ts packages/devframe/src/adapters/initiate.ts packages/devframe/src/adapters/__tests__/initiate.test.ts packages/devframe/src/adapters/__tests__/dev.test.ts docs/content/1.guide/14.security.md docs/content/2.adapters/1.initiate.md` +> If an in-scope file changed, compare origin capture and banner timing with the excerpts below; stop on a mismatch. + +## Status + +- **Priority**: P1 +- **Effort**: M +- **Risk**: MED +- **Depends on**: none +- **Category**: security +- **Planned at**: commit `2d978f84`, 2026-09-01 + +## Why this matters + +For handler-owned hosts without an explicit public origin, the first request permanently determines the origin used in the terminal's OTP magic link. Node middleware builds that value directly from `Host`; fetch handlers trust the absolute request URL. An unauthenticated first request can redirect the credential-bearing link to another origin. + +## Current state + +- `packages/devframe/src/node/instance-shell.ts` owns late origin discovery and banner timing. +- `packages/devframe/src/adapters/__tests__/initiate.test.ts` exercises handler/middleware instances. +- `packages/devframe/src/adapters/__tests__/dev.test.ts` exercises owned listeners and wildcard binds. +- `docs/content/1.guide/14.security.md` documents the OTP fragment and trust model. + +Current origin capture: + +```ts +// instance-shell.ts:430-433 +function noteOrigin(origin: string): void { + derivedOrigin ??= origin + maybePrintBanner() + maybeRegister() +} + +// instance-shell.ts:670-674 +const host = req.headers.host +if (host) + noteOrigin(`${encrypted ? 'https' : 'http'}://${host}`) +``` + +`handleRequest()` similarly calls `noteOrigin(new URL(request.url).origin)` at lines 640-643. `interactive-auth.ts:79-90` puts this origin into the OTP URL. + +## Target trust rule + +- Explicit `options.origin` remains authoritative. +- An owned listener derives its advertised origin from the bound address/port, not an inbound Host header. +- A handler/middleware may adopt a request-derived origin only when its parsed hostname passes `isLoopbackHostname`, or when its canonical origin exactly equals an entry in the existing `allowedOrigins` array. +- If `allowedOrigins` is `false` or a dynamic `WsOriginRegistry`, request-derived origin adoption is disabled; non-loopback deployments in those modes must provide explicit `origin`. Do not honor forwarded headers. +- A rejected candidate must not print a banner, register a poisoned origin, or prevent a later valid candidate from being adopted. + +Reuse `isLoopbackHostname` from `devframe/rpc/transports/ws-server`; do not reuse `isAllowedOrigin`, because it accepts an origin-shaped string before this plan's stricter canonical-origin validation. + +## Commands you will need + +| Purpose | Command | Expected on success | +|---|---|---| +| Adapter tests | `pnpm exec vitest run packages/devframe/src/adapters/__tests__/initiate.test.ts packages/devframe/src/adapters/__tests__/dev.test.ts` | all tests pass | +| Core typecheck | `pnpm --filter devframe typecheck` | exit 0 | +| Full verification | `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` | every command exits 0 | + +## Scope + +**In scope**: + +- `packages/devframe/src/node/instance-shell.ts` +- `packages/devframe/src/adapters/initiate.ts` +- `packages/devframe/src/adapters/__tests__/initiate.test.ts` +- `packages/devframe/src/adapters/__tests__/dev.test.ts` +- `docs/content/1.guide/14.security.md` +- `docs/content/2.adapters/1.initiate.md` + +**Out of scope**: + +- General reverse-proxy support or automatic trust of `Forwarded`/`X-Forwarded-*`. +- Changes to OTP entropy, TTL, token persistence, or per-handler OTP state. +- Vite `allowedHosts` examples (finding 16 was not selected). +- Changes to WebSocket origin authorization semantics. + +## Git workflow + +- Branch if needed: `fix/auth-link-origin`. +- Commit style: `fix(devframe): validate authentication link origins`. +- Do not push/open a PR unless instructed. + +## Steps + +### Step 1: Separate candidate validation from origin adoption + +Replace unconditional `noteOrigin` with a function that canonicalizes a candidate URL and checks it against the trusted rule above. Reject credentials, paths, query strings, fragments, malformed ports, and non-HTTP(S) schemes. Compare canonical origins exactly. + +Keep the first-valid-origin behavior, not first-request behavior. Invalid candidates must be ignored without setting `derivedOrigin`. + +Ignore invalid candidates silently to avoid a request-amplified warning. Do not add a diagnostic in this plan. + +**Verify**: `pnpm --filter devframe typecheck` -> exit 0. + +### Step 2: Route both request adapters through validation + +Apply the same candidate validation to web `Request` and Node middleware paths. For owned listeners, preserve current `localhost:` behavior independently of request headers. Ensure an explicit `origin` bypasses derivation because it was supplied by the host framework. + +**Verify**: `pnpm exec vitest run packages/devframe/src/adapters/__tests__/dev.test.ts` -> all tests pass. + +### Step 3: Add first-request poisoning regression tests + +In `initiate.test.ts`, construct an instance with a banner spy and no explicit origin. Cover: + +- a first request with an untrusted authority does not print/adopt/register it; +- a later loopback request becomes the origin and prints exactly one link; +- an exactly allow-listed non-loopback origin is accepted; +- an origin that only prefix/suffix-matches an allow-listed value is rejected; +- explicit `origin` wins regardless of inbound Host; +- protocol and port are canonicalized consistently. + +Assert only the URL origin and fragment parameter presence; never snapshot a live credential value. + +**Verify**: `pnpm exec vitest run packages/devframe/src/adapters/__tests__/initiate.test.ts` -> all tests pass. + +### Step 4: Correct the public guidance + +Document that non-loopback handler deployments set `origin` explicitly and that request-derived origins are accepted only through the loopback/exact allow-list policy. Follow repository terminology and positive framing. + +**Verify**: `pnpm test` -> build, tests, and API snapshots pass. + +## Done criteria + +- [ ] No raw Host/request URL can become an OTP-link origin without validation. +- [ ] Invalid first requests do not lock out a later valid origin. +- [ ] Explicit origin and owned-listener behavior still work. +- [ ] Regression tests cover hostile-first/valid-second ordering and exact allow-list matching. +- [ ] Targeted tests, typecheck, and full verification pass. +- [ ] Only in-scope files, any required diagnostic page, and `plans/README.md` changed. + +## STOP conditions + +- A host framework requires arbitrary request-derived non-loopback origins without any explicit trusted configuration. +- Canonical origin validation would need DNS resolution in the request path. +- The change begins trusting forwarded headers implicitly. +- Existing API snapshots show an unrelated public change. + +## Maintenance notes + +The terminal magic link is a credential-delivery mechanism, so its destination must always come from trusted configuration or a strict local policy. Review future registry-origin and absolute-dock URL derivation against the same rule. diff --git a/plans/007-enforce-symlink-containment.md b/plans/007-enforce-symlink-containment.md new file mode 100644 index 00000000..aa4419a0 --- /dev/null +++ b/plans/007-enforce-symlink-containment.md @@ -0,0 +1,159 @@ +# Plan 007: Reject pre-existing symlink escapes from filesystem roots + +> **Executor instructions**: Follow this plan step by step and verify both read and mutation paths. Do not claim race-free containment if the implementation only performs lexical checks. Stop on a listed condition. Update `plans/README.md` when complete unless a reviewer owns the index. +> +> **Drift check (run first)**: `git diff --stat 2d978f84..HEAD -- packages/devframe/src/utils/serve-static.ts packages/devframe/src/utils/serve-static.test.ts plugins/assets/src/node/paths.ts plugins/assets/src/node/scanner.ts plugins/assets/src/rpc/functions/delete.ts plugins/assets/src/rpc/functions/list.ts plugins/assets/src/rpc/functions/mkdir.ts plugins/assets/src/rpc/functions/read-image-meta.ts plugins/assets/src/rpc/functions/read-text.ts plugins/assets/src/rpc/functions/rename.ts plugins/assets/src/rpc/functions/upload.ts plugins/assets/test/assets.test.ts services/open/src/index.ts services/open/test/service.test.ts` +> If any in-scope file changed, compare its path resolution/I/O call with the excerpts below; stop on a mismatch. + +## Status + +- **Priority**: P2 +- **Effort**: M +- **Risk**: MED +- **Depends on**: none +- **Category**: security +- **Planned at**: commit `2d978f84`, 2026-09-01 + +## Why this matters + +Static serving and asset RPC operations prove containment only from normalized path strings. Filesystem operations then follow symlinks, so a symlink inside an allowed root can redirect reads, writes, deletes, renames, or editor-opening outside that root. Canonical checks must reject deterministic, pre-existing symlink escapes. This plan does not claim to defeat a concurrent local process replacing path components between validation and I/O. + +## Current state + +- `packages/devframe/src/utils/serve-static.ts` serves local SPA/static roots through h3 and Connect variants. +- `plugins/assets/src/node/paths.ts` is the common lexical resolver used by asset RPC handlers. +- `plugins/assets/test/assets.test.ts` has integration coverage for lexical `..` traversal. +- `services/open/src/index.ts:70-100` uses the same lexical allowed-root model for editor/finder actions installed by the assets devframe. + +Current lexical checks: + +```ts +// serve-static.ts:65-70 +const abs = normalize(join(absDir, cleaned)) +if (abs !== absDir && !abs.startsWith(absDir + sep)) + return null +const direct = await statFile(abs) // stat follows symlinks + +// plugins/assets/src/node/paths.ts:10-16 +const normalizedRoot = resolve(root) +const absolute = resolve(normalizedRoot, cleaned) +if (absolute !== normalizedRoot && !absolute.startsWith(`${normalizedRoot}/`)) + throw diagnostics.DP_ASSETS_0001({ path: relativePath }) +``` + +The minimal correct change may use separate helpers for async static reads and synchronous asset path resolution; do not add a broad abstraction unless it genuinely fits both call patterns. + +## Commands you will need + +| Purpose | Command | Expected on success | +|---|---|---| +| Static tests | `pnpm exec vitest run packages/devframe/src/utils/serve-static.test.ts` | all tests pass | +| Asset tests | `pnpm exec vitest run plugins/assets/test/assets.test.ts` | all tests pass | +| Open-service tests | `pnpm exec vitest run services/open/test/service.test.ts` | all tests pass | +| Typechecks | `pnpm --filter devframe typecheck && pnpm --filter @devframes/plugin-assets typecheck && pnpm --filter @devframes/service-open typecheck` | exit 0 | +| Full verification | `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` | every command exits 0 | + +## Scope + +**In scope**: + +- `packages/devframe/src/utils/serve-static.ts` +- `packages/devframe/src/utils/serve-static.test.ts` +- `plugins/assets/src/node/paths.ts` +- `plugins/assets/src/node/scanner.ts` +- `plugins/assets/src/rpc/functions/delete.ts` +- `plugins/assets/src/rpc/functions/list.ts` +- `plugins/assets/src/rpc/functions/mkdir.ts` +- `plugins/assets/src/rpc/functions/read-image-meta.ts` +- `plugins/assets/src/rpc/functions/read-text.ts` +- `plugins/assets/src/rpc/functions/rename.ts` +- `plugins/assets/src/rpc/functions/upload.ts` +- `plugins/assets/test/assets.test.ts` +- `services/open/src/index.ts` +- `services/open/test/service.test.ts` + +**Out of scope**: + +- Remote asset provider paths (Plan 004). +- Upload quotas, file type/content validation, and active SVG handling. +- Supporting arbitrary symlinked asset trees through a compatibility flag. +- Filesystem sandboxing outside configured roots. + +## Git workflow + +- Branch if needed: `fix/symlink-containment`. +- Commit style: `fix: enforce symlink-aware filesystem roots`. +- Do not push/open a PR unless instructed. + +## Steps + +### Step 1: Define the symlink policy in tests first + +Add Linux/macOS tests (skip only where creating symlinks is unavailable) with a managed/served root, an outside directory, and both file and directory symlinks inside the root. Tests must prove: + +- static h3 and Node middleware return 404 for a symlink escaping the served root; +- ordinary in-root files still serve; +- asset read, upload, rename, delete, mkdir, and open-service operations cannot cross an escaping ancestor symlink; +- reads/static serving allow a symlink only when its canonical target remains inside the canonical root; +- mutations reject every pre-existing symlink path component, including symlinks whose targets remain in-root; +- the open service allows canonical in-root targets and rejects canonical escapes. + +In `scanner.ts`, explicitly configure the glob not to follow symbolic links and omit symlink entries from returned `AssetInfo` values. Reuse `DP_ASSETS_0001` for all rejected RPC paths; do not add a new diagnostic in this plan. + +Use temporary directories and never reference real system files. + +**Verify**: run all three targeted test commands -> the new escape tests fail before implementation while existing tests pass. + +### Step 2: Canonicalize static read targets + +Resolve the canonical served root once per handler construction. In `resolveTarget`, canonicalize each existing candidate and require it to remain beneath that root before returning `ResolvedFile`. Apply the check to direct files, index candidates, extension candidates, and SPA fallback. + +Recheck containment as close as practical to opening the file. `O_NOFOLLOW` may add final-component defense where portable, but do not describe it as protecting ancestor replacement races. + +**Verify**: `pnpm exec vitest run packages/devframe/src/utils/serve-static.test.ts` -> all tests pass. + +### Step 3: Canonicalize asset mutation ancestors + +Keep lexical rejection in `resolveAssetPath`, then canonicalize the root and the nearest existing ancestor of the requested target. Require that ancestor to remain within the canonical root. For existing targets, validate the target's canonical path too. + +Because uploads/mkdir may create missing components, walk existing components with `lstat` and reject every symlink before creation, then repeat the walk after directory creation and immediately before opening/renaming/deleting. Apply canonical containment to the open service's allowed-root validation. + +Preserve `DP_ASSETS_0001` for outside-root rejection; if a new node-side error is required, follow the package's existing coded diagnostics convention. + +**Verify**: `pnpm exec vitest run plugins/assets/test/assets.test.ts` -> all tests pass. + +### Step 4: Run cross-package verification + +Run the three package typechecks, targeted tests, then the complete repository gate. Check Windows-specific path handling in code even if symlink tests skip on Windows CI. + +**Verify**: `pnpm lint && pnpm knip && pnpm test && pnpm typecheck && pnpm build` -> exit 0. + +## Test plan + +- Escaping final-component file symlink. +- Escaping ancestor-directory symlink. +- Existing and not-yet-existing mutation targets. +- Both static handler implementations. +- Every assets mutation/read family and the installed open service. +- Positive ordinary nested paths and the chosen in-root symlink policy. + +## Done criteria + +- [ ] Static reads reject canonical paths outside the served root. +- [ ] Asset/open operations reject escaping symlink ancestors before I/O. +- [ ] Both final-component and ancestor symlinks have regression tests. +- [ ] Lexical `..` tests continue to pass. +- [ ] Targeted tests, all affected typechecks, and full verification pass. +- [ ] Only in-scope files and `plans/README.md` changed. + +## STOP conditions + +- Existing product behavior explicitly requires following symlinks outside configured roots. +- The accepted threat model requires protection against a concurrent local process replacing path components between validation and I/O. +- A mutation path cannot be protected without changing its public atomicity/overwrite contract. +- The open service has external consumers that require a different symlink policy from assets. +- Tests require elevated privileges or real host files. + +## Maintenance notes + +Canonical checks close pre-existing symlink escapes but do not eliminate filesystem replacement races. If the threat model includes a concurrent local attacker who can mutate the managed root, STOP and escalate to a separate design using descriptor-relative/native sandbox operations; Node's ordinary path APIs and final-component `O_NOFOLLOW` are insufficient for that claim. diff --git a/plans/README.md b/plans/README.md new file mode 100644 index 00000000..c263bd73 --- /dev/null +++ b/plans/README.md @@ -0,0 +1,35 @@ +# Security Implementation Plans + +Generated by the improve skill on 2026-09-01 at commit `2d978f84`. Execute in the order below unless dependencies say otherwise. Each executor must read its plan fully, honor STOP conditions, run every verification gate, and update its status row. + +## Execution Order And Status + +| Plan | Title | Priority | Effort | Depends on | Status | +|---|---|---|---|---|---| +| 001 | Pin privileged GitHub Actions dependencies | P1 | S | - | TODO | +| 002 | Require authentication on route-based MCP | P1 | M | 001 | TODO | +| 003 | Enforce shared-state exposure policy on direct MCP reads | P1 | S | 002 | TODO | +| 004 | Contain remote asset materialization | P1 | S | - | TODO | +| 005 | Block Data Inspector prototype-chain writes | P1 | S | - | TODO | +| 006 | Validate request-derived authentication-link origins | P1 | M | - | TODO | +| 007 | Reject pre-existing symlink escapes from filesystem roots | P2 | M | - | TODO | + +Status values: TODO | IN PROGRESS | DONE | BLOCKED (with reason) | REJECTED (with rationale) + +## Dependency Notes + +- Plan 001 lands first because the release path should stop following mutable privileged workflow code before security fixes are published. +- Plan 003 follows Plan 002 so MCP's read policy is tested behind the corrected identity boundary. It may be developed in parallel but should land immediately after Plan 002. +- Plans 004-007 are independent and can execute in separate worktrees. Each executor runs its plan's drift command before editing; only `plans/README.md` overlaps. + +## Findings Considered And Rejected + +- Dependency audit output reported critical/high advisories in `tar`, `postcss`, `sharp`, `svgo`, `brace-expansion`, and `nanoid`, but review did not establish a reachable vulnerable runtime or distribution path. Reassess when dependency call paths or advisory conditions change. +- Open Graph private-address fetching matches a tool whose purpose includes inspecting local development URLs; no separate private-network boundary is currently documented. +- Code-server workspace selection is an explicit tool input, so arbitrary folder selection alone was not treated as a containment bypass. +- Bearer-token expiration and static-token revocation behavior are lifecycle policy choices rather than implementation bypasses under the documented model. +- Executable asset formats share the user app's development trust boundary; the audit did not establish a distinct origin boundary that the current behavior violates. + +## Audit Scope + +This was a standard-effort, hotspot-weighted security audit of core RPC/auth/transports, hub browser boundaries, built-in devframes, framework kits, CI, starter, and representative examples. It did not audit correctness, performance, general test coverage, architecture, documentation quality, or product direction. Findings selected for plans were 1-6 plus the MCP shared-state dependency identified as finding 8.