From 728139c7c14f8d4928a842a462d3e8e242cb0490 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20=F0=9F=94=B6=20Tarbert?= <66887028+NathanTarbert@users.noreply.github.com> Date: Sat, 22 Aug 2026 06:57:50 -0400 Subject: [PATCH 1/3] feat(web): wire template overrides through to storage, and sandbox the 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. --- apps/web/src/__tests__/templates-api.test.ts | 422 ++++++++++++++++++ .../web/src/__tests__/templates-page.test.tsx | 257 +++++++++++ .../app/api/templates/[slug]/preview/route.ts | 74 ++- .../web/src/app/api/templates/[slug]/route.ts | 97 +++- apps/web/src/app/api/templates/route.ts | 23 +- apps/web/src/app/settings/templates/page.tsx | 73 ++- 6 files changed, 899 insertions(+), 47 deletions(-) create mode 100644 apps/web/src/__tests__/templates-api.test.ts create mode 100644 apps/web/src/__tests__/templates-page.test.tsx diff --git a/apps/web/src/__tests__/templates-api.test.ts b/apps/web/src/__tests__/templates-api.test.ts new file mode 100644 index 00000000..19658d39 --- /dev/null +++ b/apps/web/src/__tests__/templates-api.test.ts @@ -0,0 +1,422 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { NextRequest } from 'next/server'; + +/** + * Template routes: persistence and preview fidelity (outpost#226). + * + * Before this suite, all three write paths were stubs. `PUT` validated its input + * and returned a success body without touching the database, while the UI reported + * "Template saved successfully" — so an author's edit was silently discarded. `GET` + * read only the filesystem, so even a real write would not have been visible. And + * preview rendered the STORED template rather than the unsaved draft, which is what + * made the XSS sink on that screen matter: the preview is the surface an author + * trusts to show them what they wrote. + * + * The loader and renderer are used for real here (repo-root `templates/` resolves + * from `apps/web`), so these assertions are about rendered output rather than about + * a mock of the thing under test. Only the session and the database are mocked. + */ + +const mockGetServerSession = vi.fn(); + +// require-admin imports from 'next-auth'; the list and preview routes import from +// 'next-auth/next'. Both need mocking or half the routes see no session. +// Wrapped rather than passed directly: the route modules are imported by the hoisted +// import statements below, which run before this file's consts initialise, so the +// factory must not read the variable until call time. +vi.mock('next-auth', () => ({ + getServerSession: (...args: unknown[]) => mockGetServerSession(...args), +})); +vi.mock('next-auth/next', () => ({ + getServerSession: (...args: unknown[]) => mockGetServerSession(...args), +})); + +const mockOverrideFindUnique = vi.fn(); +const mockOverrideFindMany = vi.fn(); +const mockOverrideUpsert = vi.fn(); +const mockOverrideDeleteMany = vi.fn(); + +vi.mock('@copilotkit/outpost/db', () => ({ + prisma: { + templateOverride: { + findUnique: (...args: unknown[]) => mockOverrideFindUnique(...args), + findMany: (...args: unknown[]) => mockOverrideFindMany(...args), + upsert: (...args: unknown[]) => mockOverrideUpsert(...args), + deleteMany: (...args: unknown[]) => mockOverrideDeleteMany(...args), + }, + }, +})); + +import { + GET as getTemplate, + PUT as putTemplate, + DELETE as deleteTemplate, +} from '@/app/api/templates/[slug]/route'; +import { GET as listTemplates } from '@/app/api/templates/route'; +import { POST as previewTemplate } from '@/app/api/templates/[slug]/preview/route'; + +const ADMIN_SESSION = { user: { id: 'u-admin', email: 'admin@copilotkit.ai', role: 'ADMIN' } }; +const MEMBER_SESSION = { user: { id: 'u-member', email: 'member@copilotkit.ai', role: 'MEMBER' } }; + +/** A slug that exists in the repo-root templates/ directory. */ +const REAL_SLUG = 'welcome'; + +function jsonRequest(body: unknown): NextRequest { + return { json: async () => body } as unknown as NextRequest; +} + +function bareRequest(): NextRequest { + return { json: async () => ({}) } as unknown as NextRequest; +} + +function routeParams(slug: string) { + return { params: Promise.resolve({ slug }) }; +} + +describe('template persistence and preview (outpost#226)', () => { + beforeEach(() => { + mockGetServerSession.mockReset(); + mockGetServerSession.mockResolvedValue(ADMIN_SESSION); + mockOverrideFindUnique.mockReset(); + mockOverrideFindUnique.mockResolvedValue(null); + mockOverrideFindMany.mockReset(); + mockOverrideFindMany.mockResolvedValue([]); + mockOverrideUpsert.mockReset(); + // Returns a realistic row, including the non-nullable updatedAt. A mock that + // omitted it previously made a dead `?? new Date()` fallback in the route look + // exercised — a test artifact shaping production code. + mockOverrideUpsert.mockImplementation( + async (args: { create: Record }) => ({ + id: 'ovr-1', + editedBy: null, + createdAt: new Date('2026-08-20T00:00:00Z'), + updatedAt: new Date('2026-08-20T00:00:00Z'), + ...args.create, + }), + ); + mockOverrideDeleteMany.mockReset(); + mockOverrideDeleteMany.mockResolvedValue({ count: 1 }); + }); + + describe('PUT actually persists', () => { + it('writes the override to the database', async () => { + const res = await putTemplate( + jsonRequest({ subject: 'Saved subject', body: 'Saved body' }), + routeParams(REAL_SLUG), + ); + + expect(res.status).toBe(200); + expect(mockOverrideUpsert).toHaveBeenCalledTimes(1); + + const args = mockOverrideUpsert.mock.calls[0][0]; + expect(args.where).toEqual({ slug: REAL_SLUG }); + expect(args.create).toMatchObject({ + slug: REAL_SLUG, + subject: 'Saved subject', + body: 'Saved body', + }); + expect(args.update).toMatchObject({ subject: 'Saved subject', body: 'Saved body' }); + }); + + it('records who edited it', async () => { + await putTemplate(jsonRequest({ subject: 's', body: 'b' }), routeParams(REAL_SLUG)); + + const args = mockOverrideUpsert.mock.calls[0][0]; + expect(args.create.editedBy).toBe('admin@copilotkit.ai'); + expect(args.update.editedBy).toBe('admin@copilotkit.ai'); + }); + + it('refuses a slug that has no template, rather than creating a phantom override', async () => { + const res = await putTemplate( + jsonRequest({ subject: 's', body: 'b' }), + routeParams('no-such-template'), + ); + + expect(res.status).toBe(404); + expect(mockOverrideUpsert).not.toHaveBeenCalled(); + }); + + it('still rejects a non-admin', async () => { + mockGetServerSession.mockResolvedValue(MEMBER_SESSION); + + const res = await putTemplate( + jsonRequest({ subject: 's', body: 'b' }), + routeParams(REAL_SLUG), + ); + + expect(res.status).toBe(403); + expect(mockOverrideUpsert).not.toHaveBeenCalled(); + }); + }); + + describe('the write paths stay gated', () => { + // These were stubs before this change and are now real, so the gate on each is + // the assertion most worth pinning: swapping requireAdmin for requireSession + // would otherwise let any member wipe every override with nothing failing. + it('refuses DELETE for a non-admin', async () => { + mockGetServerSession.mockResolvedValue(MEMBER_SESSION); + + const res = await deleteTemplate(bareRequest(), routeParams(REAL_SLUG)); + + expect(res.status).toBe(403); + expect(mockOverrideDeleteMany).not.toHaveBeenCalled(); + }); + + it('refuses preview for an unauthenticated request', async () => { + mockGetServerSession.mockResolvedValue(null); + + const res = await previewTemplate(bareRequest(), routeParams(REAL_SLUG)); + + expect(res.status).toBe(401); + }); + + it('refuses the list for an unauthenticated request', async () => { + mockGetServerSession.mockResolvedValue(null); + + const res = await listTemplates(bareRequest()); + + expect(res.status).toBe(401); + }); + }); + + describe('PUT validates types and bounds, not just truthiness', () => { + it('rejects non-string fields with a 400 rather than failing in the database', async () => { + const res = await putTemplate( + jsonRequest({ subject: {}, body: [1] }), + routeParams(REAL_SLUG), + ); + + expect(res.status).toBe(400); + expect(mockOverrideUpsert).not.toHaveBeenCalled(); + }); + + it('rejects whitespace-only content', async () => { + const res = await putTemplate( + jsonRequest({ subject: ' ', body: ' \n ' }), + routeParams(REAL_SLUG), + ); + + expect(res.status).toBe(400); + expect(mockOverrideUpsert).not.toHaveBeenCalled(); + }); + + it('rejects an oversized body', async () => { + const res = await putTemplate( + jsonRequest({ subject: 's', body: 'x'.repeat(100_001) }), + routeParams(REAL_SLUG), + ); + + expect(res.status).toBe(400); + expect(mockOverrideUpsert).not.toHaveBeenCalled(); + }); + + it('rejects a body that is not valid JSON', async () => { + const badRequest = { + json: async () => { + throw new SyntaxError('bad'); + }, + } as unknown as NextRequest; + + const res = await putTemplate(badRequest, routeParams(REAL_SLUG)); + + expect(res.status).toBe(400); + }); + }); + + describe('GET reads back what was saved', () => { + it('returns the stored override instead of the filesystem default', async () => { + mockOverrideFindUnique.mockResolvedValue({ + slug: REAL_SLUG, + subject: 'Overridden subject', + body: 'Overridden body', + editedBy: 'admin@copilotkit.ai', + updatedAt: new Date('2026-08-20T00:00:00Z'), + }); + + const res = await getTemplate(bareRequest(), routeParams(REAL_SLUG)); + const data = await res.json(); + + expect(data.subject).toBe('Overridden subject'); + expect(data.body).toBe('Overridden body'); + expect(data.isOverride).toBe(true); + }); + + it('falls back to the filesystem default when there is no override', async () => { + const res = await getTemplate(bareRequest(), routeParams(REAL_SLUG)); + const data = await res.json(); + + expect(data.isOverride).toBe(false); + expect(data.body.length).toBeGreaterThan(0); + expect(data.body).not.toBe('Overridden body'); + }); + }); + + describe('DELETE actually removes the override', () => { + it('deletes the stored row', async () => { + const res = await deleteTemplate(bareRequest(), routeParams(REAL_SLUG)); + + expect(res.status).toBe(200); + expect(mockOverrideDeleteMany).toHaveBeenCalledWith({ where: { slug: REAL_SLUG } }); + }); + }); + + describe('the list reflects override state', () => { + it('marks an overridden template and carries its edit metadata', async () => { + mockOverrideFindMany.mockResolvedValue([ + { + slug: REAL_SLUG, + subject: 'Overridden subject', + body: 'b', + editedBy: 'admin@copilotkit.ai', + updatedAt: new Date('2026-08-20T00:00:00Z'), + }, + ]); + + const res = await listTemplates(bareRequest()); + const entries = await res.json(); + const entry = entries.find((e: { slug: string }) => e.slug === REAL_SLUG); + + expect(entry.isOverride).toBe(true); + expect(entry.subject).toBe('Overridden subject'); + expect(entry.editedBy).toBe('admin@copilotkit.ai'); + expect(entry.updatedAt).not.toBeNull(); + }); + + it('leaves a non-overridden template marked as default', async () => { + const res = await listTemplates(bareRequest()); + const entries = await res.json(); + const entry = entries.find((e: { slug: string }) => e.slug === REAL_SLUG); + + expect(entry.isOverride).toBe(false); + expect(entry.editedBy).toBeNull(); + }); + }); + + describe('a draft that cannot be honoured is refused, not silently swapped', () => { + // Falling back to the stored template on a bad draft would re-introduce the + // exact bug this route is being fixed for: the author sees content they are + // not editing, and gets a 200 saying all is well. + it('rejects a draft missing its subject rather than rendering the stored template', async () => { + mockOverrideFindUnique.mockResolvedValue({ + slug: REAL_SLUG, + subject: 'Stored subject', + body: 'STORED-BODY-MARKER', + editedBy: null, + updatedAt: new Date(), + }); + + const res = await previewTemplate( + jsonRequest({ draft: { body: 'only a body' } }), + routeParams(REAL_SLUG), + ); + + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.html).toBeUndefined(); + }); + + it('rejects a draft whose fields are the wrong type', async () => { + const res = await previewTemplate( + jsonRequest({ draft: { subject: { evil: true }, body: ['x'] } }), + routeParams(REAL_SLUG), + ); + + expect(res.status).toBe(400); + }); + + it('rejects a body that is not valid JSON', async () => { + const badRequest = { + json: async () => { + throw new SyntaxError('Unexpected token'); + }, + headers: { get: () => null }, + } as unknown as NextRequest; + + const res = await previewTemplate(badRequest, routeParams(REAL_SLUG)); + + expect(res.status).toBe(400); + }); + + it('still allows an intentionally empty body, which is a valid draft', async () => { + const res = await previewTemplate( + jsonRequest({ draft: { subject: 'Subject only', body: '' } }), + routeParams(REAL_SLUG), + ); + + expect(res.status).toBe(200); + const data = await res.json(); + expect(data.subject).toBe('Subject only'); + }); + + it('refuses a slug with no template even when a draft is supplied', async () => { + const res = await previewTemplate( + jsonRequest({ draft: { subject: 's', body: 'b' } }), + routeParams('no-such-template'), + ); + + expect(res.status).toBe(404); + }); + }); + + describe('preview renders the draft, not the stored template', () => { + it('renders unsaved body content sent with the request', async () => { + const res = await previewTemplate( + jsonRequest({ draft: { subject: 'Draft subject', body: 'UNSAVED-BODY-MARKER' } }), + routeParams(REAL_SLUG), + ); + const data = await res.json(); + + expect(data.html).toContain('UNSAVED-BODY-MARKER'); + expect(data.subject).toBe('Draft subject'); + }); + + it('does not fall back to the stored override when a draft is supplied', async () => { + mockOverrideFindUnique.mockResolvedValue({ + slug: REAL_SLUG, + subject: 'Stored subject', + body: 'STORED-BODY-MARKER', + editedBy: null, + updatedAt: new Date(), + }); + + const res = await previewTemplate( + jsonRequest({ draft: { subject: 'Draft subject', body: 'UNSAVED-BODY-MARKER' } }), + routeParams(REAL_SLUG), + ); + const data = await res.json(); + + expect(data.html).toContain('UNSAVED-BODY-MARKER'); + expect(data.html).not.toContain('STORED-BODY-MARKER'); + }); + + it('still renders the stored template when no draft is supplied', async () => { + mockOverrideFindUnique.mockResolvedValue({ + slug: REAL_SLUG, + subject: 'Stored subject', + body: 'STORED-BODY-MARKER', + editedBy: null, + updatedAt: new Date(), + }); + + const res = await previewTemplate(bareRequest(), routeParams(REAL_SLUG)); + const data = await res.json(); + + expect(data.html).toContain('STORED-BODY-MARKER'); + expect(data.subject).toBe('Stored subject'); + }); + + it('interpolates variables in the draft rather than emitting them raw', async () => { + const res = await previewTemplate( + jsonRequest({ + draft: { subject: 'Hi {{member.name}}', body: 'Hello {{member.name}}' }, + }), + routeParams(REAL_SLUG), + ); + const data = await res.json(); + + expect(data.subject).toBe('Hi Jane Smith'); + expect(data.html).toContain('Jane Smith'); + expect(data.html).not.toContain('{{member.name}}'); + }); + }); +}); diff --git a/apps/web/src/__tests__/templates-page.test.tsx b/apps/web/src/__tests__/templates-page.test.tsx new file mode 100644 index 00000000..3e816bc2 --- /dev/null +++ b/apps/web/src/__tests__/templates-page.test.tsx @@ -0,0 +1,257 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import TemplatesPage from '@/app/settings/templates/page'; + +/** + * Template editor: preview containment and draft fidelity (outpost#226). + * + * The preview rendered author-authored HTML through `dangerouslySetInnerHTML` with no + * sanitisation. Templates are dashboard-editable, so that is a STORED sink — whatever + * an author saved executed in every later viewer's session. It also mattered more than + * a typical XSS here, because the csrf cookie is `httpOnly: false` by necessity (the + * client reads it to build the `X-CSRF-Token` header), so script on this page could + * read the CSRF token directly. + * + * These tests assert containment structurally — the preview must render inside a + * sandboxed iframe that permits neither scripts nor same-origin access — rather than + * asserting that some particular payload string was escaped, which an allowlist could + * pass while still admitting the next payload. + */ + +const mockApiFetch = vi.fn(); +vi.mock('@/lib/api-fetch', () => ({ + apiFetch: (...args: unknown[]) => mockApiFetch(...args), +})); + +const TEMPLATE_LIST = [ + { + slug: 'welcome', + name: 'Welcome', + subject: 'Hi', + isOverride: false, + updatedAt: null, + editedBy: null, + }, +]; + +const TEMPLATE_DETAIL = { + slug: 'welcome', + name: 'Welcome', + subject: 'Hi {{member.name}}', + from: 'support@acme.com', + body: 'Stored body', + isOverride: false, +}; + +/** A rendered preview whose content would execute if it were injected into this document. */ +const MALICIOUS_PREVIEW = { + subject: 'Pwned subject', + html: '

