fix(integrations): silence the Withings scope-skip path from the 3-strike alert ladder

The v1.4.26 scope-skip short-circuit in `sync-activity.ts` and
`sync-sleep.ts` called `recordSyncFailure` for a deliberate no-op,
which incremented `IntegrationStatus.consecutiveFailures` and tripped
the 3-strike admin Telegram alert for every legacy connection. Webhook
deliveries (appli=16 / appli=44) could run the counter from 0 to 3 in
seconds on a freshly-deployed v1.4.26 install, and pre-existing
counters that survived the deploy (e.g. an 87-strike row from the
pre-fix transient burst) paged immediately on the first scope-skip
after the 24h re-alert window closed.

Extract a new `parkIntegrationAtReauth` helper next to
`recordSyncFailure` that sets the row to `error_reauth` without
incrementing the counter, without writing an `integrations.sync.failed`
audit row, and without entering the threshold ladder. A standalone
`integrations.reauth_required` audit row is written on the first park
so the operations trail still shows the event; re-parking the same
scope-skip is idempotent (no extra audit write). Swap the two
deliberate scope-skip call-sites in `withings/sync-activity.ts` and
`withings/sync-sleep.ts` to the new helper.

The defence-in-depth 403 catch-block in each sync routine stays on
`recordSyncFailure` — a 403 reaching the catch IS genuinely unexpected
once the scope-skip lands above, so the alert ladder should still fire
on that asymmetric pair. BL-P3-2 parity verified between activity and
sleep.

Tests extended:
- `integrations/__tests__/admin-alert.test.ts` adds a
  `parkIntegrationAtReauth` block asserting state=error_reauth without
  counter increment, no `dispatchNotification`, and audit-log
  idempotency on a second call.
- `withings/__tests__/sync-activity.test.ts` +
  `withings/__tests__/sync-sleep.test.ts` swap their scope-skip
  assertions to `parkIntegrationAtReauth` and keep the 403 catch-block
  on `recordSyncFailure` to prove the defence-in-depth alert path
  survives.
This commit is contained in:
Marc-André Bombeck committed 2026-05-15 16:30:56 +02:00
1 parent 3ed78a032b
commit e060ab33a2
6 files changed
+293 -34

No files matched your search

