fix(web): sandbox the template preview against stored XSS - #248
fix(web): sandbox the template preview against stored XSS#248NathanTarbert wants to merge 3 commits into
Conversation
NathanTarbert
changed the title
9eebd89 to
c3e38ee
Compare
CPK-8188 Review PR #248 — template preview sandbox + override persistence
CopilotKit/outpost#248 is the web half of CopilotKit/outpost#238, split out so each can be reviewed on its own. Commit Covers the code behind outpost#226 — tracked here by CPK-7972, with CPK-8043 (the sandbox) and CPK-8044 (the PUT stub) both landing in this PR. What the review already establishedFrom the #238 pass, and worth not re-deriving:
Three items open, all recorded in the PR bodyNot fixed. Two of the three were flagged as unsettled in #238's own body and shipped out of draft anyway, which is the reason for the split.
Deciding item 1 — wire it, or stop the UI claiming it — is what this ticket is for. |
c3e38ee to
472a987
Compare
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
Re-reviewed on its own, as promised. The sandbox is correct and I want it shipped — sandbox="" with neither allow-scripts nor allow-same-origin, srcDoc, no dangerouslySetInnerHTML or rehype-raw sink left in apps/web, asserted with an exact-value test. outpost-web is 732 passing across 53 files, and the loader/renderer/markdown pipeline really executes in them.
Two of the three items you recorded in the body are still open, and I am gating on them rather than filing them, for the reason you already named on #238: an item listed as unsettled in a PR body is an item that ships.
Item 2 is now #253 — your reading was right, and it is on main rather than caused by this PR, so it is not yours to fix here.
Blocker 1 — the slug traversal, and the existence checks do not close it
I ran this rather than reasoning about it. Against the real loadFromFilesystem:
slug="../README" -> READ "# Outpost\n\nAI-powered support operations platform…"
slug="../CLAUDE" -> READ "# CLAUDE.md\n\nOutpost — AI-powered customer support…"
slug="../docs/deployment" -> READ "# Deployment Guide\n\nOutpost consists of seven services…"
slug="invite/../../README" -> READ "# Outpost\n\nAI-powered support operations platform…"
GET /api/templates/[slug] at :36 calls loadTemplate(slug, findOverride) with no validation, and returns subject and body in the JSON. So an admin request for ../docs/deployment gets the deployment guide — service topology and env-var names — back over the API.
The part worth flagging beyond my original note: the new fail-closed checks are the traversal. route.ts:101 and preview/route.ts:59 both guard with if (!loadFromFilesystem(slug)). For every slug above, that read succeeds, so the guard passes and then approves the request. The check performs the escape it appears to prevent. That reads as safe on the page and is not.
..%2FREADME returns null at the loader, but Next decodes percent-encoding in a dynamic segment before the handler runs, so the route receives ../README and the plain form is what matters anyway.
listTemplateSlugs() already produces the allowlist — it returns exactly digest, escalation, invite, sla-breach, ticket-created, ticket-resolved, welcome. Testing membership in it at the route boundary closes this and makes the three existence checks cheaper at the same time, since a slug that passed the allowlist no longer needs a filesystem probe to prove it exists.
Blocker 2 — the editor still reports a save that changes nothing
Unchanged from #238: sendEmail consults an override only when handed a dbLookup (shared/src/email/sender.ts:166), and neither api/team/invite/route.ts:72 nor invite/resend/route.ts:52 passes one. This PR does not touch either file. So the write path works, every read surface confirms it — list flips to Custom, GET returns the override, the preview renders it — and the invitee gets the on-disk copy.
I am not asking for the wiring in this PR. Either of these clears it:
- pass
findOverrideasdbLookupat both call sites, with one test asserting a stored override changes what reaches the transport; or - hold the claim: no
Custombadge and no unqualified "Template saved successfully" until it is wired. A banner reading "Saved. Not yet used for outgoing email." is honest and costs one string.
The second is the one I would take if you want the sandbox in today, and I would rather not hold a real XSS fix while the email path gets designed. Given #253, note that neither option makes an edited template reach an invitee in production right now — which is an argument for the honest banner rather than the wiring, since the wiring would still not be true.
Smaller, all optional
preview/route.ts— the bodiless-POST guard keys oncontent-length !== '0', but a bodiless POST can arrive with nocontent-lengthat all (chunked, or a server-sidenew Request(url, {method:'POST'})), and that path 400s instead of previewing the stored template. The comment above it describes the intent correctly; the check is narrower than the comment.- Preview applies no length bound while PUT applies
SUBJECT_MAX/BODY_MAX, and preview needs only a session rather than ADMIN — so a largedraft.bodyruns interpolate and markdown unbounded. Reusing the two constants lines them up. - If the
subjectbound exists for header safety, a CR/LF check belongs next to it; length alone still allowsWelcome\r\nBcc: …. GETreturns an override even when the filesystem template is gone (loadTemplatesynthesises the meta) while PUT and preview both 404 — the one case where the surfaces disagree.- Reset deletes with no confirm step and renders whether or not
isOverrideis true.
Send the allowlist and whichever half of blocker 2 you prefer, and I will re-verify immediately — the sandbox itself is done and I have no notes on it.
…e preview
The templates settings screen previously read from the filesystem and stubbed its
writes. This makes the whole path real.
api/templates (list)
Merges stored overrides over the filesystem defaults in a single findMany, so
an overridden entry reports the override's subject and edit metadata and the
list shows what is actually in effect.
api/templates/[slug]
GET reads through loadTemplate, so an override wins over the default. PUT
upserts for real, with type-checked validation rather than truthiness (an
object or array in either field used to reach the upsert and surface as a
500), length bounds on both columns since the subject becomes an email header
downstream, a 404 for a slug with no filesystem template, and an editedBy that
falls back to the member id when the session carries no email. DELETE uses
deleteMany so resetting an already-default template is a no-op rather than a
Prisma 404, and reports hadOverride so the UI can say which happened.
api/templates/[slug]/preview
Accepts a draft { subject, body } and feeds it through the loader's lookup, so
the draft wins over the stored row. Without this the editor sent an empty body
and the server rendered whatever was stored, which meant an author previewed
content they were not about to save.
settings/templates
Failures now surface the route's own error message instead of a fixed string,
so an empty field reads as an empty field rather than as an outage. The
preview renders in a sandboxed iframe rather than through
dangerouslySetInnerHTML: template bodies are author-editable and stored, so
injecting them executed saved markup in every later viewer's session, and the
csrf cookie has to be readable by client JS for the double-submit header.
sandbox="" grants nothing; allow-scripts and allow-same-origin together would
let the frame drop its own sandbox, so neither is set.
Tests: 35 across two new files, covering the draft-wins preview, the validation
and bounds, idempotent reset, and the override merge. tsc --noEmit clean.
Known gaps, tracked in review notes on the PR rather than fixed here: the two
sendEmail call sites still pass no dbLookup, so a saved override does not yet
reach a real invite email; and the slug route param is not validated against
listTemplateSlugs before it reaches the filesystem.
…ating a save Addresses both blockers on #248. ## The slug reached the filesystem unvalidated `loadFromFilesystem` builds `join(dir, slug + '.md')` with no validation, and Next decodes percent-encoding in a dynamic segment before a handler runs. Reproduced against the real loader before changing anything: ../README → READ # Outpost… ../CLAUDE → READ # CLAUDE.md… ../docs/deployment → READ # Deployment Guide… invite/../../README → READ # Outpost… `GET /api/templates/[slug]` returns `subject` and `body` in its JSON, so a request for `../docs/deployment` handed back the deployment guide — service topology and env-var names — over the API. The part worth stating plainly: the fail-closed existence checks were not an incomplete defence, they were the traversal. `if (!loadFromFilesystem(slug))` SUCCEEDS for every path above, so it approved the request it appeared to reject. The check performed the escape it looked like it prevented. `isKnownTemplateSlug` tests membership in `listTemplateSlugs()`, which already returns exactly the seven real slugs. A traversal path is never a member however it is spelled, and a slug that passes is already known to exist — so the three existence probes are gone rather than supplemented. All four handlers now answer the same way for the same input; DELETE is included not because `deleteMany` on an unknown slug is dangerous but because GET returning 404 while DELETE reports `reset: true` reads as one of them being wrong. Where `templates/` is absent from the running image (outpost#253) this returns false for everything and callers 404 — the behaviour those callers already had, failing in the safe direction. ## The editor claimed a save it does not deliver `sendEmail` consults an override only when handed a `dbLookup` (shared/src/email/sender.ts:166), and neither api/team/invite/route.ts:72 nor invite/resend/route.ts:52 passes one. So the row is written, every read surface honours it — the list, GET, the preview — and the invitee receives the on-disk copy. Held the claim rather than wiring the lookup, which is the option Jerel preferred: per outpost#253 the wiring would not make an edited template reach an invitee today either, so it would trade one untrue banner for another. The banner now reads "Saved. Not yet used for outgoing email", the badge reads "Custom (preview only)", and the page description says the same. Both are pinned by tests, because the dishonest string is the shorter and more natural one to write. ## Also The bodiless-POST guard keyed on `content-length !== '0'`, but a bodiless POST can arrive with no such header at all — chunked, or a server-side `new Request(url, { method: 'POST' })` — and those got a 400 instead of a preview. It now reads the body text and treats empty as "render what is stored". That fix needed care: a request body can only be read once, so the first version called `request.json()` and then `text()`, which returns empty after the stream is consumed — an unparseable body would have read as "no body" and previewed the stored template, reintroducing the bug this route exists to fix. The text is read once and parsed here instead. The test stubs model that, since they previously stubbed only `json()`. Verification: apps/web 751 tests, typecheck 10/10, lint 10/10, test 10/10. Mutations: reverting GET to the existence probe fails 4 traversal tests; restoring the bare success claim fails the banner test. Not in this PR: the smaller optional items from the review — preview's missing length bounds, a CR/LF check on `subject`, GET answering for an override whose filesystem template is gone, and reset having no confirm step.
472a987 to
d9fc475
Compare
|
Thanks Jerel — both blockers were right, and the traversal was worse than my original note. Fixed in The slug traversalI reproduced your cases against the real loader before touching anything, and then the fix against the same inputs:
The loader still reads a traversal path — that's its nature — but nothing reaches it now. Your point about the existence checks is the part I'd have missed on my own, and it's the reason this needed gating rather than filing: the checks were the traversal. DELETE is included too. Not because On #253 — where The save claimTook your second option, and your reasoning for preferring it is what decided it: per #253 the wiring wouldn't make an edited template reach an invitee today either, so it would trade one untrue banner for another.
All pinned by tests, because the dishonest string is the shorter and more natural one to write. The bodiless-POST guardFixed, and it needed more care than it looked. My first version read Verification
I also ran the changed-file format check locally before pushing this time — it caught 5 files. That's the gate from #250 earning its place twice now. Not in this PRThe smaller optional items: preview's missing length bounds against PUT's |
Missed by my local check, which read `git status` — uncommitted changes only — so it did not see files changed in earlier commits on the branch. CI diffs the whole PR against main, which is the set that matters. Checking `git diff origin/main...HEAD` instead now agrees with CI. Whole-PR file set passes prettier; typecheck, lint and test all 10/10.
2 participants
The template preview renders in a sandboxed iframe, and a saved override is persisted and read back by every template surface. Closes #226 once the review items below are settled.
This is the web half of #238, rebased onto current
mainand unchanged from commit5427278; that PR is now Community Signal only.What's here
settings/templates/page.tsxrenders the preview viasrcDocin an iframe withsandbox=""— neitherallow-scriptsnorallow-same-origin. Asserted structurally with an exact-value test. No otherdangerouslySetInnerHTMLorrehype-rawsink remains inapps/web.PUTupsertstemplateOverride,GETreturns it, the list flipsDefault→Custom, the preview renders it.templates-api.test.tsandtemplates-page.test.tsx. The loader, renderer and markdown pipeline execute for real; only session and Prisma are mocked.Open items from Jerel's review
Jerel caught three things on #238, two of which are load-bearing. They are recorded here against the code they concern rather than left in the other PR's thread. Not fixed in this PR.
sendEmailconsults an override only when handed adbLookup(shared/src/email/sender.ts:166,:171), and neitherteam/invite/route.ts:72norinvite/resend/route.ts:52passes one — so an invitee receives the on-disk copy while the editor shows aCustombadge and a success banner. Two ways to close it: passfindOverrideasdbLookupat both call sites, with a test that a stored override changes what reaches the transport; or hold the badge and the unqualified banner until it is wired.templates/is seven.mdfiles at the repo root,.dockerignore:7excludes*.md, andapps/web/Dockerfile's runner stage does not copytemplates/. If that reading is right,findTemplatesDir()resolves to nothing in production and each fail-closed gate returns 404; the 35 tests pass because they run fromapps/web, where the repo-roottemplates/is present. Worth confirming against a built image — happy to be corrected here.loader.ts:73-75buildsjoin(dir, \${slug}.md`)with no validation, and Next decodes percent-encoding in a dynamic segment. Admin-gated and bounded to.md, so the reachable surface is small; a/^[a-z0-9-]+$/` check at the route boundary closes it.outpost-websuite: 732 passing.