diff --git a/apps/moderator/.env.example b/apps/moderator/.env.example index a225b7f9ab..86a96992a3 100644 --- a/apps/moderator/.env.example +++ b/apps/moderator/.env.example @@ -30,27 +30,28 @@ AUTH_INTERNAL_TOKEN= # --- Main-app callbacks --- # Shared secret for the main app's token-guarded internal endpoints (e.g. the Meilisearch re-index # callback /api/internal/search-index-update). Must match the main app's WEBHOOK_TOKEN. Unset = sync no-ops. -# Also ACCEPTED inbound on every endpoint wrapped in WebhookEndpoint ($lib/server/webhook-endpoint) β€” -# /api/mod/* and the XGuard agent API. There is no user behind it: nothing it reaches may attribute a -# write, and revoking access means rotating it for every holder. -# πŸ”΄ OUTBOUND USE IS WHY THIS CANNOT SIMPLY BE ROTATED. Four services here present it TO the main app, -# so its value is pinned by the main app's and is not ours to change independently. Prefer -# MOD_INBOUND_TOKEN below for anything that only calls IN. +# πŸ”΄ OUTBOUND ONLY β€” NO LONGER ACCEPTED INBOUND. It was dropped from `acceptedTokens()` in +# $lib/server/webhook-endpoint once the runtime attribution signal showed no caller presenting it +# inbound; presenting it to this app now gets a 401. It is STILL REQUIRED: four services here present +# it TO the main app, so its value is pinned by the main app's and is not ours to change +# independently. +# πŸ”΄ DO NOT UNSET IT AS "migration cleanup". Two of those four callers (Meilisearch sync, KoNO +# finalize) only console.warn and skip, so unsetting fails SILENTLY β€” and inbound keeps working on +# MOD_INBOUND_TOKEN the whole time, so nothing loud tells you. WEBHOOK_TOKEN= -# Inbound-only service token β€” accepted on the same WebhookEndpoint routes as WEBHOOK_TOKEN, and -# equivalent to it inside this app, but it reaches NOTHING beyond this app: no outbound caller sends -# it and the main app does not know it. Hand this one to a service that only ever calls in (the -# abuse-detection cronjobs posting to /api/mod/abuse-report) so a leak there cannot touch the main -# app's admin surface. -# Either variable alone is a complete configuration; the endpoints answer 503 only while BOTH are unset. -# A variable set to an EMPTY value counts as unset, so blanking one cannot open the endpoints up. -# -# πŸ”΄ MIGRATING OFF WEBHOOK_TOKEN IS A CODE CHANGE, NOT AN UNSET. Removing it from `acceptedTokens()` -# in $lib/server/webhook-endpoint stops accepting it INBOUND and is safe. UNSETTING the variable also -# strips it from the four OUTBOUND callers above β€” and two of them (Meilisearch sync, KoNO finalize) -# only console.warn and skip, so that failure is silent. Inbound would keep working on -# MOD_INBOUND_TOKEN the whole time, so nothing loud tells you. +# The ONLY credential accepted inbound, on every endpoint wrapped in WebhookEndpoint +# ($lib/server/webhook-endpoint) β€” /api/mod/* and the XGuard agent API. Inbound-only: nothing in THIS +# app sends it outbound, and β€” the part that bounds a leak β€” the main app does not ACCEPT it, so a +# leak here cannot touch the main app's admin surface. (The main app does KNOW it: it declares +# MOD_INBOUND_TOKEN and presents it on delegated calls INTO this app β€” πŸ”΄ but only WHERE IT IS SET. +# It is optional in the main app's schema, and its `||` fallback selects the credential this app now +# refuses when it is unset, so a caller environment missing this key 401s. "Inbound-only" is a claim +# about what accepts it, never about who holds it.) There is no user behind it β€” nothing it reaches +# may attribute a write, and revoking access means rotating it for every holder. +# πŸ”΄ THIS IS NOW THE WHOLE INBOUND CONFIGURATION. Unset (or set EMPTY, which counts as unset) and the +# endpoints answer 503 β€” they do NOT fall back to WEBHOOK_TOKEN, which is what stops a blanked +# credential silently re-opening them to the platform-wide admin token. MOD_INBOUND_TOKEN= diff --git a/apps/moderator/src/__tests__/hooks.server.test.ts b/apps/moderator/src/__tests__/hooks.server.test.ts index e43cf81aec..039ac40027 100644 --- a/apps/moderator/src/__tests__/hooks.server.test.ts +++ b/apps/moderator/src/__tests__/hooks.server.test.ts @@ -8,10 +8,14 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; * the runtime signal the WEBHOOK_TOKEN removal is graded on (see the header of * `$lib/server/webhook-endpoint`), which makes these behavioural guards, not logging cosmetics: * - * - BOTH classes emit. A legacy-only emitter would make a zero unfalsifiable. - * - The record carries NOTHING derived from the token's bytes. - * - `locals.tokenClient` is still exactly `'webhook'`, for both classes. Three call sites compare - * that field strictly; widening it would 401 every token-authenticated request with no type error. + * - EVERY ACCEPTED class emits. A single-class emitter would make a zero unfalsifiable. πŸ”΄ With one + * accepted class the two-class version of this is DORMANT β€” see the DORMANT note in-file; what + * remains pinned is that the retired class emits NOTHING, which is what lets its count reach zero. + * - The record carries NOTHING derived from the token's bytes. πŸ”΄ Driven with the ACCEPTED secret: + * driven with the retired one it would inspect an EMPTY array and pass vacuously. + * - `locals.tokenClient` is still exactly `'webhook'` for the accepted class, and is left UNSET for + * a refused one. Three call sites compare that field strictly; widening it would 401 every + * token-authenticated request with no type error. * - A refused credential records nothing. * - A logging fault never becomes a failed request. * @@ -139,50 +143,43 @@ describe('inbound credential attribution', () => { ]); }); - it('records WEBHOOK_TOKEN for the legacy credential β€” the class the migration is waiting to stop seeing', async () => { + it('πŸ”΄ the RETIRED credential emits NOTHING β€” so a zero for it is a real zero, not a filtered one', async () => { + // The retired class must contribute no attribution records at all. If a refused credential still + // emitted, its count could never fall to zero and would read as ongoing use forever β€” the signal + // this whole mechanism exists to provide would be permanently stuck. `beforeEach` still SETS the + // variable (four outbound callers need it), so this is a statement about the accepted set. await run(eventFor({ query: LEGACY_SECRET })); - expect(emitted()).toEqual([ - { - type: 'info', - event: CREDENTIAL_ATTRIBUTION_EVENT, - credential: 'WEBHOOK_TOKEN', - path: PATH, - method: METHOD, - userAgent: USER_AGENT, - }, - ]); + expect(emitted()).toEqual([]); }); - it('πŸ”΄ BOTH classes emit from one deployment β€” the zero is only evidence beside a live control', async () => { - // The property this whole change exists to provide, and the one a "log only the legacy token" - // optimisation would silently remove. Both requests hit the SAME env state, so a non-zero - // MOD_INBOUND_TOKEN count is the in-band positive control that proves the emit path was live at - // the moment a WEBHOOK_TOKEN count read zero. Asserted as a SET of classes so it cannot be - // satisfied by two lines naming the same one. - await run(eventFor({ query: INBOUND_SECRET })); - await run(eventFor({ query: LEGACY_SECRET })); - expect(emitted().map((record) => record.credential)).toEqual([ - 'MOD_INBOUND_TOKEN', - 'WEBHOOK_TOKEN', - ]); - }); + // πŸ”΄ DORMANT: 'BOTH classes emit from one deployment β€” the zero is only evidence beside a live + // control'. It needs two ACCEPTED classes and cannot be written at one. The property it pinned is + // the reason the emit covers every accepted class rather than just a retiring one: a non-zero count + // for a live class is the in-band positive control proving the emit path was live at the moment a + // retiring class's count read zero. Restore it if a second class is added β€” asserted as a SET of + // classes, so it cannot be satisfied by two lines naming the same one. it('attributes a Bearer-presented credential the same as a query-presented one', async () => { - await run(eventFor({ authorization: `Bearer ${LEGACY_SECRET}` })); + await run(eventFor({ authorization: `Bearer ${INBOUND_SECRET}` })); expect(emitted()).toHaveLength(1); - expect(emitted()[0].credential).toBe('WEBHOOK_TOKEN'); + expect(emitted()[0].credential).toBe('MOD_INBOUND_TOKEN'); }); it('πŸ”΄ carries NOTHING derived from the token bytes, even when the token is in the URL', async () => { // `?token=` is how the main app calls in, so `url.href`/`url.search` would put the live secret on // a log stream far more readable than the secret store. `pathname` is what keeps it out. - await run(eventFor({ query: LEGACY_SECRET })); + // + // πŸ”΄ Driven with the ACCEPTED secret deliberately. Presenting the retired one emits nothing, so + // every assertion below would run against an EMPTY array and pass without testing anything β€” a + // vacuous green on the one test here that guards a secret. + await run(eventFor({ query: INBOUND_SECRET })); + expect(emitted()).toHaveLength(1); // the record exists, so the assertions below have a subject const serialized = JSON.stringify(emitted()); - expect(serialized).not.toContain(LEGACY_SECRET); + expect(serialized).not.toContain(INBOUND_SECRET); // …and no PREFIX of it either, which is what a "just the first few characters for correlation" // edit would add. Six characters past the shared `sk-` prefix is already an oracle. - expect(serialized).not.toContain(LEGACY_SECRET.slice(0, 9)); - expect(serialized).not.toContain(String(LEGACY_SECRET.length)); + expect(serialized).not.toContain(INBOUND_SECRET.slice(0, 9)); + expect(serialized).not.toContain(String(INBOUND_SECRET.length)); }); }); @@ -208,7 +205,7 @@ describe('what must NOT be recorded', () => { const { response, resolve } = await run(eventFor({ query: LEGACY_SECRET })); expect(response.status).toBe(503); await expect(response.json()).resolves.toEqual({ - message: 'Neither MOD_INBOUND_TOKEN nor WEBHOOK_TOKEN is configured on this deployment.', + message: 'MOD_INBOUND_TOKEN is not configured on this deployment.', }); expect(resolve).not.toHaveBeenCalled(); expect(logToAxiom).not.toHaveBeenCalled(); @@ -237,10 +234,13 @@ describe('locals.tokenClient stays exactly "webhook"', () => { expect(event.locals.tokenClient).toBe('webhook'); }); - it('for WEBHOOK_TOKEN', async () => { + it('and is NOT set for the retired credential, which never authenticates', async () => { + // The mirror of the case above. A refused credential must leave `tokenClient` unset, so nothing + // downstream can treat a retired token as a webhook caller β€” `WebhookEndpoint` and + // `defineWebhookEndpoint` gate on this field, and a stale `'webhook'` here would wave it through. const event = eventFor({ query: LEGACY_SECRET }); await run(event); - expect(event.locals.tokenClient).toBe('webhook'); + expect(event.locals.tokenClient).toBeUndefined(); }); it('and grants are emptied on token ingress, so nothing reached this way inherits a permission', async () => { diff --git a/apps/moderator/src/hooks.server.ts b/apps/moderator/src/hooks.server.ts index 6bf3684c7a..d6b0b10ca5 100644 --- a/apps/moderator/src/hooks.server.ts +++ b/apps/moderator/src/hooks.server.ts @@ -29,17 +29,21 @@ const PUBLIC_PATHS = new Set(['/favicon.svg']); export const CREDENTIAL_ATTRIBUTION_EVENT = 'webhook credential presented'; /** - * Records WHICH inbound service credential authenticated a request β€” the runtime signal that makes - * dropping WEBHOOK_TOKEN from `acceptedTokens` a checkable claim rather than an inferred one (see the - * header of $lib/server/webhook-endpoint). + * Records WHICH inbound service credential authenticated a request β€” the runtime signal that made + * dropping WEBHOOK_TOKEN from `acceptedTokens` a checkable claim rather than an inferred one, and that + * will grade the next such removal the same way (see the header of $lib/server/webhook-endpoint). * - * πŸ”΄ EMITTED FOR EVERY CLASS, NOT JUST THE LEGACY ONE. Logging only WEBHOOK_TOKEN looks like the - * obvious saving β€” it is the thing we are waiting to stop seeing β€” and it destroys the evidence. A - * zero from a legacy-only emitter is indistinguishable from an emitter that was never deployed, never - * ingested, or quietly broken, so it can never license the removal. With both classes emitted, a - * non-zero MOD_INBOUND_TOKEN count is the IN-BAND POSITIVE CONTROL standing beside the WEBHOOK_TOKEN - * zero in the same window and proving the instrument was live when it read zero. The pair is the - * evidence. Do not narrow this to the legacy class. + * πŸ”΄ EMITTED FOR EVERY ACCEPTED CLASS, NOT JUST A RETIRING ONE. Narrowing this to whichever class is + * on its way out looks like the obvious saving β€” it is the thing we are waiting to stop seeing β€” and + * it destroys the evidence. A zero from a single-class emitter is indistinguishable from an emitter + * that was never deployed, never ingested, or quietly broken, so it can never license a removal. With + * every class emitted, a non-zero count for a live class is the IN-BAND POSITIVE CONTROL standing + * beside the retiring class's zero in the same window, proving the instrument was live when it read + * zero. The pair is the evidence. Do not narrow this to one class. + * + * πŸ”΄ A REFUSED CREDENTIAL EMITS NOTHING, and that is deliberate rather than an omission. Recording + * refusals would mean a retired class's count could never fall to zero, so the signal that authorises + * the next removal would be permanently stuck at "still in use". Pinned in __tests__/hooks.server. * * πŸ”΄ NOTHING DERIVED FROM THE TOKEN'S BYTES GOES IN THE RECORD β€” no value, prefix, suffix, length or * hash. Any of those is a credential oracle on a log stream that is far more widely readable than the diff --git a/apps/moderator/src/lib/server/__tests__/webhook-endpoint-timing.test.ts b/apps/moderator/src/lib/server/__tests__/webhook-endpoint-timing.test.ts index b23ca6ce98..1303b5df51 100644 --- a/apps/moderator/src/lib/server/__tests__/webhook-endpoint-timing.test.ts +++ b/apps/moderator/src/lib/server/__tests__/webhook-endpoint-timing.test.ts @@ -12,11 +12,26 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; * the real `node:crypto` binding the module under test imports, not from a re-implementation: a model * of the loop written in the test file would go on passing after a `break` was added to production. * + * πŸ”΄ THE DISCRIMINATING CASE IS DORMANT, AND THIS FILE IS KEPT FOR THE DAY IT IS NOT. + * `ACCEPTED_CREDENTIALS` has ONE entry since the legacy class was dropped, and at length 1 the + * property is STRUCTURALLY UNOBSERVABLE: a loop with a `break` and a loop without both perform exactly + * one comparison, so no assertion written here can tell them apart. That is not the same as the + * property being unnecessary β€” the loop is still in production and still has no `break`, and the + * moment a second class is added (scoped tokens; see the module header) the leak it prevents is real + * again. The three cases that USED to discriminate are recorded in `DORMANT` below rather than + * deleted, because a guard nobody knows existed does not get rebuilt. + * + * πŸ”΄ SO DO NOT READ THIS FILE'S GREEN AS COVERAGE OF THE NO-EARLY-EXIT PROPERTY. What still executes + * is real and worth having β€” ALL FIVE live tests: the counter's own positive control, the + * length-mismatch invariant, the no-match control, and two REGRESSION cases pinning that the retired + * class is neither a candidate nor authenticatable (both go red if it is restored to the accepted + * set) β€” but NONE of them is a test of control flow. + * * πŸ”΄ EVERY FIXTURE SECRET BELOW IS THE SAME LENGTH. The loop checks length before calling * timingSafeEqual (that function throws on a length mismatch), so a shorter second secret would be - * skipped by the length check and never counted β€” the test would then read 1 comparison and blame a - * `break` that is not there. Equal lengths are what make the count a measurement of the loop rather - * than of the fixture. + * skipped by the length check and never counted β€” a count would then read low and blame a `break` + * that is not there. Equal lengths are what make the count a measurement of the loop rather than of + * the fixture. Keep that true of any secret added when this file is re-armed. * * This file mocks a module at load, so it is deliberately separate from `webhook-endpoint.test.ts` * rather than being a `describe` block with a local reset: a module mock is file-scoped, and reaching @@ -59,7 +74,13 @@ const saved = { MOD_INBOUND_TOKEN: process.env.MOD_INBOUND_TOKEN, }; -/** Every test owns its own reset, so no verdict here depends on the order the file runs in. */ +/** + * Every test owns its own reset, so no verdict here depends on the order the file runs in. + * + * LEGACY_SECRET is still injected even though it is no longer an accepted class β€” that is exactly + * what the removal regression below measures. A deployment that still sets the variable (and every + * one of them does, because four outbound callers need it) must not gain a second candidate. + */ beforeEach(() => { timingSafeEqualSpy.mockClear(); setEnv({ MOD_INBOUND_TOKEN: INBOUND_SECRET, WEBHOOK_TOKEN: LEGACY_SECRET }); @@ -68,41 +89,69 @@ afterEach(() => setEnv(saved)); describe('the token comparison loop', () => { it('POSITIVE CONTROL: the counter tracks candidates β€” one configured credential produces one comparison', () => { - // Without this, a `2` below would be indistinguishable from a spy that fires on something other - // than the loop. Watch the number MOVE with the number of configured credentials before reading - // any other count in this file as evidence about control flow. + // Without this, any count below would be indistinguishable from a spy that fires on something + // other than the loop. Establish that the number tracks configured credentials before reading any + // other count in this file as evidence. setEnv({ MOD_INBOUND_TOKEN: INBOUND_SECRET }); const result = authenticateWebhookToken(requestPresenting(INBOUND_SECRET)); expect(result).toEqual({ kind: 'authenticated', credential: 'MOD_INBOUND_TOKEN' }); expect(timingSafeEqualSpy).toHaveBeenCalledTimes(1); }); - it('REGRESSION: a match on the FIRST candidate STILL compares the second β€” no early exit', () => { - // The one observation that discriminates. An early exit answers 1 here and 2 in both controls - // below, so this case alone is what separates the two implementations. + it('REGRESSION: WEBHOOK_TOKEN set in the environment is NOT a candidate β€” the removal took effect at RUNTIME', () => { + // The guard for dropping the legacy class. `beforeEach` sets BOTH variables, which is the real + // deployment shape, so a removal that only edited the array while `acceptedTokens()` went on + // reading the variable would produce TWO comparisons here. One is the assertion that the class is + // genuinely gone from the accepted set rather than merely absent from a list. const result = authenticateWebhookToken(requestPresenting(INBOUND_SECRET)); expect(result).toEqual({ kind: 'authenticated', credential: 'MOD_INBOUND_TOKEN' }); - expect(timingSafeEqualSpy).toHaveBeenCalledTimes(2); + expect(timingSafeEqualSpy).toHaveBeenCalledTimes(1); }); - it('CONTROL: a match on the LAST candidate compares both β€” an early-exit loop answers 2 here too', () => { + it('REGRESSION: the legacy secret is now REFUSED, not merely unattributed', () => { + // Presenting the retired credential must fail closed. Asserted on the verdict rather than the + // count, because a count alone cannot tell "compared and rejected" from "never a candidate". const result = authenticateWebhookToken(requestPresenting(LEGACY_SECRET)); - expect(result).toEqual({ kind: 'authenticated', credential: 'WEBHOOK_TOKEN' }); - expect(timingSafeEqualSpy).toHaveBeenCalledTimes(2); + expect(result.kind).toBe('refused'); }); - it('CONTROL: no match compares both β€” an early-exit loop answers 2 here too', () => { + it('CONTROL: no match still compares every candidate', () => { const result = authenticateWebhookToken(requestPresenting('wrong-secret-value-cccccc')); expect(result.kind).toBe('refused'); - expect(timingSafeEqualSpy).toHaveBeenCalledTimes(2); + expect(timingSafeEqualSpy).toHaveBeenCalledTimes(1); }); it('INVARIANT: a length mismatch is never handed to timingSafeEqual, which throws on one', () => { - // The length guard is what keeps a short token a 401 rather than a 500 out of the hook. Both - // configured secrets are longer than this, so a comparison count of 0 is the assertion that the - // guard ran ahead of the call for BOTH candidates. + // The length guard is what keeps a short token a 401 rather than a 500 out of the hook. The + // configured secret is longer than this, so a comparison count of 0 asserts the guard ran ahead + // of the call. const result = authenticateWebhookToken(requestPresenting('short')); expect(result.kind).toBe('refused'); expect(timingSafeEqualSpy).toHaveBeenCalledTimes(0); }); }); + +/* + * DORMANT β€” the three cases that discriminated a `break` from no `break`. They are recorded rather + * than deleted because at ONE accepted credential they cannot be written truthfully (see the header), + * and a guard nobody knows existed does not get rebuilt. They are NOT expressed as skipped tests on + * purpose: a `describe.skip` reports green-ish and reads as coverage, which is the thing this block + * exists to avoid claiming. + * + * TO RE-ARM, when ACCEPTED_CREDENTIALS gains a second entry: set both variables to same-LENGTH, + * different-byte secrets (the header explains why equal length is load-bearing) and restore + * + * 'a match on the FIRST candidate STILL compares the second β€” no early exit' + * β†’ authenticate with the FIRST secret; expect 2 comparisons. + * This is the only case that discriminates: an early exit answers 1 here and 2 in both + * controls below, so the other two are worthless without it. + * + * 'a match on the LAST candidate compares both β€” an early-exit loop answers 2 here too' + * β†’ authenticate with the SECOND secret; expect 2 comparisons. + * + * 'no match compares both β€” an early-exit loop answers 2 here too' + * β†’ present a wrong secret of equal length; expect 2 comparisons. + * + * The production invariant they pin is in `webhook-endpoint.ts`: the match loop records which + * candidate matched with a single assignment, NO `break` and NO `else`. + */ diff --git a/apps/moderator/src/lib/server/__tests__/webhook-endpoint.test.ts b/apps/moderator/src/lib/server/__tests__/webhook-endpoint.test.ts index 87bfb45ea6..9adc469abc 100644 --- a/apps/moderator/src/lib/server/__tests__/webhook-endpoint.test.ts +++ b/apps/moderator/src/lib/server/__tests__/webhook-endpoint.test.ts @@ -18,9 +18,16 @@ import { * REGRESSION β€” red before the change, green after: the cases that pin WHICH credential matched, and * the tagged-union verdict shape that carries it. * INVARIANT β€” green on BOTH sides once the union shape is accounted for: the accept/refuse/503 - * decisions themselves, which this change must not move. They are here because the change's whole - * point is that they must KEEP passing β€” the main app presents WEBHOOK_TOKEN on every call into - * this app, so recording which credential matched must not stop either one from matching. + * decisions themselves, which a change to WHICH credentials are accepted must not move. The + * accept/refuse boundary moved deliberately when the legacy class was dropped; what these pin is + * that the SHAPE of the decision did not β€” 503 still means "nothing configured" and 401 still + * means "a credential was presented and refused", and the two never trade places. + * + * πŸ”΄ A TEST THAT PINS A STRING THE CHANGE EDITS IS A REGRESSION, WHATEVER IT IS ABOUT. The 503-body + * case reads like an invariant β€” it is about the fail-closed path, which did not move β€” but it + * asserts the WHOLE message, and dropping a credential class rewrites that message. It was labelled + * INVARIANT and is red on the pre-change side, which made the matrix claim above false. Relabelled + * REGRESSION. Sort by what the MATRIX does, never by what the test is thematically about. * * `$env/dynamic/private` is aliased to `src/test/env.mock.ts`, which is `process.env` itself, and * `acceptedTokens()` re-reads it per call β€” so assigning here really does change what the function @@ -74,18 +81,17 @@ describe('authenticateWebhookToken', () => { expect(authenticateWebhookToken(eventWith({}))).toEqual({ kind: 'none' }); }); - it('REGRESSION: attributes WEBHOOK_TOKEN via ?token= β€” the main app calls in this way', () => { - expect(authenticateWebhookToken(eventWith({ query: LEGACY }))).toEqual({ - kind: 'authenticated', - credential: 'WEBHOOK_TOKEN', - }); + it('REGRESSION: the retired WEBHOOK_TOKEN is REFUSED via ?token= β€” the shape the main app used to call in', () => { + // `beforeEach` still SETS the variable, because four outbound callers need it and every real + // deployment has it. Refusing here is therefore a statement about the accepted SET, not about the + // variable being absent β€” which is the whole distinction this removal turns on. + expect(refusalOf(authenticateWebhookToken(eventWith({ query: LEGACY }))).status).toBe(401); }); - it('REGRESSION: attributes WEBHOOK_TOKEN via Authorization: Bearer', () => { - expect(authenticateWebhookToken(eventWith({ authorization: `Bearer ${LEGACY}` }))).toEqual({ - kind: 'authenticated', - credential: 'WEBHOOK_TOKEN', - }); + it('REGRESSION: the retired WEBHOOK_TOKEN is REFUSED via Authorization: Bearer', () => { + expect( + refusalOf(authenticateWebhookToken(eventWith({ authorization: `Bearer ${LEGACY}` }))).status + ).toBe(401); }); it('REGRESSION: attributes MOD_INBOUND_TOKEN via ?token=', () => { @@ -102,14 +108,19 @@ describe('authenticateWebhookToken', () => { }); }); - it('REGRESSION: the two classes are attributed DIFFERENTLY from the same deployment', () => { - // The pairwise-distinct check the whole signal rests on: a stub returning one constant for every - // caller satisfies either test above on its own, and fails this one. Both verdicts are read from - // the SAME env state, so nothing but the presented bytes can be deciding. - const legacy = authenticateWebhookToken(eventWith({ query: LEGACY })); - const inbound = authenticateWebhookToken(eventWith({ query: INBOUND })); - expect(legacy).toEqual({ kind: 'authenticated', credential: 'WEBHOOK_TOKEN' }); - expect(inbound).toEqual({ kind: 'authenticated', credential: 'MOD_INBOUND_TOKEN' }); + it('REGRESSION: the verdict is decided by the PRESENTED BYTES, not by the request shape', () => { + // What survives of the old pairwise-distinct check now that there is one class to attribute. A + // stub that returned `authenticated` for every caller would satisfy the two tests above on their + // own and fails this one. Both verdicts are read from the SAME env state, so nothing but the + // presented bytes can be deciding. + // + // πŸ”΄ The stronger form β€” two classes attributed DIFFERENTLY from one deployment β€” is dormant at + // one accepted credential, for the same reason the no-early-exit guard is (see + // webhook-endpoint-timing.test.ts). Restore it if a second class is added. + const accepted = authenticateWebhookToken(eventWith({ query: INBOUND })); + const retired = authenticateWebhookToken(eventWith({ query: LEGACY })); + expect(accepted).toEqual({ kind: 'authenticated', credential: 'MOD_INBOUND_TOKEN' }); + expect(retired.kind).toBe('refused'); }); it('REGRESSION: MOD_INBOUND_TOKEN alone is a complete configuration β€” the post-migration state', () => { @@ -120,15 +131,19 @@ describe('authenticateWebhookToken', () => { }); }); - it('REGRESSION: WEBHOOK_TOKEN alone is a complete configuration β€” the pre-migration state', () => { + it('REGRESSION: WEBHOOK_TOKEN alone is NO LONGER a configuration β€” it fails CLOSED with 503', () => { + // The inverse of the test this replaces, and the sharpest statement of what the removal did. A + // deployment carrying ONLY the retired variable now has no accepted class at all, so every + // wrapped endpoint is unreachable (503, a deployment problem) rather than reachable on a + // platform-wide admin credential. 503 and not 401: the caller is not at fault. setEnv({ WEBHOOK_TOKEN: LEGACY }); - expect(authenticateWebhookToken(eventWith({ query: LEGACY }))).toEqual({ - kind: 'authenticated', - credential: 'WEBHOOK_TOKEN', - }); + expect(refusalOf(authenticateWebhookToken(eventWith({ query: LEGACY }))).status).toBe(503); }); - it('INVARIANT: the two tokens are independent β€” presenting one does not depend on the other being set', () => { + it('INVARIANT: with the retired variable UNSET, its secret is still refused β€” 401, not 503', () => { + // Distinct from the 503 case above, and the pair is the point: 503 says "this deployment accepts + // nothing", 401 says "something IS accepted and you did not present it". Here MOD_INBOUND_TOKEN + // is configured, so presenting the retired secret must be the caller's fault, not the config's. setEnv({ MOD_INBOUND_TOKEN: INBOUND }); expect(refusalOf(authenticateWebhookToken(eventWith({ query: LEGACY }))).status).toBe(401); }); @@ -154,20 +169,14 @@ describe('authenticateWebhookToken', () => { ); }); - it('REGRESSION: both variables set to the SAME value attribute to the LEGACY class, not the preferred one', () => { - // πŸ”΄ The ambiguous case, and the direction of the bias is the point. One shared value is - // indistinguishable on the wire, so attribution has to pick; picking the most legacy class can - // only OVERSTATE legacy use, and a migration proof that errs toward "still in use" is safe while - // one that errs toward zero authorises the removal that 401s every delegated moderation action. - // - // This also pins the no-`else`/last-match-wins shape of the match loop against the tidier-looking - // first-match-wins rewrite, which would report the post-migration class here. - setEnv({ WEBHOOK_TOKEN: LEGACY, MOD_INBOUND_TOKEN: LEGACY }); - expect(authenticateWebhookToken(eventWith({ query: LEGACY }))).toEqual({ - kind: 'authenticated', - credential: 'WEBHOOK_TOKEN', - }); - }); + // πŸ”΄ DORMANT: 'both variables set to the SAME value attribute to the LEGACY class'. The ambiguous + // case needs TWO accepted classes to exist, so it cannot be written today. It pinned the + // last-match-wins shape of the match loop against a tidier-looking first-match-wins rewrite, and the + // direction of that bias was the point: one shared value is indistinguishable on the wire, so + // attribution has to pick, and picking the most LEGACY class can only overstate legacy use β€” a + // migration proof erring toward "still in use" is safe, one erring toward zero authorises a removal + // that 401s every delegated moderation action. The property is recorded on ACCEPTED_CREDENTIALS in + // webhook-endpoint.ts; restore this test if a second class is added. it('REGRESSION: EVERY accepted class is reachable β€” no class can be listed but never attributable', () => { // The ledger guard. A class added to ACCEPTED_CREDENTIALS but not read in `acceptedTokens` would @@ -192,7 +201,10 @@ describe('authenticateWebhookToken', () => { // `.trim()`ed to ZERO length, and timingSafeEqual(, ) is TRUE β€” so a request // presenting `?token=` with no value authenticated and every wrapped endpoint was open. Latent (a // real deployment holds a real value) but real, and pinned here so it stays shut. - setEnv({ WEBHOOK_TOKEN: ' ' }); + // πŸ”΄ Pinned on the ACCEPTED class. Setting the retired variable here instead would make this test + // pass for the WRONG REASON β€” a 503 because nothing is accepted at all, rather than because the + // whitespace-only value was filtered β€” and the bypass this closed would be untested. + setEnv({ MOD_INBOUND_TOKEN: ' ' }); expect(refusalOf(authenticateWebhookToken(eventWith({ query: '' }))).status).toBe(503); }); @@ -201,14 +213,14 @@ describe('authenticateWebhookToken', () => { expect(refusalOf(authenticateWebhookToken(eventWith({ query: LEGACY }))).status).toBe(503); }); - it('INVARIANT: the 503 names EVERY accepted class, so an operator knows any one would fix it', async () => { + it('REGRESSION: the 503 names EVERY accepted class, so an operator knows any one would fix it', async () => { setEnv({}); const body = await refusalOf(authenticateWebhookToken(eventWith({ query: LEGACY }))).json(); // The whole normalised sentence, not a substring: a body that names only one class still contains // that class's name, so a `stringContaining` guard passes on exactly the message that would send // an operator to set the wrong variable. expect(body).toEqual({ - message: 'Neither MOD_INBOUND_TOKEN nor WEBHOOK_TOKEN is configured on this deployment.', + message: 'MOD_INBOUND_TOKEN is not configured on this deployment.', }); }); @@ -219,22 +231,15 @@ describe('authenticateWebhookToken', () => { expect(refusalOf(authenticateWebhookToken(eventWith({ query: '' }))).status).toBe(503); }); - it('INVARIANT: a blank MOD_INBOUND_TOKEN does not become a usable credential alongside a real one', () => { + it('REGRESSION: a blank MOD_INBOUND_TOKEN does NOT fall back to a set WEBHOOK_TOKEN', () => { + // πŸ”΄ The failure mode with the worst blast radius, and the reason this is a REGRESSION rather than + // an invariant: if the removal were half-done β€” the class dropped from the list but still read as + // a fallback β€” blanking the narrow credential would silently re-open every wrapped endpoint to the + // platform-wide admin token. Both assertions are 503 (nothing accepted), never 401 (a bad token + // against something that IS accepted); the two statuses are what tell those states apart. setEnv({ WEBHOOK_TOKEN: LEGACY, MOD_INBOUND_TOKEN: '' }); - expect(refusalOf(authenticateWebhookToken(eventWith({ query: '' }))).status).toBe(401); - // …and the real one still works, still attributed to the class it came from. - expect(authenticateWebhookToken(eventWith({ query: LEGACY }))).toEqual({ - kind: 'authenticated', - credential: 'WEBHOOK_TOKEN', - }); - }); - - it('REGRESSION: a WEBHOOK_TOKEN injected with surrounding whitespace still matches, and is still attributed', () => { - setEnv({ WEBHOOK_TOKEN: ` ${LEGACY}\n` }); - expect(authenticateWebhookToken(eventWith({ query: LEGACY }))).toEqual({ - kind: 'authenticated', - credential: 'WEBHOOK_TOKEN', - }); + expect(refusalOf(authenticateWebhookToken(eventWith({ query: '' }))).status).toBe(503); + expect(refusalOf(authenticateWebhookToken(eventWith({ query: LEGACY }))).status).toBe(503); }); it('REGRESSION: the same whitespace tolerance applies to MOD_INBOUND_TOKEN', () => { diff --git a/apps/moderator/src/lib/server/api-catalog.ts b/apps/moderator/src/lib/server/api-catalog.ts index f1d39d8903..8389722681 100644 --- a/apps/moderator/src/lib/server/api-catalog.ts +++ b/apps/moderator/src/lib/server/api-catalog.ts @@ -39,8 +39,10 @@ function specOf(mod: EndpointModule, method: string): EndpointSpec | undefined { function visibleTo(user: SessionUser, auth: EndpointAuth | null): boolean { // A legacy route carries no readable auth, so listing it would be a claim we cannot support. if (!auth) return false; - // WEBHOOK_TOKEN is a deployment secret, not a per-moderator grant: any moderator may read that these - // endpoints exist, and none of them can call one from a browser anyway. + // A webhook credential (MOD_INBOUND_TOKEN) is a deployment secret, not a per-moderator grant: any + // moderator may read that these endpoints exist, and none of them can call one from a browser + // anyway. Named generically on purpose β€” this branch keys on `auth.kind`, not on which variable is + // accepted, so it stays correct if the accepted set changes again. if (auth.kind === 'webhook') return true; return canAccess(user, auth.page); } diff --git a/apps/moderator/src/lib/server/webhook-endpoint.ts b/apps/moderator/src/lib/server/webhook-endpoint.ts index d678caa8b1..eef2645d38 100644 --- a/apps/moderator/src/lib/server/webhook-endpoint.ts +++ b/apps/moderator/src/lib/server/webhook-endpoint.ts @@ -19,38 +19,35 @@ import { env } from '$env/dynamic/private'; // There is NO USER behind the token. `locals.user` is deliberately never populated here, so anything // reached this way cannot attribute a write β€” which is what keeps `human_judgement` human-only. // -// TWO CREDENTIALS ARE ACCEPTED, and the difference is reach, not privilege β€” inside this app they -// authorise exactly the same thing: +// ONE CREDENTIAL IS ACCEPTED INBOUND: MOD_INBOUND_TOKEN. It is inbound-only β€” this app never presents +// it outbound β€” which is what lets a service that only calls IN hold something narrower than the +// shared token it used to need. // -// MOD_INBOUND_TOKEN β€” inbound-only, and the one to prefer for anything that only calls IN. -// WEBHOOK_TOKEN β€” accepted for COMPATIBILITY. This app is on both ends of it, so its value is -// shared rather than local (see .env.example) and a second accepted token, not a rotation, is -// what lets an inbound-only caller hold something narrower. +// πŸ”΄ WEBHOOK_TOKEN IS NO LONGER ACCEPTED INBOUND, AND IS STILL REQUIRED. Those are two different +// facts and collapsing them breaks this app. It was dropped from `acceptedTokens` once the runtime +// signal below showed no caller presenting it inbound. It remains SET, and four services here present +// it OUTBOUND to the main app (kono.ts, search-index.ts, training-moderation.service.ts, +// user-actions.service.ts) β€” two of which degrade by WARNING rather than failing, so unsetting the +// variable goes QUIET rather than loud. Do not "finish the migration" by removing it. // -// So this is a MIGRATION SEAM, not a permission model. Move each inbound caller to MOD_INBOUND_TOKEN, -// and once none present WEBHOOK_TOKEN inbound, drop it from `acceptedTokens`. +// πŸ”΄ HOW THAT REMOVAL WAS GRADED β€” recorded because the same standard applies to the next one, and +// because the obvious shortcut is wrong. Every token-authenticated request logs which credential class +// matched (`webhook credential presented`, emitted from hooks.server.ts). The verdict needs BOTH +// numbers over one window: a ZERO for the retiring class is evidence only beside a NON-ZERO count for +// a live one in the SAME window, which is the in-band positive control proving the emit path was live +// and ingesting at the moment the zero was read. Without the pair, "nobody presents it any more" and +// "the log line never shipped, or stopped being ingested" are the identical observation. That is why +// the emit covers EVERY class rather than just a retiring one. // -// πŸ”΄ TWO WAYS TO GET THAT REMOVAL WRONG, and neither is visible from this file alone: -// 1. "None present it inbound" is a claim about THE MAIN APP, not about this repo. The main app -// presents WEBHOOK_TOKEN on every call into this app, from its own codebase β€” so grepping HERE, -// finding no inbound presenter and concluding the migration is done 401s every delegated -// moderation action. The RUNTIME SIGNAL that settles it now exists: every token-authenticated -// request logs which credential class matched (`webhook credential presented`, emitted from -// hooks.server.ts), so the claim is checkable instead of inferred. -// 2. Dropping it from `acceptedTokens` is NOT the same as unsetting the variable. Four services in -// this app present it OUTBOUND, and two of them degrade by WARNING rather than failing. -// -// πŸ”΄ HOW TO READ THAT SIGNAL β€” the ZERO ALONE IS NOT THE EVIDENCE. Count the attribution lines grouped -// by `credential` over a window long enough to cover every inbound caller's slowest schedule. The -// verdict needs BOTH numbers: `WEBHOOK_TOKEN` at zero is only meaningful next to a NON-ZERO -// `MOD_INBOUND_TOKEN` in the same window, which is the in-band positive control proving the emit path -// was live and ingesting at the moment the legacy count read zero. Without the pair, "nobody presents -// it any more" and "the log line never shipped, or stopped being ingested" produce the identical -// observation. That is why the emit covers BOTH classes and not just the legacy one. +// πŸ”΄ AND THE WINDOW MUST HAVE EXERCISED THE CALLERS. A window in which some inbound path never ran +// carries no evidence about that path β€” no calls and no legacy credential look the same from the +// count. The removal waited for a window in which every inbound caller had actually called, including +// the main app's own delegated moderation actions, which are human-driven and can be quiet for hours. // // Scoping a token to particular endpoints is a separate, later change: `EndpointAuth` is already // `{kind:'webhook'} | {kind:'session'; page}` (api-endpoint.ts), so `{kind:'webhook'; scope}` has -// somewhere to go. +// somewhere to go. πŸ”΄ If that ever adds a SECOND accepted class, re-arm the no-early-exit guard in +// `__tests__/webhook-endpoint-timing.test.ts` β€” it is DORMANT at one credential and says so in place. // // Operational rationale for the split lives in the private infra repo, not here. @@ -73,15 +70,19 @@ function presentedToken(event: { url: URL; request: Request }): Buffer { } /** - * The credential VARIABLE NAMES this app can accept inbound, in PREFERENCE-DESCENDING order β€” most - * preferred first, most legacy last. The order is load-bearing twice over: it decides which name the - * attribution record carries when a deployment sets both variables to the same value (see the match - * loop below), and it is the order the 503 body lists them in. + * The credential VARIABLE NAMES this app accepts inbound, in PREFERENCE-DESCENDING order β€” most + * preferred first, most legacy last. + * + * πŸ”΄ ONE ENTRY TODAY, and two properties of this list are dormant rather than gone. Keep the ordering + * rule if a second class is ever added: it decides which name the attribution record carries when a + * deployment sets two variables to the SAME value (the match loop below is last-match-wins, so an + * ambiguous deployment attributes to the most LEGACY class β€” which can only ever overstate legacy use, + * never understate it), and it is the order the 503 body lists them in. * * A credential class is a variable NAME, never a value. Nothing derived from a token's bytes β€” not a * prefix, a length, or a hash β€” may leave this module. */ -export const ACCEPTED_CREDENTIALS = ['MOD_INBOUND_TOKEN', 'WEBHOOK_TOKEN'] as const; +export const ACCEPTED_CREDENTIALS = ['MOD_INBOUND_TOKEN'] as const; export type AcceptedCredential = (typeof ACCEPTED_CREDENTIALS)[number]; @@ -108,7 +109,6 @@ function acceptedTokens(): AcceptedToken[] { // and forgetting to read it here is a type error, not a credential that silently never matches. const values: Record = { MOD_INBOUND_TOKEN: env.MOD_INBOUND_TOKEN, - WEBHOOK_TOKEN: env.WEBHOOK_TOKEN, }; return ACCEPTED_CREDENTIALS.map((credential) => ({ credential, @@ -143,17 +143,22 @@ export function authenticateWebhookToken(event: { url: URL; request: Request }): const accepted = acceptedTokens(); // Fails CLOSED β€” with NO secret configured every wrapped endpoint is unreachable rather than // unguarded. 503 rather than 401 so an operator reading logs sees a deployment problem, not a caller - // with a bad token. Either variable alone is a complete configuration: WEBHOOK_TOKEN alone is the - // state before migration, MOD_INBOUND_TOKEN alone the state after. The body names every accepted - // class, derived from the list itself so the two cannot drift. + // with a bad token. MOD_INBOUND_TOKEN is now the ONLY accepted inbound class, so it is also the only + // complete configuration; the legacy class was dropped once the runtime signal showed no caller + // presenting it (see the header). The body names every accepted class, derived from the list itself + // so the two cannot drift. if (accepted.length === 0) { + // Phrased for EITHER list length: "Neither A nor B" is ungrammatical with one name, and this list + // is one name today. Keeping it derived rather than hardcoded is what stops the body drifting from + // the accepted set when a second class is added back. const names = ACCEPTED_CREDENTIALS.join(' nor '); + const message = + ACCEPTED_CREDENTIALS.length > 1 + ? `Neither ${names} is configured on this deployment.` + : `${names} is not configured on this deployment.`; return { kind: 'refused', - response: Response.json( - { message: `Neither ${names} is configured on this deployment.` }, - { status: 503 } - ), + response: Response.json({ message }, { status: 503 }), }; } diff --git a/apps/moderator/src/routes/xguard/docs/+page.svelte b/apps/moderator/src/routes/xguard/docs/+page.svelte index cd872da8db..0709e478da 100644 --- a/apps/moderator/src/routes/xguard/docs/+page.svelte +++ b/apps/moderator/src/routes/xguard/docs/+page.svelte @@ -57,9 +57,11 @@
  • Ask a moderator admin for MOD_INBOUND_TOKEN. It is a service secret, not a per-person credential β€” but it is inbound-only: - this app accepts it and nothing else does, so it cannot be used anywhere but here. Ask for - this one. WEBHOOK_TOKEN is also accepted for - compatibility and reaches further than this app, so it is the wrong thing to hand out. + this app accepts it and nothing else does, so it cannot be used anywhere but here. It is now + the only credential these endpoints accept. + WEBHOOK_TOKEN used to be accepted here as well and + no longer is β€” it reaches far beyond this app, which is why it was dropped. + If a caller of yours started returning 401, that is the reason: it needs the token above.
  • Send it as ?token= or diff --git a/apps/moderator/src/routes/xguard/docs/__tests__/credential-handed-out.test.ts b/apps/moderator/src/routes/xguard/docs/__tests__/credential-handed-out.test.ts index 86214a8489..8fa359f169 100644 --- a/apps/moderator/src/routes/xguard/docs/__tests__/credential-handed-out.test.ts +++ b/apps/moderator/src/routes/xguard/docs/__tests__/credential-handed-out.test.ts @@ -7,13 +7,15 @@ import { describe, expect, it } from 'vitest'; * WHICH credential the XGuard docs tell an operator to ask for. * * πŸ”΄ This page is the human distribution channel for a service token, so what it names is what - * people end up holding. It told operators to ask for `WEBHOOK_TOKEN` β€” which this app accepts, so - * nothing was broken and nothing failed β€” but that token is shared with the main app, while - * `MOD_INBOUND_TOKEN` is accepted here and nowhere else. XGuard's API is an inbound-only consumer, - * so every operator onboarded through this page was handed more reach than the job needs. + * people end up holding. It once told operators to ask for `WEBHOOK_TOKEN` β€” which this app accepted + * AT THE TIME, so nothing was broken and nothing failed β€” but that token is shared with the main app, + * while `MOD_INBOUND_TOKEN` is accepted here and nowhere else. XGuard's API is an inbound-only + * consumer, so every operator onboarded through this page was handed more reach than the job needed. + * πŸ”΄ PAST TENSE THROUGHOUT, deliberately: this app NO LONGER accepts `WEBHOOK_TOKEN` at all β€” it was + * dropped from `acceptedTokens()`, so an operator still holding it now gets a 401. * - * πŸ”΄ DELIBERATELY NOT `expect(page).not.toMatch(/WEBHOOK_TOKEN/)`. The page names the legacy token - * on purpose, to say why it is the wrong one to hand out β€” a blanket ban on the word would fail on + * πŸ”΄ DELIBERATELY NOT `expect(page).not.toMatch(/WEBHOOK_TOKEN/)`. The page names the retired token + * on purpose, to say that it is refused and why β€” a blanket ban on the word would fail on * the explanation and push a future editor to delete the very sentence that prevents the mistake. * (That exact trap cost two rounds elsewhere in this app: a guard forbidding a string failed on the * comment explaining the string.) Assert the INSTRUCTION and the COPY-PASTEABLE EXAMPLE β€” the two @@ -41,15 +43,33 @@ describe('the XGuard docs hand out the inbound-only token', () => { it('the lab README points the same way, and says why', () => { // Two surfaces, one instruction: an operator reaches either. The "why" is what stops the next - // editor "simplifying" back to the shared token because both work. - expect(readme).toMatch(/Hand out `MOD_INBOUND_TOKEN`, not `WEBHOOK_TOKEN`/); + // editor "simplifying" back to the shared token. + // + // πŸ”΄ The old phrasing pinned here was `Hand out MOD_INBOUND_TOKEN, not WEBHOOK_TOKEN` β€” a + // CONTRAST that only made sense while both were accepted. Once the legacy class was dropped the + // README had to state it outright, so this asserts the current claim rather than the old one. + expect(readme).toMatch(/Hand out `MOD_INBOUND_TOKEN`/); + expect(readme, 'it must say this is the ONLY accepted credential').toMatch( + /only credential these endpoints accept/ + ); expect(readme, 'the reason must survive, not just the name').toMatch(/inbound-only/); + expect(readme, 'and it must say the legacy one is retired, not merely discouraged').toMatch( + /no longer is/ + ); }); - it('the page still explains that the legacy token also works', () => { - // The positive half of the rule above. Deleting this explanation would make the page read as - // "only MOD_INBOUND_TOKEN is accepted", which is false while compatibility lasts and would send - // anyone debugging a working legacy caller down the wrong path. + it('the page explains that the legacy token is NO LONGER accepted, and why', () => { + // The positive half of the rule above, INVERTED when the legacy class was dropped from + // `acceptedTokens()`. Until then the page said the legacy token "also works" and this test + // pinned that sentence; keeping it would have enforced a claim the code contradicts and blocked + // the correction. + // + // πŸ”΄ Still asserted POSITIVELY rather than as a ban on the word. The page names the legacy token + // on purpose β€” an operator whose caller just started 401ing needs to find the reason here β€” so a + // `not.toMatch(/WEBHOOK_TOKEN/)` guard would punish the explanation that prevents the support + // ticket. What must survive is the STATEMENT, so that is what is pinned. expect(page).toMatch(/WEBHOOK_TOKEN/); + expect(page, 'the page must say it is no longer accepted').toMatch(/no longer is/); + expect(page, 'and must name the 401 an operator would actually see').toMatch(/401/); }); }); diff --git a/apps/moderator/xguard-lab/README.md b/apps/moderator/xguard-lab/README.md index 4f2040f90b..1d44ae7397 100644 --- a/apps/moderator/xguard-lab/README.md +++ b/apps/moderator/xguard-lab/README.md @@ -40,12 +40,16 @@ Everything above except reviewing is also an HTTP endpoint under `/api/xguard/*` There is no user behind it: calls are not attributed to anybody, and revoking access means rotating the token for everyone holding it. -πŸ”΄ **Hand out `MOD_INBOUND_TOKEN`, not `WEBHOOK_TOKEN`.** Both are accepted β€” `acceptedTokens()` in -`$lib/server/webhook-endpoint` returns both β€” but only the first is inbound-only: nothing outside -this app accepts it, and nothing in this app sends it anywhere. `WEBHOOK_TOKEN` is shared with the -main app and is kept accepted for compatibility while callers migrate. Since revocation here means -rotating for every holder, which token an agent operator holds decides how much a rotation costs and -how far a leak reaches. +πŸ”΄ **Hand out `MOD_INBOUND_TOKEN`. It is the only credential these endpoints accept.** +`acceptedTokens()` in `$lib/server/webhook-endpoint` returns it and nothing else. It is +inbound-only: nothing outside this app accepts it, and nothing in this app sends it anywhere. +`WEBHOOK_TOKEN` **used to be accepted here and no longer is** β€” it is shared with the main app and +reaches far beyond this one, which is why it was dropped once no caller still presented it inbound. +A caller that _starts_ returning 401 against `/api/xguard/*` is most likely presenting the retired +token β€” check that first, but it is not the only cause: a rotated or mistyped secret, a wrong token, +and a non-Bearer `Authorization` scheme all 401 the same way. Since +revocation here means rotating for every holder, which token an agent operator holds decides how +much a rotation costs and how far a leak reaches. `/xguard/docs` is the operator guide, and its endpoint list is generated from the routes rather than written down. Read it there; a copy here would be the version that goes stale. diff --git a/packages/civitai-moderation/src/client.ts b/packages/civitai-moderation/src/client.ts index 7a4cfad068..2bbca2b5cc 100644 --- a/packages/civitai-moderation/src/client.ts +++ b/packages/civitai-moderation/src/client.ts @@ -9,8 +9,11 @@ export type ModeratorClientConfig = { /** Base URL of the moderator spoke app, e.g. `https://moderator.civitai.com`. Falls back to * `process.env.MODERATOR_APP_URL`. */ endpoint?: string; - /** Shared internal secret for the `/api/mod/*` ingress (the same WEBHOOK_TOKEN syncSearchIndex uses). - * Falls back to `process.env.WEBHOOK_TOKEN`. */ + /** Credential for the `/api/mod/*` ingress. πŸ”΄ PASS THIS EXPLICITLY. The spoke accepts only + * `MOD_INBOUND_TOKEN`; `WEBHOOK_TOKEN` was dropped from its accepted set, so the + * `process.env.WEBHOOK_TOKEN` fallback below now yields a credential the server REFUSES (401). + * The fallback is kept only so an existing caller does not change shape on this commit β€” it is + * not a working default, and a new integrator should not rely on it. */ token?: string; /** Override fetch (tests / non-global-fetch runtimes). */ fetch?: typeof fetch; diff --git a/src/server/services/__tests__/moderator-app-token.test.ts b/src/server/services/__tests__/moderator-app-token.test.ts index a944865403..e4de9f6373 100644 --- a/src/server/services/__tests__/moderator-app-token.test.ts +++ b/src/server/services/__tests__/moderator-app-token.test.ts @@ -8,11 +8,19 @@ import { serverSchema } from '~/env/server-schema'; * `moderator-app.service` builds the ONE client the main app uses to call the moderator spoke * (`apps/moderator`). Which credential it presents is the whole subject of this file. * - * The spoke is dropping the platform-wide `WEBHOOK_TOKEN` from its `acceptedTokens()`, so this - * caller has to start presenting the narrow `MOD_INBOUND_TOKEN`. The `||` fallback is what makes - * the two repos' deploy ORDER irrelevant, and it is the thing most likely to be "tidied away" by - * someone who reads it as a redundant default β€” so both arms are pinned here, plus the boundary - * case that decides which arm an EMPTY value takes. + * The spoke HAS DROPPED the platform-wide `WEBHOOK_TOKEN` from its `acceptedTokens()`, so this + * caller must present the narrow `MOD_INBOUND_TOKEN`. + * + * πŸ”΄ THE `||` FALLBACK NO LONGER MAKES DEPLOY ORDER IRRELEVANT, AND THAT IS THE POINT TO KNOW BEFORE + * TOUCHING IT. `||` falls through on an EMPTY local value, never on a REJECTION β€” so its legacy arm + * is now a guaranteed 401 rather than a working bridge, and an environment that has not been given + * `MOD_INBOUND_TOKEN` fails QUIETLY: the 401 is < 500, so `image.controller.ts` maps it to a + * BAD_REQUEST and a moderator sees a toast rather than an incident. πŸ”΄ Not literally silent β€” + * `onFailure` in `moderator-app.service.ts` does emit `moderator-app-request-failed` to Axiom. + * Whether anything ALERTS on that stream is not settled in this repo, so do not assume either way. + * The fallback is retained so this caller does not change shape on the same commit, not because it + * still works. Both arms stay pinned here β€” the legacy arm as a statement of what it now selects, + * not as a supported path β€” plus the boundary case that decides which arm an EMPTY value takes. * * The assertion is on the `token` the service actually hands `createModeratorClient`, i.e. the * expression that does the work. A test matching the variable NAME would also pass against a diff --git a/src/server/services/moderator-app.service.ts b/src/server/services/moderator-app.service.ts index 8cc646f20e..a602537f61 100644 --- a/src/server/services/moderator-app.service.ts +++ b/src/server/services/moderator-app.service.ts @@ -8,17 +8,27 @@ import { logToAxiom } from '~/server/logging/client'; // in the other direction. Import this instance; there should be exactly one configured client. Failures // are logged and rethrown (mutations aren't retried). // -// πŸ”΄ THE `||` FALLBACK IS THE DESIGN, NOT A LEFTOVER β€” do not "clean it up" into a required var. -// This app and the moderator app deploy from different pipelines, and the ConfigMap supplying -// MOD_INBOUND_TOKEN lands from a third one (the infra repo). The fallback makes that ordering -// irrelevant: config first and nothing changes; code first and the call keeps working on the legacy -// token. Remove it and there is a window in which this call is simply broken. +// πŸ”΄ THE `||` FALLBACK NO LONGER PROTECTS DEPLOY ORDER β€” IT IS NOW A SILENT FAILURE PATH. +// It used to: this app and the moderator app deploy from different pipelines, the ConfigMap supplying +// MOD_INBOUND_TOKEN lands from a third (the infra repo), and while the spoke still accepted +// WEBHOOK_TOKEN the legacy arm kept the call working whichever landed first. +// +// πŸ”΄ THE SPOKE HAS NOW DROPPED WEBHOOK_TOKEN FROM ITS ACCEPTED SET, so that arm is a guaranteed 401. +// `||` falls through on an EMPTY local value, never on a REJECTION β€” so in any environment where +// MOD_INBOUND_TOKEN is unset this silently selects a credential the spoke refuses. Nothing here +// errors: `onFailure` logs to Axiom, the 401 reaches `image.controller.ts` and, being < 500, is +// mapped to a BAD_REQUEST β€” a moderator clicking block/unblock gets a toast, not an incident. +// +// So MOD_INBOUND_TOKEN is now effectively REQUIRED wherever this client is used, even though the +// schema still types it optional. The fallback is retained only so this caller did not change shape +// in the same commit as the spoke's removal; it is not a working default. Deleting it, and making +// the variable required so a missing key is a loud boot failure rather than a silent 401, is the +// correct follow-up β€” not a "cleanup" to avoid. // // MOD_INBOUND_TOKEN is the narrow, inbound-only credential the moderator app accepts. // WEBHOOK_TOKEN is the platform-wide admin credential β€” it authenticates ~134 endpoints in this app, -// so presenting it here is far more authority than this one call needs. The moderator app is dropping -// it from its accepted set; once MOD_INBOUND_TOKEN is set in every environment, the fallback (and -// then this comment) can go. +// so presenting it here was far more authority than this one call needs, which is why the spoke +// stopped accepting it. // // πŸ”΄ THE ENDPOINT IS DELIBERATELY *NOT* `MODERATOR_APP_URL`, AND THE TWO ARE NOT INTERCHANGEABLE. // `MODERATOR_APP_URL` has a second consumer with the opposite requirement: `src/pages/moderator/