diff --git a/CLAUDE.md b/CLAUDE.md index 84b6b5a7..075c79ab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,14 +31,17 @@ Open: roadmap milestone 11's two-platform deploy proof — 1.1.0 gave a scaffold deployable artifact (`packages/cli/src/serve.ts`; `x new` writes `apps/web/server.ts`, `prerender.ts`, a Dockerfile and `docker-compose.prod.yml`; `ROLE=migrate` runs release-phase migrations), but the demo app on Compose **and** K8s from one image with an invisible rolling -restart is still not demonstrated. Four known gaps ship with 1.1.0 and are named in -[`CHANGELOG.md`](CHANGELOG.md): `x build --target binary` compiled and crashed at import — **fixed**, the -version read is lazy and `x build` passes `--define ULTIMATE_FRAMEWORK_VERSION`, though the target -is still unproven end to end; -`docker-compose.prod.yml` pairs a published host port with `replicas: 3`; the shared cache tier's -Lua invalidation `DEL`s keys it never declares in `KEYS`, so it fails on Dragonfly and Redis -Cluster; `resolveEnvironment` exists in both `core` and `seo` with different return types. Milestone -detail: [`docs/idea/14-roadmap.md`](docs/idea/14-roadmap.md). +restart is still not demonstrated. Of the four known gaps named in +[`CHANGELOG.md`](CHANGELOG.md), **two are now closed and two remain**, `As of 2026-08`: + +| Gap | State | +|---|---| +| `x build --target binary` compiled and crashed at import | **fixed** — the version read is lazy and `x build` passes `--define ULTIMATE_FRAMEWORK_VERSION`. The target is still unproven end to end, and `docker/Dockerfile` compiles the binary *without* that define | +| the shared cache tier's Lua invalidation `DEL`s keys it never declares in `KEYS` | **fixed** — the script returns the member list and the tier deletes value keys client-side, one key per `DEL`, so it is slot-local on Redis Cluster and Dragonfly | +| `docker-compose.prod.yml` pairs a published host port with `replicas` above 1 | **open** — and it is `web` *and* `sync`, in the framework's compose file, the demo's, and the one `x new` scaffolds | +| `resolveEnvironment` exists in both `core` and `seo` with different return types | **open** — a real axiom-1 violation, deliberately deferred: both are shipped public APIs with different return unions, so unifying them is a breaking change that needs a major | + +Milestone detail: [`docs/idea/14-roadmap.md`](docs/idea/14-roadmap.md). ## Design axioms (override any instinct that conflicts) diff --git a/framework.manifest.json b/framework.manifest.json index da476974..c2cc9280 100644 --- a/framework.manifest.json +++ b/framework.manifest.json @@ -1,6 +1,6 @@ { "version": 1, - "buildId": "909ec1253bd0eb60d74d9efc72f93fb113e4a152054998eee8a7617c2dcc75c1", + "buildId": "ccbdbee407e4ce19555992f302f7f4941a14598c4dd2d28beb69b5f04774c59a", "tiers": { "0": [ "core", @@ -445,6 +445,11 @@ "owner": "cache", "at": "packages/cache/src/errors.ts" }, + { + "code": "X_CACHE_TTL_INVALID", + "owner": "cache", + "at": "packages/cache/src/errors.ts" + }, { "code": "X_CATALOG_INVALID", "owner": "i18n", diff --git a/packages/cache/CLAUDE.md b/packages/cache/CLAUDE.md index 8ebede76..12ea95c8 100644 --- a/packages/cache/CLAUDE.md +++ b/packages/cache/CLAUDE.md @@ -24,7 +24,29 @@ Tier 1. Tagged caching + THE invalidation graph. swallowing it would return `undefined` as if it were the value. `LruCache.set` still throws `X_CACHE_TOO_LARGE` to a direct caller — the stack is the layer that degrades, not the tier. - `tag.x` typing comes from the `CacheTagRegistry` augmentation, generated by `x manifest`. -- Clocks are injected (`LruOptions.clock`); read them through `nowMs()`. +- Clocks are injected (`LruOptions.clock`, `CacheStackOptions.clock`); read them through `nowMs()`. +- **`ttlMs` is positive and finite, and `assertTtl` (in `tiers.ts`) is the one place that says so.** + Every tier calls it before it writes. `0` used to be "never expires" here and `EX 1` in `redis.ts`, + so one stack answered two ways; the rule lives beside `CacheSetOptions` precisely so a new tier + cannot invent a third reading. `X_CACHE_TTL_INVALID`, never a resolution. +- **A promotion carries the entry's remaining life, not `options.ttlMs`** — `createCacheStack.read` + writes the closer tiers with `hit.expiresAt - now`, and drops a hit that fails `isExpired`. A + fresh full lease per read is a hot key that never goes stale enough to refetch. `isExpired` was + exported and unit-tested and called by nothing; the stack is its one caller. +- **Every tier's `get` therefore reports `expiresAt`, or the promotion above has nothing to carry.** + `redis.ts` reads it from `PTTL`, issued alongside the `GET` so Bun pipelines the pair — the server + owns the clock, so it survives skew between the node that wrote and the node that reads, and no + stored payload shape changes under a running deployment. `-1`/`-2` are sentinels, not durations: + they mean no expiry, never one millisecond ago. +- **`redis.ts`'s script deletes only keys it was handed in `KEYS`.** The members of a tag set are + value keys in slots this node may not own, so `DEL`ing them from Lua is a cross-slot access that + fails on Redis Cluster and Dragonfly strict mode — into `report.errors`, so the bust reads as + partial and stale rows serve until TTL. The script returns the members; the tier deletes them + client-side, one key per `DEL`, which is slot-local under every topology. +- **`report.cdn` is what depends on the tags; `report.tiers` is what cleared.** The `cdn` tier + purges `cdn-path` dependents itself, alongside the tags, so `busted` is built from `tiers` + + `isr` + `liveQueries` and never from `cdn` — folding in a list nothing purged is exactly the + partial-bust-reading-as-clean this log exists to prevent. - A purge driver is selected by `selectPurgeDriver` from the environment, never from an `app.config.ts` field — nothing loads that file's contents at runtime. Two CDN credentials at once is refused, not resolved, and half a pair is refused too: "no CDN" is the one wrong answer, diff --git a/packages/cache/README.md b/packages/cache/README.md index 2a0156e0..d71034a0 100644 --- a/packages/cache/README.md +++ b/packages/cache/README.md @@ -27,7 +27,7 @@ Reads walk down until a hit, then populate every tier they walked past. Writes p |---|---|---|---|---| | 0 | `request-memo` | ALS context (`WeakMap`) | dies with the request | never | | 1 | `lru` | in-process, byte-budgeted | tag index | never | -| 2 | `redis` | `Bun.redis` | tag→keys set, one `EVAL` | single node | +| 2 | `redis` | `Bun.redis` | tag→keys set, one `EVAL` + slot-local `DEL`s | single node | | 3 | `cdn` | headers + purge driver | surrogate keys | no CDN | A tier is a `CacheTier` (`get`/`set`/`del`/`invalidateTags`). Swap or omit any of them @@ -45,6 +45,19 @@ const feed = await stack.read('feed:org-1', () => db.posts.recent(), { }); ``` +**`ttlMs` is positive and finite, in every tier.** Omit it for the tier's default; anything else +is `X_CACHE_TTL_INVALID`. There is no "never expires" and no "do not cache" — `0` used to mean the +first in the LRU tier and one second in the Redis tier, so a stack holding both answered +differently depending on which one hit, and neither reading was what the caller meant. A value you +do not want held is a value you do not put in the cache. + +**A promoted hit carries its own remaining life.** When a read hits a far tier and populates the +closer ones, it writes them with `expiresAt - now`, not with the `ttlMs` the caller passed — a +fresh full lease on every read is a hot key that never gets stale enough to be refetched. An entry +already past its expiry is dropped on the way through and the read falls to `load()`. Each tier +supplies that expiry from its own store, so the number is real: the Redis tier reads `PTTL` +alongside the value, in the same pipelined round trip. + Every tier call the stack makes is best-effort: a tier that throws on `get`, `set` or `del` is a tier that did not answer, so `read`, `write` and `drop` carry on. A feed too big for the LRU (`X_CACHE_TOO_LARGE`) or a Redis with no socket costs the entry, never the read. The one call left @@ -89,7 +102,7 @@ One function. Returns the report the `/_x` cache panel and `x cache bust --json` "tags": ["post:1"], "tiers": [{ "tier": "lru", "keys": ["feed"] }, { "tier": "redis", "keys": ["feed"] }], "isr": ["/blog", "/blog/hello"], - "cdn": ["post:1"], + "cdn": ["/feed.xml"], "liveQueries": [], "durationMs": 1.4, "errors": [] @@ -99,6 +112,12 @@ One function. Returns the report the `/_x` cache panel and `x cache bust --json` A dead tier lands in `errors` and never throws — a Redis outage must not fail the write that triggered the bust. Entries there expire by TTL instead. +`cdn` is what the dependency graph hangs off these tags, not what cleared: the `cdn` tier purges +those paths (as surrogate keys, alongside the tags), so what actually cleared is that tier's row +in `tiers`. With no `cdn` tier registered the list purges nowhere, which is why +`recentInvalidations()` reports `busted` from `tiers` and never from `cdn` — a partial bust that +reads as a clean one is the failure that log exists to catch. + Every report is also kept: `recentInvalidations()` hands back the last 100, newest first, each one naming the span that triggered it. That is the log the `/_x` cache panel renders — "did it actually clear?" is answerable without a log dive because the one fan-out path retained the @@ -113,7 +132,9 @@ cacheHeaders({ sMaxAge: 300, staleWhileRevalidate: 86_400, tags: [tag('post', id ``` The surrogate keys **are** the tags, byte for byte, so an edge purge and an app-level -invalidation can never mean different things. Three `PurgeDriver`s ship: +invalidation can never mean different things. A `cdn-path` dependent registered against a tag goes +out in the same purge — as a surrogate key, the one currency `PurgeDriver` has — so a host +registering one must tag that response with its own path. Three `PurgeDriver`s ship: | Driver | Purge | Purge all | Batch | |---|---|---|---| @@ -155,6 +176,7 @@ cache). | `X_CACHE_PURGE_FAILED` | the CDN refused a purge, or a key it would split on whitespace | | `X_CACHE_TAG_UNKNOWN` | a tag no entity declared — usually a typo | | `X_CACHE_TOO_LARGE` | one entry exceeds a tier's whole byte budget | +| `X_CACHE_TTL_INVALID` | a `ttlMs` that is not a positive, finite number of milliseconds | ## Boundary diff --git a/packages/cache/src/cdn.ts b/packages/cache/src/cdn.ts index 1ff488f0..7f9e2611 100644 --- a/packages/cache/src/cdn.ts +++ b/packages/cache/src/cdn.ts @@ -3,6 +3,7 @@ // the response, and purging by surrogate key when a tag changes. Surrogate keys ARE the // tags — same strings, so a CDN purge cannot drift from an app-level invalidation. +import { dependentsOfKind } from './graph'; import type { CacheTag } from './tags'; import { serializeTags } from './tags'; import type { CacheEntry, CacheSetOptions, CacheTier, TierInvalidation } from './tiers'; @@ -96,8 +97,18 @@ export function createCdnTier(options: CdnTierOptions = {}): CacheTier { if (paths.length > 0) await driver.purge(paths); }, + /** + * The tags themselves plus every `cdn-path` the graph hangs off them — one purge, one list. + * + * Those paths were computed by `invalidate.ts` and reported as busted while nothing ever + * purged them, so `x cache bust --json` named a path the edge still held for its whole + * `s-maxage`: a partial bust reading as a clean one, which is the one failure the report + * exists to prevent. They go out **as surrogate keys**, the single currency `PurgeDriver` + * has — the same convention `pathsForKey` already documents, so a host registering a + * `cdn-path` dependent must tag that response with its own path. + */ async invalidateTags(tags: readonly CacheTag[]): Promise { - const keys = serializeTags(tags); + const keys = [...new Set([...serializeTags(tags), ...dependentsOfKind(tags, 'cdn-path')])]; if (keys.length === 0) return { tier: 'cdn', keys: [] }; const accepted = await driver.purge(keys); return { tier: 'cdn', keys: accepted }; diff --git a/packages/cache/src/errors.ts b/packages/cache/src/errors.ts index 3260930e..eaa74c71 100644 --- a/packages/cache/src/errors.ts +++ b/packages/cache/src/errors.ts @@ -8,6 +8,7 @@ export const CACHE_OWNED_ERROR_CODES = [ 'X_CACHE_PURGE_FAILED', 'X_CACHE_TAG_UNKNOWN', 'X_CACHE_TOO_LARGE', + 'X_CACHE_TTL_INVALID', ] as const; /** Every code cache can throw. It borrows none: every remote driver here is implemented. */ @@ -21,6 +22,7 @@ export const CACHE_ERROR_TITLES: Readonly> = X_CACHE_PURGE_FAILED: 'the CDN refused a purge', X_CACHE_TAG_UNKNOWN: 'a tag no entity declared', X_CACHE_TOO_LARGE: "one entry exceeds the tier's byte budget", + X_CACHE_TTL_INVALID: 'a cache TTL that is not a positive number of milliseconds', }; // One unconditional call, so a second package claiming one of cache's codes throws @@ -72,6 +74,29 @@ export class CacheTooLargeError extends UltimateError { } } +/** + * A `ttlMs` that is not a positive, finite number of milliseconds. + * + * `0` used to mean two things: "never expires" in the LRU tier and `EX 1` — one second — in the + * Redis tier, so a stack holding both answered differently depending on which one hit. Neither is + * what a caller writing `0` intends, and the third reading ("do not cache") has its own spelling: + * do not call the cache. Refused rather than resolved, so the miswiring is a failure and not a + * behaviour that varies by deployment. + */ +export class CacheTtlInvalidError extends UltimateError { + constructor(input: { key: string; ttlMs: number; tier: string }) { + super({ + code: 'X_CACHE_TTL_INVALID', + cause: `entry "${input.key}" was written to the ${input.tier} tier with ttlMs=${String( + input.ttlMs, + )}; a TTL is a positive, finite number of milliseconds`, + fix: `cache.write('${input.key}', value, { ttlMs: 60_000 }) # or drop the option for the tier default; a value you do not want held is one you do not write`, + docs: docsFor('X_CACHE_TTL_INVALID'), + meta: { key: input.key, ttlMs: input.ttlMs, tier: input.tier }, + }); + } +} + /** * A remote purge did not happen. Never fatal on its own — `invalidateTags` collects it into * `report.errors` so a dead CDN cannot fail the write that triggered the bust — which is exactly diff --git a/packages/cache/src/index.ts b/packages/cache/src/index.ts index f5eb9cf0..b2442b9a 100644 --- a/packages/cache/src/index.ts +++ b/packages/cache/src/index.ts @@ -10,6 +10,7 @@ export { CachePurgeFailedError, CacheTagUnknownError, CacheTooLargeError, + CacheTtlInvalidError, } from './errors'; export type { CacheDependent, DependentKind } from './graph'; export { @@ -77,8 +78,9 @@ export type { CacheEntry, CacheSetOptions, CacheStack, + CacheStackOptions, CacheTier, TierInvalidation, TierName, } from './tiers'; -export { createCacheStack, isExpired, nowMs, sortTiers, TIER_ORDER } from './tiers'; +export { assertTtl, createCacheStack, isExpired, nowMs, sortTiers, TIER_ORDER } from './tiers'; diff --git a/packages/cache/src/invalidate.test.ts b/packages/cache/src/invalidate.test.ts index e05f99e2..34d1e79b 100644 --- a/packages/cache/src/invalidate.test.ts +++ b/packages/cache/src/invalidate.test.ts @@ -1,5 +1,6 @@ import { beforeEach, describe, expect, test } from 'bun:test'; import { systemClock, withSpan } from '@ultimat3/core'; +import { createCdnTier } from './cdn'; import { CacheDriverUnavailableError, CacheTagUnknownError } from './errors'; import { registerDependent, resetGraph } from './graph'; import type { InvalidationEvent } from './invalidate'; @@ -48,15 +49,15 @@ function fakeRedis(): RedisLike & { readonly sent: string[][] } { return Promise.resolve(1); } if (command === 'EVAL') { - // Mirrors INVALIDATE_SCRIPT: members of every tag set, then drop the sets. + // Mirrors INVALIDATE_SCRIPT exactly, and the mirroring is the point: the script reads + // the members and drops the TAG SETS ONLY. It must not delete a value key — a script may + // only touch what it was handed in KEYS, and a fake that deleted them anyway would hide + // a tier that stopped issuing its own DELs. const count = Number(args[1]); const buckets = args.slice(2, 2 + count); const removed: string[] = []; for (const bucket of buckets) { - for (const member of sets.get(bucket) ?? []) { - values.delete(member); - removed.push(member); - } + for (const member of sets.get(bucket) ?? []) removed.push(member); sets.delete(bucket); } return Promise.resolve(removed); @@ -125,7 +126,7 @@ beforeEach(() => { describe('invalidateTags fan-out', () => { test('reaches every registered tier and reports what each one dropped', async () => { - const lru = createLruTier({ maxBytes: 10_000, defaultTtlMs: 0 }); + const lru = createLruTier({ maxBytes: 10_000, defaultTtlMs: 3_600_000 }); const redis = createRedisTier({ client: fakeRedis() }); const cdn = cdnSpy(); // Registered out of order on purpose: the stack must normalise to TIER_ORDER. @@ -155,7 +156,7 @@ describe('invalidateTags fan-out', () => { }); test('a failing tier is reported, never thrown — the write that triggered it must not fail', async () => { - const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 0 }); + const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 3_600_000 }); registerTier(lru); registerTier(brokenRedisTier()); await lru.set('k', 1, { tags: [tag('post')] }); @@ -196,7 +197,7 @@ describe('invalidateTags fan-out', () => { describe('recentInvalidations log', () => { test('an invalidation is recorded with its wire tags, its duration and the keys every tier reported', async () => { - const lru = createLruTier({ maxBytes: 10_000, defaultTtlMs: 0 }); + const lru = createLruTier({ maxBytes: 10_000, defaultTtlMs: 3_600_000 }); registerTier(lru); await lru.set('feed', ['a'], { tags: [tag('post')] }); @@ -208,11 +209,22 @@ describe('recentInvalidations log', () => { expect(event?.busted).toContain('feed'); }); - test('busted includes ISR paths, CDN paths and live queries, not just tier keys, and has no duplicates', async () => { - const lru = createLruTier({ maxBytes: 10_000, defaultTtlMs: 0 }); - const cdn = cdnSpy(); + test('busted includes ISR paths, live queries and what the CDN tier actually purged, with no duplicates', async () => { + const lru = createLruTier({ maxBytes: 10_000, defaultTtlMs: 3_600_000 }); + const purged: string[] = []; registerTier(lru); - registerTier(cdn); + registerTier( + createCdnTier({ + purge: { + name: 'recording', + purge: (keys) => { + purged.push(...keys); + return Promise.resolve(keys); + }, + purgeAll: () => Promise.resolve(), + }, + }), + ); registerDependent([tag('post')], { kind: 'isr-route', id: '/blog' }); registerDependent([tag('post')], { kind: 'cdn-path', id: '/feed.xml' }); registerDependent([tag('post')], { kind: 'live-query', id: 'live:post-list' }); @@ -221,12 +233,30 @@ describe('recentInvalidations log', () => { await invalidateTags([tag('post')]); + // The path is not merely *reported* busted: the driver was actually asked to purge it. + expect(purged).toEqual(['post', '/feed.xml']); + const [event] = recentInvalidations(); + expect(event?.busted).toEqual(['post', '/feed.xml', '/blog', 'live:post-list']); + }); + + test('with no CDN tier registered, a cdn-path is a dependent and never a bust', async () => { + // `report.cdn` came from the graph and was folded into `busted` whether or not anything + // purged it, so `x cache bust --json` named `/blog/hello` cleared while the edge held it + // for its whole s-maxage. A partial bust that reads as a clean one is the failure the log + // exists to catch. + const lru = createLruTier({ maxBytes: 10_000, defaultTtlMs: 3_600_000 }); + registerTier(lru); + registerDependent([tag('post', '1')], { kind: 'cdn-path', id: '/blog/hello' }); + + const report = await invalidateTags([tag('post', '1')]); + + expect(report.cdn).toEqual(['/blog/hello']); const [event] = recentInvalidations(); - expect(event?.busted).toEqual(['post', '/blog', '/feed.xml', 'live:post-list']); + expect(event?.busted).not.toContain('/blog/hello'); }); test('newest first, and the log never grows past the cap (drive it past 100)', async () => { - const lru = createLruTier({ maxBytes: 10_000, defaultTtlMs: 0 }); + const lru = createLruTier({ maxBytes: 10_000, defaultTtlMs: 3_600_000 }); registerTier(lru); for (let i = 0; i < 105; i += 1) { @@ -240,7 +270,7 @@ describe('recentInvalidations log', () => { }); test('source is the calling span name when invalidateTags runs inside a withSpan call', async () => { - const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 0 }); + const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 3_600_000 }); registerTier(lru); await withSpan('job.reindex', () => invalidateTags([tag('post')])); @@ -250,7 +280,7 @@ describe('recentInvalidations log', () => { }); test('source falls back to the literal invalidateTags when there is no active span', async () => { - const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 0 }); + const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 3_600_000 }); registerTier(lru); await invalidateTags([tag('post')]); @@ -271,7 +301,7 @@ describe('recentInvalidations log', () => { }); test('recentInvalidations hands back a copy: mutating it does not change the next answer', async () => { - const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 0 }); + const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 3_600_000 }); registerTier(lru); await invalidateTags([tag('post')]); @@ -289,7 +319,7 @@ describe('recentInvalidations log', () => { }); test('resetTiers clears the log', async () => { - const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 0 }); + const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 3_600_000 }); registerTier(lru); await invalidateTags([tag('post')]); expect(recentInvalidations().length).toBe(1); @@ -300,7 +330,7 @@ describe('recentInvalidations log', () => { }); test('invalidateWireTags records exactly one event, not two', async () => { - const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 0 }); + const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 3_600_000 }); registerTier(lru); await invalidateWireTags(['post']); @@ -309,7 +339,7 @@ describe('recentInvalidations log', () => { }); test('at is an ISO-8601 timestamp from the frozen system clock', async () => { - const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 0 }); + const lru = createLruTier({ maxBytes: 1_000, defaultTtlMs: 3_600_000 }); registerTier(lru); const before = systemClock.now().toISOString(); diff --git a/packages/cache/src/invalidate.ts b/packages/cache/src/invalidate.ts index c519b41e..66bb6cdb 100644 --- a/packages/cache/src/invalidate.ts +++ b/packages/cache/src/invalidate.ts @@ -20,6 +20,11 @@ export interface InvalidationReport { readonly tiers: readonly TierInvalidation[]; /** ISR route paths queued for regeneration. */ readonly isr: readonly string[]; + /** + * CDN paths the graph hangs off these tags — what *depends* on them, not what cleared. The + * `cdn` tier is what purges them (as surrogate keys, with the tags), so what actually cleared + * is that tier's row in `tiers`. With no `cdn` tier registered this list purges nowhere. + */ readonly cdn: readonly string[]; readonly liveQueries: readonly string[]; readonly durationMs: number; @@ -33,8 +38,13 @@ export interface InvalidationEvent { /** Wire-form tags, exactly `report.tags`. */ readonly tags: readonly string[]; /** - * Everything the fan-out actually cleared: every tier key, plus the ISR paths, CDN paths - * and live queries. + * Everything the fan-out actually cleared: every tier key — the `cdn` tier's accepted purge + * keys included — plus the ISR paths and the live queries. + * + * Deliberately NOT `report.cdn`: that is the dependency graph's answer to "what depends on + * these tags", and folding it in here reported a path as busted when no `cdn` tier was + * registered to purge it. A partial bust that reads as a clean one is the failure this log + * exists to catch. */ readonly busted: readonly string[]; /** @@ -151,7 +161,6 @@ export function invalidateTags(tags: readonly CacheTag[]): Promise entry.keys), ...report.isr, - ...report.cdn, ...report.liveQueries, ]), source, diff --git a/packages/cache/src/lru.test.ts b/packages/cache/src/lru.test.ts index 7de0bef7..490e613c 100644 --- a/packages/cache/src/lru.test.ts +++ b/packages/cache/src/lru.test.ts @@ -19,7 +19,7 @@ const filler = (bytes: number): string => 'x'.repeat(bytes); describe('LruCache byte budget', () => { test('evicts least-recently-used entries once the byte budget is exceeded', () => { - const cache = new LruCache({ maxBytes: 400, defaultTtlMs: 0 }); + const cache = new LruCache({ maxBytes: 400, defaultTtlMs: 3_600_000 }); cache.set('a', filler(100)); cache.set('b', filler(100)); cache.set('c', filler(100)); @@ -37,7 +37,7 @@ describe('LruCache byte budget', () => { }); test('bytes accounting returns to zero when every entry is deleted', () => { - const cache = new LruCache({ maxBytes: 10_000, defaultTtlMs: 0 }); + const cache = new LruCache({ maxBytes: 10_000, defaultTtlMs: 3_600_000 }); cache.set('a', { hello: 'world' }); cache.set('b', [1, 2, 3]); expect(cache.stats().bytes).toBeGreaterThan(0); @@ -65,7 +65,7 @@ describe('LruCache byte budget', () => { }); test('clear() resets hit/miss/eviction counters along with entries and bytes', () => { - const cache = new LruCache({ maxBytes: 400, defaultTtlMs: 0 }); + const cache = new LruCache({ maxBytes: 400, defaultTtlMs: 3_600_000 }); cache.set('a', filler(100)); cache.set('b', filler(100)); cache.set('c', filler(100)); @@ -92,7 +92,7 @@ describe('LruCache byte budget', () => { describe('LruCache tag invalidation', () => { test('invalidating a tag drops only entries carrying that tag', () => { - const cache = new LruCache({ maxBytes: 10_000, defaultTtlMs: 0 }); + const cache = new LruCache({ maxBytes: 10_000, defaultTtlMs: 3_600_000 }); cache.set('post:list', ['a'], { tags: [tag('post')] }); cache.set('post:1', { id: '1' }, { tags: [tag('post', '1')] }); cache.set('post:2', { id: '2' }, { tags: [tag('post', '2')] }); @@ -108,7 +108,7 @@ describe('LruCache tag invalidation', () => { }); test('invalidating a collection tag sweeps every row of that entity', () => { - const cache = new LruCache({ maxBytes: 10_000, defaultTtlMs: 0 }); + const cache = new LruCache({ maxBytes: 10_000, defaultTtlMs: 3_600_000 }); cache.set('post:1', 1, { tags: [tag('post', '1')] }); cache.set('post:2', 2, { tags: [tag('post', '2')] }); cache.set('user:1', 3, { tags: [tag('user', '1')] }); @@ -120,7 +120,7 @@ describe('LruCache tag invalidation', () => { }); test('overwriting a key re-indexes its tags so the stale tag no longer matches', () => { - const cache = new LruCache({ maxBytes: 10_000, defaultTtlMs: 0 }); + const cache = new LruCache({ maxBytes: 10_000, defaultTtlMs: 3_600_000 }); cache.set('k', 1, { tags: [tag('post', '1')] }); cache.set('k', 2, { tags: [tag('user', '9')] }); @@ -137,3 +137,49 @@ describe('estimateBytes', () => { expect(estimateBytes(new Uint8Array(16))).toBe(16); }); }); + +describe('the one TTL rule', () => { + // `0` used to be "never expires" here and `EX 1` — one second — in the Redis tier, so a stack + // holding both answered differently depending on which one hit. Neither is what a caller + // writing `0` means, so no tier resolves it. + test('a ttlMs that is not positive and finite is refused, not reinterpreted', () => { + const cache = new LruCache({ maxBytes: 10_000 }); + for (const ttlMs of [0, -1, Number.NaN, Number.POSITIVE_INFINITY]) { + expect(codeOf(() => cache.set('k', 1, { ttlMs }))).toBe('X_CACHE_TTL_INVALID'); + } + expect(cache.get('k')).toBeUndefined(); + }); + + test('a tier default that is not positive is refused on the write that relies on it', () => { + const cache = new LruCache({ maxBytes: 10_000, defaultTtlMs: 0 }); + expect(codeOf(() => cache.set('k', 1))).toBe('X_CACHE_TTL_INVALID'); + }); + + test('a refused overwrite leaves the entry it would have replaced', () => { + // The reject used to land after the unlink, so `set(k, v, { ttlMs: 0 })` on a live key both + // threw AND dropped the good value — a validation error that mutates is a second bug. + const cache = new LruCache({ maxBytes: 10_000, clock: fakeClock(1_000) }); + cache.set('k', 'kept', { ttlMs: 5_000 }); + expect(codeOf(() => cache.set('k', 'rejected', { ttlMs: 0 }))).toBe('X_CACHE_TTL_INVALID'); + expect(cache.get('k')?.value).toBe('kept'); + expect(cache.stats().entries).toBe(1); + }); + + test('every entry therefore carries a finite expiry the stack can read', () => { + const clock = fakeClock(1_000); + const cache = new LruCache({ maxBytes: 10_000, clock }); + cache.set('k', 1, { ttlMs: 5_000 }); + expect(cache.get('k')?.expiresAt).toBe(6_000); + clock.advance(5_000); + expect(cache.get('k')).toBeUndefined(); + }); +}); + +function codeOf(run: () => unknown): string { + try { + run(); + } catch (error) { + return (error as { code?: string }).code ?? 'no-code'; + } + return 'no-throw'; +} diff --git a/packages/cache/src/lru.ts b/packages/cache/src/lru.ts index 0fd320ab..0e4a8219 100644 --- a/packages/cache/src/lru.ts +++ b/packages/cache/src/lru.ts @@ -9,7 +9,7 @@ import { CacheTooLargeError } from './errors'; import type { CacheTag } from './tags'; import { serializeTag } from './tags'; import type { CacheEntry, CacheSetOptions, CacheTier, TierInvalidation } from './tiers'; -import { nowMs } from './tiers'; +import { assertTtl, nowMs } from './tiers'; export interface LruOptions { /** Byte budget for the whole tier. Default 64 MiB. */ @@ -23,7 +23,7 @@ interface LruNode { key: string; value: unknown; bytes: number; - /** Epoch ms, or `Number.POSITIVE_INFINITY` for no expiry. */ + /** Epoch ms. Always finite: `assertTtl` refuses the `0` that used to mean "never expires". */ expiresAt: number; tags: readonly CacheTag[]; prev: LruNode | undefined; @@ -103,11 +103,7 @@ export class LruCache { } this.touch(node); this.hits += 1; - return { - value: node.value as T, - tags: node.tags, - ...(node.expiresAt === Number.POSITIVE_INFINITY ? {} : { expiresAt: node.expiresAt }), - }; + return { value: node.value as T, tags: node.tags, expiresAt: node.expiresAt }; } set(key: string, value: T, options: CacheSetOptions = {}): void { @@ -116,15 +112,17 @@ export class LruCache { throw new CacheTooLargeError({ key, bytes, maxBytes: this.maxBytes, tier: 'lru' }); } + // Validate BEFORE evicting the entry being replaced: a rejected write must leave the cache + // exactly as it found it, or `X_CACHE_TTL_INVALID` also silently drops a live, valid value. + const ttl = assertTtl(key, options.ttlMs ?? this.defaultTtlMs, 'lru'); const existing = this.map.get(key); if (existing !== undefined) this.unlink(existing); - const ttl = options.ttlMs ?? this.defaultTtlMs; const node: LruNode = { key, value, bytes, - expiresAt: ttl <= 0 ? Number.POSITIVE_INFINITY : nowMs(this.clock) + ttl, + expiresAt: nowMs(this.clock) + ttl, tags: options.tags ?? [], prev: undefined, next: undefined, diff --git a/packages/cache/src/redis.test.ts b/packages/cache/src/redis.test.ts index dcac69ae..819ba37f 100644 --- a/packages/cache/src/redis.test.ts +++ b/packages/cache/src/redis.test.ts @@ -3,14 +3,21 @@ // reach again. A fake Redis records every command so the wire traffic itself is the assertion. import { describe, expect, test } from 'bun:test'; +import { frozenClock } from '@ultimat3/core'; import { CacheDriverUnavailableError } from './errors'; +import { createLruTier } from './lru'; import type { RedisLike } from './redis'; import { createRedisTier } from './redis'; import { tag } from './tags'; +import { createCacheStack } from './tiers'; function fakeRedis(): RedisLike & { readonly sent: string[][] } { const sets = new Map>(); const values = new Map(); + // The lease `EX` bought, in ms. A fake that answered no `PTTL` could not catch a tier that + // stopped asking for one — and a hit read back without its remaining life is promoted on the + // caller's ttl, which is how a value one second from expiry gets a fresh five minutes. + const expiries = new Map(); const sent: string[][] = []; return { sent, @@ -25,8 +32,14 @@ function fakeRedis(): RedisLike & { readonly sent: string[][] } { sent.push([command, ...args]); if (command === 'SET') { values.set(String(args[0]), String(args[1])); + if (args[2] === 'EX') expiries.set(String(args[0]), Number(args[3]) * 1_000); return Promise.resolve('OK'); } + if (command === 'PTTL') { + const key = String(args[0]); + if (!values.has(key)) return Promise.resolve(-2); + return Promise.resolve(expiries.get(key) ?? -1); + } if (command === 'SADD') { const bucket = String(args[0]); const existing = sets.get(bucket) ?? new Set(); @@ -36,18 +49,19 @@ function fakeRedis(): RedisLike & { readonly sent: string[][] } { } if (command === 'DEL') { values.delete(String(args[0])); + expiries.delete(String(args[0])); return Promise.resolve(1); } if (command === 'EVAL') { - // Mirrors INVALIDATE_SCRIPT: members of every tag set, then drop the sets. + // Mirrors INVALIDATE_SCRIPT exactly, and the mirroring is the point: the script reads + // the members and drops the TAG SETS ONLY. It must not delete a value key — a script may + // only touch what it was handed in KEYS, and a fake that deleted them anyway would hide + // a tier that stopped issuing its own DELs. const count = Number(args[1]); const buckets = args.slice(2, 2 + count); const removed: string[] = []; for (const bucket of buckets) { - for (const member of sets.get(bucket) ?? []) { - values.delete(member); - removed.push(member); - } + for (const member of sets.get(bucket) ?? []) removed.push(member); sets.delete(bucket); } return Promise.resolve(removed); @@ -164,13 +178,35 @@ describe('createRedisTier', () => { expect(result.tier).toBe('redis'); // 'feed' was SADD'd into both the row bucket (x:t:post:1) and the collection bucket - // (x:t:post) at set time, and the script walks every bucket independently — so a single - // key that belongs to two buckets is reported twice. No unprefixed key is ever prefixed. - expect(result.keys).toEqual(['feed', 'feed']); - expect(result.keys.every((key) => key === 'feed')).toBe(true); + // (x:t:post) at set time and the script walks every bucket, so it comes back twice — deduped + // before it is deleted and reported, or the `/_x` panel overstates what cleared. + expect(result.keys).toEqual(['feed']); expect(await tier.get('feed')).toBeUndefined(); }); + test('the script declares every key it touches: value keys are deleted client-side', async () => { + // A Lua script may only reach keys handed to it in KEYS. DELing a SMEMBERS result from + // inside it is a cross-slot access — "attempted to access a non-local key in a cluster + // node" on Redis Cluster and in Dragonfly's strict mode, swallowed into report.errors so a + // failed bust read as partial and stale rows served until TTL. + const client = fakeRedis(); + const tier = createRedisTier({ client }); + await tier.set('feed', ['a'], { tags: [tag('post', '1')] }); + client.sent.length = 0; + + await tier.invalidateTags([tag('post', '1')]); + + const evals = client.sent.filter((entry) => entry[0] === 'EVAL'); + expect(evals).toHaveLength(1); + const [, script = '', numkeys = '0', ...keys] = evals[0] ?? []; + // The script's own body must not delete anything but the tag buckets it was handed. + expect(script).not.toContain("redis.call('DEL', key)"); + expect(keys).toHaveLength(Number(numkeys)); + // Every value key leaves as its own single-key DEL, which is always slot-local. + const deletes = client.sent.filter((entry) => entry[0] === 'DEL'); + expect(deletes).toEqual([['DEL', 'x:c:feed']]); + }); + test('invalidateTags dedupes overlapping buckets across tags', async () => { const client = fakeRedis(); const tier = createRedisTier({ client }); @@ -185,6 +221,42 @@ describe('createRedisTier', () => { expect(evalCall?.[2]).toBe('2'); }); + test('get reports the remaining lease as expiresAt, read from PTTL', async () => { + const client = fakeRedis(); + const tier = createRedisTier({ client, clock: frozenClock(10_000) }); + await tier.set('k', 'v', { ttlMs: 5_000 }); + + expect((await tier.get('k'))?.expiresAt).toBe(15_000); + // Asked alongside the GET, on the value key — never on the tag buckets. + expect(client.sent.filter((entry) => entry[0] === 'PTTL')).toEqual([['PTTL', 'x:c:k']]); + }); + + test('a stored key with no expiry reports no expiresAt', async () => { + // PTTL answers -1 for a key that exists without a lease (one written outside this tier); + // `-1` is a sentinel, not one millisecond ago, so the entry must report no expiry at all. + const client = fakeRedis(); + await client.set('x:c:leaseless', JSON.stringify({ v: 'v', t: [] })); + const tier = createRedisTier({ client, clock: frozenClock(10_000) }); + + const entry = await tier.get('leaseless'); + expect(entry?.value).toBe('v'); + expect(entry?.expiresAt).toBeUndefined(); + }); + + test('promotion out of redis carries the REMAINING lease into the LRU, not the caller ttl', async () => { + // The cross-tier expiry contract, end to end: without `expiresAt` on the redis hit the stack + // can only promote on `setOptions.ttlMs`, so a row one second from expiry gets a fresh five + // minutes in the LRU on every read and the closer tier outlives the entry it copied. + const client = fakeRedis(); + const clock = frozenClock(10_000); + const lru = createLruTier({ clock }); + const stack = createCacheStack([lru, createRedisTier({ client, clock })], { clock }); + await createRedisTier({ client, clock }).set('k', 'v', { ttlMs: 5_000 }); + + expect(await stack.read('k', () => Promise.resolve('loaded'), { ttlMs: 300_000 })).toBe('v'); + expect(lru.cache.get('k')?.expiresAt).toBe(15_000); + }); + test('constructing with no client and no Bun.redis throws CacheDriverUnavailableError lazily', async () => { // This sandbox has a real Bun.redis (a redis answers on the default port), so // `resolveClient` would otherwise never reach its throwing branch. Stub the global to diff --git a/packages/cache/src/redis.ts b/packages/cache/src/redis.ts index 67574a61..ee0ede57 100644 --- a/packages/cache/src/redis.ts +++ b/packages/cache/src/redis.ts @@ -3,11 +3,13 @@ // trip via a server-side script, not a KEYS scan. KEYS is O(n) and blocks the server; a // framework that ships it as the invalidation path is shipping an outage. -import { logger } from '@ultimat3/core'; +import type { Clock } from '@ultimat3/core'; +import { logger, systemClock } from '@ultimat3/core'; import { CacheDriverUnavailableError } from './errors'; import type { CacheTag } from './tags'; import { parseTag, serializeTag } from './tags'; import type { CacheEntry, CacheSetOptions, CacheTier, TierInvalidation } from './tiers'; +import { assertTtl, nowMs } from './tiers'; /** The slice of Bun's Redis client this tier uses. Narrow on purpose: easy to fake in tests. */ export interface RedisLike { @@ -22,6 +24,8 @@ export interface RedisTierOptions { readonly defaultTtlMs?: number; /** Injected in tests; production reads `Bun.redis`. */ readonly client?: RedisLike; + /** Turns `PTTL`'s remaining life into the absolute `expiresAt` a hit reports. */ + readonly clock?: Clock; } interface StoredEntry { @@ -30,15 +34,24 @@ interface StoredEntry { } /** - * Drop the value keys, then drop the tag sets themselves. `SMEMBERS` + `DEL` in one EVAL is - * atomic and single-trip; doing it client-side would race a concurrent write. + * Read the tag sets out and drop the tag sets themselves — and nothing else. + * + * A script may only touch keys it was handed in `KEYS`, and the members of a tag set are not + * among them: they are value keys hashing to slots this node may not even own. `DEL`ing them from + * inside the script therefore raised "attempted to access a non-local key in a cluster node" on + * Redis Cluster and in Dragonfly's strict mode — swallowed into `report.errors`, so a bust read + * as "partial", the write that triggered it still succeeded, and stale rows served until TTL. + * + * The value keys come back to the client instead, which drops them one `DEL` at a time: a single + * key is always slot-local, whatever the topology. Only the SMEMBERS + tag-set `DEL` stay atomic, + * and that is the pair that needed to be — a value key re-added by a concurrent write between the + * two halves is at worst a cache miss, never a stale read. */ const INVALIDATE_SCRIPT = ` local removed = {} for i, tagKey in ipairs(KEYS) do local members = redis.call('SMEMBERS', tagKey) for _, key in ipairs(members) do - redis.call('DEL', key) table.insert(removed, key) end redis.call('DEL', tagKey) @@ -46,6 +59,9 @@ end return removed `.trim(); +/** Concurrent `DEL`s per flush. Bun pipelines them, so this bounds memory, not round trips. */ +const DELETE_BATCH = 128; + function resolveClient(injected: RedisLike | undefined): RedisLike { if (injected !== undefined) return injected; const candidate = (Bun as unknown as { redis?: RedisLike }).redis; @@ -62,9 +78,20 @@ function resolveClient(injected: RedisLike | undefined): RedisLike { const toStrings = (value: unknown): string[] => Array.isArray(value) ? value.map((item) => String(item)) : []; +/** + * `PTTL`'s answer as milliseconds of remaining life, or `undefined` for the two sentinels it + * answers with instead of a duration: `-1` (key exists, no expiry) and `-2` (no such key). + * A driver may hand either back as a string, so the parse goes through `Number`. + */ +function remainingMs(reply: unknown): number | undefined { + const pttl = Number(reply); + return Number.isFinite(pttl) && pttl > 0 ? pttl : undefined; +} + export function createRedisTier(options: RedisTierOptions = {}): CacheTier { const prefix = options.prefix ?? 'x'; const defaultTtlMs = options.defaultTtlMs ?? 300_000; + const clock = options.clock ?? systemClock; let client: RedisLike | undefined; const conn = (): RedisLike => { @@ -83,12 +110,32 @@ export function createRedisTier(options: RedisTierOptions = {}): CacheTier { return { name: 'redis', + /** + * The value AND what is left of its lease. `set` always applies a finite `EX`, so an entry + * read back without its remaining life is an entry the stack can only promote on the + * CALLER's ttl — re-leasing a row one second from expiry for a fresh five minutes into the + * LRU on every read, which is a hot key that never goes stale enough to be refetched. + * + * `PTTL` rather than an `expiresAt` written into the payload: the server owns the clock, so + * this survives skew between the node that wrote and the node that reads, and no stored + * shape changes under a running deployment. Issued alongside the `GET` rather than after + * it — Bun pipelines the pair, so the expiry costs no extra round trip. + */ async get(key: string): Promise | undefined> { - const raw = await conn().get(valueKey(key)); + const stored = valueKey(key); + const [raw, pttl] = await Promise.all([ + conn().get(stored), + conn().send('PTTL', [stored]) as Promise, + ]); if (raw === null) return undefined; + const remaining = remainingMs(pttl); try { const parsed = JSON.parse(raw) as StoredEntry; - return { value: parsed.v as T, tags: parsed.t.map(parseTag) }; + return { + value: parsed.v as T, + tags: parsed.t.map(parseTag), + ...(remaining === undefined ? {} : { expiresAt: nowMs(clock) + remaining }), + }; } catch { // A poisoned value is a miss, never a 500. Redis TTL will reap it. logger.warn('cache.redis.corrupt-entry', { key }); @@ -98,7 +145,7 @@ export function createRedisTier(options: RedisTierOptions = {}): CacheTier { async set(key: string, value: T, setOptions?: CacheSetOptions): Promise { const tags = setOptions?.tags ?? []; - const ttlMs = setOptions?.ttlMs ?? defaultTtlMs; + const ttlMs = assertTtl(key, setOptions?.ttlMs ?? defaultTtlMs, 'redis'); const payload: StoredEntry = { v: value, t: tags.map(serializeTag) }; const stored = valueKey(key); const ttlSeconds = Math.max(1, Math.ceil(ttlMs / 1000)); @@ -122,7 +169,16 @@ export function createRedisTier(options: RedisTierOptions = {}): CacheTier { String(buckets.length), ...buckets, ]); - const stripped = toStrings(result).map((key) => key.slice(`${prefix}:c:`.length)); + // A member may sit in two tag sets; deleting it twice is harmless but reporting it twice + // makes the `/_x` panel overstate what cleared. + const members = [...new Set(toStrings(result))]; + for (let start = 0; start < members.length; start += DELETE_BATCH) { + // One key per DEL — always slot-local. Issued together so the batch costs one round trip. + await Promise.all( + members.slice(start, start + DELETE_BATCH).map((member) => conn().send('DEL', [member])), + ); + } + const stripped = members.map((key) => key.slice(`${prefix}:c:`.length)); return { tier: 'redis', keys: stripped }; }, }; diff --git a/packages/cache/src/tiers.test.ts b/packages/cache/src/tiers.test.ts index 1b8f4b56..1b350a7e 100644 --- a/packages/cache/src/tiers.test.ts +++ b/packages/cache/src/tiers.test.ts @@ -344,3 +344,90 @@ describe('createCacheStack tiers', () => { expect(stack.tiers.map((t) => t.name)).toEqual(['lru', 'redis', 'cdn']); }); }); + +describe('read-through promotion carries the entry, not the caller options', () => { + /** A tier seeded with an entry that already has an absolute expiry. */ + function expiringTier( + name: CacheTier['name'], + key: string, + entry: CacheEntry, + ): CacheTier { + return { + name, + get(k: string) { + return Promise.resolve(k === key ? (entry as CacheEntry) : undefined); + }, + set() { + return Promise.resolve(); + }, + del() { + return Promise.resolve(); + }, + invalidateTags() { + return Promise.resolve({ tier: name, keys: [] }); + }, + }; + } + + /** Records exactly what options a promotion wrote with. */ + function recordingTier(name: CacheTier['name'], writes: CacheSetOptions[]): CacheTier { + return { + name, + get() { + return Promise.resolve(undefined); + }, + set(_key: string, _value: unknown, options?: CacheSetOptions) { + writes.push(options ?? {}); + return Promise.resolve(); + }, + del() { + return Promise.resolve(); + }, + invalidateTags() { + return Promise.resolve({ tier: name, keys: [] }); + }, + }; + } + + test('a promoted hit gets its REMAINING life, never a fresh full lease', async () => { + // Re-leasing a value one second from expiry for a fresh five minutes on every read is a hot + // key that serves stale data forever: the closer tier's copy outlives the entry it copied. + const writes: CacheSetOptions[] = []; + const stack = createCacheStack( + [ + recordingTier('lru', writes), + expiringTier('redis', 'k', { value: 'v', tags: [], expiresAt: 11_000 }), + ], + { clock: fakeClock(10_000) }, + ); + + expect(await stack.read('k', () => Promise.resolve('loaded'), { ttlMs: 300_000 })).toBe('v'); + expect(writes).toEqual([{ ttlMs: 1_000, tags: [] }]); + }); + + test('an entry a tier has not reaped yet is a miss, so `load` runs', async () => { + const writes: CacheSetOptions[] = []; + const stack = createCacheStack( + [ + recordingTier('lru', writes), + expiringTier('redis', 'k', { value: 'stale', tags: [], expiresAt: 9_000 }), + ], + { clock: fakeClock(10_000) }, + ); + + expect(await stack.read('k', () => Promise.resolve('fresh'), { ttlMs: 300_000 })).toBe('fresh'); + // Written as a load, with the caller's ttl — not promoted with a negative one. + expect(writes).toEqual([{ ttlMs: 300_000 }]); + }); + + test('a hit with no recorded expiry still promotes on the caller ttl', async () => { + const writes: CacheSetOptions[] = []; + const stack = createCacheStack( + [recordingTier('lru', writes), expiringTier('redis', 'k', { value: 'v', tags: [] })], + { clock: fakeClock(10_000) }, + ); + + expect(await stack.read('k', () => Promise.resolve('loaded'), { ttlMs: 300_000 })).toBe('v'); + expect(writes).toEqual([{ ttlMs: 300_000, tags: [] }]); + }); +}); diff --git a/packages/cache/src/tiers.ts b/packages/cache/src/tiers.ts index 868e7ecb..08c7ca54 100644 --- a/packages/cache/src/tiers.ts +++ b/packages/cache/src/tiers.ts @@ -4,6 +4,8 @@ // sites. Order is data, not control flow. import type { Clock } from '@ultimat3/core'; +import { systemClock } from '@ultimat3/core'; +import { CacheTtlInvalidError } from './errors'; import type { CacheTag } from './tags'; import { bestEffort } from './tier-failures'; @@ -20,10 +22,26 @@ export interface CacheEntry { } export interface CacheSetOptions { + /** + * Lifetime in milliseconds. **Positive and finite, always** — omit it for the tier's default. + * There is no "never expires" and no "do not cache": both used to be spellings of `0` that the + * LRU and Redis tiers read differently, so every tier now refuses it (`X_CACHE_TTL_INVALID`). + */ readonly ttlMs?: number; readonly tags?: readonly CacheTag[]; } +/** + * The one TTL rule, applied by every tier before it writes. Lives here rather than in each tier + * because two tiers disagreeing about what `0` means is exactly the bug this replaced. + */ +export function assertTtl(key: string, ttlMs: number, tier: TierName): number { + if (!Number.isFinite(ttlMs) || ttlMs <= 0) { + throw new CacheTtlInvalidError({ key, ttlMs, tier }); + } + return ttlMs; +} + /** Per-tier result of an invalidation, surfaced verbatim in the `/_x` cache panel. */ export interface TierInvalidation { readonly tier: TierName; @@ -71,32 +89,51 @@ export function sortTiers(tiers: readonly CacheTier[]): readonly CacheTier[] { * answer, never a failed business read. `load()` is the one call left unguarded — it *is* the * business read, and swallowing it would return `undefined` as if it were the value. */ -export function createCacheStack(tiers: readonly CacheTier[]): CacheStack { +export interface CacheStackOptions { + /** Read through `nowMs()`; the same clock a tier takes. Defaults to `systemClock`. */ + readonly clock?: Clock; +} + +export function createCacheStack( + tiers: readonly CacheTier[], + options: CacheStackOptions = {}, +): CacheStack { const ordered = sortTiers(tiers); + const clock = options.clock ?? systemClock; return { tiers: ordered, - async read(key: string, load: () => Promise, options?: CacheSetOptions): Promise { + async read(key: string, load: () => Promise, setOptions?: CacheSetOptions): Promise { for (let i = 0; i < ordered.length; i += 1) { const tier = ordered[i]; if (tier === undefined) continue; const hit = await bestEffort(tier.name, 'get', key, () => tier.get(key)); if (hit === undefined) continue; - // Populate every tier we walked past, closest-first on the next read. + const now = nowMs(clock); + // A tier may answer with an entry it has not reaped yet; expiry is decided here, once, + // by the predicate this module already exported and nothing had ever called. + if (isExpired(hit, now)) continue; + // Populate every tier we walked past, closest-first on the next read — carrying the + // entry's REMAINING life, never the caller's original ttlMs. Re-leasing a value one + // second from expiry for a fresh five minutes on every read is a hot key that never + // goes stale enough to be refetched. + const promoted: CacheSetOptions = { + ...setOptions, + tags: hit.tags, + ...(hit.expiresAt === undefined ? {} : { ttlMs: hit.expiresAt - now }), + }; for (let up = 0; up < i; up += 1) { const closer = ordered[up]; if (closer === undefined) continue; - await bestEffort(closer.name, 'set', key, () => - closer.set(key, hit.value, { ...options, tags: hit.tags }), - ); + await bestEffort(closer.name, 'set', key, () => closer.set(key, hit.value, promoted)); } return hit.value; } const value = await load(); for (const tier of ordered) { - await bestEffort(tier.name, 'set', key, () => tier.set(key, value, options)); + await bestEffort(tier.name, 'set', key, () => tier.set(key, value, setOptions)); } return value; }, diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 5c086575..2438bf33 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -130,7 +130,12 @@ Gotchas: `afterAll` — a reset that is not handed back strips the titles of every package imported before that file, and their errors render the humanised fallback (`X_DB_DRIFT: db drift`) for the rest of the run. That is a load-order flake: green locally, red on whichever CI ordering hits it. -- Tests that call `configureCursorSigning()` must restore the previous secret. +- Tests that call `configureCursorSigning()` must restore the previous secret, or call + `resetCursorSigning()` — the only way back to "unconfigured", which restoring a literal cannot + express. The secret itself is read inside `sign()`, never at module scope: `openSecrets()` runs + during boot, so a module-scope read signed a whole process's cursors with the dev key while + `ULTIMATE_CURSOR_SECRET` was set and `x doctor` merely warned. Same call-time rule as + `@ultimat3/auth`'s `oauth-cookie.ts` / `oauth-exchange.ts`; new secrets follow it. - `PRIMITIVE_KINDS` is the executable copy of the eight-primitive rule — `PrimitiveKind` derives from it, so the list and the type cannot drift. A ninth entry fails `registrar.test.ts`, which is the point: a new capability arrives as a factory over an existing primitive (`llm()` returns diff --git a/packages/core/README.md b/packages/core/README.md index 0f65c95c..32389045 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -284,9 +284,10 @@ never a silently wrong page. | | | |---|---| | Signature | truncated HMAC-SHA256, compared in constant time | -| Secret | `ULTIMATE_CURSOR_SECRET`, or `configureCursorSigning()` at boot. Rotating it invalidates every open cursor | +| Secret | `configureCursorSigning()` at boot, else `ULTIMATE_CURSOR_SECRET`. **Read when a cursor is signed, never at import** — an app whose `openSecrets()` sets the variable during boot would otherwise sign every cursor with the dev key. Rotating it invalidates every open cursor | | Signed, not encrypted | the client already has these rows; what it must not do is *invent* a position | | `usesDevCursorSecret()` | true while the shipped dev key is in use | +| `resetCursorSigning()` | test seam: forget `configureCursorSigning` and fall back to the environment | ## One image pipeline, everywhere diff --git a/packages/core/src/actor.ts b/packages/core/src/actor.ts index 1a5d8a37..b7fd3e6d 100644 --- a/packages/core/src/actor.ts +++ b/packages/core/src/actor.ts @@ -11,15 +11,27 @@ export type ActorKind = 'user' | 'service' | 'agent' | 'anonymous'; export const ACTOR_KINDS = ['user', 'service', 'agent', 'anonymous'] as const; /** - * Augment to carry app-owned authz facts on the actor — the friend set, the block set, the org - * row — resolved ONCE per request, because a policy predicate is synchronous and may not query: + * **The channel for every app-specific fact about who is calling.** `Actor` itself carries only + * what every app has — `kind`, `id`, `orgId`, `roles`, `scopes` — and it never grows a field for + * one app's vocabulary. A `memberId`, a `tz`, a plan tier, the friend set, the block set, the org + * row: each is declared here, by module augmentation, and resolved ONCE per request, because a + * policy predicate is synchronous and may not query. * * ```ts * declare module '@ultimat3/core' { - * interface ActorFacts { readonly viewer: Viewer } + * interface ActorFacts { + * readonly memberId: string; + * readonly tz: string; + * readonly viewer: Viewer; + * } * } * ``` * + * Then `withFacts(actor, { memberId, tz })` at the request boundary and `actorFact(actor, + * 'memberId')` everywhere else. Do not thread a second identity object beside the actor and do + * not ask for a field on `Actor`/`ActorInit`: a fact declared here rides the SAME actor every + * surface already hands the policy layer, so a relational rule never needs a second authz path. + * * Same shape as `CtxServices` and `PermissionRegistry`, for the same reason: the app declares * once and every reader — predicate, action handler, component — is typed from that declaration * without a single surface package learning the app's vocabulary. diff --git a/packages/core/src/cursor.test.ts b/packages/core/src/cursor.test.ts index cc915363..016fe798 100644 --- a/packages/core/src/cursor.test.ts +++ b/packages/core/src/cursor.test.ts @@ -4,6 +4,7 @@ import { configureCursorSigning, decodeCursor, encodeCursor, + resetCursorSigning, usesDevCursorSecret, } from './cursor'; @@ -103,6 +104,41 @@ describe('the one cursor codec', () => { expect(usesDevCursorSecret()).toBe(true); }); + // The secret used to be read at MODULE scope, so an app that loads secrets through + // `openSecrets()` during boot — setting ULTIMATE_CURSOR_SECRET after this module was imported + // — signed every cursor of that process with the shipped dev key. `x doctor` warned and + // nothing failed. Same call-time rule `@ultimat3/auth`'s oauth secrets follow. + test('ULTIMATE_CURSOR_SECRET is read when a cursor is signed, not when the module loads', () => { + const previous = process.env['ULTIMATE_CURSOR_SECRET']; + // Undo any earlier `configureCursorSigning` in this file so the env is what answers. + resetCursorSigning(); + try { + delete process.env['ULTIMATE_CURSOR_SECRET']; + expect(usesDevCursorSecret()).toBe(true); + + // Set AFTER import, exactly as a boot-time secret load does. + process.env['ULTIMATE_CURSOR_SECRET'] = 'loaded-at-boot'; + expect(usesDevCursorSecret()).toBe(false); + const cursor = encodeCursor(position); + expect(decodeCursor(cursor, 'posts:acme').id).toBe('p_9'); + + // And a cursor signed under it does not verify once it rotates. + process.env['ULTIMATE_CURSOR_SECRET'] = 'rotated-at-boot'; + expect(codeOf(() => decodeCursor(cursor, 'posts:acme'))).toBe('X_CURSOR_INVALID'); + + // An explicit configure still wins over the environment. + configureCursorSigning('explicit'); + process.env['ULTIMATE_CURSOR_SECRET'] = 'ignored'; + expect(decodeCursor(encodeCursor(position), 'posts:acme').id).toBe('p_9'); + } finally { + // The explicit secret is module state, so it outlives this test unless it is dropped here: + // a later test in this process would otherwise sign with 'explicit', not its own secret. + resetCursorSigning(); + if (previous === undefined) delete process.env['ULTIMATE_CURSOR_SECRET']; + else process.env['ULTIMATE_CURSOR_SECRET'] = previous; + } + }); + test('the failure names the fix, in three lines', () => { const error = new CursorInvalidError('signature does not match'); expect(error.format()).toBe( diff --git a/packages/core/src/cursor.ts b/packages/core/src/cursor.ts index 153a7e54..402191d2 100644 --- a/packages/core/src/cursor.ts +++ b/packages/core/src/cursor.ts @@ -38,16 +38,37 @@ export class CursorInvalidError extends UltimateError { */ const DEV_SECRET = 'ultimate-dev-cursor-secret'; -let secret = Bun.env['ULTIMATE_CURSOR_SECRET'] ?? DEV_SECRET; +/** `configureCursorSigning`'s value, when an app has called it. `undefined` means "read the env". */ +let configured: string | undefined; + +/** + * Read at CALL time, never at module scope — the same rule `@ultimat3/auth`'s `oauth-cookie.ts` + * and `oauth-exchange.ts` follow, and for the same reason: an app that loads secrets through + * `openSecrets()` during boot sets `ULTIMATE_CURSOR_SECRET` *after* this module was imported, so a + * module-scope read signed every cursor of that process with the shipped dev key. `x doctor` + * warned and nothing failed. + */ +function currentSecret(): string { + return configured ?? Bun.env['ULTIMATE_CURSOR_SECRET'] ?? DEV_SECRET; +} /** Set once at boot from the app secret. Rotating it invalidates every open cursor. */ export function configureCursorSigning(next: string): void { - secret = next; + configured = next; +} + +/** + * Test seam: forget `configureCursorSigning`, so signing falls back to the environment. + * The counterpart to `resetIdCounter` — a suite that configured a secret has a way back to + * "unconfigured", which restoring a literal cannot express. + */ +export function resetCursorSigning(): void { + configured = undefined; } /** True while cursors are signed with the shipped dev key — `x doctor` reports it. */ export function usesDevCursorSecret(): boolean { - return secret === DEV_SECRET; + return currentSecret() === DEV_SECRET; } /** `base64url(payload).signature`. Opaque by contract: callers must never parse it. */ @@ -82,7 +103,7 @@ export function decodeCursor(cursor: string, scope: string): CursorPayload { /** Truncated HMAC-SHA256. 128 bits is far past forging a page position. */ function sign(body: string): string { - return new Bun.CryptoHasher('sha256', secret).update(body).digest('hex').slice(0, 32); + return new Bun.CryptoHasher('sha256', currentSecret()).update(body).digest('hex').slice(0, 32); } /** Constant time: the comparison must not leak how much of a forged signature was right. */ diff --git a/packages/core/src/ids.test.ts b/packages/core/src/ids.test.ts index cc0ad1ec..f4abfaa9 100644 --- a/packages/core/src/ids.test.ts +++ b/packages/core/src/ids.test.ts @@ -74,3 +74,19 @@ describe('nanoid and trace ids', () => { expect(spanId()).toMatch(/^[0-9a-f]{16}$/); }); }); + +describe('the monotonic counter seed', () => { + test('spans the full 10 bits COUNTER_SEED_MASK declares', () => { + // `randomBytes(2)[0] & 0x3ff` allocated two bytes and read one, so the seed could only reach + // 255 while the mask declared 1023 — the constant and the code disagreed and the second byte + // was dead weight on every uuid(). rand_a is the `7xxx` group: strip the version nibble. + const seeds = new Set(); + for (let index = 0; index < 4000; index += 1) { + resetIdCounter(); + const randA = Number.parseInt(uuid().split('-')[2]?.slice(1) ?? '0', 16); + seeds.add(randA); + } + expect(Math.max(...seeds)).toBeGreaterThan(0x0ff); + expect(Math.max(...seeds)).toBeLessThanOrEqual(0x3ff); + }); +}); diff --git a/packages/core/src/ids.ts b/packages/core/src/ids.ts index bce358dd..5f1a1ae2 100644 --- a/packages/core/src/ids.ts +++ b/packages/core/src/ids.ts @@ -28,6 +28,16 @@ function randomBytes(length: number): Uint8Array { return bytes; } +/** + * A full 10 bits, from both bytes. Reading `bytes[0]` alone masked an 8-bit value with a 10-bit + * mask, so the seed only ever reached 255 while `COUNTER_SEED_MASK` declared 1023 — the constant + * and the code disagreed, and the second byte was allocated on every `uuid()` for nothing. + */ +function seedCounter(): number { + const bytes = randomBytes(2); + return (((bytes[0] ?? 0) << 8) | (bytes[1] ?? 0)) & COUNTER_SEED_MASK; +} + export function randomHex(byteLength: number): string { const bytes = randomBytes(byteLength); let out = ''; @@ -51,10 +61,10 @@ export function uuid(clock: Clock = systemClock): string { counter += 1; if (counter > COUNTER_MAX) { epochMs += 1; - counter = randomBytes(2)[0]! & COUNTER_SEED_MASK; + counter = seedCounter(); } } else { - counter = randomBytes(2)[0]! & COUNTER_SEED_MASK; + counter = seedCounter(); } lastEpochMs = epochMs; @@ -62,7 +72,9 @@ export function uuid(clock: Clock = systemClock): string { const randA = counter.toString(16).padStart(3, '0'); const tail = randomHex(8); // Force the RFC variant bits (0b10) into the first nibble of `rand_b`. - const variantNibble = HEX[(Number.parseInt(tail[0]!, 16) & 0x3) | 0x8]!; + // charAt, not [], because the index is provably 0x8–0xb: a non-null assertion here would be + // unenforceable style debt in the one file that made `noNonNullAssertion` unraisable. + const variantNibble = HEX.charAt((Number.parseInt(tail.charAt(0), 16) & 0x3) | 0x8); return [ timeHex.slice(0, 8), diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index d5118ebd..001c65d2 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -69,6 +69,7 @@ export { configureCursorSigning, decodeCursor, encodeCursor, + resetCursorSigning, usesDevCursorSecret, } from './cursor'; export type { diff --git a/packages/db/CLAUDE.md b/packages/db/CLAUDE.md index ae82e940..84dae77f 100644 --- a/packages/db/CLAUDE.md +++ b/packages/db/CLAUDE.md @@ -399,6 +399,16 @@ and `X_DB_DRIFT` is also declared by entity — registering twice throws at impo `readOnly()` is the regex-gated client for any caller that cannot open its own transaction. The MCP `db.query` tool does not use it — it goes through `readOnlyQuery()`, which is stronger. +**`readOnlyQuery` takes ONE statement**, refused through `statementsOf` before the transaction +opens (`X_SQL_UNSAFE`, `multipleStatements`). This is not a second mutating-keyword scan — it is a +different question, and the one the layer's own guards depend on: the statement is *spliced* into +`DECLARE … CURSOR FOR`, and only the first command of that text is bounded by the `SET LOCAL +statement_timeout` set moments earlier, so `select 1; set statement_timeout = 0` undid the guard +while `guards` went on reporting `timeout:5000ms`. `BEGIN READ ONLY` still held, so this was a +defeated layer reported as an engaged one rather than a write — and a guard list that lies is worse +than a guard list that is short. `statementsOf` is the package's one splitter, so a `;` inside a +literal, a comment or a dollar-quoted body stays data. + `readonly-role.ts` and `readonly-query.ts` are layers 1–2 of that tool's defence-in-depth: a `NOLOGIN` Postgres role (`ensureReadOnlyRole`) and a per-statement `BEGIN READ ONLY` + statement timeout (`readOnlyQuery`). Only layer 1 degrades: `ensureReadOnlyRole` returns `null` on a missing diff --git a/packages/db/src/errors.ts b/packages/db/src/errors.ts index 4bbbeb36..743e97c1 100644 --- a/packages/db/src/errors.ts +++ b/packages/db/src/errors.ts @@ -155,6 +155,22 @@ export const identifierUnsafe = (name: string): DbError => meta: { name }, }); +/** + * More than one command in a text that gets **spliced** — into `DECLARE … CURSOR FOR`, or sent + * whole on a driver that degrades to the simple protocol. `X_SQL_UNSAFE` rather than a validation + * code for the same reason `branchNameInvalid` uses it: a second command riding an interpolated + * statement is an injection, not a typo. Only the first is bounded by the guards `readOnlyQuery` + * just installed, so `SET LOCAL statement_timeout` was undone by the second while `guards` still + * reported `timeout:5000ms` — a defeated layer reported as an engaged one. + */ +export const multipleStatements = (statement: string, count: number): DbError => + new DbError({ + code: 'X_SQL_UNSAFE', + cause: `a read-only query must be ONE statement; this text holds ${count}: ${statement}`, + fix: 'await readOnlyQuery(first); await readOnlyQuery(second) # one statement per call', + meta: { count }, + }); + export const branchExists = (branch: string): DbError => new DbError({ code: 'X_BRANCH_EXISTS', diff --git a/packages/db/src/index.ts b/packages/db/src/index.ts index b805b5f5..94acf732 100644 --- a/packages/db/src/index.ts +++ b/packages/db/src/index.ts @@ -65,6 +65,7 @@ export { migrationDestructive, migrationIrreversible, migrationSnapshotMissing, + multipleStatements, readonlyViolation, sqlUnsafe, } from './errors'; diff --git a/packages/db/src/readonly-query.test.ts b/packages/db/src/readonly-query.test.ts index 3c9a01a2..36036ef8 100644 --- a/packages/db/src/readonly-query.test.ts +++ b/packages/db/src/readonly-query.test.ts @@ -173,3 +173,39 @@ describe('readOnlyQuery', () => { expect(pins).toEqual({ reserves: 1, releases: 1 }); }); }); + +describe('one statement, or none at all', () => { + test('an embedded ";" is refused before the transaction opens', async () => { + // Only the FIRST command is bounded by the guards this function installs, so a second one + // undid `SET LOCAL statement_timeout` while `guards` still reported `timeout:5000ms` — the + // BEGIN READ ONLY backstop held, but the reported guard list was a lie. + const client = createRecordingClient(); + let code = 'no-throw'; + try { + await readOnlyQuery('select 1; set statement_timeout = 0', { client, maxRows: 10 }); + } catch (error) { + code = (error as { code?: string }).code ?? 'no-code'; + } + expect(code).toBe('X_SQL_UNSAFE'); + // Nothing was opened: no BEGIN, no DECLARE, no ROLLBACK to clean up. + expect(client.texts).toEqual([]); + }); + + test('the refusal does not depend on maxRows — the direct path splices too', async () => { + const client = createRecordingClient(); + await expect(readOnlyQuery('select 1; delete from posts', { client })).rejects.toThrow( + /X_SQL_UNSAFE/, + ); + expect(client.texts).toEqual([]); + }); + + test('a ";" inside a literal or a comment is data, not a second statement', async () => { + const client = createRecordingClient(); + await expect(readOnlyQuery("select ';'", { client })).resolves.toBeDefined(); + await expect(readOnlyQuery('select 1 -- ; nope', { client })).resolves.toBeDefined(); + // A trailing ";" is still one statement and still stripped before the DECLARE. + const trailing = createRecordingClient(); + await readOnlyQuery('select 1;', { client: trailing, maxRows: 5 }); + expect(trailing.texts).toContain('DECLARE ultimate_read_cursor NO SCROLL CURSOR FOR select 1'); + }); +}); diff --git a/packages/db/src/readonly-query.ts b/packages/db/src/readonly-query.ts index 762abf00..ac75bfd1 100644 --- a/packages/db/src/readonly-query.ts +++ b/packages/db/src/readonly-query.ts @@ -4,8 +4,10 @@ // meant to ask for. import { baseClient, type DbClient, type DbConnection, isReservable } from './client'; +import { multipleStatements } from './errors'; import { identifier, raw, sql } from './sql'; import { stripSqlNoise } from './sql-noise'; +import { statementsOf } from './statement-split'; /** Default per-statement ceiling for an agent-authored read. */ export const READONLY_TIMEOUT_MS = 5_000; @@ -78,6 +80,14 @@ export async function readOnlyQuery( statement: string, options: ReadOnlyQueryOptions = {}, ): Promise> { + // ONE statement, decided before anything is opened. Not a second mutating-keyword scan — a + // different question, and the one the guards below depend on: only the first command of a text + // is bounded by them, so `select 1; set statement_timeout = 0` undid the timeout this function + // had just installed while `guards` went on reporting `timeout:5000ms`. `statementsOf` is the + // package's one splitter, so a `;` inside a literal, a comment or a dollar-quoted body is data. + const statements = statementsOf(statement); + if (statements.length > 1) throw multipleStatements(statement, statements.length); + const client = options.client ?? baseClient(); // A pooled BEGIN that lands on a different physical connection than the query that follows is // not a transaction at all, so a reservable client must pin one connection for the sequence. @@ -146,7 +156,8 @@ async function readRows( ): Promise { if (fetch === undefined || !cursorable(statement)) return connection.query(raw(statement)); - // A trailing `;` would close `DECLARE` before its query and turn one statement into two. + // A trailing `;` would close `DECLARE` before its query and turn one statement into two. An + // EMBEDDED one is refused up in `readOnlyQuery`, before the transaction opens. const query = statement.trim().replace(/;\s*$/, ''); await connection.execute(raw(`DECLARE ${CURSOR_NAME} NO SCROLL CURSOR FOR ${query}`)); const rows = await connection.query(raw(`FETCH FORWARD ${fetch} FROM ${CURSOR_NAME}`)); diff --git a/packages/i18n/src/context.test.ts b/packages/i18n/src/context.test.ts index 9904d106..32411940 100644 --- a/packages/i18n/src/context.test.ts +++ b/packages/i18n/src/context.test.ts @@ -43,11 +43,48 @@ describe('resolveLocale', () => { expect(resolved).toEqual({ locale: 'ar', direction: 'rtl', source: 'user' }); }); + test('an explicit choice outranks the browser Accept-Language', () => { + // The header is what the browser was installed as; the switcher cookie, the stored user + // preference and `?locale=` are what a person chose. Ranking the header first meant a user + // who picked Spanish got English on every request afterwards. + expect(resolveLocale({ header: 'en-US,en;q=0.9', cookie: 'es' }, { supported })).toEqual({ + locale: 'es', + direction: 'ltr', + source: 'cookie', + }); + expect(resolveLocale({ header: 'en-US,en;q=0.9', user: 'es' }, { supported })).toEqual({ + locale: 'es', + direction: 'ltr', + source: 'user', + }); + expect(resolveLocale({ header: 'en-US,en;q=0.9', query: 'es' }, { supported })).toEqual({ + locale: 'es', + direction: 'ltr', + source: 'query', + }); + // `?locale=` is per request, so it outranks the cookie the switcher wrote. + expect(resolveLocale({ cookie: 'de', query: 'es' }, { supported })).toEqual({ + locale: 'es', + direction: 'ltr', + source: 'query', + }); + }); + test('reads the locale cookie out of a raw Cookie header', () => { expect(localeCookieOf('sid=abc; x_locale=pt-BR; theme=dark')).toBe('pt-BR'); expect(localeCookieOf('sid=abc')).toBeUndefined(); expect(localeCookieOf(undefined)).toBeUndefined(); }); + + test('a cookie that will not decode is a value, never a URIError out of the request', () => { + // `x_locale=%` threw straight out of a per-request path; the raw value simply fails to + // normalise and the next source wins. + expect(localeCookieOf('x_locale=%')).toBe('%'); + expect(localeCookieOf('sid=abc; x_locale=%E0%A4%A; theme=dark')).toBe('%E0%A4%A'); + expect( + resolveLocale({ cookie: localeCookieOf('x_locale=%'), user: 'es' }, { supported }), + ).toEqual({ locale: 'es', direction: 'ltr', source: 'user' }); + }); }); describe('ambient translator', () => { diff --git a/packages/i18n/src/context.ts b/packages/i18n/src/context.ts index 4f05d03f..26271ed8 100644 --- a/packages/i18n/src/context.ts +++ b/packages/i18n/src/context.ts @@ -49,7 +49,13 @@ export interface LocaleConfig { order: readonly LocaleSourceName[]; } -const DEFAULT_ORDER: readonly LocaleSourceName[] = ['header', 'cookie', 'user', 'query']; +/** + * Explicit before inferred, always. `Accept-Language` is what the browser was installed as; the + * query, the cookie and the user row are what a person *chose*. Ranking the header first meant a + * language switcher wrote a cookie that never won again — and `@ultimat3/http`'s negotiator, which + * takes the explicit value ahead of the header, disagreed with this one about the same request. + */ +const DEFAULT_ORDER: readonly LocaleSourceName[] = ['query', 'cookie', 'user', 'header']; let config: LocaleConfig = { supported: SUPPORTED_LOCALES, @@ -68,7 +74,7 @@ export function localeConfig(): LocaleConfig { } /** - * Header → cookie → user record → query → default, per `config.order`. + * Query → cookie → user record → header → default, per `config.order`. * An unsupported tag is skipped rather than thrown: a stale cookie must not 500 a page. */ export function resolveLocale( @@ -95,7 +101,15 @@ export function localeCookieOf(cookieHeader?: string | null): string | undefined const index = pair.indexOf('='); if (index === -1) continue; if (pair.slice(0, index).trim() !== LOCALE_COOKIE) continue; - return decodeURIComponent(pair.slice(index + 1).trim()); + const raw = pair.slice(index + 1).trim(); + try { + return decodeURIComponent(raw); + } catch { + // A cookie is client-authored: `x_locale=%` is a `URIError` out of a per-request path, and + // a 500 for a malformed locale is worse than the raw value, which `resolveLocale` then + // fails to normalise and skips. Same guard as `@ultimat3/auth`'s `decodeCookieValue`. + return raw; + } } return undefined; } diff --git a/packages/money/CLAUDE.md b/packages/money/CLAUDE.md index 092666e8..8bf45b4a 100644 --- a/packages/money/CLAUDE.md +++ b/packages/money/CLAUDE.md @@ -20,7 +20,8 @@ alias is re-declared, if `minor` widens back to a `bigint`, or if either field l | `currency.ts` | ISO-4217 table + minor-unit exponent. Every scale derives from here. | | `arithmetic.ts` | add/subtract/multiply/compare, refuses mixed currencies | | `allocate.ts` | largest-remainder splits that preserve the total | -| `rounding.ts` | explicit modes, no implicit default | +| `factor.ts` | the exact fraction a scaling factor's decimal spelling names. `factorFraction` is internal — never exported; the `Fraction` **type** is public, because `ExchangeRate.ratio` is one | +| `rounding.ts` | explicit modes, no implicit default, over a float (`roundToInteger`) or a ratio (`roundRatio`) | | `format.ts` | `Intl.NumberFormat` only, digits from the exponent | | `convert.ts` | explicit rate + `RateProvider`, records provenance | @@ -30,6 +31,18 @@ alias is re-declared, if `minor` widens back to a `bigint`, or if either field l - Never `/ 100`. Use `scaleOf(currency)` / `exponentOf(currency)`. - Never combine currencies without `convert()` first. - Never round without naming a `RoundingMode` in the call or accepting the stated default. +- **Never scale in floats and round after.** `multiply`, `divide` and `convert` take the factor's + decimal spelling as an exact fraction (`factorFraction`) and hand it to `roundRatio`, so the mode + judges 100.5 and not the 100.49999999999999 `100 * 1.005` produces. A new scaling entry point + goes through the same pair — `roundToInteger(a * b, mode)` is the bug, written again. +- **A derived rate carries its fraction, never its reciprocal.** `fixedRateProvider` answers the + inverse direction by swapping `ExchangeRate.ratio`'s numerator and denominator: a table naming + `USD/EUR: 0.92` names 23/25, so EUR→USD is exactly 25/23, where `1 / 0.92` is a double whose own + decimal spelling rounds a large amount one minor unit low. `rate` stays the readable number the + audit trail records; `convert` scales by `ratio` whenever the provider supplied one. +- **One place decides a sign.** `formatMoney` is `formatMoneyParts` joined, and `accounting` + reaches `Intl` as `currencySign` — so the locale places the minus and picks the parenthesised + form, and a UI styling the parts cannot render a different format from the label beside it. - Adding a currency: one row in `currency.ts` with its correct exponent, plus a format test. ## Commands diff --git a/packages/money/README.md b/packages/money/README.md index 8479db5e..1e5504dc 100644 --- a/packages/money/README.md +++ b/packages/money/README.md @@ -57,6 +57,12 @@ returns the source amount, the rate, and its timestamp alongside the result — audit has to be able to reproduce the number. Implement `RateProvider` for a live feed; `fixedRateProvider()` covers tests, seeds and manually agreed invoice rates. +A rate may also carry `ratio` — the exact `Fraction` its `rate` approximates — and `convert` +scales by that when it is there. It is how a derived direction stays exact: a table naming +`USD/EUR: 0.92` names 23/25, so `fixedRateProvider` answers EUR→USD with 25/23 rather than the +double `1 / 0.92`, whose own decimal spelling rounds a large amount one minor unit low. `rate` +stays the readable number the audit trail records. + ## Errors | Code | When | diff --git a/packages/money/src/arithmetic.test.ts b/packages/money/src/arithmetic.test.ts index 7d207f13..48d35d54 100644 --- a/packages/money/src/arithmetic.test.ts +++ b/packages/money/src/arithmetic.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test'; -import { add, compare, isZero, max, multiply, negate, subtract, sum } from './arithmetic'; +import { add, compare, divide, isZero, max, multiply, negate, subtract, sum } from './arithmetic'; import { money } from './money'; describe('cross-currency safety', () => { @@ -64,3 +64,39 @@ function causeOf(run: () => unknown): string { } return 'no-throw'; } + +describe('scaling is exact, not an IEEE-754 product', () => { + // `100 * 1.005` is 100.49999999999999, so multiplying first hid the exact 100.5 from `half-up` + // and a 0.5% fee on €1.00 was billed as nothing. + test('multiply rounds the value written, not the value the double holds', () => { + expect(multiply(money(100, 'EUR'), 1.005)).toEqual({ minor: 101, currency: 'EUR' }); + expect(multiply(money(100, 'EUR'), 1.005, 'down')).toEqual({ minor: 100, currency: 'EUR' }); + expect(multiply(money(100, 'EUR'), 1.005, 'half-even')).toEqual({ + minor: 100, + currency: 'EUR', + }); + }); + + test('multiply stays symmetric around zero at the exact half', () => { + expect(multiply(money(-100, 'EUR'), 1.005)).toEqual({ minor: -101, currency: 'EUR' }); + expect(multiply(money(-100, 'EUR'), 1.005, 'down')).toEqual({ minor: -100, currency: 'EUR' }); + }); + + test('a factor in exponent notation carries its own decimal expansion', () => { + expect(multiply(money(1_000_000, 'EUR'), 1e-4)).toEqual({ minor: 100, currency: 'EUR' }); + expect(multiply(money(2, 'EUR'), 2.5e2)).toEqual({ minor: 500, currency: 'EUR' }); + }); + + test('divide rounds the exact quotient', () => { + expect(divide(money(1000, 'EUR'), 3)).toEqual({ minor: 333, currency: 'EUR' }); + expect(divide(money(1, 'EUR'), 2)).toEqual({ minor: 1, currency: 'EUR' }); + expect(divide(money(1, 'EUR'), 2, 'half-even')).toEqual({ minor: 0, currency: 'EUR' }); + expect(divide(money(-1, 'EUR'), 2)).toEqual({ minor: -1, currency: 'EUR' }); + // Scaling by 0.1 and dividing by 10 are the same question and must not answer differently. + expect(divide(money(105, 'EUR'), 10)).toEqual(multiply(money(105, 'EUR'), 0.1)); + }); + + test('an amount scaled past the safe-integer range is refused, never approximated', () => { + expect(codeOf(() => multiply(money(1_000_000_000, 'EUR'), 1e9))).toBe('X_MONEY_NOT_INTEGER'); + }); +}); diff --git a/packages/money/src/arithmetic.ts b/packages/money/src/arithmetic.ts index d0013ab8..3abd647b 100644 --- a/packages/money/src/arithmetic.ts +++ b/packages/money/src/arithmetic.ts @@ -4,8 +4,9 @@ */ import { allocationInvalid, currencyMismatch, currencyRequired } from './errors'; +import { factorFraction } from './factor'; import { type Money, money } from './money'; -import { DEFAULT_ROUNDING, type RoundingMode, roundToInteger } from './rounding'; +import { DEFAULT_ROUNDING, type RoundingMode, roundRatio } from './rounding'; /** Throws `X_CURRENCY_MISMATCH` unless both operands carry the same currency. */ export function assertSameCurrency(left: Money, right: Money): string { @@ -33,18 +34,27 @@ export function sum(amounts: readonly Money[], currency?: string): Money { /** * Scale by a plain number (a tax rate, a quantity, a percentage). The result is rounded * to whole minor units with an explicit mode — the default is stated, not implied. + * + * The scale is taken as the exact fraction `factor`'s decimal spelling names, never as a float + * product: `100 * 1.005` is 100.49999999999999, so multiplying first hid the exact 100.5 from + * `half-up` and billed a 0.5% fee on €1.00 as nothing. */ export function multiply( amount: Money, factor: number, mode: RoundingMode = DEFAULT_ROUNDING, ): Money { - return money(roundToInteger(amount.minor * factor, mode), amount.currency); + const scale = factorFraction(factor); + return money( + roundRatio(BigInt(amount.minor) * scale.numerator, scale.denominator, mode), + amount.currency, + ); } /** * Divide into a single share. Use `allocate` when the whole must be preserved — - * `divide` alone loses the remainder by design. + * `divide` alone loses the remainder by design. Exact for the same reason `multiply` is: + * dividing by `d` is scaling by the reciprocal of the fraction `d` names. */ export function divide( amount: Money, @@ -54,7 +64,11 @@ export function divide( if (divisor === 0) { throw allocationInvalid('cannot divide money by zero — use allocate() to split a total'); } - return money(roundToInteger(amount.minor / divisor, mode), amount.currency); + const scale = factorFraction(divisor); + return money( + roundRatio(BigInt(amount.minor) * scale.denominator, scale.numerator, mode), + amount.currency, + ); } export function negate(amount: Money): Money { diff --git a/packages/money/src/convert.test.ts b/packages/money/src/convert.test.ts index cd8ebaa0..94ec44a2 100644 --- a/packages/money/src/convert.test.ts +++ b/packages/money/src/convert.test.ts @@ -43,6 +43,39 @@ describe('convertWith', () => { expect(back.amount.minor).toBe(1000); }); + test('the inverse is the swapped fraction, not the reciprocal double', async () => { + // `1 / 0.92` is 1.0869565217391304, and expanding THAT decimal loses a minor unit: the table + // named 23/25, so this direction is exactly 25/23 and nothing else was ever observed. + const rate = await provider.rateFor('EUR', 'USD'); + expect(rate?.ratio).toEqual({ numerator: 100n, denominator: 92n }); + + const big = await convertWith(provider, money(7_999_999_999_999_980, 'EUR'), 'USD'); + expect(big.amount.minor).toBe(8_695_652_173_913_022); + // The audit trail still records the readable number a human recognises as the rate. + expect(big.rate).toBe(1 / 0.92); + }); + + test('a direct rate carries its own decimal spelling as the fraction', async () => { + const rate = await provider.rateFor('USD', 'EUR'); + expect(rate?.ratio).toEqual({ numerator: 92n, denominator: 100n }); + }); + + test('a ratio that is not positive is a missing rate, never a negative conversion', () => { + const poisoned: ExchangeRate = { + ...usdToEur, + ratio: { numerator: -92n, denominator: 100n }, + }; + expect(codeOf(() => convert(money(1000, 'USD'), 'EUR', poisoned))).toBe('X_RATE_MISSING'); + expect( + codeOf(() => + convert(money(1000, 'USD'), 'EUR', { + ...usdToEur, + ratio: { numerator: 92n, denominator: 0n }, + }), + ), + ).toBe('X_RATE_MISSING'); + }); + test('a missing pair throws instead of assuming parity', async () => { let code = 'no-throw'; try { @@ -68,3 +101,62 @@ function codeOf(run: () => unknown): string { } return 'no-throw'; } + +describe('conversion is exact', () => { + // `10000 * 1.005` is 10049.999999999998; the exact 10050 must reach `half-up` whole. + test('the rate is applied as the fraction it spells, not as a float product', () => { + const rate: ExchangeRate = { from: 'EUR', to: 'USD', rate: 1.005, at }; + expect(convert(money(100, 'EUR'), 'USD', rate).amount).toEqual({ + minor: 101, + currency: 'USD', + }); + expect(convert(money(100, 'EUR'), 'USD', rate, { rounding: 'down' }).amount).toEqual({ + minor: 100, + currency: 'USD', + }); + }); + + test('exactness survives the minor-unit exponent shift', () => { + // USD (2 digits) -> JPY (0): 1005 cents at 1.005 is exactly 10.1002... major, so ¥10. + const rate: ExchangeRate = { from: 'USD', to: 'JPY', rate: 1.005, at }; + expect(convert(money(1005, 'USD'), 'JPY', rate).amount).toEqual({ + minor: 10, + currency: 'JPY', + }); + }); +}); + +describe('fixedRateProvider and a historical ask', () => { + const provider = fixedRateProvider({ 'USD/EUR': 0.92 }, at); + + test('an `at` the table cannot honour is undefined, never today stamped as then', async () => { + // The provider holds one observation. Answering with it under another date repriced a + // historical invoice at the wrong rate AND recorded a date nobody asked for. + expect(await provider.rateFor('USD', 'EUR', new Date('2020-01-01T00:00:00.000Z'))).toBe( + undefined, + ); + expect( + await codeOfAsync(() => + convertWith(provider, money(10000, 'USD'), 'EUR', { + at: new Date('2020-01-01T00:00:00.000Z'), + }), + ), + ).toBe('X_RATE_MISSING'); + }); + + test('the instant the table records is honoured, and so is no instant at all', async () => { + expect((await provider.rateFor('USD', 'EUR', new Date(at.getTime())))?.rate).toBe(0.92); + const result = await convertWith(provider, money(10000, 'USD'), 'EUR'); + expect(result.amount).toEqual({ minor: 9200, currency: 'EUR' }); + expect(result.at).toBe(at.toISOString()); + }); +}); + +async function codeOfAsync(run: () => Promise): Promise { + try { + await run(); + } catch (error) { + return (error as { code?: string }).code ?? 'no-code'; + } + return 'no-throw'; +} diff --git a/packages/money/src/convert.ts b/packages/money/src/convert.ts index 7ca4040c..423b22d3 100644 --- a/packages/money/src/convert.ts +++ b/packages/money/src/convert.ts @@ -6,14 +6,23 @@ import { assertCurrency, exponentOf } from './currency'; import { rateMissing } from './errors'; +import { type Fraction, factorFraction } from './factor'; import { type Money, money } from './money'; -import { DEFAULT_ROUNDING, type RoundingMode, roundToInteger } from './rounding'; +import { DEFAULT_ROUNDING, type RoundingMode, roundRatio } from './rounding'; export interface ExchangeRate { from: string; to: string; /** Major units of `to` per one major unit of `from`. */ rate: number; + /** + * The exact value `rate` approximates, for a provider that knows one. `rate` is the number the + * audit trail records and a human reads; a reciprocal cannot be both. `1 / 0.92` is the double + * 1.0869565217391304, whose decimal spelling is NOT 25/23 — so scaling by it loses a minor unit + * on a large amount, and the table that named `USD/EUR = 0.92` never wrote that number at all. + * Omit it and `convert` expands `rate`'s own decimal spelling, which is exact for a direct rate. + */ + ratio?: Fraction; /** When the rate was observed — part of the audit trail, not decoration. */ at: Date; /** Where it came from: `ecb`, `openexchange`, `manual:invoice-4711`. */ @@ -52,12 +61,20 @@ export function convert( if (!Number.isFinite(rate.rate) || rate.rate <= 0) { throw rateMissing(amount.currency, target); } + if (rate.ratio !== undefined && (rate.ratio.numerator <= 0n || rate.ratio.denominator <= 0n)) { + throw rateMissing(amount.currency, target); + } - const scale = 10 ** (exponentOf(target) - exponentOf(amount.currency)); - const converted = roundToInteger( - amount.minor * rate.rate * scale, - options.rounding ?? DEFAULT_ROUNDING, - ); + // Exact, not a float product: `minor * rate * scale` shows the rounding mode a value IEEE-754 + // has already moved, and a converted invoice line is off by a minor unit with nothing to trace. + // The provider's own fraction wins when it has one — see `ExchangeRate.ratio`. + const fraction = rate.ratio ?? factorFraction(rate.rate); + const exponent = exponentOf(target) - exponentOf(amount.currency); + let numerator = BigInt(amount.minor) * fraction.numerator; + let denominator = fraction.denominator; + if (exponent > 0) numerator *= 10n ** BigInt(exponent); + else if (exponent < 0) denominator *= 10n ** BigInt(-exponent); + const converted = roundRatio(numerator, denominator, options.rounding ?? DEFAULT_ROUNDING); return { amount: money(converted, target), @@ -95,9 +112,23 @@ export async function convertWith( }; } +/** + * The exact fraction a table entry names, or `undefined` for a number that is not a usable rate. + * `convert` refuses those on `rate.rate` with `X_RATE_MISSING`; expanding them here first would + * answer `X_NOT_ROUNDABLE` for the same mistake. + */ +function exactRate(value: number): Fraction | undefined { + return Number.isFinite(value) && value > 0 ? factorFraction(value) : undefined; +} + /** * Fixed-table provider for tests, seeds and manual invoice rates. * Keys are `FROM/TO`; the inverse is derived so a table needs one direction only. + * + * A table holds ONE observation, so a `wanted` instant other than its own is a rate this + * provider does not have — `undefined`, per `RateProvider`. Answering with today's number + * stamped `at: today` repriced a historical invoice against a rate nobody asked for and wrote a + * date into the audit trail that contradicted the request. */ export function fixedRateProvider( rates: Readonly>, @@ -106,12 +137,38 @@ export function fixedRateProvider( ): RateProvider { return { name, - async rateFor(from: string, to: string): Promise { + async rateFor(from: string, to: string, wanted?: Date): Promise { + if (wanted !== undefined && wanted.getTime() !== at.getTime()) return undefined; const direct = rates[`${from}/${to}`]; - if (direct !== undefined) return { from, to, rate: direct, at, source: name }; + if (direct !== undefined) { + const ratio = exactRate(direct); + return { + from, + to, + rate: direct, + at, + source: name, + ...(ratio === undefined ? {} : { ratio }), + }; + } const inverse = rates[`${to}/${from}`]; if (inverse !== undefined && inverse !== 0) { - return { from, to, rate: 1 / inverse, at, source: name }; + // Swapped, never divided. A table holding `USD/EUR: 0.92` names 23/25, so the EUR/USD + // direction is exactly 25/23 — where `1 / 0.92` is a double whose own decimal spelling + // rounds a large amount one minor unit low. `rate` keeps the readable approximation. + const named = exactRate(inverse); + const ratio = + named === undefined + ? undefined + : { numerator: named.denominator, denominator: named.numerator }; + return { + from, + to, + rate: 1 / inverse, + at, + source: name, + ...(ratio === undefined ? {} : { ratio }), + }; } return undefined; }, diff --git a/packages/money/src/factor.test.ts b/packages/money/src/factor.test.ts new file mode 100644 index 00000000..f2735e43 --- /dev/null +++ b/packages/money/src/factor.test.ts @@ -0,0 +1,43 @@ +// Single responsibility: pins decimal-to-fraction conversion, the step every scale in this +// package takes before it rounds. WHY it needs its own suite: the bug it prevents is invisible +// downstream — `factorFraction(1.005)` answering 1004999…/10^18 still rounds to *something*, and +// the missing minor unit only ever shows up on an invoice line nobody can reproduce. + +import { describe, expect, test } from 'bun:test'; +import { factorFraction } from './factor'; + +describe('factorFraction', () => { + test('expands the decimal that was written, not the double that holds it', () => { + // 1.005 is stored as 1.00499999999999989…; the fraction is what the caller typed. + expect(factorFraction(1.005)).toEqual({ numerator: 1005n, denominator: 1000n }); + expect(factorFraction(0.1)).toEqual({ numerator: 1n, denominator: 10n }); + expect(factorFraction(3)).toEqual({ numerator: 3n, denominator: 1n }); + expect(factorFraction(0)).toEqual({ numerator: 0n, denominator: 1n }); + }); + + test('carries the sign on the numerator so the denominator stays positive', () => { + expect(factorFraction(-2.5)).toEqual({ numerator: -25n, denominator: 10n }); + }); + + test('reads exponent notation, which is how a double under 1e-6 spells itself', () => { + expect(String(1e-7)).toBe('1e-7'); + expect(factorFraction(1e-7)).toEqual({ numerator: 1n, denominator: 10_000_000n }); + expect(factorFraction(2.5e3)).toEqual({ numerator: 2500n, denominator: 1n }); + expect(factorFraction(1.5e-3)).toEqual({ numerator: 15n, denominator: 10_000n }); + }); + + test('a non-finite factor is refused rather than expanded into a guess', () => { + for (const bad of [Number.NaN, Number.POSITIVE_INFINITY, Number.NEGATIVE_INFINITY]) { + expect(codeOf(() => factorFraction(bad))).toBe('X_MONEY_NOT_INTEGER'); + } + }); +}); + +function codeOf(run: () => unknown): string { + try { + run(); + } catch (error) { + return (error as { code?: string }).code ?? 'no-code'; + } + return 'no-throw'; +} diff --git a/packages/money/src/factor.ts b/packages/money/src/factor.ts new file mode 100644 index 00000000..decb38b3 --- /dev/null +++ b/packages/money/src/factor.ts @@ -0,0 +1,33 @@ +/** + * The exact fraction a scaling factor's decimal spelling names. + * A rate, a quantity or a percentage arrives as an IEEE-754 double, and `1.005` is held as + * 1.00499999999999989…; scaling first and rounding after therefore shows the rounding mode a + * value nobody wrote. The shortest round-trip decimal IS what was written, so its expansion is + * the exact value every scale in this package is taken against. + */ + +import { notRoundable } from './errors'; + +/** `numerator / denominator`, exactly. `denominator` is always a positive power of ten. */ +export interface Fraction { + readonly numerator: bigint; + readonly denominator: bigint; +} + +/** The grammar `Number.prototype.toString` emits for every finite double, exponent included. */ +const SPELLING = /^([+-]?)(\d+)(?:\.(\d+))?(?:[eE]([+-]?\d+))?$/; + +export function factorFraction(factor: number): Fraction { + if (!Number.isFinite(factor)) throw notRoundable(factor); + const match = SPELLING.exec(String(factor)); + if (match === null) throw notRoundable(factor); + const [, sign = '', whole = '0', fraction = '', exponent = '0'] = match; + + let numerator = BigInt(`${whole}${fraction}`); + let denominator = 10n ** BigInt(fraction.length); + const shift = BigInt(exponent); + if (shift > 0n) numerator *= 10n ** shift; + else if (shift < 0n) denominator *= 10n ** -shift; + + return { numerator: sign === '-' ? -numerator : numerator, denominator }; +} diff --git a/packages/money/src/format.test.ts b/packages/money/src/format.test.ts index 1e156240..0d017836 100644 --- a/packages/money/src/format.test.ts +++ b/packages/money/src/format.test.ts @@ -52,3 +52,36 @@ describe('formatMoneyParts', () => { expect(yen.find((part) => part.type === 'fraction')).toBeUndefined(); }); }); + +describe('one place decides the sign', () => { + test('the parts and the string agree on an accounting negative', () => { + const options = { accounting: true } as const; + const joined = formatMoneyParts(money(-1299, 'EUR'), 'en-US', options) + .map((part) => part.value) + .join(''); + expect(normalize(joined)).toBe(normalize(formatMoney(money(-1299, 'EUR'), 'en-US', options))); + expect(normalize(joined)).toBe('(€12.99)'); + }); + + test('the parts and the string agree on a plain negative', () => { + const joined = formatMoneyParts(money(-1299, 'EUR'), 'en-US') + .map((part) => part.value) + .join(''); + expect(normalize(joined)).toBe(normalize(formatMoney(money(-1299, 'EUR'), 'en-US'))); + expect(normalize(joined)).toBe('-€12.99'); + }); + + test('sign placement belongs to the locale, not to a hand-rolled prefix', () => { + // nl-NL puts the minus after the symbol; prefixing it here rendered a format Intl never emits. + expect(normalize(formatMoney(money(-129900, 'EUR'), 'nl-NL'))).toBe( + normalize( + new Intl.NumberFormat('nl-NL', { + style: 'currency', + currency: 'EUR', + minimumFractionDigits: 2, + maximumFractionDigits: 2, + }).format(-1299), + ), + ); + }); +}); diff --git a/packages/money/src/format.ts b/packages/money/src/format.ts index 7c0e2a5a..6a43645a 100644 --- a/packages/money/src/format.ts +++ b/packages/money/src/format.ts @@ -9,7 +9,10 @@ import { type Money, toDecimalNumber } from './money'; export interface FormatMoneyOptions { /** How the currency appears: `€1,299.00` / `EUR 1,299.00` / `1,299.00 euros`. */ display?: 'symbol' | 'narrowSymbol' | 'code' | 'name'; - /** Accounting negatives: `(€12.99)` instead of `-€12.99`. */ + /** + * Accounting negatives — `(€12.99)` in `en-US`. Passed to `Intl` as `currencySign`, so the + * locale decides the notation: `de-DE` has no parenthesised form in CLDR and keeps `-1.299,00 €`. + */ accounting?: boolean; /** Drop `.00` on whole amounts — price lists, never invoices. */ trimZeroFraction?: boolean; @@ -19,24 +22,30 @@ export interface FormatMoneyOptions { grouping?: 'auto' | 'never'; } -/** `formatMoney(money(129900,'EUR'), 'de-DE')` → `1.299,00 €`. */ +/** + * `formatMoney(money(129900,'EUR'), 'de-DE')` → `1.299,00 €`. + * + * Delegates to `formatMoneyParts` and joins: a UI styling the symbol off the parts and a label + * rendering the string must not disagree about where the sign goes. Hand-prefixing `-` here put + * it outside the symbol (`-€ 1.299,00`) where `nl-NL` puts it inside (`€ -1.299,00`), and + * `accounting` was applied on this path only. + */ export function formatMoney( amount: Money, locale: string, options: FormatMoneyOptions = {}, ): string { - const rendered = formatterFor(amount.currency, locale, options).format( - Math.abs(toDecimalNumber(amount)), - ); - if (amount.minor < 0) { - return options.accounting === true ? `(${rendered})` : `-${rendered}`; - } - return rendered; + return formatMoneyParts(amount, locale, options) + .map((part) => part.value) + .join(''); } /** * Parts, for UI that styles the symbol or the decimals differently (a smaller superscript * cent, a muted currency code). Never re-split a formatted string with a regex. + * + * The signed value goes to `Intl`, so sign placement and the accounting notation are the + * locale's — the one place either is decided. */ export function formatMoneyParts( amount: Money, @@ -73,12 +82,14 @@ function formatterFor( const exponent = exponentOf(currency); const digits = options.fractionDigits ?? (options.trimZeroFraction === true ? undefined : exponent); + const sign = options.accounting === true ? 'accounting' : 'standard'; const key = [ locale, currency, options.display ?? 'symbol', digits ?? 'auto', options.grouping ?? 'auto', + sign, ].join('|'); const cached = cache.get(key); if (cached !== undefined) return cached; @@ -87,6 +98,7 @@ function formatterFor( style: 'currency', currency, currencyDisplay: options.display ?? 'symbol', + currencySign: sign, ...(digits === undefined ? { minimumFractionDigits: 0, maximumFractionDigits: exponent } : { minimumFractionDigits: digits, maximumFractionDigits: digits }), diff --git a/packages/money/src/index.ts b/packages/money/src/index.ts index 8e90d923..496a6d20 100644 --- a/packages/money/src/index.ts +++ b/packages/money/src/index.ts @@ -57,6 +57,8 @@ export { moneyNotInteger, rateMissing, } from './errors'; +/** `ExchangeRate.ratio` is one of these; a provider with an exact rate writes the pair itself. */ +export type { Fraction } from './factor'; export { currencySymbol, type FormatMoneyOptions, diff --git a/packages/money/src/rounding.test.ts b/packages/money/src/rounding.test.ts index fc4a47b1..9d171919 100644 --- a/packages/money/src/rounding.test.ts +++ b/packages/money/src/rounding.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test'; -import { roundToDigits, roundToInteger } from './rounding'; +import { roundRatio, roundToDigits, roundToInteger } from './rounding'; describe('roundToInteger', () => { test('the modes disagree exactly at .5, which is the point', () => { @@ -25,3 +25,36 @@ describe('roundToInteger', () => { expect(roundToDigits(1.75, 1, 'half-even')).toBe(1.8); }); }); + +describe('roundRatio', () => { + test('the modes disagree exactly at the half, on a value no float can hold', () => { + // 1005/1000 of 100 is exactly 100.5. The float product is 100.49999999999999. + expect(roundRatio(100_500n, 1000n, 'half-up')).toBe(101); + expect(roundRatio(100_500n, 1000n, 'half-even')).toBe(100); + expect(roundRatio(101_500n, 1000n, 'half-even')).toBe(102); + expect(roundRatio(100_500n, 1000n, 'down')).toBe(100); + expect(roundRatio(100_001n, 1000n, 'up')).toBe(101); + }); + + test('is symmetric around zero, whichever side carries the sign', () => { + expect(roundRatio(-100_500n, 1000n, 'half-up')).toBe(-101); + expect(roundRatio(100_500n, -1000n, 'half-up')).toBe(-101); + expect(roundRatio(-100_500n, -1000n, 'half-up')).toBe(101); + expect(roundRatio(-100_500n, 1000n, 'down')).toBe(-100); + }); + + test('an exact integer ratio needs no mode at all', () => { + expect(roundRatio(9200n, 1n)).toBe(9200); + expect(roundRatio(-9200n, 1n)).toBe(-9200); + }); + + test('a zero denominator is this package miswired, so it is X_INVARIANT', () => { + let code = 'no-throw'; + try { + roundRatio(1n, 0n); + } catch (error) { + code = (error as { code?: string }).code ?? 'no-code'; + } + expect(code).toBe('X_INVARIANT'); + }); +}); diff --git a/packages/money/src/rounding.ts b/packages/money/src/rounding.ts index e30305f0..9b53f9e9 100644 --- a/packages/money/src/rounding.ts +++ b/packages/money/src/rounding.ts @@ -3,6 +3,7 @@ * whichever one `Math.round` happens to implement is not an answer. */ +import { invariant } from '@ultimat3/core'; import { notRoundable } from './errors'; export type RoundingMode = @@ -42,6 +43,54 @@ export function roundToInteger(value: number, mode: RoundingMode = DEFAULT_ROUND } } +/** + * Round the exact rational `numerator / denominator` with the same four modes. + * + * The float path above can only judge a value IEEE-754 has already moved: `100 * 1.005` is + * 100.49999999999999, so `half-up` answers 100 where the exact 100.5 owes 101 — a 0.5% fee on + * €1.00 charged as nothing. A scale therefore reaches a mode as a fraction, never as a product. + */ +export function roundRatio( + numerator: bigint, + denominator: bigint, + mode: RoundingMode = DEFAULT_ROUNDING, +): number { + invariant( + denominator !== 0n, + 'X_INVARIANT', + 'cannot round a ratio whose denominator is zero', + 'roundRatio(numerator, 1n, mode) # a zero denominator names no value; divide(amount, 0) is refused before it reaches here, so this is a caller building the fraction itself', + ); + // One sign, carried out front, so each mode sees a magnitude exactly as `roundToInteger` does. + const negative = numerator < 0n !== denominator < 0n; + const top = numerator < 0n ? -numerator : numerator; + const bottom = denominator < 0n ? -denominator : denominator; + const whole = top / bottom; + const remainder = top % bottom; + // `remainder / bottom` vs `1/2` without leaving the integers: compare `2 * remainder` to `bottom`. + const twice = remainder * 2n; + + let rounded: bigint; + switch (mode) { + case 'down': + rounded = whole; + break; + case 'up': + rounded = remainder > 0n ? whole + 1n : whole; + break; + case 'half-up': + rounded = twice >= bottom ? whole + 1n : whole; + break; + case 'half-even': + if (twice > bottom) rounded = whole + 1n; + else if (twice < bottom) rounded = whole; + else rounded = whole % 2n === 0n ? whole : whole + 1n; + break; + } + // Past 2^53 the `Number` is already approximate, which `money()` refuses as X_MONEY_NOT_INTEGER. + return Number(negative ? -rounded : rounded); +} + /** * Round to `digits` decimal places, used when converting a decimal string whose * precision exceeds the currency's minor unit. diff --git a/packages/schema/src/json-schema.ts b/packages/schema/src/json-schema.ts index 9ad97492..2f0107a9 100644 --- a/packages/schema/src/json-schema.ts +++ b/packages/schema/src/json-schema.ts @@ -64,10 +64,28 @@ function stringNode(node: SchemaNode): JsonSchema { }; } +/** + * JSON Schema's `pattern` is an ECMA-262 source with no flag syntax, so a flagged pattern is + * stated in prose instead of silently narrowed: a consumer applying `pattern` alone would refuse + * values this schema accepts, and there is nowhere honest to hide that. + */ +function patternNote(node: SchemaNode): string | undefined { + if (node.pattern === undefined) return undefined; + const flags = node.patternFlags; + return flags === undefined || flags === '' + ? undefined + : `pattern is applied with RegExp flags "${flags}"`; +} + function convert(node: SchemaNode): JsonSchema { + const notes = [node.description, patternNote(node)].filter( + (part): part is string => part !== undefined, + ); + const described = notes.length === 0 ? undefined : notes.join(' — '); + const annotate = (schema: JsonSchema): JsonSchema => ({ ...schema, - ...(node.description === undefined ? {} : { description: node.description }), + ...(described === undefined ? {} : { description: described }), ...(node.hasDefault === true ? { default: node.default } : {}), }); @@ -104,7 +122,14 @@ function convert(node: SchemaNode): JsonSchema { return annotate({ type: 'object', properties: { - minor: { type: 'integer', description: 'amount in minor units, never a float' }, + minor: { + type: 'integer', + description: 'amount in minor units, never a float', + // The safe-integer range the validator enforces, so a generated client refuses the + // same value the boundary does instead of learning about it from a 500. + minimum: -Number.MAX_SAFE_INTEGER, + maximum: Number.MAX_SAFE_INTEGER, + }, currency: { type: 'string', pattern: '^[A-Z]{3}$' }, }, required: ['minor', 'currency'], diff --git a/packages/schema/src/node.ts b/packages/schema/src/node.ts index 365a5039..6a97a894 100644 --- a/packages/schema/src/node.ts +++ b/packages/schema/src/node.ts @@ -40,6 +40,12 @@ export interface SchemaNode { readonly maxLength?: number | undefined; /** Source string of the RegExp, so the node stays JSON-serialisable. */ readonly pattern?: string | undefined; + /** + * The RegExp's flags, carried beside the source for the same reason. Dropping them made + * `t.string.pattern(/^[a-z]+$/i)` reject `ABC` while quoting the pattern that matches it. + * JSON Schema's `pattern` has no flags, so `json-schema.ts` states them in `description`. + */ + readonly patternFlags?: string | undefined; readonly minimum?: number | undefined; readonly maximum?: number | undefined; readonly integer?: boolean | undefined; diff --git a/packages/schema/src/validators.test.ts b/packages/schema/src/validators.test.ts index f221ada3..49712491 100644 --- a/packages/schema/src/validators.test.ts +++ b/packages/schema/src/validators.test.ts @@ -197,6 +197,31 @@ describe('recordSchema', () => { expect(result.issues?.[0]?.path).toEqual(['b']); expect(result.issues?.[1]?.path).toEqual(['d']); }); + + test('a `__proto__` key cannot reach the prototype of the output object', () => { + // Before: `out.a` read "pwned" while `Object.keys(out)` was empty, so every + // `input.settings[k] ?? fallback` in a handler answered with the attacker's value. + const schema = recordSchema(objectSchema({ a: builtinT.string })); + const result = validate(schema, JSON.parse('{"__proto__":{"a":"pwned"}}')); + expect(result.issues?.length).toBe(1); + expect(result.issues?.[0]?.path).toEqual(['__proto__']); + }); + + test('`constructor` and `prototype` are refused for the same reason', () => { + const schema = recordSchema(builtinT.number); + expect(validate(schema, { constructor: 1 }).issues?.[0]?.path).toEqual(['constructor']); + expect(validate(schema, { prototype: 1 }).issues?.[0]?.path).toEqual(['prototype']); + }); + + test('the accepted record carries no prototype at all', () => { + const schema = recordSchema(builtinT.number); + const result = validate(schema, { a: 1 }); + expect(result.issues).toBeUndefined(); + if (result.issues === undefined) { + expect(Object.getPrototypeOf(result.value)).toBe(null); + expect(result.value).toEqual({ a: 1 }); + } + }); }); describe('nullableSchema', () => { @@ -234,6 +259,21 @@ describe('builtinT.string', () => { expect(validate(builtinT.string, 123).issues).toBeDefined(); }); + test('a pattern keeps its flags, in the check and in the message', () => { + // The node held `regex.source` alone, so `new RegExp(node.pattern)` was a DIFFERENT regex: + // `/^[a-z]+$/i` refused `ABC` and the error quoted the pattern that would have matched it. + const insensitive = builtinT.string.pattern(/^[a-z]+$/i); + expect(insensitive.node.patternFlags).toBe('i'); + expect(validate(insensitive, 'ABC').issues).toBeUndefined(); + expect(validate(insensitive, 'abc').issues).toBeUndefined(); + expect(validate(insensitive, 'A1').issues).toBeDefined(); + expect(validate(insensitive, 'A1').issues?.[0]?.message).toContain('/^[a-z]+$/i'); + // An unflagged pattern carries no flags field and its message is unchanged. + const plain = builtinT.string.pattern(/^[a-z]+$/); + expect(plain.node.patternFlags).toBeUndefined(); + expect(validate(plain, 'ABC').issues).toBeDefined(); + }); + test('min/max/pattern chain onto a new schema without mutating the original', () => { const withMin = builtinT.string.min(3); expect(withMin.node.minLength).toBe(3); @@ -384,6 +424,21 @@ describe('builtinT.money', () => { test('rejects a non-object', () => { expect(validate(builtinT.money, 'money').issues).toBeDefined(); }); + + test('rejects a minor amount past the safe-integer range', () => { + // `Number.isInteger(2**53)` is true and `money()`/`parseMinor` both refuse it, so accepting + // it here turned a 422 with a field path into a 500 at the row write. + const result = validate(builtinT.money, { minor: 9_007_199_254_740_992, currency: 'EUR' }); + expect(result.issues?.length).toBe(1); + expect(result.issues?.[0]?.path).toEqual(['minor']); + expect( + validate(builtinT.money, { minor: -9_007_199_254_740_992, currency: 'EUR' }).issues, + ).toBeDefined(); + // The largest amount that IS representable still passes. + expect( + validate(builtinT.money, { minor: Number.MAX_SAFE_INTEGER, currency: 'EUR' }).issues, + ).toBeUndefined(); + }); }); describe('builtinT.timezone', () => { diff --git a/packages/schema/src/validators.ts b/packages/schema/src/validators.ts index 8b929826..77f76c2b 100644 --- a/packages/schema/src/validators.ts +++ b/packages/schema/src/validators.ts @@ -27,8 +27,9 @@ import type { InferInput, InferOutput, StandardIssue } from './standard'; * * It lives at tier 0 because that is the only tier every other package may import, and `number` * rather than `bigint` because money crosses the wire on every surface this framework projects — - * `JSON.stringify` refuses a bigint, and this node is also the OpenAPI contract. A stored value - * past `Number.MAX_SAFE_INTEGER` is refused where it is decoded, loudly; it is never widened here. + * `JSON.stringify` refuses a bigint, and this node is also the OpenAPI contract. A value past + * `Number.MAX_SAFE_INTEGER` is refused HERE, at the boundary, with the field path — and again + * where it is decoded; it is never widened. * * Never a float, and never an amount without its currency. */ @@ -64,6 +65,16 @@ export interface ObjectSchema extends Schema, Sha type Simplified = { [K in keyof S]: S[K] } & {}; +/** + * The literal the caller wrote, flags included. An error quoting `/^[a-z]+$/` for a pattern + * carrying `i` names something that would have matched the value it just refused. + */ +function describePattern(node: SchemaNode): string { + return node.patternFlags === undefined || node.patternFlags === '' + ? String(node.pattern) + : `/${node.pattern}/${node.patternFlags}`; +} + function stringLike(node: SchemaNode, what: string, test?: (value: string) => boolean) { const check: Check = (value, path) => { if (typeof value !== 'string') return fail(path, expected(what, value)); @@ -73,8 +84,8 @@ function stringLike(node: SchemaNode, what: string, test?: (value: string) => bo if (node.maxLength !== undefined && value.length > node.maxLength) { return fail(path, expected(`${what} of at most ${node.maxLength} chars`, value)); } - if (node.pattern !== undefined && !new RegExp(node.pattern).test(value)) { - return fail(path, expected(`${what} matching ${node.pattern}`, value)); + if (node.pattern !== undefined && !new RegExp(node.pattern, node.patternFlags).test(value)) { + return fail(path, expected(`${what} matching ${describePattern(node)}`, value)); } if (test !== undefined && !test(value)) return fail(path, expected(what, value)); return pass(value); @@ -92,7 +103,18 @@ function makeStringSchema( ...base, min: (length) => makeStringSchema({ ...node, minLength: length }, what, test), max: (length) => makeStringSchema({ ...node, maxLength: length }, what, test), - pattern: (regex) => makeStringSchema({ ...node, pattern: regex.source }, what, test), + pattern: (regex) => + makeStringSchema( + // Flags travel with the source: a node holding only `source` rebuilt a *different* + // RegExp, so `/^[a-z]+$/i` refused `ABC` and quoted the pattern that matches it. + { + ...node, + pattern: regex.source, + ...(regex.flags === '' ? {} : { patternFlags: regex.flags }), + }, + what, + test, + ), }; } @@ -226,6 +248,15 @@ export function unionSchema( }); } +/** + * Keys that reach an object's prototype rather than its own properties. A record's keys are the + * caller's, so `{"__proto__":{…}}` on a `{}` literal set the OUTPUT's prototype: `Object.keys` + * answered `[]` while `settings[k] ?? fallback` handed a handler the attacker's value for a key + * that was never sent. Refused by name AND built on a null prototype — the null prototype alone + * would keep `__proto__` as a silent own key nobody declared. + */ +const PROTOTYPE_KEYS: ReadonlySet = new Set(['__proto__', 'constructor', 'prototype']); + export function recordSchema( values: S, ): Schema>>, Record>> { @@ -236,8 +267,15 @@ export function recordSchema( (value, path) => { if (!isPlainObject(value)) return fail(path, expected('an object', value)); const issues: StandardIssue[] = []; - const out: Record = {}; + const out: Record = Object.create(null) as Record; for (const [key, entry] of Object.entries(value)) { + if (PROTOTYPE_KEYS.has(key)) { + issues.push({ + message: expected(`a record key that is not ${[...PROTOTYPE_KEYS].join(' | ')}`, key), + path: [...path, key], + }); + continue; + } const result = valueCheck(entry, [...path, key]); if (result.ok) out[key] = result.value; else issues.push(...result.issues); @@ -300,7 +338,12 @@ const moneySchema: Schema = makeSchema = makeSchema/.meta/a/b.json`, and `.meta/a/b.json` was + itself a legal key, so an uploader could rewrite another object's recorded `contentType` to + `text/html` and have a route serve attacker HTML from the app's origin. Reserved in + `assertSafeKey`, so it holds for S3 too — a key valid on one driver and refused on another is two + key rules. The `list()` skip stays as a second line of defence. +- **`localDriver` refuses to construct outside development without a usable signing secret** + (`X_ENV_MISSING`, borrowed from core). Usable means neither the `signingSecret` option nor + `STORAGE_SIGNING_SECRET` is missing, empty **or** the published `DEV_SIGNING_SECRET` — pasting + the literal into `app.config.ts` configures nothing, so it is refused exactly as its absence is. + `DEV_SIGNING_SECRET` is published in this repo, and `acceptSignedUpload` trusts a signed URL's + constraints over the app's `uploadPolicy` — so the fallback is a universal grant to mint any + `PUT`. Refused at construction, not at the first `signedUrl()`: a process that cannot sign safely + must not finish booting. The cause names the environment `resolveEnvironment()` resolved, which + may be `NODE_ENV`'s, never a variable the process did not set. `usesDevStorageSecret()` is the + `x doctor` predicate, mirroring core's `usesDevCursorSecret()`; it reads the env var, so a disk + handed an explicit `signingSecret` is outside its question. - **The mounted read half is `@ultimat3/cli`'s `dev-storage.ts`, not this package.** `GET /_storage/:disk/*key` gates on `@ultimat3/policy`'s `evaluate()` (`storage:read`), which is tier 2 and unreachable from here — so a "serve this object" helper in this package could only ever be diff --git a/packages/storage/README.md b/packages/storage/README.md index c4fe3abb..20b25d1d 100644 --- a/packages/storage/README.md +++ b/packages/storage/README.md @@ -39,7 +39,11 @@ per listed row. `assertSafeKey()` runs on every key before it reaches a driver. Rejected: `..` segments, absolute keys, backslashes, NUL/control bytes, percent-encoded separators (`%2e`, `%2f`), -empty segments, over 1024 chars. No sanitising — a key that needed fixing was built wrong. +empty segments, over 1024 chars, and a first segment of `.meta` (`META_DIR`) — the local driver's +sidecar namespace, reserved on **every** driver so one key rule covers disk and S3 alike. Without +it, `put('.meta/a/b.json', …)` overwrote the recorded content type of `a/b` and a route serving +that object answered attacker HTML from the app's own origin. No sanitising — a key that needed +fixing was built wrong. `scopedKey('org-1', 'avatars', 'a.png')` is `org/org-1/avatars/a.png`; guard every client-supplied key with `isWithinOrg(key, ctx.actor.orgId)`. A surface that serves objects pairs it with `isTenantScoped(key)`: only a key already inside `org/` is another tenant's to refuse, so @@ -50,6 +54,15 @@ it with `isTenantScoped(key)`: only a key already inside `org/` is another tenan The HMAC covers the **constraints**, not just the key — `v1 \n METHOD \n key \n expiresAt \n maxBytes \n contentType`. A client that edits `?x-max=` invalidates the signature — it cannot widen what it was granted. + +The HMAC key is `signingSecret`, else `STORAGE_SIGNING_SECRET`, else the shipped +`DEV_SIGNING_SECRET` — and **only in `development` or `test`**. Anywhere else `localDriver` refuses +to construct (`X_ENV_MISSING`) unless one of those two is set to something that is not the shipped +literal: the dev literal is published in this repo, so signing with it lets anyone mint a `PUT` for +any key with a `maxBytes` and `contentType` of their choosing, which `acceptSignedUpload` then +trusts over the app's own `uploadPolicy`. Setting `STORAGE_SIGNING_SECRET=$DEV_SIGNING_SECRET`, or +pasting the literal into `signingSecret`, is refused exactly as an unset variable is. `usesDevStorageSecret()` is the +`x doctor` probe for it, the twin of core's `usesDevCursorSecret()`. Verification is constant-time, checks the signature *before* the expiry (a forged URL never learns it was merely late), takes a `Clock` so tests freeze time, and returns `{ ok: false, reason }` rather than throwing — `malformed | unsafe-key | signature-mismatch | @@ -132,6 +145,7 @@ attached key is a job that deletes production data the first time an app forgets | `X_STORAGE_ORG_MISMATCH` | the key is well-formed and unforged, and belongs to another org | | `X_STORAGE_UPLOAD_FAILED` | client half: the presigned `PUT` answered non-2xx or never landed | | `X_NOT_IMPLEMENTED` | S3 user metadata | +| `X_ENV_MISSING` | core's: S3 credential env vars, or a `localDriver` built outside development where neither `signingSecret` nor `STORAGE_SIGNING_SECRET` holds a secret other than the published `DEV_SIGNING_SECRET` | | `X_IMAGE_UNSUPPORTED` | core's: an `avif`/`webp` encode, or a source no built-in decoder reads | | `X_IMAGE_DECODE_FAILED` | core's: truncated or corrupt image bytes | diff --git a/packages/storage/src/driver-local.test.ts b/packages/storage/src/driver-local.test.ts index b5eef70a..41c2a1aa 100644 --- a/packages/storage/src/driver-local.test.ts +++ b/packages/storage/src/driver-local.test.ts @@ -3,8 +3,14 @@ import { mkdtemp, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { frozenClock } from '@ultimat3/core'; import type { StorageDriver } from './driver'; -import { localDriver } from './driver-local'; +import { + DEV_SIGNING_SECRET, + localDriver, + STORAGE_SIGNING_SECRET_KEY, + usesDevStorageSecret, +} from './driver-local'; import { isStorageError } from './errors'; +import { META_DIR, scopedKey } from './path'; let root = ''; let driver: StorageDriver; @@ -12,6 +18,16 @@ let driver: StorageDriver; const bytesOf = (text: string): Uint8Array => new TextEncoder().encode(text); const textOf = (bytes: Uint8Array): string => new TextDecoder().decode(bytes); +/** The error code a driver call answered with, or how it failed to answer with one. */ +async function catchCode(run: () => Promise): Promise { + try { + await run(); + } catch (error) { + return isStorageError(error) ? error.code : `not-a-storage-error: ${String(error)}`; + } + return 'no-throw'; +} + beforeEach(async () => { root = await mkdtemp(`${tmpdir()}/ultimate-storage-`); driver = localDriver({ @@ -99,6 +115,31 @@ describe('localDriver', () => { expect(await Bun.file(`${root}/../${escapee}`).exists()).toBe(false); }); + test('the reserved .meta namespace is refused at the driver boundary, not just by the validator', async () => { + // `put('a/b', png)` writes its sidecar to `/.meta/a/b.json`. If `.meta/a/b.json` were a + // legal key, an uploader could rewrite the recorded contentType of `a/b` to `text/html` and + // have the read route serve attacker HTML from the app's own origin. Asserted through every + // method, because a guard is only where it is written. + const reserved = `${META_DIR}/org/org-1/a.png.json`; + expect( + await Promise.all([ + catchCode(() => driver.put(reserved, bytesOf('x'))), + catchCode(() => driver.get(reserved)), + catchCode(() => driver.stream(reserved)), + catchCode(() => driver.exists(reserved)), + catchCode(() => driver.delete(reserved)), + catchCode(() => driver.signedUrl(reserved)), + ]), + ).toEqual(Array(6).fill('X_STORAGE_PATH_UNSAFE')); + expect(await Bun.file(`${root}/${reserved}`).exists()).toBe(false); + + // Only the FIRST segment is reserved: a tenant key of its own named `.meta` collides with + // nothing, since its sidecar lands under `/.meta/org/org-1/.meta/…`. + const scoped = scopedKey('org-1', META_DIR, 'a.json'); + await driver.put(scoped, bytesOf('ordinary'), { contentType: 'application/json' }); + expect(textOf((await driver.get(scoped)).bytes)).toBe('ordinary'); + }); + test('list paginates by cursor in lexicographic order', async () => { for (const name of ['a.txt', 'b.txt', 'c.txt']) { await driver.put(`org/org-1/${name}`, bytesOf(name), { contentType: 'text/plain' }); @@ -131,3 +172,105 @@ describe('localDriver', () => { expect(await new Response(stream).text()).toBe('streamed'); }); }); + +describe('the dev signing secret', () => { + // The env key the driver itself declares — a rename must break this test, not slip past it + // because the test spelled the old name out a second time. + const KEY = STORAGE_SIGNING_SECRET_KEY; + const ENV = 'ULTIMATE_ENV'; + let previousSecret: string | undefined; + let previousEnv: string | undefined; + + beforeEach(() => { + previousSecret = process.env[KEY]; + previousEnv = process.env[ENV]; + }); + + afterEach(() => { + if (previousSecret === undefined) delete process.env[KEY]; + else process.env[KEY] = previousSecret; + if (previousEnv === undefined) delete process.env[ENV]; + else process.env[ENV] = previousEnv; + }); + + test('usesDevStorageSecret reports the shipped key, exactly as the cursor one does', () => { + delete process.env[KEY]; + expect(usesDevStorageSecret()).toBe(true); + process.env[KEY] = ''; + expect(usesDevStorageSecret()).toBe(true); + process.env[KEY] = DEV_SIGNING_SECRET; + expect(usesDevStorageSecret()).toBe(true); + process.env[KEY] = 'a-real-secret'; + expect(usesDevStorageSecret()).toBe(false); + }); + + /** The code `localDriver` refused to construct with, or how it failed to refuse. */ + const bootCode = (options: { readonly signingSecret?: string }): string => { + try { + localDriver({ root, ...options }); + } catch (error) { + return isStorageError(error) ? error.code : `not-a-storage-error: ${String(error)}`; + } + return 'no-throw'; + }; + + test('a production disk refuses to boot with no secret at all', () => { + // The literal is in this repo, so anyone holding it can mint a PUT for any key with any + // maxBytes and contentType — and acceptSignedUpload trusts the signed constraints over the + // app's own uploadPolicy. Refused at construction, so the boot fails, not the first upload. + for (const environment of ['production', 'staging']) { + process.env[ENV] = environment; + delete process.env[KEY]; + expect(bootCode({})).toBe('X_ENV_MISSING'); + process.env[KEY] = ''; + expect(bootCode({})).toBe('X_ENV_MISSING'); + } + }); + + test('a production disk refuses to boot ON the published key, however it arrives', () => { + // Setting STORAGE_SIGNING_SECRET to the published literal is not configuring a secret, it is + // spelling the fallback out — and pasting it into `app.config.ts` is the same key again. + // Both used to boot, and a booted process signs grants anyone in this repo can forge. + for (const environment of ['production', 'staging']) { + process.env[ENV] = environment; + process.env[KEY] = DEV_SIGNING_SECRET; + expect(bootCode({})).toBe('X_ENV_MISSING'); + delete process.env[KEY]; + expect(bootCode({ signingSecret: DEV_SIGNING_SECRET })).toBe('X_ENV_MISSING'); + } + }); + + test('the refusal names the resolved environment, whichever variable resolved it', () => { + // resolveEnvironment() reads NODE_ENV when ULTIMATE_ENV is unset, so a cause that blamed + // ULTIMATE_ENV reported a variable this process never set. + delete process.env[KEY]; + process.env[ENV] = 'staging'; + let cause = ''; + try { + localDriver({ root }); + } catch (error) { + cause = isStorageError(error) ? error.cause : String(error); + } + expect(cause).toContain('the resolved environment is "staging"'); + expect(cause).not.toContain('ULTIMATE_ENV'); + }); + + test('the dev key still signs a dev disk, so `x dev` needs no configuration', () => { + // The whole point of the fallback: zero-config locally, refused everywhere else. + process.env[ENV] = 'development'; + process.env[KEY] = DEV_SIGNING_SECRET; + expect(localDriver({ root }).name).toBe('local'); + delete process.env[KEY]; + expect(localDriver({ root }).name).toBe('local'); + }); + + test('a production disk with a real secret boots, and dev still needs none', () => { + process.env[ENV] = 'production'; + process.env[KEY] = 'a-real-secret'; + expect(localDriver({ root }).name).toBe('local'); + delete process.env[KEY]; + expect(localDriver({ root, signingSecret: 'passed-in' }).name).toBe('local'); + process.env[ENV] = 'development'; + expect(localDriver({ root }).name).toBe('local'); + }); +}); diff --git a/packages/storage/src/driver-local.ts b/packages/storage/src/driver-local.ts index 512ed593..aa4dcb11 100644 --- a/packages/storage/src/driver-local.ts +++ b/packages/storage/src/driver-local.ts @@ -3,7 +3,7 @@ // Content type, etag and user metadata live in a sidecar under `.meta/`: a POSIX file has // nowhere to keep them, and `get` must round-trip exactly what `put` was handed. -import { type Clock, systemClock } from '@ultimat3/core'; +import { type Clock, isLocal, resolveEnvironment, systemClock } from '@ultimat3/core'; import { DEFAULT_CONTENT_TYPE, DEFAULT_LIST_LIMIT, @@ -19,13 +19,35 @@ import { sha256Base64, toBytes, } from './driver'; -import { checksumMismatch, objectNotFound } from './errors'; -import { assertSafeKey } from './path'; +import { checksumMismatch, objectNotFound, signingSecretMissing } from './errors'; +import { assertSafeKey, META_DIR } from './path'; import { buildSignedUrl } from './signed-url'; -const META_DIR = '.meta'; const DRIVER_NAME = 'local'; +/** + * The dev-only fallback signing key. A literal, not a per-process random one, so a restart does + * not invalidate every URL `x dev` handed out — and published in this repo, which is exactly why + * `localDriver` refuses to use it outside a development or test environment. + */ +export const DEV_SIGNING_SECRET = 'ultimate-dev-signing-secret'; + +/** The env key production must set. Named once, read by the driver and by the predicate below. */ +export const STORAGE_SIGNING_SECRET_KEY = 'STORAGE_SIGNING_SECRET'; + +/** + * True while a local disk built without an explicit `signingSecret` would sign with the shipped + * development key — `x doctor` reports it, exactly as it reports `usesDevCursorSecret()`. + * + * Reads the environment, not a driver instance: this is the same question `x doctor` asks about + * the cursor secret, and a disk handed an explicit `signingSecret` in `app.config.ts` never + * consults the variable at all. + */ +export function usesDevStorageSecret(): boolean { + const configured = process.env[STORAGE_SIGNING_SECRET_KEY]; + return configured === undefined || configured === '' || configured === DEV_SIGNING_SECRET; +} + export interface LocalDriverOptions { /** Directory the disk owns outright. Created on first write. */ readonly root: string; @@ -70,10 +92,20 @@ export function localDriver(options: LocalDriverOptions): StorageDriver { const root = options.root.replace(/\/+$/, ''); const clock = options.clock ?? systemClock; const baseUrl = options.baseUrl ?? `/_storage/${DRIVER_NAME}`; - // A dev disk must work with zero config; a production disk that forgets the secret still - // gets a *shared* secret, never a per-process random one that breaks on restart. - const secret = - options.signingSecret ?? process.env['STORAGE_SIGNING_SECRET'] ?? 'ultimate-dev-signing-secret'; + // A dev disk must work with zero config. Outside development the fallback is refused rather + // than used: the literal is published, so signing with it hands every reader the power to mint + // a PUT for any key with any size and type limit — which `acceptSignedUpload` then trusts over + // the app's own `uploadPolicy`. Refused HERE, at construction, so the boot fails rather than + // the first upload. + // The published literal counts as no secret at all, whichever way it arrives: an env var or an + // `app.config.ts` that pasted it in signs exactly as weakly as the fallback does. + const supplied = options.signingSecret ?? process.env[STORAGE_SIGNING_SECRET_KEY]; + const configured = + supplied === undefined || supplied === '' || supplied === DEV_SIGNING_SECRET + ? undefined + : supplied; + if (configured === undefined && !isLocal()) throw signingSecretMissing(resolveEnvironment()); + const secret = configured ?? DEV_SIGNING_SECRET; const filePath = (key: string): string => `${root}/${key}`; const metaPath = (key: string): string => `${root}/${META_DIR}/${key}.json`; diff --git a/packages/storage/src/driver-s3.test.ts b/packages/storage/src/driver-s3.test.ts index b4b89d9b..54d90b5c 100644 --- a/packages/storage/src/driver-s3.test.ts +++ b/packages/storage/src/driver-s3.test.ts @@ -15,6 +15,7 @@ import { s3Driver, } from './driver-s3'; import { isStorageError, objectNotFound } from './errors'; +import { META_DIR, scopedKey } from './path'; /** The driver's private `DRIVER_NAME`; the fake reports failures against the same disk. */ const FAKE_DISK = 's3'; @@ -273,6 +274,31 @@ describe('s3Driver', () => { // The guard fires before any of these methods ever call `client.file()`. expect(fake.fileCalls).toEqual([]); }); + + test('the reserved .meta first segment is refused here too, though S3 keeps no sidecar', async () => { + // The reservation belongs to `assertSafeKey`, not to the local driver: a key valid on S3 + // and refused on disk is two key rules, and an app that migrates disks would discover the + // difference through objects it can no longer write. + const fake = new FakeS3Client(); + const driver = s3Driver({ bucket: 'b', client: fake }); + const reserved = `${META_DIR}/org/org-1/a.png.json`; + + for (const caught of [ + await catchError(() => driver.put(reserved, bytesOf('x'))), + await catchError(() => driver.get(reserved)), + await catchError(() => driver.delete(reserved)), + await catchError(() => driver.exists(reserved)), + await catchError(() => driver.stream(reserved)), + await catchError(() => driver.signedUrl(reserved)), + ]) { + expect(codeOf(caught)).toBe('X_STORAGE_PATH_UNSAFE'); + } + expect(fake.fileCalls).toEqual([]); + + // Only the first segment is reserved — `.meta` deeper in a key is an ordinary name. + await driver.put(scopedKey('org-1', META_DIR, 'a.json'), bytesOf('ordinary')); + expect([...new Set(fake.fileCalls)]).toEqual(['org/org-1/.meta/a.json']); + }); }); describe('list', () => { diff --git a/packages/storage/src/errors.ts b/packages/storage/src/errors.ts index 6acb289b..9e5112f6 100644 --- a/packages/storage/src/errors.ts +++ b/packages/storage/src/errors.ts @@ -22,8 +22,10 @@ export const STORAGE_OWNED_ERROR_CODES = [ * `X_NOT_IMPLEMENTED` is `@ultimat3/core`'s. `storageNotImplemented()` throws it and this package * keeps no title for it — one code, one owner, one title, or the two copies drift apart in silence. * `X_IMAGE_UNSUPPORTED` / `X_IMAGE_DECODE_FAILED` are core's too and surface unwrapped (`image.ts`). + * `X_ENV_MISSING` is core's for the same reason: an unset `STORAGE_SIGNING_SECRET` outside + * development is a missing environment variable, not a storage concept needing its own code. */ -export const STORAGE_BORROWED_ERROR_CODES = ['X_NOT_IMPLEMENTED'] as const; +export const STORAGE_BORROWED_ERROR_CODES = ['X_NOT_IMPLEMENTED', 'X_ENV_MISSING'] as const; /** Every code storage can throw through `StorageError`: the owned ones plus the borrowed one. */ export const STORAGE_ERROR_CODES = [ @@ -191,6 +193,25 @@ export const uploadFailed = (path: string, status: number, detail: string): Stor meta: { path, status, detail }, }); +/** + * The local disk fell back to the shipped dev signing key outside development. + * + * That literal is published in this repo, so anyone holding it can mint a signed `PUT` for any + * key — including another org's — with `maxBytes` and `contentType` of their choosing, and + * `acceptSignedUpload` trusts the signed constraints over the app's `uploadPolicy`. A 200KB + * avatar grant becomes an unlimited upload of any type. Refused at construction, not at the + * first `signedUrl()`: a process that cannot sign safely must not finish booting. + */ +export const signingSecretMissing = (environment: string): StorageError => + new StorageError({ + code: 'X_ENV_MISSING', + // The environment names what `resolveEnvironment()` resolved, which may have come from + // NODE_ENV — naming ULTIMATE_ENV here reported a variable the process never set. + cause: `the local disk has no usable signing secret (no signingSecret option, and STORAGE_SIGNING_SECRET is unset, empty or the published development key) and the resolved environment is "${environment}", so it would sign URLs with the shipped development key`, + fix: 'export STORAGE_SIGNING_SECRET="$(openssl rand -hex 32)"', + meta: { key: 'STORAGE_SIGNING_SECRET', environment }, + }); + /** An interface-complete driver whose remote half is not bound yet. Always carries a fix. */ export const storageNotImplemented = (feature: string, fix: string): StorageError => new StorageError({ diff --git a/packages/storage/src/index.ts b/packages/storage/src/index.ts index e07ee85e..4e562b1b 100644 --- a/packages/storage/src/index.ts +++ b/packages/storage/src/index.ts @@ -39,7 +39,12 @@ export { toBytes, } from './driver'; export type { LocalDriverOptions } from './driver-local'; -export { localDriver } from './driver-local'; +export { + DEV_SIGNING_SECRET, + localDriver, + STORAGE_SIGNING_SECRET_KEY, + usesDevStorageSecret, +} from './driver-local'; export type { S3ClientLike, S3DriverOptions, @@ -64,6 +69,7 @@ export { StorageError, signedUrlExpired, signedUrlRejected, + signingSecretMissing, storageNotImplemented, tooLarge, uploadFailed, @@ -99,6 +105,7 @@ export { keyDirname, keyExtname, MAX_KEY_LENGTH, + META_DIR, ORG_PREFIX, orgPrefix, scopedKey, diff --git a/packages/storage/src/path.test.ts b/packages/storage/src/path.test.ts index 32eb633e..bdd8e086 100644 --- a/packages/storage/src/path.test.ts +++ b/packages/storage/src/path.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from 'bun:test'; import { isStorageError } from './errors'; -import { assertSafeKey, isTenantScoped, isWithinOrg, joinKey, scopedKey } from './path'; +import { assertSafeKey, isTenantScoped, isWithinOrg, joinKey, META_DIR, scopedKey } from './path'; /** The code, or a description of why there wasn't one — a failing assert then reads clearly. */ function codeOf(fn: () => unknown): string { @@ -99,3 +99,23 @@ describe('joinKey', () => { expect(joinKey('a', 'b/c', 'd.png')).toBe('a/b/c/d.png'); }); }); + +describe('the sidecar namespace is reserved', () => { + // `.meta/.json` is where the local driver records an object's content type and etag. As a + // legal key, `put('.meta/a/b.json', '{"contentType":"text/html","etag":"x"}')` overwrote the + // sidecar for `a/b`, so `head('a/b')` reported text/html and a route serving that object + // returned attacker HTML from the app's own origin. + test('a key whose first segment is .meta is refused, on every driver', () => { + expect(codeOf(() => assertSafeKey('.meta/a/b.json'))).toBe(UNSAFE); + expect(codeOf(() => assertSafeKey('.meta'))).toBe(UNSAFE); + expect(codeOf(() => assertSafeKey(joinKey(META_DIR, 'a', 'b.json')))).toBe(UNSAFE); + expect(codeOf(() => assertSafeKey(scopedKey('o1', META_DIR, 'x.json')))).toBe( + 'no-error-thrown', + ); + }); + + test('.meta anywhere but the first segment is an ordinary key', () => { + expect(assertSafeKey('org/o1/.meta/a.json')).toBe('org/o1/.meta/a.json'); + expect(assertSafeKey('.metadata/a.json')).toBe('.metadata/a.json'); + }); +}); diff --git a/packages/storage/src/path.ts b/packages/storage/src/path.ts index 6d09a784..360b7c7e 100644 --- a/packages/storage/src/path.ts +++ b/packages/storage/src/path.ts @@ -9,6 +9,15 @@ import { pathUnsafe } from './errors'; export const MAX_KEY_LENGTH = 1024; export const ORG_PREFIX = 'org'; +/** + * Reserved first segment: the local driver's sidecar namespace, where an object's recorded + * content type and etag live. Without the reservation `/.meta/a/b.json` was a legal object + * key, so an uploader could overwrite the sidecar for `a/b` and make a route serving that object + * answer `text/html` from the app's own origin. Reserved for EVERY driver, not just the local + * one — a key that is valid on S3 and refused on disk is two key rules. + */ +export const META_DIR = '.meta'; + // `%2e%2e%2f` decodes to `../` in any layer that decodes twice (proxy, then framework). const ENCODED_SEPARATOR = /%(?:2e|2f|5c|00)/i; @@ -30,7 +39,9 @@ function unsafeReason(key: string): string | undefined { if (key.includes('\\')) return 'contains a backslash'; if (key.startsWith('/')) return 'is absolute (leading "/")'; if (ENCODED_SEPARATOR.test(key)) return 'contains a percent-encoded separator (%2e/%2f/%5c)'; - for (const segment of key.split('/')) { + const segments = key.split('/'); + if (segments[0] === META_DIR) return `starts with the reserved "${META_DIR}" segment`; + for (const segment of segments) { if (segment.length === 0) return 'contains an empty segment ("//" or a trailing "/")'; if (segment === '.' || segment === '..') return `contains a "${segment}" segment`; if (segment !== segment.trim()) return `has a padded segment ${JSON.stringify(segment)}`; diff --git a/wiki/Error-Codes.md b/wiki/Error-Codes.md index 5654e53d..53c1d4c3 100644 --- a/wiki/Error-Codes.md +++ b/wiki/Error-Codes.md @@ -244,6 +244,7 @@ synthesizes `https://ultimate.dev/errors/` for a code no page here documen | `X_CACHE_TAG_UNKNOWN` | a tag no entity declared | typo in `invalidates: [tag.pots]` | `x manifest` to regenerate the tag graph, then fix the tag | | `X_CACHE_TOO_LARGE` | one entry exceeds the tier's byte budget | caching a whole row set | raise `cache..maxBytes`, or cache a projection | | `X_CACHE_DRIVER_UNAVAILABLE` | a tier's backing store is missing | no Redis binding, or a purge driver built without `FASTLY_API_TOKEN` / `CLOUDFLARE_API_TOKEN` | provision the tier, or drop it from `app.config.ts` | +| `X_CACHE_TTL_INVALID` | a cache TTL that is not a positive number of milliseconds | `ttlMs: 0`, a negative value, `NaN` or `Infinity`. `0` used to mean "never expires" in the memory tier and "one second" in the Redis tier, so a stack holding both answered differently depending on which tier hit | pass a positive `ttlMs`, or omit it and inherit the tier's default — "do not cache" is expressed by not declaring a `cache` block | | `X_CACHE_PURGE_FAILED` | the CDN refused a purge | a wrong or unscoped API token, a zone without tag purge, a throttle, a key carrying whitespace or a comma | `meta.retryable === true` → the identical purge can land again; `false` → set the env key the `fix` names, then `x dev` | ## Storage @@ -438,7 +439,7 @@ synthesizes `https://ultimate.dev/errors/` for a code no page here documen | Code | Means | Typical cause | Fix | |---|---|---|---| | `X_CLI_UNKNOWN_COMMAND` | not a command | a typo | `x help` — the suggestion is in `fix` | -| `X_CLI_BAD_FLAG` | flag rejected | unknown flag, or a bad value | `x --help` | +| `X_CLI_BAD_FLAG` | flag or positional rejected | unknown flag, a bad value (`--port abc`, `--workers 4.9` — refused, never coerced), or a required positional left out. A missing positional says so and names it, rather than inventing a flag that does not exist | `x --help` | | `X_CLI_UNEXPECTED` | the CLI itself failed | a bug, or a broken environment | `x doctor --json` and attach it to an issue | | `X_VERIFY_FAILED` | one or more verify steps failed | the gate is red | `x verify --json` — every step's findings arrive in one run | | `X_TYPECHECK_FAILED` | `tsc` failed | a type error anywhere in the workspace | `bunx tsc -b --pretty false` | diff --git a/wiki/I18n.md b/wiki/I18n.md index 19206736..75aa87b5 100644 --- a/wiki/I18n.md +++ b/wiki/I18n.md @@ -118,7 +118,7 @@ A dynamic call contributes its static head (`plans.`) as a runtime-key prefix, s | Rule | Detail | |---|---| | `site/` | path prefix is authoritative and prerendered per locale. No client-side locale swap | -| `app/` / `api/` | resolution order `header → cookie → user → query`, configurable via `configureLocales({ order })` | +| `app/` / `api/` | resolution order `query → cookie → user → header`, configurable via `configureLocales({ order })`. An explicit choice outranks the browser: `?locale=` is how an email preview link pins a locale, the cookie is what the language switcher writes, and `Accept-Language` is only the fallback when nobody has chosen | | Cookie | `x_locale`, written only by an explicit language switcher | | Unsupported tag | skipped, not thrown — a stale cookie must never 500 a page. `assertSupportedLocale` throws `X_LOCALE_UNSUPPORTED` where an unknown tag is a caller bug | | Direction | `currentDirection()` returns `'rtl'` for `ar`, `he`, `fa`, `ur`, … and is written to `` |