fix(moderator): stop accepting the shared admin token inbound (#4518)

MOD_INBOUND_TOKEN becomes the only credential accepted on WebhookEndpoint routes
(/api/mod/* and the XGuard agent API). WEBHOOK_TOKEN is the platform-wide admin
credential, so accepting it here let any holder of it drive this app's
moderation actions.

Gated on the runtime attribution signal read as a PAIR: the retiring class at
zero beside a non-zero live class in the same window, over a window in which
every inbound caller had actually called. A zero alone cannot be told from an
emitter that never shipped or stopped being ingested.

Does NOT unset the variable: four services still present it OUTBOUND to the main
app and two degrade by warn-and-skip, so unsetting fails silently.

Prerequisite landed first, out of repo: MOD_INBOUND_TOKEN was set in exactly one
of the 14 live main-app config sources. It is now also in civitai-next (which
every PR preview inherits from) and civitai-next-stage, verified present on a
live pod before this merged. Without that, the main app's
MOD_INBOUND_TOKEN || WEBHOOK_TOKEN fallback would have silently selected a
credential this change refuses -- it falls through on an EMPTY value, never on a
REJECTION.

The no-early-exit timing guard is structurally unobservable at one credential
(break and no-break both compare once). The loop is unchanged; the three
discriminating cases are recorded in place with restore instructions rather than
deleted, and deliberately not left as skipped tests, which would read as
coverage.

Two adversarial audit rounds; both are posted on the PR as audit-claims blocks.
Round 1 found that the main app could still present the retired credential where
the narrow one was unset, plus six stale claims -- including operator docs that
said the retired token still worked, pinned green by a test. Round 2 found that
round 1's own fix corrected a test docblock and left the shipped source comment
it mirrors, leaving the two in contradiction. The ladder stopped on the
attribution gate: both rounds changed zero executable production lines.

Open follow-up, documented in moderator-app.service.ts: the || fallback is now a
silent-401 path rather than a deploy-order bridge. Removing it and making
MOD_INBOUND_TOKEN required belongs in its own change.
This commit is contained in:
Zachary Lowden
2026-08-31 16:08:27 -05:00
committed by GitHub
parent ee79c8b771
commit d353f785c3
13 changed files with 334 additions and 221 deletions
+20 -19
View File
@@ -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=
@@ -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 () => {
+14 -10
View File
@@ -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
@@ -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`.
*/
@@ -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(<empty>, <empty>) 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', () => {
+4 -2
View File
@@ -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);
}
@@ -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<AcceptedCredential, string | undefined> = {
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 }),
};
}
@@ -57,9 +57,11 @@
<li class="mb-1">
Ask a moderator admin for <code class="font-mono text-dark-1">MOD_INBOUND_TOKEN</code>. It
is a service secret, not a per-person credential — but it is <strong>inbound-only</strong>:
this app accepts it and nothing else does, so it cannot be used anywhere but here. Ask for
this one. <code class="font-mono text-dark-1">WEBHOOK_TOKEN</code> 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 <strong>only</strong> credential these endpoints accept.
<code class="font-mono text-dark-1">WEBHOOK_TOKEN</code> used to be accepted here as well and
<strong>no longer is</strong> — 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.
</li>
<li class="mb-1">
Send it as <code class="font-mono text-dark-1">?token=</code> or
@@ -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/);
});
});
+10 -6
View File
@@ -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.
+5 -2
View File
@@ -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;
@@ -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
+18 -8
View File
@@ -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/