@@ -8,10 +8,51 @@
*
* The formatter is a pure function so we can test deterministically
* without touching Prisma or the dispatcher.
*
* v1.4.27 — additionally asserts `parkIntegrationAtReauth` semantics:
* the helper writes the row but neither increments the failure counter
* nor pages admins through the dispatcher. The two scope-skip
* call-sites in `withings/sync-{activity,sleep}.ts` must use this
* helper rather than `recordSyncFailure`.
*/
import { describe, it, expect } from "vitest";
import { describe, it, expect, vi, beforeEach } from "vitest";
import { formatAdminAlertPayload } from "../status";
vi.mock("@/lib/db", () => ({
prisma: {
integrationStatus: {
findUnique: vi.fn(),
upsert: vi.fn(),
update: vi.fn(),
},
user: {
findUnique: vi.fn(),
findMany: vi.fn(),
},
},
}));
vi.mock("@/lib/auth/audit", () => ({
auditLog: vi.fn().mockResolvedValue(undefined),
}));
vi.mock("@/lib/notifications/dispatcher", () => ({
dispatchNotification: vi.fn().mockResolvedValue(undefined),
}));
vi.mock("@/lib/crypto", () => ({
encrypt: (s: string) => `enc(${s})`,
decrypt: (s: string) =>
s.startsWith("enc(") && s.endsWith(")") ? s.slice(4, -1) : s,
}));
import { formatAdminAlertPayload, parkIntegrationAtReauth } from "../status";
import { prisma } from "@/lib/db";
import { auditLog } from "@/lib/auth/audit";
import { dispatchNotification } from "@/lib/notifications/dispatcher";
beforeEach(() => {
vi.resetAllMocks();
});
describe("formatAdminAlertPayload — Withings re-auth", () => {
it("uses the integration display name (Withings, not 'withings')", () => {
@@ -148,3 +189,109 @@ describe("formatAdminAlertPayload — omits errorCode parens when undefined", ()
expect(out.message).toContain("transient error — network reset");
});
});
describe("parkIntegrationAtReauth — silent scope-skip park (v1.4.27 F20)", () => {
it("sets state=error_reauth without incrementing consecutiveFailures and without paging admins", async () => {
// Pre-existing row at counter=87 — the maintainer's reported
// surviving-deploy state. The first scope-skip after the v1.4.27
// deploy must NOT push the row to 88 and must NOT trigger the
// 3-strike alert ladder.
vi.mocked(prisma.integrationStatus.findUnique).mockResolvedValueOnce({
state: "error_transient",
lastError: "enc(old)",
} as never);
vi.mocked(prisma.integrationStatus.upsert).mockResolvedValueOnce(
{} as never,
);
await parkIntegrationAtReauth({
userId: "u1",
integration: "withings",
message: "Withings connection is missing the user.activity scope.",
errorCode: "scope_missing",
});
const upsertArgs = vi.mocked(prisma.integrationStatus.upsert).mock
.calls[0][0];
expect(upsertArgs.update).toMatchObject({
state: "error_reauth",
lastError: "enc(Withings connection is missing the user.activity scope.)",
});
// The whole point of the helper: counter is not in the update set.
expect(upsertArgs.update).not.toHaveProperty("consecutiveFailures");
// No admin alert fired.
expect(dispatchNotification).not.toHaveBeenCalled();
// A standalone reauth audit row IS written so the ops trail still
// shows the park event — but it lands on
// `integrations.reauth_required`, not `integrations.sync.failed`.
expect(auditLog).toHaveBeenCalledWith(
"integrations.reauth_required",
expect.objectContaining({
userId: "u1",
details: expect.objectContaining({
integration: "withings",
errorCode: "scope_missing",
source: "scope_skip",
}),
}),
);
expect(auditLog).not.toHaveBeenCalledWith(
"integrations.sync.failed",
expect.anything(),
);
});
it("is idempotent on the audit log — a second call for the same scope-skip writes no extra audit row", async () => {
// First call: row already parked with the same lastError. The
// helper detects "no fresh park" and skips the audit write.
vi.mocked(prisma.integrationStatus.findUnique).mockResolvedValueOnce({
state: "error_reauth",
lastError: "enc(Withings connection is missing the user.activity scope.)",
} as never);
vi.mocked(prisma.integrationStatus.upsert).mockResolvedValueOnce(
{} as never,
);
await parkIntegrationAtReauth({
userId: "u1",
integration: "withings",
message: "Withings connection is missing the user.activity scope.",
errorCode: "scope_missing",
});
// Upsert still runs (it's the "set lastAttemptAt" touch), but the
// audit log is unchanged.
expect(prisma.integrationStatus.upsert).toHaveBeenCalledOnce();
expect(auditLog).not.toHaveBeenCalled();
expect(dispatchNotification).not.toHaveBeenCalled();
});
it("creates a fresh row at counter=0 when no row exists yet", async () => {
// First-ever sync attempt on a legacy connection: no row in the
// table. Helper must create the row at counter=0 so a later
// genuine transient burst still has the full 3-strike runway.
vi.mocked(prisma.integrationStatus.findUnique).mockResolvedValueOnce(null);
vi.mocked(prisma.integrationStatus.upsert).mockResolvedValueOnce(
{} as never,
);
await parkIntegrationAtReauth({
userId: "u-new",
integration: "withings",
message: "scope missing",
errorCode: "scope_missing",
});
const upsertArgs = vi.mocked(prisma.integrationStatus.upsert).mock
.calls[0][0];
expect(upsertArgs.create).toMatchObject({
userId: "u-new",
integration: "withings",
state: "error_reauth",
consecutiveFailures: 0,
});
expect(dispatchNotification).not.toHaveBeenCalled();
});
});
+74
View File
@@ -270,6 +270,80 @@ export async function recordSyncFailure(
}
}
/**
* Park a connection at `error_reauth` from a deliberate scope-skip
* short-circuit. Unlike `recordSyncFailure`, this helper:
*
* 1. does NOT increment `consecutiveFailures`.
* 2. does NOT write an `integrations.sync.failed` audit row through
* `recordSyncFailure`. A standalone `integrations.reauth_required`
* row is written instead so the operations trail still shows the
* park event.
* 3. does NOT enter the 3-strike alert ladder — no admin Telegram
* page fires.
*
* Idempotent: a second call for the same scope-skip leaves the row at
* `error_reauth` with the same encrypted message and the same counter
* value (no increment). Use it from sync routines that have detected a
* deliberate, structural scope gap (e.g. legacy Withings connection
* missing `user.activity`). The defence-in-depth catch-block path
* stays on `recordSyncFailure` because a 403 reaching the catch is
* genuinely unexpected once the scope-skip lands.
*
* The audit-row-once semantics mean a row is only written if the row
* is not already parked at `error_reauth` with the same `lastError`.
* Re-parking the same scope-skip is a no-op for the audit log.
*/
export async function parkIntegrationAtReauth(opts: {
userId: string;
integration: IntegrationKey;
message: string;
errorCode: string;
}): Promise<void> {
const { userId, integration, message, errorCode } = opts;
const now = new Date();
const encryptedError = safeEncryptError(message);
// Idempotency probe: re-parking the same scope-skip should NOT emit
// another audit row. We read the current row first; only write the
// audit log when the state or error changes.
const existing = await prisma.integrationStatus.findUnique({
where: { userId_integration: { userId, integration } },
select: { state: true, lastError: true },
});
const isFreshPark =
existing?.state !== "error_reauth" || existing.lastError !== encryptedError;
await prisma.integrationStatus.upsert({
where: { userId_integration: { userId, integration } },
create: {
userId,
integration,
state: "error_reauth",
lastError: encryptedError,
lastAttemptAt: now,
// First-ever row for this (user, integration) — counter stays at
// 0 so a later genuine transient burst still has the full
// 3-strike runway before paging.
consecutiveFailures: 0,
},
update: {
state: "error_reauth",
lastError: encryptedError,
lastAttemptAt: now,
// Deliberately omit `consecutiveFailures` — the existing value
// is preserved exactly. This is the whole point of the helper.
},
});
if (isFreshPark) {
await auditLog("integrations.reauth_required", {
userId,
details: { integration, message, errorCode, source: "scope_skip" },
});
}
}
/**
* Mark a connection as needing re-auth without recording a fresh
* "attempt". Used by the OAuth/refresh-token flows when they detect
@@ -22,6 +22,7 @@ vi.mock("@/lib/db", () => ({
vi.mock("@/lib/integrations/status", () => ({
isReauthRequired: vi.fn().mockResolvedValue(false),
parkIntegrationAtReauth: vi.fn(),
recordSyncFailure: vi.fn(),
recordSyncSuccess: vi.fn(),
}));
@@ -45,7 +46,11 @@ vi.mock("@/lib/logging/context", () => ({
}));
import { prisma } from "@/lib/db";
import { recordSyncFailure, recordSyncSuccess } from "@/lib/integrations/status";
import {
parkIntegrationAtReauth,
recordSyncFailure,
recordSyncSuccess,
} from "@/lib/integrations/status";
import { fetchWithingsActivity, syncUserActivity } from "../sync-activity";
@@ -284,7 +289,12 @@ describe("syncUserActivity — scope-skip guard (v1.4.26)", () => {
expect(recordSyncSuccess).not.toHaveBeenCalled();
});
it("parks the connection at error_reauth with kind=reauth_required + errorCode=scope_missing", async () => {
it("parks the connection via parkIntegrationAtReauth (NOT recordSyncFailure) — v1.4.27 F20", async () => {
// The deliberate scope-skip is a no-op park, not a failure burst.
// Calling `recordSyncFailure` here would increment the per-user
// counter and trip the 3-strike admin alert ladder. The v1.4.27
// fix swaps to `parkIntegrationAtReauth`, which leaves the counter
// untouched and never pages admins.
vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({
scope: "user.metrics",
} as never);
@@ -292,18 +302,20 @@ describe("syncUserActivity — scope-skip guard (v1.4.26)", () => {
await syncUserActivity("user-1");
expect(recordSyncFailure).toHaveBeenCalledTimes(1);
expect(recordSyncFailure).toHaveBeenCalledWith(
expect(parkIntegrationAtReauth).toHaveBeenCalledTimes(1);
expect(parkIntegrationAtReauth).toHaveBeenCalledWith(
expect.objectContaining({
userId: "user-1",
integration: "withings",
kind: "reauth_required",
errorCode: "scope_missing",
}),
);
// CRITICAL: the failure path is NOT touched on a scope-skip — that
// is what silences the false 3-strike Telegram alert.
expect(recordSyncFailure).not.toHaveBeenCalled();
});
it("treats a null scope (pre-v1.4.25 connection) as missing user.activity", async () => {
it("treats a null scope (pre-v1.4.25 connection) as missing user.activity and parks silently", async () => {
vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({
scope: null,
} as never);
@@ -314,18 +326,19 @@ describe("syncUserActivity — scope-skip guard (v1.4.26)", () => {
expect(imported).toBe(0);
expect(fetchMock).not.toHaveBeenCalled();
expect(recordSyncFailure).toHaveBeenCalledWith(
expect.objectContaining({ kind: "reauth_required" }),
expect(parkIntegrationAtReauth).toHaveBeenCalledWith(
expect.objectContaining({ errorCode: "scope_missing" }),
);
expect(recordSyncFailure).not.toHaveBeenCalled();
});
it("classifies a Withings 403 in the catch-block as reauth_required (defence-in-depth)", async () => {
// The scope-skip above is the primary guard. This test covers the
// race where Withings revokes scope between OAuth and the next
// sync without invalidating the refresh token — `getValidToken`
// succeeds, the connection scope still claims `user.activity`,
// but the resource call 403s. Must park at reauth_required, not
// transient, so pg-boss stops retrying.
it("classifies a Withings 403 in the catch-block as reauth_required and STILL pages (defence-in-depth)", async () => {
// BL-P3-2 — the defence-in-depth 403 catch-block stays on
// `recordSyncFailure` so a genuinely unexpected 403 (Withings
// revokes scope between OAuth and the next sync without
// invalidating the refresh token) still trips the 3-strike alert.
// This is the asymmetric pair to the scope-skip park above: the
// park is silent, the catch is loud.
vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({
scope: "user.metrics,user.activity",
} as never);
@@ -346,5 +359,7 @@ describe("syncUserActivity — scope-skip guard (v1.4.26)", () => {
errorCode: "403",
}),
);
// The park helper is NOT used for the catch — only the scope-skip.
expect(parkIntegrationAtReauth).not.toHaveBeenCalled();
});
});
+24 -13
View File
@@ -21,6 +21,7 @@ vi.mock("@/lib/db", () => ({
vi.mock("@/lib/integrations/status", () => ({
isReauthRequired: vi.fn().mockResolvedValue(false),
parkIntegrationAtReauth: vi.fn(),
recordSyncFailure: vi.fn(),
recordSyncSuccess: vi.fn(),
}));
@@ -44,7 +45,11 @@ vi.mock("@/lib/logging/context", () => ({
}));
import { prisma } from "@/lib/db";
import { recordSyncFailure, recordSyncSuccess } from "@/lib/integrations/status";
import {
parkIntegrationAtReauth,
recordSyncFailure,
recordSyncSuccess,
} from "@/lib/integrations/status";
import {
fetchWithingsSleep,
@@ -279,7 +284,11 @@ describe("syncUserSleep — scope-skip guard (v1.4.26)", () => {
expect(recordSyncSuccess).not.toHaveBeenCalled();
});
it("parks the connection at error_reauth with kind=reauth_required + errorCode=scope_missing", async () => {
it("parks the connection via parkIntegrationAtReauth (NOT recordSyncFailure) — v1.4.27 F20 + BL-P3-2 parity", async () => {
// BL-P3-2 — sleep mirrors activity. The deliberate scope-skip is a
// no-op park, not a failure burst; calling `recordSyncFailure`
// here would increment the counter and could trip the 3-strike
// admin alert ladder. Swap to `parkIntegrationAtReauth`.
vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({
scope: "user.metrics",
} as never);
@@ -287,18 +296,18 @@ describe("syncUserSleep — scope-skip guard (v1.4.26)", () => {
await syncUserSleep("user-1");
expect(recordSyncFailure).toHaveBeenCalledTimes(1);
expect(recordSyncFailure).toHaveBeenCalledWith(
expect(parkIntegrationAtReauth).toHaveBeenCalledTimes(1);
expect(parkIntegrationAtReauth).toHaveBeenCalledWith(
expect.objectContaining({
userId: "user-1",
integration: "withings",
kind: "reauth_required",
errorCode: "scope_missing",
}),
);
expect(recordSyncFailure).not.toHaveBeenCalled();
});
it("treats a null scope (pre-v1.4.25 connection) as missing user.activity", async () => {
it("treats a null scope (pre-v1.4.25 connection) as missing user.activity and parks silently", async () => {
vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({
scope: null,
} as never);
@@ -309,16 +318,17 @@ describe("syncUserSleep — scope-skip guard (v1.4.26)", () => {
expect(imported).toBe(0);
expect(fetchMock).not.toHaveBeenCalled();
expect(recordSyncFailure).toHaveBeenCalledWith(
expect.objectContaining({ kind: "reauth_required" }),
expect(parkIntegrationAtReauth).toHaveBeenCalledWith(
expect.objectContaining({ errorCode: "scope_missing" }),
);
expect(recordSyncFailure).not.toHaveBeenCalled();
});
it("classifies a Withings 403 in the catch-block as reauth_required (defence-in-depth)", async () => {
// Scope claim says we're fine but the resource call 403s — the
// race where Withings revokes scope without invalidating the
// refresh token. Must park at reauth_required, not transient,
// so pg-boss stops retrying.
it("classifies a Withings 403 in the catch-block as reauth_required and STILL pages (defence-in-depth)", async () => {
// BL-P3-2 — symmetric to sync-activity. The catch-block stays on
// `recordSyncFailure` because a 403 reaching the catch IS
// genuinely unexpected after the scope-skip lands above. The
// 3-strike alert ladder fires here.
vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({
scope: "user.metrics,user.activity",
} as never);
@@ -339,5 +349,6 @@ describe("syncUserSleep — scope-skip guard (v1.4.26)", () => {
errorCode: "403",
}),
);
expect(parkIntegrationAtReauth).not.toHaveBeenCalled();
});
});
+9 -2
View File
@@ -47,6 +47,7 @@ import {
import { getValidToken } from "./sync";
import {
isReauthRequired,
parkIntegrationAtReauth,
recordSyncFailure,
recordSyncSuccess,
} from "@/lib/integrations/status";
@@ -226,10 +227,16 @@ export async function syncUserActivity(
getEvent()?.addWarning(
`withings activity sync skipped for ${userId}: missing user.activity scope (legacy connection — reconnect required)`,
);
await recordSyncFailure({
// v1.4.27 — silent park. The scope-skip is a deliberate no-op, not
// a failure burst, so the 3-strike admin alert must NOT fire from
// this branch. `parkIntegrationAtReauth` sets the row to
// `error_reauth` without incrementing the failure counter and
// without entering the threshold ladder. The defence-in-depth 403
// catch-block below stays on `recordSyncFailure` — a 403 reaching
// the catch IS unexpected once the scope-skip lands.
await parkIntegrationAtReauth({
userId,
integration: "withings",
kind: "reauth_required",
message:
"Withings connection is missing the user.activity scope. Reconnect Withings in Settings to enable activity sync.",
errorCode: "scope_missing",
+7 -2
View File
@@ -45,6 +45,7 @@ import {
import { getValidToken } from "./sync";
import {
isReauthRequired,
parkIntegrationAtReauth,
recordSyncFailure,
recordSyncSuccess,
} from "@/lib/integrations/status";
@@ -173,10 +174,14 @@ export async function syncUserSleep(
getEvent()?.addWarning(
`withings sleep sync skipped for ${userId}: missing user.activity scope (legacy connection — reconnect required)`,
);
await recordSyncFailure({
// v1.4.27 — silent park (BL-P3-2 parity with activity sync). The
// scope-skip is a deliberate no-op; the 3-strike admin alert must
// NOT fire from this branch. The defence-in-depth 403 catch-block
// below stays on `recordSyncFailure` so a genuinely unexpected 403
// after the scope-skip lands still pages admins.
await parkIntegrationAtReauth({
userId,
integration: "withings",
kind: "reauth_required",
message:
"Withings connection is missing the user.activity scope. Reconnect Withings in Settings to enable sleep sync.",
errorCode: "scope_missing",