From aaaf252519fc7fca77c3440db9a6bbb473d92285 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Fri, 21 Aug 2026 18:06:19 +0200 Subject: [PATCH 1/2] Keep a detection's rule identity, and let a guard be shut down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes to the reporting and refresh paths, all cases of something reporting success while doing nothing useful. **A detection named the wrong rule bundle.** The reporter was handed the ETag read at boot and never heard about a refresh, so once a guard picked up new rules every later detection still claimed the old bundle. Anyone following that up would open a rule document that may not even contain the rule that fired. The identity is now mutable and re-stamped after an accepted swap — and only after one, so a refresh that fell back to cached rules keeps the previous identity, which is the one still true. **A reporting-only guard could not be stopped.** The stop method was installed only when a refresh loop or a block log existed, so a guard configured for detection reporting alone had a live reporter nothing could reach: its last batch depended on a timer, and a clean shutdown was impossible. There is now one `stop()`, always present, that stops the loop, the block log and the reporter and flushes what is buffered. `stopRefresh()` is kept as an alias. **Reporting with no credential posted into a refusal.** The detections endpoint needs a verified, site-bound token. With `reportDetections: true` and no credential resolvable, the guard now starts no reporter, warns once, and says `detectionReporting = 'unavailable-no-credential'` rather than spending a request per batch on nothing. `detectionHealth()` reports local counts — attempted, acknowledged, refused, dropped — and the last acknowledgement, so a delivery path that refuses everything is distinguishable from an app where no rule fired. **The refresh poller did not back off.** Resolving rules deliberately absorbs an API or network failure into cached or bundled rules, so the tick never threw and the scheduler read a fleet-wide outage as a healthy poll — every installed guard kept knocking at its normal interval for as long as it lasted. Rule resolution now reports whether the rules came from the source, the loop backs off on that, and `refresh()` resolves with the same answer instead of rejecting. Also corrects source comments that still described a rules or detections fetch as something the server accepts without a credential. Tests: the new guards are each mutation-checked — remove the re-stamp, the credential gate, the always-installed stop, the delivered/refused split, or the backoff, and exactly the test naming that guarantee fails. Co-Authored-By: Claude Opus 5 (1M context) --- AGENT-INSTALL.md | 17 +- src/protect/detections.js | 64 +++++- src/protect/engine/pulse-client.js | 27 ++- src/protect/protect.d.ts | 28 ++- src/protect/rules/refresh.js | 15 +- src/protect/rules/source.js | 49 +++-- src/protect/runtime.js | 100 ++++++--- src/pulse-token.ts | 4 +- tests/protect/detections.test.ts | 284 +++++++++++++++++++++++++- tests/protect/refresh-backoff.test.ts | 169 +++++++++++++++ tests/protect/runtime-pulse.test.ts | 6 +- 11 files changed, 675 insertions(+), 88 deletions(-) create mode 100644 tests/protect/refresh-backoff.test.ts diff --git a/AGENT-INSTALL.md b/AGENT-INSTALL.md index 0134a45..b18c616 100644 --- a/AGENT-INSTALL.md +++ b/AGENT-INSTALL.md @@ -138,9 +138,10 @@ would have stopped while it is still in dry-run. Two separate paths, with differ `reportFirewallLog: false` in `createProtection`. - **Every rule that matched** goes to `monitor/pulse/detections/` — including matches that blocked, which are reported on both paths. This is **off unless you pass `reportDetections: true`** to - `createProtection`; the scaffolded guard does not pass it. It also requires a provisioned site UUID and - is disabled by `PATCHSTACK_TELEMETRY=off`. It exists because a rule carrying `dry-run` blocks nothing, - so without it nothing distinguishes a rule that is protecting from one that is quietly wrong. + `createProtection`; the scaffolded guard does not pass it. It also requires a provisioned site UUID, a + resolvable credential, and is disabled by `PATCHSTACK_TELEMETRY=off`. It exists because a rule carrying + `dry-run` blocks nothing, so without it nothing distinguishes a rule that is protecting from one that is + quietly wrong. What a detection report contains, per matched rule: the rule id, the request path **with any query string removed**, the parameter names that rule reads (from the rule's own definition), which phase matched, @@ -159,6 +160,16 @@ and not the value of any header, cookie or query-string parameter — including named above. Reports are batched, capped in memory, and dropped rather than retried if Patchstack cannot be reached — a reporting failure never delays or fails a request. +The endpoint needs a credential, so `reportDetections: true` with none resolved starts nothing: the guard +warns once at boot and `protection.detectionReporting` reads `unavailable-no-credential` instead of `on`. +When reporting is on, `protection.detectionHealth()` returns local counts — detections attempted, +acknowledged, refused or unreachable, dropped for queue pressure — and the time of the last +acknowledgement. Those counts stay in your process; nothing extra is sent to report them. + +`protection.stop()` stops everything the guard has running in the background — the rule-refresh loop, the +block-log reporter, the detection reporter — and flushes what is buffered. `protection.stopRefresh()` is +the same method under its older name. Call it on shutdown; it is safe to call twice. + Two more endpoints the package can call, for completeness: - `GET monitor/widget/settings/` — how `status` tells "this site was deleted on diff --git a/src/protect/detections.js b/src/protect/detections.js index 32444b7..6645d7c 100644 --- a/src/protect/detections.js +++ b/src/protect/detections.js @@ -92,7 +92,11 @@ export function createDetectionReporter(opts) { const siteUuid = opts.siteUuid ?? process.env?.PATCHSTACK_SITE_UUID; if (!siteUuid) { // Nothing to report against. A no-op rather than a throw: reporting is never worth failing a boot. - return { record() {}, flush() {}, stop() {}, dropped: () => 0 }; + // It answers the whole interface, so a caller never has to know which kind it holds. + return { + record() {}, flush() {}, stop() {}, setRulesEtag() {}, dropped: () => 0, + health: () => ({ sent: 0, delivered: 0, failed: 0, dropped: 0, lastDeliveredAt: null }), + }; } const configured = opts.baseUrl ?? process.env?.PATCHSTACK_PULSE_RULES_URL; @@ -103,6 +107,22 @@ export function createDetectionReporter(opts) { const flushMs = Number.isFinite(opts.flushMs) && opts.flushMs > 0 ? opts.flushMs : DEFAULT_FLUSH_MS; const maxQueue = Number.isFinite(opts.maxQueue) && opts.maxQueue > 0 ? opts.maxQueue : MAX_QUEUE; + // The bundle identity stamped onto each event. MUTABLE, because the guard's rules are: a refresh + // hot-swaps the ruleset in place, and a reporter holding the boot-time value would attribute a hit + // produced by the new bundle to the old one — sending a reviewer to a rule document that is not the + // one that fired. Updated by the runtime only after an accepted swap (see `setRulesEtag`). + let rulesEtag = opts.rulesEtag ?? null; + + // Delivery health. The capability declaration says a guard INTENDS to report; these say whether + // anything arrived. Counts and one timestamp only — a clean app and a broken delivery path are + // otherwise indistinguishable, and distinguishing them needs no request data at all. + let sent = 0; + let delivered = 0; + let failed = 0; + let droppedTotal = 0; + /** @type {string | null} */ + let lastDeliveredAt = null; + /** @type {Array>} */ let queue = []; /** @type {ReturnType | null} */ @@ -123,6 +143,8 @@ export function createDetectionReporter(opts) { // would make a truncated sample look like a complete one. const droppedWith = dropped; dropped = 0; + droppedTotal += droppedWith; + sent += batch.length; void (async () => { try { @@ -132,17 +154,26 @@ export function createDetectionReporter(opts) { 'Content-Type': 'application/json', Accept: 'application/json', 'User-Agent': '@patchstack/connect', - // Same credential path as the rules fetch, and unauthenticated when none resolves: the - // server accepts the UUID, and reporting must never hinge on getting a token. + // Same credential path as the rules fetch. The detections endpoint is site-addressed and + // requires a verified, site-bound token, so a batch sent without one is refused — which is + // why the runtime does not build a reporter when no credential resolves, rather than + // posting into a 401. ...(await pulseAuthHeader({ pulseAuth: opts.pulseAuth, endpoint: baseUrl }, fetchImpl)), }, body: JSON.stringify({ detections: batch, dropped: droppedWith }), }); - // Fail-open and silent: a rejected or unreachable endpoint must not disturb the app, and must - // not retry into a loop either. The next flush carries whatever arrives next. - if (res && typeof res.then === 'function') res.catch(() => {}); + // Fail-open and no retry: a rejected or unreachable endpoint must not disturb the app, and a + // retry loop over a refusing endpoint is worse than the lost batch. The outcome is counted, so + // that a delivery path which refuses everything is distinguishable from an app where no rule + // fired — both are silence at the server otherwise. + if (res && res.ok) { + delivered += batch.length; + lastDeliveredAt = new Date().toISOString(); + } else { + failed += batch.length; + } } catch { - /* ignore */ + failed += batch.length; } })(); }; @@ -174,7 +205,7 @@ export function createDetectionReporter(opts) { // The state this detection was handled under, which is the whole point: `false` is a rule that // saw traffic it would have stopped. enforced: detection.mode === 'block', - rules_etag: opts.rulesEtag ?? null, + rules_etag: rulesEtag, detected_at: new Date().toISOString(), }); @@ -190,6 +221,23 @@ export function createDetectionReporter(opts) { stopped = true; flush(); }, + /** + * Point later events at the bundle now running. Called after an ACCEPTED swap only — a rejected + * or failed refresh keeps the previous rules, so it must keep the previous identity too. + * + * Already-queued events are not rewritten: the stamp is taken when an event is recorded, which is + * the only moment the two are known to agree. + * + * @param {string | null | undefined} next + */ + setRulesEtag(next) { + rulesEtag = next ?? null; + }, dropped: () => dropped, + /** + * Delivery health, counted in events: attempted, acknowledged, refused or unreachable, and dropped + * for queue pressure — plus when a batch was last acknowledged. No request data of any kind. + */ + health: () => ({ sent, delivered, failed, dropped: droppedTotal + dropped, lastDeliveredAt }), }; } diff --git a/src/protect/engine/pulse-client.js b/src/protect/engine/pulse-client.js index dc676ec..6374916 100644 --- a/src/protect/engine/pulse-client.js +++ b/src/protect/engine/pulse-client.js @@ -7,8 +7,9 @@ const DEFAULT_CACHE_TTL = 300_000; // revalidate on the same tick (spreads load / avoids a thundering herd against the rules API). const JITTER_FRACTION = 0.1; -// Per-site rules client for Pulse (npm/JS) apps. Public endpoint — the site UUID is the only -// credential, passed in the path. Fail-open: any error returns success:false + empty rules so +// Per-site rules client for Pulse (npm/JS) apps. The site UUID addresses the site in the path; it is +// not a credential — the endpoint requires a verified token bound to that same site, so a fetch without +// one is refused. Fail-open regardless: any error returns success:false + empty rules so // createProtection falls back to the disk cache or the bundled rules. // // Conditional fetch: pass a prior `etag` (persisted with the last cached bundle) and the client @@ -61,24 +62,22 @@ export class PulseRuleClient { } const url = `${this.#baseUrl}/rules/${encodeURIComponent(this.#siteUuid)}`; try { - // Unauthenticated when no credential resolved, or when the exchange - // fails — the server still accepts the UUID, and protection must never - // hinge on getting a token. + // Sent without an `Authorization` header when no credential resolved or the exchange failed. + // The server refuses that, and the refusal is handled the same way as any other failure: fall + // back to cached or bundled rules. Attempting it anyway is deliberate — protection must never + // hinge on the token path, and the runtime warns at boot when no credential resolves. const auth = await pulseAuthHeader( { pulseAuth: this.#pulseAuth, endpoint: this.#baseUrl, timeoutMs: this.#timeoutMs }, fetch, ); const headers = { Accept: 'application/json', ...auth }; - // Claimed only on an authenticated request. The rules endpoint still accepts a bare UUID, so on that - // path this header would be an assertion anyone holding the UUID could make — and it asserts the - // reassuring thing: that reporting is on. A dashboard would then say a site is covered because a - // stranger said so. + // Claimed only on an authenticated request. Fetching rules must never hinge on getting a token + // (protection comes first), but CLAIMING a capability may: an unauthenticated request is one whose + // statements about this site carry no weight, and this header asserts the reassuring thing — that + // reporting is on. // - // Fetching rules must never hinge on getting a token (protection comes first), but CLAIMING a - // capability may: an unauthenticated request is one whose statements about this site carry no weight. - // This check only removes the ACCIDENTAL case. The forgeable one is not the client's to prevent, so - // anything acting on this header has to require a verified token itself before believing it — a - // client-side gate is a courtesy, never the guarantee. + // A courtesy, never the guarantee: a client-side gate only removes the accidental case. Anything + // acting on this header has to require a verified token itself before believing it. if (this.#reportsDetections && typeof auth.Authorization === 'string') { headers['X-Patchstack-Detections'] = 'enabled'; } diff --git a/src/protect/protect.d.ts b/src/protect/protect.d.ts index d91d85a..ed7b9eb 100644 --- a/src/protect/protect.d.ts +++ b/src/protect/protect.d.ts @@ -25,13 +25,31 @@ export interface Protection { node(options?: { maxBodyBytes?: number; screenResponses?: boolean }): (req: unknown, res: unknown, next: () => void) => void; /** Present when `egress: true` — restores the original global fetch. */ uninstallEgress?: () => void; - /** Present with a live source — re-fetch + hot-swap the rules once (used by the loop + push). */ - refresh?: () => Promise; + /** Present with a live source — re-fetch + hot-swap the rules once (used by the loop + push). + * Resolves with the outcome of the attempt: `ok: false` means the rules in force came from the + * cache or the bundled fallback, not from the source. It does not reject on a source failure. */ + refresh?: () => Promise<{ ok: boolean; reason?: string }>; /** Present with a live source — a fetch handler that runs `refresh()` when the request carries * the configured refresh secret (a push/zero-day trigger). No secret set → the handler 404s. */ refreshHandler?: () => (request: Request) => Promise; - /** Present when `refreshMs > 0` — stops the live rule-refresh loop. */ - stopRefresh?: () => void; + /** Stops everything with a timer or a buffer behind it: the refresh loop, the block log, the + * detection reporter (flushing what it holds). Always present, and safe to call twice. */ + stop: () => void; + /** Alias of `stop`, under the name callers already have. */ + stopRefresh: () => void; + /** Whether detection reporting is running, requested but undeliverable, or not requested. + * `unavailable-no-credential` means `reportDetections` was set but no credential resolved, so + * nothing is being sent. */ + detectionReporting: "on" | "off" | "unavailable-no-credential"; + /** Present when detection reporting is on — delivery counts (in events) and the last acknowledgement. + * Carries no request data. */ + detectionHealth?: () => { + sent: number; + delivered: number; + failed: number; + dropped: number; + lastDeliveredAt: string | null; + }; } export interface CreateProtectionOptions { @@ -81,6 +99,8 @@ export interface CreateProtectionOptions { * this is a counting channel, not a copy of your traffic. * * Off by default because switching it on adds an outbound request to every guard with a site UUID. + * Needs a resolvable API credential: the endpoint requires a verified, site-bound token, so with no + * credential no reporter is created and `detectionReporting` reads `unavailable-no-credential`. */ reportDetections?: boolean; /** How long to buffer detections before posting a batch. Default 5000ms. */ diff --git a/src/protect/rules/refresh.js b/src/protect/rules/refresh.js index d784a3a..7a7156a 100644 --- a/src/protect/rules/refresh.js +++ b/src/protect/rules/refresh.js @@ -3,7 +3,10 @@ // caller-supplied `tick` (which re-fetches + hot-swaps the engines): // - startRefresh: a self-scheduling poll LOOP (reschedules after each tick settles, so runs never // overlap), with ±jitter (avoid a thundering herd) and exponential backoff on consecutive -// failures. `unref`'d — it never keeps the process alive. +// failures. `unref`'d — it never keeps the process alive. A tick counts as failed when it throws +// OR when it reports `{ ok: false }`: the rule resolver absorbs an API or network failure into +// usable fallback rules, so a thrown error is not the only shape an outage takes, and a poller +// that waits for one keeps the whole fleet knocking at the normal interval while it lasts. // - makeRefreshHandler: a PUSH endpoint — an authenticated fetch handler the platform/SaaS hits // for an immediate refresh (zero-day fast lane) instead of waiting for the next poll. @@ -29,8 +32,9 @@ export function startRefresh(tick, { refreshMs, onError } = {}) { const run = async () => { if (stopped) return; try { - await tick(); - failures = 0; + const status = await tick(); + if (status && status.ok === false) failures++; + else failures = 0; } catch (err) { failures++; notify(onError, err, 'onError'); @@ -60,7 +64,10 @@ export function makeRefreshHandler(tick, secret) { if (provided !== secret) return new Response('forbidden', { status: 403 }); let refreshed = true; try { - await tick(); + // `{ ok: false }` means the tick ran but the rules did not come from the source, which is not a + // refresh — the caller pushed because it had something to deliver, and it did not arrive. + const status = await tick(); + if (status && status.ok === false) refreshed = false; } catch { refreshed = false; // fail-open: report the outcome, never throw } diff --git a/src/protect/rules/source.js b/src/protect/rules/source.js index 7d69980..3314dfa 100644 --- a/src/protect/rules/source.js +++ b/src/protect/rules/source.js @@ -2,6 +2,11 @@ // UUID (Pulse) or token — fetch it (conditional/If-None-Match via the persisted etag), and fall // back through last-known-good → bundled → empty. Fail-open: a fetch/parse error never throws. // The `store` (see ./store.js) is passed in so a refresh reuses the same tiered cache. +// +// Every returned bundle carries `source: { ok, reason? }` — whether the RULES came from the source or +// from a fallback. Absorbing a failure into usable rules is right for protection and insufficient for a +// caller that has its own decision to make: a poller reading only thrown errors treats an outage as a +// healthy poll. The rules answer "what do I enforce"; `source` answers "are these current". import { PatchstackRuleClient } from '../engine/index.js'; import { PulseRuleClient } from '../engine/pulse-client.js'; import { validateBundle } from './validate.js'; @@ -36,6 +41,11 @@ function reportRejections(rejected, options, label) { ), 'onError'); } +/** A bundle plus the outcome of the attempt that produced it. `source` is never written to the store. */ +function fromSource(bundle, reason) { + return reason === undefined ? { ...bundle, source: { ok: true } } : { ...bundle, source: { ok: false, reason } }; +} + export async function resolveRules(options, store, ctx = {}) { // The INITIAL load is on the app's startup path, so the runtime gives it a short budget (see // bootTimeoutMs) and falls back to cache/bundled rather than delaying boot; refreshes get the full @@ -45,61 +55,66 @@ export async function resolveRules(options, store, ctx = {}) { const prior = await store.read(); // { bundle, etag } | null const client = new PulseRuleClient({ siteUuid: options.siteUuid, baseUrl: options.pulseRulesUrl, etag: prior?.etag, timeoutMs, pulseAuth: ctx.pulseAuth, reportsDetections: options.reportDetections === true }); const res = await client.getRules(); - if (res.success && res.notModified && prior?.bundle) return normalizeBundle(prior.bundle, options); + if (res.success && res.notModified && prior?.bundle) return fromSource(normalizeBundle(prior.bundle, options)); if (res.success && !res.notModified) { const rejected = liveUpdateRejections(res, options); if (rejected.length > 0) { reportRejections(rejected, options, 'rule update rejected'); - if (prior?.bundle) return normalizeBundle(prior.bundle, options); - if (options.rules) return normalizeBundle(options.rules, options); - return emptyBundle(); + // Reached the source and refused what it sent. Not ok: the running rules are not the delivered + // ones, and asking again at the normal interval re-downloads the same rejected bundle. + if (prior?.bundle) return fromSource(normalizeBundle(prior.bundle, options), 'update rejected'); + if (options.rules) return fromSource(normalizeBundle(options.rules, options), 'update rejected'); + return fromSource(emptyBundle(), 'update rejected'); } const bundle = normalizeBundle(res, options); await store.write({ bundle, etag: res.etag ?? null }); - return bundle; + return fromSource(bundle); } if (prior?.bundle) { notify(options.onError, new Error(`pulse rule fetch failed (${res.error ?? 'no usable response'}); using cached bundle`), 'onError'); - return normalizeBundle(prior.bundle, options); + return fromSource(normalizeBundle(prior.bundle, options), res.error ?? 'no usable response'); } if (options.rules) { notify(options.onError, new Error(`pulse rule fetch failed (${res.error ?? 'no usable response'}); using bundled fallback`), 'onError'); - return normalizeBundle(options.rules, options); + return fromSource(normalizeBundle(options.rules, options), res.error ?? 'no usable response'); } notify(options.onError, new Error(`pulse rule fetch failed (${res.error ?? 'no usable response'}); no cache — running with no rules`), 'onError'); - return emptyBundle(); + return fromSource(emptyBundle(), res.error ?? 'no usable response'); } if (options.token) { const prior = await store.read(); const client = new PatchstackRuleClient({ token: options.token, baseUrl: options.baseUrl, etag: prior?.etag, timeoutMs }); const res = await client.getRules(); - if (res.success && res.notModified && prior?.bundle) return normalizeBundle(prior.bundle, options); + if (res.success && res.notModified && prior?.bundle) return fromSource(normalizeBundle(prior.bundle, options)); if (res.success && !res.notModified) { const rejected = liveUpdateRejections(res, options); if (rejected.length > 0) { reportRejections(rejected, options, 'rule update rejected'); - if (prior?.bundle) return normalizeBundle(prior.bundle, options); - if (options.rules) return normalizeBundle(options.rules, options); - return emptyBundle(); + // Reached the source and refused what it sent. Not ok: the running rules are not the delivered + // ones, and asking again at the normal interval re-downloads the same rejected bundle. + if (prior?.bundle) return fromSource(normalizeBundle(prior.bundle, options), 'update rejected'); + if (options.rules) return fromSource(normalizeBundle(options.rules, options), 'update rejected'); + return fromSource(emptyBundle(), 'update rejected'); } const bundle = normalizeBundle(res, options); await store.write({ bundle, etag: res.etag ?? null }); - return bundle; + return fromSource(bundle); } if (prior?.bundle) { notify(options.onError, new Error(`rule fetch failed (${res.error ?? 'no usable response'}); using cached bundle`), 'onError'); - return normalizeBundle(prior.bundle, options); + return fromSource(normalizeBundle(prior.bundle, options), res.error ?? 'no usable response'); } notify(options.onError, new Error(`rule fetch failed (${res.error ?? 'no usable response'}); no cache — running with no rules`), 'onError'); - return emptyBundle(); + return fromSource(emptyBundle(), res.error ?? 'no usable response'); } + // No live source configured, so the bundle IS the source and cannot be behind one. if (options.rules) { - return normalizeBundle(options.rules, options); + return fromSource(normalizeBundle(options.rules, options)); } - return emptyBundle(); + return fromSource(emptyBundle()); } // Every rule path (live fetch, cache, bundled fallback) funnels through here, so this is where the diff --git a/src/protect/runtime.js b/src/protect/runtime.js index 3a4f236..c5a8d6d 100644 --- a/src/protect/runtime.js +++ b/src/protect/runtime.js @@ -123,11 +123,12 @@ export async function createProtection(options = {}) { // runtimes this guard is built for do not all have one: on a Worker or an edge function the file is // absent and only `PATCHSTACK_PULSE_AUTH` / `PATCHSTACK_API_KEY` can carry the credential. // - // Unauthenticated rule fetches are accepted today, so the failure is currently invisible — and it - // stays invisible once they are not, because a rejected fetch fails open onto the cached or bundled - // bundle. The guard then screens every request, reports healthy, and never receives another rule. - // That silence is the whole problem: an app protected by rules frozen at install time looks exactly - // like an app protected by current ones. + // Every site-addressed Pulse endpoint requires a verified, site-bound credential; only a first-time + // provisioning call is anonymous. So a missing credential is not a future problem — the rules fetch is + // refused now. And the refusal is invisible, because a failed fetch fails open onto the cached or + // bundled bundle: the guard then screens every request, reports healthy, and never receives another + // rule. That silence is the whole problem — an app protected by rules frozen at install time looks + // exactly like an app protected by current ones. // // A warning, not a throw. Booting is protection; refusing to boot over a missing credential would // trade a stale rule set for no rule set at all. @@ -135,7 +136,7 @@ export async function createProtection(options = {}) { const message = 'Patchstack: no API credential resolved for site ' + options.siteUuid + - '. Rule updates may be rejected and this guard would keep running on its cached rules. ' + + '. Rule updates will be rejected and this guard would keep running on its cached rules. ' + 'Set PATCHSTACK_API_KEY (or pass { pulseAuth }) — required on runtimes without a filesystem.'; notify(onError, new Error(message), 'onError'); console.warn(message); @@ -146,17 +147,35 @@ export async function createProtection(options = {}) { // on the network — the kind of thing that must be disclosed in the shipped docs before it is a default, // not after. The second is that the default belongs to whoever owns that disclosure, so the capability // lands here and the flip is a separate, deliberate change. + // + // And it needs a credential. The detections endpoint is site-addressed and site-bound-token-only, so a + // reporter built without one queues events, posts them, and is refused — spending an outbound request + // per batch to accomplish nothing, while `reportDetections: true` in the config says reporting is on. + // Refusing to build it is the honest outcome; `protection.detectionReporting` says which it is. + let detectionReporting = 'off'; if (options.reportDetections === true && options.siteUuid && telemetryEnabled()) { - detections = createDetectionReporter({ - siteUuid: options.siteUuid, - baseUrl: options.pulseRulesUrl, - pulseAuth, - // The bundle the guard is actually running, so a hit can be attributed to the rules that produced - // it rather than to whatever is current when the report is read. - rulesEtag: (await store.read())?.etag ?? null, - fetchImpl: options.fetchImpl, - flushMs: options.detectionFlushMs, - }); + if (!pulseAuth) { + detectionReporting = 'unavailable-no-credential'; + const message = + 'Patchstack: detection reporting is enabled for site ' + + options.siteUuid + + ' but no API credential resolved, so no report could be delivered. Reporting is off.'; + notify(onError, new Error(message), 'onError'); + console.warn(message); + } else { + detectionReporting = 'on'; + detections = createDetectionReporter({ + siteUuid: options.siteUuid, + baseUrl: options.pulseRulesUrl, + pulseAuth, + // The bundle the guard is actually running, so a hit can be attributed to the rules that produced + // it rather than to whatever is current when the report is read. Kept current across refreshes — + // see the refresh tick below. + rulesEtag: (await store.read())?.etag ?? null, + fetchImpl: options.fetchImpl, + flushMs: options.detectionFlushMs, + }); + } } // Mode is mutable so a Pulse refresh can flip dry-run ↔ block when SaaS enables production. // Precedence: PATCHSTACK_MODE env (local override) > API enforcement > options.mode > dry-run. @@ -660,6 +679,17 @@ export async function createProtection(options = {}) { const next = await resolveRules(options, store, { timeoutMs: options.refreshTimeoutMs, pulseAuth }); mode = resolveMode(options, next); applyBundle(next); + // After the swap, and only after it: later detections belong to the bundle now running. A refresh + // that fell back to the cached or bundled ruleset kept the previous rules, and `store.read()` then + // still holds the previous identity — which is exactly the answer that stays true. + if (detections) detections.setRulesEtag((await store.read())?.etag ?? null); + + // The tick's own outcome, separate from the guard's. `resolveRules` deliberately absorbs an API or + // network failure and returns usable rules, which is right for protection and wrong for a poller: + // a scheduler that only counts THROWN errors reads a fleet-wide outage as a healthy poll and keeps + // knocking at the normal interval. Reported, not thrown — a caller's manual `refresh()` must not + // start failing because the platform is down and the cached rules held. + return next.source ?? { ok: true }; }; if (live) { @@ -670,19 +700,26 @@ export async function createProtection(options = {}) { protection.refreshHandler = () => makeRefreshHandler(runRefreshTick, refreshSecret); } - if (options.refreshMs > 0 && live) { - const loop = startRefresh(runRefreshTick, { refreshMs: options.refreshMs, onError }); - protection.stopRefresh = () => { - loop.stop(); - firewallLog?.stop(); - detections?.stop(); - }; - } else if (firewallLog) { - protection.stopRefresh = () => { - firewallLog.stop(); - detections?.stop(); - }; - } + const loop = options.refreshMs > 0 && live + ? startRefresh(runRefreshTick, { refreshMs: options.refreshMs, onError }) + : null; + + // One method, always present, that reaches everything holding a timer or a buffer: the refresh loop, + // the block log, the detection reporter. Always present because a lifecycle method that exists only + // for some configurations is one a caller cannot rely on — and each of these components can be the + // only one installed, so any of them can be the one left running. + protection.stop = () => { + loop?.stop(); + firewallLog?.stop(); + detections?.stop(); + }; + // The name callers already have, kept as an alias for it. + protection.stopRefresh = protection.stop; + // Which of the three states reporting is in: requested and running, requested but undeliverable, or + // not requested. A boolean would collapse the middle one into "off", which is the reassuring reading. + protection.detectionReporting = detectionReporting; + // Delivery health, when there is a reporter: what was attempted, acknowledged, refused, and dropped. + if (detections) protection.detectionHealth = () => detections.health(); return protection; } @@ -730,8 +767,9 @@ async function resolveApiKey(options) { * order and the same edge-runtime caution as resolveApiKey, and falls back to * it so guards installed before pulseAuth existed keep authenticating. * - * Returning undefined is fine: the rules fetch then goes out unauthenticated, - * which the server still accepts. + * Returning undefined does not fail the boot — protection still runs on the cached or bundled rules — + * but the fetch then goes out unauthenticated and the platform refuses it, so the guard stops receiving + * rules. That is why the caller warns about it at boot rather than treating it as a normal state. */ async function resolvePulseAuth(options) { if (typeof options?.pulseAuth === 'string' && options.pulseAuth.length > 0) return options.pulseAuth; diff --git a/src/pulse-token.ts b/src/pulse-token.ts index a03210b..d0e73dd 100644 --- a/src/pulse-token.ts +++ b/src/pulse-token.ts @@ -46,7 +46,9 @@ export function clearPulseToken(): void { * * Returns null whenever a token cannot be obtained — no credential, a rejected * exchange, a network failure. Callers then send the request unauthenticated, - * which the server still accepts while it runs dual-accept. + * and every site-addressed Pulse endpoint refuses it: only a first-time + * provisioning call is anonymous. Returning null rather than throwing keeps that + * refusal on the caller's own error path, where it can fall back or report. */ export async function getPulseToken( config: Config, diff --git a/tests/protect/detections.test.ts b/tests/protect/detections.test.ts index 91f5f41..fdd889b 100644 --- a/tests/protect/detections.test.ts +++ b/tests/protect/detections.test.ts @@ -280,6 +280,11 @@ describe('the wiring actually runs', () => { const fetchMock = vi.fn(async (url: string) => { posted.push(String(url)); if (String(url).includes('/detections/')) return new Response('{}', { status: 202 }); + if (String(url).includes('/token')) { + return new Response(JSON.stringify({ access_token: 'jwt-abc', expires_in: 3600 }), { + status: 200, headers: { 'Content-Type': 'application/json' }, + }); + } return new Response( JSON.stringify({ @@ -294,6 +299,7 @@ describe('the wiring actually runs', () => { const p: any = await createProtection({ siteUuid: 'site-1', pulseRulesUrl: 'https://x.test/monitor/pulse', + pulseAuth: 'the-secret-40-chars-long-ish-value-here-987', reportDetections: true, detectionFlushMs: 1, }); @@ -308,10 +314,9 @@ describe('the wiring actually runs', () => { describe('the capability claim is only made when it carries weight', () => { it('stays silent on an unauthenticated rules fetch', async () => { - // The rules endpoint still accepts a bare UUID, so on that path this header is an assertion anyone - // holding the UUID could make — and it asserts the reassuring thing, that reporting is on. A dashboard - // would then report a site as covered because a stranger said so. Fetching rules must not hinge on a - // token; claiming a capability must. + // An unauthenticated request is one whose statements about this site carry no weight, and this header + // asserts the reassuring thing: that reporting is on. Fetching rules must not hinge on a token — + // protection comes first — but claiming a capability must. const seen: Array> = []; const fetchMock = vi.fn(async (_url: string, init?: RequestInit) => { seen.push((init?.headers ?? {}) as Record); @@ -340,3 +345,274 @@ describe('the capability claim is only made when it carries weight', () => { p.stopRefresh?.(); }); }); + +describe('the bundle identity travels with the detection', () => { + it('attributes a detection to the rules that were running when it fired', async () => { + // A detection is evidence about a rule document, so it has to name the one that fired. A guard + // refreshes in place, so the identity it stamps has to move with the swap: attributed to the boot-time + // bundle, a hit produced by revision B sends a reviewer to revision A's document — which may not even + // contain the rule. + const posts: any[] = []; + let etag = '"v1"'; + const fetchMock = vi.fn(async (url: string, init?: RequestInit) => { + const target = String(url); + if (target.includes('/token')) { + return new Response(JSON.stringify({ access_token: 'jwt-abc', expires_in: 3600 }), { + status: 200, headers: { 'Content-Type': 'application/json' }, + }); + } + if (target.includes('/detections/')) { + posts.push(JSON.parse(String(init?.body ?? '{}'))); + + return new Response('{}', { status: 202 }); + } + + return new Response( + JSON.stringify({ + firewall: [{ id: 'r1', title: 'boom', rule_v2: [{ parameter: 'get.q', match: { type: 'contains', value: 'boom' } }] }], + whitelists: [], enforcement: 'dry-run', + }), + { status: 200, headers: { 'Content-Type': 'application/json', ETag: etag } }, + ); + }); + vi.stubGlobal('fetch', fetchMock); + + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + pulseAuth: 'the-secret-40-chars-long-ish-value-here-987', + reportDetections: true, + detectionFlushMs: 1, + }); + + await p.fetchGuard()(new Request('https://app.test/api/x?q=boom')); + await drain(); + await drain(); + + etag = '"v2"'; + await p.refresh(); + + await p.fetchGuard()(new Request('https://app.test/api/x?q=boom')); + p.stop(); + await drain(); + await drain(); + + const stamped = posts.flatMap((body) => body.detections.map((d: any) => d.rules_etag)); + expect(stamped).toEqual(['"v1"', '"v2"']); + }); + + it('keeps the previous identity when a refresh could not reach the source', async () => { + // The control. A failed refresh keeps the previous rules, so it has to keep the previous identity — + // moving it would attribute a hit to a bundle this guard never received. + let fail = false; + const posts: any[] = []; + const fetchMock = vi.fn(async (url: string, init?: RequestInit) => { + const target = String(url); + if (target.includes('/token')) { + return new Response(JSON.stringify({ access_token: 'jwt-abc', expires_in: 3600 }), { + status: 200, headers: { 'Content-Type': 'application/json' }, + }); + } + if (target.includes('/detections/')) { + posts.push(JSON.parse(String(init?.body ?? '{}'))); + + return new Response('{}', { status: 202 }); + } + if (fail) throw new Error('network down'); + + return new Response( + JSON.stringify({ + firewall: [{ id: 'r1', title: 'boom', rule_v2: [{ parameter: 'get.q', match: { type: 'contains', value: 'boom' } }] }], + whitelists: [], enforcement: 'dry-run', + }), + { status: 200, headers: { 'Content-Type': 'application/json', ETag: '"v1"' } }, + ); + }); + vi.stubGlobal('fetch', fetchMock); + + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + pulseAuth: 'the-secret-40-chars-long-ish-value-here-987', + reportDetections: true, + detectionFlushMs: 1, + }); + + fail = true; + const status = await p.refresh(); + expect(status.ok, 'a refresh that fell back to cached rules is not a successful refresh').toBe(false); + + await p.fetchGuard()(new Request('https://app.test/api/x?q=boom')); + p.stop(); + await drain(); + await drain(); + + expect(posts.flatMap((body) => body.detections.map((d: any) => d.rules_etag))).toEqual(['"v1"']); + }); +}); + +describe('reporting that cannot be delivered', () => { + it('is not started, and says so', async () => { + // The detections endpoint requires a verified, site-bound token. A reporter built without a credential + // would queue every detection, post it, and be refused — an outbound request per batch, while the + // config says reporting is on. + const posted: string[] = []; + const fetchMock = vi.fn(async (url: string) => { + posted.push(String(url)); + + return new Response(JSON.stringify({ + firewall: [{ id: 'r1', title: 'boom', rule_v2: [{ parameter: 'get.q', match: { type: 'contains', value: 'boom' } }] }], + whitelists: [], enforcement: 'dry-run', + }), { status: 200, headers: { 'Content-Type': 'application/json' } }); + }); + vi.stubGlobal('fetch', fetchMock); + + const warnings: string[] = []; + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + reportDetections: true, + detectionFlushMs: 1, + onError: (err: Error) => warnings.push(err.message), + }); + + await p.fetchGuard()(new Request('https://app.test/api/x?q=boom')); + await drain(); + await drain(); + + // Distinguished from "off": a boolean would report an undeliverable configuration as a deliberate one. + expect(p.detectionReporting).toBe('unavailable-no-credential'); + expect(p.detectionHealth, 'no reporter means no health to report').toBeUndefined(); + expect(posted.some((url) => url.includes('/detections/'))).toBe(false); + expect(warnings.some((m) => m.includes('detection reporting is enabled'))).toBe(true); + + p.stop(); + }); + + it('is started, and named as running, once a credential resolves', async () => { + // The control: same configuration plus a credential. Without it the assertion above would also pass + // for an implementation that never reports at all. + const fetchMock = vi.fn(async (url: string) => { + if (String(url).includes('/token')) { + return new Response(JSON.stringify({ access_token: 'jwt-abc', expires_in: 3600 }), { + status: 200, headers: { 'Content-Type': 'application/json' }, + }); + } + + return new Response(JSON.stringify({ firewall: [], whitelists: [], enforcement: 'dry-run' }), { + status: 200, headers: { 'Content-Type': 'application/json' }, + }); + }); + vi.stubGlobal('fetch', fetchMock); + + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + pulseAuth: 'the-secret-40-chars-long-ish-value-here-987', + reportDetections: true, + }); + + expect(p.detectionReporting).toBe('on'); + expect(typeof p.detectionHealth).toBe('function'); + p.stop(); + }); +}); + +describe('the reporter can always be reached', () => { + it('flushes on stop, with no refresh loop and no block log installed', async () => { + // Reporting alone is a valid configuration: no refresh interval, no API key. The final batch must not + // depend on a timer nobody can bring forward, or a clean shutdown loses it. + const posts: any[] = []; + const fetchMock = vi.fn(async (url: string, init?: RequestInit) => { + const target = String(url); + if (target.includes('/token')) { + return new Response(JSON.stringify({ access_token: 'jwt-abc', expires_in: 3600 }), { + status: 200, headers: { 'Content-Type': 'application/json' }, + }); + } + if (target.includes('/detections/')) { + posts.push(JSON.parse(String(init?.body ?? '{}'))); + + return new Response('{}', { status: 202 }); + } + + return new Response(JSON.stringify({ + firewall: [{ id: 'r1', title: 'boom', rule_v2: [{ parameter: 'get.q', match: { type: 'contains', value: 'boom' } }] }], + whitelists: [], enforcement: 'dry-run', + }), { status: 200, headers: { 'Content-Type': 'application/json' } }); + }); + vi.stubGlobal('fetch', fetchMock); + + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + pulseAuth: 'the-secret-40-chars-long-ish-value-here-987', + reportDetections: true, + // The default buffer window, long enough that only an explicit flush can produce the post below. + }); + + expect(typeof p.stop, 'the lifecycle method exists for every configuration').toBe('function'); + + await p.fetchGuard()(new Request('https://app.test/api/x?q=boom')); + await drain(); + expect(posts.length, 'still buffered — nothing has asked it to flush').toBe(0); + + p.stop(); + await drain(); + await drain(); + + expect(posts.length).toBe(1); + expect(p.detectionHealth()).toMatchObject({ sent: 1, delivered: 1, failed: 0, dropped: 0 }); + expect(p.detectionHealth().lastDeliveredAt).not.toBeNull(); + }); +}); + +describe('delivery health', () => { + it('separates what was acknowledged from what was refused', async () => { + // The capability declaration says a guard intends to report. Only an acknowledgement says anything + // arrived, and without counting the refusals a delivery path that rejects everything reads the same + // as an app where no rule fired. + let status = 500; + const fetchImpl = vi.fn(async () => new Response('{}', { status })); + const reporter = createDetectionReporter({ + siteUuid: 'site-1', + baseUrl: 'https://x.test/monitor/pulse', + rulesEtag: '"v7"', + fetchImpl: fetchImpl as unknown as typeof fetch, + }); + + reporter.record({ rule: pinnedRule, phase: 'request', mode: 'dry-run', path: '/a' }); + reporter.flush(); + await drain(); + + expect(reporter.health()).toMatchObject({ sent: 1, delivered: 0, failed: 1 }); + expect(reporter.health().lastDeliveredAt).toBeNull(); + + status = 202; + reporter.record({ rule: pinnedRule, phase: 'request', mode: 'dry-run', path: '/a' }); + reporter.flush(); + await drain(); + + expect(reporter.health()).toMatchObject({ sent: 2, delivered: 1, failed: 1 }); + expect(reporter.health().lastDeliveredAt).not.toBeNull(); + }); + + it('counts events dropped for queue pressure, flushed or not', async () => { + const fetchImpl = vi.fn(async () => new Response('{}', { status: 202 })); + const reporter = createDetectionReporter({ + siteUuid: 'site-1', + baseUrl: 'https://x.test/monitor/pulse', + maxQueue: 2, + fetchImpl: fetchImpl as unknown as typeof fetch, + }); + + for (let i = 0; i < 5; i++) reporter.record({ rule: pinnedRule, phase: 'request', mode: 'dry-run', path: '/a' }); + + // Reported before the flush that would carry them, so a snapshot taken between batches is not short. + expect(reporter.health().dropped).toBe(3); + + reporter.flush(); + await drain(); + expect(reporter.health().dropped).toBe(3); + }); +}); diff --git a/tests/protect/refresh-backoff.test.ts b/tests/protect/refresh-backoff.test.ts new file mode 100644 index 0000000..e43caf3 --- /dev/null +++ b/tests/protect/refresh-backoff.test.ts @@ -0,0 +1,169 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { createProtection } from '../../src/protect/runtime.js'; + +/** + * Backoff on a rule source that is down. + * + * Resolving rules is deliberately forgiving: an API or network failure becomes a cached or bundled + * ruleset rather than an exception, because protection has to keep running. The poller needs the + * opposite — it needs to know the fetch did not succeed, or every installed guard keeps knocking at its + * normal interval for as long as the outage lasts, and they all come back at once when it ends. + */ + +const RULES = { + firewall: [{ id: 'r1', title: 'boom', rule_v2: [{ parameter: 'get.q', match: { type: 'contains', value: 'boom' } }] }], + whitelists: [], + enforcement: 'dry-run', +}; + +function rulesEndpoint(state: { fail: boolean }) { + return vi.fn(async (url: string) => { + if (state.fail) throw new Error('rule source unreachable'); + + return new Response(JSON.stringify(RULES), { status: 200, headers: { 'Content-Type': 'application/json' } }); + }); +} + +/** Rule fetches only, so the manifest re-post on the same tick cannot be mistaken for one. */ +function ruleFetches(fetchMock: { mock: { calls: unknown[][] } }): number { + return fetchMock.mock.calls.filter(([url]) => String(url).includes('/rules/')).length; +} + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +describe('the refresh loop', () => { + it('slows down while the source is unreachable, even though the guard keeps its rules', async () => { + vi.useFakeTimers(); + const state = { fail: false }; + const fetchMock = rulesEndpoint(state); + vi.stubGlobal('fetch', fetchMock); + + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + reportManifest: false, + refreshMs: 10_000, + }); + expect(ruleFetches(fetchMock), 'the boot fetch').toBe(1); + + state.fail = true; + await vi.advanceTimersByTimeAsync(10_000); + expect(ruleFetches(fetchMock), 'one poll, which could not reach the source').toBe(2); + + // The rules are still in force — the failure is about currency, not protection. + expect(p.rules.request.length).toBe(1); + + await vi.advanceTimersByTimeAsync(10_000); + expect(ruleFetches(fetchMock), 'backed off past the next ordinary interval').toBe(2); + + await vi.advanceTimersByTimeAsync(10_000); + expect(ruleFetches(fetchMock)).toBe(3); + + p.stop(); + }); + + it('returns to the normal interval once the source answers again', async () => { + // The other half: a backoff that never resets would leave a fleet minutes behind a zero-day rule + // because of an outage that ended. + vi.useFakeTimers(); + const state = { fail: true }; + const fetchMock = rulesEndpoint(state); + vi.stubGlobal('fetch', fetchMock); + + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + reportManifest: false, + rules: RULES, + refreshMs: 10_000, + }); + + await vi.advanceTimersByTimeAsync(10_000); + const failedPolls = ruleFetches(fetchMock); + + state.fail = false; + // Long enough to get through the backed-off delay and land the recovering poll. + await vi.advanceTimersByTimeAsync(80_000); + const recovered = ruleFetches(fetchMock); + expect(recovered).toBeGreaterThan(failedPolls); + + await vi.advanceTimersByTimeAsync(10_000); + expect(ruleFetches(fetchMock), 'polling at the configured interval again').toBeGreaterThan(recovered); + + p.stop(); + }); + + it('does not back off on a healthy poll', async () => { + // The control. Without it the first test would pass for a loop that backs off unconditionally. + vi.useFakeTimers(); + const state = { fail: false }; + const fetchMock = rulesEndpoint(state); + vi.stubGlobal('fetch', fetchMock); + + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + reportManifest: false, + refreshMs: 10_000, + }); + + await vi.advanceTimersByTimeAsync(10_000); + await vi.advanceTimersByTimeAsync(10_000); + await vi.advanceTimersByTimeAsync(10_000); + expect(ruleFetches(fetchMock), 'boot plus one poll per interval').toBe(4); + + p.stop(); + }); + + it('reports a fallback as an unsuccessful refresh to a caller that asks for one', async () => { + const state = { fail: false }; + const fetchMock = rulesEndpoint(state); + vi.stubGlobal('fetch', fetchMock); + + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + reportManifest: false, + }); + + expect(await p.refresh()).toEqual({ ok: true }); + + state.fail = true; + const failed = await p.refresh(); + expect(failed.ok).toBe(false); + expect(typeof failed.reason).toBe('string'); + + // Still resolved, not rejected: a manual refresh reports the outcome, and the rules it already had + // are still loaded. + expect(p.rules.request.length).toBe(1); + + p.stop(); + }); + + it('answers the push endpoint with what actually happened', async () => { + const state = { fail: true }; + const fetchMock = rulesEndpoint(state); + vi.stubGlobal('fetch', fetchMock); + + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + reportManifest: false, + rules: RULES, + refreshSecret: 'push-secret', + }); + + const handler = p.refreshHandler(); + const refused = await handler(new Request('https://app.test/refresh', { headers: { 'x-patchstack-refresh': 'push-secret' } })); + expect(await refused.json()).toEqual({ refreshed: false }); + + state.fail = false; + const ok = await handler(new Request('https://app.test/refresh', { headers: { 'x-patchstack-refresh': 'push-secret' } })); + expect(await ok.json()).toEqual({ refreshed: true }); + + p.stop(); + }); +}); diff --git a/tests/protect/runtime-pulse.test.ts b/tests/protect/runtime-pulse.test.ts index cea78a4..dbc7797 100644 --- a/tests/protect/runtime-pulse.test.ts +++ b/tests/protect/runtime-pulse.test.ts @@ -207,7 +207,6 @@ describe('createProtection live rule refresh (refreshMs)', () => { vi.stubGlobal('fetch', fetchMock); const protection = await createProtection({ siteUuid: 'site-1', pulseRulesUrl: 'https://x.test/monitor/pulse', mode: 'block' }); - expect(protection.stopRefresh).toBeUndefined(); await vi.advanceTimersByTimeAsync(60_000); expect(fetchMock).toHaveBeenCalledTimes(1); // only the boot fetch — no interval re-fetches @@ -217,7 +216,10 @@ describe('createProtection live rule refresh (refreshMs)', () => { }); it('does not schedule a refresh without a live source, even with refreshMs set', async () => { + // A bundle is not a source: there is nothing to re-fetch, so neither trigger is offered. Asserted on + // `refresh`/`refreshHandler` rather than on the stop method, which exists for every configuration. const protection = await createProtection({ rules, mode: 'block', refreshMs: 1000 }); - expect(protection.stopRefresh).toBeUndefined(); + expect(protection.refresh).toBeUndefined(); + expect(protection.refreshHandler).toBeUndefined(); }); }); From 8eebb6ff50d0f380fe3da035a6ae32422b768550 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Fri, 21 Aug 2026 21:01:42 +0200 Subject: [PATCH 2/2] Report the rule's own revision with a detection, beside the bundle's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bundle ETag says which bundle a detection came from, and it changes whenever anything in that bundle changes — a rule added, one withdrawn, the site's enforcement flipped. So it cannot answer the question a reviewer actually has about one rule: are these counts evidence about the document this rule has now? Each served rule can carry its own `source_revision`, and a detection now forwards it. Forwarded, not derived: the side that served the rule knows which document it is, and a value computed here would be this client's opinion of it. Null when the bundle carried none — a customer's own rule has no revision — because "cannot say" has to stay distinguishable from a revision that no longer matches. The field is an identifier, like the rest of the payload, and the shipped disclosure names it. Co-Authored-By: Claude Opus 5 (1M context) --- AGENT-INSTALL.md | 3 +- src/protect/detections.js | 27 ++++++++- .../detection-payload-contract.test.ts | 1 + tests/protect/detections.test.ts | 59 ++++++++++++++++++- 4 files changed, 86 insertions(+), 4 deletions(-) diff --git a/AGENT-INSTALL.md b/AGENT-INSTALL.md index b18c616..d7c0654 100644 --- a/AGENT-INSTALL.md +++ b/AGENT-INSTALL.md @@ -145,7 +145,8 @@ would have stopped while it is still in dry-run. Two separate paths, with differ What a detection report contains, per matched rule: the rule id, the request path **with any query string removed**, the parameter names that rule reads (from the rule's own definition), which phase matched, -whether it was enforced, the identifier of the rule bundle in use, and a timestamp. Each batch also +whether it was enforced, the identifier of the rule bundle in use, the revision of the rule itself when the +bundle carried one, and a timestamp. Each batch also carries a count of reports dropped when traffic outran the flush, so a partial sample is not read as a complete one. diff --git a/src/protect/detections.js b/src/protect/detections.js index 6645d7c..cb6f73a 100644 --- a/src/protect/detections.js +++ b/src/protect/detections.js @@ -15,8 +15,8 @@ import { isSafeOrigin } from './safe-origin.js'; * ## The payload is deliberately small * * `rule_id`, route PATH, the parameters the rule reads, a timestamp, whether it was enforced, the phase, - * and the bundle identity. That is enough to count hits per rule, compare them against traffic, and - * decide whether a rule is wrong. + * the bundle identity, and the rule's own revision where the bundle carried one. That is enough to count + * hits per rule, compare them against traffic, and decide whether a rule is wrong. * * What it never carries: **the matched value, the request body, headers, or query-string values**. A * channel that counts detections is a different thing from a copy of an application's traffic, and once @@ -61,6 +61,24 @@ export function ruleParameters(rule) { return [...out]; } +/** + * The rule's own revision, from the served rule. + * + * Read off the delivered rule rather than derived: whoever served it knows what document this is, and a + * value computed here would be this client's opinion of it. Accepts a string or a number, because the two + * kinds of rule that carry one number their revisions differently. + * + * @param {any} rule + * @returns {string | null} + */ +export function revisionOf(rule) { + const revision = rule?.source_revision; + if (typeof revision === 'string' && revision !== '') return revision; + if (typeof revision === 'number' && Number.isFinite(revision)) return String(revision); + + return null; +} + /** * The request path with the query string removed. * @@ -206,6 +224,11 @@ export function createDetectionReporter(opts) { // saw traffic it would have stopped. enforced: detection.mode === 'block', rules_etag: rulesEtag, + // The revision of THIS rule, as the bundle delivered it. The bundle identity above answers "which + // bundle", which changes whenever anything in it changes — so it cannot say whether the counts for + // one rule describe the document that rule has now. Passed through untouched, and null when the + // bundle carried none. + rule_revision: revisionOf(detection.rule), detected_at: new Date().toISOString(), }); diff --git a/tests/protect/detection-payload-contract.test.ts b/tests/protect/detection-payload-contract.test.ts index 304f916..3d1139e 100644 --- a/tests/protect/detection-payload-contract.test.ts +++ b/tests/protect/detection-payload-contract.test.ts @@ -35,6 +35,7 @@ const FIELD_DISCLOSURE: Record = { phase: /which phase matched/i, enforced: /whether it was enforced/i, rules_etag: /identifier of the rule bundle/i, + rule_revision: /revision of the rule/i, detected_at: /timestamp/i, }; diff --git a/tests/protect/detections.test.ts b/tests/protect/detections.test.ts index fdd889b..e9dc6e4 100644 --- a/tests/protect/detections.test.ts +++ b/tests/protect/detections.test.ts @@ -17,7 +17,7 @@ import { createProtection } from '../../src/protect/runtime.js'; const drain = () => new Promise((resolve) => setTimeout(resolve, 0)); /** Everything the payload is allowed to carry, and nothing else. */ -const ALLOWED_KEYS = ['rule_id', 'route', 'parameters', 'phase', 'enforced', 'rules_etag', 'detected_at']; +const ALLOWED_KEYS = ['rule_id', 'route', 'parameters', 'phase', 'enforced', 'rules_etag', 'rule_revision', 'detected_at']; const pinnedRule = { id: 'pulse-1', @@ -616,3 +616,60 @@ describe('delivery health', () => { expect(reporter.health().dropped).toBe(3); }); }); + +describe('the rule revision travels with the detection', () => { + it('reports the revision the bundle served for that rule', async () => { + // The bundle identity says WHICH BUNDLE; it changes whenever anything in the bundle changes, so it + // cannot say whether one rule's counts describe the document that rule has now. The rule's own revision + // can, and the side that served it is the side that knows it. + const { reporter, posts } = reporterWith(); + + reporter.record({ + rule: { ...pinnedRule, source_revision: 'sha256:abcdef' }, + phase: 'request', + mode: 'dry-run', + path: '/api/preview', + }); + reporter.flush(); + await drain(); + + expect(posts[0].body.detections[0].rule_revision).toBe('sha256:abcdef'); + }); + + it('reports a numeric revision as the string it was served as', async () => { + // A generated rule numbers its revisions; a curated one hashes its document. Both are identifiers, and + // the reporter forwards rather than interprets. + const { reporter, posts } = reporterWith(); + + reporter.record({ rule: { ...pinnedRule, source_revision: 13 }, phase: 'request', mode: 'dry-run', path: '/a' }); + reporter.flush(); + await drain(); + + expect(posts[0].body.detections[0].rule_revision).toBe('13'); + }); + + it('reports null when the bundle carried no revision for the rule', async () => { + // A customer's own rule has none. Null is "cannot say", which the consumer has to be able to tell apart + // from a revision that no longer matches. + const { reporter, posts } = reporterWith(); + + reporter.record({ rule: pinnedRule, phase: 'request', mode: 'dry-run', path: '/a' }); + reporter.flush(); + await drain(); + + expect(posts[0].body.detections[0].rule_revision).toBeNull(); + }); + + it('reports no revision for a value that is not one', async () => { + // An object or a boolean in that field is a served rule this client cannot read, and forwarding it + // would put an uninterpretable value where a consumer expects an identifier. + const { reporter, posts } = reporterWith(); + + reporter.record({ rule: { ...pinnedRule, source_revision: { v: 1 } }, phase: 'request', mode: 'dry-run', path: '/a' }); + reporter.record({ rule: { ...pinnedRule, source_revision: '' }, phase: 'request', mode: 'dry-run', path: '/a' }); + reporter.flush(); + await drain(); + + for (const event of posts[0].body.detections) expect(event.rule_revision).toBeNull(); + }); +});