[ENG-4060] Account for reporting outcomes and compare refresh secrets in full

AGENT-INSTALL.md

@@ -457,8 +457,9 @@ requests are aborted, it starts nothing further, and it discards what it was hol
 request to stop, not a guarantee — a transport that ignores it is detached rather than completed, so
 "resolved" means the reporter is finished with it, and a runtime that kills the process still wins
 regardless. Every detection event ends up delivered, refused or dropped and is reported in the health
-counts; block-log records have no counters, so one lost to a failed send or an expired shutdown is
-reported nowhere.
+counts. The block log keeps the same kind of local counts: `protection.blockLogHealth()` returns how many
+block records were accepted, delivered, failed or dropped, and how many are still queued. Those counts
+carry no request data and stay in your process.
 
 The client address is reported with its **provenance**, because an address is only as trustworthy as
 whatever supplied it. `client_ip_source` is one of `runtime` (the address the transport observed),

src/protect/firewall-log.js

@@ -79,7 +79,12 @@ export function resolveApiBase(pulseOrManifestUrl) {
 export function createFirewallLogReporter(opts) {
   const creds = parseApiKey(opts.apiKey);
   if (!creds) {
-    return { record() {}, flush: () => Promise.resolve(), stop: () => Promise.resolve() };
+    return {
+      record() {},
+      flush: () => Promise.resolve(),
+      stop: () => Promise.resolve(),
+      health: () => ({ recorded: 0, delivered: 0, failed: 0, dropped: 0, queued: 0 }),
+    };
   }
 
   const apiBase = safeBaseUrl(opts.apiBase, DEFAULT_API_BASE, 'block-log API').replace(/\/$/, '');
@@ -102,6 +107,27 @@ export function createFirewallLogReporter(opts) {
   /** Set when a shutdown gives up waiting: nothing may start, continue, or be retained after it. */
   let ended = false;
 
+  // Where every record went. `recorded` is what the queue accepted, and each accepted record ends up
+  // delivered (the endpoint acknowledged its batch), failed (its batch was refused or could not be sent),
+  // or dropped (a shutdown ran out of time before it was sent). `dropped` also counts records turned
+  // away because the queue was full, which were never accepted.
+  let recorded = 0;
+  let delivered = 0;
+  let failed = 0;
+  let dropped = 0;
+  /** The batch being sent, until its outcome is counted. A shutdown that gives up counts it as dropped. */
+  /** @type {{ size: number, counted: boolean } | null} */
+  let activeBatch = null;
+  /** Count a batch's outcome once: whichever of the send and a shutdown decides first. */
+  const settle = (sent, outcome) => {
+    if (sent.counted) return;
+    sent.counted = true;
+    if (outcome === 'delivered') delivered += sent.size;
+    else if (outcome === 'failed') failed += sent.size;
+    else dropped += sent.size;
+    if (activeBatch === sent) activeBatch = null;
+  };
+
   /** @type {{ token: string, expiresAt: number } | null} */
   let cachedToken = null;
   /** @type {Promise<string | null> | null} */
@@ -168,11 +194,18 @@ export function createFirewallLogReporter(opts) {
     // whether it succeeded.
     /** @type {Promise<void>} */
     let entry;
+    const sent = { size: batch.length, counted: false };
+    activeBatch = sent;
     entry = (async () => {
       try {
         const token = await fetchAccessToken(controller?.signal);
         // Not after a shutdown gave up: it has already reported itself finished.
-        if (!token || ended) return;
+        if (ended) return;
+        if (!token) {
+          settle(sent, 'failed');
+
+          return;
+        }
 
         const body = new URLSearchParams();
         body.set('type', 'firewall');
@@ -191,10 +224,14 @@ export function createFirewallLogReporter(opts) {
           // Both phases carry the attempt controller, so slow transports cannot accumulate work.
           ...(controller ? { signal: controller.signal } : {}),
         });
-        if (p && typeof p.then === 'function') await p.catch(() => {});
+        const res = p && typeof p.then === 'function' ? await p.catch(() => null) : p;
+        settle(sent, res && res.ok ? 'delivered' : 'failed');
       } catch {
         /* A delivery problem is never worth disturbing the app over. */
       } finally {
+        // Anything not counted above failed before a verdict. A shutdown that gave up has already counted
+        // this batch as dropped, so an answer arriving later changes nothing.
+        settle(sent, 'failed');
         clearTimeout(attemptTimer);
         if (activeController === controller) activeController = null;
         if (inFlight === entry) inFlight = null;
@@ -225,7 +262,12 @@ export function createFirewallLogReporter(opts) {
       const fid = event?.rule?.id;
       if (fid === undefined || fid === null || fid === '') return;
 
-      if (queue.length >= MAX_QUEUE) return;
+      if (queue.length >= MAX_QUEUE) {
+        dropped++;
+
+        return;
+      }
+      recorded++;
       queue.push({
         fid,
         method: event.method ?? null,
@@ -242,6 +284,10 @@ export function createFirewallLogReporter(opts) {
       if (!timer) timer = setTimeout(flush, flushMs);
     },
     flush,
+    /** Where the records went so far; see the counters above. `queued` is what is waiting now. */
+    health() {
+      return { recorded, delivered, failed, dropped, queued: queue.length };
+    },
     /**
      * Stop, and hand back a wait for what was outstanding.
      *
@@ -275,6 +321,9 @@ export function createFirewallLogReporter(opts) {
         ended = true;
         activeController?.abort();
         activeController = null;
+        // The batch in flight may never settle: a transport can ignore its abort signal.
+        if (activeBatch) settle(activeBatch, 'dropped');
+        dropped += queue.length;
         queue = [];
         inFlight = null;
       };

src/protect/notify.js

@@ -12,9 +12,11 @@
  * applied to the hooks that were missed rather than restated for one of them.
  *
  * Not silent, though. A hook that throws is a bug in the host's code and swallowing it entirely would
- * hide it forever, so the first failure per hook is reported — once, because these run per request and a
- * persistently broken hook would otherwise print on every one. Same reasoning as the engine's
- * report-once for a persistently broken rule.
+ * hide it forever, so the first failure of each callback is reported — once, because these run per request
+ * and a persistently broken hook would otherwise print on every one. Same reasoning as the engine's
+ * report-once for a persistently broken rule. "Each callback" is each function under each hook name, so a
+ * second guard's broken hook is reported even after the first guard's was; the total is capped, so a host
+ * that creates a fresh callback per request cannot turn this into per-request output.
  *
  * @param {unknown} fn the callback, or anything that is not a function (then this is a no-op)
  * @param {unknown} arg the single argument to hand it
@@ -25,8 +27,12 @@
  *   caller has already decided not to fall back. Synchronous handlers get the stronger answer.
  */
 
-/** Hooks already reported as broken. Module-scoped: one warning per hook per process, not per guard. */
-const reported = new Set();
+/** Callbacks already reported as broken, by function, then by hook name. */
+let reported = new WeakMap();
+
+/** Warnings written so far, and the most this process writes before saying the rest are suppressed. */
+let warnings = 0;
+const MAX_WARNINGS = 20;
 
 export function notify(fn, arg, label) {
   if (typeof fn !== 'function') return false;
@@ -40,35 +46,47 @@ export function notify(fn, arg, label) {
     // able to kill the app, which is a worse outcome than the throw we set out to contain.
     if (result !== null && typeof result === 'object' && typeof result.then === 'function') {
       try {
-        result.then(undefined, (err) => warnOnce(label, err));
+        result.then(undefined, (err) => warnOnce(fn, label, err));
       } catch {
         // A `then` that throws on access. Nothing more to attach to; the value is not a usable promise.
       }
     }
 
     return true;
   } catch (err) {
-    warnOnce(label, err);
+    warnOnce(fn, label, err);
 
     return false;
   }
 }
 
 /**
- * Report a broken callback once per process.
+ * Report a broken callback once.
  *
  * Must not throw: it runs inside the containment, so its own failure would be the thing that breaks the
  * guarantee it exists to report on.
  */
-function warnOnce(label, err) {
-  if (reported.has(label)) return;
-  reported.add(label);
+function warnOnce(fn, label, err) {
+  let labels = reported.get(fn);
+  if (labels?.has(label)) return;
+  if (!labels) {
+    labels = new Set();
+    reported.set(fn, labels);
+  }
+  labels.add(label);
+  if (warnings > MAX_WARNINGS) return;
+  warnings++;
   try {
+    if (warnings > MAX_WARNINGS) {
+      console.warn('Patchstack: further failing callbacks passed to createProtection are not reported in this process.');
+
+      return;
+    }
     // Named as the host's callback, not as a Patchstack failure: pointing at ourselves for someone
     // else's throw sends them reading the wrong code.
     console.warn(
       `Patchstack: the ${label} callback passed to createProtection failed and was ignored. ` +
-        `Protection is unaffected; this is reported once per process. ` +
+        `Protection is unaffected; this callback's failures are reported once. ` +
         `Cause: ${err && err.message ? err.message : String(err)}`,
     );
   } catch {
@@ -78,5 +96,6 @@ function warnOnce(label, err) {
 
 /** Test seam: forget which hooks have been reported, so warn-once is assertable more than once. */
 export function resetNotifyWarnings() {
-  reported.clear();
+  reported = new WeakMap();
+  warnings = 0;
 }

src/protect/protect.d.ts

@@ -62,9 +62,9 @@ export interface Protection {
    *   reporter is finished with it, not that the underlying request has ended.
    * - A runtime that terminates the process regardless still wins, whatever this resolves.
    *
-   * What is accounted for also differs by reporter. Every detection event ends up delivered, refused or
-   * dropped, and `detectionHealth()` reports each. Block-log records have no counters at all, so one lost
-   * to a failed token exchange, a failed post, or a shutdown that ran out of time is reported nowhere.
+   * Both reporters account for what they held. Every detection event ends up delivered, refused or
+   * dropped, and `detectionHealth()` reports each; every block-log record ends up delivered, failed or
+   * dropped, and `blockLogHealth()` reports those.
    */
   stop: () => Promise<void>;
   /** Alias of `stop`, under the name callers already have. */
@@ -128,6 +128,20 @@ export interface Protection {
       lastAcknowledgedAt: string | null;
     };
   };
+  /** Present when block-log reporting is on — where the block records went, in records. Carries no
+   *  request data. */
+  blockLogHealth?: () => {
+    /** Records the queue accepted. Each ends up delivered, failed, or dropped by a shutdown. */
+    recorded: number;
+    /** Records in a batch the endpoint acknowledged. */
+    delivered: number;
+    /** Records in a batch that was refused, or that could not be sent (including a failed token exchange). */
+    failed: number;
+    /** Records a shutdown discarded, plus records turned away because the queue was full. */
+    dropped: number;
+    /** Records waiting to be sent now. */
+    queued: number;
+  };
 }
 
 /**

src/protect/rules/refresh.js

@@ -159,7 +159,7 @@ export function makeRefreshHandler(tick, secret) {
     // No secret configured → the endpoint doesn't exist (never an open refresh-DoS surface).
     if (!secret) return new Response('not found', { status: 404 });
     const provided = request?.headers?.get?.('x-patchstack-refresh') ?? null;
-    if (provided !== secret) return new Response('forbidden', { status: 403 });
+    if (!(await sameSecret(provided, secret))) return new Response('forbidden', { status: 403 });
     let refreshed = true;
     try {
       // `{ ok: false }` means the tick ran but the rules did not come from the source, which is not a
@@ -172,3 +172,44 @@ export function makeRefreshHandler(tick, secret) {
     return new Response(JSON.stringify({ refreshed }), { status: 200, headers: { 'content-type': 'application/json' } });
   };
 }
+
+/**
+ * Whether a presented refresh secret equals the configured one, in time that does not depend on where
+ * they first differ.
+ *
+ * Both are digested and the fixed-length digests compared in full, so neither the position of the first
+ * differing character nor the secret's length shapes the time taken. Without Web Crypto the strings are
+ * compared in full over the longer length instead.
+ */
+async function sameSecret(provided, secret) {
+  if (typeof provided !== 'string' || typeof secret !== 'string') return false;
+  const subtle = globalThis.crypto?.subtle;
+  if (subtle && typeof TextEncoder === 'function') {
+    try {
+      const encoder = new TextEncoder();
+      const [a, b] = await Promise.all([
+        subtle.digest('SHA-256', encoder.encode(provided)),
+        subtle.digest('SHA-256', encoder.encode(secret)),
+      ]);
+
+      return equalBytes(new Uint8Array(a), new Uint8Array(b));
+    } catch {
+      // Fall through to the full-length comparison.
+    }
+  }
+  let diff = provided.length ^ secret.length;
+  const length = Math.max(provided.length, secret.length);
+  for (let i = 0; i < length; i++) {
+    diff |= (provided.charCodeAt(i) || 0) ^ (secret.charCodeAt(i) || 0);
+  }
+
+  return diff === 0;
+}
+
+function equalBytes(a, b) {
+  if (a.length !== b.length) return false;
+  let diff = 0;
+  for (let i = 0; i < a.length; i++) diff |= a[i] ^ b[i];
+
+  return diff === 0;
+}

src/protect/runtime.js

@@ -1725,6 +1725,11 @@ export async function createProtection(options = {}) {
     get: () => (detections ? () => detections.health() : undefined),
     enumerable: true,
   });
+  // The same for the block log, when there is one: accepted, delivered, failed, dropped, and still queued.
+  Object.defineProperty(protection, 'blockLogHealth', {
+    get: () => (firewallLog ? () => firewallLog.health() : undefined),
+    enumerable: true,
+  });
 
   return protection;
 }