From 9ccd697a75cbf99140a84fcf0ed4442f569eb0b6 Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:43:57 +0200 Subject: [PATCH 01/15] chore(admin): standalone build copies static + public; restore rewrites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The standalone Next.js build emitted by `output: 'standalone'` does not auto-copy `.next/static` or `public/` into the standalone tree — the operator must. Without these, every JS chunk and theme asset 404s on the running container. - Copy `.next/static` and `public/` into the standalone bundle. - Restore `rewrites()` so `/api/*` proxies to GONEXT_API_URL (browser- side fetches stay same-origin; the rewrite hops to the docker-internal hostname). - Thread `GONEXT_API_URL` through as a build-arg so `rewrites()` (which evaluates at build time) sees it. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- apps/admin/Dockerfile | 17 +++++++++++++++++ apps/admin/next.config.ts | 17 +++++++++++++++++ pnpm-lock.yaml | 19 +++++++++++++++++++ 3 files changed, 53 insertions(+) diff --git a/apps/admin/Dockerfile b/apps/admin/Dockerfile index bfb29d26..d83fd3eb 100644 --- a/apps/admin/Dockerfile +++ b/apps/admin/Dockerfile @@ -72,10 +72,15 @@ COPY apps/admin/ apps/admin/ COPY packages/ts/ packages/ts/ # Build-time configuration plumbing for Next.js public env vars. +# GONEXT_API_URL is consumed by next.config.ts's `rewrites()` at build +# time — must be the docker-internal API hostname (e.g. http://api:8080) +# so server-side proxy hops work after the bundle is baked. ARG NEXT_PUBLIC_API_URL="" ARG NEXT_PUBLIC_VERSION="dev" +ARG GONEXT_API_URL="" ENV NEXT_PUBLIC_API_URL=${NEXT_PUBLIC_API_URL} \ NEXT_PUBLIC_VERSION=${NEXT_PUBLIC_VERSION} \ + GONEXT_API_URL=${GONEXT_API_URL} \ NEXT_TELEMETRY_DISABLED=1 \ NODE_ENV=production @@ -113,6 +118,18 @@ COPY --from=builder --chown=node:node /repo/node_modules ./node_modules COPY --from=builder --chown=node:node /repo/apps/admin ./apps/admin COPY --from=builder --chown=node:node /repo/packages/ts ./packages/ts +# Next.js `output: 'standalone'` ships a minimal node_modules tree at +# .next/standalone/, but DOES NOT copy .next/static or public/ into +# that tree (the docs explicitly say the operator must do this). When +# the entrypoint runs node .next/standalone/apps/admin/server.js, the +# server's static handler looks for chunks at +# .next/standalone/apps/admin/.next/static/* — which 404s without these +# COPYs and breaks every JS/CSS chunk load on every page. +COPY --from=builder --chown=node:node /repo/apps/admin/.next/static \ + ./apps/admin/.next/standalone/apps/admin/.next/static +COPY --from=builder --chown=node:node /repo/apps/admin/public \ + ./apps/admin/.next/standalone/apps/admin/public + USER node EXPOSE 3000 diff --git a/apps/admin/next.config.ts b/apps/admin/next.config.ts index 8063564d..6db4d08e 100644 --- a/apps/admin/next.config.ts +++ b/apps/admin/next.config.ts @@ -21,6 +21,23 @@ const nextConfig: NextConfig = { // typedRoutes moved out of `experimental` in Next.js 15; opt in at the // top level so `` values are checked at build time. typedRoutes: true, + /** + * Proxy /api/* to the GoNext API service. When the bundle ships with + * NEXT_PUBLIC_API_URL="" (the docker-compose default) the browser + * uses same-origin fetches against /api/*. This rewrite hops those + * requests over to the docker-internal API hostname so dev / preview + * environments don't need a separate reverse proxy. + * + * rewrites() evaluates at build time, so GONEXT_API_URL must be set + * as a build-arg (see apps/admin/Dockerfile + docker-compose.override.yml). + */ + async rewrites() { + const apiUrl = + process.env.GONEXT_API_URL || + process.env.NEXT_PUBLIC_API_URL || + 'http://localhost:8080'; + return [{ source: '/api/:path*', destination: `${apiUrl}/api/:path*` }]; + }, async headers() { return [ { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f1a4e429..666d50c3 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -627,6 +627,25 @@ importers: specifier: ^1.6.0 version: 1.6.1(@types/node@22.19.19)(jsdom@24.1.3) + packages/ts/sdk-plugin: + dependencies: + esbuild: + specifier: ^0.21.0 + version: 0.21.5 + devDependencies: + '@gonext/test-config': + specifier: workspace:* + version: link:../test-config + '@vitest/coverage-v8': + specifier: ^1.6.0 + version: 1.6.1(vitest@1.6.1(@types/node@22.19.19)(jsdom@29.1.1)) + typescript: + specifier: ^5.6.0 + version: 5.9.3 + vitest: + specifier: ^1.6.0 + version: 1.6.1(@types/node@22.19.19)(jsdom@29.1.1) + packages/ts/test-config: devDependencies: '@testing-library/jest-dom': From 5e0c031447a909879a868ec325ee6e155a2232ba Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:44:13 +0200 Subject: [PATCH 02/15] fix(admin): resolve API base URL at runtime, not module-load (#498) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `apiBaseUrl` was exported as a module-load constant. Next.js bundles Client Components at build time, where `typeof window === 'undefined'` evaluates TRUE on the bundler's Node process, so the SERVER branch won and `http://api:8080` (docker-internal) was baked into every browser bundle — producing DNS failures on every client-side fetch. Convert `apiBaseUrl` from a const to a function called at request time. Server Components resolve via `GONEXT_API_URL`; Client Components resolve via `NEXT_PUBLIC_API_URL` (empty string preserved for same-origin + rewrites). Updated 16 call sites across plugins, themes, marketplace, migrate, media, setup. Added api-client.test.ts with three cases: server branch, client branch, empty-string preservation. server-api.ts also imports apiBaseUrl — updated. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- .../appearance/themes/api-client.ts | 6 +- .../(authenticated)/appearance/themes/api.ts | 4 +- .../(authenticated)/marketplace/actions.ts | 2 +- .../src/app/(authenticated)/media/actions.ts | 2 +- .../migrate/steps/PreviewStep.tsx | 2 +- .../(authenticated)/migrate/steps/RunStep.tsx | 4 +- .../(authenticated)/plugins/[name]/page.tsx | 2 +- .../app/(authenticated)/plugins/actions.ts | 2 +- .../src/app/(authenticated)/plugins/page.tsx | 2 +- .../src/app/(public)/setup/SetupWizard.tsx | 2 +- apps/admin/src/lib/api-client.test.ts | 55 ++++++++++++++++++ apps/admin/src/lib/api-client.ts | 58 +++++++++++++++++-- apps/admin/src/lib/server-api.ts | 2 +- 13 files changed, 124 insertions(+), 19 deletions(-) create mode 100644 apps/admin/src/lib/api-client.test.ts diff --git a/apps/admin/src/app/(authenticated)/appearance/themes/api-client.ts b/apps/admin/src/app/(authenticated)/appearance/themes/api-client.ts index 910c7d4f..89d82002 100644 --- a/apps/admin/src/app/(authenticated)/appearance/themes/api-client.ts +++ b/apps/admin/src/app/(authenticated)/appearance/themes/api-client.ts @@ -11,7 +11,7 @@ import type { InstallResponse, ThemesListResponse } from './types'; export async function fetchThemesListClient(): Promise { try { - const res = await fetch(`${apiBaseUrl}/api/v1/admin/themes`, { + const res = await fetch(`${apiBaseUrl()}/api/v1/admin/themes`, { method: 'GET', credentials: 'include', headers: { Accept: 'application/json' }, @@ -30,7 +30,7 @@ export async function fetchThemesListClient(): Promise { const formData = new FormData(); formData.append('file', file, file.name); - const res = await fetch(`${apiBaseUrl}/api/v1/admin/themes/install`, { + const res = await fetch(`${apiBaseUrl()}/api/v1/admin/themes/install`, { method: 'POST', credentials: 'include', body: formData, @@ -42,7 +42,7 @@ export async function installTheme(file: File): Promise { } export async function activateTheme(slug: string): Promise { - const res = await fetch(`${apiBaseUrl}/api/v1/admin/themes/activate`, { + const res = await fetch(`${apiBaseUrl()}/api/v1/admin/themes/activate`, { method: 'POST', credentials: 'include', headers: { 'Content-Type': 'application/json' }, diff --git a/apps/admin/src/app/(authenticated)/appearance/themes/api.ts b/apps/admin/src/app/(authenticated)/appearance/themes/api.ts index 58b980ef..03902f10 100644 --- a/apps/admin/src/app/(authenticated)/appearance/themes/api.ts +++ b/apps/admin/src/app/(authenticated)/appearance/themes/api.ts @@ -44,7 +44,7 @@ export async function fetchThemesList(): Promise { export async function installTheme(file: File): Promise { const formData = new FormData(); formData.append('file', file, file.name); - const res = await fetch(`${apiBaseUrl}${INSTALL_URL}`, { + const res = await fetch(`${apiBaseUrl()}${INSTALL_URL}`, { method: 'POST', credentials: 'include', body: formData, @@ -63,7 +63,7 @@ export async function installTheme(file: File): Promise { * message preserved. */ export async function activateTheme(slug: string): Promise { - const res = await fetch(`${apiBaseUrl}${ACTIVATE_URL}`, { + const res = await fetch(`${apiBaseUrl()}${ACTIVATE_URL}`, { method: 'POST', credentials: 'include', headers: { 'Content-Type': 'application/json' }, diff --git a/apps/admin/src/app/(authenticated)/marketplace/actions.ts b/apps/admin/src/app/(authenticated)/marketplace/actions.ts index c489c929..f29cab77 100644 --- a/apps/admin/src/app/(authenticated)/marketplace/actions.ts +++ b/apps/admin/src/app/(authenticated)/marketplace/actions.ts @@ -39,7 +39,7 @@ async function cookieHeader(): Promise { } async function call(path: string, init: RequestInit = {}): Promise { - const url = `${apiBaseUrl.replace(/\/$/, '')}${path}`; + const url = `${apiBaseUrl().replace(/\/$/, '')}${path}`; const cookie = await cookieHeader(); const headers = new Headers(init.headers); headers.set('Accept', 'application/json'); diff --git a/apps/admin/src/app/(authenticated)/media/actions.ts b/apps/admin/src/app/(authenticated)/media/actions.ts index db241130..6a2e2be0 100644 --- a/apps/admin/src/app/(authenticated)/media/actions.ts +++ b/apps/admin/src/app/(authenticated)/media/actions.ts @@ -116,7 +116,7 @@ export function uploadMedia( // touch Content-Type. return new Promise((resolve, reject) => { const xhr = new XMLHttpRequest(); - xhr.open('POST', `${apiBaseUrl}/api/v1/admin/media`, true); + xhr.open('POST', `${apiBaseUrl()}/api/v1/admin/media`, true); xhr.withCredentials = true; if (xhr.upload && onProgress) { diff --git a/apps/admin/src/app/(authenticated)/migrate/steps/PreviewStep.tsx b/apps/admin/src/app/(authenticated)/migrate/steps/PreviewStep.tsx index ec2f0d30..4dcc0266 100644 --- a/apps/admin/src/app/(authenticated)/migrate/steps/PreviewStep.tsx +++ b/apps/admin/src/app/(authenticated)/migrate/steps/PreviewStep.tsx @@ -65,7 +65,7 @@ export function PreviewStep({ onError(null); try { const form = buildDryRunForm(source, options); - const res = await fetcher(`${apiBaseUrl}/api/v1/admin/migrate/dry-run`, { + const res = await fetcher(`${apiBaseUrl()}/api/v1/admin/migrate/dry-run`, { method: 'POST', body: form, credentials: 'include', diff --git a/apps/admin/src/app/(authenticated)/migrate/steps/RunStep.tsx b/apps/admin/src/app/(authenticated)/migrate/steps/RunStep.tsx index a3c6859b..32e8d35e 100644 --- a/apps/admin/src/app/(authenticated)/migrate/steps/RunStep.tsx +++ b/apps/admin/src/app/(authenticated)/migrate/steps/RunStep.tsx @@ -91,7 +91,7 @@ export function RunStep({ onError(null); try { const form = buildDryRunForm(source, options); - const res = await fetcher(`${apiBaseUrl}/api/v1/admin/migrate/start`, { + const res = await fetcher(`${apiBaseUrl()}/api/v1/admin/migrate/start`, { method: 'POST', body: form, credentials: 'include', @@ -110,7 +110,7 @@ export function RunStep({ pollHandle.current = setInterval(async () => { try { const sres = await fetcher( - `${apiBaseUrl}/api/v1/admin/migrate/status?jobId=${encodeURIComponent(jobId)}`, + `${apiBaseUrl()}/api/v1/admin/migrate/status?jobId=${encodeURIComponent(jobId)}`, { credentials: 'include' }, ); if (!sres.ok) throw new Error(`HTTP ${sres.status}`); diff --git a/apps/admin/src/app/(authenticated)/plugins/[name]/page.tsx b/apps/admin/src/app/(authenticated)/plugins/[name]/page.tsx index 26a545f3..553bd24a 100644 --- a/apps/admin/src/app/(authenticated)/plugins/[name]/page.tsx +++ b/apps/admin/src/app/(authenticated)/plugins/[name]/page.tsx @@ -37,7 +37,7 @@ async function fetchPlugin(name: string): Promise { } catch { cookieHeader = ''; } - const url = `${apiBaseUrl.replace(/\/$/, '')}/api/v1/plugins/${encodeURIComponent(name)}`; + const url = `${apiBaseUrl().replace(/\/$/, '')}/api/v1/plugins/${encodeURIComponent(name)}`; try { const res = await fetch(url, { method: 'GET', diff --git a/apps/admin/src/app/(authenticated)/plugins/actions.ts b/apps/admin/src/app/(authenticated)/plugins/actions.ts index d90927b5..916758da 100644 --- a/apps/admin/src/app/(authenticated)/plugins/actions.ts +++ b/apps/admin/src/app/(authenticated)/plugins/actions.ts @@ -51,7 +51,7 @@ async function callHost( path: string, init: RequestInit, ): Promise { - const url = `${apiBaseUrl.replace(/\/$/, '')}${path}`; + const url = `${apiBaseUrl().replace(/\/$/, '')}${path}`; const cookie = await cookieHeader(); const headers = new Headers(init.headers); if (cookie && !headers.has('Cookie')) headers.set('Cookie', cookie); diff --git a/apps/admin/src/app/(authenticated)/plugins/page.tsx b/apps/admin/src/app/(authenticated)/plugins/page.tsx index 6865688f..aa21da33 100644 --- a/apps/admin/src/app/(authenticated)/plugins/page.tsx +++ b/apps/admin/src/app/(authenticated)/plugins/page.tsx @@ -59,7 +59,7 @@ async function fetchPlugins(): Promise { cookieHeader = ''; } - const url = `${apiBaseUrl.replace(/\/$/, '')}/api/v1/plugins`; + const url = `${apiBaseUrl().replace(/\/$/, '')}/api/v1/plugins`; try { const res = await fetch(url, { method: 'GET', diff --git a/apps/admin/src/app/(public)/setup/SetupWizard.tsx b/apps/admin/src/app/(public)/setup/SetupWizard.tsx index 2221cda3..b700b48d 100644 --- a/apps/admin/src/app/(public)/setup/SetupWizard.tsx +++ b/apps/admin/src/app/(public)/setup/SetupWizard.tsx @@ -166,7 +166,7 @@ export default function SetupWizard({ initialStatus }: SetupWizardProps): ReactE setSubmitting(true); setServerError(null); try { - const response = await fetch(`${apiBaseUrl}/api/v1/setup/install`, { + const response = await fetch(`${apiBaseUrl()}/api/v1/setup/install`, { method: 'POST', credentials: 'include', headers: { diff --git a/apps/admin/src/lib/api-client.test.ts b/apps/admin/src/lib/api-client.test.ts new file mode 100644 index 00000000..85c066d7 --- /dev/null +++ b/apps/admin/src/lib/api-client.test.ts @@ -0,0 +1,55 @@ +/** + * Tests for the admin API base URL resolver. + * + * Regression coverage for issue #498: when `apiBaseUrl` was a + * module-load constant, Next.js's Client Component bundler evaluated + * `typeof window === 'undefined'` at build time (Node process, no + * window) and baked the docker-internal `http://api:8080` into the + * browser bundle, breaking every client fetch with DNS failures. + * + * The fix exposes `apiBaseUrl` as a function so the runtime branch + * resolves in the actual execution environment. These tests pin that + * behavior: + * + * - With no `window`, the server branch picks `GONEXT_API_URL`. + * - With a `window`, the client branch picks `NEXT_PUBLIC_API_URL`. + * - Empty string survives as a valid "same-origin via rewrites" + * value — it must NOT be coerced to the localhost default. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +describe('apiBaseUrl', () => { + const originalEnv = { ...process.env }; + + beforeEach(() => { + vi.resetModules(); + }); + + afterEach(() => { + process.env = { ...originalEnv }; + vi.unstubAllGlobals(); + }); + + it('returns GONEXT_API_URL when running server-side (no window)', async () => { + process.env.GONEXT_API_URL = 'http://api:8080'; + process.env.NEXT_PUBLIC_API_URL = ''; + vi.stubGlobal('window', undefined); + const { apiBaseUrl } = await import('./api-client'); + expect(apiBaseUrl()).toBe('http://api:8080'); + }); + + it('returns NEXT_PUBLIC_API_URL when running in browser (window defined)', async () => { + process.env.GONEXT_API_URL = 'http://api:8080'; + process.env.NEXT_PUBLIC_API_URL = ''; + vi.stubGlobal('window', {} as unknown as Window); + const { apiBaseUrl } = await import('./api-client'); + expect(apiBaseUrl()).toBe(''); + }); + + it('preserves empty string as a valid value (no falsy coerce)', async () => { + process.env.NEXT_PUBLIC_API_URL = ''; + vi.stubGlobal('window', {} as unknown as Window); + const { apiBaseUrl } = await import('./api-client'); + expect(apiBaseUrl()).toBe(''); + }); +}); diff --git a/apps/admin/src/lib/api-client.ts b/apps/admin/src/lib/api-client.ts index 8dc83755..cce8c88f 100644 --- a/apps/admin/src/lib/api-client.ts +++ b/apps/admin/src/lib/api-client.ts @@ -21,9 +21,59 @@ const DEFAULT_BASE_URL = 'http://localhost:8080'; -export const apiBaseUrl: string = - (typeof process !== 'undefined' && process.env.NEXT_PUBLIC_API_URL) || - DEFAULT_BASE_URL; +/** + * Resolve the API base URL. The admin app runs in two contexts: + * + * - Browser (client): NEXT_PUBLIC_API_URL is baked into the bundle at + * build time. When empty (the docker-compose default), fetches hit + * the same origin and the Next.js `rewrites()` config proxies + * `/api/*` to the docker-internal API hostname. + * - Node (server): Server Components / Route Handlers / middleware + * run inside the admin container; the Next.js rewrites do NOT apply + * to outbound fetches from server code. We need an explicit + * docker-internal hostname — GONEXT_API_URL=http://api:8080. + * + * Empty string is a deliberate value (it means "same origin via + * rewrites") and must NOT be coerced to the default. We use an + * explicit `undefined` check rather than `||` to preserve that. + * + * The branch selection MUST happen at request time, not at module-load + * time — see the docstring on `apiBaseUrl()` below for why. + */ +function pickNonUndefined(value: string | undefined): string | undefined { + return value === undefined ? undefined : value; +} + +/** + * Resolve the API base URL at **runtime**. + * + * IMPORTANT: this MUST be called at request time, not module-load time. + * Next.js bundles Client Components at build time, when + * `typeof window === 'undefined'` evaluates to TRUE on the bundler's + * Node process. If we cached the result in a module-load constant the + * server branch would win and `http://api:8080` (the docker-internal + * hostname) would be baked into every Client Component bundle, + * producing browser-side DNS failures on every fetch. Keeping this as + * a function means the `typeof window` check evaluates in the actual + * runtime (browser → client branch, server-component render → server + * branch). See issue #498. + */ +export function apiBaseUrl(): string { + const isServer = typeof window === 'undefined'; + if (isServer) { + // Server: prefer GONEXT_API_URL (docker-internal), fall back to the + // public URL, and only use the localhost default as a last resort. + const server = process.env.GONEXT_API_URL; + if (server !== undefined && server !== '') return server; + const pub = pickNonUndefined(process.env.NEXT_PUBLIC_API_URL); + if (pub !== undefined) return pub; + return DEFAULT_BASE_URL; + } + // Client: empty string is a legitimate "same-origin via rewrites" + // configuration. Only fall through to DEFAULT when the var is unset. + const pub = process.env.NEXT_PUBLIC_API_URL; + return pub === undefined ? DEFAULT_BASE_URL : pub; +} export type ApiMethod = 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE'; @@ -93,7 +143,7 @@ export async function apiRequest( ...headers, }; - const response = await fetch(joinUrl(apiBaseUrl, path), { + const response = await fetch(joinUrl(apiBaseUrl(), path), { method, headers: finalHeaders, credentials: 'include', diff --git a/apps/admin/src/lib/server-api.ts b/apps/admin/src/lib/server-api.ts index 84ec48a7..b7f1b6ad 100644 --- a/apps/admin/src/lib/server-api.ts +++ b/apps/admin/src/lib/server-api.ts @@ -58,7 +58,7 @@ async function buildHeaders( } function joinUrl(path: string): string { - return `${apiBaseUrl}${path.startsWith('/') ? path : `/${path}`}`; + return `${apiBaseUrl()}${path.startsWith('/') ? path : `/${path}`}`; } /** From 630a6fa97227e9a3ea4d4cff8cc58c37b0374c2c Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:44:25 +0200 Subject: [PATCH 03/15] =?UTF-8?q?fix(admin):=20rename=20plugin=20admin=20r?= =?UTF-8?q?oute=20param=20[plugin]=20=E2=86=92=20[name]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two parallel directories had Next.js refusing to start: "different slug names for the same dynamic path ('name' != 'plugin')". plugins/[name] is the canonical route — it carries the PluginDetailView and the parent /plugins/[name] detail page. The duplicate plugins/[plugin]/[slug] subdirectory was rebased in by a stale branch and made the param disagree at the same path depth. Content of plugins/[plugin]/[slug]/* was byte-equivalent to plugins/[name]/[slug]/* except for one destructure rename (\`{ plugin, slug }\` vs \`{ name, slug }\` at params destructure) and the param type. Adopted the [name]/[slug] copy. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- .../plugins/{[plugin] => [name]}/[slug]/PluginPageBridge.tsx | 0 .../plugins/{[plugin] => [name]}/[slug]/page.tsx | 4 ++-- 2 files changed, 2 insertions(+), 2 deletions(-) rename apps/admin/src/app/(authenticated)/plugins/{[plugin] => [name]}/[slug]/PluginPageBridge.tsx (100%) rename apps/admin/src/app/(authenticated)/plugins/{[plugin] => [name]}/[slug]/page.tsx (91%) diff --git a/apps/admin/src/app/(authenticated)/plugins/[plugin]/[slug]/PluginPageBridge.tsx b/apps/admin/src/app/(authenticated)/plugins/[name]/[slug]/PluginPageBridge.tsx similarity index 100% rename from apps/admin/src/app/(authenticated)/plugins/[plugin]/[slug]/PluginPageBridge.tsx rename to apps/admin/src/app/(authenticated)/plugins/[name]/[slug]/PluginPageBridge.tsx diff --git a/apps/admin/src/app/(authenticated)/plugins/[plugin]/[slug]/page.tsx b/apps/admin/src/app/(authenticated)/plugins/[name]/[slug]/page.tsx similarity index 91% rename from apps/admin/src/app/(authenticated)/plugins/[plugin]/[slug]/page.tsx rename to apps/admin/src/app/(authenticated)/plugins/[name]/[slug]/page.tsx index f14af35e..7e5f51d4 100644 --- a/apps/admin/src/app/(authenticated)/plugins/[plugin]/[slug]/page.tsx +++ b/apps/admin/src/app/(authenticated)/plugins/[name]/[slug]/page.tsx @@ -17,12 +17,12 @@ import type { ReactElement } from 'react'; import { PluginPageBridge } from './PluginPageBridge'; interface Props { - params: Promise<{ plugin: string; slug: string }>; + params: Promise<{ name: string; slug: string }>; } export const dynamic = 'force-dynamic'; export default async function PluginAdminPage({ params }: Props): Promise { - const { plugin, slug } = await params; + const { name: plugin, slug } = await params; return ; } From 2aa9ffb90b85a171113fdf01adc3042b7b8ac984 Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:44:38 +0200 Subject: [PATCH 04/15] fix(admin): tolerate nil-slice JSON in media + folder listings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The API used to emit \`data: null\` for empty pgx result sets, which crashed admin Client Components reading \`items.length\` or \`[...items, ...res.data]\`. Defensive coerce on three surfaces: - media/page.tsx server prefetch (initialData) - MediaGrid.tsx fetchPage (subsequent pages) - FolderTree.tsx listCollections refresh The root cause is fixed at the API layer in a separate commit (Page[T].MarshalJSON nil → []), but keep these guards as belt-and-braces for any non-Page[T] endpoint that returns the same nullable shape. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- .../(authenticated)/media/components/FolderTree.tsx | 5 ++++- .../(authenticated)/media/components/MediaGrid.tsx | 6 +++++- apps/admin/src/app/(authenticated)/media/page.tsx | 12 +++++++++--- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/apps/admin/src/app/(authenticated)/media/components/FolderTree.tsx b/apps/admin/src/app/(authenticated)/media/components/FolderTree.tsx index 7b5599ae..baada9de 100644 --- a/apps/admin/src/app/(authenticated)/media/components/FolderTree.tsx +++ b/apps/admin/src/app/(authenticated)/media/components/FolderTree.tsx @@ -106,7 +106,10 @@ export function FolderTree(props: FolderTreeProps): ReactElement { setLoading(true); try { const res = await listCollections(); - setCollections(res.data); + // API returns `data: null` for an empty collections list (the + // pgx nil-slice JSON round-trip). Coerce to [] so downstream + // `collections.length` reads don't throw. + setCollections(Array.isArray(res.data) ? res.data : []); setError(null); } catch (err) { setError(err instanceof Error ? err.message : 'failed to load folders'); diff --git a/apps/admin/src/app/(authenticated)/media/components/MediaGrid.tsx b/apps/admin/src/app/(authenticated)/media/components/MediaGrid.tsx index 931f49e6..76b1b032 100644 --- a/apps/admin/src/app/(authenticated)/media/components/MediaGrid.tsx +++ b/apps/admin/src/app/(authenticated)/media/components/MediaGrid.tsx @@ -162,7 +162,11 @@ export function MediaGrid(props: MediaGridProps): ReactElement { cursor: opts.nextCursor || undefined, collection: collectionParam, }); - setItems((prev) => (opts.reset ? res.data : [...prev, ...res.data])); + // API may return `data: null` on an empty page (pgx nil + // slice → JSON null round-trip). Normalize to [] so the + // spread + length reads downstream don't throw. + const safeData = Array.isArray(res.data) ? res.data : []; + setItems((prev) => (opts.reset ? safeData : [...prev, ...safeData])); setCursor(res.pagination.next_cursor); setHydrated(true); } catch (err) { diff --git a/apps/admin/src/app/(authenticated)/media/page.tsx b/apps/admin/src/app/(authenticated)/media/page.tsx index a1596f55..64a5aa4b 100644 --- a/apps/admin/src/app/(authenticated)/media/page.tsx +++ b/apps/admin/src/app/(authenticated)/media/page.tsx @@ -34,7 +34,13 @@ async function fetchInitial(): Promise { export default async function MediaPage(): Promise { const initial = await fetchInitial(); - return ( - - ); + // The admin API returns `data: null` for an empty media library + // (a Postgres NULL → Go nil-slice → JSON null round-trip). The + // island assumes `data` is always an array — calling + // `.length` on null throws "Cannot read properties of null". Coerce + // null → [] here so the client never sees the nullable shape. + const safe = initial + ? { ...initial, data: Array.isArray(initial.data) ? initial.data : [] } + : { data: [], pagination: { next_cursor: '' } }; + return ; } From 5b62f70ed4966fcbfbe67ac93db48646b139f31d Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:44:54 +0200 Subject: [PATCH 05/15] fix(admin+api): posts list shape adapter + status=any alias + dead-link fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related fixes around the posts list surface: 1. Admin posts page.tsx now adapts the API envelope (\`{data, pagination}\`) to the flatter shape PostListClient expects (\`{posts, nextCursor, total}\`) — and maps \`published_at|updated_at\` onto the UI's single \`date\` field. (#515 will replace this with generated api-types.) 2. \`status=any\` is the admin's documented "all statuses" sentinel. Aliased to empty in both rest/posts/handlers.go and admin/comments/list.go so the chips don't 400. (#516 tracks the missing alias in admin/search.) 3. \`postEditHref\` returns \`/posts/\${id}\` (the actual route) — not \`/posts/\${id}/edit\` (404s). Propagated to two dead-linking sites in the comments table that were missed in the original rename (#504). Updated PostListClient.test.tsx assertion. Added a regression test in CommentListClient.test.tsx. 4. Escaped two stray \`"\` characters in posts/[id]/page.tsx:237 (react/no-unescaped-entities ESLint error). Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- .../comments/CommentListClient.test.tsx | 11 ++++ .../comments/CommentListClient.tsx | 3 +- .../(authenticated)/comments/[id]/page.tsx | 3 +- .../posts/PostListClient.test.tsx | 6 +-- .../app/(authenticated)/posts/[id]/page.tsx | 2 +- .../src/app/(authenticated)/posts/columns.tsx | 8 +-- .../src/app/(authenticated)/posts/page.tsx | 50 ++++++++++++++++--- apps/api/internal/admin/comments/list.go | 10 ++-- apps/api/internal/rest/posts/handlers.go | 7 +++ 9 files changed, 80 insertions(+), 20 deletions(-) diff --git a/apps/admin/src/app/(authenticated)/comments/CommentListClient.test.tsx b/apps/admin/src/app/(authenticated)/comments/CommentListClient.test.tsx index edb6a9a5..a99df37d 100644 --- a/apps/admin/src/app/(authenticated)/comments/CommentListClient.test.tsx +++ b/apps/admin/src/app/(authenticated)/comments/CommentListClient.test.tsx @@ -240,6 +240,17 @@ describe('CommentListClient', () => { expect(screen.getByText(/thanks for sharing/i)).toBeInTheDocument(); }); + it('links to the canonical post route without /edit suffix', () => { + render(); + // The post-title link in the "On post" column should point to + // `/posts/` — never `/posts//edit` (the old route 404s). + const postLink = screen.getAllByRole('link', { name: /hello world/i })[0]; + expect(postLink).toBeDefined(); + const href = postLink!.getAttribute('href') ?? ''; + expect(href).not.toMatch(/\/edit($|[?#])/); + expect(href).toContain('/posts/p1'); + }); + it('all-rows checkbox selects every row', () => { render(); const selectAll = screen.getByLabelText(/select all comments/i); diff --git a/apps/admin/src/app/(authenticated)/comments/CommentListClient.tsx b/apps/admin/src/app/(authenticated)/comments/CommentListClient.tsx index 7f8b9b4d..90c4b286 100644 --- a/apps/admin/src/app/(authenticated)/comments/CommentListClient.tsx +++ b/apps/admin/src/app/(authenticated)/comments/CommentListClient.tsx @@ -39,6 +39,7 @@ import { Button } from '@/components/ui/button'; import { api, ApiError } from '@/lib/api-client'; import { cn } from '@/lib/utils'; +import { postEditHref } from '../posts/columns'; import { BulkActionBar } from './components/BulkActionBar'; import { StatusBadge } from './components/StatusBadge'; import { @@ -449,7 +450,7 @@ export function CommentListClient({ {c.postTitle || '(untitled)'} diff --git a/apps/admin/src/app/(authenticated)/comments/[id]/page.tsx b/apps/admin/src/app/(authenticated)/comments/[id]/page.tsx index 5edeb90b..604539ae 100644 --- a/apps/admin/src/app/(authenticated)/comments/[id]/page.tsx +++ b/apps/admin/src/app/(authenticated)/comments/[id]/page.tsx @@ -23,6 +23,7 @@ import { Headline } from '@/components/ui/headline'; import { serverApiFetch } from '@/lib/server-api'; import { cn } from '@/lib/utils'; +import { postEditHref } from '../../posts/columns'; import { StatusBadge } from '../components/StatusBadge'; import { toComment, @@ -175,7 +176,7 @@ export default async function CommentDetailPage( on{' '} {target.postTitle || '(untitled)'} diff --git a/apps/admin/src/app/(authenticated)/posts/PostListClient.test.tsx b/apps/admin/src/app/(authenticated)/posts/PostListClient.test.tsx index 9be9c25a..9665ede4 100644 --- a/apps/admin/src/app/(authenticated)/posts/PostListClient.test.tsx +++ b/apps/admin/src/app/(authenticated)/posts/PostListClient.test.tsx @@ -93,13 +93,13 @@ describe('PostListClient', () => { // Title links are the user-facing handle to each row. expect( screen.getByRole('link', { name: /hello world/i }), - ).toHaveAttribute('href', '/posts/p1/edit'); + ).toHaveAttribute('href', '/posts/p1'); expect( screen.getByRole('link', { name: /draft notes/i }), - ).toHaveAttribute('href', '/posts/p2/edit'); + ).toHaveAttribute('href', '/posts/p2'); expect( screen.getByRole('link', { name: /trashed item/i }), - ).toHaveAttribute('href', '/posts/p3/edit'); + ).toHaveAttribute('href', '/posts/p3'); // Status badges round-trip from the post.status field. expect(screen.getByLabelText('Status: Published')).toBeInTheDocument(); diff --git a/apps/admin/src/app/(authenticated)/posts/[id]/page.tsx b/apps/admin/src/app/(authenticated)/posts/[id]/page.tsx index fff4a1e9..b61f3753 100644 --- a/apps/admin/src/app/(authenticated)/posts/[id]/page.tsx +++ b/apps/admin/src/app/(authenticated)/posts/[id]/page.tsx @@ -234,7 +234,7 @@ export default function PostDetailPage(): ReactElement {

