From ddc23c5a6e9dfa911bb6d98b6afc704f02013358 Mon Sep 17 00:00:00 2001 From: DavidBabinec Date: Fri, 31 Jul 2026 12:58:42 +0200 Subject: [PATCH] fix(publish): stop author bindings publishing the account's email address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `{currentEntry.author}` rendered `{"displayName":"owner@example.com","roleSlug":"owner","roleName":"Owner"}` into live pages. Three causes stacked. The token interpolator JSON-stringified any non-scalar "so mistakes stay visible" — an author-facing debug affordance running in the public publish path. The entry and loop frames overlaid the internal `PublicDataUserReference` object into the same flat namespace as user cells, under the very key an author would reach for. And both setup and `createUser` defaulted `display_name` — a publicly rendered field — to the account's email address, so the legitimate `authorName` field leaked it too. `author` and `publishedBy` are now the display name itself, which is what a binding to them was always meant to produce, and both are declared in the loop source's field list so they are discoverable rather than accidental. A non-scalar binding now renders as missing (honouring any author fallback) instead of dumping whatever internal shape sat behind the key — the frame cannot know which of its values are safe to print. Display names are empty until somebody sets one. Setup takes an optional `displayName` and the first onboarding screen offers it, since the account page was previously the only place to set it. Migration 024 clears display names that are exactly the account's email, because otherwise no existing install benefits. Admin surfaces already fall back to the email for their own authenticated display and are unchanged. Test: `{currentEntry.author}` renders the display name; object and array bindings render empty and still honour a fallback; scalars are untouched; setup with and without a name; `createUser` no longer falls back to the email. React Doctor flags `prefer-useReducer` on AdminPreAuthForm: the new field takes it from 6 useState calls to 7. Those are independent form scalars plus two status flags — a reducer would not simplify them, and refactoring an auth form does not belong in a security fix. --- server/db/migrations-pg.ts | 9 +++ server/db/migrations-sqlite.ts | 13 ++++ server/handlers/cms/setup.ts | 6 +- server/publish/loopPrefetch.ts | 9 ++- server/repositories/users.ts | 11 ++- src/__tests__/loops/dataRowsSource.test.ts | 8 ++- .../publisher/authorBindingLeak.test.ts | 57 +++++++++++++++ src/__tests__/server/loopPrefetch.test.ts | 18 +++-- src/__tests__/server/setupDisplayName.test.ts | 72 +++++++++++++++++++ src/admin/preauth/AdminPreAuthForm.module.css | 6 ++ src/admin/preauth/AdminPreAuthForm.tsx | 40 ++++++++--- src/core/data/publicDataUser.ts | 11 ++- src/core/loops/sources/dataRows.ts | 9 +-- src/core/persistence/cmsAuth.ts | 2 + src/core/templates/tokenInterpolation.ts | 20 +++--- 15 files changed, 249 insertions(+), 42 deletions(-) create mode 100644 src/__tests__/publisher/authorBindingLeak.test.ts create mode 100644 src/__tests__/server/setupDisplayName.test.ts diff --git a/server/db/migrations-pg.ts b/server/db/migrations-pg.ts index e6ca8ee65..d129ffaa2 100644 --- a/server/db/migrations-pg.ts +++ b/server/db/migrations-pg.ts @@ -1147,4 +1147,13 @@ export const pgMigrations: Migration[] = [ alter table collab_documents add column generation text not null default ''; `, }, + { + // See migrations-sqlite.ts:024 — clears display names that are just the + // account's email address, because author bindings render them publicly. + id: '024_clear_email_display_names', + sql: ` + update users set display_name = '' + where trim(lower(display_name)) = trim(lower(email)); + `, + }, ] diff --git a/server/db/migrations-sqlite.ts b/server/db/migrations-sqlite.ts index 734a53fb3..a4c5cdd74 100644 --- a/server/db/migrations-sqlite.ts +++ b/server/db/migrations-sqlite.ts @@ -1211,4 +1211,17 @@ export const sqliteMigrations: Migration[] = [ alter table collab_documents add column generation text not null default ''; `, }, + { + // `display_name` is rendered on PUBLIC pages through author bindings, and + // setup used to default it to the account's email address — so every + // install created before this has an address sitting in a public field. + // Clear it only where it is exactly the address; a name somebody actually + // chose is left alone. Admin surfaces already fall back to the email for + // their own display, so an empty value costs nothing there. + id: '024_clear_email_display_names', + sql: ` + update users set display_name = '' + where trim(lower(display_name)) = trim(lower(email)); + `, + }, ] diff --git a/server/handlers/cms/setup.ts b/server/handlers/cms/setup.ts index c9cea4613..9581fe11d 100644 --- a/server/handlers/cms/setup.ts +++ b/server/handlers/cms/setup.ts @@ -54,12 +54,16 @@ export async function handleSetupRoutes(req: Request, db: DbClient): Promise { it('offers author display fields without exposing user ids as binding fields', () => { + // `author` is declared, because it is the name an author reaches for and + // an undeclared key is exactly how a binding to a non-field goes unnoticed. expect(DataRowsSource.fields).toContainEqual({ - id: 'authorName', + id: 'author', label: 'Author name', }) + expect(DataRowsSource.fields).toContainEqual({ + id: 'authorName', + label: 'Author name (alias)', + }) expect(DataRowsSource.fields).toContainEqual({ id: 'authorRoleName', label: 'Author role', diff --git a/src/__tests__/publisher/authorBindingLeak.test.ts b/src/__tests__/publisher/authorBindingLeak.test.ts new file mode 100644 index 000000000..bb27b8bbd --- /dev/null +++ b/src/__tests__/publisher/authorBindingLeak.test.ts @@ -0,0 +1,57 @@ +/** + * A binding must never publish internal shape. + * + * `{currentEntry.author}` used to resolve to the `PublicDataUserReference` + * object, and the token interpolator JSON-stringified any non-scalar "so + * mistakes stay visible" — an author-facing debug affordance running in the + * public publish path. A single template binding therefore published + * `{"displayName":"owner@example.com","roleSlug":"owner","roleName":"Owner"}` + * into live HTML, and into every screenshot taken of it. + * + * Two independent guards keep that shut, and both are asserted here: the + * people keys are leaf strings, and a non-scalar value renders as nothing + * rather than as JSON. + */ +import { describe, expect, it } from 'bun:test' +import { interpolateTokens } from '@core/templates/tokenInterpolation' +import type { TemplateContext } from '@core/templates/contextFrames' + +function contextWithEntry(fields: Record): TemplateContext { + return { entryStack: [{ id: 'row_1', fields }] } as unknown as TemplateContext +} + +describe('author bindings never publish internal shape', () => { + it('renders the display name for {currentEntry.author}', () => { + const ctx = contextWithEntry({ author: 'Rosa Ferrandis' }) + expect(interpolateTokens('By {currentEntry.author}', ctx)).toBe('By Rosa Ferrandis') + }) + + it('renders nothing for a binding that resolves to an object', () => { + const ctx = contextWithEntry({ + author: { displayName: 'owner@example.com', roleSlug: 'owner', roleName: 'Owner' }, + }) + const out = interpolateTokens('By {currentEntry.author}', ctx) + + expect(out).toBe('By ') + expect(out).not.toContain('@') + expect(out).not.toContain('roleSlug') + expect(out).not.toContain('displayName') + }) + + it('renders nothing for a binding that resolves to an array', () => { + const ctx = contextWithEntry({ tags: ['a', 'b'] }) + expect(interpolateTokens('{currentEntry.tags}', ctx)).toBe('') + }) + + it('still honours an author-supplied fallback when the value is non-scalar', () => { + const ctx = contextWithEntry({ author: { displayName: 'owner@example.com' } }) + expect(interpolateTokens('{currentEntry.author|Anonymous}', ctx)).toBe('Anonymous') + }) + + it('leaves scalar bindings untouched', () => { + const ctx = contextWithEntry({ title: 'El inventario', year: 2025, pardoned: false }) + expect(interpolateTokens('{currentEntry.title} ({currentEntry.year})', ctx)) + .toBe('El inventario (2025)') + expect(interpolateTokens('{currentEntry.pardoned}', ctx)).toBe('false') + }) +}) diff --git a/src/__tests__/server/loopPrefetch.test.ts b/src/__tests__/server/loopPrefetch.test.ts index 4d315cc2b..32842b94a 100644 --- a/src/__tests__/server/loopPrefetch.test.ts +++ b/src/__tests__/server/loopPrefetch.test.ts @@ -49,24 +49,22 @@ describe('loopPrefetch', () => { createdAt: '2026-05-01T10:02:00.000Z', }) + // Every people key is a LEAF. `author` used to be the reference object, + // so a template binding it published `{"displayName":…,"roleSlug":…}` + // straight into the page. expect(item.fields).toMatchObject({ - author: { - displayName: 'Author Name', - roleSlug: 'editor', - roleName: 'Editor', - }, + author: 'Author Name', authorName: 'Author Name', authorRoleName: 'Editor', authorRoleSlug: 'editor', - publishedBy: { - displayName: 'Publisher Name', - roleSlug: 'admin', - roleName: 'Admin', - }, + publishedBy: 'Publisher Name', publishedByName: 'Publisher Name', publishedByRoleName: 'Admin', publishedByRoleSlug: 'admin', }) + for (const key of ['author', 'publishedBy']) { + expect(typeof item.fields[key]).not.toBe('object') + } expect('authorUserId' in item.fields).toBe(false) expect('authorId' in item.fields).toBe(false) expect('publishedByUserId' in item.fields).toBe(false) diff --git a/src/__tests__/server/setupDisplayName.test.ts b/src/__tests__/server/setupDisplayName.test.ts new file mode 100644 index 000000000..aee62024e --- /dev/null +++ b/src/__tests__/server/setupDisplayName.test.ts @@ -0,0 +1,72 @@ +/** + * A display name is never silently the account's email address. + * + * `display_name` is rendered on PUBLIC pages through author bindings, and both + * setup and `createUser` used to default it to the email. So a fresh install + * had the owner's address sitting in a publicly-bindable field before anyone + * had written a line of content, and the only way to change it was the account + * page after the fact. + * + * Setup now takes an optional `displayName`, and an unset name stays empty. + */ +import { describe, expect, it } from 'bun:test' +import type { DbClient } from '../../../server/db' +import { handleCmsRequest } from '../../../server/handlers/cms' +import { createUser, findUserByEmail } from '../../../server/repositories/users' +import { createTestDb } from '../helpers/createTestDb' + +const EMAIL = 'owner@example.com' +const PASSWORD = 'long-enough-password' + +async function runSetup(db: DbClient, body: Record): Promise { + return await handleCmsRequest( + new Request('http://localhost/admin/api/cms/setup', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ siteName: 'Test', email: EMAIL, password: PASSWORD, ...body }), + }), + db, + ) +} + +describe('setup display name', () => { + it('leaves the display name empty when none is given', async () => { + const { db } = await createTestDb() + expect((await runSetup(db, {})).status).toBe(201) + + const owner = await findUserByEmail(db, EMAIL) + expect(owner?.displayName).toBe('') + expect(owner?.displayName).not.toContain('@') + }) + + it('stores the display name given at setup', async () => { + const { db } = await createTestDb() + expect((await runSetup(db, { displayName: 'Rosa Ferrandis' })).status).toBe(201) + + expect((await findUserByEmail(db, EMAIL))?.displayName).toBe('Rosa Ferrandis') + }) + + it('trims a whitespace-only display name to empty rather than the email', async () => { + const { db } = await createTestDb() + expect((await runSetup(db, { displayName: ' ' })).status).toBe(201) + + expect((await findUserByEmail(db, EMAIL))?.displayName).toBe('') + }) +}) + +describe('createUser display name', () => { + it('does not fall back to the email address', async () => { + const { db } = await createTestDb() + await runSetup(db, {}) + + const created = await createUser(db, { + email: 'editor@example.com', + displayName: '', + passwordHash: 'x', + roleId: 'admin', + }) + + expect(created.displayName).toBe('') + expect(created.displayName).not.toContain('@') + }) +}) diff --git a/src/admin/preauth/AdminPreAuthForm.module.css b/src/admin/preauth/AdminPreAuthForm.module.css index 9d15f0728..37699825d 100644 --- a/src/admin/preauth/AdminPreAuthForm.module.css +++ b/src/admin/preauth/AdminPreAuthForm.module.css @@ -43,6 +43,12 @@ font-weight: 600; } +/* Secondary note inside a field label — lighter than the label it follows. */ +.hint { + color: var(--text-subtle); + font-weight: 400; +} + @keyframes instaticAdminSpin { to { transform: rotate(360deg); diff --git a/src/admin/preauth/AdminPreAuthForm.tsx b/src/admin/preauth/AdminPreAuthForm.tsx index c0b4377aa..12950bd28 100644 --- a/src/admin/preauth/AdminPreAuthForm.tsx +++ b/src/admin/preauth/AdminPreAuthForm.tsx @@ -68,6 +68,7 @@ export function AdminPreAuthForm({ onAuthenticated, }: AdminPreAuthFormProps) { const [siteName, setSiteName] = useState('My Site') + const [displayName, setDisplayName] = useState('') const [email, setEmail] = useState('') const [password, setPassword] = useState('') const [mfaCode, setMfaCode] = useState('') @@ -75,6 +76,7 @@ export function AdminPreAuthForm({ const [error, setError] = useState(initialError) const siteNameId = useId() + const displayNameId = useId() const emailId = useId() const passwordId = useId() const mfaCodeId = useId() @@ -86,7 +88,7 @@ export function AdminPreAuthForm({ return } await runAuthAction(async () => { - await setupCms({ siteName, email, password }) + await setupCms({ siteName, email, password, displayName }) await loginCms({ email, password }) onAuthenticated(await getCurrentCmsUser()) }, 'Setup failed', setSubmitting, setError) @@ -167,16 +169,32 @@ export function AdminPreAuthForm({ /> ) : phase === 'setup' && ( - + <> + + + {/* Optional, and public: this is what author bindings render on + published pages. Left blank they render nothing — which is + why it is offered here rather than only on the account page. */} + + )} {phase !== 'mfa' && ( diff --git a/src/core/data/publicDataUser.ts b/src/core/data/publicDataUser.ts index ba852c423..24ea45126 100644 --- a/src/core/data/publicDataUser.ts +++ b/src/core/data/publicDataUser.ts @@ -1,8 +1,15 @@ /** * Public-facing user reference attached to published data rows. * - * Strips internal-only ids and email addresses, retaining only the bits the - * publisher and frontend templates need to render bylines safely. + * Drops internal-only ids and the `email` field, retaining only the bits the + * publisher and frontend templates need to render bylines. + * + * It does NOT guarantee the result is free of email addresses: `displayName` + * carries whatever the account has set, and setup used to default that to the + * address. Two things keep it out of public HTML now — a display name is empty + * until someone sets one (`createUser`), and the binding frames expose the + * display name as a leaf string rather than this object, because a non-scalar + * binding used to be serialised wholesale into the page. */ interface PublicDataUserReference { diff --git a/src/core/loops/sources/dataRows.ts b/src/core/loops/sources/dataRows.ts index edef240c3..ab96d8151 100644 --- a/src/core/loops/sources/dataRows.ts +++ b/src/core/loops/sources/dataRows.ts @@ -164,12 +164,12 @@ function rowToLoopItem( versionNumber: Number(row.version_number), tableId: row.table_id, tableSlug: row.table_slug, - // People - author, + // People — display names, never the reference object. See loopPrefetch. + author: author?.displayName ?? null, authorName: author?.displayName ?? null, authorRoleSlug: author?.roleSlug ?? null, authorRoleName: author?.roleName ?? null, - publishedBy, + publishedBy: publishedBy?.displayName ?? null, publishedByName: publishedBy?.displayName ?? null, publishedByRoleSlug: publishedBy?.roleSlug ?? null, publishedByRoleName: publishedBy?.roleName ?? null, @@ -503,7 +503,8 @@ export const DataRowsSource: LoopEntitySource = { fields: [ { id: 'slug', label: 'Slug' }, { id: 'title', label: 'Title (post-type)' }, - { id: 'authorName', label: 'Author name' }, + { id: 'author', label: 'Author name' }, + { id: 'authorName', label: 'Author name (alias)' }, { id: 'authorRoleName', label: 'Author role' }, { id: 'body', label: 'Body (post-type, markdown)', format: 'html' }, { id: 'featuredMedia', label: 'Featured media (post-type)', format: 'media' }, diff --git a/src/core/persistence/cmsAuth.ts b/src/core/persistence/cmsAuth.ts index 6f8a3d681..f655820db 100644 --- a/src/core/persistence/cmsAuth.ts +++ b/src/core/persistence/cmsAuth.ts @@ -11,6 +11,8 @@ interface CmsSetupInput { siteName: string email: string password: string + /** The owner's public name. Omitted or empty means author bindings render nothing. */ + displayName?: string } interface CmsLoginInput { diff --git a/src/core/templates/tokenInterpolation.ts b/src/core/templates/tokenInterpolation.ts index 6946abc99..7314c14c2 100644 --- a/src/core/templates/tokenInterpolation.ts +++ b/src/core/templates/tokenInterpolation.ts @@ -276,14 +276,18 @@ export function interpolateTokens(input: string, context: TemplateRenderDataCont } else if (typeof rawValue === 'number' || typeof rawValue === 'boolean') { out += String(rawValue) } else { - // Objects / arrays — fall back to JSON for visibility. Real-world - // bindings should target leaf fields; surfacing the JSON keeps - // mistakes visible instead of silent. - try { - out += JSON.stringify(rawValue) - } catch { - // ignore - } + // Objects / arrays resolve to NOTHING, deliberately. + // + // This branch used to JSON.stringify the value "so mistakes stay + // visible". That is an author-facing debug affordance running in the + // public publish path: `{currentEntry.author}` against a reference + // object published the account's display name, role slug and role name + // into live HTML. A binding that misses is a blank, never a dump of + // whatever internal shape happened to sit behind the key — the frame + // cannot know which of its values are safe to print. + // + // Treated as missing, so an author-supplied fallback still applies. + if (seg.fallback !== undefined) out += seg.fallback } } return out