diff --git a/scripts/__tests__/shadcn-sync-fetch-cache.test.ts b/scripts/__tests__/shadcn-sync-fetch-cache.test.ts new file mode 100644 index 000000000..7f9da3450 --- /dev/null +++ b/scripts/__tests__/shadcn-sync-fetch-cache.test.ts @@ -0,0 +1,273 @@ +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import http from 'node:http'; +import type { AddressInfo } from 'node:net'; +import fsp from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; + +import { fetchUrl, fetchRegistry, isRegistryEntry, cacheFileFor, cacheStats } from '../shadcn-sync.js'; + +/** + * objectstack#5803 — the registry cache stored whatever came back. + * + * `fetchUrl` never looked at `res.statusCode`, and a non-JSON body was caught + * and resolved as a raw string, so an egress allowlist answering + * `403 Host not in allowlist: ui.shadcn.com…` resolved as if it were a + * component. `fetchRegistry` then wrote it to disk unconditionally with a + * one-hour TTL, and for the next hour every `pnpm shadcn:check` answered from + * those 46 poisoned entries without retrying — reported, in the summary line, + * as "46 cached, 0 fetched". + * + * The reproduction that motivated the fix, verbatim: + * + * run 1: ✗ … Registry returned no usable file content (46 errors) + * Registry: 0 cached, 46 fetched + * run 2: ✗ … Registry returned no usable file content (46 errors) + * Registry: 46 cached, 0 fetched <- never retried + * + * These tests are OFFLINE by construction: the shadcn registry is not + * reachable from CI (nor from the sandbox this was written in), which is + * exactly the condition that produced the bug. Every case below drives a local + * `http` fixture server — a real socket, real status lines, real chunking — + * with `http.get` injected in place of `https.get`. The status/parse logic + * under test is transport-independent, so nothing is stubbed out. + */ + +const REGISTRY_ENTRY = { + name: 'button', + type: 'registry:ui', + files: [{ path: 'ui/button.tsx', content: 'export const Button = () => null\n' }], +}; + +/** What the sandbox's egress blocker actually returns, byte for byte. */ +const EGRESS_BLOCK_BODY = + 'Host not in allowlist: ui.shadcn.com. Add this host to your network egress settings to allow access.'; + +type Responder = (req: http.IncomingMessage, res: http.ServerResponse) => void; + +const respondJson = + (body: unknown, code = 200): Responder => + (_req, res) => { + res.writeHead(code, { 'content-type': 'application/json' }); + res.end(JSON.stringify(body)); + }; + +const respondText = + (code: number, body: string, contentType = 'text/plain'): Responder => + (_req, res) => { + res.writeHead(code, { 'content-type': contentType }); + res.end(body); + }; + +let server: http.Server; +let origin = ''; +let requests = 0; +let respond: Responder = respondJson(REGISTRY_ENTRY); +let cacheDir = ''; + +const url = (name = 'button') => `${origin}/r/styles/default/${name}.json`; + +/** `fetchRegistry` with the fixture server and a throwaway cache directory. */ +const registry = (name: string, opts: Record = {}) => + fetchRegistry(url(name), { cacheDir, get: http.get, ...opts }); + +const readCacheDir = () => fsp.readdir(cacheDir).catch(() => [] as string[]); + +const writeCacheEntry = async (name: string, data: unknown, ageMs = 0) => { + await fsp.mkdir(cacheDir, { recursive: true }); + await fsp.writeFile( + cacheFileFor(url(name), cacheDir), + JSON.stringify({ url: url(name), fetchedAt: Date.now() - ageMs, data }), + ); +}; + +beforeAll(async () => { + server = http.createServer((req, res) => { + requests++; + respond(req, res); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', () => resolve())); + origin = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); +}); + +beforeEach(async () => { + requests = 0; + respond = respondJson(REGISTRY_ENTRY); + Object.assign(cacheStats, { hits: 0, misses: 0, failures: 0, evicted: 0 }); + cacheDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'shadcn-sync-cache-')); +}); + +afterEach(async () => { + await fsp.rm(cacheDir, { recursive: true, force: true }); +}); + +describe('fetchUrl — the response status is part of the contract', () => { + it('resolves the parsed JSON of a 2xx registry response', async () => { + respond = respondJson(REGISTRY_ENTRY); + await expect(fetchUrl(url(), { get: http.get })).resolves.toEqual(REGISTRY_ENTRY); + }); + + it('rejects a 403 egress block, with the status and the body in the message', async () => { + respond = respondText(403, EGRESS_BLOCK_BODY); + // Before the fix this RESOLVED with the string below, which then flowed + // into the cache. The status is in the message because "it failed" is not + // actionable on its own — 403 says "allowlist", 502 says "try again". + await expect(fetchUrl(url(), { get: http.get })).rejects.toThrow(/HTTP 403/); + await expect(fetchUrl(url(), { get: http.get })).rejects.toThrow(/Host not in allowlist/); + }); + + it('rejects a 502 HTML error page instead of resolving its markup', async () => { + respond = respondText(502, '

502 Bad Gateway

', 'text/html'); + await expect(fetchUrl(url(), { get: http.get })).rejects.toThrow(/HTTP 502/); + }); + + it('rejects a 200 whose body is not JSON', async () => { + // A captive-portal / proxy interstitial: the status says fine, the body is + // HTML. `data.files?.[0]?.content` reads `undefined` on a string, so this + // used to surface as "no usable file content" rather than as a fetch error. + respond = respondText(200, 'Sign in', 'text/html'); + await expect(fetchUrl(url(), { get: http.get })).rejects.toThrow(/Malformed JSON \(HTTP 200\)/); + }); + + it('rejects a redirect rather than following it into an interstitial', async () => { + respond = (_req, res) => { + res.writeHead(302, { location: 'https://example.invalid/login' }); + res.end(); + }; + await expect(fetchUrl(url(), { get: http.get })).rejects.toThrow(/HTTP 302/); + }); + + it('decodes a body whose multi-byte character is split across chunks', async () => { + // Pins `res.setEncoding('utf-8')`. Concatenating raw Buffers stringifies + // each chunk alone, so a character split at the boundary becomes U+FFFD and + // the payload silently changes — which, now that a parse failure is fatal, + // would be a hard error on a component containing any non-ASCII text. + const entry = { files: [{ path: 'ui/x.tsx', content: 'const label = "关闭"\n' }] }; + const payload = Buffer.from(JSON.stringify(entry), 'utf-8'); + const splitAt = payload.indexOf(Buffer.from('关', 'utf-8')) + 1; // mid-character + respond = (_req, res) => { + res.writeHead(200, { 'content-type': 'application/json' }); + res.write(payload.subarray(0, splitAt)); + res.end(payload.subarray(splitAt)); + }; + await expect(fetchUrl(url(), { get: http.get })).resolves.toEqual(entry); + }); +}); + +describe('isRegistryEntry — what is allowed onto disk', () => { + it.each([ + ['a registry entry', REGISTRY_ENTRY, true], + ['the egress block text', EGRESS_BLOCK_BODY, false], + ['a JSON error envelope', { error: 'not found' }, false], + ['an entry with no files', { name: 'button', files: [] }, false], + ['an entry whose content is blank', { files: [{ path: 'ui/x.tsx', content: ' \n' }] }, false], + ['an entry whose content is missing', { files: [{ path: 'ui/x.tsx' }] }, false], + ['null', null, false], + ['undefined', undefined, false], + ])('%s -> %s', (_label, data, expected) => { + expect(isRegistryEntry(data)).toBe(expected); + }); +}); + +describe('fetchRegistry — only registry data is ever cached', () => { + it('caches a well-formed entry and answers the next read from disk', async () => { + await expect(registry('button')).resolves.toEqual(REGISTRY_ENTRY); + expect(await readCacheDir()).toHaveLength(1); + + await expect(registry('button', { allowCache: true })).resolves.toEqual(REGISTRY_ENTRY); + expect(requests).toBe(1); // the second call never left the process + expect(cacheStats.hits).toBe(1); + }); + + it('writes nothing when the fetch is blocked with a 403', async () => { + respond = respondText(403, EGRESS_BLOCK_BODY); + await expect(registry('button', { allowCache: true })).rejects.toThrow(/HTTP 403/); + expect(await readCacheDir()).toEqual([]); + expect(cacheStats.failures).toBe(1); + expect(cacheStats.misses).toBe(0); + }); + + it('writes nothing for a 200 that parses but is not a registry entry', async () => { + // Handed back to the caller — `--check` reports "no usable file content" — + // but barred from disk, so the next run still retries. + respond = respondJson({ error: 'not found' }); + await expect(registry('button', { allowCache: true })).resolves.toEqual({ error: 'not found' }); + expect(await readCacheDir()).toEqual([]); + }); + + it('a blocked run does not turn the next run into a cache hit', async () => { + // The reported symptom, reduced: run, run again, and the second must still + // go to the network instead of reading "46 cached, 0 fetched". + respond = respondText(403, EGRESS_BLOCK_BODY); + await expect(registry('button', { allowCache: true })).rejects.toThrow(/HTTP 403/); + await expect(registry('button', { allowCache: true })).rejects.toThrow(/HTTP 403/); + expect(requests).toBe(2); + expect(cacheStats.hits).toBe(0); + expect(await readCacheDir()).toEqual([]); + }); + + it('never serves a poisoned entry left behind by an older build', async () => { + // Read-side validation. Write-side alone would leave every already-poisoned + // checkout serving this for the rest of its hour with no way out but to + // wait (or to know that `--no-cache` exists). + await writeCacheEntry('button', EGRESS_BLOCK_BODY); + expect(await readCacheDir()).toHaveLength(1); + + await expect(registry('button', { allowCache: true })).resolves.toEqual(REGISTRY_ENTRY); + expect(requests).toBe(1); + expect(cacheStats.hits).toBe(0); + expect(cacheStats.evicted).toBe(1); + + // …and the poison is gone, replaced by the real entry. + const written = JSON.parse(await fsp.readFile(cacheFileFor(url('button'), cacheDir), 'utf-8')); + expect(written.data).toEqual(REGISTRY_ENTRY); + }); + + it('evicts a poisoned entry even when the retry also fails', async () => { + await writeCacheEntry('button', EGRESS_BLOCK_BODY); + respond = respondText(403, EGRESS_BLOCK_BODY); + + await expect(registry('button', { allowCache: true })).rejects.toThrow(/HTTP 403/); + expect(cacheStats.evicted).toBe(1); + expect(await readCacheDir()).toEqual([]); + }); + + it('refetches a well-formed entry once it is older than the TTL', async () => { + await writeCacheEntry('button', { files: [{ path: 'ui/button.tsx', content: 'old\n' }] }, 2 * 60 * 60 * 1000); + await expect(registry('button', { allowCache: true })).resolves.toEqual(REGISTRY_ENTRY); + expect(requests).toBe(1); + // Expiry is not corruption: an aged-out entry is refetched, not evicted. + expect(cacheStats.evicted).toBe(0); + }); + + it('does not read the cache unless allowCache is set (--update stays live)', async () => { + await writeCacheEntry('button', REGISTRY_ENTRY); + await expect(registry('button')).resolves.toEqual(REGISTRY_ENTRY); + expect(requests).toBe(1); + expect(cacheStats.hits).toBe(0); + }); +}); + +describe('the CLI still runs when the file is the entry point', () => { + // `main()` is now behind an `invokedAsCli()` guard so importing this module + // (everything above) does not execute the CLI. Get that guard wrong and + // `pnpm shadcn:check` becomes a silent no-op that exits 0 — so it is pinned + // by actually running the script. + it('node scripts/shadcn-sync.js --list prints the component list', () => { + const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); + const result = spawnSync(process.execPath, ['scripts/shadcn-sync.js', '--list'], { + cwd: repoRoot, + encoding: 'utf-8', + timeout: 60_000, + }); + expect(result.status).toBe(0); + expect(result.stdout).toContain('Component List'); + expect(result.stdout).toContain('Custom ObjectUI Components:'); + }); +}); diff --git a/scripts/shadcn-sync.js b/scripts/shadcn-sync.js index 4e5b5fd4d..8d9964de8 100755 --- a/scripts/shadcn-sync.js +++ b/scripts/shadcn-sync.js @@ -22,6 +22,7 @@ */ import fs from 'fs/promises'; +import { realpathSync } from 'fs'; import path from 'path'; import { fileURLToPath } from 'url'; import https from 'https'; @@ -55,7 +56,17 @@ const BACKUP_DIR = path.join(REPO_ROOT, 'packages/components/.backup'); */ const CACHE_DIR = path.join(REPO_ROOT, 'node_modules/.cache/shadcn-sync'); const CACHE_TTL_MS = 60 * 60 * 1000; -const cacheStats = { hits: 0, misses: 0 }; +/** + * `hits` served from a valid cached entry + * `misses` fetched live and the response came back + * `failures` fetched live and the request failed (non-2xx, unparseable, socket) + * `evicted` cached entries dropped on read because they were not registry data + * + * `failures`/`evicted` exist so a poisoned or blocked run cannot look like a + * quiet success in the summary line — under an egress block the old counters + * printed "46 fetched" for 46 error pages (objectstack#5803). + */ +const cacheStats = { hits: 0, misses: 0, failures: 0, evicted: 0 }; // ANSI color codes const colors = { @@ -80,24 +91,98 @@ function logSection(title) { console.log('='.repeat(60) + '\n'); } -async function fetchUrl(url) { +/** + * One-line excerpt of a response body, for an error message. + * + * Control characters are replaced by spaces (tested by code point, so this + * source file carries none itself): these strings end up in a terminal and in + * CI logs, and an error page carrying a stray NUL or an ANSI escape would + * otherwise be pasted straight through. The body is also sliced before the + * scan, so a multi-megabyte HTML error page costs nothing to summarise. + */ +function bodySnippet(body, max = 120) { + const flat = Array.from(String(body).slice(0, max * 8)) + .map((ch) => (ch.codePointAt(0) < 0x20 || ch.codePointAt(0) === 0x7f ? ' ' : ch)) + .join('') + .replace(/\s+/g, ' ') + .trim(); + return flat.length > max ? `${flat.slice(0, max)}…` : flat; +} + +/** + * Fetch one registry URL and resolve its parsed JSON. + * + * Two failure modes used to resolve as if they had succeeded (objectstack#5803): + * + * 1. **A non-2xx response.** `https.get` emits `error` only for TRANSPORT + * failures. A 403 from an egress allowlist, a 502 from a CDN, a 404 from a + * moved endpoint — each arrives as a perfectly ordinary response whose + * *body* is the error text. Never looking at `statusCode` meant that text + * was handed back as though it were the component. + * 2. **A 2xx that is not JSON.** The old code caught the `JSON.parse` throw + * and resolved the raw string instead, so an HTML interstitial flowed on to + * consumers that all read `data.files?.[0]?.content` and quietly saw + * `undefined` — the lenient read that hid the failure. + * + * Both now reject. Every caller already has an error path (`--check` marks the + * component `error`, `--update` refuses to write); what they did not have was + * anything telling them the fetch had failed at all. + * + * Redirects are deliberately NOT followed: a 3xx to a login or interstitial + * page is the same class of poison. The registry endpoints are direct JSON, so + * if upstream ever starts redirecting, this fails loudly with the status in the + * message instead of ingesting whatever the redirect target serves. + * + * `get` is injectable so the tests can drive a real fixture server over plain + * `http` — the status/parse logic under test is transport-independent. + */ +async function fetchUrl(url, { get = https.get } = {}) { return new Promise((resolve, reject) => { - https.get(url, (res) => { + const req = get(url, (res) => { + const status = res.statusCode ?? 0; let data = ''; + // Decode as UTF-8 across chunk boundaries. Concatenating raw Buffers + // stringifies each chunk on its own, which corrupts any multi-byte + // character split across a boundary — harmless while a mangled body was + // silently tolerated, but now it would surface as a hard parse error. + res.setEncoding('utf-8'); res.on('data', (chunk) => { data += chunk; }); + res.on('error', reject); res.on('end', () => { + if (status < 200 || status >= 300) { + const where = res.statusMessage ? `HTTP ${status} ${res.statusMessage}` : `HTTP ${status}`; + reject(new Error(`${where} from ${url}${data ? ` — ${bodySnippet(data)}` : ''}`)); + return; + } try { resolve(JSON.parse(data)); - } catch (e) { - resolve(data); + } catch { + reject(new Error(`Malformed JSON (HTTP ${status}) from ${url} — ${bodySnippet(data)}`)); } }); - }).on('error', reject); + }); + req.on('error', reject); }); } -function cacheFileFor(url) { - return path.join(CACHE_DIR, `${crypto.createHash('sha1').update(url).digest('hex').slice(0, 16)}.json`); +/** + * Is this parsed response an actual registry entry we can act on? + * + * The bar is exactly what every consumer reads: a `files` array whose first + * entry carries non-empty `content`. A 200 that parses but carries an error + * envelope (`{"error":"…"}`), or an entry with no files, fails it. + * + * This is the *semantic* half of the defence, independent of the transport + * check in `fetchUrl`: a proxy that answers 200 with a JSON error object is + * still not something to write into the cache. + */ +function isRegistryEntry(data) { + const content = data?.files?.[0]?.content; + return Array.isArray(data?.files) && typeof content === 'string' && content.trim().length > 0; +} + +function cacheFileFor(url, cacheDir = CACHE_DIR) { + return path.join(cacheDir, `${crypto.createHash('sha1').update(url).digest('hex').slice(0, 16)}.json`); } /** @@ -111,30 +196,87 @@ function cacheFileFor(url) { * from a cached copy, but nothing that writes a component file ever does. * A stale `--check` is a mildly out-of-date status line; a stale `--update` * would put hour-old code on disk under a message saying it is current. + * + * ## Only registry data is ever cached (objectstack#5803) + * + * The cache used to store whatever `fetchUrl` resolved, which under an egress + * block was the string `Host not in allowlist: ui.shadcn.com…`. All 46 entries + * were written, and for the next hour every `--check` answered from them + * without retrying — one moment of network trouble degraded the command for an + * hour, while reporting "46 cached" as if all were well. + * + * So the entry is validated on BOTH sides of the disk: + * + * - **write**: only a response that passes `isRegistryEntry` is persisted. A + * rejected fetch or a junk payload leaves the cache untouched, so the next + * run retries for real. + * - **read**: an entry that does not pass is not served, and is deleted. Read + * validation is what makes recovery immediate — a cache already poisoned by + * an older build (or by any future write path) is dropped on first contact + * instead of being trusted until its hour is up. Validating only on write + * would leave every existing poisoned tree serving garbage for the rest of + * its TTL, with no way to tell the user beyond "wait". + * + * `cacheDir` and `get` exist for the tests (a throwaway directory and a local + * fixture server). Both carry real defaults rather than being left undeclared: + * once `scripts/` is type-checked with `allowJs` (objectui#3494), a destructured + * option with no default is absent from this function's INFERRED signature, so + * passing it from a `.ts` caller is a hard error. Declaring the injection point + * is the fix; suppressing it at the call site would not be. */ -async function fetchRegistry(url, { allowCache = false } = {}) { +async function fetchRegistry(url, { allowCache = false, cacheDir = CACHE_DIR, get = https.get } = {}) { + const cacheFile = cacheFileFor(url, cacheDir); + if (allowCache) { + let entry; try { - const entry = JSON.parse(await fs.readFile(cacheFileFor(url), 'utf-8')); - if (typeof entry?.fetchedAt === 'number' && Date.now() - entry.fetchedAt < CACHE_TTL_MS) { - cacheStats.hits++; - return entry.data; - } + entry = JSON.parse(await fs.readFile(cacheFile, 'utf-8')); } catch { /* absent, unreadable or corrupt — fall through and refetch */ } + if (entry !== undefined) { + if (isRegistryEntry(entry?.data)) { + // Well-formed but expired entries are left alone: they are simply + // refetched, and the write below replaces them. + if (typeof entry?.fetchedAt === 'number' && Date.now() - entry.fetchedAt < CACHE_TTL_MS) { + cacheStats.hits++; + return entry.data; + } + } else { + // Poison: an error page, an error envelope, or a shape we cannot use. + // Drop it now so a repeat run cannot be tempted by it again. + try { + await fs.rm(cacheFile, { force: true }); + cacheStats.evicted++; + } catch { + /* read-only cache dir — the entry is ignored either way */ + } + } + } } - const data = await fetchUrl(url); + let data; + try { + data = await fetchUrl(url, { get }); + } catch (error) { + // Counted, then rethrown untouched: the caller decides what a failed fetch + // means (`--check` reports the component, `--update` refuses to write). + cacheStats.failures++; + throw error; + } cacheStats.misses++; - try { - await fs.mkdir(CACHE_DIR, { recursive: true }); - await fs.writeFile(cacheFileFor(url), JSON.stringify({ url, fetchedAt: Date.now(), data })); - } catch { - /* the cache is an optimisation; never fail a run because it can't be written */ + if (isRegistryEntry(data)) { + try { + await fs.mkdir(cacheDir, { recursive: true }); + await fs.writeFile(cacheFile, JSON.stringify({ url, fetchedAt: Date.now(), data })); + } catch { + /* the cache is an optimisation; never fail a run because it can't be written */ + } } + // Junk that parsed as JSON is still handed back — callers already report it + // ("Registry returned no usable file content"). It is only barred from disk. return data; } @@ -452,11 +594,15 @@ async function checkAllComponents(options = {}) { // A run that returns in under a second is otherwise indistinguishable from // one that silently fetched nothing, so always say where the data came from. - if (cacheStats.hits > 0 || cacheStats.misses > 0) { + // Failed and evicted are reported separately and only when non-zero: a run + // whose fetches all failed must not read as "46 fetched" (objectstack#5803). + if (cacheStats.hits > 0 || cacheStats.misses > 0 || cacheStats.failures > 0 || cacheStats.evicted > 0) { const ttlMin = Math.round(CACHE_TTL_MS / 60000); + const parts = [`${cacheStats.hits} cached`, `${cacheStats.misses} fetched`]; + if (cacheStats.failures > 0) parts.push(`${cacheStats.failures} FAILED`); + if (cacheStats.evicted > 0) parts.push(`${cacheStats.evicted} unusable entries evicted`); log( - `\nRegistry: ${cacheStats.hits} cached, ${cacheStats.misses} fetched ` + - `(cache TTL ${ttlMin}min — --no-cache to force live)`, + `\nRegistry: ${parts.join(', ')} ` + `(cache TTL ${ttlMin}min — --no-cache to force live)`, 'dim', ); } @@ -859,8 +1005,34 @@ async function main() { } } -main().catch(error => { - log(`Fatal error: ${error.message}`, 'red'); - console.error(error); - process.exit(1); -}); +/** + * Is this file the process entry point? + * + * The CLI must keep running exactly as before when invoked as + * `node scripts/shadcn-sync.js …`, while an `import` of this module (the tests + * in `scripts/__tests__/shadcn-sync-fetch-cache.test.ts`) must NOT execute it. + * Compared through `realpathSync` because ESM resolves `import.meta.url` to the + * real path while `process.argv[1]` may still carry a symlink. + */ +function invokedAsCli() { + const entry = process.argv[1]; + if (!entry) return false; + const resolved = path.resolve(entry); + try { + return realpathSync(resolved) === __filename; + } catch { + return resolved === __filename; + } +} + +if (invokedAsCli()) { + main().catch(error => { + log(`Fatal error: ${error.message}`, 'red'); + console.error(error); + process.exit(1); + }); +} + +// Exported for scripts/__tests__/shadcn-sync-fetch-cache.test.ts. The CLI is +// the only production consumer; nothing else imports this module. +export { fetchUrl, fetchRegistry, isRegistryEntry, cacheFileFor, cacheStats, bodySnippet, CACHE_TTL_MS };