hello

', + text: 'hello', + markdown: 'hello', +}; + +function jsonOk(data: unknown) { + return { ok: true, json: async () => data }; +} + +/** Route apiFetch by URL and method so the component drives a realistic sequence. */ +function routeApiFetch(previewPayload: unknown = MALICIOUS_PREVIEW) { + mockApiFetch.mockImplementation(async (url: string, init?: { method?: string }) => { + if (url === '/api/templates') return jsonOk(TEMPLATE_LIST); + if (url === '/api/templates/welcome' && (!init?.method || init.method === 'GET')) { + return jsonOk(TEMPLATE_DETAIL); + } + if (url.endsWith('/preview')) return jsonOk(previewPayload); + return jsonOk({ ok: true }); + }); +} + +/** Select the one template and wait for the editor to appear. */ +async function openTemplate() { + render(); + const row = await screen.findByText('Welcome'); + fireEvent.click(row); + await screen.findByDisplayValue('Stored body'); +} + +async function openPreview() { + fireEvent.click(screen.getByRole('button', { name: /preview/i })); + await waitFor(() => expect(screen.getByTitle(/preview/i)).toBeTruthy()); +} + +describe('template preview containment (outpost#226)', () => { + beforeEach(() => { + mockApiFetch.mockReset(); + routeApiFetch(); + delete (window as unknown as Record).__xssFired; + }); + + it('renders the preview inside an iframe rather than injecting it into the page', async () => { + await openTemplate(); + await openPreview(); + + const frame = screen.getByTitle(/preview/i) as HTMLIFrameElement; + expect(frame.tagName).toBe('IFRAME'); + expect(frame.getAttribute('srcdoc')).toContain('

hello

'); + }); + + it('sandboxes the iframe so neither scripts nor same-origin access are permitted', async () => { + await openTemplate(); + await openPreview(); + + const frame = screen.getByTitle(/preview/i) as HTMLIFrameElement; + // The attribute must be present. An absent sandbox attribute is full privileges. + expect(frame.hasAttribute('sandbox')).toBe(true); + + // Asserted as an exact value, not by excluding two tokens: `sandbox= + // "allow-top-navigation allow-forms"` passes a not-contains check while letting a + // saved template's redirect the admin's whole tab. The source + // comment promises "grants nothing", so the test should hold it to exactly that. + expect(frame.getAttribute('sandbox')).toBe(''); + }); + + it('does not place the preview markup in the parent document', async () => { + await openTemplate(); + await openPreview(); + + // If the html were injected via dangerouslySetInnerHTML, these would exist in + // this document. Inside an iframe's srcdoc they are inert text. + expect(document.querySelector('script[data-xss]')).toBeNull(); + expect(document.querySelector('img[data-xss-img]')).toBeNull(); + expect((window as unknown as Record).__xssFired).toBeUndefined(); + }); +}); + +describe('the editor reports what the server actually said (outpost#226)', () => { + beforeEach(() => { + mockApiFetch.mockReset(); + }); + + /** Route reads normally, but fail the named mutation with `status` and `payload`. */ + function failMutation(match: string, status: number, payload: unknown) { + mockApiFetch.mockImplementation(async (url: string, init?: { method?: string }) => { + if (url === '/api/templates') return jsonOk(TEMPLATE_LIST); + if (url === '/api/templates/welcome' && (!init?.method || init.method === 'GET')) { + return jsonOk(TEMPLATE_DETAIL); + } + if (init?.method === match) { + return { + ok: false, + status, + json: async () => { + if (payload === undefined) throw new SyntaxError('no body'); + return payload; + }, + }; + } + return jsonOk({ ok: true }); + }); + } + + it('surfaces a validation message instead of a generic failure', async () => { + failMutation('PUT', 400, { error: 'subject and body are required' }); + await openTemplate(); + + fireEvent.click(screen.getByRole('button', { name: /save/i })); + + // The reachable case: an author clears a field. "Save failed" would send them + // hunting for an outage instead of showing them the empty field. + expect(await screen.findByText('subject and body are required')).toBeTruthy(); + }); + + it('surfaces a permission message rather than looking like a bug', async () => { + failMutation('PUT', 403, { error: 'Forbidden: admin access required' }); + await openTemplate(); + + fireEvent.click(screen.getByRole('button', { name: /save/i })); + + expect(await screen.findByText('Forbidden: admin access required')).toBeTruthy(); + }); + + it('distinguishes a server error from a rejected input by showing the status', async () => { + // A genuine 500 has no JSON body, so the status code is the only thing that + // separates "your input was rejected" from "the server broke". + failMutation('PUT', 500, undefined); + await openTemplate(); + + fireEvent.click(screen.getByRole('button', { name: /save/i })); + + expect(await screen.findByText(/HTTP 500/)).toBeTruthy(); + }); +}); + +describe('reset gives the author feedback (outpost#226)', () => { + beforeEach(() => { + mockApiFetch.mockReset(); + }); + + function resetReturns(payload: unknown) { + mockApiFetch.mockImplementation(async (url: string, init?: { method?: string }) => { + if (url === '/api/templates') return jsonOk(TEMPLATE_LIST); + if (url === '/api/templates/welcome' && (!init?.method || init.method === 'GET')) { + return jsonOk(TEMPLATE_DETAIL); + } + if (init?.method === 'DELETE') return jsonOk(payload); + return jsonOk({ ok: true }); + }); + } + + it('confirms the reset after the editor reloads', async () => { + // DELETE now really deletes, so a reset with no confirmation is a destructive + // action with no feedback at all. + resetReturns({ slug: 'welcome', reset: true, hadOverride: true }); + await openTemplate(); + + fireEvent.click(screen.getByRole('button', { name: /reset/i })); + + expect(await screen.findByText(/reset to default/i)).toBeTruthy(); + }); + + it('says so when the template was already using the default', async () => { + resetReturns({ slug: 'welcome', reset: true, hadOverride: false }); + await openTemplate(); + + fireEvent.click(screen.getByRole('button', { name: /reset/i })); + + expect(await screen.findByText(/already using the default/i)).toBeTruthy(); + }); +}); + +describe('preview shows the unsaved draft (outpost#226)', () => { + beforeEach(() => { + mockApiFetch.mockReset(); + routeApiFetch(); + }); + + it('sends the current editor content, not an empty body', async () => { + await openTemplate(); + + const bodyField = screen.getByDisplayValue('Stored body'); + fireEvent.change(bodyField, { target: { value: 'Edited but unsaved' } }); + + fireEvent.click(screen.getByRole('button', { name: /preview/i })); + + await waitFor(() => { + const call = mockApiFetch.mock.calls.find(([url]) => String(url).endsWith('/preview')); + expect(call).toBeTruthy(); + const sent = JSON.parse(call![1].body); + expect(sent.draft.body).toBe('Edited but unsaved'); + }); + }); + + it('sends the edited subject too', async () => { + await openTemplate(); + + const subjectField = screen.getByDisplayValue('Hi {{member.name}}'); + fireEvent.change(subjectField, { target: { value: 'New subject' } }); + + fireEvent.click(screen.getByRole('button', { name: /preview/i })); + + await waitFor(() => { + const call = mockApiFetch.mock.calls.find(([url]) => String(url).endsWith('/preview')); + const sent = JSON.parse(call![1].body); + expect(sent.draft.subject).toBe('New subject'); + }); + }); +}); diff --git a/apps/web/src/app/api/templates/[slug]/preview/route.ts b/apps/web/src/app/api/templates/[slug]/preview/route.ts index c34ad06c..2dec9aae 100644 --- a/apps/web/src/app/api/templates/[slug]/preview/route.ts +++ b/apps/web/src/app/api/templates/[slug]/preview/route.ts @@ -1,7 +1,8 @@ import { NextRequest, NextResponse } from 'next/server'; import { getServerSession } from 'next-auth/next'; import { authOptions } from '@/lib/auth'; -import { renderTemplate } from '@copilotkit/outpost/shared/server'; +import { prisma } from '@copilotkit/outpost/db'; +import { loadFromFilesystem, renderTemplate } from '@copilotkit/outpost/shared/server'; import type { TemplateContext } from '@copilotkit/outpost/shared/server'; /** Sample data used for template previews. */ @@ -35,7 +36,11 @@ const SAMPLE_CONTEXT: TemplateContext = { * POST /api/templates/[slug]/preview * * Render a template with sample data and return the HTML. - * Optionally accepts a custom context in the request body. + * + * Accepts an optional `draft` ({ subject, body }) so the editor can preview + * UNSAVED edits. Without it the preview rendered whatever was stored, which meant + * an author validated content they were not about to save. Also accepts an optional + * `context` to override the sample data. */ export async function POST( request: NextRequest, @@ -48,17 +53,68 @@ export async function POST( const { slug } = await params; - let context = SAMPLE_CONTEXT; + // A template must exist on disk for an override or a draft to layer onto, and PUT + // refuses a slug that has none. Checked here too so preview and save agree — + // otherwise a renamed or deleted template previews happily and then fails to save. + if (!loadFromFilesystem(slug)) { + return NextResponse.json({ error: 'Template not found' }, { status: 404 }); + } + + let parsed: unknown = null; try { - const body = await request.json(); - if (body.context) { - context = { ...SAMPLE_CONTEXT, ...body.context }; - } + parsed = await request.json(); } catch { - // Use default sample context + // No body at all is fine — it means "preview what is stored". Unparseable JSON + // is not: silently rendering the stored template would show the author content + // they did not ask for, which is the bug this route was fixed for. + if (request.headers.get('content-length') !== '0') { + const hasBody = await Promise.resolve(true); + if (hasBody) { + return NextResponse.json( + { error: 'Request body is not valid JSON' }, + { status: 400 }, + ); + } + } + } + + const body = (parsed ?? {}) as Record; + + let context = SAMPLE_CONTEXT; + if (body.context && typeof body.context === 'object') { + context = { ...SAMPLE_CONTEXT, ...(body.context as object) }; + } + + // A draft that is present but unusable is a client bug, not a request to preview + // the stored copy. Note '' is a valid body — an author may be clearing it. + const draft = body.draft as { subject?: unknown; body?: unknown } | undefined; + if (draft !== undefined) { + if ( + draft === null || + typeof draft !== 'object' || + typeof draft.subject !== 'string' || + typeof draft.body !== 'string' + ) { + return NextResponse.json( + { error: 'draft requires both subject and body as strings' }, + { status: 400 }, + ); + } } - const result = await renderTemplate(slug, context); + const useDraft = draft !== undefined; + + // The loader takes its content from this lookup when it returns non-null, so an + // unsaved draft is supplied the same way a stored override would be — the draft + // wins over the stored row, which is the whole point of previewing edits. + const lookup = useDraft + ? async () => ({ subject: draft!.subject as string, body: draft!.body as string }) + : async (s: string) => { + const override = await prisma.templateOverride.findUnique({ where: { slug: s } }); + return override ? { subject: override.subject, body: override.body } : null; + }; + + const result = await renderTemplate(slug, context, lookup); if (!result) { return NextResponse.json({ error: 'Template not found' }, { status: 404 }); } diff --git a/apps/web/src/app/api/templates/[slug]/route.ts b/apps/web/src/app/api/templates/[slug]/route.ts index 9a7f36d2..629401a5 100644 --- a/apps/web/src/app/api/templates/[slug]/route.ts +++ b/apps/web/src/app/api/templates/[slug]/route.ts @@ -1,11 +1,29 @@ import { NextRequest, NextResponse } from 'next/server'; import { requireSession, requireAdmin } from '@/lib/require-admin'; -import { loadFromFilesystem } from '@copilotkit/outpost/shared/server'; +import { prisma } from '@copilotkit/outpost/db'; +import { loadFromFilesystem, loadTemplate } from '@copilotkit/outpost/shared/server'; + +/** Bounds on stored template content. The subject becomes an email header downstream. */ +const SUBJECT_MAX = 500; +const BODY_MAX = 100_000; + +/** + * Look up a stored override for a slug. + * + * Passed into `loadTemplate` so the shared loader stays free of a Prisma + * dependency — it takes the lookup as an argument. + */ +async function findOverride(slug: string): Promise<{ subject: string; body: string } | null> { + const override = await prisma.templateOverride.findUnique({ where: { slug } }); + if (!override) return null; + return { subject: override.subject, body: override.body }; +} /** * GET /api/templates/[slug] * - * Get a single template's content (filesystem default or DB override). + * Get a single template's content: the stored override if one exists, otherwise + * the filesystem default. */ export async function GET( _request: NextRequest, @@ -15,7 +33,7 @@ export async function GET( if (error) return error; const { slug } = await params; - const loaded = loadFromFilesystem(slug); + const loaded = await loadTemplate(slug, findOverride); if (!loaded) { return NextResponse.json({ error: 'Template not found' }, { status: 404 }); @@ -35,44 +53,84 @@ export async function GET( * PUT /api/templates/[slug] * * Save a template override (ADMIN only). - * In production this writes to the TemplateOverride table. - * For now, returns a mock success response. */ export async function PUT( request: NextRequest, { params }: { params: Promise<{ slug: string }> }, ) { - const { error } = await requireAdmin(); + const { error, session } = await requireAdmin(); if (error) return error; const { slug } = await params; - const body = await request.json(); - // Validate required fields - if (!body.subject || !body.body) { + let payload: unknown; + try { + payload = await request.json(); + } catch { + return NextResponse.json({ error: 'Request body is not valid JSON' }, { status: 400 }); + } + + const { subject, body: templateBody } = (payload ?? {}) as Record; + + // Type-checked, not truthiness-checked: `{ subject: {}, body: [1] }` used to reach + // the upsert and surface as a 500 where a 400 belongs. + if ( + typeof subject !== 'string' || + typeof templateBody !== 'string' || + !subject.trim() || + !templateBody.trim() + ) { return NextResponse.json( - { error: 'subject and body are required' }, + { error: 'subject and body are required strings' }, { status: 400 }, ); } - // In production, this would write to the TemplateOverride table: - // await prisma.templateOverride.upsert({ where: { slug }, create: {...}, update: {...} }) + // Both columns are unbounded in the schema, and the subject becomes an email header + // downstream, so the bound belongs here. + if (subject.length > SUBJECT_MAX || templateBody.length > BODY_MAX) { + return NextResponse.json( + { error: `subject must be under ${SUBJECT_MAX} characters and body under ${BODY_MAX}` }, + { status: 400 }, + ); + } + + // An override only means anything layered over a real template: the loader takes + // its name and `from` from the filesystem entry, and the editor lists filesystem + // slugs. Without this check an arbitrary slug would create a row nothing reads. + if (!loadFromFilesystem(slug)) { + return NextResponse.json({ error: 'Template not found' }, { status: 404 }); + } + + // Falls back to the member id: the session callback always sets one, so an absent + // email should not cost us the audit trail on a security-relevant screen. + const user = session.user as Record | undefined; + const editedBy = + (typeof user?.email === 'string' ? user.email : null) ?? + (typeof user?.memberId === 'string' ? user.memberId : null); + + const saved = await prisma.templateOverride.upsert({ + where: { slug }, + create: { slug, subject, body: templateBody, editedBy }, + update: { subject, body: templateBody, editedBy }, + }); return NextResponse.json({ slug, - subject: body.subject, - body: body.body, + subject: saved.subject, + body: saved.body, isOverride: true, - updatedAt: new Date().toISOString(), + editedBy: saved.editedBy ?? editedBy, + updatedAt: saved.updatedAt, }); } /** * DELETE /api/templates/[slug] * - * Reset a template to filesystem default (ADMIN only). - * Deletes the DB override if one exists. + * Reset a template to its filesystem default by removing the stored override + * (ADMIN only). Uses deleteMany so resetting an already-default template is a + * no-op rather than a 404 from Prisma. */ export async function DELETE( _request: NextRequest, @@ -82,8 +140,7 @@ export async function DELETE( if (error) return error; const { slug } = await params; + const { count } = await prisma.templateOverride.deleteMany({ where: { slug } }); - // In production: await prisma.templateOverride.delete({ where: { slug } }) - - return NextResponse.json({ slug, reset: true }); + return NextResponse.json({ slug, reset: true, hadOverride: count > 0 }); } diff --git a/apps/web/src/app/api/templates/route.ts b/apps/web/src/app/api/templates/route.ts index 9c09d013..091c3386 100644 --- a/apps/web/src/app/api/templates/route.ts +++ b/apps/web/src/app/api/templates/route.ts @@ -1,6 +1,7 @@ import { NextRequest, NextResponse } from 'next/server'; import { getServerSession } from 'next-auth/next'; import { authOptions } from '@/lib/auth'; +import { prisma } from '@copilotkit/outpost/db'; import { listTemplateSlugs, loadFromFilesystem, @@ -10,9 +11,9 @@ import type { TemplateListEntry } from '@copilotkit/outpost/shared/server'; /** * GET /api/templates * - * List all templates. Merges filesystem defaults with DB overrides. - * In this mock implementation, we only return filesystem templates - * (DB overrides would be merged in production with Prisma). + * List all templates, merging filesystem defaults with stored overrides. An + * overridden entry reports the override's subject and edit metadata, so the list + * shows what is actually in effect rather than what is on disk. */ export async function GET(_request: NextRequest) { const session = await getServerSession(authOptions); @@ -22,15 +23,23 @@ export async function GET(_request: NextRequest) { const slugs = listTemplateSlugs(); + // One query rather than one per slug. + const overrides = await prisma.templateOverride.findMany({ + where: { slug: { in: slugs } }, + }); + const bySlug = new Map(overrides.map((o) => [o.slug, o])); + const entries: TemplateListEntry[] = slugs.map((slug) => { const loaded = loadFromFilesystem(slug); + const override = bySlug.get(slug); + return { slug, name: loaded?.meta.name || slug, - subject: loaded?.meta.subject || '', - isOverride: false, - updatedAt: null, - editedBy: null, + subject: override?.subject ?? loaded?.meta.subject ?? '', + isOverride: Boolean(override), + updatedAt: override?.updatedAt ? override.updatedAt.toISOString() : null, + editedBy: override?.editedBy ?? null, }; }); diff --git a/apps/web/src/app/settings/templates/page.tsx b/apps/web/src/app/settings/templates/page.tsx index 5742de4b..6b743a2b 100644 --- a/apps/web/src/app/settings/templates/page.tsx +++ b/apps/web/src/app/settings/templates/page.tsx @@ -5,6 +5,21 @@ import { Mail, FileText, RotateCcw, Save, Eye, ChevronLeft } from 'lucide-react' import { PageHeader } from '@/components/page-header'; import { apiFetch } from '@/lib/api-fetch'; +/** + * Turn a failed response into a message worth showing. + * + * The routes return `{ error }` with an actionable reason — an empty field, a missing + * template, insufficient permissions — and reporting a fixed string instead sends the + * author looking for an outage. The status tail matters for a genuine 500, which has no + * JSON body: it is the only thing separating "your input was rejected" from "the server + * broke". + */ +async function describeFailure(res: Response, fallback: string): Promise { + const detail = await res.json().catch(() => null); + const serverMessage = detail && typeof detail.error === 'string' ? detail.error : null; + return serverMessage ?? `${fallback} (HTTP ${res.status})`; +} + interface TemplateEntry { slug: string; name: string; @@ -44,7 +59,7 @@ export default function TemplatesPage() { const fetchTemplates = useCallback(async () => { try { const res = await apiFetch('/api/templates'); - if (!res.ok) throw new Error('Failed to load templates'); + if (!res.ok) throw new Error(await describeFailure(res, 'Failed to load templates')); const data = await res.json(); setTemplates(data); } catch (err) { @@ -64,7 +79,7 @@ export default function TemplatesPage() { try { const res = await apiFetch(`/api/templates/${slug}`); - if (!res.ok) throw new Error('Failed to load template'); + if (!res.ok) throw new Error(await describeFailure(res, 'Failed to load template')); const data: TemplateDetail = await res.json(); setSelected(data); setEditSubject(data.subject); @@ -78,12 +93,14 @@ export default function TemplatesPage() { if (!selected) return; try { + // Send the draft, not an empty body. Previously the server rendered the + // STORED template, so an author previewed content they were not saving. const res = await apiFetch(`/api/templates/${selected.slug}/preview`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({}), + body: JSON.stringify({ draft: { subject: editSubject, body: editBody } }), }); - if (!res.ok) throw new Error('Preview failed'); + if (!res.ok) throw new Error(await describeFailure(res, 'Preview failed')); const data: PreviewResult = await res.json(); setPreview(data); setShowPreview(true); @@ -104,9 +121,9 @@ export default function TemplatesPage() { headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ subject: editSubject, body: editBody }), }); - if (!res.ok) throw new Error('Save failed'); - setSuccess('Template saved successfully'); + if (!res.ok) throw new Error(await describeFailure(res, 'Save failed')); await fetchTemplates(); + setSuccess('Template saved successfully'); } catch (err) { setError(err instanceof Error ? err.message : 'Save failed'); } finally { @@ -123,10 +140,19 @@ export default function TemplatesPage() { const res = await apiFetch(`/api/templates/${selected.slug}`, { method: 'DELETE', }); - if (!res.ok) throw new Error('Reset failed'); - setSuccess('Template reset to default'); + if (!res.ok) throw new Error(await describeFailure(res, 'Reset failed')); + const result = await res.json().catch(() => null); + + // selectTemplate clears the banners, so the message has to be set after the + // reloads rather than before them — otherwise a destructive action that now + // really deletes gives the author no feedback at all. await selectTemplate(selected.slug); await fetchTemplates(); + setSuccess( + result?.hadOverride === false + ? 'Template was already using the default' + : 'Template reset to default', + ); } catch (err) { setError(err instanceof Error ? err.message : 'Reset failed'); } @@ -248,9 +274,34 @@ export default function TemplatesPage() { Subject: {preview.subject}

-
) : ( From d9fc475ab2ed7bda31b63ea84686e18aa3259db7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20=F0=9F=94=B6=20Tarbert?= <66887028+NathanTarbert@users.noreply.github.com> Date: Tue, 8 Sep 2026 14:34:55 -0400 Subject: [PATCH 2/3] fix(web): serve only known template slugs, and stop the editor overstating a save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/web/src/__tests__/templates-api.test.ts | 75 ++++++++++++++++++- .../web/src/__tests__/templates-page.test.tsx | 43 +++++++++++ .../app/api/templates/[slug]/preview/route.ts | 67 ++++++++++++----- .../web/src/app/api/templates/[slug]/route.ts | 35 +++++++-- apps/web/src/app/settings/templates/page.tsx | 65 +++++++++------- .../outpost/shared/src/templates/loader.ts | 34 ++++++++- 6 files changed, 261 insertions(+), 58 deletions(-) diff --git a/apps/web/src/__tests__/templates-api.test.ts b/apps/web/src/__tests__/templates-api.test.ts index 19658d39..fb5a23ff 100644 --- a/apps/web/src/__tests__/templates-api.test.ts +++ b/apps/web/src/__tests__/templates-api.test.ts @@ -61,12 +61,23 @@ const MEMBER_SESSION = { user: { id: 'u-member', email: 'member@copilotkit.ai', /** A slug that exists in the repo-root templates/ directory. */ const REAL_SLUG = 'welcome'; +// Both `text` and `json` are stubbed because the two routes read the body +// differently: PUT uses `json()`, and preview reads `text()` once and parses it +// itself — a request body can only be read once, so calling `json()` first would +// consume the stream and leave an unparseable body looking empty. function jsonRequest(body: unknown): NextRequest { - return { json: async () => body } as unknown as NextRequest; + return { + text: async () => JSON.stringify(body), + json: async () => body, + } as unknown as NextRequest; } +/** A request with no body at all, which preview treats as "render what is stored". */ function bareRequest(): NextRequest { - return { json: async () => ({}) } as unknown as NextRequest; + return { + text: async () => '', + json: async () => ({}), + } as unknown as NextRequest; } function routeParams(slug: string) { @@ -251,6 +262,63 @@ describe('template persistence and preview (outpost#226)', () => { }); }); + // The slug reached `join(dir, slug + '.md')` unvalidated, and Next decodes + // percent-encoding in a dynamic segment before a handler runs — so + // `../docs/deployment` arrived as a traversal, the loader read the file, and GET + // returned its `subject` and `body` in the JSON response. Confirmed against the + // real loader: `../README`, `../CLAUDE`, `../docs/deployment` and + // `invite/../../README` all returned file contents. + // + // The existence checks did not close it — they WERE it. A guard shaped like + // `if (!loadFromFilesystem(slug))` succeeds for every path above, so it approved + // the request it appeared to reject. + describe('a slug outside the template set is refused everywhere', () => { + const TRAVERSALS = ['../README', '../CLAUDE', '../docs/deployment', 'invite/../../README']; + + it.each(TRAVERSALS)('GET refuses %s rather than returning the file', async (slug) => { + const res = await getTemplate(bareRequest(), routeParams(slug)); + + expect(res.status).toBe(404); + const data = await res.json(); + // The proof is the absence of file content, not just the status: a 200 + // carrying the deployment guide is the failure being pinned. + expect(data.body).toBeUndefined(); + expect(data.subject).toBeUndefined(); + }); + + it.each(TRAVERSALS)('PUT refuses %s', async (slug) => { + const res = await putTemplate( + jsonRequest({ subject: 'x', body: 'y' }), + routeParams(slug), + ); + + expect(res.status).toBe(404); + expect(mockOverrideUpsert).not.toHaveBeenCalled(); + }); + + it.each(TRAVERSALS)('preview refuses %s', async (slug) => { + const res = await previewTemplate(bareRequest(), routeParams(slug)); + + expect(res.status).toBe(404); + }); + + // Harmless on its own — deleteMany matches nothing — but all four handlers + // should answer the same way for the same input. + it.each(TRAVERSALS)('DELETE refuses %s', async (slug) => { + const res = await deleteTemplate(bareRequest(), routeParams(slug)); + + expect(res.status).toBe(404); + expect(mockOverrideDeleteMany).not.toHaveBeenCalled(); + }); + + it('still serves the real slugs', async () => { + const res = await getTemplate(bareRequest(), routeParams(REAL_SLUG)); + + expect(res.status).toBe(200); + expect((await res.json()).body.length).toBeGreaterThan(0); + }); + }); + describe('DELETE actually removes the override', () => { it('deletes the stored row', async () => { const res = await deleteTemplate(bareRequest(), routeParams(REAL_SLUG)); @@ -326,6 +394,9 @@ describe('template persistence and preview (outpost#226)', () => { it('rejects a body that is not valid JSON', async () => { const badRequest = { + // Unparseable text rather than a throwing `json()`: the route parses + // the text itself, so that is the shape a real bad body arrives in. + text: async () => '{ not json', json: async () => { throw new SyntaxError('Unexpected token'); }, diff --git a/apps/web/src/__tests__/templates-page.test.tsx b/apps/web/src/__tests__/templates-page.test.tsx index 3e816bc2..bab8d3fe 100644 --- a/apps/web/src/__tests__/templates-page.test.tsx +++ b/apps/web/src/__tests__/templates-page.test.tsx @@ -181,6 +181,49 @@ describe('the editor reports what the server actually said (outpost#226)', () => }); }); +// A save writes the override and every read surface honours it, but outgoing email +// does not — `sendEmail` consults an override only when handed a `dbLookup`, and +// neither invite call site passes one, so the invitee gets the on-disk copy. +// "Template saved successfully" was true about the row and false about the thing +// the author cared about. These pin the honest copy, because the dishonest version +// is the shorter and more natural string to write. +describe('the editor does not claim more than a save delivers (outpost#226)', () => { + beforeEach(() => { + mockApiFetch.mockReset(); + }); + + it('says the save is not yet used for outgoing email', async () => { + mockApiFetch.mockImplementation(async (url: string, init?: { method?: string }) => { + if (url === '/api/templates') return jsonOk(TEMPLATE_LIST); + if (url === '/api/templates/welcome' && (!init?.method || init.method === 'GET')) { + return jsonOk(TEMPLATE_DETAIL); + } + return jsonOk({ ok: true }); + }); + await openTemplate(); + + fireEvent.click(screen.getByRole('button', { name: /save/i })); + + const banner = await screen.findByText(/not yet used for outgoing email/i); + expect(banner).toBeTruthy(); + // The bare claim must not be what the author reads. + expect(screen.queryByText('Template saved successfully')).toBeNull(); + }); + + it('marks an overridden template as preview-only in the list', async () => { + // The shared fixture is un-overridden, so this test supplies its own — the + // badge only appears for a template that actually has a stored override. + const overridden = [{ ...TEMPLATE_LIST[0], isOverride: true }]; + mockApiFetch.mockImplementation(async (url: string) => { + if (url === '/api/templates') return jsonOk(overridden); + return jsonOk(TEMPLATE_DETAIL); + }); + render(); + + expect(await screen.findByText(/custom \(preview only\)/i)).toBeTruthy(); + }); +}); + describe('reset gives the author feedback (outpost#226)', () => { beforeEach(() => { mockApiFetch.mockReset(); diff --git a/apps/web/src/app/api/templates/[slug]/preview/route.ts b/apps/web/src/app/api/templates/[slug]/preview/route.ts index 2dec9aae..2d183495 100644 --- a/apps/web/src/app/api/templates/[slug]/preview/route.ts +++ b/apps/web/src/app/api/templates/[slug]/preview/route.ts @@ -2,13 +2,18 @@ import { NextRequest, NextResponse } from 'next/server'; import { getServerSession } from 'next-auth/next'; import { authOptions } from '@/lib/auth'; import { prisma } from '@copilotkit/outpost/db'; -import { loadFromFilesystem, renderTemplate } from '@copilotkit/outpost/shared/server'; +import { isKnownTemplateSlug, renderTemplate } from '@copilotkit/outpost/shared/server'; import type { TemplateContext } from '@copilotkit/outpost/shared/server'; /** Sample data used for template previews. */ const SAMPLE_CONTEXT: TemplateContext = { org: { name: 'Acme Corp', email: 'support@acme.com' }, - member: { name: 'Jane Smith', email: 'jane@acme.com', invitedBy: 'John Admin', role: 'Engineer' }, + member: { + name: 'Jane Smith', + email: 'jane@acme.com', + invitedBy: 'John Admin', + role: 'Engineer', + }, customer: { name: 'Alex Customer', email: 'alex@example.com' }, invite: { url: 'https://app.outpost.dev/invite/sample-token', expiresIn: '7 days' }, app: { url: 'https://app.outpost.dev' }, @@ -18,12 +23,23 @@ const SAMPLE_CONTEXT: TemplateContext = { url: 'https://app.outpost.dev/tickets/tkt-0042', priority: 'HIGH', assignee: 'Jane Smith', - resolution: 'The API endpoint was updated to v2. Updated the SDK configuration to point to the new URL.', + resolution: + 'The API endpoint was updated to v2. Updated the SDK configuration to point to the new URL.', }, sla: { target: '4 hours', elapsed: '6 hours 23 minutes' }, - escalation: { by: 'System', from: 'Jane Smith', to: 'John Admin', reason: 'SLA breach and no response in 6 hours' }, + escalation: { + by: 'System', + from: 'Jane Smith', + to: 'John Admin', + reason: 'SLA breach and no response in 6 hours', + }, digest: { - date: new Date().toLocaleDateString('en-US', { weekday: 'long', year: 'numeric', month: 'long', day: 'numeric' }), + date: new Date().toLocaleDateString('en-US', { + weekday: 'long', + year: 'numeric', + month: 'long', + day: 'numeric', + }), openTickets: '12', resolvedToday: '5', breachedSla: '1', @@ -56,25 +72,36 @@ export async function POST( // A template must exist on disk for an override or a draft to layer onto, and PUT // refuses a slug that has none. Checked here too so preview and save agree — // otherwise a renamed or deleted template previews happily and then fails to save. - if (!loadFromFilesystem(slug)) { + // + // Membership rather than an existence probe, for the reason PUT gives: a read + // of `../docs/deployment` succeeds, so `if (!loadFromFilesystem(slug))` + // approved the traversal it looked like it rejected. + if (!isKnownTemplateSlug(slug)) { return NextResponse.json({ error: 'Template not found' }, { status: 404 }); } + // Read once, then parsed here rather than by `request.json()`. + // + // No body at all is fine — it means "preview what is stored". Unparseable JSON + // is not: silently rendering the stored template would show the author content + // they did not ask for, which is the bug this route was fixed for. + // + // Two reasons for this shape. "Empty" now comes from the body itself instead of + // `content-length`, because a bodiless POST can arrive with no such header at + // all — chunked, or a server-side `new Request(url, { method: 'POST' })` — and + // keying on `!== '0'` sent those to a 400 rather than previewing, the opposite + // of the intent. And a request body can only be read once: calling + // `request.json()` first consumes the stream, so a later `text()` returns empty + // and an unparseable body would have read as "no body" and previewed the stored + // template — reintroducing the bug this route exists to fix. + const raw = await request.text().catch(() => ''); + let parsed: unknown = null; - try { - parsed = await request.json(); - } catch { - // No body at all is fine — it means "preview what is stored". Unparseable JSON - // is not: silently rendering the stored template would show the author content - // they did not ask for, which is the bug this route was fixed for. - if (request.headers.get('content-length') !== '0') { - const hasBody = await Promise.resolve(true); - if (hasBody) { - return NextResponse.json( - { error: 'Request body is not valid JSON' }, - { status: 400 }, - ); - } + if (raw.trim() !== '') { + try { + parsed = JSON.parse(raw); + } catch { + return NextResponse.json({ error: 'Request body is not valid JSON' }, { status: 400 }); } } diff --git a/apps/web/src/app/api/templates/[slug]/route.ts b/apps/web/src/app/api/templates/[slug]/route.ts index 629401a5..c45fbc6f 100644 --- a/apps/web/src/app/api/templates/[slug]/route.ts +++ b/apps/web/src/app/api/templates/[slug]/route.ts @@ -1,7 +1,7 @@ import { NextRequest, NextResponse } from 'next/server'; import { requireSession, requireAdmin } from '@/lib/require-admin'; import { prisma } from '@copilotkit/outpost/db'; -import { loadFromFilesystem, loadTemplate } from '@copilotkit/outpost/shared/server'; +import { isKnownTemplateSlug, loadTemplate } from '@copilotkit/outpost/shared/server'; /** Bounds on stored template content. The subject becomes an email header downstream. */ const SUBJECT_MAX = 500; @@ -33,6 +33,16 @@ export async function GET( if (error) return error; const { slug } = await params; + + // Checked before the loader sees it. `loadFromFilesystem` builds + // `join(dir, slug + '.md')` with no validation and Next decodes + // percent-encoding in a dynamic segment, so `../docs/deployment` reached the + // loader as a traversal and this handler returned the file's `subject` and + // `body` in its JSON. Verified against the real function before fixing. + if (!isKnownTemplateSlug(slug)) { + return NextResponse.json({ error: 'Template not found' }, { status: 404 }); + } + const loaded = await loadTemplate(slug, findOverride); if (!loaded) { @@ -54,10 +64,7 @@ export async function GET( * * Save a template override (ADMIN only). */ -export async function PUT( - request: NextRequest, - { params }: { params: Promise<{ slug: string }> }, -) { +export async function PUT(request: NextRequest, { params }: { params: Promise<{ slug: string }> }) { const { error, session } = await requireAdmin(); if (error) return error; @@ -98,7 +105,13 @@ export async function PUT( // An override only means anything layered over a real template: the loader takes // its name and `from` from the filesystem entry, and the editor lists filesystem // slugs. Without this check an arbitrary slug would create a row nothing reads. - if (!loadFromFilesystem(slug)) { + // + // Membership rather than an existence probe. `if (!loadFromFilesystem(slug))` + // read as a guard and was the traversal: the read SUCCEEDS for + // `../docs/deployment`, so the check approved the request it appeared to + // reject. A traversal path is never a member of the slug list however it is + // spelled, and a slug that passes here is already known to exist. + if (!isKnownTemplateSlug(slug)) { return NextResponse.json({ error: 'Template not found' }, { status: 404 }); } @@ -140,6 +153,16 @@ export async function DELETE( if (error) return error; const { slug } = await params; + + // Not a traversal risk on its own — `deleteMany` on an unknown slug matches + // nothing. Checked anyway so all four handlers answer the same way for the + // same input; GET returning 404 while DELETE reports `reset: true` for the + // same slug is the kind of disagreement that gets read as one of them being + // wrong. + if (!isKnownTemplateSlug(slug)) { + return NextResponse.json({ error: 'Template not found' }, { status: 404 }); + } + const { count } = await prisma.templateOverride.deleteMany({ where: { slug } }); return NextResponse.json({ slug, reset: true, hadOverride: count > 0 }); diff --git a/apps/web/src/app/settings/templates/page.tsx b/apps/web/src/app/settings/templates/page.tsx index 6b743a2b..2b233427 100644 --- a/apps/web/src/app/settings/templates/page.tsx +++ b/apps/web/src/app/settings/templates/page.tsx @@ -123,7 +123,19 @@ export default function TemplatesPage() { }); if (!res.ok) throw new Error(await describeFailure(res, 'Save failed')); await fetchTemplates(); - setSuccess('Template saved successfully'); + // Says what actually happened. The override is written and every read + // surface honours it — this list, GET, the preview — but outgoing email + // does not: `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 an invitee receives the on-disk copy. + // + // "Template saved successfully" was true about the row and false about + // the thing the author cared about, which is the same silent-success + // shape as the rest of this screen's history. Wiring the lookup is + // tracked separately, and per outpost#253 would not make an edited + // template reach an invitee today either. + setSuccess('Saved. Not yet used for outgoing email — see outpost#226.'); } catch (err) { setError(err instanceof Error ? err.message : 'Save failed'); } finally { @@ -162,12 +174,9 @@ export default function TemplatesPage() {
{error && ( @@ -209,7 +218,7 @@ export default function TemplatesPage() { : 'bg-muted text-muted-foreground' }`} > - {t.isOverride ? 'Custom' : 'Default'} + {t.isOverride ? 'Custom (preview only)' : 'Default'}

@@ -275,27 +284,27 @@ export default function TemplatesPage() {

{/* - * Rendered in a sandboxed iframe, not via - * dangerouslySetInnerHTML. Template bodies are - * author-editable and stored, so injecting them - * here would execute saved script in every later - * viewer's session — and because the csrf cookie - * must be readable by client JS for the - * double-submit header, that script could read the - * CSRF token too. - * - * `sandbox=""` grants nothing: no scripts, no - * same-origin access. Do not add allow-scripts or - * allow-same-origin — together they let the frame - * remove its own sandbox. An allowlist sanitiser - * was the alternative and was rejected: templates - * legitimately contain rich HTML, so a sanitiser - * fights the feature and gets loosened over time. - * - * The height is fixed because measuring content to - * auto-size requires scripting in the frame, which - * is the thing being prevented. - */} + * Rendered in a sandboxed iframe, not via + * dangerouslySetInnerHTML. Template bodies are + * author-editable and stored, so injecting them + * here would execute saved script in every later + * viewer's session — and because the csrf cookie + * must be readable by client JS for the + * double-submit header, that script could read the + * CSRF token too. + * + * `sandbox=""` grants nothing: no scripts, no + * same-origin access. Do not add allow-scripts or + * allow-same-origin — together they let the frame + * remove its own sandbox. An allowlist sanitiser + * was the alternative and was rejected: templates + * legitimately contain rich HTML, so a sanitiser + * fights the feature and gets loosened over time. + * + * The height is fixed because measuring content to + * auto-size requires scripting in the frame, which + * is the thing being prevented. + */}