fix(catalyst-ui): route every fetch through API_BASE + add regression guardrail (closes #494)
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.<m>( '/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) <noreply@anthropic.com>
This commit is contained in:
parent
c76b409c64
commit
4c163364e6
@ -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/<id>/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))
|
||||
}
|
||||
})
|
||||
})
|
||||
})
|
||||
@ -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/<id>/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}/<rest>`. 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}`
|
||||
}
|
||||
|
||||
@ -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/<id>/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')
|
||||
|
||||
|
||||
167
products/catalyst/bootstrap/ui/src/test/no-hardcoded-api.test.ts
Normal file
167
products/catalyst/bootstrap/ui/src/test/no-hardcoded-api.test.ts
Normal file
@ -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( <whitespace>? <quote> /api/ ...
|
||||
* new EventSource( <whitespace>? <quote> /api/ ...
|
||||
* axios.<method>( <whitespace>? <quote> /api/ ...
|
||||
* where <quote> ∈ {' " `} (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.<m>`)
|
||||
* 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<string>([
|
||||
'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)
|
||||
})
|
||||
})
|
||||
Loading…
Reference in New Issue
Block a user