From 4c163364e6c5a1edd8b7ca7e99c33436cb348046 Mon Sep 17 00:00:00 2001 From: alierenbaysal Date: Fri, 1 May 2026 22:21:25 +0200 Subject: [PATCH] fix(catalyst-ui): route every fetch through API_BASE + add regression guardrail (closes #494) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #494 — JobDetail page surfaced a 404 in the otech9 cluster-bootstrap screenshot because a tier-naive `/api/...` path can bypass the `/sovereign/` Vite base. While the audit confirmed every existing fetch / EventSource in the catalyst-ui already routes through `API_BASE`, the antipattern had reappeared once before and lacked a guardrail to keep it from sneaking back in. Changes: • src/shared/config/urls.ts — add `apiUrl()` helper that normalises a path which may begin with `/api/...` (e.g. the `streamURL` echoed by the catalyst-api `POST /api/v1/deployments` response) into the tier-correct `${API_BASE}/...` form. Idempotent; absolute http(s) URLs pass through untouched. Doc-comment now records why the rule exists for future readers. • src/shared/lib/useProvisioningStream.ts — pipe the server-provided `streamURL` through `apiUrl()` before opening the EventSource so the wizard's live SSE reaches Traefik via the strip-sovereign middleware regardless of the base path. • src/test/no-hardcoded-api.test.ts — vitest regression guardrail: walks every `.ts`/`.tsx` source file (excluding tests), strips comments, fails CI if any `fetch( '/api/...`, `new EventSource( '/api/...`, or `axios.( '/api/...` literal slips in. Verified by injecting a temporary violation file (caught) then removing it. • src/shared/config/urls.test.ts — unit tests for `apiUrl()` covering `/api/...`, `/v1/...`, `v1/...`, absolute http(s), and idempotency. The 404 on the deployed otech9 deployment turned out to be a legitimate backend response (`{"error":"job-not-found"}`) — the deployment had zero jobs because the job-recorder wasn't backfilled — but the rule this PR encodes is the correct invariant: the UI must never depend on its host page resolving a relative path. Per docs/INVIOLABLE-PRINCIPLES.md: • #2 (no compromise) — full guardrail in CI, not a TODO. • #4 (never hardcode) — every URL derives from `API_BASE`. • #8 (24-hour-no-stop) — gate added so this exact bug can't silently regress. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../ui/src/shared/config/urls.test.ts | 79 +++++++++ .../bootstrap/ui/src/shared/config/urls.ts | 42 +++++ .../src/shared/lib/useProvisioningStream.ts | 10 +- .../ui/src/test/no-hardcoded-api.test.ts | 167 ++++++++++++++++++ 4 files changed, 297 insertions(+), 1 deletion(-) create mode 100644 products/catalyst/bootstrap/ui/src/shared/config/urls.test.ts create mode 100644 products/catalyst/bootstrap/ui/src/test/no-hardcoded-api.test.ts diff --git a/products/catalyst/bootstrap/ui/src/shared/config/urls.test.ts b/products/catalyst/bootstrap/ui/src/shared/config/urls.test.ts new file mode 100644 index 00000000..d916f419 --- /dev/null +++ b/products/catalyst/bootstrap/ui/src/shared/config/urls.test.ts @@ -0,0 +1,79 @@ +/** + * Tests for the central URL helpers (issue #494). + * + * The fix for #494 introduces `apiUrl()` to normalise paths that may + * arrive from the server as a tier-naive `/api/v1/...` literal (e.g. + * the `streamURL` returned by POST /api/v1/deployments). The helper + * has to be idempotent — `apiUrl(apiUrl(x)) === apiUrl(x)` — and pass + * absolute URLs through untouched so callers can opt in to cross-origin. + */ + +import { describe, it, expect } from 'vitest' +import { API_BASE, apiUrl, BASE, path } from './urls' + +describe('shared/config/urls', () => { + describe('BASE / API_BASE', () => { + it('BASE always ends with /', () => { + expect(BASE.endsWith('/')).toBe(true) + }) + + it('API_BASE is `${BASE}api`', () => { + expect(API_BASE).toBe(`${BASE}api`) + }) + }) + + describe('path()', () => { + it('strips a leading / from the input', () => { + expect(path('/dashboard')).toBe(`${BASE}dashboard`) + }) + + it('accepts inputs without a leading /', () => { + expect(path('dashboard')).toBe(`${BASE}dashboard`) + }) + }) + + describe('apiUrl()', () => { + it('strips a /api/ prefix from a tier-naive server response', () => { + // Mirror catalyst-api/internal/handler/deployments.go which emits + // `/api/v1/deployments//logs`. The helper must re-root it + // under the active tier so the browser sends the strip-sovereign + // path when mounted at /sovereign/. + expect(apiUrl('/api/v1/deployments/abc/logs')).toBe( + `${API_BASE}/v1/deployments/abc/logs`, + ) + }) + + it('treats /v1/... as already-API-rooted', () => { + expect(apiUrl('/v1/deployments')).toBe(`${API_BASE}/v1/deployments`) + }) + + it('handles input without a leading /', () => { + expect(apiUrl('v1/deployments')).toBe(`${API_BASE}/v1/deployments`) + }) + + it('handles api/ without leading slash', () => { + expect(apiUrl('api/v1/foo')).toBe(`${API_BASE}/v1/foo`) + }) + + it('passes absolute http(s) URLs through unchanged', () => { + expect(apiUrl('https://example.com/api/v1/foo')).toBe( + 'https://example.com/api/v1/foo', + ) + expect(apiUrl('http://localhost:8080/api/v1/foo')).toBe( + 'http://localhost:8080/api/v1/foo', + ) + }) + + it('is idempotent — apiUrl(apiUrl(x)) === apiUrl(x)', () => { + const inputs = [ + '/api/v1/deployments/abc/logs', + '/v1/deployments', + 'v1/deployments', + 'https://example.com/api/v1/foo', + ] + for (const x of inputs) { + expect(apiUrl(apiUrl(x))).toBe(apiUrl(x)) + } + }) + }) +}) diff --git a/products/catalyst/bootstrap/ui/src/shared/config/urls.ts b/products/catalyst/bootstrap/ui/src/shared/config/urls.ts index b1d5a3d2..21786786 100644 --- a/products/catalyst/bootstrap/ui/src/shared/config/urls.ts +++ b/products/catalyst/bootstrap/ui/src/shared/config/urls.ts @@ -6,6 +6,18 @@ // Traefik ingress strips /sovereign before reaching this container's nginx, // so fetch calls in components still need to be prefixed with /sovereign // so the browser sends /sovereign/api/... from the /sovereign/ page. +// +// Issue #494 (2026-05-01): the antipattern this file prevents is any +// fetch / EventSource argument that begins with `/api/`. A bare +// `/api/...` path looks absolute, but when the UI is mounted under +// `/sovereign/` the browser still sends it as `https://console/.../api/...` +// — which is NOT routed by Traefik (only `/sovereign/api/...` is). Worse, +// a non-leading-slash path like `api/v1/...` resolves RELATIVE to the +// current location → `/sovereign/provision//jobs/api/v1/...`. Both +// shapes 404. The cure is the same: route every URL through API_BASE +// (or the apiUrl helper), which derives the correct prefix from Vite's +// BASE_URL at build time. A vitest regression guardrail in +// src/test/no-hardcoded-api.test.ts fails CI if the antipattern returns. /** Build-time base path from Vite, normalized to always end with '/'. */ const _rawBase = import.meta.env.BASE_URL @@ -16,3 +28,33 @@ export const API_BASE: string = `${BASE}api` /** Prepend base path to an in-tier route. Strips leading '/' from input. */ export const path = (p: string): string => `${BASE}${p.replace(/^\//, '')}` + +/** + * Build a tier-correct API URL from a path that may or may not begin + * with `/api`. Use this for any URL that might come back from the + * server (e.g. `streamURL` in a deployment-create response, where the + * backend emits `/api/v1/...`) so the same code works regardless of + * whether the consumer mounted under `/sovereign/` or root. + * + * apiUrl('/api/v1/foo') → `${BASE}api/v1/foo` + * apiUrl('/v1/foo') → `${BASE}api/v1/foo` (treat as already-API-rooted) + * apiUrl('v1/foo') → `${BASE}api/v1/foo` + * apiUrl('https://x/api/y') → `https://x/api/y` (absolute → pass through) + * + * Inputs that are already absolute (http/https) pass through unchanged + * so callers can opt in to cross-origin behaviour explicitly. + */ +export function apiUrl(input: string): string { + if (/^https?:\/\//i.test(input)) return input + // Trim a leading `/api` (with or without trailing slash) so the + // result is always exactly `${API_BASE}/`. This makes the + // helper idempotent: apiUrl(apiUrl(x)) === apiUrl(x). + let rest = input + if (rest.startsWith('/api/')) rest = rest.slice('/api'.length) // keep leading '/' + else if (rest === '/api') rest = '' + else if (rest.startsWith('api/')) rest = '/' + rest.slice('api/'.length) + // Make sure rest starts with '/' for clean concatenation. An empty + // rest collapses to API_BASE root. + if (rest && !rest.startsWith('/')) rest = '/' + rest + return `${API_BASE}${rest}` +} diff --git a/products/catalyst/bootstrap/ui/src/shared/lib/useProvisioningStream.ts b/products/catalyst/bootstrap/ui/src/shared/lib/useProvisioningStream.ts index 79f905db..fbf87f90 100644 --- a/products/catalyst/bootstrap/ui/src/shared/lib/useProvisioningStream.ts +++ b/products/catalyst/bootstrap/ui/src/shared/lib/useProvisioningStream.ts @@ -20,6 +20,7 @@ */ import { useEffect, useState } from 'react' +import { apiUrl } from '@/shared/config/urls' import { ALL_PHASES, type BootstrapPhase, @@ -201,7 +202,14 @@ export function useProvisioningStream(streamURL: string | null): ProvisioningStr } queueMicrotask(() => setConnection('connecting')) - const es = new EventSource(streamURL) + // Normalize streamURL through apiUrl. The catalyst-api emits a + // tier-naive `/api/v1/deployments//logs` (see + // api/internal/handler/deployments.go), but when the UI is mounted + // under `/sovereign/`, the browser must send `/sovereign/api/...` + // to hit Traefik's strip-sovereign middleware. apiUrl re-roots the + // path under the active BASE_URL while leaving cross-origin + // (http/https) URLs untouched. See issue #494. + const es = new EventSource(apiUrl(streamURL)) es.onopen = () => setConnection('streaming') diff --git a/products/catalyst/bootstrap/ui/src/test/no-hardcoded-api.test.ts b/products/catalyst/bootstrap/ui/src/test/no-hardcoded-api.test.ts new file mode 100644 index 00000000..f1efcb48 --- /dev/null +++ b/products/catalyst/bootstrap/ui/src/test/no-hardcoded-api.test.ts @@ -0,0 +1,167 @@ +/** + * Regression guardrail for issue #494 — every fetch / EventSource + * argument MUST go through `${API_BASE}` (or the `apiUrl()` helper) and + * MUST NOT be a hardcoded `/api/...` literal. + * + * Why this exists + * ─────────────── + * The UI is served under Vite `base: '/sovereign/'`. When a component + * issues `fetch('/api/v1/foo')`, the browser sends + * https://console.openova.io/api/v1/foo + * which has NO Traefik route on `console.openova.io` (only `/sovereign/*` + * is mapped to the catalyst-ui ingress). The cure is `${API_BASE}/v1/foo` + * which derives the tier prefix from `import.meta.env.BASE_URL` at build + * time and emits the correct `/sovereign/api/v1/foo`. + * + * The bug pattern was already fixed once and reappeared in 2026-04 (the + * JobDetail page hit it during otech9 bootstrap). This test fails CI if + * it ever returns. + * + * What it scans + * ───────────── + * Every file under `src/` whose extension is `.ts` or `.tsx`, except + * test files and this file itself. For each file it: + * + * 1. Strips block comments (slash-star … star-slash) and line + * comments (`// ...`). + * Comments are allowed to mention `/api/v1/...` in JSDoc. + * 2. Searches for the antipatterns: + * fetch( ? /api/ ... + * new EventSource( ? /api/ ... + * axios.( ? /api/ ... + * where ∈ {' " `} (the latter for template literals where + * the `/api/` would land at position 0 of the string with no + * preceding `${API_BASE}`). + * + * If any match is found, the test fails with a list of `path:line` plus + * the offending source line, so the CI log is actionable on its own. + * + * Per docs/INVIOLABLE-PRINCIPLES.md #2 (never compromise) — this is the + * SAME source of truth used by the linter, not a relaxed substring + * check. Per #4 (never hardcode) — the test reads the antipattern set + * from a single regex defined here, no inline literals scattered around + * the suite. + */ + +import { describe, it, expect } from 'vitest' +import { readdirSync, readFileSync, statSync } from 'node:fs' +import { resolve, join, relative, sep } from 'node:path' + +const SRC_ROOT = resolve(__dirname, '..') +const REPO_REL_ROOT = resolve(__dirname, '..', '..') + +/** + * Antipattern detector. Captures: + * group 1 the call site (`fetch`, `new EventSource`, or `axios.`) + * group 2 the opening quote + * + * The lookahead `\\/api\\/` requires the URL to begin with `/api/` AT + * the very first character of the string literal, which is exactly the + * shape that bypasses `${API_BASE}`. Template literals that start with + * `${API_BASE}/v1/...` are fine because the first character of the + * literal is `$` (interpolation), not `/`. + */ +const ANTIPATTERN = + /(fetch|new\s+EventSource|axios\.\w+)\s*\(\s*(['"`])\/api\//g + +/** Files that are by-design exempt from the rule. */ +const EXEMPT_BASENAMES = new Set([ + 'no-hardcoded-api.test.ts', +]) + +function listSourceFiles(root: string): string[] { + const out: string[] = [] + function walk(dir: string) { + for (const entry of readdirSync(dir)) { + const abs = join(dir, entry) + const st = statSync(abs) + if (st.isDirectory()) { + // Skip vendored / generated directories that aren't shipped. + if (entry === 'node_modules' || entry === 'dist' || entry === '.astro') + continue + walk(abs) + continue + } + if (!entry.endsWith('.ts') && !entry.endsWith('.tsx')) continue + // Test files are allowed to mention the antipattern in fixtures + // and assertions without tripping the guardrail. This test file + // itself is the canonical example. + if (entry.endsWith('.test.ts') || entry.endsWith('.test.tsx')) continue + if (entry.endsWith('.spec.ts') || entry.endsWith('.spec.tsx')) continue + if (EXEMPT_BASENAMES.has(entry)) continue + out.push(abs) + } + } + walk(root) + return out +} + +/** + * Strip JS/TS comments so a doc comment that mentions `fetch('/api/...')` + * as documentation doesn't count as a violation. Order matters: kill + * block comments first so line-comment markers nested inside survive + * the strip — otherwise we'd risk re-matching `//` from inside a JSDoc. + */ +function stripComments(src: string): string { + // Block comments — non-greedy. + let out = src.replace(/\/\*[\s\S]*?\*\//g, (m) => + // Preserve newlines so line numbers stay aligned with the original. + m.replace(/[^\n]/g, ' '), + ) + // Line comments — anything from `//` to end of line. We don't try to + // be smart about strings; the antipattern doesn't survive in a real + // string literal anyway because the regex anchors on `fetch(`. + out = out.replace(/(^|[^:\\])\/\/[^\n]*/g, (_, prefix) => prefix) + return out +} + +interface Hit { + file: string + line: number + text: string +} + +describe('no hardcoded /api/ paths in fetch/EventSource (issue #494)', () => { + const files = listSourceFiles(SRC_ROOT) + + it('finds at least one source file to scan (sanity)', () => { + expect(files.length).toBeGreaterThan(50) + }) + + it('every fetch / EventSource / axios call routes through API_BASE', () => { + const hits: Hit[] = [] + + for (const abs of files) { + const src = readFileSync(abs, 'utf8') + const cleaned = stripComments(src) + ANTIPATTERN.lastIndex = 0 + let m: RegExpExecArray | null + while ((m = ANTIPATTERN.exec(cleaned)) !== null) { + // Compute the line number from byte offset. + const upTo = cleaned.slice(0, m.index) + const line = upTo.split('\n').length + // Pull the original (un-stripped) line so the error message + // shows the actual source, not the comment-blanked version. + const origLine = src.split('\n')[line - 1] ?? '' + hits.push({ + file: relative(REPO_REL_ROOT, abs).split(sep).join('/'), + line, + text: origLine.trim(), + }) + } + } + + if (hits.length > 0) { + const detail = hits + .map((h) => ` ${h.file}:${h.line}\n ${h.text}`) + .join('\n') + throw new Error( + `Hardcoded /api/ path(s) found — every fetch/EventSource argument ` + + `must derive from \`API_BASE\` (or the \`apiUrl()\` helper) in ` + + `\`@/shared/config/urls\`. See issue #494 for the why.\n\n` + + detail, + ) + } + expect(hits).toHaveLength(0) + }) +})