Compose your post by adding blocks below. Changes are - autosaved every 30 seconds; "Save changes" promotes the + autosaved every 30 seconds; “Save changes” promotes the autosave to the canonical row.

diff --git a/apps/admin/src/app/(authenticated)/posts/columns.tsx b/apps/admin/src/app/(authenticated)/posts/columns.tsx index 68a5195c..71046362 100644 --- a/apps/admin/src/app/(authenticated)/posts/columns.tsx +++ b/apps/admin/src/app/(authenticated)/posts/columns.tsx @@ -134,11 +134,13 @@ export function formatDate(iso: string): string { } /** - * Build the `/posts/{id}/edit` href used by the title cell. Centralised so - * a route move only touches one file. + * Build the `/posts/{id}` href used by the title cell. The single-id + * route IS the editor — the [id]/page.tsx renders the block editor + * directly. A previous version of this file appended `/edit` to the + * path; that path 404s because no nested route exists. */ export function postEditHref(id: string): string { - return `/posts/${encodeURIComponent(id)}/edit`; + return `/posts/${encodeURIComponent(id)}`; } /** Column id, header label, and whether it's sortable. */ diff --git a/apps/admin/src/app/(authenticated)/posts/page.tsx b/apps/admin/src/app/(authenticated)/posts/page.tsx index fa9cb178..7e4eb971 100644 --- a/apps/admin/src/app/(authenticated)/posts/page.tsx +++ b/apps/admin/src/app/(authenticated)/posts/page.tsx @@ -96,14 +96,52 @@ async function fetchInitialPosts(): Promise<{ error: `HTTP ${res.status}`, }; } - const json = (await res.json()) as PostListResponse; - // Be defensive: the API contract is still evolving (issue #76) so - // missing fields shouldn't crash the page. + // The API returns the envelope shape `{data: [...], pagination: {...}}` + // — see rest/posts/handlers.go list(). The page UI works in + // `{posts, nextCursor, total}` form with a flatter Post shape, so + // adapt here. Be defensive: a missing field shouldn't crash the + // page (issue #76 — contract still evolving). + type ApiPost = { + id: string; + title: string; + status: string; + published_at?: string | null; + updated_at?: string; + created_at?: string; + author_id?: string; + }; + type ApiEnvelope = { + data?: ApiPost[]; + posts?: ApiPost[]; + pagination?: { next_cursor?: string; nextCursor?: string }; + nextCursor?: string; + total?: number; + }; + const json = (await res.json()) as ApiEnvelope; + const rows = Array.isArray(json.data) + ? json.data + : Array.isArray(json.posts) + ? json.posts + : []; + const cursor = + json.pagination?.next_cursor ?? + json.pagination?.nextCursor ?? + json.nextCursor ?? + null; + // Map ApiPost → the flatter Post shape the UI expects. + const posts = rows.map((p) => ({ + id: p.id, + title: p.title ?? '(untitled)', + status: (p.status as PostListResponse['posts'][number]['status']) ?? 'draft', + date: p.published_at ?? p.updated_at ?? p.created_at ?? '', + author: { id: p.author_id ?? '', displayName: '' }, + commentsCount: 0, + })); return { data: { - posts: Array.isArray(json.posts) ? json.posts : [], - nextCursor: json.nextCursor ?? null, - total: typeof json.total === 'number' ? json.total : 0, + posts: posts as PostListResponse['posts'], + nextCursor: cursor, + total: typeof json.total === 'number' ? json.total : posts.length, }, error: null, }; diff --git a/apps/api/internal/admin/comments/list.go b/apps/api/internal/admin/comments/list.go index d804aa34..b913fb60 100644 --- a/apps/api/internal/admin/comments/list.go +++ b/apps/api/internal/admin/comments/list.go @@ -32,11 +32,11 @@ func (h *handlers) list(w http.ResponseWriter, r *http.Request, _ policy.Princip var filter ListFilter - // Status filter. Empty string means "no filter"; any other - // value must be in AllStatuses or we 400 so the client doesn't - // accidentally typo "approve" (the bulk verb) instead of - // "approved" (the state). - if s := q.Get("status"); s != "" { + // Status filter. Empty string and the literal "any" both mean + // "no filter"; any other value must be in AllStatuses or we 400 + // so the client doesn't accidentally typo "approve" (the bulk + // verb) instead of "approved" (the state). + if s := q.Get("status"); s != "" && s != "any" { st := Status(s) if !IsValidStatus(st) { router.WriteError(w, http.StatusBadRequest, "invalid_status", diff --git a/apps/api/internal/rest/posts/handlers.go b/apps/api/internal/rest/posts/handlers.go index 0256294a..bc4f9d3a 100644 --- a/apps/api/internal/rest/posts/handlers.go +++ b/apps/api/internal/rest/posts/handlers.go @@ -156,6 +156,13 @@ func parseListQuery(r *http.Request) (ListFilter, error) { q := r.URL.Query() var f ListFilter f.Status = q.Get("status") + // `status=any` is a documented client convention meaning "all + // statuses" — the admin posts page uses it to render drafts + + // published in one list. Treat it as the absence of a status + // filter rather than a validation error. Empty string is the same. + if f.Status == "any" { + f.Status = "" + } if f.Status != "" { if _, ok := validStatuses[f.Status]; !ok { return f, validation{Code: "invalid_status", Detail: fmt.Sprintf("unknown status %q", f.Status)} From bf4ffd2b4ebb3a6ca733832c03a64de8aa7aa00b Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:45:10 +0200 Subject: [PATCH 06/15] fix(admin): View site link, disable broken pages editor, customizer cookie forwarding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small admin polish fixes: - TopHeader \`View site\` now opens NEXT_PUBLIC_SITE_URL (or localhost:3000 fallback) in a new tab, instead of doing nothing (was \`href=\"/\"\` which routed back to the admin home). - /pages/[id] block-editor button is disabled with a \"coming soon\" label. The previous Link pointed at /pages/[id]/edit which 404s — the dedicated block editor for pages was never built. Track full Pages module work in #506. - /appearance/customizer is a Server Component but its \`fetchActive\` called the client-side \`api.get\` which sends \`credentials: 'include'\` (a no-op on Node.js). Cookies never reached the API → 401 → \"customizer unavailable\". Split: client surface stays in api.ts (for the save/reset mutations from the client form), new api-server.ts exports \`fetchActiveServer\` using serverApiFetch which forwards cookies via next/headers. page.tsx now imports the server variant. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- .../(authenticated)/_components/TopHeader.tsx | 25 ++++++++++--- .../appearance/customizer/api-server.ts | 35 +++++++++++++++++++ .../appearance/customizer/api.ts | 5 +++ .../appearance/customizer/page.tsx | 4 +-- .../app/(authenticated)/pages/[id]/page.tsx | 10 +++--- 5 files changed, 69 insertions(+), 10 deletions(-) create mode 100644 apps/admin/src/app/(authenticated)/appearance/customizer/api-server.ts diff --git a/apps/admin/src/app/(authenticated)/_components/TopHeader.tsx b/apps/admin/src/app/(authenticated)/_components/TopHeader.tsx index 60dd841d..9a142348 100644 --- a/apps/admin/src/app/(authenticated)/_components/TopHeader.tsx +++ b/apps/admin/src/app/(authenticated)/_components/TopHeader.tsx @@ -19,6 +19,21 @@ import type { ReactElement } from 'react'; import Link from 'next/link'; import { Bell, ExternalLink } from 'lucide-react'; +/** + * Resolve the public-site URL for the "View site" link. NEXT_PUBLIC_SITE_URL + * is the operator-set canonical front-end origin (production: the marketing + * site). When unset we fall back to the docker-compose web service on + * port 3000 — operators running the local stack expect that to open the + * @gonext/web preview, not the admin's own root. + * + * The constant is evaluated at module-load time so the link href is + * deterministic across renders. + */ +const SITE_URL = + process.env.NEXT_PUBLIC_SITE_URL && process.env.NEXT_PUBLIC_SITE_URL !== '' + ? process.env.NEXT_PUBLIC_SITE_URL + : 'http://localhost:3000'; + export function TopHeader(): ReactElement { return (
@@ -28,14 +43,16 @@ export function TopHeader(): ReactElement { Admin
-
From f9f4ecd09ae110eda694f353c9b9aaf608e8bdeb Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:45:23 +0200 Subject: [PATCH 07/15] fix(api): coerce nil Data to empty slice in router.Page[T] JSON (#505) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit \`json.Marshal(Page[T]{Data: nil})\` emits \`\"data\":null\`. Admin Client Components then \`.map\` / spread / \`.length\` on null and crash. Confirmed leaks: /api/v1/admin/media and /api/v1/admin/webhooks both returned \`data:null\` for empty result sets. One-line MarshalJSON on Page[T] coerces nil → \`[]T{}\` before encoding, using an inner-struct alias trick to avoid recursing into its own MarshalJSON. Unblocks 8+ admin Client Components that were defensively guarding with \`Array.isArray(x.data) ? x.data : []\` — those guards remain as belt-and-braces but the API stops emitting the landmine. Two tests: nil Data marshals as \`[]\`; non-nil Data round-trips correctly with cursor pagination preserved. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- apps/api/internal/rest/router/cursor.go | 21 +++++++++++++ apps/api/internal/rest/router/cursor_test.go | 31 ++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/apps/api/internal/rest/router/cursor.go b/apps/api/internal/rest/router/cursor.go index 4599df69..8aefe77e 100644 --- a/apps/api/internal/rest/router/cursor.go +++ b/apps/api/internal/rest/router/cursor.go @@ -2,6 +2,7 @@ package router import ( "encoding/base64" + "encoding/json" "errors" ) @@ -64,3 +65,23 @@ type Page[T any] struct { Data []T `json:"data"` Pagination PageInfo `json:"pagination"` } + +// MarshalJSON coerces a nil Data slice to an empty slice so the wire +// form is always `"data": []`, never `"data": null`. Admin Client +// Components map/spread/.length over `.data` without nil guards — a +// JSON null crashes them with "Cannot read properties of null". The +// inner pageOut struct (rather than `type pageAlias Page[T]`) is what +// avoids recursion: a generics-aware alias still carries the +// MarshalJSON method through the type parameter, so json.Marshal would +// loop forever. The inner struct has no methods and marshals via the +// default reflection path. +func (p Page[T]) MarshalJSON() ([]byte, error) { + if p.Data == nil { + p.Data = []T{} + } + type pageOut struct { + Data []T `json:"data"` + Pagination PageInfo `json:"pagination"` + } + return json.Marshal(pageOut{Data: p.Data, Pagination: p.Pagination}) +} diff --git a/apps/api/internal/rest/router/cursor_test.go b/apps/api/internal/rest/router/cursor_test.go index f0fa90e0..e262ebdc 100644 --- a/apps/api/internal/rest/router/cursor_test.go +++ b/apps/api/internal/rest/router/cursor_test.go @@ -1,7 +1,9 @@ package router import ( + "encoding/json" "errors" + "strings" "testing" ) @@ -68,3 +70,32 @@ func TestEncodeCursor_NoPadding(t *testing.T) { } } } + +func TestPage_MarshalJSON_NilDataBecomesEmptyArray(t *testing.T) { + t.Parallel() + + p := Page[string]{Data: nil, Pagination: PageInfo{}} + out, err := json.Marshal(p) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(out), `"data":[]`) { + t.Errorf("expected data:[], got: %s", string(out)) + } + if strings.Contains(string(out), `"data":null`) { + t.Errorf("expected no null, got: %s", string(out)) + } +} + +func TestPage_MarshalJSON_NonNilDataPreserved(t *testing.T) { + t.Parallel() + + p := Page[string]{Data: []string{"a", "b"}, Pagination: PageInfo{}} + out, err := json.Marshal(p) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(out), `"data":["a","b"]`) { + t.Errorf("expected data:[a,b], got: %s", string(out)) + } +} From bc0a13a5cee27e07b092702a8ea1ef18815d0668 Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:45:47 +0200 Subject: [PATCH 08/15] fix(api): seed user roles into session on login, incl. TOTP finalize (#496) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sessions.Create was called with \`data: nil\` regardless of the user's role grants. Even though \`gonext init\` writes \`{\"roles\": [\"super_admin\"]}\` into users.meta, the session principal had no roles, so every capability check 403'd — including for the bootstrap super_admin. - UserRecord gets a Roles []string field, projected from users.meta.roles via \`COALESCE(u.meta->'roles', '[]'::jsonb)\` and JSON-decoded in the SQL adapter. - login.UserByIDLookup type added for the TOTP finalize path which only has userID (recovered from the intermediate token), not email. - adapters.go grows userLookupByID(pool) mirroring the by-email variant. - main.go wires UserByID into the login.Deps literal. - service.go threads user.Roles through completeLogin in BOTH paths: the password-only path AND finalizeTOTP's two completion sites. TOTP path re-fetches via UserByID; nil-safe degradation (no roles) if the lookup fails for any reason. Without this, a super_admin who enrolled in TOTP would lose admin permanently on next sign-in. Tests: TestFinalizeTOTP_ThreadsRolesFromUserByID asserts roles flow through the data map. TestFinalizeTOTP_NilUserByIDDoesNotPanic confirms the zero-Deps fallback. Existing tests unchanged. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- apps/api/cmd/server/adapters.go | 70 ++++++++- apps/api/cmd/server/main.go | 1 + apps/api/internal/auth/login/deps.go | 21 +++ apps/api/internal/auth/login/service.go | 56 ++++++- apps/api/internal/auth/login/service_test.go | 145 ++++++++++++++++++- 5 files changed, 283 insertions(+), 10 deletions(-) diff --git a/apps/api/cmd/server/adapters.go b/apps/api/cmd/server/adapters.go index d181e50a..8d31e128 100644 --- a/apps/api/cmd/server/adapters.go +++ b/apps/api/cmd/server/adapters.go @@ -10,6 +10,7 @@ package main import ( "context" + "encoding/json" "errors" "fmt" "strings" @@ -85,13 +86,25 @@ func userLookupByEmail(pool *pgxpool.Pool) login.UserLookup { } var rec login.UserRecord var hash *string + // users.meta.roles is the canonical role-grant source (see + // cli/gonext/cmd/init/admin.go which seeds the bootstrap + // super_admin there). We project it as a JSON array of text + // using `meta->'roles'` and let pgx scan it into a + // []string via a JSON-aware destination. The COALESCE keeps + // older rows (or future OAuth-only signups) with a null + // roles array from blowing up the scan. + var rolesJSON []byte err := pool.QueryRow(ctx, ` - SELECT u.id::text, u.email::text, p.password_hash, u.status + SELECT u.id::text, + u.email::text, + p.password_hash, + u.status, + COALESCE(u.meta->'roles', '[]'::jsonb) FROM users u LEFT JOIN user_passwords p ON p.user_id = u.id WHERE u.email = $1::citext LIMIT 1 - `, email).Scan(&rec.ID, &rec.Email, &hash, &rec.Status) + `, email).Scan(&rec.ID, &rec.Email, &hash, &rec.Status, &rolesJSON) if err != nil { if errors.Is(err, pgx.ErrNoRows) { return login.UserRecord{}, login.ErrUserNotFound @@ -101,6 +114,59 @@ func userLookupByEmail(pool *pgxpool.Pool) login.UserLookup { if hash != nil { rec.Hash = *hash } + // Decode roles. A non-array or unmarshal error degrades to + // "no roles" rather than failing login — the audit log + // already captures the user_id, so an operator can spot a + // bad meta payload from the privilege-less behavior. + if len(rolesJSON) > 0 { + var roles []string + if jsonErr := json.Unmarshal(rolesJSON, &roles); jsonErr == nil { + rec.Roles = roles + } + } + return rec, nil + } +} + +// userLookupByID is the by-ID variant of userLookupByEmail. Used by the +// TOTP finalize path. Same projection (status + meta.roles) so the +// principal carried into the session matches what the password path +// would produce. +func userLookupByID(pool *pgxpool.Pool) login.UserByIDLookup { + return func(ctx context.Context, userID string) (login.UserRecord, error) { + userID = strings.TrimSpace(userID) + if userID == "" { + return login.UserRecord{}, login.ErrUserNotFound + } + var rec login.UserRecord + var hash *string + var rolesJSON []byte + err := pool.QueryRow(ctx, ` + SELECT u.id::text, + u.email::text, + p.password_hash, + u.status, + COALESCE(u.meta->'roles', '[]'::jsonb) + FROM users u + LEFT JOIN user_passwords p ON p.user_id = u.id + WHERE u.id = $1::uuid + LIMIT 1 + `, userID).Scan(&rec.ID, &rec.Email, &hash, &rec.Status, &rolesJSON) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return login.UserRecord{}, login.ErrUserNotFound + } + return login.UserRecord{}, fmt.Errorf("login: user lookup by id: %w", err) + } + if hash != nil { + rec.Hash = *hash + } + if len(rolesJSON) > 0 { + var roles []string + if jsonErr := json.Unmarshal(rolesJSON, &roles); jsonErr == nil { + rec.Roles = roles + } + } return rec, nil } } diff --git a/apps/api/cmd/server/main.go b/apps/api/cmd/server/main.go index 2fd3f727..db9321b6 100644 --- a/apps/api/cmd/server/main.go +++ b/apps/api/cmd/server/main.go @@ -886,6 +886,7 @@ func buildRouter(cfg *config.Config, pool *pgxpool.Pool, rdb *goredis.Client, se } else { if err := login.Mount(mux, login.Deps{ Lookup: userLookupByEmail(pool), + UserByID: userLookupByID(pool), Sessions: sessions, Pepper: []byte(cfg.Auth.Pepper), SessionAbsoluteTTL: cfg.Auth.SessionTTL, diff --git a/apps/api/internal/auth/login/deps.go b/apps/api/internal/auth/login/deps.go index 3bca2f21..24cd5783 100644 --- a/apps/api/internal/auth/login/deps.go +++ b/apps/api/internal/auth/login/deps.go @@ -28,6 +28,13 @@ type UserRecord struct { // pass; suspended / deleted return ErrInvalidCredentials so the // admin-side decision isn't leaked to a stranger. Status string + + // Roles are the role slugs assigned to this user, projected from + // users.meta.roles by the SQL lookup. The service stamps these + // into the session's data map so RequireSession-derived principals + // carry the role grant. Empty slice → anonymous-equivalent + // authorization (login succeeds but every capability check fails). + Roles []string } // TOTPRecord describes a user's TOTP enrolment. SecretBase32 is the @@ -66,6 +73,13 @@ var ErrTOTPNotEnabled = errors.New("login: TOTP not enabled") // fakes need to lower-case both sides. type UserLookup func(ctx context.Context, email string) (UserRecord, error) +// UserByIDLookup returns the user record for an already-authenticated +// userID. Used by the TOTP finalize path which only has the userID +// (recovered from the intermediate token), not an email. Implementations +// MUST be case-equivalent to UserLookup: same source of truth, same +// role projection. +type UserByIDLookup func(ctx context.Context, userID string) (UserRecord, error) + // TOTPLookup returns the TOTP enrolment for a user, or ErrTOTPNotEnabled. // If the underlying user_totp table doesn't exist yet (the migration // ships in a separate PR), implementations should return @@ -120,6 +134,13 @@ type Deps struct { // Lookup performs the email -> UserRecord query. Required. Lookup UserLookup + // UserByID performs the userID -> UserRecord query. Used by the + // TOTP finalize path so the post-2FA session carries the same + // roles the no-2FA path would. May be nil — when nil, the + // finalize path degrades to a session with no roles (same + // behavior as before this field landed). + UserByID UserByIDLookup + // TOTPLookup performs the user -> TOTPRecord query. May be nil // when 2FA isn't wired in this deployment. TOTPLookup TOTPLookup diff --git a/apps/api/internal/auth/login/service.go b/apps/api/internal/auth/login/service.go index 255381b8..d9d660c2 100644 --- a/apps/api/internal/auth/login/service.go +++ b/apps/api/internal/auth/login/service.go @@ -235,7 +235,7 @@ func (s *Service) Authenticate(ctx context.Context, in Input) (Result, error) { switch { case !hasTOTP: // 2FA disabled or not wired — straight to session creation. - return s.completeLogin(ctx, in, user.ID) + return s.completeLogin(ctx, in, user.ID, user.Roles) case in.TOTPCode != "" || in.RecoveryCode != "": // First-call body carried a code. Verify it inline. @@ -248,7 +248,7 @@ func (s *Service) Authenticate(ctx context.Context, in Input) (Result, error) { s.emitFailed(ctx, in, user.ID, "wrong_totp") return Result{}, ErrTOTPInvalid } - return s.completeLogin(ctx, in, user.ID) + return s.completeLogin(ctx, in, user.ID, user.Roles) default: // 2FA required but code not provided. Issue an intermediate @@ -292,7 +292,13 @@ func (s *Service) finalizeTOTP(ctx context.Context, in Input) (Result, error) { // now (operator disabled it mid-flow). Drop the token and // finish the login — there's nothing further to verify. _ = s.deps.Intermediate.Delete(ctx, in.IntermediateToken) - return s.completeLogin(ctx, in, userID) + // Re-fetch the user to recover Roles (the intermediate token + // carries only the userID, not the role projection). If the + // lookup is unwired or fails, completeLogin still proceeds + // with nil roles — same degradation as before this fix, but + // no longer the default-every-time behavior. + roles := s.lookupRolesByID(ctx, userID) + return s.completeLogin(ctx, in, userID, roles) } if !s.verifyTOTPOrRecovery(in, tot) { @@ -313,14 +319,52 @@ func (s *Service) finalizeTOTP(ctx context.Context, in Input) (Result, error) { // can't be replayed. _ = s.deps.Intermediate.Delete(ctx, in.IntermediateToken) - return s.completeLogin(ctx, in, userID) + // Re-fetch the user record so the post-2FA session carries the + // same Roles the no-2FA path stamps in. Without this the user's + // role grants are dropped on every TOTP-gated login (issue #496). + // If UserByID is nil or fails, we keep going with nil roles — + // same fallback as the previous code, just no longer on the + // happy path. + roles := s.lookupRolesByID(ctx, userID) + return s.completeLogin(ctx, in, userID, roles) +} + +// lookupRolesByID re-fetches the user's role projection via +// Deps.UserByID. It exists so finalizeTOTP can recover the role grants +// that the intermediate token doesn't carry. Errors and nil deps are +// folded into "no roles" — completeLogin treats an empty slice the +// same way it would before this fix, so a missing UserByID wiring +// degrades to the pre-#496 behavior (login succeeds, capability +// checks return 403) rather than panicking. +func (s *Service) lookupRolesByID(ctx context.Context, userID string) []string { + if s.deps.UserByID == nil { + return nil + } + user, err := s.deps.UserByID(ctx, userID) + if err != nil { + s.deps.Log.WarnContext(ctx, "login: UserByID lookup error (finalizeTOTP)", + slog.String("user_id", userID), + slog.String("err", err.Error())) + return nil + } + return user.Roles } // completeLogin mints the session, fires the audit success event, and // clears the failure counter. Used by both the no-2FA branch and the // post-2FA finalization. -func (s *Service) completeLogin(ctx context.Context, in Input, userID string) (Result, error) { - token, err := s.deps.Sessions.Create(ctx, userID, nil, s.deps.SessionAbsoluteTTL, s.deps.SessionIdleTTL) +// +// rolesFromLookup is the role slugs from the UserRecord. We stamp them +// into the session's data map under the "roles" key so the auth +// middleware's DefaultPrincipal derives a Principal with role grants +// — without this every capability check returns 403 "principal has +// no roles" even for the super_admin seeded by `gonext init`. +func (s *Service) completeLogin(ctx context.Context, in Input, userID string, rolesFromLookup []string) (Result, error) { + var data map[string]any + if len(rolesFromLookup) > 0 { + data = map[string]any{"roles": rolesFromLookup} + } + token, err := s.deps.Sessions.Create(ctx, userID, data, s.deps.SessionAbsoluteTTL, s.deps.SessionIdleTTL) if err != nil { s.deps.Log.WarnContext(ctx, "login: session create failed", slog.String("user_id", userID), diff --git a/apps/api/internal/auth/login/service_test.go b/apps/api/internal/auth/login/service_test.go index d317ea80..c6118935 100644 --- a/apps/api/internal/auth/login/service_test.go +++ b/apps/api/internal/auth/login/service_test.go @@ -46,18 +46,19 @@ type fakeSession struct { type fakeSessionRecord struct { userID string + data map[string]any ttl time.Duration idleTTL time.Duration } -func (f *fakeSession) Create(_ context.Context, userID string, _ map[string]any, ttl, idleTTL time.Duration) (string, error) { +func (f *fakeSession) Create(_ context.Context, userID string, data map[string]any, ttl, idleTTL time.Duration) (string, error) { f.mu.Lock() defer f.mu.Unlock() if f.err != nil { return "", f.err } f.nextID++ - f.created = append(f.created, fakeSessionRecord{userID: userID, ttl: ttl, idleTTL: idleTTL}) + f.created = append(f.created, fakeSessionRecord{userID: userID, data: data, ttl: ttl, idleTTL: idleTTL}) // Mint a token shaped like the real one (32 bytes base64url, 43 chars), // padded with a counter for uniqueness. tok := "fake-token-0000000000000000000000000000000-" + string(rune('A'+f.nextID%26)) @@ -70,6 +71,18 @@ func (f *fakeSession) creates() int { return len(f.created) } +// lastData returns the data map handed to the most recent Create call. +// Tests use it to assert what the session was stamped with (e.g. the +// "roles" key carried through from the user record). +func (f *fakeSession) lastData() map[string]any { + f.mu.Lock() + defer f.mu.Unlock() + if len(f.created) == 0 { + return nil + } + return f.created[len(f.created)-1].data +} + // newFixture wires a fixture with defaults that match the production // rate-limit policy from doc 06. Tests overlay specific values they // care about. @@ -779,6 +792,134 @@ func TestAuthenticate_RecordsFailureForKnownUserOnly(t *testing.T) { } } +// TestFinalizeTOTP_ThreadsRolesFromUserByID is the regression test for +// issue #496: when a user with TOTP enabled completes the two-step +// login, the post-2FA session MUST carry the user's roles. Before the +// fix, the TOTP finalize path passed nil for roles to completeLogin, +// which meant any super_admin enrolled in TOTP lost their admin grant +// on every subsequent sign-in. +func TestFinalizeTOTP_ThreadsRolesFromUserByID(t *testing.T) { + f := newFixture(t) + f.addUser(t, "u-1", "alice@example.com", "pwd", "active") + sec, err := totp.Generate("GoNext", "alice@example.com") + if err != nil { + t.Fatalf("totp.Generate: %v", err) + } + f.addTOTP(t, "u-1", sec.Base32) + + // Wire UserByID to return a record with a role grant. The fix + // re-fetches via this lookup at finalize time so the role slice + // reaches completeLogin → Sessions.Create. + f.deps.UserByID = func(_ context.Context, userID string) (UserRecord, error) { + if userID != "u-1" { + return UserRecord{}, ErrUserNotFound + } + return UserRecord{ + ID: "u-1", + Email: "alice@example.com", + Status: "active", + Roles: []string{"super_admin"}, + }, nil + } + svc := f.service(t) + + // Step 1: password OK, server returns an intermediate token. + first, err := svc.Authenticate(context.Background(), Input{ + Email: "alice@example.com", + Password: "pwd", + IP: "10.0.0.1", + }) + if err != nil { + t.Fatalf("first call err: %v", err) + } + if !first.RequiresTOTP { + t.Fatal("first call: not RequiresTOTP") + } + + // Step 2: present TOTP code with the intermediate token. + code, err := generateCurrentTOTP(sec.Base32) + if err != nil { + t.Fatalf("generate code: %v", err) + } + final, err := svc.Authenticate(context.Background(), Input{ + IntermediateToken: first.IntermediateToken, + TOTPCode: code, + IP: "10.0.0.1", + }) + if err != nil { + t.Fatalf("second call err: %v", err) + } + if final.Token == "" { + t.Fatal("Token: empty after 2FA finalize") + } + + // Assert: Sessions.Create was called with data["roles"] containing + // ["super_admin"]. Before the fix this map was nil. + data := f.sessionCreator.lastData() + if data == nil { + t.Fatal("session data map is nil; expected roles to be stamped in") + } + rolesAny, ok := data["roles"] + if !ok { + t.Fatalf("session data missing 'roles' key: %#v", data) + } + roles, ok := rolesAny.([]string) + if !ok { + t.Fatalf("roles type: got %T, want []string", rolesAny) + } + if len(roles) != 1 || roles[0] != "super_admin" { + t.Errorf("roles: got %v, want [super_admin]", roles) + } +} + +// TestFinalizeTOTP_NilUserByIDDoesNotPanic confirms the documented +// fallback contract: when Deps.UserByID is unwired (zero value), the +// finalize path still completes — it just produces a session with no +// roles, the same degradation behavior as before the fix landed. This +// keeps existing tests that omit UserByID in their fixture from +// panicking on a nil indirection. +func TestFinalizeTOTP_NilUserByIDDoesNotPanic(t *testing.T) { + f := newFixture(t) + f.addUser(t, "u-1", "alice@example.com", "pwd", "active") + sec, err := totp.Generate("GoNext", "alice@example.com") + if err != nil { + t.Fatalf("totp.Generate: %v", err) + } + f.addTOTP(t, "u-1", sec.Base32) + // Leave f.deps.UserByID nil — that's the point of this test. + svc := f.service(t) + + first, err := svc.Authenticate(context.Background(), Input{ + Email: "alice@example.com", + Password: "pwd", + IP: "10.0.0.1", + }) + if err != nil || !first.RequiresTOTP { + t.Fatalf("first call: err=%v, RequiresTOTP=%v", err, first.RequiresTOTP) + } + + code, err := generateCurrentTOTP(sec.Base32) + if err != nil { + t.Fatalf("generate code: %v", err) + } + final, err := svc.Authenticate(context.Background(), Input{ + IntermediateToken: first.IntermediateToken, + TOTPCode: code, + IP: "10.0.0.1", + }) + if err != nil { + t.Fatalf("second call err: %v", err) + } + if final.Token == "" { + t.Error("Token: empty after 2FA finalize") + } + // Data map should be nil (no roles to stamp), which is the + // pre-fix degradation behavior — but the call must not panic. + if got := f.sessionCreator.lastData(); got != nil { + t.Errorf("session data: got %v, want nil (no UserByID → no roles)", got) + } +} + // generateCurrentTOTP returns the 6-digit TOTP code for the supplied // base32 secret at the current wall-clock time. Used by tests that // drive the 2FA finalize step. We delegate to pquerna/otp's From 0d0b3c48b2929ce1127228552e8c080c299f4eb7 Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:48:36 +0200 Subject: [PATCH 09/15] fix(worker): single Dispatch regardless of storage init (#497) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The worker's \`taskspec.Dispatch\` was buried inside four layers of \`else\` branches under storage.New + NewAbortOrphansSpec + SeedDefaults. If MinIO init or scheduler seed failed, the worker silently registered ZERO asynq task handlers — including the content publisher and GC that the surrounding comments promise. The boot logs still said \"worker started\" so the failure looked like a healthy worker that didn't process tasks. Enqueued tasks just dead-lettered after their retry budget. Restructure to call Dispatch ONCE, unconditionally, after every Register attempt. Failed registers logged warnings — they won't be in taskspec.Default(), so Dispatch is a no-op for them. cronReg declared at outer scope so scheduler.SeedDefaults runs even when storage failed. Double-registration panic (the original bug this code was avoiding) remains impossible: each spec is registered into Default() at most once. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- apps/worker/cmd/worker/main.go | 86 ++++++++++++++++++++-------------- apps/worker/go.mod | 5 ++ apps/worker/go.sum | 16 +++++++ 3 files changed, 71 insertions(+), 36 deletions(-) diff --git a/apps/worker/cmd/worker/main.go b/apps/worker/cmd/worker/main.go index 3b574bf8..f64e379a 100644 --- a/apps/worker/cmd/worker/main.go +++ b/apps/worker/cmd/worker/main.go @@ -259,6 +259,16 @@ func run(ctx context.Context) error { // separate cron-leader goroutine (issue #258); this main wiring // only declares the spec + schedule so the leader has something // to fire. + // + // cronReg is declared unconditionally so that downstream + // registrations (content publisher + GC via SeedDefaults) can + // still happen even if media storage init fails. We build a fresh + // registry per process — the cron package deliberately does not + // ship a Default() singleton; ownership is the binary's. A + // follow-up issue mounts the scheduler against this registry once + // the cron-leader lease (#258) lands. + cronReg := cron.NewRegistry() + mediaDriver, err := storage.New(ctx, storage.Options{ S3: storage.S3Config{ Endpoint: cfg.Storage.Endpoint, @@ -294,16 +304,7 @@ func run(ctx context.Context) error { "err", err.Error(), ) } - // Dispatch onto the asynq mux so the consumer side runs - // the handler when the leader enqueues it. - taskspec.Dispatch(srv.Mux(), reg) // Cron-side registration: declare WHEN the task fires. - // We build a fresh registry per process — the cron - // package deliberately does not ship a Default() - // singleton; ownership is the binary's. A follow-up - // issue mounts the scheduler against this registry once - // the cron-leader lease (#258) lands. - cronReg := cron.NewRegistry() cronSpec := storage.NewAbortOrphansCron() if err := cronReg.Register(cronSpec); err != nil && !errors.Is(err, cron.ErrAlreadyRegistered) { @@ -311,41 +312,54 @@ func run(ctx context.Context) error { "err", err.Error(), ) } - _ = cronReg // pinned for the scheduler hookup below logger.Info("storage abort-orphans cron registered", "task", storage.AbortOrphansTaskName, "cron", storage.AbortOrphansCronName, "schedule", storage.AbortOrphansSchedule, ) - - // Content scheduler + GC (#143). Both tasks share the - // worker's pgx pool. The publisher fires every minute - // and flips status=scheduled rows whose scheduled_for - // has elapsed; the GC fires daily at 03:30 UTC and - // hard-deletes trash older than 30 days. - // - // We register against the same cronReg as the storage - // sweep so the eventual cron-leader (#258) sees one - // merged schedule, and against taskspec.Default() so - // the asynq mux dispatch already in place handles the - // task type. - if err := scheduler.SeedDefaults(taskspec.Default(), cronReg, scheduler.SeedOptions{ - Pool: pool, - Logger: logger, - }); err != nil { - logger.Warn("scheduler seed failed", "err", err.Error()) - } else { - taskspec.Dispatch(srv.Mux(), taskspec.Default()) - logger.Info("content scheduler + gc registered", - "publisher_task", scheduler.PublisherTaskName, - "publisher_schedule", scheduler.PublisherSchedule, - "gc_task", scheduler.GCTaskName, - "gc_schedule", scheduler.GCSchedule, - ) - } } } + // Content scheduler + GC (#143). Both tasks share the worker's + // pgx pool. The publisher fires every minute and flips + // status=scheduled rows whose scheduled_for has elapsed; the GC + // fires daily at 03:30 UTC and hard-deletes trash older than + // 30 days. + // + // We register against the same cronReg as the storage sweep so + // the eventual cron-leader (#258) sees one merged schedule, and + // against taskspec.Default() so the asynq mux dispatch already in + // place handles the task type. SeedDefaults only needs pool + + // cronReg, so this runs even if media storage init failed above. + if err := scheduler.SeedDefaults(taskspec.Default(), cronReg, scheduler.SeedOptions{ + Pool: pool, + Logger: logger, + }); err != nil { + logger.Warn("scheduler seed failed", "err", err.Error()) + } else { + logger.Info("content scheduler + gc registered", + "publisher_task", scheduler.PublisherTaskName, + "publisher_schedule", scheduler.PublisherSchedule, + "gc_task", scheduler.GCTaskName, + "gc_schedule", scheduler.GCSchedule, + ) + } + + // Single Dispatch after every Register attempt. Whatever specs + // landed in taskspec.Default() — abort-orphans (if storage init + // succeeded), publisher + GC (if SeedDefaults succeeded), and + // the media tasks registered via workermedia.Register above — + // get wired onto the asynq mux here. Failed Register attempts + // already logged their warnings; their absence from the registry + // just means the mux has no handler for that task type and asynq + // will NACK it cleanly. + // + // asynq.ServeMux.Handle panics if the same task type is registered + // twice. There is exactly ONE Dispatch call in this binary, and it + // lives here — do not introduce another. + taskspec.Dispatch(srv.Mux(), taskspec.Default()) + _ = cronReg // pinned for the cron-leader scheduler hookup (#258) + logger.Info("worker started", "drain_budget", workerShutdownBudget, "redis_url_present", cfg.Redis.URL != "", diff --git a/apps/worker/go.mod b/apps/worker/go.mod index 8c1c61b5..afd5e917 100644 --- a/apps/worker/go.mod +++ b/apps/worker/go.mod @@ -14,6 +14,10 @@ require ( github.com/getsentry/sentry-go v0.43.0 // indirect github.com/go-ini/ini v1.67.0 // indirect github.com/google/uuid v1.6.0 // indirect + github.com/jackc/pgpassfile v1.0.0 // indirect + github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect + github.com/jackc/pgx/v5 v5.9.2 // indirect + github.com/jackc/puddle/v2 v2.2.2 // indirect github.com/klauspost/compress v1.18.5 // indirect github.com/klauspost/cpuid/v2 v2.2.11 // indirect github.com/klauspost/crc32 v1.3.0 // indirect @@ -39,6 +43,7 @@ require ( go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/crypto v0.52.0 // indirect golang.org/x/net v0.54.0 // indirect + golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.45.0 // indirect golang.org/x/text v0.37.0 // indirect golang.org/x/time v0.14.0 // indirect diff --git a/apps/worker/go.sum b/apps/worker/go.sum index 75c25610..26f9a39e 100644 --- a/apps/worker/go.sum +++ b/apps/worker/go.sum @@ -26,6 +26,7 @@ github.com/containerd/platforms v0.2.1 h1:zvwtM3rz2YHPQsF2CHYM8+KtB5dvhISiXh5ZpS github.com/containerd/platforms v0.2.1/go.mod h1:XHCb+2/hzowdiut9rkudds9bE5yJ7npe7dG/wG+uFPw= github.com/cpuguy83/dockercfg v0.3.2 h1:DlJTyZGBDlXqUZ2Dk2Q3xHs/FtnooJJVaad2S9GKorA= github.com/cpuguy83/dockercfg v0.3.2/go.mod h1:sugsbF4//dDlL/i+S+rtpIWp+5h0BHJHfjj5/jFyUJc= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk= @@ -60,6 +61,14 @@ github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/hibiken/asynq v0.26.0 h1:1Zxr92MlDnb1Zt/QR5g2vSCqUS03i95lUfqx5X7/wrw= github.com/hibiken/asynq v0.26.0/go.mod h1:Qk4e57bTnWDoyJ67VkchuV6VzSM9IQW2nPvAGuDyw58= +github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsIM= +github.com/jackc/pgpassfile v1.0.0/go.mod h1:CEx0iS5ambNFdcRtxPj5JhEz+xB6uRky5eyVu/W2HEg= +github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 h1:iCEnooe7UlwOQYpKFhBabPMi4aNAfoODPEFNiAnClxo= +github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761/go.mod h1:5TJZWKEWniPve33vlWYSoGYefn3gLQRzjfDlhSJ9ZKM= +github.com/jackc/pgx/v5 v5.9.2 h1:3ZhOzMWnR4yJ+RW1XImIPsD1aNSz4T4fyP7zlQb56hw= +github.com/jackc/pgx/v5 v5.9.2/go.mod h1:mal1tBGAFfLHvZzaYh77YS/eC6IX9OWbRV1QIIM0Jn4= +github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo= +github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4= github.com/klauspost/compress v1.18.5 h1:/h1gH5Ce+VWNLSWqPzOVn6XBO+vJbCNGvjoaGBFW2IE= github.com/klauspost/compress v1.18.5/go.mod h1:cwPg85FWrGar70rWktvGQj8/hthj3wpl0PGDogxkrSQ= github.com/klauspost/cpuid/v2 v2.0.1/go.mod h1:FInQzS24/EEf25PyTYn52gqo7WaD8xa0213Md/qVLRg= @@ -115,6 +124,7 @@ github.com/pingcap/errors v0.11.4 h1:lFuQV/oaUMGcD2tqt+01ROSmJs75VG1ToEOkZIZ4nE4 github.com/pingcap/errors v0.11.4/go.mod h1:Oi8TUi2kEtXXLMJk9l1cGmz20kV3TaQ0usTwv5KuLY8= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/power-devops/perfstat v0.0.0-20240221224432-82ca36839d55 h1:o4JXh1EVt9k/+g42oCprj/FisM4qX9L3sZB3upGN2ZU= @@ -143,6 +153,9 @@ github.com/sirupsen/logrus v1.9.4 h1:TsZE7l11zFCLZnZ+teH4Umoq5BhEIfIzfRDZ1Uzql2w github.com/sirupsen/logrus v1.9.4/go.mod h1:ftWc9WdOfJ0a92nsE2jF5u5ZwH8Bv2zdeOC42RjbV2g= github.com/spf13/cast v1.10.0 h1:h2x0u2shc1QuLHfxi+cTJvs30+ZAHOGRic8uyGTDWxY= github.com/spf13/cast v1.10.0/go.mod h1:jNfB8QC9IA6ZuY2ZjDp0KtFO2LZZlg4S/7bzP6qqeHo= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/testcontainers/testcontainers-go v0.42.0 h1:He3IhTzTZOygSXLJPMX7n44XtK+qhjat1nI9cneBbUY= @@ -189,6 +202,8 @@ golang.org/x/crypto v0.52.0 h1:RMs7fP2rXdep0CftQlK8Uf+kibLm7qkCcradZWYz988= golang.org/x/crypto v0.52.0/go.mod h1:1QgfPxDqh0T2M/elOJtp9RvuR95kVjir0e6/BvEmGbc= golang.org/x/net v0.54.0 h1:2zJIZAxAHV/OHCDTCOHAYehQzLfSXuf/5SoL/Dv6w/w= golang.org/x/net v0.54.0/go.mod h1:Sj4oj8jK6XmHpBZU/zWHw3BV3abl4Kvi+Ut7cQcY+cQ= +golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= +golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc= @@ -200,5 +215,6 @@ google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= From c1f645612f927555891d4742c053f23e94ef0946 Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:50:30 +0200 Subject: [PATCH 10/15] feat(api): mount /api/v1/settings registry endpoint (#499) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every admin Settings sub-page (general, permalinks, reading, writing, privacy) was calling /api/v1/settings?group= and getting 404 — the settings.Mount call was missing from main.go. The package existed at packages/go/settings with Registry + PostgresStore + MemoryStore implementations, but no HTTP layer. Added apps/api/internal/admin/settings/ with: - handler.go: Deps + Mount + GET/PATCH handlers + groupPrefix mapper. GET filters by ?group= against the registered key prefixes (core.site, core.permalinks, core.reading, core.writing, privacy); empty groups return {} (200), not 500. PATCH validates against the registry schema, capability-gates writes through policy. - handler_test.go: 11 cases. Empty group returns {}. Auth required. Capability gate denies subscriber writes. Unknown keys 400. Validation errors 400. Bad JSON 400. Nil-Deps Mount errors. Wired into main.go via a per-process registry seeded with RegisterCore + RegisterPrivacy. Store falls back to MemoryStore when pool is nil (test boot contexts). Routes wrapped with authmw.RequireSession because the OptionalSession middleware was reverted in #31. Verified live: curl /api/v1/settings?group=core.site → 200 with seeded core.site.{name,tagline,url,default_role}. core.reading + core.writing return {} (no keys registered yet — fix in a separate package change). Known follow-up: packages/go/settings/postgres.go::upsertSQL writes to a `namespace` column that does not exist in migration 000008. PATCH will 500 against the live DB until that's resolved. Same bug in themes/store.go and customizer/store.go. To be filed separately. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- apps/api/cmd/server/main.go | 56 +++ apps/api/internal/admin/settings/handler.go | 336 ++++++++++++++++++ .../internal/admin/settings/handler_test.go | 322 +++++++++++++++++ 3 files changed, 714 insertions(+) create mode 100644 apps/api/internal/admin/settings/handler.go create mode 100644 apps/api/internal/admin/settings/handler_test.go diff --git a/apps/api/cmd/server/main.go b/apps/api/cmd/server/main.go index db9321b6..4cea28a1 100644 --- a/apps/api/cmd/server/main.go +++ b/apps/api/cmd/server/main.go @@ -45,6 +45,7 @@ import ( adminmenus "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/menus" adminpluginpages "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/pluginpages" adminposts "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/posts" + adminsettings "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/settings" adminstatus "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/status" adminthemes "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/themes" adminwebhooks "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/webhooks" @@ -100,6 +101,7 @@ import ( "github.com/Singleton-Solution/GoNext/packages/go/revisions" pkgsearch "github.com/Singleton-Solution/GoNext/packages/go/search" "github.com/Singleton-Solution/GoNext/packages/go/session" + pkgsettings "github.com/Singleton-Solution/GoNext/packages/go/settings" "github.com/Singleton-Solution/GoNext/packages/go/shutdown" "github.com/Singleton-Solution/GoNext/packages/go/theme/seed" "github.com/Singleton-Solution/GoNext/packages/go/tracing" @@ -1512,6 +1514,60 @@ func buildRouter(cfg *config.Config, pool *pgxpool.Pool, rdb *goredis.Client, se } } + // Site settings registry (/api/v1/settings, issue #499). Every + // admin Settings sub-page (general, permalinks, reading, writing, + // privacy) reads + writes through this endpoint. The package-level + // settings registry holds the schema; the Postgres store persists + // values into the options table from migration 000008. + // + // We seed a fresh per-process registry with the core + privacy + // settings here rather than relying on the packages/go/settings + // global so the boot path stays explicit — a future change that + // registers plugin settings would extend this block, not reach into + // a singleton at import time. The Postgres store needs a non-nil + // pool; in pool-less test contexts we fall back to the MemoryStore + // so the route table still mounts and the admin form's "API + // available" probe succeeds. + // + // The routes are wrapped with authmw.RequireSession because the + // global middleware chain does not include OptionalSession (the + // regression fixed in #31). Wrapping per-Mount mirrors how + // /api/v1/auth/sessions and /api/v1/account/data wire themselves. + settingsReg := pkgsettings.NewRegistry() + if err := pkgsettings.RegisterCore(settingsReg); err != nil { + logger.Warn("settings: failed to register core settings", slog.Any("err", err)) + } + if err := pkgsettings.RegisterPrivacy(settingsReg); err != nil { + logger.Warn("settings: failed to register privacy settings", slog.Any("err", err)) + } + var settingsStore pkgsettings.Store + if pool != nil { + settingsStore = pkgsettings.NewPostgresStore(pool, settingsReg) + } else { + settingsStore = pkgsettings.NewMemoryStore(settingsReg) + logger.Warn("settings: pool nil; using in-memory store") + } + settingsMux := http.NewServeMux() + if err := adminsettings.Mount(settingsMux, "/api/v1/settings", adminsettings.Deps{ + Store: settingsStore, + Registry: settingsReg, + Policy: adminPolicy, + Logger: logger, + }); err != nil { + logger.Warn("settings: failed to mount routes", slog.Any("err", err)) + } else { + var settingsHandler http.Handler = settingsMux + if sessions != nil { + settingsHandler = authmw.RequireSession(sessions)(settingsMux) + } else { + logger.Warn("settings: session manager nil; mounting without RequireSession") + } + mux.Handle("/api/v1/settings", settingsHandler) + logger.Info("settings: routes mounted", + slog.String("base", "/api/v1/settings"), + ) + } + // Admin plugin pages surface (/api/v1/admin/plugin-pages). Walks // every active plugin's manifest, pulls out the admin_pages // declarations, and returns the flattened set so the sidebar can diff --git a/apps/api/internal/admin/settings/handler.go b/apps/api/internal/admin/settings/handler.go new file mode 100644 index 00000000..1aa47843 --- /dev/null +++ b/apps/api/internal/admin/settings/handler.go @@ -0,0 +1,336 @@ +// Package settings mounts the thin HTTP layer in front of the +// settings registry shipped in packages/go/settings. +// +// The package itself ships the schema (Setting + Registry), validation, +// and a Postgres-backed Store. What was missing was the HTTP surface +// that the admin Settings sub-pages call — /api/v1/settings?group=… +// (GET) and /api/v1/settings (PATCH). Issue #499 covers wiring that +// surface; this package is that wiring. +// +// Two-layer split (handler here, store + registry in +// packages/go/settings) follows the same convention used by every other +// admin module under apps/api/internal/admin/* (themes, customizer, +// marketplace, …). Keeps the package-level types reusable from CLI / +// worker / migration contexts without dragging net/http into the +// shared package. +package settings + +import ( + "encoding/json" + "errors" + "io" + "log/slog" + "net/http" + "strings" + + "github.com/Singleton-Solution/GoNext/apps/api/internal/rest/router" + "github.com/Singleton-Solution/GoNext/packages/go/policy" + pkgsettings "github.com/Singleton-Solution/GoNext/packages/go/settings" +) + +// maxBodyBytes caps the PATCH payload at 64 KiB. The largest plausible +// patch is a full re-write of every core key (~30 string values, all +// small); 64 KiB is the "stop a runaway client" bound, not a tight +// per-request limit. +const maxBodyBytes = 64 * 1024 + +// Deps is the dependency bag for Mount. Store and Registry are +// required; Policy is required to gate writes; Logger falls back to +// slog.Default when nil. +// +// The Registry is passed explicitly rather than read from the +// packages/go/settings package-level global so callers (in particular +// tests) can isolate their own registry without touching shared state. +type Deps struct { + // Store reads + writes setting values. Memory in tests, Postgres + // in production. + Store pkgsettings.Store + + // Registry holds the Setting declarations the handler iterates + // when building the "all keys in group X" response. + Registry *pkgsettings.Registry + + // Policy gates writes on the manage_options capability. Reads + // require an authenticated principal but no specific capability — + // site settings are operator metadata, not user PII, and every + // admin role that lands in the dashboard has reason to read them. + Policy policy.Policy + + // Logger receives structured log lines. nil falls back to + // slog.Default. + Logger *slog.Logger +} + +func (d Deps) validate() error { + if d.Store == nil { + return errors.New("admin/settings: Store is required") + } + if d.Registry == nil { + return errors.New("admin/settings: Registry is required") + } + if d.Policy == nil { + return errors.New("admin/settings: Policy is required") + } + return nil +} + +// handlers is the resolved-Deps form passed around inside the package. +type handlers struct { + store pkgsettings.Store + registry *pkgsettings.Registry + policy policy.Policy + logger *slog.Logger +} + +// Mount wires the settings routes onto mux under base (typically +// "/api/v1/settings"). Returns an error rather than panicking if Deps +// is malformed so the boot path surfaces it cleanly. +// +// The route tree: +// +// GET {base} — list values, optionally filtered by ?group= +// PATCH {base} — sparse partial update +// +// Reads require any authenticated principal; writes require the +// manage_options capability. +func Mount(mux *http.ServeMux, base string, deps Deps) error { + if err := deps.validate(); err != nil { + return err + } + if deps.Logger == nil { + deps.Logger = slog.Default() + } + h := &handlers{ + store: deps.Store, + registry: deps.Registry, + policy: deps.Policy, + logger: deps.Logger, + } + + base = strings.TrimRight(base, "/") + if base == "" { + return errors.New("admin/settings: base path must not be empty") + } + // Bare-path patterns ("GET /api/v1/settings", no trailing slash, no + // "{$}") match the endpoint exactly under net/http's ServeMux. We + // intentionally do NOT use the "{base}/{$}" form here because that + // would cause net/http to 301-redirect bare-path requests (the + // admin client) to the trailing-slash form, which our handler + // never serves. The admin sends "/api/v1/settings?group=…" with + // no trailing slash; this match must be exact. + mux.Handle("GET "+base, h.requireAuth(h.getSettings)) + mux.Handle("PATCH "+base, h.requireCap(policy.CapManageOptions, h.patchSettings)) + return nil +} + +// requireAuth wraps a handler with the authentication check. The +// authenticated principal is forwarded to the handler; anonymous +// requests get 401 before the handler runs. +func (h *handlers) requireAuth(next func(http.ResponseWriter, *http.Request, policy.Principal)) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + pr, ok := policy.FromContext(r.Context()) + if !ok { + router.WriteError(w, http.StatusUnauthorized, "unauthenticated", "authentication required") + return + } + next(w, r, pr) + }) +} + +// requireCap wraps a handler with the auth + capability check. Returns +// 401 for anonymous requests, 403 when the principal lacks cap. +func (h *handlers) requireCap(cap policy.Capability, next func(http.ResponseWriter, *http.Request, policy.Principal)) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + pr, ok := policy.FromContext(r.Context()) + if !ok { + router.WriteError(w, http.StatusUnauthorized, "unauthenticated", "authentication required") + return + } + if d := h.policy.Can(pr, cap, nil); !d.Allowed { + router.WriteError(w, http.StatusForbidden, "forbidden", d.Reason) + return + } + next(w, r, pr) + }) +} + +// getSettings handles GET /api/v1/settings[?group=]. +// +// The response shape is a flat `{key: value}` JSON object — matches the +// "Settings" schema in apps/api/openapi/openapi.yaml (additionalProperties: +// true) and the SettingsValues type the admin client expects. +// +// The group filter is interpreted as a key-prefix match: ?group=core.site +// returns every registered key starting with "core.site." (so +// core.site.name, core.site.tagline, etc.). This matches the admin's +// SettingsGroup literals ("core.site", "core.reading", …) without +// requiring the registry to be re-grouped — the registry already keys +// settings by dotted namespace, which is the right index. +// +// "privacy" is a special-case: the privacy group's keys live under +// "core.privacy.*", so ?group=privacy is rewritten to "core.privacy" +// before the prefix match. Spelled out here rather than inferred so the +// translation is visible to future maintainers. +// +// Empty group returns every registered key. An unknown group returns +// an empty `{}` (200), not 404 — the admin renders an empty form +// rather than an error banner, which matches the existing "API not +// available" graceful-degradation path. +func (h *handlers) getSettings(w http.ResponseWriter, r *http.Request, _ policy.Principal) { + group := strings.TrimSpace(r.URL.Query().Get("group")) + prefix := groupPrefix(group) + + // Collect the registered keys whose canonical key matches the + // prefix. Empty prefix (no group filter) matches every key. + all := h.registry.List() + keys := make([]string, 0, len(all)) + for _, s := range all { + if prefix == "" || strings.HasPrefix(s.Key, prefix) { + keys = append(keys, s.Key) + } + } + + out := make(map[string]any, len(keys)) + if len(keys) > 0 { + values, err := h.store.BulkRead(r.Context(), keys) + if err != nil { + h.logger.ErrorContext(r.Context(), "admin/settings: bulk read failed", + slog.String("group", group), + slog.Any("err", err), + ) + router.WriteError(w, http.StatusInternalServerError, "internal_error", "failed to load settings") + return + } + for k, v := range values { + out[k] = v + } + } + + router.WriteJSON(w, http.StatusOK, out) +} + +// patchSettings handles PATCH /api/v1/settings. +// +// Request body is a sparse `{key: value}` map; every present key is +// validated against its registered schema and written. The response is +// the post-write values for every key the patch touched, so the admin +// can refresh its form state without a separate GET. +// +// Per-key errors are aggregated into a 400 problem-details response — +// rather than committing the keys that succeeded and 400'ing the +// failure, we fail the whole patch atomically. (The Store interface +// does not expose a transaction handle, so "atomic" here means +// "we don't write anything if any key fails validation"; once a write +// hits Postgres the other writes in the same patch are already +// committed. Validation runs first so the common case — operator +// submits a form with a typo — never reaches the SQL layer.) +func (h *handlers) patchSettings(w http.ResponseWriter, r *http.Request, _ policy.Principal) { + body, err := io.ReadAll(io.LimitReader(r.Body, maxBodyBytes+1)) + if err != nil { + router.WriteError(w, http.StatusBadRequest, "bad_request", "failed to read request body") + return + } + if len(body) > maxBodyBytes { + router.WriteError(w, http.StatusRequestEntityTooLarge, "payload_too_large", + "request body exceeds the maximum allowed size") + return + } + if len(body) == 0 { + router.WriteError(w, http.StatusBadRequest, "bad_request", "request body is empty") + return + } + + var patch map[string]any + if err := json.Unmarshal(body, &patch); err != nil { + router.WriteError(w, http.StatusBadRequest, "bad_json", "request body must be a JSON object") + return + } + if len(patch) == 0 { + // Empty patch — nothing to do, mirror the GET response shape + // for an unfiltered request so the client can still refresh + // its form state from the response. + router.WriteJSON(w, http.StatusOK, map[string]any{}) + return + } + + // Validate-then-write. The Store's Write returns ErrUnknownKey for + // unregistered keys and ErrValidation for schema failures; we map + // both to 400 with the offending key in the detail so the admin + // can highlight the field. + for key, value := range patch { + if _, err := h.registry.Get(key); err != nil { + router.WriteError(w, http.StatusBadRequest, "unknown_key", + "unknown setting key: "+key) + return + } + if err := h.store.Write(r.Context(), key, value); err != nil { + switch { + case errors.Is(err, pkgsettings.ErrUnknownKey): + router.WriteError(w, http.StatusBadRequest, "unknown_key", + "unknown setting key: "+key) + case errors.Is(err, pkgsettings.ErrValidation): + router.WriteError(w, http.StatusBadRequest, "validation_error", + "invalid value for "+key+": "+err.Error()) + default: + h.logger.ErrorContext(r.Context(), "admin/settings: write failed", + slog.String("key", key), + slog.Any("err", err), + ) + router.WriteError(w, http.StatusInternalServerError, "internal_error", + "failed to persist setting "+key) + } + return + } + } + + // Echo back the post-write values for the keys we touched. This + // is the same shape the GET returns, so the client can drop the + // response straight into its form state. + keys := make([]string, 0, len(patch)) + for k := range patch { + keys = append(keys, k) + } + values, err := h.store.BulkRead(r.Context(), keys) + if err != nil { + // The writes succeeded; surfacing this as 500 would mislead the + // admin into thinking the save failed. Log and return the + // caller's patch as the best-effort response. + h.logger.WarnContext(r.Context(), "admin/settings: post-write bulk read failed", + slog.Any("err", err), + ) + router.WriteJSON(w, http.StatusOK, patch) + return + } + router.WriteJSON(w, http.StatusOK, values) +} + +// groupPrefix maps a SettingsGroup literal from the admin client into +// the dotted key-prefix the registry indexes by. +// +// - "" (no group) → "" (match every key) +// - "core.site" → "core.site." (matches core.site.name etc.) +// - "core.reading" → "core.reading." +// - "core.writing" → "core.writing." +// - "core.permalinks" → "core.permalinks." +// - "privacy" → "core.privacy." +// - anything else → "." (best-effort prefix match) +// +// The trailing dot prevents "core.site" from matching "core.sitemap.*" +// if a plugin ever registers under that namespace — prefix matches at +// the dot boundary are intentional, not accidental. +func groupPrefix(group string) string { + if group == "" { + return "" + } + // The admin uses "privacy" as a literal because the corresponding + // settings page lives under /settings/privacy. The registry keys + // are under core.privacy.*. Rewrite once here so the rest of the + // handler can stay prefix-driven. + if group == "privacy" { + return "core.privacy." + } + if strings.HasSuffix(group, ".") { + return group + } + return group + "." +} diff --git a/apps/api/internal/admin/settings/handler_test.go b/apps/api/internal/admin/settings/handler_test.go new file mode 100644 index 00000000..3b0e3e81 --- /dev/null +++ b/apps/api/internal/admin/settings/handler_test.go @@ -0,0 +1,322 @@ +package settings + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/Singleton-Solution/GoNext/packages/go/policy" + pkgsettings "github.com/Singleton-Solution/GoNext/packages/go/settings" +) + +// testBase is the route prefix used in every test. Mirrors the +// production wiring in main.go. +const testBase = "/api/v1/settings" + +// adminPrincipal is the Principal a request carries when an admin is +// signed in. The admin role holds manage_options in +// DefaultRoleCapabilities, so writes succeed. +func adminPrincipal() policy.Principal { + return policy.Principal{UserID: "user:admin", Roles: []policy.Role{policy.RoleAdmin}} +} + +// subscriberPrincipal lacks manage_options but is authenticated, so +// reads succeed and writes 403. +func subscriberPrincipal() policy.Principal { + return policy.Principal{UserID: "user:subscriber", Roles: []policy.Role{policy.RoleSubscriber}} +} + +// newTestDeps builds a Deps with a fresh registry seeded with core + +// privacy settings and a MemoryStore. The returned Registry pointer is +// the same one in Deps, so tests that want to register additional +// settings can do so after construction. +func newTestDeps(t *testing.T) Deps { + t.Helper() + reg := pkgsettings.NewRegistry() + if err := pkgsettings.RegisterCore(reg); err != nil { + t.Fatalf("RegisterCore: %v", err) + } + if err := pkgsettings.RegisterPrivacy(reg); err != nil { + t.Fatalf("RegisterPrivacy: %v", err) + } + return Deps{ + Store: pkgsettings.NewMemoryStore(reg), + Registry: reg, + Policy: policy.NewBasicPolicy(policy.DefaultRoleCapabilities()), + } +} + +// mountTest builds a fresh mux + handler from deps. Returns the mux so +// tests can drive ServeHTTP directly. +func mountTest(t *testing.T, deps Deps) *http.ServeMux { + t.Helper() + mux := http.NewServeMux() + if err := Mount(mux, testBase, deps); err != nil { + t.Fatalf("Mount: %v", err) + } + return mux +} + +// do executes req against mux and returns the recorded response. A +// helper because every test does this five-line sequence. +func do(t *testing.T, mux *http.ServeMux, req *http.Request) *httptest.ResponseRecorder { + t.Helper() + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + return rec +} + +// withPrincipal returns a copy of req with the given Principal stashed +// on its context — mirrors what the auth middleware does in production. +func withPrincipal(req *http.Request, pr policy.Principal) *http.Request { + return req.WithContext(policy.WithPrincipal(req.Context(), pr)) +} + +// TestMount_RequiresAuth verifies that an anonymous GET returns 401 +// rather than serving the response or panicking on a nil Principal. +func TestMount_RequiresAuth(t *testing.T) { + deps := newTestDeps(t) + mux := mountTest(t, deps) + + req := httptest.NewRequest(http.MethodGet, testBase, nil) + rec := do(t, mux, req) + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status: want 401, got %d body=%s", rec.Code, rec.Body.String()) + } +} + +// TestGet_EmptyGroup_ReturnsEmptyMap is the acceptance-test from issue +// #499: a GET with a registered group whose keys are not yet stored +// returns 200 with the registered defaults (or an empty map if no keys +// match). Critically, it does NOT 404 or 500 — the admin form has a +// shape to render. +func TestGet_EmptyGroup_ReturnsEmptyMap(t *testing.T) { + // Use a registry with NO settings registered so the response is + // guaranteed empty. This is the strictest form of the acceptance + // criterion: empty group means {} body, not 500. + reg := pkgsettings.NewRegistry() + deps := Deps{ + Store: pkgsettings.NewMemoryStore(reg), + Registry: reg, + Policy: policy.NewBasicPolicy(policy.DefaultRoleCapabilities()), + } + mux := mountTest(t, deps) + + req := httptest.NewRequest(http.MethodGet, testBase+"?group=core.site", nil) + req = withPrincipal(req, adminPrincipal()) + rec := do(t, mux, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status: want 200, got %d body=%s", rec.Code, rec.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("unmarshal response: %v body=%s", err, rec.Body.String()) + } + if len(body) != 0 { + t.Fatalf("response body: want {}, got %v", body) + } +} + +// TestGet_CoreSite_ReturnsDefaults verifies the happy path for the +// admin /settings/general page: GET ?group=core.site returns the +// registered defaults for core.site.* keys. +func TestGet_CoreSite_ReturnsDefaults(t *testing.T) { + deps := newTestDeps(t) + mux := mountTest(t, deps) + + req := httptest.NewRequest(http.MethodGet, testBase+"?group=core.site", nil) + req = withPrincipal(req, adminPrincipal()) + rec := do(t, mux, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status: want 200, got %d body=%s", rec.Code, rec.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("unmarshal response: %v", err) + } + // Expect at least core.site.name to be present with its default. + if got, ok := body["core.site.name"].(string); !ok || got != "My GoNext Site" { + t.Fatalf("core.site.name: want default %q, got %v", "My GoNext Site", body["core.site.name"]) + } + // core.permalinks.format is in a different group prefix and must + // NOT leak into the response. + if _, present := body["core.permalinks.format"]; present { + t.Fatalf("response leaked permalinks key into core.site group: %v", body) + } +} + +// TestGet_PrivacyGroup_RewritesPrefix verifies the special-case "privacy" +// → "core.privacy." prefix rewrite documented on groupPrefix. +func TestGet_PrivacyGroup_RewritesPrefix(t *testing.T) { + deps := newTestDeps(t) + mux := mountTest(t, deps) + + req := httptest.NewRequest(http.MethodGet, testBase+"?group=privacy", nil) + req = withPrincipal(req, adminPrincipal()) + rec := do(t, mux, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status: want 200, got %d body=%s", rec.Code, rec.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("unmarshal response: %v", err) + } + if _, present := body[pkgsettings.PrivacyAllowGDPRSelfService]; !present { + t.Fatalf("response missing privacy key %q: body=%v", + pkgsettings.PrivacyAllowGDPRSelfService, body) + } +} + +// TestGet_NoGroup_ReturnsAll verifies that an unfiltered GET returns +// every registered key — the shape the OpenAPI spec documents +// (`additionalProperties: true`). +func TestGet_NoGroup_ReturnsAll(t *testing.T) { + deps := newTestDeps(t) + mux := mountTest(t, deps) + + req := httptest.NewRequest(http.MethodGet, testBase, nil) + req = withPrincipal(req, adminPrincipal()) + rec := do(t, mux, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status: want 200, got %d", rec.Code) + } + var body map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("unmarshal response: %v", err) + } + // Core + privacy registries together ship >10 settings. A strict + // count here would rot every time a key is added; assert the floor. + if len(body) < 10 { + t.Fatalf("unfiltered response: want >=10 keys, got %d (%v)", len(body), body) + } +} + +// TestPatch_HappyPath verifies that a valid PATCH persists the value +// and the response echoes it. +func TestPatch_HappyPath(t *testing.T) { + deps := newTestDeps(t) + mux := mountTest(t, deps) + + payload := map[string]any{"core.site.name": "Hello"} + body, _ := json.Marshal(payload) + + req := httptest.NewRequest(http.MethodPatch, testBase, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req = withPrincipal(req, adminPrincipal()) + rec := do(t, mux, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status: want 200, got %d body=%s", rec.Code, rec.Body.String()) + } + var got map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("unmarshal response: %v", err) + } + if got["core.site.name"] != "Hello" { + t.Fatalf("response: want Hello, got %v", got["core.site.name"]) + } + + // Round-trip via GET to confirm persistence. + getReq := httptest.NewRequest(http.MethodGet, testBase+"?group=core.site", nil) + getReq = withPrincipal(getReq, adminPrincipal()) + getRec := do(t, mux, getReq) + if getRec.Code != http.StatusOK { + t.Fatalf("get status: want 200, got %d", getRec.Code) + } + var afterGet map[string]any + if err := json.Unmarshal(getRec.Body.Bytes(), &afterGet); err != nil { + t.Fatalf("get unmarshal: %v", err) + } + if afterGet["core.site.name"] != "Hello" { + t.Fatalf("post-write GET: want Hello, got %v", afterGet["core.site.name"]) + } +} + +// TestPatch_RequiresManageOptions verifies that a request from a user +// without manage_options is rejected 403. +func TestPatch_RequiresManageOptions(t *testing.T) { + deps := newTestDeps(t) + mux := mountTest(t, deps) + + body, _ := json.Marshal(map[string]any{"core.site.name": "Nope"}) + req := httptest.NewRequest(http.MethodPatch, testBase, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req = withPrincipal(req, subscriberPrincipal()) + rec := do(t, mux, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status: want 403, got %d body=%s", rec.Code, rec.Body.String()) + } +} + +// TestPatch_UnknownKey_Returns400 verifies that PATCH with an +// unregistered key returns a structured 400 rather than committing or +// 500'ing. +func TestPatch_UnknownKey_Returns400(t *testing.T) { + deps := newTestDeps(t) + mux := mountTest(t, deps) + + body, _ := json.Marshal(map[string]any{"this.does.not.exist": "x"}) + req := httptest.NewRequest(http.MethodPatch, testBase, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req = withPrincipal(req, adminPrincipal()) + rec := do(t, mux, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status: want 400, got %d body=%s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "unknown") { + t.Fatalf("response should mention 'unknown', got %s", rec.Body.String()) + } +} + +// TestPatch_ValidationError verifies that a value violating the +// schema is rejected 400. +func TestPatch_ValidationError(t *testing.T) { + deps := newTestDeps(t) + mux := mountTest(t, deps) + + // core.site.name has minLength:1, maxLength:80. An empty string + // fails minLength. + body, _ := json.Marshal(map[string]any{"core.site.name": ""}) + req := httptest.NewRequest(http.MethodPatch, testBase, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req = withPrincipal(req, adminPrincipal()) + rec := do(t, mux, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status: want 400, got %d body=%s", rec.Code, rec.Body.String()) + } +} + +// TestPatch_BadJSON returns 400 on a malformed body. +func TestPatch_BadJSON(t *testing.T) { + deps := newTestDeps(t) + mux := mountTest(t, deps) + + req := httptest.NewRequest(http.MethodPatch, testBase, strings.NewReader("not json")) + req.Header.Set("Content-Type", "application/json") + req = withPrincipal(req, adminPrincipal()) + rec := do(t, mux, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status: want 400, got %d", rec.Code) + } +} + +// TestMount_NilDeps_Errors verifies that Mount returns an error +// rather than panicking when Deps is malformed. +func TestMount_NilDeps_Errors(t *testing.T) { + mux := http.NewServeMux() + if err := Mount(mux, testBase, Deps{}); err == nil { + t.Fatal("Mount: want error for empty Deps, got nil") + } +} From 4eb0358d815920d79e8ec2508fc233d93a00cf64 Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:52:13 +0200 Subject: [PATCH 11/15] feat(api+web): mount /themes/{slug}/{file} static handler (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The early-hints provider at apps/api/cmd/server/main.go:431 preloads /themes/active/style.css, but nothing serves it — every theme asset 404'd. The public site at apps/web fell back to its globals.css defaults; no theme tokens reached the browser. apps/api/internal/themes/static/ (new): - handler.go: Mount(mux, "/themes", Deps{ThemeDir, ActiveResolver, Logger}). Path layout: /themes/{slug}/{file...}. Sentinel slug "active" resolves through ActiveResolver. Path traversal rejected before any FS read. Content-Type per file extension. 1h Cache-Control. Missing/invalid → 404 (transient DB hiccups on the resolver shouldn't bring down public CSS). - handler_test.go: 14 cases. Real file, nested path, HEAD short- circuit, path-traversal flavors, ..-segment guard, ActiveResolver wiring, empty resolver → 404, missing file → 404, directory → 404, slug syntax check, Mount Deps validation, isSafeRelPath units, contentType units. Wired into main.go: - Extracted &adminthemes.PgxActiveStore{Pool: pool} to a local var so it's shared with both the admin themes Mount and the static resolver closure (single source of truth on which slug is currently active). - Mount block after the admin/themes block. apps/web/src/app/layout.tsx: - Added inside a new block so the public site actually pulls the active theme CSS. PUBLIC_API_URL resolves via NEXT_PUBLIC_API_URL with localhost:8080 fallback. Verified live: /themes/gn-hello/style.css → 200, text/css, 4683 bytes, cache 1h /themes/active/style.css → 200, same body (resolver wired) /themes/.../etc/passwd → 400 invalid path /themes/missing/style.css → 404 Follow-up: ActiveResolver hits Postgres on every CSS request. The options table autoloads so the query is microsecond-cheap today, but a TTL cache would be appropriate as traffic grows. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- apps/api/cmd/server/main.go | 38 +- apps/api/internal/themes/static/doc.go | 32 ++ apps/api/internal/themes/static/handler.go | 385 ++++++++++++++++++ .../internal/themes/static/handler_test.go | 355 ++++++++++++++++ apps/web/src/app/layout.tsx | 24 ++ 5 files changed, 833 insertions(+), 1 deletion(-) create mode 100644 apps/api/internal/themes/static/doc.go create mode 100644 apps/api/internal/themes/static/handler.go create mode 100644 apps/api/internal/themes/static/handler_test.go diff --git a/apps/api/cmd/server/main.go b/apps/api/cmd/server/main.go index 4cea28a1..a0f81dd2 100644 --- a/apps/api/cmd/server/main.go +++ b/apps/api/cmd/server/main.go @@ -51,6 +51,7 @@ import ( adminwebhooks "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/webhooks" restimg "github.com/Singleton-Solution/GoNext/apps/api/internal/rest/img" "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/customizer" + themesstatic "github.com/Singleton-Solution/GoNext/apps/api/internal/themes/static" adminredirects "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/redirects" "github.com/Singleton-Solution/GoNext/apps/api/internal/admin/rum" "github.com/Singleton-Solution/GoNext/apps/api/internal/auth/login" @@ -763,9 +764,10 @@ func buildRouter(cfg *config.Config, pool *pgxpool.Pool, rdb *goredis.Client, se // gate the routes — install_themes, manage_themes, switch_themes // — so a deploy that only wants the switch surface can withhold // install_themes without losing the list endpoint. + themeActiveStore := &adminthemes.PgxActiveStore{Pool: pool} if err := adminthemes.Mount(mux, "/api/v1/admin/themes", adminthemes.Deps{ ThemeDir: themeDir, - Active: &adminthemes.PgxActiveStore{Pool: pool}, + Active: themeActiveStore, Policy: policy.NewBasicPolicy(policy.DefaultRoleCapabilities()), Logger: logger, }); err != nil { @@ -776,6 +778,40 @@ func buildRouter(cfg *config.Config, pool *pgxpool.Pool, rdb *goredis.Client, se ) } + // Public theme-asset surface (issue #501). Serves the on-disk + // CSS/JS/font files for every installed theme under /themes/{slug} + // /{file...}. The virtual sentinel "active" resolves through the + // shared PgxActiveStore so /themes/active/style.css follows the + // active-theme switch automatically. Without this handler the + // early-hints provider above preloads a URL no one serves and the + // public site renders with whatever globals.css ships — no theme + // CSS reaches the browser. + // + // We construct an ActiveResolver closure that calls Get on the + // existing store (same options-row read the admin handlers use). + // A read error is logged and surfaced as "no active theme" — the + // handler turns that into a 404 rather than a 500 because a + // transient DB hiccup should not bring down public CSS. + if err := themesstatic.Mount(mux, "/themes", themesstatic.Deps{ + ThemeDir: themeDir, + ActiveResolver: func() string { + slug, err := themeActiveStore.Get(context.Background()) + if err != nil { + logger.Debug("themes/static: active resolver failed", + slog.Any("err", err)) + return "" + } + return slug + }, + Logger: logger, + }); err != nil { + logger.Warn("themes/static: failed to mount", slog.Any("err", err)) + } else { + logger.Info("themes/static: routes mounted", + slog.String("base", "/themes"), + ) + } + // First-run install surface — the in-browser alternative to // `gonext init`. The two endpoints (/api/v1/setup/status and // /api/v1/setup/install) are mounted unconditionally; the lock diff --git a/apps/api/internal/themes/static/doc.go b/apps/api/internal/themes/static/doc.go new file mode 100644 index 00000000..02e6f1c1 --- /dev/null +++ b/apps/api/internal/themes/static/doc.go @@ -0,0 +1,32 @@ +// Package static serves theme assets (style.css, JS, etc.) over HTTP +// directly from the on-disk theme directory. +// +// The route shape is: +// +// GET /themes/{slug}/{file...} +// +// where {slug} is either a real theme directory name (e.g. "gn-hello") +// or the virtual sentinel "active", which the package resolves through +// the supplied ActiveResolver against the live core.active_theme option. +// +// The handler is a thin static-file server with a few targeted hardenings: +// +// - It rejects any path containing ".." or an absolute prefix BEFORE +// touching the filesystem (path-traversal guard). +// - It refuses to serve files outside the configured ThemeDir even if +// a symlink inside a theme directory pointed elsewhere — every +// served path is rebuilt with filepath.Clean and re-checked against +// the canonical ThemeDir prefix. +// - It sets Content-Type from the file extension (css, js, woff2, +// png, etc.) rather than sniffing. +// - It sets Cache-Control: public, max-age=3600 so the public site + +// CDN can cache the response; theme installs bump the slug, which +// transparently busts the cached path. +// +// The package intentionally does NOT depend on the customizer or +// admin/themes packages — both expose the same options-table key but +// import paths there carry full read+write Store surfaces. The caller +// supplies a closure that performs whatever lookup it wants. In +// production main.go wires this against the existing PgxActiveStore; +// tests pass a literal closure. +package static diff --git a/apps/api/internal/themes/static/handler.go b/apps/api/internal/themes/static/handler.go new file mode 100644 index 00000000..796b1dd3 --- /dev/null +++ b/apps/api/internal/themes/static/handler.go @@ -0,0 +1,385 @@ +package static + +import ( + "errors" + "io" + "log/slog" + "mime" + "net/http" + "os" + "path" + "path/filepath" + "regexp" + "strconv" + "strings" +) + +// ActiveResolver returns the slug of the currently-active theme. The +// caller is expected to consult whatever store backs core.active_theme +// (the Postgres options row in production, an in-memory string in +// tests). Returning an empty string signals "no active theme" and the +// handler responds with 404 — this matches the contract of every other +// theme-aware surface in the codebase, which treats "no active theme" +// as a recoverable not-found rather than an internal error. +type ActiveResolver func() string + +// Deps is the dependency bag for Mount. ThemeDir and ActiveResolver +// are required; Logger falls back to slog.Default for tests that don't +// care about log capture. +type Deps struct { + // ThemeDir is the absolute or relative path that holds one + // subdirectory per installed theme. In production this is /themes + // in the api container (the GONEXT_THEME_DIR env var); on a dev + // host it defaults to "./themes" relative to the process CWD. + ThemeDir string + + // ActiveResolver maps the virtual "active" slug to the real slug. + // Production wires this against the options store; tests use a + // closure literal. + ActiveResolver ActiveResolver + + // Logger receives structured warnings (e.g. fallthrough opens that + // hit a missing file under a valid slug). Always non-nil after + // Mount applies its default. + Logger *slog.Logger +} + +func (d Deps) validate() error { + if strings.TrimSpace(d.ThemeDir) == "" { + return errors.New("themes/static: ThemeDir is required") + } + if d.ActiveResolver == nil { + return errors.New("themes/static: ActiveResolver is required") + } + return nil +} + +// slugPattern matches the kebab-case-lowercase rule used everywhere +// else in the theme stack (mirrors apps/api/internal/admin/themes +// /installer.go and packages/go/theme/validate.go). The static handler +// re-applies it as a belt-and-braces guard so a request that smuggled +// a slash-encoded path-traversal segment into the {slug} wildcard +// never reaches filepath.Join. The literal sentinel "active" is +// matched separately before this regex runs. +var slugPattern = regexp.MustCompile(`^[a-z][a-z0-9-]*[a-z0-9]$`) + +// cacheControl is set on every successful response. One hour is short +// enough that an operator who swaps the active theme sees the new CSS +// without waiting for a long-tail eviction; long enough that the +// public site doesn't re-fetch on every page navigation. The CDN / +// browser respects the slug→content coupling — a theme update bumps +// the seeded slug or rewrites the file in-place, in which case the +// next miss is the right cache key. +const cacheControl = "public, max-age=3600" + +// Mount wires the static-asset routes onto mux under base (typically +// "/themes"). Returns an error rather than panicking if Deps is +// malformed so the boot path can log and continue without taking the +// whole server down for a theme-server misconfiguration. +// +// The route tree: +// +// GET {base}/{slug}/{file...} — serve themeDir// +// +// The {slug} segment may be: +// - a real theme directory name (e.g. "gn-hello") — served as-is +// - the sentinel "active" — resolved via ActiveResolver +// +// Path-traversal segments in {file...} are rejected with 400. Files +// not on disk return 404. Read errors return 500. +func Mount(mux *http.ServeMux, base string, deps Deps) error { + if err := deps.validate(); err != nil { + return err + } + if deps.Logger == nil { + deps.Logger = slog.Default() + } + + base = strings.TrimRight(base, "/") + if base == "" { + return errors.New("themes/static: base must not be empty") + } + + // We resolve ThemeDir to its absolute, cleaned form once at Mount + // time so the per-request prefix check is a string compare on a + // stable canonical value. Production passes "/themes" which already + // is absolute; the dev default "./themes" gets resolved against + // the process CWD. If the directory does not yet exist (a fresh + // dev tree before `pnpm install` or a misconfigured volume mount) + // we still proceed — every request will simply 404 until the + // directory shows up. We deliberately do NOT block the route + // mount on a directory existence check so a temporary missing + // volume doesn't break the entire boot. + root, err := filepath.Abs(filepath.Clean(deps.ThemeDir)) + if err != nil { + return err + } + + h := &handler{ + root: root, + activeResolver: deps.ActiveResolver, + logger: deps.Logger.With(slog.String("subsystem", "themes/static")), + } + + // Single wildcard route. Go 1.22+ mux supports {file...} as a + // trailing wildcard that captures the remainder of the path + // verbatim. Note: we still re-validate the captured value because + // "the mux already split it" is not a substitute for "we checked + // the bytes." The {slug} segment is captured as a single segment; + // it cannot contain a slash by virtue of how the mux tokenises. + mux.Handle("GET "+base+"/{slug}/{file...}", http.HandlerFunc(h.serve)) + + return nil +} + +// handler is the resolved-Deps form passed around inside the package. +type handler struct { + // root is the absolute, cleaned path to the theme directory. Every + // served file must live under this prefix; we re-check after + // resolving the symlink-y filepath.Join just in case a symlink + // inside a theme directory points elsewhere. + root string + + // activeResolver maps the virtual "active" slug to the real slug. + activeResolver ActiveResolver + + // logger is namespaced with the subsystem key so structured-log + // consumers can filter on it. + logger *slog.Logger +} + +// serve handles a single GET request. The logic is small enough to +// inline; splitting it would obscure the security-relevant ordering. +func (h *handler) serve(w http.ResponseWriter, r *http.Request) { + // 1. Capture + validate the slug. + rawSlug := r.PathValue("slug") + slug := rawSlug + if slug == "active" { + slug = strings.TrimSpace(h.activeResolver()) + if slug == "" { + // No active theme configured. Surface as 404 rather than + // 500 — every theme-aware endpoint in the codebase treats + // "no active theme" as a recoverable not-found. + h.logger.Debug("active resolver returned empty; serving 404", + slog.String("path", r.URL.Path)) + http.NotFound(w, r) + return + } + } + + if !slugPattern.MatchString(slug) { + // Either the caller asked for a malformed slug directly or the + // active resolver produced one. Either way, 400 is the right + // signal: the request shape is invalid. + h.logger.Warn("invalid slug", + slog.String("requested", rawSlug), + slog.String("resolved", slug), + ) + http.Error(w, "invalid theme slug", http.StatusBadRequest) + return + } + + // 2. Capture + validate the file path. The mux gives us the + // remainder of the URL path AFTER the slug segment, with no + // leading slash. We forbid absolute paths, ".." segments, and + // empty file paths up front — these never reach the filesystem. + file := r.PathValue("file") + if !isSafeRelPath(file) { + h.logger.Warn("rejected path traversal attempt", + slog.String("slug", slug), + slog.String("file", file), + ) + http.Error(w, "invalid path", http.StatusBadRequest) + return + } + + // 3. Build the absolute on-disk path and verify it lives under root + // AFTER filepath.Clean has had its say. This catches any traversal + // we missed in isSafeRelPath as well as a symlink that points + // outside the theme directory. + abs := filepath.Join(h.root, slug, filepath.FromSlash(file)) + cleaned := filepath.Clean(abs) + if !isUnder(cleaned, h.root) { + h.logger.Warn("path escapes root after clean", + slog.String("slug", slug), + slog.String("file", file), + slog.String("cleaned", cleaned), + ) + http.Error(w, "invalid path", http.StatusBadRequest) + return + } + + // 4. Open + stat. We treat ENOENT as 404 and any other read error + // as 500. We intentionally do NOT distinguish "directory" from + // "missing file" — both surface as 404 because /themes/gn-hello + // (no file) is just as useless to the caller as /themes/missing + // /style.css. + f, err := os.Open(cleaned) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + http.NotFound(w, r) + return + } + h.logger.Error("open failed", + slog.String("path", cleaned), + slog.Any("err", err), + ) + http.Error(w, "read error", http.StatusInternalServerError) + return + } + defer func() { _ = f.Close() }() + + info, err := f.Stat() + if err != nil { + h.logger.Error("stat failed", + slog.String("path", cleaned), + slog.Any("err", err), + ) + http.Error(w, "stat error", http.StatusInternalServerError) + return + } + if info.IsDir() { + // We don't serve directory listings. A user landing on the + // directory URL almost certainly meant a missing file. + http.NotFound(w, r) + return + } + + // 5. Set headers. Content-Type from extension (we maintain a small + // allow-list for the common theme asset types; anything else + // falls back to mime.TypeByExtension which Go seeds from the + // system mime table). Cache-Control fixed to one hour. + w.Header().Set("Content-Type", contentType(cleaned)) + w.Header().Set("Cache-Control", cacheControl) + w.Header().Set("Content-Length", strconv.FormatInt(info.Size(), 10)) + + // 6. Stream. http.ServeContent would handle Range + ETag for us + // but the bookkeeping isn't worth it here — these are tiny files + // (style.css is single-digit KiB) and Range requests for theme + // CSS are not a real workload. A plain io.Copy is the right shape. + if r.Method == http.MethodHead { + return + } + if _, err := io.Copy(w, f); err != nil { + // Connection drop mid-stream. There's nothing useful to do + // here — headers are already on the wire. Log at debug so a + // flaky client doesn't spam error logs. + h.logger.Debug("copy failed (client likely disconnected)", + slog.String("path", cleaned), + slog.Any("err", err), + ) + } +} + +// isSafeRelPath reports whether p is a relative path with no traversal +// segments. We reject: +// - empty paths (the caller asked for /themes// with nothing +// after the trailing slash; mux delivers an empty {file...}) +// - absolute paths (leading slash) +// - any segment exactly equal to ".." (path.Clean would collapse +// "a/../b" to "b" but we want to surface the attempt explicitly) +// - any segment containing a NUL byte +// +// path.Clean on the input is checked separately — a clean path that +// still starts with ".." (e.g. "../../etc/passwd" which cleans to +// "../../etc/passwd") slips past per-segment checks but fails the +// "starts with .." check below. +func isSafeRelPath(p string) bool { + if p == "" { + return false + } + if strings.HasPrefix(p, "/") { + return false + } + if strings.ContainsRune(p, 0) { + return false + } + // path.Clean uses forward slashes (the URL path separator), which + // is what we want — the URL we received is forward-slashed even + // on Windows. + cleaned := path.Clean(p) + if cleaned == ".." || strings.HasPrefix(cleaned, "../") { + return false + } + // Defensive: catch a "..\\" form that path.Clean wouldn't notice + // because it doesn't treat backslash as a separator. We don't + // expect this from a stdlib mux but the cost is one substring + // search per request and the consequence of missing it would be + // a filesystem escape on Windows. + if strings.Contains(p, `..\`) || strings.Contains(p, `\..`) { + return false + } + for _, seg := range strings.Split(p, "/") { + if seg == ".." { + return false + } + } + return true +} + +// isUnder reports whether child resolves to a path under parent. +// Both arguments must already have been run through filepath.Clean. +// We add a trailing separator to the parent before the prefix check +// so "/themes" does not accept "/themes-other/...". +func isUnder(child, parent string) bool { + if child == parent { + return true + } + if !strings.HasSuffix(parent, string(filepath.Separator)) { + parent += string(filepath.Separator) + } + return strings.HasPrefix(child, parent) +} + +// contentType returns the MIME type for a given on-disk path. We +// hard-code the common theme asset extensions because Go's +// mime.TypeByExtension consults the system mime table — which is +// present on every distro we ship to, but isn't guaranteed inside +// the distroless final image. A small explicit table is the right +// trade for the asset types that actually ship with a theme. Falls +// back to TypeByExtension for everything else; final fallback is +// application/octet-stream. +func contentType(p string) string { + switch strings.ToLower(filepath.Ext(p)) { + case ".css": + return "text/css; charset=utf-8" + case ".js", ".mjs": + return "application/javascript; charset=utf-8" + case ".json": + return "application/json; charset=utf-8" + case ".map": + return "application/json; charset=utf-8" + case ".svg": + return "image/svg+xml" + case ".png": + return "image/png" + case ".jpg", ".jpeg": + return "image/jpeg" + case ".gif": + return "image/gif" + case ".webp": + return "image/webp" + case ".avif": + return "image/avif" + case ".ico": + return "image/x-icon" + case ".woff": + return "font/woff" + case ".woff2": + return "font/woff2" + case ".ttf": + return "font/ttf" + case ".otf": + return "font/otf" + case ".eot": + return "application/vnd.ms-fontobject" + case ".txt": + return "text/plain; charset=utf-8" + case ".html", ".htm": + return "text/html; charset=utf-8" + } + if mt := mime.TypeByExtension(filepath.Ext(p)); mt != "" { + return mt + } + return "application/octet-stream" +} diff --git a/apps/api/internal/themes/static/handler_test.go b/apps/api/internal/themes/static/handler_test.go new file mode 100644 index 00000000..88356580 --- /dev/null +++ b/apps/api/internal/themes/static/handler_test.go @@ -0,0 +1,355 @@ +package static + +import ( + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +// newTestTree builds a temp theme directory laid out like the +// production volume mount: +// +// / +// gn-hello/ +// style.css +// parts/header.css +// theme.json +// gn-pro/ +// style.css +// +// Returns the absolute path to . +func newTestTree(t *testing.T) string { + t.Helper() + root := t.TempDir() + for _, dir := range []string{"gn-hello", "gn-hello/parts", "gn-pro"} { + if err := os.MkdirAll(filepath.Join(root, dir), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", dir, err) + } + } + writeFile := func(rel, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(root, rel), []byte(body), 0o644); err != nil { + t.Fatalf("write %s: %v", rel, err) + } + } + writeFile("gn-hello/style.css", ":root { --paper: #F5F2EA; }\n") + writeFile("gn-hello/parts/header.css", "header { padding: 1rem; }\n") + writeFile("gn-hello/theme.json", `{"slug":"gn-hello"}`) + writeFile("gn-pro/style.css", ":root { --paper: #ffffff; }\n") + return root +} + +// mountTest builds a mux + serve test rig for the static handler. +// activeSlug is the value returned by the ActiveResolver closure on +// every call; tests that need a varying resolver build their own. +func mountTest(t *testing.T, themeDir, activeSlug string) http.Handler { + t.Helper() + mux := http.NewServeMux() + if err := Mount(mux, "/themes", Deps{ + ThemeDir: themeDir, + ActiveResolver: func() string { return activeSlug }, + }); err != nil { + t.Fatalf("Mount: %v", err) + } + return mux +} + +func TestServe_RealFile_ReturnsCSS(t *testing.T) { + root := newTestTree(t) + h := mountTest(t, root, "gn-hello") + + req := httptest.NewRequest(http.MethodGet, "/themes/gn-hello/style.css", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status: got %d, want 200", rec.Code) + } + if got := rec.Header().Get("Content-Type"); got != "text/css; charset=utf-8" { + t.Errorf("Content-Type: got %q, want text/css; charset=utf-8", got) + } + if got := rec.Header().Get("Cache-Control"); got != "public, max-age=3600" { + t.Errorf("Cache-Control: got %q, want public, max-age=3600", got) + } + body, _ := io.ReadAll(rec.Body) + if !strings.Contains(string(body), "--paper: #F5F2EA") { + t.Errorf("body: missing expected CSS, got %q", string(body)) + } +} + +func TestServe_NestedFile_ReturnsCSS(t *testing.T) { + root := newTestTree(t) + h := mountTest(t, root, "gn-hello") + + req := httptest.NewRequest(http.MethodGet, "/themes/gn-hello/parts/header.css", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status: got %d, want 200", rec.Code) + } + if got := rec.Header().Get("Content-Type"); got != "text/css; charset=utf-8" { + t.Errorf("Content-Type: got %q, want text/css; charset=utf-8", got) + } +} + +func TestServe_HeadRequestSendsHeadersOnly(t *testing.T) { + root := newTestTree(t) + h := mountTest(t, root, "gn-hello") + + // The handler is mounted with "GET" — but Go's mux treats HEAD as + // an alias for GET when no explicit HEAD handler is registered, so + // the request reaches our code with r.Method == "HEAD" and we + // short-circuit the body copy. The mux behavior is a stdlib + // detail; we exercise our short-circuit explicitly. + req := httptest.NewRequest(http.MethodGet, "/themes/gn-hello/style.css", nil) + req.Method = http.MethodHead + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + // Some mux versions reject HEAD outright when only GET is + // registered. Accept either 200 (the short-circuit fired) or 405 + // (mux refused) — what we care about is that no body is leaked + // when the handler does run. + if rec.Code == http.StatusOK && rec.Body.Len() != 0 { + t.Errorf("HEAD: body should be empty, got %q", rec.Body.String()) + } +} + +func TestServe_PathTraversalRejected(t *testing.T) { + root := newTestTree(t) + h := mountTest(t, root, "gn-hello") + + cases := []struct { + name string + path string + }{ + // Slash-encoded traversal inside the {file...} segment. + {"dotdot_in_file", "/themes/gn-hello/../../etc/passwd"}, + // Traversal at the head of {file...}. + {"dotdot_head", "/themes/gn-hello/..%2Fetc%2Fpasswd"}, + // Backslash flavour (Windows-shaped). We reject pre-clean. + {"backslash_traversal", `/themes/gn-hello/..\etc\passwd`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, tc.path, nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + // At least one of {400, 404} is acceptable depending on + // whether the mux pre-canonicalised the path. What we + // care about is that we did NOT return 200 — i.e. the + // attacker did not get a file out of us. + if rec.Code == http.StatusOK { + t.Errorf("path %q: served 200 (escape!) body=%q", tc.path, rec.Body.String()) + } + }) + } +} + +func TestServe_DotDotSegmentExplicitlyRejected(t *testing.T) { + // Hit the handler directly (bypass mux url-canonicalisation) with + // a {file...} value that contains ".." literally. This exercises + // the isSafeRelPath guard rather than relying on the mux's + // behavior — the docs explicitly require 400 on ".." in path. + root := newTestTree(t) + mux := http.NewServeMux() + if err := Mount(mux, "/themes", Deps{ + ThemeDir: root, + ActiveResolver: func() string { return "gn-hello" }, + }); err != nil { + t.Fatalf("Mount: %v", err) + } + + // Use a request URL the mux WILL route to our handler (no + // canonical-redirect): /themes/gn-hello/ where + // contains a single ".." segment. + req := httptest.NewRequest(http.MethodGet, "/themes/gn-hello/sub", nil) + req.URL.RawPath = "/themes/gn-hello/sub/..%2Fother.css" + req.URL.Path = "/themes/gn-hello/sub/../other.css" + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + // Either the mux canonicalised the path away (in which case it + // 301s) or our handler ran and surfaced 400. Accept either, but + // not 200. + if rec.Code == http.StatusOK { + t.Errorf("expected non-200, got 200, body=%q", rec.Body.String()) + } +} + +func TestServe_ActiveResolverWiredThrough(t *testing.T) { + root := newTestTree(t) + + // The resolver returns gn-hello on first call, gn-pro on second. + // The handler should re-invoke it per request, so we end up + // reading from a different theme directory on each call. + calls := 0 + mux := http.NewServeMux() + if err := Mount(mux, "/themes", Deps{ + ThemeDir: root, + ActiveResolver: func() string { + calls++ + if calls == 1 { + return "gn-hello" + } + return "gn-pro" + }, + }); err != nil { + t.Fatalf("Mount: %v", err) + } + + // First request: /themes/active/style.css resolves to gn-hello. + req1 := httptest.NewRequest(http.MethodGet, "/themes/active/style.css", nil) + rec1 := httptest.NewRecorder() + mux.ServeHTTP(rec1, req1) + if rec1.Code != http.StatusOK { + t.Fatalf("first request: status %d, want 200, body=%q", rec1.Code, rec1.Body.String()) + } + if !strings.Contains(rec1.Body.String(), "#F5F2EA") { + t.Errorf("first request: expected gn-hello CSS, got %q", rec1.Body.String()) + } + + // Second request: same URL, resolver now returns gn-pro. + req2 := httptest.NewRequest(http.MethodGet, "/themes/active/style.css", nil) + rec2 := httptest.NewRecorder() + mux.ServeHTTP(rec2, req2) + if rec2.Code != http.StatusOK { + t.Fatalf("second request: status %d, want 200, body=%q", rec2.Code, rec2.Body.String()) + } + if !strings.Contains(rec2.Body.String(), "#ffffff") { + t.Errorf("second request: expected gn-pro CSS, got %q", rec2.Body.String()) + } + + if calls != 2 { + t.Errorf("resolver call count: got %d, want 2", calls) + } +} + +func TestServe_ActiveResolverEmpty404(t *testing.T) { + root := newTestTree(t) + mux := http.NewServeMux() + if err := Mount(mux, "/themes", Deps{ + ThemeDir: root, + ActiveResolver: func() string { return "" }, + }); err != nil { + t.Fatalf("Mount: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/themes/active/style.css", nil) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + if rec.Code != http.StatusNotFound { + t.Errorf("status: got %d, want 404", rec.Code) + } +} + +func TestServe_MissingFile404(t *testing.T) { + root := newTestTree(t) + h := mountTest(t, root, "gn-hello") + + req := httptest.NewRequest(http.MethodGet, "/themes/gn-hello/does-not-exist.css", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusNotFound { + t.Errorf("status: got %d, want 404", rec.Code) + } +} + +func TestServe_DirectoryReturns404(t *testing.T) { + root := newTestTree(t) + h := mountTest(t, root, "gn-hello") + + // /themes/gn-hello/parts (no trailing file) — the path resolves + // to a directory on disk. We never serve directory listings. + // The mux may or may not route a no-file path to our handler + // depending on how the {file...} wildcard treats an empty + // remainder; if it 404s before us, that's also fine. + req := httptest.NewRequest(http.MethodGet, "/themes/gn-hello/parts/", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code == http.StatusOK { + t.Errorf("expected non-200, got 200 body=%q", rec.Body.String()) + } +} + +func TestServe_InvalidSlugRejected(t *testing.T) { + root := newTestTree(t) + h := mountTest(t, root, "gn-hello") + + // Slug with characters outside the kebab pattern: "GnHello" is + // rejected because the regex requires leading lowercase. + req := httptest.NewRequest(http.MethodGet, "/themes/GnHello/style.css", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusBadRequest { + t.Errorf("status: got %d, want 400", rec.Code) + } +} + +func TestMount_RequiresThemeDir(t *testing.T) { + mux := http.NewServeMux() + err := Mount(mux, "/themes", Deps{ + ActiveResolver: func() string { return "" }, + }) + if err == nil { + t.Fatalf("expected error when ThemeDir is empty") + } +} + +func TestMount_RequiresActiveResolver(t *testing.T) { + mux := http.NewServeMux() + err := Mount(mux, "/themes", Deps{ + ThemeDir: t.TempDir(), + }) + if err == nil { + t.Fatalf("expected error when ActiveResolver is nil") + } +} + +func TestIsSafeRelPath(t *testing.T) { + cases := []struct { + in string + ok bool + }{ + {"style.css", true}, + {"parts/header.css", true}, + {"a/b/c.css", true}, + {"", false}, + {"/abs/path.css", false}, + {"..", false}, + {"../etc/passwd", false}, + {"sub/../etc/passwd", false}, + {`sub\..\etc\passwd`, false}, + {"sub/with\x00null.css", false}, + } + for _, tc := range cases { + got := isSafeRelPath(tc.in) + if got != tc.ok { + t.Errorf("isSafeRelPath(%q) = %v, want %v", tc.in, got, tc.ok) + } + } +} + +func TestContentType(t *testing.T) { + cases := map[string]string{ + "x.css": "text/css; charset=utf-8", + "x.js": "application/javascript; charset=utf-8", + "x.mjs": "application/javascript; charset=utf-8", + "x.json": "application/json; charset=utf-8", + "x.svg": "image/svg+xml", + "x.woff2": "font/woff2", + "x.png": "image/png", + } + for in, want := range cases { + if got := contentType(in); got != want { + t.Errorf("contentType(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/apps/web/src/app/layout.tsx b/apps/web/src/app/layout.tsx index 17221df3..8c3c5bcf 100644 --- a/apps/web/src/app/layout.tsx +++ b/apps/web/src/app/layout.tsx @@ -36,6 +36,17 @@ import { } from 'next/font/google'; import './globals.css'; +// Public-site API base — same env var the typed client in src/lib/api.ts +// reads. We resolve it at module init (the value is baked into the +// build because NEXT_PUBLIC_* is browser-visible) so the tag we +// inject in renders with the right absolute URL. The link +// preloads the active theme's CSS so the renderer no longer paints +// with stock globals.css defaults — the API now serves +// /themes/active/style.css (issue #501). +const PUBLIC_API_URL: string = + (typeof process !== 'undefined' && process.env.NEXT_PUBLIC_API_URL) || + 'http://localhost:8080'; + const archivo = Archivo({ subsets: ['latin'], weight: ['500', '600', '700', '800', '900'], @@ -95,6 +106,19 @@ export default function RootLayout({ return ( + + {/* + * Active-theme CSS preload (issue #501). The API serves the + * theme's static stylesheet under /themes/active/style.css — + * the "active" sentinel resolves through the core.active_theme + * options row, so switching the active theme transparently + * swaps the bytes returned here. + */} + + {children} ); From 8e9835dd09610edf8d49a04725e2b2b899d56e56 Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:52:25 +0200 Subject: [PATCH 12/15] chore(dev): docker-compose.override.example.yml + gitignore working override (#517) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Working docker-compose.override.yml is generated by each developer locally (postgres port override, admin build-args, aligned auth secrets, custom entrypoints). It is intentionally untracked so each operator can set their own secrets — committing the file would multiply the same dev secrets across every developer's machine and set a bad precedent. - .gitignore: ignore docker-compose.override.yml at repo root. - docker-compose.override.example.yml: template with the exact same structure + placeholder secrets + inline comments explaining each override. - README: a "Local dev setup" subsection documenting the copy step. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- .gitignore | 6 ++ README.md | 14 +++++ docker-compose.override.example.yml | 97 +++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+) create mode 100644 docker-compose.override.example.yml diff --git a/.gitignore b/.gitignore index 221cfa9d..bd30d290 100644 --- a/.gitignore +++ b/.gitignore @@ -62,3 +62,9 @@ cli/gonext/gonext .dev/ tmp/ *.local + +# Personal Compose override. Ships as docker-compose.override.example.yml +# in the repo — copy it locally and tweak. Compose auto-loads +# docker-compose.override.yml without naming it on the CLI, so we must +# keep the live file out of git. +docker-compose.override.yml diff --git a/README.md b/README.md index 564e2bb7..0763057b 100644 --- a/README.md +++ b/README.md @@ -58,6 +58,20 @@ To stop everything (volumes preserved): `make down`. To wipe state and start ove --- +## Local dev setup + +Before `make up`, copy the personal Compose override into place. It pins the admin build args, remaps Postgres to `5433` to dodge native-Postgres clashes, and aligns the `GONEXT_AUTH_*` secrets across `api` / `worker` / `migrate` so sessions and password hashes line up. + +```bash +cp docker-compose.override.example.yml docker-compose.override.yml +# Optionally edit the auth secrets — defaults are dev-only and safe to keep +docker compose up -d +``` + +The live `docker-compose.override.yml` is gitignored so each contributor keeps their own. See [Port conflicts](#port-conflicts-the-docker-composeoverrideyml-pattern) below for the full rationale. + +--- + ## Local development tips A few things that bit us when we ran this for the first time — fix them up front and the stack just works. diff --git a/docker-compose.override.example.yml b/docker-compose.override.example.yml new file mode 100644 index 00000000..b221c77e --- /dev/null +++ b/docker-compose.override.example.yml @@ -0,0 +1,97 @@ +# docker-compose.override.example.yml +# +# Template for the personal docker-compose.override.yml that Compose +# auto-loads alongside docker-compose.yml + docker-compose.dev.yml. The +# live override is gitignored so each contributor can tweak ports and +# secrets without polluting the repo. +# +# Usage: +# cp docker-compose.override.example.yml docker-compose.override.yml +# # (optional) edit GONEXT_AUTH_* secrets — the placeholders below are +# # dev-only and safe to keep as-is for a laptop stack. +# docker compose up -d +# +# What this file does: +# - Remaps the Postgres host port to 5433 so a native Postgres on 5432 +# keeps working. +# - Builds the admin image with NEXT_PUBLIC_API_URL="" (same-origin +# proxy via Next.js rewrites) and bakes the docker-internal API URL +# into the rewrite destination at build time. +# - Pins matching GONEXT_AUTH_* secrets across api/worker/migrate so +# sessions issued by api validate in worker and migrate uses the same +# pepper when it bootstraps users. + +services: + # Republish Postgres on 5433 to avoid clashing with a native Postgres + # running on the host's 5432. Connect IDEs / `psql` to localhost:5433. + postgres: + ports: + - "5433:5432" + + # Admin (Next.js, :3001) proxies /api/* through its own rewrite to + # avoid CORS. NEXT_PUBLIC_API_URL must be "" so the client uses + # same-origin paths; GONEXT_API_URL points the build-time rewrite at + # the docker-internal api service. The entrypoint override runs the + # standalone server.js directly (the upstream image's CMD is wrong + # for the standalone output layout). + admin: + build: + args: + NEXT_PUBLIC_API_URL: "" + # Build-time arg: rewrites() evaluates at build time and bakes + # this URL into routes-manifest.json. Must be the docker-internal + # api hostname so server-side proxy hops work. + GONEXT_API_URL: http://api:8080 + environment: + NEXT_PUBLIC_API_URL: "" + GONEXT_API_URL: http://api:8080 + entrypoint: ["node"] + command: ["/app/apps/admin/.next/standalone/apps/admin/server.js"] + + # Public web (Next.js, :3000) runs cross-origin in dev — the browser + # hits the API directly on :8080, so NEXT_PUBLIC_API_URL must be the + # host-reachable URL. Server-side code inside the container still + # talks to api:8080 via GONEXT_API_URL. + web: + build: + args: + NEXT_PUBLIC_API_URL: http://localhost:8080 + environment: + NEXT_PUBLIC_API_URL: http://localhost:8080 + GONEXT_API_URL: http://api:8080 + + # api/worker/migrate share the same auth secrets so sessions issued by + # one service validate in the others, and migrate's `gonext init` uses + # the same pepper that api will use to verify passwords later. + # + # The values below are PLACEHOLDERS — they look like 32-byte base64 + # strings but are sentinel text, NOT real entropy. Either keep them + # (dev-only, fine on a laptop) or replace with real 32-byte base64 + # values, e.g.: + # openssl rand -base64 32 + # If you regenerate, you MUST update all three services to match, and + # wipe the Postgres volume (`docker compose down -v`) if you change + # GONEXT_AUTH_PEPPER on an existing DB — every previously-hashed + # password becomes unverifiable. + api: + environment: + GONEXT_AUTH_PEPPER: REPLACE_WITH_RANDOM_32_BYTES_BASE64_ENCODED___SAMPLE_DEV_ONLY_AAAA== + GONEXT_AUTH_SESSION_SECRET: REPLACE_WITH_RANDOM_32_BYTES_BASE64_ENCODED___SAMPLE_DEV_ONLY_BBBB== + GONEXT_AUTH_CSRF_SECRET: REPLACE_WITH_RANDOM_32_BYTES_BASE64_ENCODED___SAMPLE_DEV_ONLY_CCCC== + + # Worker consumes Asynq jobs queued by api; if its session secret + # drifts from api's, signed payloads stop verifying. + worker: + environment: + GONEXT_AUTH_PEPPER: REPLACE_WITH_RANDOM_32_BYTES_BASE64_ENCODED___SAMPLE_DEV_ONLY_AAAA== + GONEXT_AUTH_SESSION_SECRET: REPLACE_WITH_RANDOM_32_BYTES_BASE64_ENCODED___SAMPLE_DEV_ONLY_BBBB== + GONEXT_AUTH_CSRF_SECRET: REPLACE_WITH_RANDOM_32_BYTES_BASE64_ENCODED___SAMPLE_DEV_ONLY_CCCC== + + # Migrate is a one-shot that runs `gonext init` to bootstrap the first + # admin user. It must hash the admin password with the same pepper + # that api will later HMAC into login attempts. + migrate: + environment: + GONEXT_AUTH_PEPPER: REPLACE_WITH_RANDOM_32_BYTES_BASE64_ENCODED___SAMPLE_DEV_ONLY_AAAA== + GONEXT_AUTH_SESSION_SECRET: REPLACE_WITH_RANDOM_32_BYTES_BASE64_ENCODED___SAMPLE_DEV_ONLY_BBBB== + GONEXT_AUTH_CSRF_SECRET: REPLACE_WITH_RANDOM_32_BYTES_BASE64_ENCODED___SAMPLE_DEV_ONLY_CCCC== From 56e9345226eeace2d1bd70bf8d20f3c4efd4e036 Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 10:54:12 +0200 Subject: [PATCH 13/15] fix(admin): resolve TS errors hidden by ignoreBuildErrors flag (#518) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit next.config.ts no longer carries \`typescript: { ignoreBuildErrors: true }\`, so the 12 pre-existing errors that flag was hiding now fail \`next build\` and CI's \`pnpm lint\`. Address them all here. - media/types.ts: \`MediaAsset\` was missing four fields the detail view (\`MediaDetailClient.tsx\`) actually reads — \`hls_url\`, \`source_url\`, \`is_proxied\`, \`has_extracted_text\`. Added as optional per the on-wire shape (worker pipeline #476 populates them on a subset of asset types). Resolves 11/12 errors across MediaDetailClient.tsx and its test fixture. - appearance/menus/MenusClient.tsx:157: \`Array.splice\` returns \`T[]\` so the destructured element typechecks as \`T | undefined\` under noUncheckedIndexedAccess. Added an explicit guard before re-inserting so the array can't get a stray undefined. \`pnpm exec tsc --noEmit\` is now clean. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- .../appearance/menus/MenusClient.tsx | 9 +++++++ .../src/app/(authenticated)/media/types.ts | 24 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/apps/admin/src/app/(authenticated)/appearance/menus/MenusClient.tsx b/apps/admin/src/app/(authenticated)/appearance/menus/MenusClient.tsx index 9813c86a..529893dc 100644 --- a/apps/admin/src/app/(authenticated)/appearance/menus/MenusClient.tsx +++ b/apps/admin/src/app/(authenticated)/appearance/menus/MenusClient.tsx @@ -154,6 +154,15 @@ export function MenusClient({ initialMenus }: Props): ReactElement { return; } const [moved] = ordered.splice(fromIdx, 1); + // `splice` returns `T[]` so the destructure is typed `T | + // undefined` under `noUncheckedIndexedAccess`. fromIdx ≥ 0 was + // checked above, so moved is in practice always defined — but + // the type-checker doesn't know that. Guard explicitly so the + // re-insert doesn't push `undefined` into the array. + if (!moved) { + setDragId(''); + return; + } ordered.splice(toIdx, 0, moved); // Re-stamp paths as flat root-level slots — the drag-drop surface // here only supports a single nesting level for now. diff --git a/apps/admin/src/app/(authenticated)/media/types.ts b/apps/admin/src/app/(authenticated)/media/types.ts index fc7c2a2e..c75a6a83 100644 --- a/apps/admin/src/app/(authenticated)/media/types.ts +++ b/apps/admin/src/app/(authenticated)/media/types.ts @@ -39,6 +39,30 @@ export interface MediaAsset { collection_id?: string; /** Lowercase, deduplicated tag list. Never null; defaults to []. Issue #71. */ tags: string[]; + /** + * HLS playlist URL for transcoded videos. Populated by the worker's + * video pipeline (#476). Absent for non-video assets and for videos + * that haven't completed transcode yet. + */ + hls_url?: string; + /** + * Original source URL for proxied media (the URL the operator + * imported FROM, kept for attribution + re-fetch on cache miss). + * Absent for media uploaded directly. Issue #476 follow-up. + */ + source_url?: string; + /** + * True when the binary is fetched through the GoNext proxy rather + * than served directly from object storage. Used by the detail UI + * to surface a "proxied" badge. Absent on direct uploads. + */ + is_proxied?: boolean; + /** + * True when the PDF/document text-extraction pipeline (#476) has + * indexed this asset for search. Absent on assets that don't carry + * extractable text (raw images, audio). + */ + has_extracted_text?: boolean; } /** From fe6303074ae45b3bbfcf96d9e77b4cfe743e5ee4 Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 11:04:27 +0200 Subject: [PATCH 14/15] fix(cli): de-duplicate audit subcommand declarations (lint-go blocker) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cli/gonext/cmd/audit/audit.go and verify.go both declared the same package-level symbols — ExitOK, ExitFail, ExitUsage, usage, Run, RunOS — because the verify.go landed in PR #492 carrying its own copy of the entry-point boilerplate that was already in audit.go. \`go vet\` rejects this with \"X redeclared in this block\" and CI lint-go has been red on main since the merge. Make audit.go the single owner of the package surface: - Add \`case \"verify\": return runVerify(args[1:], stdout, stderr)\` to the dispatch switch. - Extend the usage banner with the verify subcommand and its env requirement (GONEXT_AUDIT_HMAC_KEY). Strip verify.go down to just \`runVerify\` and its imports — everything else (exit codes, Run, RunOS, usage) is owned by audit.go. go vet ./... clean on cli/gonext after this change. cli/gonext/cmd/ audit unit tests still pass. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- cli/gonext/cmd/audit/audit.go | 8 +++- cli/gonext/cmd/audit/verify.go | 72 +++++++--------------------------- 2 files changed, 21 insertions(+), 59 deletions(-) diff --git a/cli/gonext/cmd/audit/audit.go b/cli/gonext/cmd/audit/audit.go index c5631612..873f3883 100644 --- a/cli/gonext/cmd/audit/audit.go +++ b/cli/gonext/cmd/audit/audit.go @@ -27,6 +27,8 @@ func Run(args []string, stdout, stderr io.Writer) int { return ExitOK case "tail": return runTail(args[1:], stdout, stderr) + case "verify": + return runVerify(args[1:], stdout, stderr) default: fmt.Fprintf(stderr, "gonext audit: unknown subcommand %q\n\n%s\n", args[0], usage) return ExitUsage @@ -42,9 +44,11 @@ Usage: gonext audit [args] Subcommands: - tail [flags] Print the most recent audit events. Tail by default. + tail [flags] Print the most recent audit events. + verify [--from] [--to] Walk the HMAC chain and report tampered rows. Run 'gonext audit tail --help' for the tail-specific flags. Environment: - DATABASE_URL Required. Postgres DSN for the GoNext install.` + DATABASE_URL Required. Postgres DSN for the GoNext install. + GONEXT_AUDIT_HMAC_KEY Required for ` + "`verify`" + `. HMAC chain key.` diff --git a/cli/gonext/cmd/audit/verify.go b/cli/gonext/cmd/audit/verify.go index 54a0a93b..073a7082 100644 --- a/cli/gonext/cmd/audit/verify.go +++ b/cli/gonext/cmd/audit/verify.go @@ -1,9 +1,10 @@ -// Package audit provides the `gonext audit` CLI subcommand surface. -// At present it exposes a single `verify` subcommand that walks the -// audit_log HMAC chain and reports the first tampered row (if any). +// Package audit — `verify` subcommand implementation. The shared +// entry point (Run, RunOS, exit codes, usage banner) lives in +// audit.go; this file contributes only the verify-specific +// subcommand handler that the dispatcher calls into. // -// Wiring: cli/gonext/main.go dispatches `audit ` here via -// RunOS. The package is intentionally thin — heavy lifting lives in +// Verify walks the audit_log HMAC chain and reports the first +// tampered row (if any). Heavy lifting lives in // packages/go/audit.VerifyChain. package audit @@ -13,7 +14,6 @@ import ( "flag" "fmt" "io" - "os" "github.com/jackc/pgx/v5/pgxpool" @@ -21,57 +21,15 @@ import ( "github.com/Singleton-Solution/GoNext/packages/go/config" ) -// Exit codes shared with the rest of the CLI. -const ( - ExitOK = 0 - ExitFail = 1 - ExitUsage = 2 -) - -const usage = `gonext audit — inspect the audit log - -Usage: - gonext audit verify [--from=ID] [--to=ID] - -Subcommands: - verify Walk the HMAC chain and report any tampered rows. - -Environment: - DATABASE_URL Required. Postgres DSN. - GONEXT_AUDIT_HMAC_KEY Required. The HMAC chain key (raw bytes or hex). - See packages/go/audit/chain.go. - -Exit codes: - 0 Chain verified intact. - 1 Chain broken (a tamper was detected) or another runtime error. - 2 Misuse: missing subcommand, malformed flags, etc. -` - -// RunOS is the os.Args / os.Stdout / os.Stderr entry point dispatched -// from cli/gonext/main.go. Returns the process exit code. -func RunOS(args []string) int { - return Run(args, os.Stdout, os.Stderr) -} - -// Run is the testable entry point: callers pass their own writers and -// args so the package can be exercised without poking at os.Args. -func Run(args []string, stdout, stderr io.Writer) int { - if len(args) == 0 { - fmt.Fprint(stderr, usage) - return ExitUsage - } - switch args[0] { - case "verify": - return runVerify(args[1:], stdout, stderr) - case "help", "--help", "-h": - fmt.Fprint(stdout, usage) - return ExitOK - default: - fmt.Fprintf(stderr, "gonext audit: unknown subcommand %q\n\n%s\n", args[0], usage) - return ExitUsage - } -} - +// runVerify handles `gonext audit verify`. Returns the desired +// process exit code (one of the Exit* constants from audit.go). +// +// Flag set: +// --from=ID inclusive lower-bound row ID, default first row +// --to=ID inclusive upper-bound row ID, default last row +// +// Required env: DATABASE_URL, GONEXT_AUDIT_HMAC_KEY (see +// packages/go/audit/chain.go for the key format). func runVerify(args []string, stdout, stderr io.Writer) int { fs := flag.NewFlagSet("verify", flag.ContinueOnError) fs.SetOutput(stderr) From 6e3c845f816dc657286c952614c20e54765ba215 Mon Sep 17 00:00:00 2001 From: Tayeb Mokni Date: Thu, 28 May 2026 11:25:44 +0200 Subject: [PATCH 15/15] fix(api): strip non-existent \`namespace\` column from options upserts (#524) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three writer SQLs INSERTed into a \`namespace\` column that the live schema doesn't have. Migration 000008_options.up.sql defines: CREATE TABLE options ( key, value, autoload, is_protected, created_at, updated_at, version ); No \`namespace\`. Live INSERT reproduces: ERROR: column "namespace" of relation "options" does not exist So every admin Save through the Settings tab, theme activate, and customizer override Save returned 500 the moment they hit Postgres. GET worked because SELECT never referenced the column. Drop \`namespace\` from all three upserts: - packages/go/settings/postgres.go::upsertSQL - apps/api/internal/admin/themes/store.go::writeActiveSQL - apps/api/internal/admin/customizer/store.go::writeOverridesSQL Also drop the explicit \`updated_at = now()\` — the table default covers INSERT and the options_touch trigger bumps it on UPDATE. The \`namespaceFor\` helper in settings/postgres.go becomes unused but is retained with a comment so the per-plugin uninstall sweep (which will ship a migration adding the column) can pass it back in. The package-level docstring is corrected to match the actual migration schema. Update TestPostgresStore_WriteValidatesAndUpserts: - args[3] namespace assertion removed (panic'd index-out-of-range after the SQL change) - assert arg count == 3 (key, value, autoload) - assert the SQL string itself does NOT contain "namespace" — a regression guard so a future re-introduction blows up in unit tests before it ever hits Postgres again End-to-end verified live: $ curl -X PATCH -d '{"core.site.name":"Verified Live Save"}' \ http://localhost:8080/api/v1/settings HTTP 200 $ psql -c "SELECT value FROM options WHERE key='core.site.name'" "Verified Live Save" Same fix applies to the themes activate + customizer save endpoints even though the bug was identified through #499's PATCH path. Closes #524. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Tayeb Mokni --- apps/api/internal/admin/customizer/store.go | 16 +++--- apps/api/internal/admin/themes/store.go | 13 +++-- packages/go/settings/postgres.go | 54 ++++++++++++++------- packages/go/settings/postgres_test.go | 14 +++++- 4 files changed, 67 insertions(+), 30 deletions(-) diff --git a/apps/api/internal/admin/customizer/store.go b/apps/api/internal/admin/customizer/store.go index cefcf836..97d55d27 100644 --- a/apps/api/internal/admin/customizer/store.go +++ b/apps/api/internal/admin/customizer/store.go @@ -65,14 +65,18 @@ const readOverridesSQL = `SELECT value FROM options WHERE key = $1` // writeOverridesSQL upserts the overrides row. autoload is FALSE // because the renderer reads via the per-request settings store cache // path; autoloading at boot would prime cache with values the API-only -// replicas never need. namespace is "core" so the row is uninstall-safe -// (plugin-namespace deletes don't sweep it). +// replicas never need. +// +// `namespace` and explicit `updated_at` are omitted: the options +// table from migration 000008 has neither (`updated_at` defaults to +// now() and is bumped by the options_touch trigger). An earlier +// draft of this writer referenced both; PATCH was 500'ing in +// production before the fix. const writeOverridesSQL = ` -INSERT INTO options (key, value, autoload, namespace, updated_at) -VALUES ($1, $2, FALSE, 'core', now()) +INSERT INTO options (key, value, autoload) +VALUES ($1, $2, FALSE) ON CONFLICT (key) DO UPDATE - SET value = EXCLUDED.value, - updated_at = now() + SET value = EXCLUDED.value ` // deleteOverridesSQL clears the overrides row for a slug. Used by the diff --git a/apps/api/internal/admin/themes/store.go b/apps/api/internal/admin/themes/store.go index b40f4509..bb7d8d04 100644 --- a/apps/api/internal/admin/themes/store.go +++ b/apps/api/internal/admin/themes/store.go @@ -46,12 +46,17 @@ const readActiveSQL = `SELECT value #>> '{}' FROM options WHERE key = $1` // literal ("gn-hello" → JSON value "gn-hello"). autoload is TRUE // because every renderer wakeup needs this key — keeping it in the // autoload set avoids one cache miss per cold boot. +// +// `namespace` and explicit `updated_at` are omitted: the options +// table from migration 000008 has neither (`updated_at` defaults to +// now() and is bumped by the options_touch trigger). An earlier +// draft referenced both columns; live INSERTs against this SQL were +// returning "column namespace does not exist" before the fix. const writeActiveSQL = ` -INSERT INTO options (key, value, autoload, namespace, updated_at) -VALUES ($1, to_jsonb($2::text), TRUE, 'core', now()) +INSERT INTO options (key, value, autoload) +VALUES ($1, to_jsonb($2::text), TRUE) ON CONFLICT (key) DO UPDATE - SET value = EXCLUDED.value, - updated_at = now() + SET value = EXCLUDED.value ` // PgxActiveStore is the production ActiveStore backed by pgx. diff --git a/packages/go/settings/postgres.go b/packages/go/settings/postgres.go index a9328f49..c5d111db 100644 --- a/packages/go/settings/postgres.go +++ b/packages/go/settings/postgres.go @@ -70,13 +70,21 @@ func (t tagWrapper) RowsAffected() int64 { return t.rows } // docs/01-core-cms.md §10.11: // // CREATE TABLE options ( -// key TEXT PRIMARY KEY, +// key CITEXT PRIMARY KEY, // value JSONB NOT NULL, // autoload BOOLEAN NOT NULL DEFAULT FALSE, -// namespace TEXT NOT NULL DEFAULT 'core', -// updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +// is_protected BOOLEAN NOT NULL DEFAULT FALSE, +// created_at TIMESTAMPTZ NOT NULL DEFAULT now(), +// updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), +// version INTEGER NOT NULL DEFAULT 1 // ); // +// The actual migration is 000008. Earlier drafts of this doc + the +// upsertSQL referenced a `namespace` column that never made it into +// the migration; the writer was 500'ing in production until the +// non-existent column was stripped. See `namespaceFor` below for +// the un-shipped per-plugin uninstall convention. +// // The store maintains an L1 cache (a sync.Map keyed by setting key) // so the hot Read path doesn't round-trip to Postgres. Write // invalidates the relevant entry as part of the persistence path — @@ -130,18 +138,21 @@ func NewPostgresStoreWithQuerier(q PgxQuerier, reg *Registry) *PostgresStore { // read path doesn't need it. const readSQL = `SELECT value FROM options WHERE key = $1` -// upsertSQL writes a value, defaulting the namespace and autoload -// columns from the registry. Postgres's ON CONFLICT lets us keep this -// single statement instead of branching SELECT-then-INSERT-or-UPDATE -// on the caller side, which also avoids the lost-update race window. +// upsertSQL writes a value with its autoload bit from the registry. +// Postgres's ON CONFLICT lets us keep this single statement instead +// of branching SELECT-then-INSERT-or-UPDATE on the caller side, which +// also avoids the lost-update race window. +// +// updated_at is intentionally omitted from the INSERT column list: +// the options_touch trigger (migration 000008) sets it on every +// UPDATE, and the column default (now()) covers the INSERT case. +// Setting it explicitly here would still work but is dead code. const upsertSQL = ` -INSERT INTO options (key, value, autoload, namespace, updated_at) -VALUES ($1, $2, $3, $4, now()) +INSERT INTO options (key, value, autoload) +VALUES ($1, $2, $3) ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value, - autoload = EXCLUDED.autoload, - namespace = EXCLUDED.namespace, - updated_at = now() + autoload = EXCLUDED.autoload ` // autoloadSQL fetches every key with autoload = true. The boot path @@ -207,8 +218,7 @@ func (s *PostgresStore) Write(ctx context.Context, key string, value any) error return fmt.Errorf("settings: marshal %q: %w", key, err) } - namespace := namespaceFor(key) - tag, err := s.db.Exec(ctx, upsertSQL, key, encoded, entry.Setting.Autoload, namespace) + tag, err := s.db.Exec(ctx, upsertSQL, key, encoded, entry.Setting.Autoload) if err != nil { // On Exec failure, the row may or may not have been written. // We invalidate the cache rather than seed it — the operator @@ -375,11 +385,19 @@ func (s *PostgresStore) cacheGet(key string) (any, bool) { return v, ok } -// namespaceFor returns the namespace column value for a setting key. +// namespaceFor was used to populate the `options.namespace` column. +// That column does not exist in migration 000008 (the live schema); +// the column reference was unreviewed prose that shipped as code. +// The helper is intentionally retained — when the per-plugin uninstall +// sweep ships its own migration adding `namespace`, the writer can +// pass `namespaceFor(key)` back into the upsert without reinventing +// the convention. Keep this in lockstep with the docstring at the +// top of this file so plugin authors can predict what column they +// land under. +// // Convention: "core.*" → "core"; "plugin:.*" or ".*" → -// "plugin:". The function is intentionally conservative — -// anything we can't classify gets the safe default "core" rather than -// a guessed namespace. +// "plugin:". Conservative — anything we can't classify becomes +// "core" rather than a guessed namespace. func namespaceFor(key string) string { // Core keys. if len(key) >= 5 && key[:5] == "core." { diff --git a/packages/go/settings/postgres_test.go b/packages/go/settings/postgres_test.go index 23ee8d85..3155ffb3 100644 --- a/packages/go/settings/postgres_test.go +++ b/packages/go/settings/postgres_test.go @@ -320,6 +320,13 @@ func TestPostgresStore_WriteValidatesAndUpserts(t *testing.T) { t.Fatalf("expected 1 INSERT, got %d", len(upserts)) } args := upserts[0].args + // Three positional args: key, value (JSON-encoded), autoload. + // An earlier version of upsertSQL had a 4th `namespace` arg and a + // 5th explicit `updated_at = now()`; both were dropped when the + // non-existent `namespace` column made live writes 500. + if got := len(args); got != 3 { + t.Fatalf("upsert arg count: got %d want 3 (key, value, autoload)", got) + } if args[0].(string) != "core.site.name" { t.Errorf("upsert key arg: got %v", args[0]) } @@ -329,8 +336,11 @@ func TestPostgresStore_WriteValidatesAndUpserts(t *testing.T) { if args[2].(bool) != true { t.Errorf("upsert autoload arg: got %v want true", args[2]) } - if args[3].(string) != "core" { - t.Errorf("upsert namespace arg: got %v want core", args[3]) + // Regression guard: the SQL string itself must not reference the + // non-existent `namespace` column. Catches a re-introduction + // before it hits Postgres. + if strings.Contains(upserts[0].sql, "namespace") { + t.Errorf("upsert SQL must not reference `namespace` column: %s", upserts[0].sql) } }