From 6467f168542de6eecd38b1604e1a0178ea3a1ca5 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 05:23:55 +0700 Subject: [PATCH 1/5] fix(pricing): apply post-promo DeepSeek rates, machine-check curated expiries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deepseek-v4-pro was still pinned to its 75%-off launch promo 55 days after the promo ended, so every DeepSeek row billed at 25% of true cost. The file documented the cutover date itself, in prose, and nothing read it. A stale price is worse than a missing one: a missing price shows $0 and looks broken, a stale price looks fine forever. - Correct deepseek-v4-pro to the standard rates (1.74 / 3.48 / 0.0145 / 1.74) - Replace the free-text _meta.*_expiry keys with a structured _meta.expiries array (id / expires_at / what / action), carrying the Sonnet 5 2026-08-31 cutover that was about to repeat the same failure - Add validate:curated-expiry to ci:local so a PR opened on or after an expiry date fails until a human applies the action and clears the entry; it also rejects any regression to free-text *_expiry keys - Stop duplicating price literals in model-breakdown's coverage test — it asserts the lookup path (aliases, prefixes, casing) and reads expected rates from the curated table, so a legitimate price fix touches one file Closes #87 --- package.json | 3 +- scripts/validate-curated-expiry.cjs | 126 +++++++++++++++++++++++++ src/lib/pricing/curated-overrides.json | 13 ++- test/curated-expiry.test.js | 114 ++++++++++++++++++++++ test/model-breakdown.test.js | 31 +++--- 5 files changed, 272 insertions(+), 15 deletions(-) create mode 100644 scripts/validate-curated-expiry.cjs create mode 100644 test/curated-expiry.test.js diff --git a/package.json b/package.json index a8ce2fae..5b7bdd39 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,7 @@ "access": "public" }, "scripts": { - "ci:local": "npm run dashboard:build && npm --prefix dashboard run test && npm test && npm run validate:copy && npm run validate:ui-hardcode && npm run validate:guardrails && npm run docs:openwiki:check && node --test test/architecture-guardrails.test.js", + "ci:local": "npm run dashboard:build && npm --prefix dashboard run test && npm test && npm run validate:copy && npm run validate:ui-hardcode && npm run validate:guardrails && npm run validate:curated-expiry && npm run docs:openwiki:check && node --test test/architecture-guardrails.test.js", "copy:pull": "node scripts/copy-sync.cjs pull", "copy:push": "node scripts/copy-sync.cjs push", "dashboard:build": "npm --prefix dashboard run build", @@ -42,6 +42,7 @@ "prepublishOnly": "node scripts/build-pricing-seed.cjs", "test": "node --test test/*.test.js", "validate:copy": "node scripts/validate-copy-registry.cjs", + "validate:curated-expiry": "node scripts/validate-curated-expiry.cjs", "validate:guardrails": "node scripts/validate-architecture-guardrails.cjs", "validate:retros": "node scripts/validate-retros.cjs", "validate:ui-hardcode": "node scripts/ops/validate-ui-hardcode.cjs" diff --git a/scripts/validate-curated-expiry.cjs b/scripts/validate-curated-expiry.cjs new file mode 100644 index 00000000..cea270a0 --- /dev/null +++ b/scripts/validate-curated-expiry.cjs @@ -0,0 +1,126 @@ +// Fails when a time-boxed pricing fact in curated-overrides.json has passed its +// expiry date. +// +// Why this exists: `_meta` used to carry expiry dates as free-text prose ("update +// this file before the cutover"). Nothing read them, so deepseek-v4-pro stayed +// pinned to a 75%-off launch promo for 55 days after the promo ended — every +// DeepSeek row on the dashboard billed at 25% of its true cost, with no signal. +// A stale price is worse than a missing one: a missing price shows $0 and looks +// broken, a stale price looks fine forever. See issue #87. +// +// This turns "remember to edit a JSON file in May" into "the next PR fails". + +const fs = require("fs"); +const path = require("path"); + +const ROOT = path.resolve(__dirname, ".."); +const OVERRIDES_PATH = path.join(ROOT, "src", "lib", "pricing", "curated-overrides.json"); + +const DATE_RE = /^\d{4}-\d{2}-\d{2}$/; +const REQUIRED_FIELDS = ["id", "expires_at", "what", "action"]; + +// An expiry is due at UTC midnight on its date, so an entry dated 2026-08-31 +// fails from the first moment of 2026-08-31 onward. +function parseExpiryMs(value) { + if (typeof value !== "string" || !DATE_RE.test(value)) return null; + const ms = Date.parse(`${value}T00:00:00Z`); + if (!Number.isFinite(ms)) return null; + // Reject dates that round-trip differently (e.g. 2026-02-31 → Mar 3). + if (new Date(ms).toISOString().slice(0, 10) !== value) return null; + return ms; +} + +function isNonEmptyString(value) { + return typeof value === "string" && value.trim() !== ""; +} + +// Pure: takes the parsed `_meta` object and a timestamp, returns findings. +// Exported so tests can drive it without touching the clock or the real file. +function checkExpiries(meta, nowMs) { + const errors = []; + + if (meta == null || typeof meta !== "object") { + return { errors: ["_meta is missing or not an object"], checked: 0 }; + } + + // Guard against regressing to the pattern this check replaced. + for (const key of Object.keys(meta)) { + if (/_expiry$/.test(key)) { + errors.push( + `_meta.${key}: free-text expiry keys are not checked by anything. ` + + "Move it into the _meta.expiries array so it is enforced.", + ); + } + } + + const entries = meta.expiries; + if (entries === undefined) return { errors, checked: 0 }; + if (!Array.isArray(entries)) { + errors.push("_meta.expiries must be an array"); + return { errors, checked: 0 }; + } + + const seenIds = new Set(); + + entries.forEach((entry, index) => { + const label = `_meta.expiries[${index}]`; + + if (entry == null || typeof entry !== "object" || Array.isArray(entry)) { + errors.push(`${label}: must be an object`); + return; + } + + for (const field of REQUIRED_FIELDS) { + if (!isNonEmptyString(entry[field])) { + errors.push(`${label}: '${field}' is required and must be a non-empty string`); + } + } + + if (isNonEmptyString(entry.id)) { + if (seenIds.has(entry.id)) errors.push(`${label}: duplicate id '${entry.id}'`); + seenIds.add(entry.id); + } + + const expiresMs = parseExpiryMs(entry.expires_at); + if (expiresMs === null) { + errors.push(`${label}: 'expires_at' must be a real calendar date as YYYY-MM-DD`); + return; + } + + if (nowMs >= expiresMs) { + const daysPast = Math.floor((nowMs - expiresMs) / 86400000); + errors.push( + `${label} '${entry.id}' EXPIRED ${entry.expires_at} (${daysPast} day(s) ago)\n` + + ` what: ${entry.what}\n` + + ` action: ${entry.action}\n` + + " Apply the action above, then remove or advance this entry.", + ); + } + }); + + return { errors, checked: entries.length }; +} + +function main() { + let parsed; + try { + parsed = JSON.parse(fs.readFileSync(OVERRIDES_PATH, "utf8")); + } catch (e) { + console.error(`Curated expiry errors:\n- cannot read ${OVERRIDES_PATH}: ${e.message}`); + process.exit(1); + } + + const { errors, checked } = checkExpiries(parsed._meta, Date.now()); + + if (errors.length) { + console.error("Curated expiry errors:"); + errors.forEach((line) => console.error(`- ${line}`)); + process.exit(1); + } + + console.log(`Curated expiry ok: ${checked} time-boxed entr${checked === 1 ? "y" : "ies"} still valid.`); +} + +if (require.main === module) main(); + +module.exports = { checkExpiries, parseExpiryMs }; diff --git a/src/lib/pricing/curated-overrides.json b/src/lib/pricing/curated-overrides.json index 2e64d8a7..b575bbdf 100644 --- a/src/lib/pricing/curated-overrides.json +++ b/src/lib/pricing/curated-overrides.json @@ -2,8 +2,15 @@ "_meta": { "note": "Curated price overrides. Always wins over LiteLLM. Two reasons to live here: (1) self-defined alias names that LiteLLM will never carry (kiro-*, hy3-*, composer-*, kimi-for-coding, free-tier OpenRouter routes); (2) prices we want to pin even if LiteLLM has the model (e.g. cache_write fields LiteLLM often omits). Units: USD per million tokens. Edit this file to override pricing without redeploying.", "units": "usd_per_million_tokens", - "deepseek_v4_pro_discount_expiry": "2026-05-31T15:59:00Z — DeepSeek v4-pro is currently at a 75% promotional discount. After expiry the prices revert to 4x: input $1.74/M, output $3.48/M, cache_read $0.0145/M, cache_write $1.74/M. Update this file before the cutover.", - "sonnet5_intro_price_expiry": "2026-08-31 — Sonnet 5 intro price 2/10 reverts to 3/15 sticker. The src pricing path auto-tracks LiteLLM (no edit needed); re-vendor the seed after the cutover. See issue #16." + "expiries_note": "Time-boxed pricing facts. Machine-checked by scripts/validate-curated-expiry.cjs (npm run validate:curated-expiry, part of ci:local): once expires_at has passed, the check FAILS until a human applies `action` and then removes or advances the entry. Free-text expiry notes are not allowed here — a date nobody checks is how deepseek-v4-pro stayed on a 75%-off promo price for 55 days past its cutover (issue #87).", + "expiries": [ + { + "id": "sonnet5-intro-price", + "expires_at": "2026-08-31", + "what": "Sonnet 5 introductory pricing (2/10) reverts to the 3/15 sticker price.", + "action": "No curated entry to edit — the src pricing path auto-tracks LiteLLM. Re-vendor the bundled seed (npm run pricing:build-seed) so a cold start also prices Sonnet 5 correctly, then delete this entry. See issue #16." + } + ] }, "exact": { "claude-fable-5": { "input": 10, "output": 50, "cache_read": 1, "cache_write": 12.5, "note": "Pinned from Anthropic public pricing until bundled LiteLLM seed carries Fable 5." }, @@ -20,7 +27,7 @@ "MiniMax-M2.7": { "input": 0.3, "output": 1.2, "cache_read": 0.06, "cache_write": 0.375 }, "MiniMax-M2.7-highspeed":{ "input": 0.6, "output": 2.4, "cache_read": 0.06, "cache_write": 0.375 }, "deepseek-v4-flash":{ "input": 0.14, "output": 0.28, "cache_read": 0.0028, "cache_write": 0.14 }, - "deepseek-v4-pro": { "input": 0.435,"output": 0.87, "cache_read": 0.003625, "cache_write": 0.435 }, + "deepseek-v4-pro": { "input": 1.74, "output": 3.48, "cache_read": 0.0145, "cache_write": 1.74, "note": "Standard (post-promo) pricing. The 75%-off launch promo expired 2026-05-31; these rates applied 2026-07-25 (issue #87)." }, "deepseek-chat": { "input": 0.14, "output": 0.28, "cache_read": 0.0028, "cache_write": 0.14 }, "grok-build": { "input": 1.25, "output": 2.50, "cache_read": 0.20, "note": "Grok Build TUI estimate. Local telemetry currently exposes totalTokens without a stable prompt/output/cache split, so TokenTracker estimates input/output split until Grok exposes per-call usage details." }, "grok-4-0709": { "input": 3.00, "output": 15.00, "cache_read": 0.75 }, diff --git a/test/curated-expiry.test.js b/test/curated-expiry.test.js new file mode 100644 index 00000000..1ba558a7 --- /dev/null +++ b/test/curated-expiry.test.js @@ -0,0 +1,114 @@ +const test = require("node:test"); +const assert = require("node:assert"); + +const { checkExpiries, parseExpiryMs } = require("../scripts/validate-curated-expiry.cjs"); +const curated = require("../src/lib/pricing/curated-overrides.json"); + +const AT = (iso) => Date.parse(`${iso}T00:00:00Z`); + +const VALID_ENTRY = { + id: "sample", + expires_at: "2026-08-31", + what: "Intro price reverts to sticker.", + action: "Re-vendor the seed, then delete this entry.", +}; + +test("passes while the expiry is still in the future", () => { + const { errors, checked } = checkExpiries({ expiries: [VALID_ENTRY] }, AT("2026-08-30")); + assert.deepStrictEqual(errors, []); + assert.strictEqual(checked, 1); +}); + +test("fails from UTC midnight of the expiry date (boundary is inclusive)", () => { + const { errors } = checkExpiries({ expiries: [VALID_ENTRY] }, AT("2026-08-31")); + assert.strictEqual(errors.length, 1); + assert.match(errors[0], /EXPIRED 2026-08-31 \(0 day\(s\) ago\)/); +}); + +test("an expired entry reports the action so the fix is in the failure output", () => { + const { errors } = checkExpiries({ expiries: [VALID_ENTRY] }, AT("2026-09-10")); + assert.strictEqual(errors.length, 1); + assert.match(errors[0], /'sample'/); + assert.match(errors[0], /10 day\(s\) ago/); + assert.match(errors[0], /Re-vendor the seed/); +}); + +test("rejects a missing or blank required field", () => { + for (const field of ["id", "expires_at", "what", "action"]) { + const entry = { ...VALID_ENTRY, [field]: " " }; + const { errors } = checkExpiries({ expiries: [entry] }, AT("2026-01-01")); + assert.ok( + errors.some((e) => e.includes(`'${field}' is required`)), + `expected a required-field error for ${field}, got ${JSON.stringify(errors)}`, + ); + } +}); + +test("rejects a malformed or impossible date", () => { + for (const bad of ["2026-8-31", "31/08/2026", "2026-02-31", "soon", ""]) { + const { errors } = checkExpiries( + { expiries: [{ ...VALID_ENTRY, expires_at: bad }] }, + AT("2026-01-01"), + ); + assert.ok( + errors.some((e) => e.includes("YYYY-MM-DD")), + `expected a date error for ${JSON.stringify(bad)}, got ${JSON.stringify(errors)}`, + ); + } +}); + +test("rejects duplicate ids", () => { + const { errors } = checkExpiries( + { expiries: [VALID_ENTRY, { ...VALID_ENTRY }] }, + AT("2026-01-01"), + ); + assert.ok(errors.some((e) => e.includes("duplicate id 'sample'"))); +}); + +test("rejects a regression to free-text *_expiry keys", () => { + const { errors } = checkExpiries( + { some_promo_expiry: "2026-05-31 — remember to update this", expiries: [] }, + AT("2026-01-01"), + ); + assert.strictEqual(errors.length, 1); + assert.match(errors[0], /free-text expiry keys are not checked/); +}); + +test("expiries_note is allowed even though it ends in a checked-looking word", () => { + const { errors } = checkExpiries({ expiries_note: "how this works", expiries: [] }, AT("2026-01-01")); + assert.deepStrictEqual(errors, []); +}); + +test("an absent expiries array is fine; a non-array is not", () => { + assert.deepStrictEqual(checkExpiries({}, AT("2026-01-01")).errors, []); + assert.ok( + checkExpiries({ expiries: "2026-08-31" }, AT("2026-01-01")).errors.some((e) => + e.includes("must be an array"), + ), + ); +}); + +test("a non-object entry is reported rather than crashing", () => { + const { errors } = checkExpiries({ expiries: ["2026-08-31", null] }, AT("2026-01-01")); + assert.strictEqual(errors.length, 2); + errors.forEach((e) => assert.match(e, /must be an object/)); +}); + +test("parseExpiryMs pins to UTC midnight regardless of the local timezone", () => { + assert.strictEqual(parseExpiryMs("2026-08-31"), Date.UTC(2026, 7, 31)); + assert.strictEqual(parseExpiryMs("nope"), null); +}); + +test("the real curated-overrides.json has no expired entries today", () => { + const { errors } = checkExpiries(curated._meta, Date.now()); + assert.deepStrictEqual(errors, [], errors.join("\n")); +}); + +test("deepseek-v4-pro carries post-promo pricing, not the expired 75%-off rates", () => { + // Regression guard for issue #87: the promo rates were input 0.435 / output 0.87. + const pro = curated.exact["deepseek-v4-pro"]; + assert.strictEqual(pro.input, 1.74); + assert.strictEqual(pro.output, 3.48); + assert.strictEqual(pro.cache_read, 0.0145); + assert.strictEqual(pro.cache_write, 1.74); +}); diff --git a/test/model-breakdown.test.js b/test/model-breakdown.test.js index 07e399a6..68fd6d38 100644 --- a/test/model-breakdown.test.js +++ b/test/model-breakdown.test.js @@ -356,20 +356,29 @@ test("computeRowCost still bills reasoning for non-Codex sources (e.g. gemini)", }); test("pricing covers production MiniMax and DeepSeek model ids used by leaderboard", () => { - const cases = [ - ["MiniMax-M2.7", { input: 0.3, output: 1.2, cache_read: 0.06, cache_write: 0.375 }], - ["MiniMax-M2.7-highspeed", { input: 0.6, output: 2.4, cache_read: 0.06, cache_write: 0.375 }], - ["deepseek-v4-flash", { input: 0.14, output: 0.28, cache_read: 0.0028, cache_write: 0.14 }], - ["deepseek-v4-pro", { input: 0.435, output: 0.87, cache_read: 0.003625, cache_write: 0.435 }], - ]; - - for (const [model, expected] of cases) { - assert.deepEqual(localApi.getModelPricing(model), expected, `${model} must not fall back to zero pricing`); + // This test is about the LOOKUP PATH — that these ids resolve at all, and that + // prefixed/lower-cased variants resolve to the same entry, rather than falling + // back to ZERO_PRICING. It deliberately reads the expected rates from the + // curated table instead of duplicating literals: a copy of the numbers here + // just means a legitimate price correction fails an unrelated test. The rates + // themselves are pinned once, in test/curated-expiry.test.js (issue #87). + const curated = require("../src/lib/pricing/curated-overrides.json").exact; + const models = ["MiniMax-M2.7", "MiniMax-M2.7-highspeed", "deepseek-v4-flash", "deepseek-v4-pro"]; + + for (const model of models) { + const pricing = localApi.getModelPricing(model); + assert.ok( + pricing.input > 0 && pricing.output > 0, + `${model} must not fall back to zero pricing`, + ); + for (const field of ["input", "output", "cache_read", "cache_write"]) { + assert.equal(pricing[field], curated[model][field], `${model}.${field} must match the curated table`); + } } // DB rows can arrive with provider/model prefixes or lower-cased aliases. - assert.deepEqual(localApi.getModelPricing("openrouter/minimax-m2.7"), cases[0][1]); - assert.deepEqual(localApi.getModelPricing("DeepSeek-V4-Pro"), cases[3][1]); + assert.deepEqual(localApi.getModelPricing("openrouter/minimax-m2.7"), localApi.getModelPricing("MiniMax-M2.7")); + assert.deepEqual(localApi.getModelPricing("DeepSeek-V4-Pro"), localApi.getModelPricing("deepseek-v4-pro")); }); // ───────────────────────────────────────────────────────────────────────────── From 62759c8624149444e41dff5821facf8fb6fed6e8 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 05:24:08 +0700 Subject: [PATCH 2/5] fix(serve,sync): reject rebound Host headers, heartbeat the sync lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two small hardening fixes for windows that are cheap to close now and expensive to diagnose afterwards. Host header (#88): binding to loopback does not make the Host header trustworthy. Under DNS rebinding a browser sends Host: attacker.example to 127.0.0.1 and treats the response as same-origin, so CORS never applies. Mutations were already gated on a loopback Origin, but every GET /functions/* endpoint — full spend history, model mix, project names — was readable by any page the victim had open. Requests whose Host is not loopback now get a 403 before any routing. isLoopbackHostname is reused from local-api so the Host allowlist and the Origin allowlist cannot drift. The request handler moved out of cmdServe into createRequestHandler so the test boots a real server and asserts the API handler is never reached — a guard that exists but is never wired is exactly the failure being prevented. Sync lock (#89): the lock had a 5-minute staleness window and never refreshed its own mtime, while local-sync fires on an interval. Any sync longer than one tick — full-corpus rebuilds and migration reparses are — had its lock stolen, letting two writers interleave appends into queue.jsonl. A torn line is silently skipped by the reader, and a skipped retraction row is a permanent overcount. - Heartbeat the lock mtime every 30s (unref'd, cleared on release) and raise the stale window to 30 minutes: "stale" now means the holder died - Record pid/host/startedAt in the lock, and reclaim immediately when the recorded process is gone — faster recovery than the old window, not slower - Gate the stale takeover behind an atomic mkdir mutex. The previous check-then-act let two waiters both delete and both acquire; a rename-based claim was tried first and still failed a 4-way race test, because rename is atomic but does not bind the check to the act - Clean up a failed lock write instead of leaking the fd and an empty file Closes #88 Closes #89 --- src/commands/serve.js | 114 ++++++++++++++------- src/lib/fs.js | 174 ++++++++++++++++++++++++++++----- src/lib/local-api.js | 2 + test/host-header-guard.test.js | 113 +++++++++++++++++++++ test/sync-lock.test.js | 160 ++++++++++++++++++++++++++++++ 5 files changed, 500 insertions(+), 63 deletions(-) create mode 100644 test/host-header-guard.test.js create mode 100644 test/sync-lock.test.js diff --git a/src/commands/serve.js b/src/commands/serve.js index ad65c8da..8b1a3aee 100644 --- a/src/commands/serve.js +++ b/src/commands/serve.js @@ -4,7 +4,7 @@ const path = require("node:path"); const fssync = require("node:fs"); const { resolveTrackerPaths } = require("../lib/tracker-paths"); -const { createLocalApiHandler, resolveQueuePath } = require("../lib/local-api"); +const { createLocalApiHandler, resolveQueuePath, isLoopbackHostname } = require("../lib/local-api"); const { buildServeDataPreflightMessage, summarizeQueueData, @@ -18,6 +18,78 @@ const DEFAULT_MAX_PORT_ATTEMPTS = 20; const NPM_PACKAGE_NAME = "@ipv9/tokentracker-cli"; const LOCAL_BIND_HOST = "127.0.0.1"; +// Anti-DNS-rebinding guard. Binding the socket to loopback does not make the +// Host header trustworthy: under DNS rebinding a browser sends +// `Host: attacker.example:` to 127.0.0.1 and treats the response as +// same-origin, so CORS never applies. Mutations are already gated on a loopback +// Origin; without this check every GET /functions/* endpoint — full spend +// history, model mix, project names — is readable by any page the victim +// happens to have open. Issue #88. +// +// Only the hostname matters: the port is chosen at runtime by +// listenOnAvailablePort, so pinning it here would be fragile without adding any +// protection. An absent Host (HTTP/1.0, some local probes) is allowed — the +// socket is already loopback-bound, and there is no rebinding vector without a +// browser sending a name. +function isAllowedHostHeader(hostHeader) { + if (hostHeader == null || hostHeader === "") return true; + try { + return isLoopbackHostname(new URL(`http://${hostHeader}`).hostname); + } catch (_e) { + return false; + } +} + +// Extracted from cmdServe so the wiring — not just the predicate — is testable: +// a guard that exists but is never reached is the failure mode this is guarding +// against in the first place. +function createRequestHandler({ handleApi, dashboardDir }) { + return async function handleRequest(req, res) { + try { + // Reject rebound hostnames before anything reads the request. Issue #88. + if (!isAllowedHostHeader(req.headers.host)) { + res.writeHead(403, { "Content-Type": "text/plain" }); + res.end("Forbidden: TokenTracker only serves loopback hosts.\n"); + return; + } + + const url = new URL(req.url || "/", `http://${req.headers.host || "localhost"}`); + + // CORS preflight + if (req.method === "OPTIONS") { + res.writeHead(204, { + "Access-Control-Allow-Methods": "GET, POST, OPTIONS", + "Access-Control-Allow-Headers": "Content-Type, Authorization", + }); + res.end(); + return; + } + + // API routes + if ( + url.pathname.startsWith("/functions/") + || url.pathname.startsWith("/api/") + || url.pathname.startsWith("/proxy/") + ) { + const handled = await handleApi(req, res, url); + if (handled) return; + } + + // Static files + const served = await serveStaticFile(dashboardDir, url.pathname, res); + if (served) return; + + // SPA fallback + await serveStaticFile(dashboardDir, "/index.html", res); + } catch (e) { + if (!res.headersSent) { + res.writeHead(500, { "Content-Type": "text/plain" }); + res.end("Internal Server Error"); + } + } + }; +} + function buildPortInUseHint(port) { return `Port ${port} is unavailable. Try: npx ${NPM_PACKAGE_NAME} serve --port ${port + 1}\n`; } @@ -110,43 +182,7 @@ async function cmdServe(argv) { // 3. Create handler const handleApi = createLocalApiHandler({ queuePath }); - const server = http.createServer(async (req, res) => { - try { - const url = new URL(req.url || "/", `http://${req.headers.host || "localhost"}`); - - // CORS preflight - if (req.method === "OPTIONS") { - res.writeHead(204, { - "Access-Control-Allow-Methods": "GET, POST, OPTIONS", - "Access-Control-Allow-Headers": "Content-Type, Authorization", - }); - res.end(); - return; - } - - // API routes - if ( - url.pathname.startsWith("/functions/") - || url.pathname.startsWith("/api/") - || url.pathname.startsWith("/proxy/") - ) { - const handled = await handleApi(req, res, url); - if (handled) return; - } - - // Static files - const served = await serveStaticFile(dashboardDir, url.pathname, res); - if (served) return; - - // SPA fallback - await serveStaticFile(dashboardDir, "/index.html", res); - } catch (e) { - if (!res.headersSent) { - res.writeHead(500, { "Content-Type": "text/plain" }); - res.end("Internal Server Error"); - } - } - }); + const server = http.createServer(createRequestHandler({ handleApi, dashboardDir })); // 4. Listen. Default startup follows README behavior and picks the next // available port; an explicit --port/PORT remains strict. @@ -316,6 +352,8 @@ module.exports = { NPM_PACKAGE_NAME, LOCAL_BIND_HOST, isPortUnavailableError, + isAllowedHostHeader, + createRequestHandler, listenOnAvailablePort, getLocalServerUrl, parseArgs, diff --git a/src/lib/fs.js b/src/lib/fs.js index 28cebea6..b2d8ca00 100644 --- a/src/lib/fs.js +++ b/src/lib/fs.js @@ -1,4 +1,5 @@ const fs = require("node:fs/promises"); +const os = require("node:os"); const path = require("node:path"); async function ensureDir(p) { @@ -47,37 +48,157 @@ async function chmod600IfPossible(filePath) { } catch (_e) {} } -const LOCK_STALE_MS = 5 * 60 * 1000; // 5 minutes +// The holder heartbeats the lock's mtime, so "stale" now means "the holder +// died", not "the holder is slow". That lets the threshold be generous: the old +// 5-minute window silently stole the lock from any sync that ran longer than +// one local-sync tick (full-corpus rebuilds and migration reparses do — see the +// post-mortem at src/commands/sync.js:74-123), letting two writers interleave +// appends into queue.jsonl. A torn line is skipped by the reader, and a skipped +// retraction row is a permanent silent overcount. Issue #89. +const LOCK_STALE_MS = 30 * 60 * 1000; // 30 minutes +const LOCK_HEARTBEAT_MS = 30 * 1000; // touch mtime every 30s while held +const MAX_LOCK_ATTEMPTS = 3; -async function openLock(lockPath, { quietIfLocked }) { +// mkdir is atomic and exclusive, which makes it a usable mutex on every +// filesystem we care about. Held only for the few syscalls of a takeover. +const TAKEOVER_ABANDONED_MS = 60 * 1000; + +function takeoverMutexPath(lockPath) { + return `${lockPath}.takeover`; +} + +async function acquireTakeoverMutex(lockPath) { + const mutexPath = takeoverMutexPath(lockPath); try { - const handle = await fs.open(lockPath, "wx"); - return { - async release() { - await handle.close().catch(() => {}); - await fs.unlink(lockPath).catch(() => {}); - }, - }; + await fs.mkdir(mutexPath); + return true; } catch (e) { - if (e && e.code === "EEXIST") { - // Check if lock is stale - try { - const stat = await fs.stat(lockPath); - if (Date.now() - stat.mtimeMs > LOCK_STALE_MS) { - await fs.unlink(lockPath).catch(() => {}); - return openLock(lockPath, { quietIfLocked }); - } - } catch (_statErr) { - // Lock file disappeared between checks, retry - return openLock(lockPath, { quietIfLocked }); - } - if (!quietIfLocked) { - process.stdout.write("Another sync is already running.\n"); - } - return null; + if (!e || e.code !== "EEXIST") return false; + } + // A process that died mid-takeover would otherwise block every future + // takeover forever. The window is milliseconds, so anything this old is dead. + try { + const stat = await fs.stat(mutexPath); + if (Date.now() - stat.mtimeMs > TAKEOVER_ABANDONED_MS) { + await fs.rmdir(mutexPath).catch(() => {}); } + } catch (_e) { + // Vanished under us — the caller retries either way. + } + return false; +} + +async function readLockOwner(lockPath) { + try { + const raw = await fs.readFile(lockPath, "utf8"); + const parsed = JSON.parse(raw); + if (parsed && typeof parsed === "object") return parsed; + } catch (_e) { + // Empty, truncated, or pre-upgrade lock file — fall back to the mtime rule. + } + return null; +} + +function isProcessAlive(pid) { + try { + process.kill(pid, 0); + return true; + } catch (e) { + // EPERM means the process exists but belongs to someone else. + return Boolean(e && e.code === "EPERM"); + } +} + +// A lock whose owner process is gone is stale immediately — no need to wait out +// the full window. Only trust the pid when the lock was taken on this host, and +// never when it is our own pid (a recycled pid would look alive forever). +function isOwnerGone(owner) { + if (!owner || typeof owner.pid !== "number" || !Number.isInteger(owner.pid)) return false; + if (owner.host !== os.hostname()) return false; + if (owner.pid === process.pid) return false; + return !isProcessAlive(owner.pid); +} + +// Throws EEXIST when the lock is already held — that is the caller's signal to +// decide whether the existing lock is stale. +async function createHeldLock(lockPath, heartbeatMs) { + const handle = await fs.open(lockPath, "wx"); + const owner = { pid: process.pid, host: os.hostname(), startedAt: new Date().toISOString() }; + + try { + await handle.writeFile(`${JSON.stringify(owner, null, 2)}\n`, { encoding: "utf8" }); + } catch (e) { + // A write failure would otherwise leave a leaked fd plus an empty lock file + // that nothing releases — blocking every sync until the stale window elapses. + await handle.close().catch(() => {}); + await fs.unlink(lockPath).catch(() => {}); throw e; } + + // unref'd so a held lock can never keep the CLI process alive on its own. + const heartbeat = setInterval(() => { + const now = new Date(); + fs.utimes(lockPath, now, now).catch(() => {}); + }, heartbeatMs); + if (typeof heartbeat.unref === "function") heartbeat.unref(); + + return { + owner, + async release() { + clearInterval(heartbeat); + await handle.close().catch(() => {}); + await fs.unlink(lockPath).catch(() => {}); + }, + }; +} + +// Removing a stale lock is a check-then-act, and the caller's check ran outside +// any mutual exclusion: by now another waiter may already have reclaimed it and +// be holding a *fresh* lock. Deleting (or renaming) the path blind would drop +// that winner's lock and let two syncs run at once. The mkdir mutex makes the +// re-check and the delete atomic with respect to other waiters. +async function reclaimStaleLock(lockPath) { + if (!(await acquireTakeoverMutex(lockPath))) return; + try { + const current = await fs.stat(lockPath).catch(() => null); + if (!current) return; + const currentOwner = await readLockOwner(lockPath); + const stillStale = Date.now() - current.mtimeMs > LOCK_STALE_MS || isOwnerGone(currentOwner); + if (stillStale) await fs.unlink(lockPath).catch(() => {}); + } finally { + await fs.rmdir(takeoverMutexPath(lockPath)).catch(() => {}); + } +} + +async function openLock(lockPath, { quietIfLocked, heartbeatMs = LOCK_HEARTBEAT_MS } = {}, attempt = 1) { + try { + return await createHeldLock(lockPath, heartbeatMs); + } catch (e) { + if (!e || e.code !== "EEXIST") throw e; + + const retry = () => openLock(lockPath, { quietIfLocked, heartbeatMs }, attempt + 1); + const giveUp = () => { + if (!quietIfLocked) process.stdout.write("Another sync is already running.\n"); + return null; + }; + + if (attempt >= MAX_LOCK_ATTEMPTS) return giveUp(); + + let stat; + try { + stat = await fs.stat(lockPath); + } catch (_statErr) { + return retry(); // Lock file disappeared between checks. + } + + const owner = await readLockOwner(lockPath); + const expired = Date.now() - stat.mtimeMs > LOCK_STALE_MS; + // A live holder heartbeats its lock, so a fresh mtime means "still working". + if (!expired && !isOwnerGone(owner)) return giveUp(); + + await reclaimStaleLock(lockPath); + return retry(); + } } module.exports = { @@ -88,4 +209,7 @@ module.exports = { writeJson, chmod600IfPossible, openLock, + // Exported for tests. + LOCK_STALE_MS, + LOCK_HEARTBEAT_MS, }; diff --git a/src/lib/local-api.js b/src/lib/local-api.js index b2400731..8eab3228 100644 --- a/src/lib/local-api.js +++ b/src/lib/local-api.js @@ -1522,6 +1522,8 @@ module.exports = { resolveQueuePath, // Exported for cross-consumer tests (pricing + native contract lock). MODEL_PRICING, + // Shared with serve.js so the Host allowlist and the Origin allowlist agree. + isLoopbackHostname, getModelPricing, computeRowCost, ensurePricingLoaded, diff --git a/test/host-header-guard.test.js b/test/host-header-guard.test.js new file mode 100644 index 00000000..a7b67dae --- /dev/null +++ b/test/host-header-guard.test.js @@ -0,0 +1,113 @@ +const assert = require("node:assert/strict"); +const http = require("node:http"); +const os = require("node:os"); +const path = require("node:path"); +const { test } = require("node:test"); + +const { isAllowedHostHeader, createRequestHandler } = require("../src/commands/serve"); + +// Boots a real server with a stub API handler so the assertions cover the +// wiring, not just the predicate: a guard that is never reached would pass a +// predicate-only test while leaving the hole wide open. Issue #88. +async function withServer(run) { + const seen = []; + const handleApi = async (req, res, url) => { + seen.push(url.pathname); + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ secret: "spend history" })); + return true; + }; + const server = http.createServer( + createRequestHandler({ handleApi, dashboardDir: path.join(os.tmpdir(), "tt-no-dashboard") }), + ); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address(); + try { + return await run({ port, seen }); + } finally { + await new Promise((resolve) => server.close(resolve)); + } +} + +function request({ port, hostHeader, method = "GET", pathname = "/functions/tokentracker-usage-summary" }) { + return new Promise((resolve, reject) => { + const headers = {}; + if (hostHeader !== undefined) headers.Host = hostHeader; + const req = http.request({ host: "127.0.0.1", port, method, path: pathname, headers }, (res) => { + let body = ""; + res.on("data", (chunk) => { body += chunk; }); + res.on("end", () => resolve({ status: res.statusCode, body })); + }); + req.on("error", reject); + req.end(); + }); +} + +test("a rebound hostname is rejected before the API handler runs", async () => { + await withServer(async ({ port, seen }) => { + const res = await request({ port, hostHeader: `attacker.example:${port}` }); + assert.equal(res.status, 403); + assert.match(res.body, /loopback/i); + assert.equal(res.body.includes("spend history"), false, "no data may leak in the 403 body"); + assert.deepEqual(seen, [], "the API handler must never be reached"); + }); +}); + +test("an OPTIONS preflight from a rebound hostname is also rejected", async () => { + await withServer(async ({ port }) => { + const res = await request({ port, hostHeader: "attacker.example", method: "OPTIONS" }); + assert.equal(res.status, 403); + }); +}); + +test("loopback hosts still reach the API on any port", async () => { + await withServer(async ({ port, seen }) => { + for (const hostHeader of [`127.0.0.1:${port}`, `localhost:${port}`, "localhost", `[::1]:${port}`]) { + const res = await request({ port, hostHeader }); + assert.equal(res.status, 200, `expected ${hostHeader} to be allowed`); + assert.match(res.body, /spend history/); + } + assert.equal(seen.length, 4); + }); +}); + +test("isAllowedHostHeader accepts every loopback spelling", () => { + for (const host of [ + "127.0.0.1", + "127.0.0.1:17680", + "localhost", + "localhost:7680", + "[::1]", + "[::1]:17680", + ]) { + assert.equal(isAllowedHostHeader(host), true, `${host} should be allowed`); + } +}); + +test("isAllowedHostHeader rejects non-loopback and lookalike hosts", () => { + for (const host of [ + "attacker.example", + "attacker.example:17680", + "tokentracker.local", + "127.0.0.1.attacker.example", + "localhost.attacker.example", + // userinfo trick: the real hostname is after the '@' + "localhost:17680@attacker.example", + "0.0.0.0", + "192.168.1.20:17680", + ]) { + assert.equal(isAllowedHostHeader(host), false, `${host} should be rejected`); + } +}); + +test("an absent or empty Host header is allowed (HTTP/1.0 clients, local probes)", async () => { + assert.equal(isAllowedHostHeader(undefined), true); + assert.equal(isAllowedHostHeader(null), true); + assert.equal(isAllowedHostHeader(""), true); +}); + +test("a malformed Host header is rejected rather than throwing", () => { + for (const host of ["::::", "[unclosed", "%%%"]) { + assert.equal(isAllowedHostHeader(host), false, `${host} should be rejected`); + } +}); diff --git a/test/sync-lock.test.js b/test/sync-lock.test.js new file mode 100644 index 00000000..6f6968a2 --- /dev/null +++ b/test/sync-lock.test.js @@ -0,0 +1,160 @@ +const assert = require("node:assert/strict"); +const { spawn } = require("node:child_process"); +const fs = require("node:fs/promises"); +const os = require("node:os"); +const path = require("node:path"); +const { test } = require("node:test"); + +const { openLock, LOCK_STALE_MS } = require("../src/lib/fs"); + +async function tmpLockPath(name) { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), `tt-lock-${name}-`)); + return path.join(dir, "sync.lock"); +} + +async function backdate(lockPath, ms) { + const when = new Date(Date.now() - ms); + await fs.utimes(lockPath, when, when); +} + +async function readOwner(lockPath) { + return JSON.parse(await fs.readFile(lockPath, "utf8")); +} + +const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + +// A pid that is definitely gone: spawn a child, kill it, wait for exit. +async function deadPid() { + const child = spawn(process.execPath, ["-e", "setTimeout(() => {}, 60000)"], { stdio: "ignore" }); + const pid = child.pid; + await new Promise((resolve) => { + child.once("exit", resolve); + child.kill("SIGKILL"); + }); + return pid; +} + +test("the lock records who holds it, and release removes it", async () => { + const lockPath = await tmpLockPath("owner"); + const lock = await openLock(lockPath, { quietIfLocked: true }); + assert.ok(lock, "expected to acquire the lock"); + + const owner = await readOwner(lockPath); + assert.equal(owner.pid, process.pid); + assert.equal(owner.host, os.hostname()); + assert.ok(Date.parse(owner.startedAt) > 0, "startedAt must be an ISO timestamp"); + + await lock.release(); + await assert.rejects(fs.stat(lockPath), { code: "ENOENT" }); +}); + +test("a lock held by a live holder is not stolen, even past the old 5-minute window", async () => { + const lockPath = await tmpLockPath("live"); + const held = await openLock(lockPath, { quietIfLocked: true }); + assert.ok(held); + + // Older than the window that used to steal it, newer than the current one. + await backdate(lockPath, 6 * 60 * 1000); + + const second = await openLock(lockPath, { quietIfLocked: true }); + assert.equal(second, null, "a heartbeating holder must keep its lock"); + + await held.release(); +}); + +test("the heartbeat keeps advancing the lock mtime while it is held", async () => { + const lockPath = await tmpLockPath("heartbeat"); + const lock = await openLock(lockPath, { quietIfLocked: true, heartbeatMs: 20 }); + assert.ok(lock); + + await backdate(lockPath, 10 * 60 * 1000); + const stale = (await fs.stat(lockPath)).mtimeMs; + + await sleep(120); + const beating = (await fs.stat(lockPath)).mtimeMs; + assert.ok(beating > stale, "heartbeat should have touched the lock"); + + await lock.release(); +}); + +test("release stops the heartbeat", async () => { + const lockPath = await tmpLockPath("release-stops"); + const lock = await openLock(lockPath, { quietIfLocked: true, heartbeatMs: 20 }); + await lock.release(); + + // Re-create the file so there is something for a leaked timer to touch. + await fs.writeFile(lockPath, "{}\n"); + await backdate(lockPath, 60 * 1000); + const before = (await fs.stat(lockPath)).mtimeMs; + + await sleep(120); + const after = (await fs.stat(lockPath)).mtimeMs; + assert.equal(after, before, "a released lock must not keep touching the path"); +}); + +test("a lock past the stale window is taken over", async () => { + const lockPath = await tmpLockPath("stale"); + const first = await openLock(lockPath, { quietIfLocked: true }); + assert.ok(first); + // Simulate a dead holder: stop the heartbeat, then age the file out. + await first.release(); + await fs.writeFile(lockPath, JSON.stringify({ pid: process.pid, host: os.hostname() })); + await backdate(lockPath, LOCK_STALE_MS + 60_000); + + const second = await openLock(lockPath, { quietIfLocked: true }); + assert.ok(second, "an expired lock should be reclaimed"); + assert.equal((await readOwner(lockPath)).pid, process.pid); + await second.release(); +}); + +test("a lock whose owner process is gone is reclaimed immediately, without waiting out the window", async () => { + const lockPath = await tmpLockPath("dead-owner"); + const pid = await deadPid(); + await fs.writeFile(lockPath, JSON.stringify({ pid, host: os.hostname(), startedAt: new Date().toISOString() })); + // mtime is fresh — only the dead pid justifies the takeover. + + const lock = await openLock(lockPath, { quietIfLocked: true }); + assert.ok(lock, "a lock owned by a dead process should be reclaimed at once"); + assert.equal((await readOwner(lockPath)).pid, process.pid); + await lock.release(); +}); + +test("a fresh lock recorded on another host is left alone even if the pid looks dead", async () => { + const lockPath = await tmpLockPath("other-host"); + const pid = await deadPid(); + await fs.writeFile(lockPath, JSON.stringify({ pid, host: `${os.hostname()}-elsewhere` })); + + const lock = await openLock(lockPath, { quietIfLocked: true }); + assert.equal(lock, null, "pid liveness is only meaningful on the recording host"); +}); + +test("a corrupt or pre-upgrade lock file falls back to the mtime rule", async () => { + const lockPath = await tmpLockPath("corrupt"); + await fs.writeFile(lockPath, "not json at all"); + + const fresh = await openLock(lockPath, { quietIfLocked: true }); + assert.equal(fresh, null, "a fresh unreadable lock is still a lock"); + + await backdate(lockPath, LOCK_STALE_MS + 60_000); + const reclaimed = await openLock(lockPath, { quietIfLocked: true }); + assert.ok(reclaimed, "an aged unreadable lock should be reclaimed"); + await reclaimed.release(); +}); + +test("concurrent takeover of one stale lock yields exactly one winner", async () => { + const lockPath = await tmpLockPath("race"); + await fs.writeFile(lockPath, JSON.stringify({ pid: 1, host: `${os.hostname()}-elsewhere` })); + await backdate(lockPath, LOCK_STALE_MS + 60_000); + + const results = await Promise.all( + Array.from({ length: 4 }, () => openLock(lockPath, { quietIfLocked: true })), + ); + const winners = results.filter(Boolean); + assert.equal(winners.length, 1, `expected exactly one winner, got ${winners.length}`); + + // The takeover mutex must not be left behind, or every future takeover blocks. + const leftovers = (await fs.readdir(path.dirname(lockPath))).filter((f) => f.includes(".takeover")); + assert.deepEqual(leftovers, []); + + await winners[0].release(); +}); From f73195a6bffc0d47a0fb7f618b4f0af047c01229 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 06:12:48 +0700 Subject: [PATCH 3/5] fix(serve,pricing): close QA findings on the Host guard and expiry check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent QA pass (Codex, xhigh) on the merged preview of #91 + #92. - Refuse absolute-form request targets. Host said loopback while the target carried its own authority, and routing parses the absolute URL — so the allowlist and the router disagreed about which site the request was for. Not reachable from a browser (absolute-form only goes to proxies), but a parser differential is not something to leave open in the one guard that stands between a rebound page and the whole spend history. - Allow the fully-qualified loopback spelling. WHATWG URL canonicalises a trailing dot away for IPv4 literals but not for names, so `localhost.` got a spurious 403 while `127.0.0.1.` passed. - Refuse userinfo in a Host header. `evil.example@127.0.0.1` was accepted; the origin genuinely is loopback so this was never a bypass, but Host has no userinfo component and anything carrying one is malformed. - Scan _meta values, not key names, for unenforced dates. Matching only `*_expiry` meant `promo_cutover: "2026-08-31 — update the price"` sailed past and would have expired in silence — the exact failure the validator exists to prevent. Now any YYYY-MM-DD parked anywhere in _meta (including nested) must live in the expiries array. --- scripts/validate-curated-expiry.cjs | 33 +++++++++++---- src/commands/serve.js | 27 ++++++++++++- test/curated-expiry.test.js | 27 ++++++++++--- test/host-header-guard.test.js | 63 ++++++++++++++++++++++++++++- 4 files changed, 136 insertions(+), 14 deletions(-) diff --git a/scripts/validate-curated-expiry.cjs b/scripts/validate-curated-expiry.cjs index cea270a0..09fbd3c6 100644 --- a/scripts/validate-curated-expiry.cjs +++ b/scripts/validate-curated-expiry.cjs @@ -17,6 +17,8 @@ const ROOT = path.resolve(__dirname, ".."); const OVERRIDES_PATH = path.join(ROOT, "src", "lib", "pricing", "curated-overrides.json"); const DATE_RE = /^\d{4}-\d{2}-\d{2}$/; +// Any YYYY-MM-DD appearing anywhere inside a free-text note. +const DATE_IN_TEXT_RE = /\d{4}-\d{2}-\d{2}/; const REQUIRED_FIELDS = ["id", "expires_at", "what", "action"]; // An expiry is due at UTC midnight on its date, so an entry dated 2026-08-31 @@ -30,6 +32,14 @@ function parseExpiryMs(value) { return ms; } +// Walks a _meta value of any shape so a date cannot hide one level down. +function collectStrings(value, out = []) { + if (typeof value === "string") out.push(value); + else if (Array.isArray(value)) value.forEach((v) => collectStrings(v, out)); + else if (value && typeof value === "object") Object.values(value).forEach((v) => collectStrings(v, out)); + return out; +} + function isNonEmptyString(value) { return typeof value === "string" && value.trim() !== ""; } @@ -43,13 +53,22 @@ function checkExpiries(meta, nowMs) { return { errors: ["_meta is missing or not an object"], checked: 0 }; } - // Guard against regressing to the pattern this check replaced. - for (const key of Object.keys(meta)) { - if (/_expiry$/.test(key)) { - errors.push( - `_meta.${key}: free-text expiry keys are not checked by anything. ` - + "Move it into the _meta.expiries array so it is enforced.", - ); + // Guard against regressing to the pattern this check replaced. Matching on + // the key name alone was too weak — `promo_cutover: "2026-08-31 — update the + // price"` would sail straight past a `*_expiry` name check and expire in + // silence, which is the exact failure being designed out. So scan the VALUES: + // any date-looking string parked in _meta is a time-boxed fact that belongs + // in `expiries`, whatever its key is called. + for (const [key, value] of Object.entries(meta)) { + if (key === "expiries") continue; + for (const text of collectStrings(value)) { + if (DATE_IN_TEXT_RE.test(text)) { + errors.push( + `_meta.${key}: contains a date ("${text.slice(0, 60).trim()}…") but nothing enforces it. ` + + "Move the fact into the _meta.expiries array, or drop the date from the note.", + ); + break; + } } } diff --git a/src/commands/serve.js b/src/commands/serve.js index 8b1a3aee..3e302951 100644 --- a/src/commands/serve.js +++ b/src/commands/serve.js @@ -34,12 +34,30 @@ const LOCAL_BIND_HOST = "127.0.0.1"; function isAllowedHostHeader(hostHeader) { if (hostHeader == null || hostHeader === "") return true; try { - return isLoopbackHostname(new URL(`http://${hostHeader}`).hostname); + const url = new URL(`http://${hostHeader}`); + // Userinfo has no meaning in a Host header. Rejecting it outright removes a + // parser-differential class rather than relying on every parser agreeing on + // where the authority ends. + if (url.username || url.password) return false; + // `localhost.` is the valid fully-qualified spelling of localhost. WHATWG + // URL canonicalises the trailing dot away for IPv4 literals but not for + // names, so strip it here or the FQDN form gets a spurious 403. + return isLoopbackHostname(url.hostname.replace(/\.$/, "")); } catch (_e) { return false; } } +// An origin server is not a proxy: a request-target must be origin-form +// ("/path") or asterisk-form ("*"). Absolute-form ("GET http://evil/x") carries +// its own authority, which WOULD win over the Host header when the URL is +// parsed for routing — so the Host allowlist and the routing would disagree +// about which site this request is for. Refuse instead of picking a winner. +function isAllowedRequestTarget(target) { + if (target == null || target === "") return false; + return target === "*" || target.startsWith("/"); +} + // Extracted from cmdServe so the wiring — not just the predicate — is testable: // a guard that exists but is never reached is the failure mode this is guarding // against in the first place. @@ -53,6 +71,12 @@ function createRequestHandler({ handleApi, dashboardDir }) { return; } + if (!isAllowedRequestTarget(req.url)) { + res.writeHead(400, { "Content-Type": "text/plain" }); + res.end("Bad Request: absolute-form request targets are not served.\n"); + return; + } + const url = new URL(req.url || "/", `http://${req.headers.host || "localhost"}`); // CORS preflight @@ -353,6 +377,7 @@ module.exports = { LOCAL_BIND_HOST, isPortUnavailableError, isAllowedHostHeader, + isAllowedRequestTarget, createRequestHandler, listenOnAvailablePort, getLocalServerUrl, diff --git a/test/curated-expiry.test.js b/test/curated-expiry.test.js index 1ba558a7..49b81d2c 100644 --- a/test/curated-expiry.test.js +++ b/test/curated-expiry.test.js @@ -65,17 +65,34 @@ test("rejects duplicate ids", () => { assert.ok(errors.some((e) => e.includes("duplicate id 'sample'"))); }); -test("rejects a regression to free-text *_expiry keys", () => { +test("rejects a date parked in free text, whatever the key is called", () => { + // Key-name matching alone was too weak: `promo_cutover` would have sailed + // past a `*_expiry` check and then expired in silence — the exact failure + // this validator exists to prevent. + for (const key of ["some_promo_expiry", "promo_cutover", "note", "todo"]) { + const { errors } = checkExpiries( + { [key]: "2026-05-31 — remember to update this", expiries: [] }, + AT("2026-01-01"), + ); + assert.strictEqual(errors.length, 1, `expected ${key} to be flagged`); + assert.match(errors[0], /nothing enforces it/); + } +}); + +test("finds a date nested inside an object or array in _meta", () => { const { errors } = checkExpiries( - { some_promo_expiry: "2026-05-31 — remember to update this", expiries: [] }, + { notes: { deep: ["all fine", "revisit 2026-09-01"] }, expiries: [] }, AT("2026-01-01"), ); assert.strictEqual(errors.length, 1); - assert.match(errors[0], /free-text expiry keys are not checked/); + assert.match(errors[0], /_meta.notes/); }); -test("expiries_note is allowed even though it ends in a checked-looking word", () => { - const { errors } = checkExpiries({ expiries_note: "how this works", expiries: [] }, AT("2026-01-01")); +test("prose without a date is left alone", () => { + const { errors } = checkExpiries( + { expiries_note: "how this works", units: "usd_per_million_tokens", expiries: [] }, + AT("2026-01-01"), + ); assert.deepStrictEqual(errors, []); }); diff --git a/test/host-header-guard.test.js b/test/host-header-guard.test.js index a7b67dae..2c687861 100644 --- a/test/host-header-guard.test.js +++ b/test/host-header-guard.test.js @@ -4,7 +4,11 @@ const os = require("node:os"); const path = require("node:path"); const { test } = require("node:test"); -const { isAllowedHostHeader, createRequestHandler } = require("../src/commands/serve"); +const { + isAllowedHostHeader, + isAllowedRequestTarget, + createRequestHandler, +} = require("../src/commands/serve"); // Boots a real server with a stub API handler so the assertions cover the // wiring, not just the predicate: a guard that is never reached would pass a @@ -29,6 +33,21 @@ async function withServer(run) { } } +// Raw socket write, so the request-target can be absolute-form — http.request +// always sends origin-form and cannot express it. +function rawRequest(port, requestLine, extraHeaders = "") { + const net = require("node:net"); + return new Promise((resolve, reject) => { + const socket = net.connect(port, "127.0.0.1", () => { + socket.write(`${requestLine}\r\n${extraHeaders}Connection: close\r\n\r\n`); + }); + let body = ""; + socket.on("data", (chunk) => { body += chunk; }); + socket.on("end", () => resolve(body)); + socket.on("error", reject); + }); +} + function request({ port, hostHeader, method = "GET", pathname = "/functions/tokentracker-usage-summary" }) { return new Promise((resolve, reject) => { const headers = {}; @@ -111,3 +130,45 @@ test("a malformed Host header is rejected rather than throwing", () => { assert.equal(isAllowedHostHeader(host), false, `${host} should be rejected`); } }); + +test("an absolute-form request target is refused rather than routed by its own authority", async () => { + // Host says loopback, the target says otherwise. Routing would parse the + // absolute URL and take evil.example as the authority, disagreeing with the + // allowlist that just passed the request. + await withServer(async ({ port, seen }) => { + const response = await rawRequest( + port, + "GET http://evil.example/functions/tokentracker-usage-summary HTTP/1.1", + "Host: localhost\r\n", + ); + assert.match(response.split("\r\n")[0], /400/); + assert.equal(response.includes("spend history"), false); + assert.deepEqual(seen, [], "the API handler must never be reached"); + }); +}); + +test("the fully-qualified loopback spelling is allowed", () => { + for (const host of ["localhost.", "localhost.:17680", "127.0.0.1.", "127.0.0.1.:7680"]) { + assert.equal(isAllowedHostHeader(host), true, `${host} should be allowed`); + } + // Stripping the dot must not turn a foreign name into a loopback one. + assert.equal(isAllowedHostHeader("evil.example."), false); + assert.equal(isAllowedHostHeader("localhost.evil.example."), false); +}); + +test("userinfo in a Host header is refused even when the host itself is loopback", () => { + // Not a rebinding bypass on its own — the origin really is 127.0.0.1 — but + // Host has no userinfo component, so anything carrying one is malformed and + // only creates room for two parsers to disagree. + assert.equal(isAllowedHostHeader("evil.example@127.0.0.1"), false); + assert.equal(isAllowedHostHeader("user:pass@localhost"), false); +}); + +test("isAllowedRequestTarget accepts only origin-form and asterisk-form", () => { + for (const target of ["/", "/functions/x", "/api/y?z=1", "*"]) { + assert.equal(isAllowedRequestTarget(target), true, `${target} should be allowed`); + } + for (const target of ["http://evil.example/x", "https://evil.example/x", "evil.example:443", "", null]) { + assert.equal(isAllowedRequestTarget(target), false, `${JSON.stringify(target)} should be refused`); + } +}); From 5c2516279ac18379ba8e5f7dab09a28a6140089b Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 06:29:28 +0700 Subject: [PATCH 4/5] fix(serve): treat empty userinfo as userinfo, refuse network-path targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA re-check found two gaps in the previous round's guard. - "@localhost" and ":@localhost" parse to a falsy url.username, so checking the parsed fields let exactly the malformed forms through while the fully-spelled "user:pass@localhost" was refused. Test the raw header for "@" instead. - "//evil/x" and "/\\evil/x" start with a slash and so passed the origin-form check, but WHATWG URL resolves both against a foreign authority (new URL("/\\evil/x", "http://localhost").hostname === "evil"). Routing only reads url.pathname today, so nothing is exploitable now — but handing a handler a URL that points at someone else's origin is the same guard-vs-parser disagreement absolute-form creates. --- src/commands/serve.js | 22 ++++++++++++++++------ test/host-header-guard.test.js | 19 +++++++++++++++++++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/commands/serve.js b/src/commands/serve.js index 3e302951..b5d993db 100644 --- a/src/commands/serve.js +++ b/src/commands/serve.js @@ -33,12 +33,13 @@ const LOCAL_BIND_HOST = "127.0.0.1"; // browser sending a name. function isAllowedHostHeader(hostHeader) { if (hostHeader == null || hostHeader === "") return true; + // Userinfo has no meaning in a Host header, so anything carrying it is + // malformed. Tested on the RAW value: an EMPTY userinfo ("@localhost", + // ":@localhost") parses to a falsy url.username, so checking the parsed + // fields alone lets exactly the malformed forms through. + if (hostHeader.includes("@")) return false; try { const url = new URL(`http://${hostHeader}`); - // Userinfo has no meaning in a Host header. Rejecting it outright removes a - // parser-differential class rather than relying on every parser agreeing on - // where the authority ends. - if (url.username || url.password) return false; // `localhost.` is the valid fully-qualified spelling of localhost. WHATWG // URL canonicalises the trailing dot away for IPv4 literals but not for // names, so strip it here or the FQDN form gets a spurious 403. @@ -54,8 +55,17 @@ function isAllowedHostHeader(hostHeader) { // parsed for routing — so the Host allowlist and the routing would disagree // about which site this request is for. Refuse instead of picking a winner. function isAllowedRequestTarget(target) { - if (target == null || target === "") return false; - return target === "*" || target.startsWith("/"); + if (typeof target !== "string" || target === "") return false; + if (target === "*") return true; + if (!target.startsWith("/")) return false; + // "//evil/x" is a network-path reference, and WHATWG URL treats a backslash + // like a slash, so "/\evil/x" behaves the same way: both make the parsed URL + // adopt a foreign authority even though the Host header said loopback. + // Routing only reads url.pathname today, but leaving the parsed URL pointing + // at someone else's origin is the same guard-vs-parser disagreement that + // absolute-form creates. + if (target.startsWith("//") || target.startsWith("/\\")) return false; + return true; } // Extracted from cmdServe so the wiring — not just the predicate — is testable: diff --git a/test/host-header-guard.test.js b/test/host-header-guard.test.js index 2c687861..d112ba44 100644 --- a/test/host-header-guard.test.js +++ b/test/host-header-guard.test.js @@ -172,3 +172,22 @@ test("isAllowedRequestTarget accepts only origin-form and asterisk-form", () => assert.equal(isAllowedRequestTarget(target), false, `${JSON.stringify(target)} should be refused`); } }); + +test("an empty userinfo is still userinfo", () => { + // These parse to a falsy url.username, so a check on the parsed fields alone + // would wave them through while rejecting the fully-spelled form. + for (const host of ["@localhost", ":@localhost", "@127.0.0.1", "@evil.example"]) { + assert.equal(isAllowedHostHeader(host), false, `${host} should be refused`); + } +}); + +test("network-path and backslash targets are refused like absolute-form", () => { + // new URL("/\\evil/x", "http://localhost").hostname === "evil" + for (const target of ["//evil.example/x", "/\\evil.example/x", "//x", "/\\x"]) { + assert.equal(isAllowedRequestTarget(target), false, `${JSON.stringify(target)} should be refused`); + } + // A single slash followed by a normal path is still fine. + for (const target of ["/", "/a//b", "/a/\\b"]) { + assert.equal(isAllowedRequestTarget(target), true, `${JSON.stringify(target)} should be allowed`); + } +}); From 693fa9f2f823b7dd5a8836cc9c03960091899db1 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 06:48:43 +0700 Subject: [PATCH 5/5] fix(serve): refuse control characters in the Host header and request target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA re-check found the last hole in the prefix checks: WHATWG URL strips tab, LF and CR from its input BEFORE parsing, so "///evil/x" becomes "//evil/x" and adopts a foreign authority after passing a startsWith("//") test. Same trick applies to the Host string. Verified over a socket that Node's own parser returns 400 for those bytes in a request-target before the handler ever runs, so this was not reachable through the real server — recorded in the test so the next reader does not have to re-derive it. Fixed anyway: a guard that holds only because a different layer happens to be strict is exactly the guard-vs-parser disagreement this function exists to prevent. --- src/commands/serve.js | 10 ++++++++++ test/host-header-guard.test.js | 15 +++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/commands/serve.js b/src/commands/serve.js index b5d993db..0440f1f5 100644 --- a/src/commands/serve.js +++ b/src/commands/serve.js @@ -38,6 +38,9 @@ function isAllowedHostHeader(hostHeader) { // ":@localhost") parses to a falsy url.username, so checking the parsed // fields alone lets exactly the malformed forms through. if (hostHeader.includes("@")) return false; + // Same reason as the request target: URL parsing strips these bytes, so they + // can smuggle a different authority past a check on the raw string. + if (/[\u0000-\u0020\u007f]/.test(hostHeader)) return false; try { const url = new URL(`http://${hostHeader}`); // `localhost.` is the valid fully-qualified spelling of localhost. WHATWG @@ -65,6 +68,13 @@ function isAllowedRequestTarget(target) { // at someone else's origin is the same guard-vs-parser disagreement that // absolute-form creates. if (target.startsWith("//") || target.startsWith("/\\")) return false; + // WHATWG URL strips tab, LF and CR from its input BEFORE parsing, so + // "///evil/x" becomes "//evil/x" and adopts a foreign authority after + // passing the prefix checks above. Node's own parser rejects these bytes in a + // request-target with a 400 before the handler runs, so this is not reachable + // over the wire today — but a guard that only holds because a different layer + // is strict is the disagreement this function exists to prevent. + if (/[\u0000-\u0020\u007f]/.test(target)) return false; return true; } diff --git a/test/host-header-guard.test.js b/test/host-header-guard.test.js index d112ba44..32ba71b9 100644 --- a/test/host-header-guard.test.js +++ b/test/host-header-guard.test.js @@ -191,3 +191,18 @@ test("network-path and backslash targets are refused like absolute-form", () => assert.equal(isAllowedRequestTarget(target), true, `${JSON.stringify(target)} should be allowed`); } }); + +test("control characters cannot smuggle an authority past the prefix checks", () => { + // new URL() strips tab/LF/CR before parsing, so "///evil/x" resolves to + // authority "evil". Node returns 400 for these bytes in a request-target + // before the handler runs (verified over a socket), so this is defence in + // depth — but the guard should not depend on another layer being strict. + for (const target of ["/\t//evil.example/x", "/\t/evil.example/x", "/\n//evil/x", "/\r//evil/x"]) { + assert.equal(isAllowedRequestTarget(target), false, `${JSON.stringify(target)} should be refused`); + // Documents WHY: the parsed authority is not loopback. + assert.notEqual(new URL(target, "http://localhost").hostname, "localhost"); + } + for (const host of ["local\thost", "localhost\n", "loc\rallhost"]) { + assert.equal(isAllowedHostHeader(host), false, `${JSON.stringify(host)} should be refused`); + } +});