From db56a33f77cc919113e4861a4d4ca24ef46e3dda Mon Sep 17 00:00:00 2001 From: Thibault Le Ouay Ducasse Date: Sat, 17 Oct 2026 15:20:12 +0200 Subject: [PATCH] fix bugs --- .../monitor/__tests__/monitor.test.ts | 202 ++++++ .../src/routes/rpc/handlers/monitor/errors.ts | 26 + .../src/routes/rpc/handlers/monitor/index.ts | 60 +- .../__tests__/private-location.test.ts | 58 ++ .../rpc/handlers/private-location/index.ts | 13 +- .../status-page/__tests__/status-page.test.ts | 68 ++ .../routes/rpc/handlers/status-page/index.ts | 111 +++- apps/server/src/routes/slack/commands.ts | 5 +- apps/server/src/routes/v1/pages/post.test.ts | 19 + apps/server/src/routes/v1/pages/post.ts | 17 + apps/server/src/routes/v1/pages/put.test.ts | 27 + apps/server/src/routes/v1/pages/put.ts | 18 + .../status-page/src/components/nav/header.tsx | 24 +- .../api/src/lib/subscriber-verification.ts | 49 ++ packages/api/src/router/email/index.ts | 114 +--- .../src/router/statusPage.subscribe.test.ts | 67 ++ packages/api/src/router/statusPage.ts | 5 +- .../__tests__/status-report-impacts.test.ts | 50 ++ .../services/src/agent-tools/status-report.ts | 9 +- .../__tests__/dedupe-and-atomicity.test.ts | 591 ++++++++++++++++++ packages/services/src/import/phase-writers.ts | 462 ++++++++------ packages/services/src/import/preview.ts | 17 +- .../__tests__/postmortem-draft.test.ts | 48 ++ .../services/src/incident/postmortem-draft.ts | 42 +- .../invitation/__tests__/invitation.test.ts | 52 +- packages/services/src/invitation/accept.ts | 35 +- packages/services/src/invitation/create.ts | 2 +- .../src/monitor/__tests__/monitor.test.ts | 69 ++ packages/services/src/monitor/clone.ts | 10 +- packages/services/src/monitor/internal.ts | 32 +- packages/services/src/monitor/relations.ts | 19 +- .../page-subscriber/__tests__/slack.test.ts | 32 + .../services/src/page-subscriber/slack.ts | 8 +- .../services/src/page/__tests__/page.test.ts | 99 +++ packages/services/src/page/create.ts | 5 + packages/services/src/page/index.ts | 1 + packages/services/src/page/internal.ts | 20 + packages/services/src/page/update.ts | 6 + .../services/src/sso/__tests__/sso.test.ts | 77 +++ packages/services/src/sso/client.ts | 1 + packages/services/src/sso/enable.ts | 131 ++-- .../__tests__/status-report.test.ts | 81 ++- packages/services/src/status-report/list.ts | 8 +- packages/services/src/status-report/notify.ts | 8 +- 44 files changed, 2320 insertions(+), 478 deletions(-) create mode 100644 packages/api/src/lib/subscriber-verification.ts create mode 100644 packages/api/src/router/statusPage.subscribe.test.ts create mode 100644 packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts diff --git a/apps/server/src/routes/rpc/handlers/monitor/__tests__/monitor.test.ts b/apps/server/src/routes/rpc/handlers/monitor/__tests__/monitor.test.ts index d6acd4a10..220505808 100644 --- a/apps/server/src/routes/rpc/handlers/monitor/__tests__/monitor.test.ts +++ b/apps/server/src/routes/rpc/handlers/monitor/__tests__/monitor.test.ts @@ -2018,6 +2018,208 @@ describe("MonitorService.TriggerMonitor", () => { await db.delete(monitor).where(eq(monitor.id, icmpMon.id)); } }); + test("succeeds when only some regions fail to dispatch", async () => { + const mon = await createMonitor(1, { + name: `${TEST_PREFIX}-partial-trigger`, + url: "https://example.com", + periodicity: "10m", + active: true, + regions: "ams,fra", + }); + + const realFetch = globalThis.fetch; + const regions: string[] = []; + globalThis.fetch = ((_url: string | URL | Request, init?: RequestInit) => { + const region = new Headers(init?.headers).get("fly-prefer-region") ?? ""; + regions.push(region); + return region === "fra" + ? Promise.reject(new TypeError("network error")) + : Promise.resolve(new Response(null, { status: 200 })); + }) as typeof fetch; + + try { + const res = await connectRequest( + "TriggerMonitor", + { id: String(mon.id) }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(200); + expect(regions.sort()).toEqual(["ams", "fra"]); + } finally { + globalThis.fetch = realFetch; + await db.delete(monitorRun).where(eq(monitorRun.monitorId, mon.id)); + await db.delete(monitor).where(eq(monitor.id, mon.id)); + } + }); + + test("returns Unavailable when no region could be reached", async () => { + const mon = await createMonitor(1, { + name: `${TEST_PREFIX}-failed-trigger`, + url: "https://example.com", + periodicity: "10m", + active: true, + regions: "ams,fra", + }); + + const realFetch = globalThis.fetch; + globalThis.fetch = (() => + Promise.reject(new TypeError("network error"))) as typeof fetch; + + try { + const res = await connectRequest( + "TriggerMonitor", + { id: String(mon.id) }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(503); + expect(res.headers.get("error-reason")).toBe("MONITOR_TRIGGER_FAILED"); + } finally { + globalThis.fetch = realFetch; + await db.delete(monitorRun).where(eq(monitorRun.monitorId, mon.id)); + await db.delete(monitor).where(eq(monitor.id, mon.id)); + } + }); +}); + +// HTTP, TCP and DNS updates skip the validation interceptor like ICMP and gRPC, +// so the same bounds must be enforced in their handlers. +describe("MonitorService - update bounds (HTTP, TCP, DNS)", () => { + const cases = [ + { + method: "UpdateHTTPMonitor", + id: () => testHttpMonitorId, + target: "url", + }, + { + method: "UpdateTCPMonitor", + id: () => testTcpMonitorId, + target: "uri", + }, + { + method: "UpdateDNSMonitor", + id: () => testDnsMonitorId, + target: "uri", + }, + ] as const; + + for (const { method, id, target } of cases) { + test(`${method} rejects out-of-bounds patches`, async () => { + for (const monitor of [ + { retry: "1000000" }, + { retry: "11" }, + { name: "n".repeat(257) }, + { description: "d".repeat(1025) }, + { [target]: "u".repeat(2049) }, + ]) { + const res = await connectRequest( + method, + { id: String(id()), monitor }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(400); + } + }); + + test(`${method} leaves the stored monitor untouched on rejection`, async () => { + const before = await db + .select() + .from(monitor) + .where(eq(monitor.id, id())) + .get(); + + const res = await connectRequest( + method, + { + id: String(id()), + monitor: { name: "should-not-be-written", retry: "1000000" }, + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(400); + + const after = await db + .select() + .from(monitor) + .where(eq(monitor.id, id())) + .get(); + expect(after?.name).toBe(before?.name); + expect(after?.retry).toBe(before?.retry); + }); + } +}); + +// `Number("0x10")` is 16 and `Number("1e3")` is 1000: a loosely parsed id +// silently targets another row instead of failing. +describe("MonitorService - strict id parsing", () => { + const looseIds = (id: number) => [ + `0x${id.toString(16)}`, + `${id}.0`, + `${id}abc`, + `+${id}`, + "", + ]; + + test("DeleteMonitor rejects non-decimal ids without deleting", async () => { + const mon = await createMonitor(1, { + name: `${TEST_PREFIX}-strict-id`, + url: "https://example.com", + periodicity: "10m", + active: true, + regions: "ams", + }); + + try { + for (const id of looseIds(mon.id)) { + const res = await connectRequest( + "DeleteMonitor", + { id }, + { "x-openstatus-key": "1" }, + ); + expect(res.status).toBe(400); + } + + const row = await db + .select() + .from(monitor) + .where(eq(monitor.id, mon.id)) + .get(); + expect(row?.deletedAt).toBeNull(); + } finally { + await db.delete(monitor).where(eq(monitor.id, mon.id)); + } + }); + + test("GetMonitor and UpdateHTTPMonitor reject non-decimal ids", async () => { + for (const id of looseIds(testHttpMonitorId).filter((id) => id !== "")) { + const get = await connectRequest( + "GetMonitor", + { id }, + { "x-openstatus-key": "1" }, + ); + expect(get.status).toBe(400); + expect(get.headers.get("error-reason")).toBe("MONITOR_ID_INVALID"); + + const update = await connectRequest( + "UpdateHTTPMonitor", + { id, monitor: { name: "should-not-be-written" } }, + { "x-openstatus-key": "1" }, + ); + expect(update.status).toBe(400); + } + }); + + test("still accepts a plain decimal id", async () => { + const res = await connectRequest( + "GetMonitor", + { id: String(testHttpMonitorId) }, + { "x-openstatus-key": "1" }, + ); + expect(res.status).toBe(200); + }); }); describe("MonitorService - Authentication", () => { diff --git a/apps/server/src/routes/rpc/handlers/monitor/errors.ts b/apps/server/src/routes/rpc/handlers/monitor/errors.ts index 05faa122e..0055f50ec 100644 --- a/apps/server/src/routes/rpc/handlers/monitor/errors.ts +++ b/apps/server/src/routes/rpc/handlers/monitor/errors.ts @@ -7,10 +7,12 @@ export const ErrorReason = { MONITOR_NOT_FOUND: "MONITOR_NOT_FOUND", MONITOR_REQUIRED: "MONITOR_REQUIRED", MONITOR_ID_REQUIRED: "MONITOR_ID_REQUIRED", + MONITOR_ID_INVALID: "MONITOR_ID_INVALID", MONITOR_CREATE_FAILED: "MONITOR_CREATE_FAILED", MONITOR_UPDATE_FAILED: "MONITOR_UPDATE_FAILED", MONITOR_PARSE_FAILED: "MONITOR_PARSE_FAILED", MONITOR_RUN_CREATE_FAILED: "MONITOR_RUN_CREATE_FAILED", + MONITOR_TRIGGER_FAILED: "MONITOR_TRIGGER_FAILED", MONITOR_INVALID_DATA: "MONITOR_INVALID_DATA", MONITOR_TYPE_MISMATCH: "MONITOR_TYPE_MISMATCH", RESPONSE_LOG_NOT_FOUND: "RESPONSE_LOG_NOT_FOUND", @@ -82,6 +84,30 @@ export function monitorIdRequiredError(): ConnectError { ); } +/** + * Creates an "invalid monitor ID" error for an id that is not a decimal integer. + */ +export function monitorIdInvalidError(monitorId: string): ConnectError { + return createError( + `Invalid monitor id: "${monitorId}"`, + Code.InvalidArgument, + ErrorReason.MONITOR_ID_INVALID, + { "monitor-id": monitorId }, + ); +} + +/** + * Creates a "trigger failed" error when no region could be reached. + */ +export function monitorTriggerFailedError(monitorId: string): ConnectError { + return createError( + "Failed to trigger the monitor in any region", + Code.Unavailable, + ErrorReason.MONITOR_TRIGGER_FAILED, + { "monitor-id": monitorId }, + ); +} + /** * Creates a "monitor type mismatch" error when trying to update with wrong type. */ diff --git a/apps/server/src/routes/rpc/handlers/monitor/index.ts b/apps/server/src/routes/rpc/handlers/monitor/index.ts index 4716537b9..77e7672b2 100644 --- a/apps/server/src/routes/rpc/handlers/monitor/index.ts +++ b/apps/server/src/routes/rpc/handlers/monitor/index.ts @@ -1,4 +1,5 @@ import type { ServiceImpl } from "@connectrpc/connect"; +import { getLogger } from "@logtape/logtape"; import { and, db, eq, isNull, sql } from "@openstatus/db"; import { monitor } from "@openstatus/db/src/schema"; import { selectMonitorSchema } from "@openstatus/db/src/schema/monitors/validation"; @@ -45,6 +46,7 @@ import { } from "../../../../libs/checker"; import { toConnectError, toServiceCtx } from "../../adapter"; import { type RpcContext, getRpcContext } from "../../interceptors"; +import { parseId } from "../../parse"; import { MONITOR_DEFAULTS, dbMonitorToDnsProto, @@ -64,11 +66,13 @@ import { timeRangeToKey, } from "./converters"; import { + monitorIdInvalidError, monitorIdRequiredError, monitorInvalidDataError, monitorNotFoundError, monitorParseFailedError, monitorRequiredError, + monitorTriggerFailedError, monitorTypeMismatchError, rateLimitExceededError, responseLogNotFoundError, @@ -87,6 +91,12 @@ import { validateMonitorPatchBounds, } from "./validators"; +const logger = getLogger("api-server"); + +function parseMonitorId(id: string): number { + return parseId(id, () => monitorIdInvalidError(id)); +} + /** * Helper to get a monitor by ID with workspace scope. */ @@ -119,7 +129,7 @@ async function validateAndGetMonitor( throw monitorIdRequiredError(); } - const dbMon = await getMonitorById(Number(id), workspaceId); + const dbMon = await getMonitorById(parseMonitorId(id), workspaceId); if (!dbMon) { throw monitorNotFoundError(id); } @@ -374,6 +384,9 @@ export const monitorServiceImpl: ServiceImpl = { // Validate regions if provided validateCommonMonitorFields(mon); + // This method skips the protovalidate interceptor (see SKIP_VALIDATION_METHODS), + // so the message's own bounds have to be applied here. + validateMonitorPatchBounds(mon); // Check workspace limits if periodicity or regions are changing checkMonitorConfigLimits( @@ -451,6 +464,9 @@ export const monitorServiceImpl: ServiceImpl = { // Validate regions if provided validateCommonMonitorFields(mon); + // This method skips the protovalidate interceptor (see SKIP_VALIDATION_METHODS), + // so the message's own bounds have to be applied here. + validateMonitorPatchBounds(mon); // Check workspace limits if periodicity or regions are changing checkMonitorConfigLimits( @@ -500,6 +516,9 @@ export const monitorServiceImpl: ServiceImpl = { // Validate regions if provided validateCommonMonitorFields(mon); + // This method skips the protovalidate interceptor (see SKIP_VALIDATION_METHODS), + // so the message's own bounds have to be applied here. + validateMonitorPatchBounds(mon); // Check workspace limits if periodicity or regions are changing checkMonitorConfigLimits( @@ -656,7 +675,7 @@ export const monitorServiceImpl: ServiceImpl = { try { run = await triggerMonitorRun({ ctx: toServiceCtx(rpcCtx), - input: { id: Number(req.id) }, + input: { id: parseMonitorId(req.id) }, }); } catch (err) { if (err instanceof NotFoundError) { @@ -678,8 +697,10 @@ export const monitorServiceImpl: ServiceImpl = { const url = getCheckerUrl(row); const timeout = getCheckerTimeout(row); - // Trigger checks for each region in parallel - await Promise.all( + // Trigger checks for each region in parallel. The run is already recorded + // (and the quota spent), so one unreachable region must not fail the call + // and invite a retry that spends it again. + const results = await Promise.allSettled( row.regions.map((region) => { const status = run.regionStatus.get(region) || "active"; const payload = getCheckerPayload(row, status); @@ -697,6 +718,21 @@ export const monitorServiceImpl: ServiceImpl = { }), ); + const failed = results.flatMap((result, i) => + result.status === "rejected" + ? [{ region: row.regions[i], reason: String(result.reason) }] + : [], + ); + if (failed.length > 0) { + logger.warn("Failed to trigger monitor in some regions", { + monitorId: row.id, + failed, + }); + } + if (results.length > 0 && failed.length === results.length) { + throw monitorTriggerFailedError(req.id); + } + return { success: true }; }, @@ -706,7 +742,7 @@ export const monitorServiceImpl: ServiceImpl = { try { await deleteMonitor({ ctx: toServiceCtx(rpcCtx), - input: { id: Number(req.id) }, + input: { id: parseMonitorId(req.id) }, }); } catch (err) { if (err instanceof NotFoundError) { @@ -804,7 +840,7 @@ export const monitorServiceImpl: ServiceImpl = { try { const result = await getMonitorStatus({ ctx: toServiceCtx(rpcCtx), - input: { monitorId: Number(req.id) }, + input: { monitorId: parseMonitorId(req.id) }, }); const regions: RegionStatus[] = result.regions.map((s) => ({ $typeName: "openstatus.monitor.v1.RegionStatus" as const, @@ -825,7 +861,7 @@ export const monitorServiceImpl: ServiceImpl = { const workspaceId = rpcCtx.workspace.id; // Get the monitor - const dbMon = await getMonitorById(Number(req.id), workspaceId); + const dbMon = await getMonitorById(parseMonitorId(req.id), workspaceId); if (!dbMon) { throw monitorNotFoundError(req.id); } @@ -920,7 +956,7 @@ export const monitorServiceImpl: ServiceImpl = { const result = await getMonitorSummary({ ctx: toServiceCtx(rpcCtx), input: { - monitorId: Number(req.id), + monitorId: parseMonitorId(req.id), timeRange: timeRangeKey, regions: requestedRegions.length > 0 ? requestedRegions : undefined, }, @@ -960,7 +996,7 @@ export const monitorServiceImpl: ServiceImpl = { const result = await listResponseLogs({ ctx: toServiceCtx(rpcCtx), input: { - monitorId: Number(req.id), + monitorId: parseMonitorId(req.id), fromTimestamp: req.fromTimestamp ? Number(req.fromTimestamp) : undefined, @@ -992,7 +1028,7 @@ export const monitorServiceImpl: ServiceImpl = { } if (err instanceof ValidationError) { const jobType = - (await getMonitorById(Number(req.id), rpcCtx.workspace.id)) + (await getMonitorById(parseMonitorId(req.id), rpcCtx.workspace.id)) ?.jobType ?? "unknown"; throw monitorTypeMismatchError(req.id, "http", jobType); } @@ -1005,7 +1041,7 @@ export const monitorServiceImpl: ServiceImpl = { try { const log = await getResponseLog({ ctx: toServiceCtx(rpcCtx), - input: { monitorId: Number(req.id), logId: req.logId }, + input: { monitorId: parseMonitorId(req.id), logId: req.logId }, }); return { $typeName: @@ -1024,7 +1060,7 @@ export const monitorServiceImpl: ServiceImpl = { } if (err instanceof ValidationError) { const jobType = - (await getMonitorById(Number(req.id), rpcCtx.workspace.id)) + (await getMonitorById(parseMonitorId(req.id), rpcCtx.workspace.id)) ?.jobType ?? "unknown"; throw monitorTypeMismatchError(req.id, "http", jobType); } diff --git a/apps/server/src/routes/rpc/handlers/private-location/__tests__/private-location.test.ts b/apps/server/src/routes/rpc/handlers/private-location/__tests__/private-location.test.ts index 791c79d61..d30102b53 100644 --- a/apps/server/src/routes/rpc/handlers/private-location/__tests__/private-location.test.ts +++ b/apps/server/src/routes/rpc/handlers/private-location/__tests__/private-location.test.ts @@ -714,3 +714,61 @@ describe("PrivateLocationService.DeletePrivateLocation", () => { expect(res.status).toBe(401); }); }); + +// `Number.parseInt("1e3")` is 1 and `parseInt("12abc")` is 12: a loosely +// parsed id silently targets another row instead of failing. +describe("PrivateLocationService - strict id parsing", () => { + test("DeletePrivateLocation rejects non-decimal ids without deleting", async () => { + const target = await mkLocation( + `${TEST_PREFIX}-strict-id`, + entitledWorkspaceId, + ); + + try { + for (const id of [ + `${target}abc`, + `${target}.9`, + `${target}e0`, + `0x${target.toString(16)}`, + `-${target}`, + ]) { + const res = await connectRequest( + "DeletePrivateLocation", + { id }, + { "x-openstatus-key": authKey }, + ); + expect(res.status).toBe(400); + } + + const remaining = await db + .select() + .from(privateLocation) + .where(eq(privateLocation.id, target)) + .all(); + expect(remaining.length).toBe(1); + } finally { + await db.delete(privateLocation).where(eq(privateLocation.id, target)); + } + }); + + test("CreatePrivateLocation rejects a monitor id with trailing garbage", async () => { + const res = await connectRequest( + "CreatePrivateLocation", + { + name: `${TEST_PREFIX}-strict-monitor-ids`, + monitorIds: [`${testMonitorId}foo`], + }, + { "x-openstatus-key": authKey }, + ); + + expect(res.status).toBe(400); + expect(res.headers.get("error-reason")).toBe("INVALID_MONITOR_ID"); + + const created = await db + .select() + .from(privateLocation) + .where(eq(privateLocation.name, `${TEST_PREFIX}-strict-monitor-ids`)) + .all(); + expect(created.length).toBe(0); + }); +}); diff --git a/apps/server/src/routes/rpc/handlers/private-location/index.ts b/apps/server/src/routes/rpc/handlers/private-location/index.ts index 7baf2d1da..0187f8aee 100644 --- a/apps/server/src/routes/rpc/handlers/private-location/index.ts +++ b/apps/server/src/routes/rpc/handlers/private-location/index.ts @@ -11,6 +11,7 @@ import { import { toConnectError, toServiceCtx } from "../../adapter"; import { getRpcContext } from "../../interceptors"; +import { parseId as parseRpcId } from "../../parse"; import { dbPrivateLocationToProto, dbPrivateLocationToProtoSummary, @@ -32,20 +33,12 @@ function toPrivateLocationConnectError(err: unknown, id: string): never { toConnectError(err); } -// `Number.parseInt` rather than `Number` — `Number("")` is 0 (finite!), which -// would silently target row 0 instead of failing. function parseId(id: string): number { - const n = Number.parseInt(id, 10); - if (!Number.isFinite(n)) throw privateLocationIdRequiredError(); - return n; + return parseRpcId(id, privateLocationIdRequiredError); } function parseMonitorIds(ids: ReadonlyArray): number[] { - return ids.map((id) => { - const n = Number.parseInt(id, 10); - if (!Number.isFinite(n)) throw invalidMonitorIdError(id); - return n; - }); + return ids.map((id) => parseRpcId(id, () => invalidMonitorIdError(id))); } function requireId(id: string): number { diff --git a/apps/server/src/routes/rpc/handlers/status-page/__tests__/status-page.test.ts b/apps/server/src/routes/rpc/handlers/status-page/__tests__/status-page.test.ts index 616b72a16..21d7ba335 100644 --- a/apps/server/src/routes/rpc/handlers/status-page/__tests__/status-page.test.ts +++ b/apps/server/src/routes/rpc/handlers/status-page/__tests__/status-page.test.ts @@ -4720,3 +4720,71 @@ describe("StatusPageService — IP restriction in read responses", () => { .where(eq(page.id, testPasswordPageId)); }); }); + +// `Number("0x10")` is 16 and `Number("1e3")` is 1000: a loosely parsed id +// silently targets another row instead of failing. +describe("StatusPageService - strict id parsing", () => { + test("DeleteStatusPage rejects non-decimal ids without deleting", async () => { + const target = await db + .insert(page) + .values({ + workspaceId: 1, + title: `${TEST_PREFIX}-strict-id`, + slug: `${TEST_PREFIX}-strict-id-${Date.now()}`, + description: "Strict id parsing", + customDomain: "", + }) + .returning() + .get(); + + try { + for (const id of [ + `0x${target.id.toString(16)}`, + `${target.id}.0`, + `${target.id}abc`, + `+${target.id}`, + ]) { + const res = await connectRequest( + "DeleteStatusPage", + { id }, + { "x-openstatus-key": "1" }, + ); + expect(res.status).toBe(400); + } + + const row = await db + .select() + .from(page) + .where(eq(page.id, target.id)) + .get(); + expect(row).toBeDefined(); + } finally { + await db.delete(page).where(eq(page.id, target.id)); + } + }); + + test("GetStatusPage rejects a non-decimal id", async () => { + const res = await connectRequest( + "GetStatusPage", + { id: `${testPageId}abc` }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(400); + }); + + test("AddMonitorComponent rejects non-decimal page and monitor ids", async () => { + for (const body of [ + { + pageId: `0x${testPageId.toString(16)}`, + monitorId: String(testMonitorId), + }, + { pageId: String(testPageId), monitorId: `${testMonitorId}e0` }, + ]) { + const res = await connectRequest("AddMonitorComponent", body, { + "x-openstatus-key": "1", + }); + expect(res.status).toBe(400); + } + }); +}); diff --git a/apps/server/src/routes/rpc/handlers/status-page/index.ts b/apps/server/src/routes/rpc/handlers/status-page/index.ts index 06d77cc5a..34e11f121 100644 --- a/apps/server/src/routes/rpc/handlers/status-page/index.ts +++ b/apps/server/src/routes/rpc/handlers/status-page/index.ts @@ -82,6 +82,7 @@ import { import { toConnectError, toServiceCtx } from "../../adapter"; import { getRpcContext } from "../../interceptors"; +import { parseId } from "../../parse"; import { dbImpactToProto } from "../status-report/converters"; import { dayStatusToProto, @@ -157,6 +158,14 @@ function serviceToConverterPage< /** * Helper to get a status page by ID with workspace scope. */ +function parseEntityId(value: string, label: string): number { + return parseId( + value, + () => + new ConnectError(`Invalid ${label}: "${value}"`, Code.InvalidArgument), + ); +} + async function getPageById(id: number, workspaceId: number) { return db .select() @@ -314,7 +323,10 @@ async function getGroupForPage( workspaceId: number, pageId: number, ) { - const group = await getGroupById(Number(groupId), workspaceId); + const group = await getGroupById( + parseEntityId(groupId, "group id"), + workspaceId, + ); if (!group || group.pageId !== pageId) { throw componentGroupNotFoundError(groupId); } @@ -670,7 +682,7 @@ export const statusPageServiceImpl: ServiceImpl = { try { const pageData = await getPage({ ctx: toServiceCtx(rpcCtx), - input: { id: Number(id) }, + input: { id: parseEntityId(id, "page id") }, }); return { statusPage: dbPageToProto(serviceToConverterPage(pageData)), @@ -719,7 +731,7 @@ export const statusPageServiceImpl: ServiceImpl = { const id = req.id?.trim(); if (!id) throw statusPageIdRequiredError(); - const pageId = Number(id); + const pageId = parseEntityId(id, "page id"); // i18n — keep at handler to preserve PermissionDenied. if (!limits.i18n) { @@ -1008,7 +1020,7 @@ export const statusPageServiceImpl: ServiceImpl = { try { await deletePage({ ctx: toServiceCtx(rpcCtx), - input: { id: Number(id) }, + input: { id: parseEntityId(id, "page id") }, }); } catch (err) { if (err instanceof NotFoundError) throw statusPageNotFoundError(id); @@ -1031,7 +1043,10 @@ export const statusPageServiceImpl: ServiceImpl = { const limits = rpcCtx.workspace.limits; // Verify page exists and belongs to workspace - const pageData = await getPageById(Number(req.pageId), workspaceId); + const pageData = await getPageById( + parseEntityId(req.pageId, "page id"), + workspaceId, + ); if (!pageData) { throw statusPageNotFoundError(req.pageId); } @@ -1041,7 +1056,7 @@ export const statusPageServiceImpl: ServiceImpl = { // Verify monitor exists and belongs to workspace const monitorData = await getMonitorById( - Number(req.monitorId), + parseEntityId(req.monitorId, "monitor id"), workspaceId, ); if (!monitorData) { @@ -1063,7 +1078,7 @@ export const statusPageServiceImpl: ServiceImpl = { name: req.name ?? monitorData.name, description: req.description ?? null, order: req.order ?? 0, - groupId: req.groupId ? Number(req.groupId) : null, + groupId: req.groupId ? parseEntityId(req.groupId, "group id") : null, }, }); @@ -1079,7 +1094,10 @@ export const statusPageServiceImpl: ServiceImpl = { const limits = rpcCtx.workspace.limits; // Verify page exists and belongs to workspace - const pageData = await getPageById(Number(req.pageId), workspaceId); + const pageData = await getPageById( + parseEntityId(req.pageId, "page id"), + workspaceId, + ); if (!pageData) { throw statusPageNotFoundError(req.pageId); } @@ -1102,7 +1120,7 @@ export const statusPageServiceImpl: ServiceImpl = { name: req.name, description: req.description ?? null, order: req.order ?? 0, - groupId: req.groupId ? Number(req.groupId) : null, + groupId: req.groupId ? parseEntityId(req.groupId, "group id") : null, }, }); @@ -1122,7 +1140,10 @@ export const statusPageServiceImpl: ServiceImpl = { } try { - await deletePageComponent({ ctx: sCtx, input: { id: Number(id) } }); + await deletePageComponent({ + ctx: sCtx, + input: { id: parseEntityId(id, "component id") }, + }); } catch (err) { if (err instanceof NotFoundError) { throw pageComponentNotFoundError(id); @@ -1142,7 +1163,10 @@ export const statusPageServiceImpl: ServiceImpl = { throw pageComponentNotFoundError(req.id); } - const component = await getComponentById(Number(id), workspaceId); + const component = await getComponentById( + parseEntityId(id, "component id"), + workspaceId, + ); if (!component) { throw pageComponentNotFoundError(id); } @@ -1167,7 +1191,7 @@ export const statusPageServiceImpl: ServiceImpl = { req.groupId !== undefined ? req.groupId === "" ? null - : Number(req.groupId) + : parseEntityId(req.groupId, "group id") : undefined, groupOrder: req.groupOrder, }, @@ -1185,7 +1209,10 @@ export const statusPageServiceImpl: ServiceImpl = { const id = req.id?.trim(); if (!id) throw pageComponentNotFoundError(req.id); - const component = await getComponentById(Number(id), rpcCtx.workspace.id); + const component = await getComponentById( + parseEntityId(id, "component id"), + rpcCtx.workspace.id, + ); if (!component) throw pageComponentNotFoundError(id); return { component: dbComponentToProto(component) }; @@ -1203,7 +1230,10 @@ export const statusPageServiceImpl: ServiceImpl = { const workspaceId = rpcCtx.workspace.id; // Verify page exists and belongs to workspace - const pageData = await getPageById(Number(req.pageId), workspaceId); + const pageData = await getPageById( + parseEntityId(req.pageId, "page id"), + workspaceId, + ); if (!pageData) { throw statusPageNotFoundError(req.pageId); } @@ -1233,7 +1263,10 @@ export const statusPageServiceImpl: ServiceImpl = { throw componentGroupNotFoundError(req.id); } - const group = await getGroupById(Number(id), workspaceId); + const group = await getGroupById( + parseEntityId(id, "group id"), + workspaceId, + ); if (!group) { throw componentGroupNotFoundError(id); } @@ -1259,7 +1292,10 @@ export const statusPageServiceImpl: ServiceImpl = { throw componentGroupNotFoundError(req.id); } - const group = await getGroupById(Number(id), workspaceId); + const group = await getGroupById( + parseEntityId(id, "group id"), + workspaceId, + ); if (!group) { throw componentGroupNotFoundError(id); } @@ -1292,7 +1328,10 @@ export const statusPageServiceImpl: ServiceImpl = { checkStatusSubscribersLimit(limits); - const pageData = await getPageById(Number(req.pageId), workspaceId); + const pageData = await getPageById( + parseEntityId(req.pageId, "page id"), + workspaceId, + ); if (!pageData) { throw statusPageNotFoundError(req.pageId); } @@ -1353,8 +1392,10 @@ export const statusPageServiceImpl: ServiceImpl = { const rpcCtx = getRpcContext(ctx); const sCtx = toServiceCtx(rpcCtx); - const pageId = Number(req.pageId); - const componentIds = (req.componentIds ?? []).map((id) => Number(id)); + const pageId = parseEntityId(req.pageId, "page id"); + const componentIds = (req.componentIds ?? []).map((id) => + parseEntityId(id, "component id"), + ); const name = req.name ?? null; if ( @@ -1419,7 +1460,10 @@ export const statusPageServiceImpl: ServiceImpl = { const workspaceId = rpcCtx.workspace.id; // Verify page exists and belongs to workspace - const pageData = await getPageById(Number(req.pageId), workspaceId); + const pageData = await getPageById( + parseEntityId(req.pageId, "page id"), + workspaceId, + ); if (!pageData) { throw statusPageNotFoundError(req.pageId); } @@ -1431,7 +1475,10 @@ export const statusPageServiceImpl: ServiceImpl = { const identifier = req.identifier.case === "email" ? { type: "email" as const, value: req.identifier.value } - : { type: "id" as const, value: Number(req.identifier.value) }; + : { + type: "id" as const, + value: parseEntityId(req.identifier.value, "subscriber id"), + }; try { await unsubscribePageSubscriber({ @@ -1453,7 +1500,10 @@ export const statusPageServiceImpl: ServiceImpl = { const workspaceId = rpcCtx.workspace.id; // Verify page exists and belongs to workspace - const pageData = await getPageById(Number(req.pageId), workspaceId); + const pageData = await getPageById( + parseEntityId(req.pageId, "page id"), + workspaceId, + ); if (!pageData) { throw statusPageNotFoundError(req.pageId); } @@ -1508,7 +1558,10 @@ export const statusPageServiceImpl: ServiceImpl = { const rpcCtx = getRpcContext(ctx); const workspaceId = rpcCtx.workspace.id; identifierValue = req.identifier.value; - pageData = await getPageById(Number(identifierValue), workspaceId); + pageData = await getPageById( + parseEntityId(identifierValue, "page id"), + workspaceId, + ); } else if (req.identifier.case === "slug") { identifierValue = req.identifier.value; pageData = await getPageBySlug({ input: { slug: identifierValue } }); @@ -1553,7 +1606,10 @@ export const statusPageServiceImpl: ServiceImpl = { const rpcCtx = getRpcContext(ctx); const workspaceId = rpcCtx.workspace.id; identifierValue = req.identifier.value; - pageData = await getPageById(Number(identifierValue), workspaceId); + pageData = await getPageById( + parseEntityId(identifierValue, "page id"), + workspaceId, + ); } else if (req.identifier.case === "slug") { identifierValue = req.identifier.value; pageData = await getPageBySlug({ input: { slug: identifierValue } }); @@ -1579,7 +1635,10 @@ export const statusPageServiceImpl: ServiceImpl = { async getStatusPageOverview(req, ctx) { try { const rpcCtx = getRpcContext(ctx); - const pageData = await getPageById(Number(req.id), rpcCtx.workspace.id); + const pageData = await getPageById( + parseEntityId(req.id, "page id"), + rpcCtx.workspace.id, + ); if (!pageData) { throw statusPageNotFoundError(req.id); @@ -1622,7 +1681,7 @@ export const statusPageServiceImpl: ServiceImpl = { if (req.identifier.case === "id") { identifierValue = req.identifier.value; pageData = await getPageById( - Number(identifierValue), + parseEntityId(identifierValue, "page id"), rpcCtx.workspace.id, ); } else if (req.identifier.case === "slug") { diff --git a/apps/server/src/routes/slack/commands.ts b/apps/server/src/routes/slack/commands.ts index d86c6aeeb..3c334e438 100644 --- a/apps/server/src/routes/slack/commands.ts +++ b/apps/server/src/routes/slack/commands.ts @@ -1,5 +1,5 @@ import { getLogger } from "@logtape/logtape"; -import { ForbiddenError } from "@openstatus/services"; +import { ForbiddenError, PreconditionFailedError } from "@openstatus/services"; import { createSlackSubscriber, listSlackSubscribersForChannel, @@ -251,6 +251,9 @@ async function runCommand(command: SlashCommand): Promise { } return `📡 This channel is now subscribed to *${page.title}*. Incident updates will appear here.`; } catch (error) { + if (error instanceof PreconditionFailedError) { + return `*${page.title}* is a protected status page and can't be subscribed to from Slack.`; + } if (error instanceof ForbiddenError) { return `*${page.title}* isn't on a plan that supports subscribers.`; } diff --git a/apps/server/src/routes/v1/pages/post.test.ts b/apps/server/src/routes/v1/pages/post.test.ts index e57a260c0..ae17a0a51 100644 --- a/apps/server/src/routes/v1/pages/post.test.ts +++ b/apps/server/src/routes/v1/pages/post.test.ts @@ -459,3 +459,22 @@ test("create a page uses monitor externalName when available", async () => { await db.delete(page).where(eq(page.id, result.data.id)); } }); + +test("create a page with a custom domain another page uses should return 409", async () => { + const res = await app.request("/v1/page", { + method: "POST", + headers: { + "x-openstatus-key": "1", + "content-type": "application/json", + }, + body: JSON.stringify({ + title: "OpenStatus", + description: "OpenStatus website", + slug: `taken-domain-${Date.now()}`, + // Seeded on page 2. + customDomain: "STATUS.ACME.COM", + }), + }); + + expect(res.status).toBe(409); +}); diff --git a/apps/server/src/routes/v1/pages/post.ts b/apps/server/src/routes/v1/pages/post.ts index 260b56e28..432336fe1 100644 --- a/apps/server/src/routes/v1/pages/post.ts +++ b/apps/server/src/routes/v1/pages/post.ts @@ -8,6 +8,8 @@ import { pageComponent, subdomainSafeList, } from "@openstatus/db/src/schema"; +import { ConflictError } from "@openstatus/services"; +import { assertCustomDomainAvailable } from "@openstatus/services/page"; import { OpenStatusApiError, openApiErrorResponses } from "@/libs/errors"; import { trackMiddleware } from "@/libs/middlewares"; @@ -65,6 +67,21 @@ export function registerPostPage(api: typeof pagesApi) { }); } + try { + await assertCustomDomainAvailable({ + tx: db, + customDomain: input.customDomain, + }); + } catch (err) { + if (err instanceof ConflictError) { + throw new OpenStatusApiError({ + code: "CONFLICT", + message: err.message, + }); + } + throw err; + } + if ( !limits["password-protection"] && (input?.passwordProtected || input?.password) diff --git a/apps/server/src/routes/v1/pages/put.test.ts b/apps/server/src/routes/v1/pages/put.test.ts index 0c76046de..88ef01e54 100644 --- a/apps/server/src/routes/v1/pages/put.test.ts +++ b/apps/server/src/routes/v1/pages/put.test.ts @@ -92,3 +92,30 @@ test("no auth key should return 401", async () => { }); expect(res.status).toBe(401); }); + +test("update the page with a custom domain another page uses should return 409", async () => { + const res = await app.request("/v1/page/1", { + method: "PUT", + headers: { + "x-openstatus-key": "1", + "Content-Type": "application/json", + }, + // Seeded on page 2. + body: JSON.stringify({ customDomain: "status.acme.com" }), + }); + + expect(res.status).toBe(409); +}); + +test("update the page keeping its own custom domain", async () => { + const res = await app.request("/v1/page/2", { + method: "PUT", + headers: { + "x-openstatus-key": "1", + "Content-Type": "application/json", + }, + body: JSON.stringify({ customDomain: "status.acme.com" }), + }); + + expect(res.status).toBe(200); +}); diff --git a/apps/server/src/routes/v1/pages/put.ts b/apps/server/src/routes/v1/pages/put.ts index 7911e1482..b76eb905f 100644 --- a/apps/server/src/routes/v1/pages/put.ts +++ b/apps/server/src/routes/v1/pages/put.ts @@ -8,6 +8,8 @@ import { pageComponent, subdomainSafeList, } from "@openstatus/db/src/schema"; +import { ConflictError } from "@openstatus/services"; +import { assertCustomDomainAvailable } from "@openstatus/services/page"; import { OpenStatusApiError, openApiErrorResponses } from "@/libs/errors"; import { trackMiddleware } from "@/libs/middlewares"; @@ -68,6 +70,22 @@ export function registerPutPage(api: typeof pagesApi) { }); } + try { + await assertCustomDomainAvailable({ + tx: db, + customDomain: input.customDomain, + excludePageId: Number(id), + }); + } catch (err) { + if (err instanceof ConflictError) { + throw new OpenStatusApiError({ + code: "CONFLICT", + message: err.message, + }); + } + throw err; + } + if ( limits["password-protection"] === false && input?.passwordProtected === true diff --git a/apps/status-page/src/components/nav/header.tsx b/apps/status-page/src/components/nav/header.tsx index 1d24d8a6b..36564b29d 100644 --- a/apps/status-page/src/components/nav/header.tsx +++ b/apps/status-page/src/components/nav/header.tsx @@ -22,12 +22,10 @@ import { } from "@openstatus/ui/components/ui/sheet"; import { cn } from "@openstatus/ui/lib/utils"; import { useMutation, useQuery } from "@tanstack/react-query"; -import { isTRPCClientError } from "@trpc/client"; import { useExtracted } from "next-intl"; import NextLink from "next/link"; import { useParams, usePathname } from "next/navigation"; import { useState } from "react"; -import { toast } from "sonner"; import { usePathnamePrefix } from "../../hooks/use-pathname-prefix"; import { useTRPC } from "../../lib/trpc/client"; @@ -73,28 +71,8 @@ export function Header({ }); const prefix = usePathnamePrefix(); - const sendPageSubscriptionMutation = useMutation( - trpc.emailRouter.sendPageSubscriptionVerification.mutationOptions({}), - ); - const subscribeMutation = useMutation( - trpc.statusPage.subscribe.mutationOptions({ - onSuccess: (data) => { - if (!data?.id || !data?.token) return; - sendPageSubscriptionMutation.mutate( - { id: data.id, token: data.token }, - { - onError: (error) => { - if (isTRPCClientError(error)) { - toast.error(error.message); - } else { - toast.error(t("Failed to subscribe")); - } - }, - }, - ); - }, - }), + trpc.statusPage.subscribe.mutationOptions({}), ); return ( diff --git a/packages/api/src/lib/subscriber-verification.ts b/packages/api/src/lib/subscriber-verification.ts new file mode 100644 index 000000000..34b3472c7 --- /dev/null +++ b/packages/api/src/lib/subscriber-verification.ts @@ -0,0 +1,49 @@ +import { type db as defaultDb, eq } from "@openstatus/db"; +import { pageSubscriber } from "@openstatus/db/src/schema"; +import { getChannel } from "@openstatus/subscriptions"; +import { TRPCError } from "@trpc/server"; + +/** + * Emails the verification link to the subscriber's own address. The token + * must never be handed back to the caller, or double opt-in is meaningless. + */ +export async function sendSubscriberVerification( + db: typeof defaultDb, + subscriberId: number, +): Promise { + const subscriber = await db.query.pageSubscriber.findFirst({ + where: eq(pageSubscriber.id, subscriberId), + with: { page: true }, + }); + + if (!subscriber?.email || !subscriber.token || subscriber.acceptedAt) { + return; + } + + const verifyUrl = subscriber.page.customDomain + ? `https://${subscriber.page.customDomain}/verify/${subscriber.token}` + : `https://${subscriber.page.slug}.openstatus.dev/verify/${subscriber.token}`; + + const channel = getChannel("email"); + if (!channel) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Email channel not found", + }); + } + + await channel.sendVerification?.( + { + id: subscriber.id, + pageId: subscriber.pageId, + pageName: subscriber.page.title, + pageSlug: subscriber.page.slug, + customDomain: subscriber.page.customDomain, + componentIds: [], + channelType: "email", + email: subscriber.email, + token: subscriber.token, + }, + verifyUrl, + ); +} diff --git a/packages/api/src/router/email/index.ts b/packages/api/src/router/email/index.ts index a601db9cd..934d5b52a 100644 --- a/packages/api/src/router/email/index.ts +++ b/packages/api/src/router/email/index.ts @@ -1,124 +1,14 @@ import { and, eq } from "@openstatus/db"; -import { - invitation, - pageSubscriber, - selectWorkspaceSchema, -} from "@openstatus/db/src/schema"; +import { invitation } from "@openstatus/db/src/schema"; import { EmailClient } from "@openstatus/emails"; -import { getChannel } from "@openstatus/subscriptions"; -import { TRPCError } from "@trpc/server"; import { z } from "zod"; import { env } from "../../env"; -import { - createTRPCRouter, - protectedProcedure, - publicProcedure, -} from "../../trpc"; +import { createTRPCRouter, protectedProcedure } from "../../trpc"; const emailClient = new EmailClient({ apiKey: env.RESEND_API_KEY }); export const emailRouter = createTRPCRouter({ - /** - * PUBLIC: Send verification email for a new page subscription - * Called after upsert to trigger the verification flow - */ - sendPageSubscriptionVerification: publicProcedure - .input( - z.object({ - id: z.number().int().positive(), - token: z.uuid(), - }), - ) - .mutation(async (opts) => { - const subscriber = await opts.ctx.db.query.pageSubscriber.findFirst({ - where: and( - eq(pageSubscriber.id, opts.input.id), - eq(pageSubscriber.token, opts.input.token), - ), - with: { - page: { - with: { - workspace: true, - }, - }, - }, - }); - - if (!subscriber) { - throw new TRPCError({ - code: "NOT_FOUND", - message: "Subscriber not found", - }); - } - - const workspace = selectWorkspaceSchema.safeParse( - subscriber.page?.workspace, - ); - - if (!workspace.success) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Invalid workspace", - }); - } - - if (!workspace.data.limits["status-subscribers"]) { - return; - } - - if (!subscriber.email) { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "No email associated with this subscription", - }); - } - - if (subscriber.acceptedAt) { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Subscription already verified", - }); - } - - if (!subscriber.token) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Subscription has no verification token", - }); - } - - const verifyUrl = subscriber.page.customDomain - ? `https://${subscriber.page.customDomain}/verify/${subscriber.token}` - : `https://${subscriber.page.slug}.openstatus.dev/verify/${subscriber.token}`; - - const channel = getChannel("email"); - if (!channel) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Email channel not found", - }); - } - - await channel.sendVerification?.( - { - id: subscriber.id, - pageId: subscriber.pageId, - pageName: subscriber.page.title, - pageSlug: subscriber.page.slug, - customDomain: subscriber.page.customDomain, - componentIds: [], - channelType: "email", - email: subscriber.email, - token: subscriber.token, - acceptedAt: subscriber.acceptedAt ?? undefined, - }, - verifyUrl, - ); - - return { success: true }; - }), - sendTeamInvitation: protectedProcedure .input(z.object({ id: z.number() })) .mutation(async (opts) => { diff --git a/packages/api/src/router/statusPage.subscribe.test.ts b/packages/api/src/router/statusPage.subscribe.test.ts new file mode 100644 index 000000000..67559e284 --- /dev/null +++ b/packages/api/src/router/statusPage.subscribe.test.ts @@ -0,0 +1,67 @@ +import { db, eq } from "@openstatus/db"; +import { page, pageSubscriber } from "@openstatus/db/src/schema"; +import { + createPage, + createTestWorkspace, +} from "@openstatus/db/src/test/factories"; +import { expect } from "@std/expect"; +import { afterAll, beforeAll, describe, test } from "@std/testing/bdd"; +import { NextRequest } from "next/server.js"; + +import { appRouter } from "../root"; +import { createInnerTRPCContext } from "../trpc"; + +let workspaceId: number; +let pageId: number; +let slug: string; + +function caller() { + return appRouter.createCaller( + createInnerTRPCContext({ + req: new NextRequest("http://rsc.internal"), + session: null, + }), + ); +} + +beforeAll(async () => { + const fixture = await createTestWorkspace(); + workspaceId = fixture.workspace.id; + const _page = await createPage(workspaceId); + pageId = _page.id; + slug = _page.slug; +}); + +afterAll(async () => { + await db.delete(pageSubscriber).where(eq(pageSubscriber.pageId, pageId)); + await db.delete(page).where(eq(page.workspaceId, workspaceId)); +}); + +describe("statusPage.subscribe", () => { + test("never returns the verification token to the caller", async () => { + const result = await caller().statusPage.subscribe({ + slug, + email: "someone-else@example.com", + subscribeComponents: false, + pageComponents: [], + }); + + expect(result?.id).toBeDefined(); + expect(result).not.toHaveProperty("token"); + + const row = await db.query.pageSubscriber.findFirst({ + where: eq(pageSubscriber.id, result?.id ?? -1), + }); + expect(row?.token).toBeTruthy(); + expect(row?.acceptedAt).toBeNull(); + expect(JSON.stringify(result)).not.toContain(row?.token ?? ""); + }); + + test("the public token-based verification email endpoint is gone", () => { + expect( + Object.keys(appRouter._def.procedures).filter((p) => + p.startsWith("emailRouter."), + ), + ).not.toContain("emailRouter.sendPageSubscriptionVerification"); + }); +}); diff --git a/packages/api/src/router/statusPage.ts b/packages/api/src/router/statusPage.ts index dba65b14c..03988ac11 100644 --- a/packages/api/src/router/statusPage.ts +++ b/packages/api/src/router/statusPage.ts @@ -35,6 +35,7 @@ import { resolvePageAccess, visitorFromCtx, } from "../lib/page-access"; +import { sendSubscriberVerification } from "../lib/subscriber-verification"; import { createTRPCRouter, publicProcedure } from "../trpc"; import { type StatusData, @@ -1447,7 +1448,9 @@ export const statusPageRouter = createTRPCRouter({ }); } - return { id: subscription.id, token: subscription.token }; + await sendSubscriberVerification(opts.ctx.db, subscription.id); + + return { id: subscription.id }; }), getSubscriptionByToken: publicProcedure diff --git a/packages/services/src/agent-tools/__tests__/status-report-impacts.test.ts b/packages/services/src/agent-tools/__tests__/status-report-impacts.test.ts index 409636b77..2d0907dd4 100644 --- a/packages/services/src/agent-tools/__tests__/status-report-impacts.test.ts +++ b/packages/services/src/agent-tools/__tests__/status-report-impacts.test.ts @@ -467,3 +467,53 @@ describe("add_status_report_update carries prior impacts forward", () => { }); }); }); + +describe("status-report agent tools: notified", () => { + test("a plan without subscribers reports notified: false on all three tools", async () => { + await withTestTransaction(async (tx) => { + const base = { ...teamCtx, db: tx }; + const ctx: ServiceContext = { + ...base, + workspace: { + ...base.workspace, + limits: { ...base.workspace.limits, "status-subscribers": false }, + }, + }; + const { pageId } = await seedPageWithComponents(tx, ctx.workspace.id); + + const created = await agentTools.create_status_report.run({ + ctx, + input: agentTools.create_status_report.inputSchema.parse({ + title: `${TEST_PREFIX}-notified`, + status: "investigating", + message: "down", + pageId, + pageComponentIds: [], + notify: true, + }), + }); + expect(created.notified).toBe(false); + + const added = await agentTools.add_status_report_update.run({ + ctx, + input: agentTools.add_status_report_update.inputSchema.parse({ + statusReportId: created.statusReport.id, + status: "monitoring", + message: "recovering", + notify: true, + }), + }); + expect(added.notified).toBe(false); + + const resolved = await agentTools.resolve_status_report.run({ + ctx, + input: agentTools.resolve_status_report.inputSchema.parse({ + statusReportId: created.statusReport.id, + message: "all good", + notify: true, + }), + }); + expect(resolved.notified).toBe(false); + }); + }); +}); diff --git a/packages/services/src/agent-tools/status-report.ts b/packages/services/src/agent-tools/status-report.ts index fd0c6d649..f19cb1bb2 100644 --- a/packages/services/src/agent-tools/status-report.ts +++ b/packages/services/src/agent-tools/status-report.ts @@ -398,11 +398,10 @@ export const createStatusReportTool: AgentTool< let notified = false; if (input.notify) { try { - await notifyStatusReport({ + notified = await notifyStatusReport({ ctx, input: { statusReportUpdateId: result.initialUpdate.id }, }); - notified = true; } catch (err) { console.warn( "notifyStatusReport failed after create_status_report", @@ -525,11 +524,10 @@ export const addStatusReportUpdateTool: AgentTool< let notified = false; if (input.notify) { try { - await notifyStatusReport({ + notified = await notifyStatusReport({ ctx, input: { statusReportUpdateId: result.statusReportUpdate.id }, }); - notified = true; } catch (err) { console.warn( "notifyStatusReport failed after add_status_report_update", @@ -705,11 +703,10 @@ export const resolveStatusReportTool: AgentTool< let notified = false; if (input.notify) { try { - await notifyStatusReport({ + notified = await notifyStatusReport({ ctx, input: { statusReportUpdateId: result.statusReportUpdate.id }, }); - notified = true; } catch (err) { console.warn( "notifyStatusReport failed after resolve_status_report", diff --git a/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts b/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts new file mode 100644 index 000000000..6c7b39540 --- /dev/null +++ b/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts @@ -0,0 +1,591 @@ +import { db, eq, inArray } from "@openstatus/db"; +import { + maintenance, + maintenanceUpdate, + monitor, + page, + pageComponent, + pageComponentGroup, + statusReport, + statusReportUpdate, +} from "@openstatus/db/src/schema"; +import type { PhaseResult } from "@openstatus/importers"; +import { expect } from "@std/expect"; +import { afterAll, beforeAll, describe, test } from "@std/testing/bdd"; + +import { + clearAuditLogFor, + createWorkspaceFixture, + makeUserCtx, + withTestTransaction, +} from "../../../test/helpers"; +import type { DB, ServiceContext } from "../../context"; +import { NotFoundError } from "../../errors"; +import { + writeComponentsPhase, + writeIncidentsPhase, + writeMaintenancesPhase, + writeMonitorsPhase, + writePagePhase, +} from "../phase-writers"; +import { previewImport } from "../preview"; +import { PreviewImportInput } from "../schemas"; + +const TEST_PREFIX = "svc-import-dedupe"; + +let teamCtx: ServiceContext; +let otherCtx: ServiceContext; + +beforeAll(async () => { + const team = await createWorkspaceFixture("team"); + teamCtx = makeUserCtx(team.workspace, { userId: team.userId }); + const other = await createWorkspaceFixture("team"); + otherCtx = makeUserCtx(other.workspace, { userId: other.userId }); +}); + +async function insertPage(tx: DB, workspaceId: number, suffix: string) { + return tx + .insert(page) + .values({ + workspaceId, + title: `${TEST_PREFIX}-${suffix}`, + description: `${TEST_PREFIX}-${suffix}-description`, + slug: `${TEST_PREFIX}-${suffix}-${workspaceId}`, + customDomain: "", + }) + .returning() + .get(); +} + +function incident( + sourceId: string, + title: string, + pageId: number, + workspaceId: number, + dates: Date[], +): PhaseResult["resources"][number] { + return { + sourceId, + name: title, + status: "created", + data: { + report: { title, status: "resolved" as const, workspaceId, pageId }, + updates: dates.map((date, i) => ({ + status: "investigating" as const, + message: `m${i}`, + date, + })), + sourceComponentIds: [], + }, + }; +} + +describe("writeIncidentsPhase idempotency key", () => { + test("keeps distinct incidents that share a title, skips them on rerun", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const pageRow = await insertPage(tx, ctx.workspace.id, "same-title"); + const title = `${TEST_PREFIX}-degraded-performance`; + const makePhase = (): PhaseResult => ({ + phase: "incidents", + status: "completed", + resources: [ + incident("inc_1", title, pageRow.id, ctx.workspace.id, [ + new Date("2024-01-01T00:00:00Z"), + ]), + incident("inc_2", title, pageRow.id, ctx.workspace.id, [ + new Date("2024-02-01T00:00:00Z"), + ]), + ], + }); + const pc = { ctx, tx, provider: "statuspage" as const }; + + const firstRun = makePhase(); + await writeIncidentsPhase(pc, firstRun, pageRow.id, new Map()); + expect(firstRun.resources.map((r) => r.status)).toEqual([ + "created", + "created", + ]); + expect(firstRun.resources[0].openstatusId).not.toBe( + firstRun.resources[1].openstatusId, + ); + + const secondRun = makePhase(); + await writeIncidentsPhase(pc, secondRun, pageRow.id, new Map()); + expect(secondRun.resources.map((r) => r.status)).toEqual([ + "skipped", + "skipped", + ]); + expect(secondRun.resources.map((r) => r.openstatusId)).toEqual( + firstRun.resources.map((r) => r.openstatusId), + ); + + const reports = await tx + .select() + .from(statusReport) + .where(eq(statusReport.pageId, pageRow.id)) + .all(); + expect(reports).toHaveLength(2); + }); + }); +}); + +// These run on the committed db: inside `withTestTransaction` the writers +// reuse the test's transaction, so a failed resource could not roll back on +// its own and the half-written rows would stay visible. +describe("per-resource atomicity", () => { + let pageId: number; + + beforeAll(async () => { + pageId = (await insertPage(db, teamCtx.workspace.id, "atomic")).id; + }); + + afterAll(async () => { + const reports = await db + .select({ id: statusReport.id }) + .from(statusReport) + .where(eq(statusReport.pageId, pageId)) + .all(); + const reportIds = reports.map((r) => r.id); + const updates = reportIds.length + ? await db + .select({ id: statusReportUpdate.id }) + .from(statusReportUpdate) + .where(inArray(statusReportUpdate.statusReportId, reportIds)) + .all() + : []; + const maintenances = await db + .select({ id: maintenance.id }) + .from(maintenance) + .where(eq(maintenance.pageId, pageId)) + .all(); + const maintenanceIds = maintenances.map((m) => m.id); + const mUpdates = maintenanceIds.length + ? await db + .select({ id: maintenanceUpdate.id }) + .from(maintenanceUpdate) + .where(inArray(maintenanceUpdate.maintenanceId, maintenanceIds)) + .all() + : []; + + await clearAuditLogFor({ + entityType: "status_report", + entityIds: reportIds, + }); + await clearAuditLogFor({ + entityType: "status_report_update", + entityIds: updates.map((u) => u.id), + }); + await clearAuditLogFor({ + entityType: "maintenance", + entityIds: maintenanceIds, + }); + await clearAuditLogFor({ + entityType: "maintenance_update", + entityIds: mUpdates.map((u) => u.id), + }); + if (reportIds.length) { + await db.delete(statusReport).where(inArray(statusReport.id, reportIds)); + } + if (maintenanceIds.length) { + await db + .delete(maintenance) + .where(inArray(maintenance.id, maintenanceIds)); + } + await db.delete(page).where(eq(page.id, pageId)); + }); + + test("a failing update rolls back its report so a rerun can create it", async () => { + const pc = { ctx: teamCtx, tx: db, provider: "statuspage" as const }; + const title = `${TEST_PREFIX}-atomic-report`; + const dates = [ + new Date("2024-03-01T00:00:00Z"), + new Date("2024-03-01T01:00:00Z"), + ]; + + const broken = incident( + "inc_a", + title, + pageId, + teamCtx.workspace.id, + dates, + ); + const brokenData = broken.data as { + updates: Array<{ message: string | null }>; + }; + brokenData.updates[1].message = null; + const firstRun: PhaseResult = { + phase: "incidents", + status: "completed", + resources: [broken], + }; + await writeIncidentsPhase(pc, firstRun, pageId, new Map()); + expect(firstRun.resources[0].status).toBe("failed"); + + const leftovers = await db + .select() + .from(statusReport) + .where(eq(statusReport.pageId, pageId)) + .all(); + expect(leftovers).toHaveLength(0); + + const secondRun: PhaseResult = { + phase: "incidents", + status: "completed", + resources: [ + incident("inc_a", title, pageId, teamCtx.workspace.id, dates), + ], + }; + await writeIncidentsPhase(pc, secondRun, pageId, new Map()); + expect(secondRun.resources[0].status).toBe("created"); + + const updates = await db + .select() + .from(statusReportUpdate) + .where( + eq( + statusReportUpdate.statusReportId, + secondRun.resources[0].openstatusId as number, + ), + ) + .all(); + expect(updates).toHaveLength(2); + }); + + test("a failing component link rolls back its maintenance", async () => { + const pc = { ctx: teamCtx, tx: db, provider: "statuspage" as const }; + const phase: PhaseResult = { + phase: "maintenances", + status: "completed", + resources: [ + { + sourceId: "mnt_a", + name: `${TEST_PREFIX}-atomic-maintenance`, + status: "created", + data: { + title: `${TEST_PREFIX}-atomic-maintenance`, + message: "planned", + from: new Date("2024-04-01T00:00:00Z"), + to: new Date("2024-04-01T02:00:00Z"), + workspaceId: teamCtx.workspace.id, + pageId, + sourceComponentIds: ["src_missing_row"], + }, + }, + ], + }; + + // points at a page component that does not exist: the link insert + // violates its foreign key after the maintenance row was written + await writeMaintenancesPhase( + pc, + phase, + pageId, + new Map([["src_missing_row", 2_147_483_000]]), + ); + expect(phase.resources[0].status).toBe("failed"); + + const leftovers = await db + .select() + .from(maintenance) + .where(eq(maintenance.pageId, pageId)) + .all(); + expect(leftovers).toHaveLength(0); + }); +}); + +describe("writeMaintenancesPhase", () => { + test("dedupes source components collapsing onto one component", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const pageRow = await insertPage(tx, ctx.workspace.id, "mnt-dedupe"); + const comp = await tx + .insert(pageComponent) + .values({ + workspaceId: ctx.workspace.id, + pageId: pageRow.id, + name: `${TEST_PREFIX}-mnt-component`, + type: "static", + }) + .returning() + .get(); + + const phase: PhaseResult = { + phase: "maintenances", + status: "completed", + resources: [ + { + sourceId: "mnt_dup", + name: `${TEST_PREFIX}-mnt-dup`, + status: "created", + data: { + title: `${TEST_PREFIX}-mnt-dup`, + message: "planned", + from: new Date("2024-05-01T00:00:00Z"), + to: new Date("2024-05-01T02:00:00Z"), + workspaceId: ctx.workspace.id, + pageId: pageRow.id, + sourceComponentIds: ["src_a", "src_b"], + }, + }, + ], + }; + await writeMaintenancesPhase( + { ctx, tx, provider: "statuspage" }, + phase, + pageRow.id, + new Map([ + ["src_a", comp.id], + ["src_b", comp.id], + ]), + ); + expect(phase.resources[0].status).toBe("created"); + }); + }); +}); + +describe("writeComponentsPhase idempotency key", () => { + test("keeps same-named components in different groups apart", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const pageRow = await insertPage(tx, ctx.workspace.id, "components"); + const [eu, us] = await Promise.all( + ["eu", "us"].map((name) => + tx + .insert(pageComponentGroup) + .values({ workspaceId: ctx.workspace.id, pageId: pageRow.id, name }) + .returning() + .get(), + ), + ); + const groupIdMap = new Map([ + ["grp_eu", eu.id], + ["grp_us", us.id], + ]); + const component = (sourceId: string, sourceGroupId: string) => ({ + sourceId, + name: "API", + status: "created" as const, + data: { + workspaceId: ctx.workspace.id, + pageId: pageRow.id, + type: "static" as const, + monitorId: null, + name: "API", + description: null, + order: 0, + sourceGroupId, + }, + }); + const makePhase = (): PhaseResult => ({ + phase: "components", + status: "completed", + resources: [ + component("cmp_eu", "grp_eu"), + component("cmp_us", "grp_us"), + ], + }); + const pc = { ctx, tx, provider: "statuspage" as const }; + + const firstMap = new Map(); + await writeComponentsPhase( + pc, + makePhase(), + pageRow.id, + groupIdMap, + firstMap, + ); + expect(firstMap.get("cmp_eu")).not.toBe(firstMap.get("cmp_us")); + + const secondMap = new Map(); + const secondRun = makePhase(); + await writeComponentsPhase( + pc, + secondRun, + pageRow.id, + groupIdMap, + secondMap, + ); + expect(secondRun.resources.map((r) => r.status)).toEqual([ + "skipped", + "skipped", + ]); + expect(secondMap).toEqual(firstMap); + }); + }); +}); + +describe("writeMonitorsPhase idempotency key", () => { + test("keeps distinct monitors on the same url apart", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const url = `https://${TEST_PREFIX}.example.com/health`; + const source = (sourceId: string, name: string) => ({ + sourceId, + name, + status: "created" as const, + data: { + workspaceId: ctx.workspace.id, + jobType: "http", + periodicity: "10m", + status: "active", + active: false, + regions: "ams", + url, + name, + description: "", + headers: "[]", + body: "", + method: "GET", + timeout: 45000, + sourceMonitorGroupId: null, + }, + }); + const makePhase = (): PhaseResult => ({ + phase: "monitors", + status: "completed", + resources: [ + source("mon_get", `${TEST_PREFIX}-get`), + source("mon_auth", `${TEST_PREFIX}-auth`), + ], + }); + const pc = { ctx, tx, provider: "checkly" as const }; + + const firstMap = new Map(); + await writeMonitorsPhase(pc, makePhase(), firstMap); + expect(firstMap.get("mon_get")).not.toBe(firstMap.get("mon_auth")); + + const secondMap = new Map(); + const secondRun = makePhase(); + await writeMonitorsPhase(pc, secondRun, secondMap); + expect(secondRun.resources.map((r) => r.status)).toEqual([ + "skipped", + "skipped", + ]); + expect(secondMap).toEqual(firstMap); + + const rows = await tx + .select() + .from(monitor) + .where(eq(monitor.url, url)) + .all(); + expect( + rows.filter((r) => r.workspaceId === ctx.workspace.id), + ).toHaveLength(2); + }); + }); +}); + +describe("writePagePhase into an existing page", () => { + test("leaves the page's title and description untouched", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const pageRow = await insertPage(tx, ctx.workspace.id, "existing"); + const phase: PhaseResult = { + phase: "page", + status: "completed", + resources: [ + { + sourceId: "page_src", + name: "Imported page", + status: "created", + data: { + workspaceId: ctx.workspace.id, + title: "Imported title", + description: "Imported description", + slug: "imported-slug", + customDomain: "", + published: true, + icon: "", + }, + }, + ], + }; + + const id = await writePagePhase( + { ctx, tx, provider: "statuspage" }, + phase, + pageRow.id, + ); + expect(id).toBe(pageRow.id); + expect(phase.resources[0].status).toBe("skipped"); + + const after = await tx + .select() + .from(page) + .where(eq(page.id, pageRow.id)) + .get(); + expect(after?.title).toBe(pageRow.title); + expect(after?.description).toBe(pageRow.description); + }); + }); +}); + +describe("writePagePhase with a taken custom domain", () => { + test("creates the page without the domain another page holds", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const domain = `${TEST_PREFIX}-${Date.now()}.example.com`; + await tx + .insert(page) + .values({ + workspaceId: otherCtx.workspace.id, + title: `${TEST_PREFIX}-holder`, + description: "", + slug: `${TEST_PREFIX}-holder-${Date.now()}`, + customDomain: domain, + }) + .run(); + const phase: PhaseResult = { + phase: "page", + status: "completed", + resources: [ + { + sourceId: "page_src", + name: "Imported page", + status: "created", + data: { + workspaceId: ctx.workspace.id, + title: "Imported title", + description: "", + slug: `${TEST_PREFIX}-taken-domain-${Date.now()}`, + customDomain: domain.toUpperCase(), + published: true, + icon: "", + }, + }, + ], + }; + + const id = await writePagePhase( + { ctx, tx, provider: "statuspage" }, + phase, + ); + const created = await tx.select().from(page).where(eq(page.id, id)).get(); + expect(created?.customDomain).toBe(""); + }); + }); +}); + +describe("previewImport", () => { + test("rejects a pageId from another workspace", async () => { + await withTestTransaction(async (tx) => { + const foreignPage = await insertPage( + tx, + otherCtx.workspace.id, + "foreign", + ); + await expect( + previewImport({ + ctx: { ...teamCtx, db: tx }, + input: PreviewImportInput.parse({ + provider: "statuspage", + apiKey: "unused", + statuspagePageId: "unused", + pageId: foreignPage.id, + }), + }), + ).rejects.toThrow(NotFoundError); + }); + }); +}); diff --git a/packages/services/src/import/phase-writers.ts b/packages/services/src/import/phase-writers.ts index a545bdc99..986f0ce6d 100644 --- a/packages/services/src/import/phase-writers.ts +++ b/packages/services/src/import/phase-writers.ts @@ -19,7 +19,14 @@ import type { PageComponentImpact } from "@openstatus/db/src/schema"; import type { PhaseResult, UpdateComponentImpact } from "@openstatus/importers"; import { emitAudit } from "../audit"; -import { type DB, type ServiceContext, tryGetActorUserId } from "../context"; +import { + type DB, + type ServiceContext, + tryGetActorUserId, + withTransaction, +} from "../context"; +import { ConflictError } from "../errors"; +import { assertCustomDomainAvailable } from "../page/internal"; import { withComponentImpacts, withPageComponentIds, @@ -54,6 +61,29 @@ export type PhaseContext = { * orchestrator in `run.ts` catches throws to abort the remaining phases. */ +/** + * Runs one resource's writes (row + children + links + audit) as a unit. A + * half-written resource would otherwise match the idempotency check on the + * next run and be skipped forever, never getting its missing children. + * Reuses the caller's transaction when `pc.tx` already is one. + */ +// Scope is checked by `runImport`; `fn` emits the resource's audit rows. +// oxlint-disable-next-line openstatus/services-mutation-guards +function atomically( + pc: PhaseContext, + fn: (tx: DB) => Promise, +): Promise { + return withTransaction({ ...pc.ctx, db: pc.tx }, fn); +} + +function earliestDate(dates: Date[]): Date | undefined { + let min: Date | undefined; + for (const d of dates) { + if (!min || d.getTime() < min.getTime()) min = d; + } + return min; +} + function auditMeta( pc: PhaseContext, extra?: Record, @@ -90,6 +120,18 @@ export async function writePagePhase( if (!limits["custom-domain"]) { data.customDomain = ""; } + // Same tolerance when another page already holds the domain. + if (data.customDomain) { + try { + await assertCustomDomainAvailable({ + tx, + customDomain: data.customDomain, + }); + } catch (err) { + if (!(err instanceof ConflictError)) throw err; + data.customDomain = ""; + } + } if (existingPageId) { const existing = await tx @@ -106,11 +148,6 @@ export async function writePagePhase( ); } - await tx - .update(page) - .set({ title: data.title, description: data.description }) - .where(eq(page.id, existingPageId)); - resource.openstatusId = existingPageId; resource.status = "skipped"; phase.status = computePhaseStatus(phase.resources); @@ -284,6 +321,12 @@ export async function writeComponentsPhase( } } + const resolvedGroupId = data.sourceGroupId + ? (groupIdMap.get(data.sourceGroupId) ?? null) + : null; + + // Keyed on the group too: same-named components in different groups + // ("API" under EU and US) are distinct and must not share an id. const existing = await tx .select() .from(pageComponent) @@ -291,6 +334,9 @@ export async function writeComponentsPhase( and( eq(pageComponent.name, data.name), eq(pageComponent.pageId, pageId), + resolvedGroupId === null + ? isNull(pageComponent.groupId) + : eq(pageComponent.groupId, resolvedGroupId), ), ) .get(); @@ -302,10 +348,6 @@ export async function writeComponentsPhase( continue; } - const resolvedGroupId = data.sourceGroupId - ? (groupIdMap.get(data.sourceGroupId) ?? null) - : null; - const [inserted] = await tx .insert(pageComponent) .values({ @@ -378,23 +420,40 @@ export async function writeIncidentsPhase( sourceComponentIds: string[]; }; - // Idempotency by `(title, pageId)` — every other phase writer - // (page slug, monitor url, component name, subscriber email) - // skips on duplicate before inserting, and imports are - // explicitly re-runnable per `run.ts`'s phase-level recovery - // model. Without this check, re-running an import would land - // duplicate status reports on every pass. - const existingReport = await tx - .select({ id: statusReport.id }) - .from(statusReport) - .where( - and( - eq(statusReport.pageId, pageId), - eq(statusReport.workspaceId, workspaceId), - eq(statusReport.title, data.report.title), - ), - ) - .get(); + // Idempotency by `(title, pageId, first update date)` so imports stay + // re-runnable per `run.ts`'s phase-level recovery model. Title alone + // collapses distinct incidents that share a generic name ("Degraded + // performance"); the first update's date pins the match to one + // incident, as `from/to` does for maintenances below. + const firstUpdateDate = earliestDate(data.updates.map((u) => u.date)); + const existingReport = firstUpdateDate + ? await tx + .selectDistinct({ id: statusReport.id }) + .from(statusReport) + .innerJoin( + statusReportUpdate, + eq(statusReportUpdate.statusReportId, statusReport.id), + ) + .where( + and( + eq(statusReport.pageId, pageId), + eq(statusReport.workspaceId, workspaceId), + eq(statusReport.title, data.report.title), + eq(statusReportUpdate.date, firstUpdateDate), + ), + ) + .get() + : await tx + .select({ id: statusReport.id }) + .from(statusReport) + .where( + and( + eq(statusReport.pageId, pageId), + eq(statusReport.workspaceId, workspaceId), + eq(statusReport.title, data.report.title), + ), + ) + .get(); if (existingReport) { resource.openstatusId = existingReport.id; @@ -422,125 +481,129 @@ export async function writeIncidentsPhase( continue; } - const [insertedReport] = await tx - .insert(statusReport) - .values({ - title: data.report.title, - status: data.report.status, - workspaceId, - pageId, - createdBy: actorUserId, - updatedBy: actorUserId, - }) - .returning(); - - if (!insertedReport) { - resource.status = "failed"; - resource.error = "Failed to insert status report"; - continue; - } - - // membership = sourceComponentIds ∪ impact-named components, upholding - // the invariant that every impact row's component is in the report set - const memberSourceIds = new Set([ - ...data.sourceComponentIds, - ...data.updates.flatMap((u) => - (u.componentImpacts ?? []).map((ci) => ci.sourceComponentId), - ), - ]); - // distinct source ids can collapse onto one internal component (the - // components phase dedupes by name+page) — dedupe to respect the PK - const memberComponentIds = new Set(); - for (const sourceCompId of memberSourceIds) { - const osCompId = componentIdMap.get(sourceCompId); - if (osCompId) memberComponentIds.add(osCompId); - } - const componentLinks = [...memberComponentIds].map((pageComponentId) => ({ - statusReportId: insertedReport.id, - pageComponentId, - })); - - await emitAudit(tx, ctx, { - action: "status_report.create", - entityType: "status_report", - entityId: insertedReport.id, - metadata: auditMeta(pc, { - sourceId: resource.sourceId, - pageId, - title: data.report.title, - }), - after: withPageComponentIds( - insertedReport, - componentLinks.map((l) => l.pageComponentId), - ), - }); - - // Insert updates one-by-one so each inserted id pairs with its source - // update's componentImpacts deterministically (batch `.returning()` - // order is not a guarantee worth relying on). - for (const u of data.updates) { - const row = await tx - .insert(statusReportUpdate) + const reportId = await atomically(pc, async (rtx) => { + const [insertedReport] = await rtx + .insert(statusReport) .values({ - status: u.status, - message: u.message, - date: u.date, - statusReportId: insertedReport.id, + title: data.report.title, + status: data.report.status, + workspaceId, + pageId, createdBy: actorUserId, updatedBy: actorUserId, }) - .returning() - .get(); - - // ids missing from `componentIdMap` get no impact row — same - // partial-map caveat as the membership links. Collapsed source ids - // (same internal component) keep the worst impact. - const impactByComponent = new Map(); - for (const ci of u.componentImpacts ?? []) { - const osCompId = componentIdMap.get(ci.sourceComponentId); - if (!osCompId) continue; - const prev = impactByComponent.get(osCompId); - impactByComponent.set( - osCompId, - prev ? worstImpact([prev, ci.impact]) : ci.impact, - ); + .returning(); + + if (!insertedReport) throw new Error("Failed to insert status report"); + + // membership = sourceComponentIds ∪ impact-named components, upholding + // the invariant that every impact row's component is in the report set + const memberSourceIds = new Set([ + ...data.sourceComponentIds, + ...data.updates.flatMap((u) => + (u.componentImpacts ?? []).map((ci) => ci.sourceComponentId), + ), + ]); + // distinct source ids can collapse onto one internal component (the + // components phase dedupes by name+group+page) — dedupe to respect the PK + const memberComponentIds = new Set(); + for (const sourceCompId of memberSourceIds) { + const osCompId = componentIdMap.get(sourceCompId); + if (osCompId) memberComponentIds.add(osCompId); } - const impactRows = [...impactByComponent].map( - ([pageComponentId, impact]) => ({ - statusReportUpdateId: row.id, + const componentLinks = [...memberComponentIds].map( + (pageComponentId) => ({ + statusReportId: insertedReport.id, pageComponentId, - impact, }), ); - if (impactRows.length > 0) { - await tx - .insert(statusReportUpdateToPageComponents) - .values(impactRows); - } - await emitAudit(tx, ctx, { - action: "status_report_update.create", - entityType: "status_report_update", - entityId: row.id, + await emitAudit(rtx, ctx, { + action: "status_report.create", + entityType: "status_report", + entityId: insertedReport.id, metadata: auditMeta(pc, { sourceId: resource.sourceId, - statusReportId: insertedReport.id, + pageId, + title: data.report.title, }), - after: withComponentImpacts( - row, - impactRows.map(({ pageComponentId, impact }) => ({ - pageComponentId, - impact, - })), + after: withPageComponentIds( + insertedReport, + componentLinks.map((l) => l.pageComponentId), ), }); - } - if (componentLinks.length > 0) { - await tx.insert(statusReportsToPageComponents).values(componentLinks); - } + // Insert updates one-by-one so each inserted id pairs with its source + // update's componentImpacts deterministically (batch `.returning()` + // order is not a guarantee worth relying on). + for (const u of data.updates) { + const row = await rtx + .insert(statusReportUpdate) + .values({ + status: u.status, + message: u.message, + date: u.date, + statusReportId: insertedReport.id, + createdBy: actorUserId, + updatedBy: actorUserId, + }) + .returning() + .get(); + + // ids missing from `componentIdMap` get no impact row — same + // partial-map caveat as the membership links. Collapsed source ids + // (same internal component) keep the worst impact. + const impactByComponent = new Map(); + for (const ci of u.componentImpacts ?? []) { + const osCompId = componentIdMap.get(ci.sourceComponentId); + if (!osCompId) continue; + const prev = impactByComponent.get(osCompId); + impactByComponent.set( + osCompId, + prev ? worstImpact([prev, ci.impact]) : ci.impact, + ); + } + const impactRows = [...impactByComponent].map( + ([pageComponentId, impact]) => ({ + statusReportUpdateId: row.id, + pageComponentId, + impact, + }), + ); + if (impactRows.length > 0) { + await rtx + .insert(statusReportUpdateToPageComponents) + .values(impactRows); + } + + await emitAudit(rtx, ctx, { + action: "status_report_update.create", + entityType: "status_report_update", + entityId: row.id, + metadata: auditMeta(pc, { + sourceId: resource.sourceId, + statusReportId: insertedReport.id, + }), + after: withComponentImpacts( + row, + impactRows.map(({ pageComponentId, impact }) => ({ + pageComponentId, + impact, + })), + ), + }); + } + + if (componentLinks.length > 0) { + await rtx + .insert(statusReportsToPageComponents) + .values(componentLinks); + } - resource.openstatusId = insertedReport.id; + return insertedReport.id; + }); + + resource.openstatusId = reportId; resource.status = "created"; } catch (err) { resource.status = "failed"; @@ -617,82 +680,82 @@ export async function writeMaintenancesPhase( continue; } - const [inserted] = await tx - .insert(maintenance) - .values({ - title: data.title, - message: data.message, - from: data.from, - to: data.to, - workspaceId, - pageId, - createdBy: actorUserId, - updatedBy: actorUserId, - }) - .returning(); - - if (!inserted) { - resource.status = "failed"; - resource.error = "Failed to insert maintenance"; - continue; - } + const maintenanceId = await atomically(pc, async (rtx) => { + const [inserted] = await rtx + .insert(maintenance) + .values({ + title: data.title, + message: data.message, + from: data.from, + to: data.to, + workspaceId, + pageId, + createdBy: actorUserId, + updatedBy: actorUserId, + }) + .returning(); - // the message is the first timeline update; the announcement time - // of imported history is unknown, so it is dated at the window start - const [insertedUpdate] = await tx - .insert(maintenanceUpdate) - .values({ - maintenanceId: inserted.id, - message: data.message, - date: data.from, - createdBy: actorUserId, - updatedBy: actorUserId, - }) - .returning(); + if (!inserted) throw new Error("Failed to insert maintenance"); - const componentLinks: Array<{ - maintenanceId: number; - pageComponentId: number; - }> = []; - for (const sourceCompId of data.sourceComponentIds) { - const osCompId = componentIdMap.get(sourceCompId); - if (osCompId) { - componentLinks.push({ + // the message is the first timeline update; the announcement time + // of imported history is unknown, so it is dated at the window start + const [insertedUpdate] = await rtx + .insert(maintenanceUpdate) + .values({ maintenanceId: inserted.id, - pageComponentId: osCompId, - }); + message: data.message, + date: data.from, + createdBy: actorUserId, + updatedBy: actorUserId, + }) + .returning(); + + // distinct source ids can collapse onto one internal component — + // dedupe to respect the composite PK + const memberComponentIds = new Set(); + for (const sourceCompId of data.sourceComponentIds) { + const osCompId = componentIdMap.get(sourceCompId); + if (osCompId) memberComponentIds.add(osCompId); + } + const componentLinks = [...memberComponentIds].map( + (pageComponentId) => ({ + maintenanceId: inserted.id, + pageComponentId, + }), + ); + if (componentLinks.length > 0) { + await rtx.insert(maintenancesToPageComponents).values(componentLinks); } - } - if (componentLinks.length > 0) { - await tx.insert(maintenancesToPageComponents).values(componentLinks); - } - - resource.openstatusId = inserted.id; - resource.status = "created"; - await emitAudit(tx, ctx, { - action: "maintenance.create", - entityType: "maintenance", - entityId: inserted.id, - metadata: auditMeta(pc, { - sourceId: resource.sourceId, - pageId, - title: data.title, - }), - after: inserted, - }); - if (insertedUpdate) { - await emitAudit(tx, ctx, { - action: "maintenance_update.create", - entityType: "maintenance_update", - entityId: insertedUpdate.id, + await emitAudit(rtx, ctx, { + action: "maintenance.create", + entityType: "maintenance", + entityId: inserted.id, metadata: auditMeta(pc, { sourceId: resource.sourceId, - maintenanceId: inserted.id, + pageId, + title: data.title, }), - after: insertedUpdate, + after: inserted, }); - } + if (insertedUpdate) { + await emitAudit(rtx, ctx, { + action: "maintenance_update.create", + entityType: "maintenance_update", + entityId: insertedUpdate.id, + metadata: auditMeta(pc, { + sourceId: resource.sourceId, + maintenanceId: inserted.id, + }), + after: insertedUpdate, + }); + } + + return inserted.id; + }); + + resource.openstatusId = maintenanceId; + resource.status = "created"; } catch (err) { resource.status = "failed"; resource.error = err instanceof Error ? err.message : String(err); @@ -756,13 +819,16 @@ export async function writeMonitorsPhase( sourceMonitorGroupId: string | null; }; - // Idempotency: same url in the workspace (active only). + // Idempotency: same url + name in the workspace (active only). Url + // alone collapses distinct source checks against one endpoint, and + // components linked to the second would point at the first. const existing = await tx .select() .from(monitor) .where( and( eq(monitor.url, data.url), + eq(monitor.name, data.name), eq(monitor.workspaceId, workspaceId), isNull(monitor.deletedAt), ), diff --git a/packages/services/src/import/preview.ts b/packages/services/src/import/preview.ts index abe56c627..90d6d818f 100644 --- a/packages/services/src/import/preview.ts +++ b/packages/services/src/import/preview.ts @@ -1,8 +1,9 @@ -import { db as defaultDb } from "@openstatus/db"; +import { and, db as defaultDb, eq } from "@openstatus/db"; +import { page } from "@openstatus/db/src/schema"; import type { ImportSummary } from "@openstatus/importers"; import type { ServiceContext } from "../context"; -import { ValidationError } from "../errors"; +import { NotFoundError, ValidationError } from "../errors"; import { addLimitWarnings } from "./limits"; import { buildProviderConfig, createProvider } from "./provider"; import { PreviewImportInput } from "./schemas"; @@ -21,6 +22,18 @@ export async function previewImport(args: { const input = PreviewImportInput.parse(args.input); const db = ctx.db ?? defaultDb; + if (input.pageId) { + const existing = await db + .select({ id: page.id }) + .from(page) + .where( + and(eq(page.id, input.pageId), eq(page.workspaceId, ctx.workspace.id)), + ) + .get(); + + if (!existing) throw new NotFoundError("page", input.pageId); + } + const provider = createProvider(input.provider); const providerConfig = buildProviderConfig({ provider: input.provider, diff --git a/packages/services/src/incident/__tests__/postmortem-draft.test.ts b/packages/services/src/incident/__tests__/postmortem-draft.test.ts index 98977c4fe..9798e7b98 100644 --- a/packages/services/src/incident/__tests__/postmortem-draft.test.ts +++ b/packages/services/src/incident/__tests__/postmortem-draft.test.ts @@ -119,6 +119,54 @@ describe("collectChannelTranscript", () => { expect(text).not.toContain("reply 499"); expect(truncated).toBe(true); }); + + test("over the cap the oldest messages go, the newest stay", async () => { + const top = Array.from({ length: 600 }, (_, i) => ({ + ts: `${1000 + i}.0`, + text: `message ${i}`, + user: "U1", + })); + const newestFirst = [...top].reverse(); + const client: SlackHistoryClient = { + conversations: { + history: async ({ cursor }) => { + const from = Number(cursor ?? 0); + const next = from + 200 < top.length ? String(from + 200) : undefined; + return { + messages: newestFirst.slice(from, from + 200), + has_more: Boolean(next), + response_metadata: { next_cursor: next }, + }; + }, + replies: async () => ({ messages: [] }), + }, + }; + const { text, truncated } = await collectChannelTranscript(client, "C1"); + const lines = text.split("\n"); + expect(lines).toHaveLength(500); + expect(lines[0]).toContain("message 100"); + expect(lines.at(-1)).toContain("message 599"); + expect(text).not.toContain("message 99\n"); + expect(truncated).toBe(true); + }); + + test("over the character cap whole lines go from the oldest end", async () => { + const big = "x".repeat(300_000); + const client = history({ + top: Array.from({ length: 5 }, (_, i) => ({ + ts: `${100 + i}.0`, + text: `${i} ${big}`, + user: "U1", + })), + }); + const { text, truncated } = await collectChannelTranscript(client, "C1"); + const lines = text.split("\n"); + expect(text.length).toBeLessThanOrEqual(1_000_000); + expect(lines).toHaveLength(3); + expect(lines[0]).toContain(": 2 x"); + expect(lines[2]).toContain(": 4 x"); + expect(truncated).toBe(true); + }); }); async function resolvedIncident(tx: DB, ctx: ServiceContext, bound: boolean) { diff --git a/packages/services/src/incident/postmortem-draft.ts b/packages/services/src/incident/postmortem-draft.ts index 5452a6801..148d67b30 100644 --- a/packages/services/src/incident/postmortem-draft.ts +++ b/packages/services/src/incident/postmortem-draft.ts @@ -67,8 +67,9 @@ function line(message: { ts?: string; user?: string; text?: string }): string { /** * The channel, oldest first, bot messages left out and threads inlined under - * their parent, capped at 500 messages (the most recent top-level ones) and - * 1,000,000 characters. + * their parent, capped at 500 messages and 1,000,000 characters. The caps drop + * the oldest top-level messages first: the end of an incident (mitigation, + * resolution) matters more than its start. */ export async function collectChannelTranscript( client: SlackHistoryClient, @@ -91,16 +92,26 @@ export async function collectChannelTranscript( } } while (cursor); - const lines: string[] = []; - const push = (entry: string): boolean => { - if (lines.length >= MAX_MESSAGES) { + // Walk newest first so the cap drops the oldest top-level messages, then + // flip each kept block (a parent and its thread) back to oldest first. + const blocks: string[][] = []; + let count = 0; + collect: for (const message of top) { + if (count >= MAX_MESSAGES) { truncated = true; - return false; + break; } - lines.push(entry); - return true; - }; - collect: for (const message of top.reverse()) { + const block: string[] = []; + blocks.push(block); + const push = (entry: string): boolean => { + if (count >= MAX_MESSAGES) { + truncated = true; + return false; + } + block.push(entry); + count += 1; + return true; + }; // A bot parent (the incident card) is left out, its human replies are not. if (!message.bot_id && message.text && !push(line(message))) break; if (!message.reply_count || !message.ts) continue; @@ -121,11 +132,16 @@ export async function collectChannelTranscript( : undefined; } while (replyCursor); } - let text = lines.join("\n"); - if (text.length > MAX_TRANSCRIPT_CHARS) { - text = text.slice(0, MAX_TRANSCRIPT_CHARS); + const lines = blocks.reverse().flat(); + // Over the character cap, drop whole lines from the oldest end. + let length = lines.reduce((sum, entry) => sum + entry.length + 1, -1); + let first = 0; + while (length > MAX_TRANSCRIPT_CHARS && first < lines.length) { + length -= lines[first].length + 1; + first += 1; truncated = true; } + const text = lines.slice(first).join("\n"); return { text, truncated }; } diff --git a/packages/services/src/invitation/__tests__/invitation.test.ts b/packages/services/src/invitation/__tests__/invitation.test.ts index 8dcbe71a4..8fcd501eb 100644 --- a/packages/services/src/invitation/__tests__/invitation.test.ts +++ b/packages/services/src/invitation/__tests__/invitation.test.ts @@ -1,5 +1,9 @@ import { eq } from "@openstatus/db"; -import { invitation, usersToWorkspaces } from "@openstatus/db/src/schema"; +import { + invitation, + usersToWorkspaces, + workspace, +} from "@openstatus/db/src/schema"; import { createUser } from "@openstatus/db/src/test/factories"; import { expect } from "@std/expect"; import { beforeAll, describe, test } from "@std/testing/bdd"; @@ -303,6 +307,52 @@ describe("acceptInvitation", () => { }); }); + test("re-checks the members cap when the plan shrank after the invite", async () => { + await withTestTransaction(async (tx) => { + const email = `${TEST_PREFIX}-downgraded-${Date.now()}@example.com`; + const created = await createInvitation({ + ctx: { ...teamCtx, db: tx }, + input: { email }, + }); + // Downgrade to free (`members: 1`) — the owner fills the only seat. + await tx + .update(workspace) + .set({ plan: "free", limits: "{}" }) + .where(eq(workspace.id, teamCtx.workspace.id)); + + await expect( + acceptInvitation({ + ctx: { + ...makeUserCtx(teamCtx.workspace, { userId: acceptingUserId }), + db: tx, + }, + input: { id: created.id, email }, + }), + ).rejects.toBeInstanceOf(LimitExceededError); + }); + }); + + test("lets an existing member redeem an invite after a downgrade", async () => { + await withTestTransaction(async (tx) => { + const email = `${TEST_PREFIX}-existing-member-${Date.now()}@example.com`; + const created = await createInvitation({ + ctx: { ...teamCtx, db: tx }, + input: { email }, + }); + await tx + .update(workspace) + .set({ plan: "free", limits: "{}" }) + .where(eq(workspace.id, teamCtx.workspace.id)); + + // The owner already holds a seat, so redeeming adds nobody. + const row = await acceptInvitation({ + ctx: { ...teamCtx, db: tx }, + input: { id: created.id, email }, + }); + expect(row.id).toBe(teamCtx.workspace.id); + }); + }); + test("throws NotFoundError for an unknown or wrong-email invitation", async () => { await withTestTransaction(async (tx) => { await expect( diff --git a/packages/services/src/invitation/accept.ts b/packages/services/src/invitation/accept.ts index ea7e4c158..826dce9cf 100644 --- a/packages/services/src/invitation/accept.ts +++ b/packages/services/src/invitation/accept.ts @@ -12,8 +12,14 @@ import { tryGetActorUserId, withTransaction, } from "../context"; -import { ConflictError, NotFoundError, UnauthorizedError } from "../errors"; +import { + ConflictError, + LimitExceededError, + NotFoundError, + UnauthorizedError, +} from "../errors"; import type { Workspace } from "../types"; +import { UNLIMITED_MEMBERS_EFFECTIVE_CAP } from "./create"; import { AcceptInvitationInput } from "./schemas"; /** @@ -67,6 +73,28 @@ export async function acceptInvitation(args: { }); if (!existing) throw new NotFoundError("invitation", input.id); + if (!existing.workspace) { + throw new NotFoundError("workspace", existing.workspaceId); + } + const workspace = selectWorkspaceSchema.parse(existing.workspace); + + // Re-check the target workspace's `members` cap: `createInvitation` + // checks at send time, but the plan may have shrunk (downgrade, trial + // end) before the invitee redeems the token. An invitee who's already + // a member doesn't add a seat, so they pass regardless. + const membersLimit = workspace.limits.members; + const effectiveLimit = + membersLimit === "Unlimited" + ? UNLIMITED_MEMBERS_EFFECTIVE_CAP + : membersLimit; + const memberRows = await tx + .select({ userId: usersToWorkspaces.userId }) + .from(usersToWorkspaces) + .where(eq(usersToWorkspaces.workspaceId, existing.workspaceId)); + const alreadyMember = memberRows.some((m) => m.userId === userId); + if (!alreadyMember && memberRows.length >= effectiveLimit) { + throw new LimitExceededError("members", effectiveLimit); + } // Conditional UPDATE — the `isNull(acceptedAt)` predicate makes the // write a no-op for any row another caller has already claimed, @@ -108,11 +136,6 @@ export async function acceptInvitation(args: { }) .onConflictDoNothing(); - if (!existing.workspace) { - throw new NotFoundError("workspace", existing.workspaceId); - } - const workspace = selectWorkspaceSchema.parse(existing.workspace); - // Strip `token` (email-link capability) from the snapshot; construct the snapshots from the pre-fetched row // to avoid a second SELECT after the UPDATE. const { token: _token, ...before } = existing; diff --git a/packages/services/src/invitation/create.ts b/packages/services/src/invitation/create.ts index d6915e45b..1baea693e 100644 --- a/packages/services/src/invitation/create.ts +++ b/packages/services/src/invitation/create.ts @@ -18,7 +18,7 @@ import { CreateInvitationInput } from "./schemas"; * behavior in `invitationRouter.create`. Large enough that enterprise plans * never brush against it in practice. */ -const UNLIMITED_MEMBERS_EFFECTIVE_CAP = 420; +export const UNLIMITED_MEMBERS_EFFECTIVE_CAP = 420; /** * Invite a user to the caller's workspace. diff --git a/packages/services/src/monitor/__tests__/monitor.test.ts b/packages/services/src/monitor/__tests__/monitor.test.ts index 25fc75c6b..29699f5f3 100644 --- a/packages/services/src/monitor/__tests__/monitor.test.ts +++ b/packages/services/src/monitor/__tests__/monitor.test.ts @@ -405,6 +405,75 @@ describe("cloneMonitor", () => { expect(clone.status).toBe("active"); }); }); + + test("rejects a source whose periodicity is outside the current plan", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const source = await createMonitor({ + ctx, + input: { + name: `${TEST_PREFIX}-clone-periodicity`, + jobType: "http", + url: "https://example.com", + method: "GET", + headers: [], + assertions: [], + active: false, + }, + }); + // Simulates a monitor that predates a downgrade. + await tx + .update(monitor) + .set({ periodicity: "30s" }) + .where(eq(monitor.id, source.id)); + + const downgradedCtx: ServiceContext = { + ...ctx, + workspace: { + ...ctx.workspace, + limits: { ...ctx.workspace.limits, periodicity: ["10m", "30m"] }, + }, + }; + + await expect( + cloneMonitor({ ctx: downgradedCtx, input: { id: source.id } }), + ).rejects.toBeInstanceOf(LimitExceededError); + }); + }); + + test("rejects a source whose regions are outside the current plan", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const source = await createMonitor({ + ctx, + input: { + name: `${TEST_PREFIX}-clone-regions`, + jobType: "http", + url: "https://example.com", + method: "GET", + headers: [], + assertions: [], + active: false, + }, + }); + await tx + .update(monitor) + .set({ regions: "ams,iad,gru" }) + .where(eq(monitor.id, source.id)); + + const downgradedCtx: ServiceContext = { + ...ctx, + workspace: { + ...ctx.workspace, + limits: { ...ctx.workspace.limits, "max-regions": 2 }, + }, + }; + + await expect( + cloneMonitor({ ctx: downgradedCtx, input: { id: source.id } }), + ).rejects.toBeInstanceOf(LimitExceededError); + }); + }); }); describe("updateMonitorTags / updateMonitorNotifiers", () => { diff --git a/packages/services/src/monitor/clone.ts b/packages/services/src/monitor/clone.ts index 2c7f90a0d..c916e13e6 100644 --- a/packages/services/src/monitor/clone.ts +++ b/packages/services/src/monitor/clone.ts @@ -6,12 +6,13 @@ import { type ServiceContext, withTransaction } from "../context"; import { InternalServiceError } from "../errors"; import { assertWithinLimit } from "../limits"; import type { Monitor } from "../types"; -import { getMonitorInWorkspace } from "./internal"; +import { assertSchedulingWithinPlan, getMonitorInWorkspace } from "./internal"; import { CloneMonitorInput } from "./schemas"; /** * Duplicate a monitor. Fails with `LimitExceededError` when cloning would - * push the workspace past its monitor quota. + * push the workspace past its monitor quota, or when the source's + * periodicity / regions are outside the current plan (e.g. after a downgrade). */ export async function cloneMonitor(args: { ctx: ServiceContext; @@ -34,6 +35,11 @@ export async function cloneMonitor(args: { workspaceId: ctx.workspace.id, }); + assertSchedulingWithinPlan(ctx.workspace.limits, { + periodicity: source.periodicity, + regions: source.regions.split(",").filter(Boolean), + }); + const { id: _id, status: _status, ...rest } = source; const row = await tx .insert(monitor) diff --git a/packages/services/src/monitor/internal.ts b/packages/services/src/monitor/internal.ts index e751cd595..afb9068c3 100644 --- a/packages/services/src/monitor/internal.ts +++ b/packages/services/src/monitor/internal.ts @@ -21,7 +21,7 @@ import { import { regionDict } from "@openstatus/regions"; import type { DB } from "../context"; -import { ForbiddenError, NotFoundError } from "../errors"; +import { ForbiddenError, LimitExceededError, NotFoundError } from "../errors"; import type { Workspace } from "../types"; /** Load a monitor by id, scoped to the workspace and excluding soft-deleted. */ @@ -180,3 +180,33 @@ export function pickDefaultRegions(workspace: Workspace): { periodicity: workspace.plan === "free" ? "30m" : "1m", }; } + +/** + * Enforce the plan's periodicity, region-count and region-access limits on + * a monitor's scheduling. Shared by every path that writes them so a clone + * (or a source that predates a downgrade) can't carry out-of-plan values. + */ +export function assertSchedulingWithinPlan( + limits: Workspace["limits"], + scheduling: { periodicity: string; regions: ReadonlyArray }, +): void { + const { periodicity, regions } = scheduling; + if ( + !limits.periodicity.includes( + periodicity as (typeof limits)["periodicity"][number], + ) + ) { + throw new LimitExceededError("periodicity", limits.periodicity.length); + } + if (limits["max-regions"] < regions.length) { + throw new LimitExceededError("max-regions", limits["max-regions"]); + } + if ( + regions.length > 0 && + !regions.every((r) => + limits.regions.includes(r as (typeof limits)["regions"][number]), + ) + ) { + throw new LimitExceededError("regions", limits.regions.length); + } +} diff --git a/packages/services/src/monitor/relations.ts b/packages/services/src/monitor/relations.ts index 0572e2470..357fba1b4 100644 --- a/packages/services/src/monitor/relations.ts +++ b/packages/services/src/monitor/relations.ts @@ -9,8 +9,8 @@ import { import { emitAudit } from "../audit"; import { requireScope } from "../auth"; import { type ServiceContext, withTransaction } from "../context"; -import { LimitExceededError } from "../errors"; import { + assertSchedulingWithinPlan, getMonitorInWorkspace, validateNotificationIds, validatePrivateLocationIds, @@ -34,22 +34,7 @@ export async function updateMonitorSchedulingRegions(args: { const { ctx } = args; requireScope(ctx, "write"); const input = UpdateMonitorSchedulingRegionsInput.parse(args.input); - const limits = ctx.workspace.limits; - - if (!limits.periodicity.includes(input.periodicity)) { - throw new LimitExceededError("periodicity", limits.periodicity.length); - } - if (limits["max-regions"] < input.regions.length) { - throw new LimitExceededError("max-regions", limits["max-regions"]); - } - if ( - input.regions.length > 0 && - !input.regions.every((r) => - limits.regions.includes(r as (typeof limits)["regions"][number]), - ) - ) { - throw new LimitExceededError("regions", limits.regions.length); - } + assertSchedulingWithinPlan(ctx.workspace.limits, input); await withTransaction(ctx, async (tx) => { const existing = await getMonitorInWorkspace({ diff --git a/packages/services/src/page-subscriber/__tests__/slack.test.ts b/packages/services/src/page-subscriber/__tests__/slack.test.ts index d34beb14b..28c7305b6 100644 --- a/packages/services/src/page-subscriber/__tests__/slack.test.ts +++ b/packages/services/src/page-subscriber/__tests__/slack.test.ts @@ -5,6 +5,7 @@ import { expect } from "@std/expect"; import { afterAll, beforeAll, describe, test } from "@std/testing/bdd"; import { createWorkspaceFixture, expectAuditRow } from "../../../test/helpers"; +import { PreconditionFailedError } from "../../errors"; import { createSlackSubscriber, listSlackSubscribersForChannel, @@ -22,6 +23,7 @@ const CHANNELS = { reactivate: "C_SLACK_REACTIVATE", remove: "C_SLACK_REMOVE", list: "C_SLACK_LIST", + protected: "C_SLACK_PROTECTED", }; async function cleanAll() { @@ -119,6 +121,36 @@ describe("createSlackSubscriber", () => { }); }); +describe("createSlackSubscriber on a protected page", () => { + for (const accessType of [ + "password", + "email-domain", + "ip-restriction", + ] as const) { + test(`rejects a ${accessType} page and writes no row`, async () => { + const gated = await createPage(WORKSPACE_ID, { + accessType, + password: accessType === "password" ? "secret" : null, + }); + + await expect( + createSlackSubscriber({ + input: { + pageId: gated.id, + teamId: TEAM_ID, + channelId: CHANNELS.protected, + }, + }), + ).rejects.toBeInstanceOf(PreconditionFailedError); + + const rows = await db.query.pageSubscriber.findMany({ + where: eq(pageSubscriber.pageId, gated.id), + }); + expect(rows).toHaveLength(0); + }); + } +}); + describe("removeSlackSubscriber", () => { test("soft-deletes the active row and audits the update", async () => { const created = await createSlackSubscriber({ diff --git a/packages/services/src/page-subscriber/slack.ts b/packages/services/src/page-subscriber/slack.ts index ba9057b98..e335b1c7c 100644 --- a/packages/services/src/page-subscriber/slack.ts +++ b/packages/services/src/page-subscriber/slack.ts @@ -12,7 +12,7 @@ import { getReadDb, withTransaction, } from "../context"; -import { NotFoundError } from "../errors"; +import { NotFoundError, PreconditionFailedError } from "../errors"; import { assertSubscribersAllowed, parseWorkspaceForContext } from "./internal"; import { type CreateSlackSubscriberInput, @@ -58,6 +58,12 @@ export async function createSlackSubscriber(args: { if (!pageData) { throw new NotFoundError("page", input.pageId); } + // A channel can't present a password, email or IP, so only public pages. + if (pageData.accessType && pageData.accessType !== "public") { + throw new PreconditionFailedError( + "Protected status pages can't be subscribed to from Slack.", + ); + } const workspace = parseWorkspaceForContext(pageData.workspace); assertSubscribersAllowed(workspace); diff --git a/packages/services/src/page/__tests__/page.test.ts b/packages/services/src/page/__tests__/page.test.ts index 3d248694f..c391e37c7 100644 --- a/packages/services/src/page/__tests__/page.test.ts +++ b/packages/services/src/page/__tests__/page.test.ts @@ -163,6 +163,10 @@ describe("createPage (full form)", () => { name: "additional locale", input: { locales: ["en", "fr"] }, }, + { + name: "custom domain", + input: { customDomain: `${TEST_PREFIX}-denied.example.com` }, + }, ] satisfies { name: string; input: Partial; @@ -548,6 +552,101 @@ describe("updatePageCustomDomain", () => { }); }); +describe("custom domain uniqueness", () => { + const uniqueDomain = (tag: string) => + `${uniqueSlug(tag)}.example.com`.toLowerCase(); + + test("createPage rejects a domain another workspace already uses", async () => { + await withTestTransaction(async (tx) => { + const domain = uniqueDomain("taken-create"); + await tx + .insert(pageTable) + .values({ + workspaceId: freeCtx.workspace.id, + title: "Owner", + description: "", + slug: uniqueSlug("domain-owner"), + customDomain: domain, + }) + .run(); + + const slug = uniqueSlug("domain-thief"); + await expect( + createPage({ + ctx: { ...teamCtx, db: tx }, + input: { + workspaceId: teamCtx.workspace.id, + title: "Thief", + description: "", + slug, + customDomain: domain.toUpperCase(), + }, + }), + ).rejects.toBeInstanceOf(ConflictError); + expect( + await tx.select().from(pageTable).where(eq(pageTable.slug, slug)).get(), + ).toBeUndefined(); + }); + }); + + test("updatePageCustomDomain rejects a domain used by another page", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const domain = uniqueDomain("taken-update"); + const owner = await newPage({ + ctx, + input: { title: "Owner", slug: uniqueSlug("domain-owner") }, + }); + await updatePageCustomDomain({ + ctx, + input: { id: owner.id, customDomain: domain }, + }); + + const other = await newPage({ + ctx, + input: { title: "Other", slug: uniqueSlug("domain-other") }, + }); + await expect( + updatePageCustomDomain({ + ctx, + input: { id: other.id, customDomain: domain }, + }), + ).rejects.toBeInstanceOf(ConflictError); + const row = await tx + .select() + .from(pageTable) + .where(eq(pageTable.id, other.id)) + .get(); + expect(row?.customDomain).toBe(""); + }); + }); + + test("updatePageCustomDomain lets a page re-save its own domain", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const domain = uniqueDomain("own"); + const p = await newPage({ + ctx, + input: { title: "Own", slug: uniqueSlug("domain-own") }, + }); + await updatePageCustomDomain({ + ctx, + input: { id: p.id, customDomain: domain }, + }); + await updatePageCustomDomain({ + ctx, + input: { id: p.id, customDomain: domain }, + }); + const row = await tx + .select() + .from(pageTable) + .where(eq(pageTable.id, p.id)) + .get(); + expect(row?.customDomain).toBe(domain); + }); + }); +}); + describe("updatePageConfiguration", () => { test("preserves omitted settings in partial updates", async () => { await withTestTransaction(async (tx) => { diff --git a/packages/services/src/page/create.ts b/packages/services/src/page/create.ts index 0f9958f9d..3a6133659 100644 --- a/packages/services/src/page/create.ts +++ b/packages/services/src/page/create.ts @@ -12,6 +12,7 @@ import { assertWithinLimit } from "../limits"; import type { Page } from "../types"; import { assertAccessTypeAllowed, + assertCustomDomainAvailable, assertSlugAvailable, validateMonitorIdsActive, } from "./internal"; @@ -26,6 +27,9 @@ export async function createPage(args: { requireScope(ctx, "write"); const input = CreatePageInput.parse(args.input); + if (input.customDomain && !ctx.workspace.limits["custom-domain"]) { + throw new LimitExceededError("custom-domain", 0); + } if (input.customTheme && !ctx.workspace.limits["custom-theme"]) { throw new LimitExceededError("custom-theme", 0); } @@ -44,6 +48,7 @@ export async function createPage(args: { limit: "status-pages", }); await assertSlugAvailable({ tx, slug: input.slug }); + await assertCustomDomainAvailable({ tx, customDomain: input.customDomain }); assertAccessTypeAllowed(ctx.workspace, { accessType: input.accessType ?? "public", passwordProtected: input.passwordProtected ?? null, diff --git a/packages/services/src/page/index.ts b/packages/services/src/page/index.ts index 0d65d39d9..8b98763d4 100644 --- a/packages/services/src/page/index.ts +++ b/packages/services/src/page/index.ts @@ -4,6 +4,7 @@ export { type StatusUpdateType, } from "./status-update-types"; export { deletePage } from "./delete"; +export { assertCustomDomainAvailable } from "./internal"; export { type MaintenanceContent, type StatusPageContent, diff --git a/packages/services/src/page/internal.ts b/packages/services/src/page/internal.ts index ae8afa799..bb570e9a4 100644 --- a/packages/services/src/page/internal.ts +++ b/packages/services/src/page/internal.ts @@ -62,6 +62,26 @@ export async function assertSlugAvailable(args: { } } +/** Assert no other page, in any workspace, already serves this custom domain. */ +export async function assertCustomDomainAvailable(args: { + tx: DB; + customDomain: string | null | undefined; + excludePageId?: number; +}): Promise { + const { tx, customDomain, excludePageId } = args; + if (!customDomain) return; + const rows = await tx + .select({ id: page.id }) + .from(page) + .where(sql`lower(${page.customDomain}) = ${customDomain.toLowerCase()}`) + .all(); + if (rows.some((r) => r.id !== excludePageId)) { + throw new ConflictError( + "This domain is already used by another status page.", + ); + } +} + /** Validate monitor ids against the workspace's active (non-soft-deleted) set. */ export async function validateMonitorIdsActive(args: { tx: DB; diff --git a/packages/services/src/page/update.ts b/packages/services/src/page/update.ts index fd31784ff..e0d90a615 100644 --- a/packages/services/src/page/update.ts +++ b/packages/services/src/page/update.ts @@ -7,6 +7,7 @@ import { type ServiceContext, withTransaction } from "../context"; import { LimitExceededError } from "../errors"; import { assertAccessTypeAllowed, + assertCustomDomainAvailable, assertSlugAvailable, getPageInWorkspace, } from "./internal"; @@ -85,6 +86,11 @@ export async function updatePageCustomDomain(args: { id: input.id, workspaceId: ctx.workspace.id, }); + await assertCustomDomainAvailable({ + tx, + customDomain: input.customDomain, + excludePageId: existing.id, + }); const updated = await tx .update(page) diff --git a/packages/services/src/sso/__tests__/sso.test.ts b/packages/services/src/sso/__tests__/sso.test.ts index 686f6fe0c..1a990164e 100644 --- a/packages/services/src/sso/__tests__/sso.test.ts +++ b/packages/services/src/sso/__tests__/sso.test.ts @@ -49,12 +49,17 @@ function fakeWorkOS( overrides: { organizationId?: string; domains?: { domain: string; state: string }[]; + deleted?: string[]; } = {}, ): WorkOSClient { const organizationId = overrides.organizationId ?? "org_test_1"; return { organizations: { createOrganization: () => Promise.resolve({ id: organizationId }), + deleteOrganization: (id) => { + overrides.deleted?.push(id); + return Promise.resolve(); + }, getOrganization: () => Promise.resolve({ domains: overrides.domains ?? [] }), }, @@ -180,6 +185,78 @@ describe("enableSso", () => { expect(again.organizationId).toBe("org_test_1"); }); }); + + test("deletes the WorkOS org it created when persisting fails", async () => { + await withTestTransaction(async (tx) => { + const deleted: string[] = []; + const workos = fakeWorkOS({ organizationId: "org_orphan", deleted }); + // Ownership is revoked while the WorkOS call is in flight, so the + // persisting transaction fails after the org already exists. + workos.organizations.createOrganization = async () => { + await tx + .update(usersToWorkspaces) + .set({ role: "member" }) + .where( + and( + eq(usersToWorkspaces.workspaceId, scaleCtx.workspace.id), + eq(usersToWorkspaces.userId, SCALE_OWNER_ID), + ), + ); + return { id: "org_orphan" }; + }; + + await expect( + enableSso({ ctx: { ...scaleCtx, db: tx, workos } }), + ).rejects.toBeInstanceOf(ForbiddenError); + expect(deleted).toEqual(["org_orphan"]); + + const row = await tx + .select() + .from(workspace) + .where(eq(workspace.id, scaleCtx.workspace.id)) + .get(); + expect(row?.workosOrganizationId).toBeNull(); + }); + }); + + test("keeps a concurrently persisted org and deletes its own", async () => { + await withTestTransaction(async (tx) => { + const deleted: string[] = []; + const workos = fakeWorkOS({ organizationId: "org_loser", deleted }); + // A concurrent enable lands its org between our read and our write. + workos.organizations.createOrganization = async () => { + await tx + .update(workspace) + .set({ workosOrganizationId: "org_winner" }) + .where(eq(workspace.id, scaleCtx.workspace.id)); + return { id: "org_loser" }; + }; + + const result = await enableSso({ ctx: { ...scaleCtx, db: tx, workos } }); + + expect(result.organizationId).toBe("org_winner"); + expect(deleted).toEqual(["org_loser"]); + }); + }); + + test("does not create or delete an org when one is already linked", async () => { + await withTestTransaction(async (tx) => { + await enableSso({ ctx: { ...scaleCtx, db: tx, workos: fakeWorkOS() } }); + + const deleted: string[] = []; + const workos = fakeWorkOS({ deleted }); + let created = 0; + workos.organizations.createOrganization = () => { + created++; + return Promise.resolve({ id: "org_unused" }); + }; + + await enableSso({ ctx: { ...scaleCtx, db: tx, workos } }); + + expect(created).toBe(0); + expect(deleted).toEqual([]); + }); + }); }); describe("disableSso", () => { diff --git a/packages/services/src/sso/client.ts b/packages/services/src/sso/client.ts index 8d921e3d2..35ec53a85 100644 --- a/packages/services/src/sso/client.ts +++ b/packages/services/src/sso/client.ts @@ -16,6 +16,7 @@ export type WorkOSConnectionState = export type WorkOSClient = { organizations: { createOrganization(payload: { name: string }): Promise<{ id: string }>; + deleteOrganization(id: string): Promise; getOrganization(id: string): Promise<{ domains: { domain: string; state: string }[]; }>; diff --git a/packages/services/src/sso/enable.ts b/packages/services/src/sso/enable.ts index 39af01993..1309e7d21 100644 --- a/packages/services/src/sso/enable.ts +++ b/packages/services/src/sso/enable.ts @@ -4,7 +4,11 @@ import { workspace } from "@openstatus/db/src/schema"; import { emitAudit } from "../audit"; import { requireScope } from "../auth"; import { type ServiceContext, withTransaction } from "../context"; -import { NotFoundError, PreconditionFailedError } from "../errors"; +import { + ConflictError, + NotFoundError, + PreconditionFailedError, +} from "../errors"; import { resolveWorkOS } from "./client"; import { requireOwner } from "./internal"; @@ -22,54 +26,103 @@ export async function enableSso(args: { const workos = resolveWorkOS(ctx.workos); - return withTransaction(ctx, async (tx) => { + // Owner check + current org, read up front so the WorkOS call below runs + // outside any DB transaction (no network I/O while holding the write lock). + const current = await withTransaction(ctx, async (tx) => { await requireOwner(tx, ctx); - - const existing = await tx + const row = await tx .select() .from(workspace) .where(eq(workspace.id, ctx.workspace.id)) .get(); - if (!existing) throw new NotFoundError("workspace", ctx.workspace.id); + if (!row) throw new NotFoundError("workspace", ctx.workspace.id); + return row; + }); - // Reuse the organization across enable/disable cycles so a workspace that - // re-upgrades keeps its already-configured IdP connection and domains. - const organizationId = - existing.workosOrganizationId ?? - ( + // Reuse the organization across enable/disable cycles so a workspace that + // re-upgrades keeps its already-configured IdP connection and domains. + const createdOrganizationId = current.workosOrganizationId + ? undefined + : ( await workos.organizations.createOrganization({ - name: existing.name || existing.slug, + name: current.name || current.slug, }) ).id; - const updated = await tx - .update(workspace) - .set({ - workosOrganizationId: organizationId, - ssoEnabled: true, - updatedAt: new Date(), - }) - .where(eq(workspace.id, ctx.workspace.id)) - .returning() - .get(); - - if (existing.workosOrganizationId) { - await emitAudit(tx, ctx, { - action: "workspace_sso.update", - entityType: "workspace_sso", - entityId: ctx.workspace.id, - before: existing, - after: updated, - }); - } else { - await emitAudit(tx, ctx, { - action: "workspace_sso.create", - entityType: "workspace_sso", - entityId: ctx.workspace.id, - after: updated, + // Best-effort: an org we just created but didn't persist would otherwise be + // orphaned in WorkOS, and the next enable would create another one. + const discardCreated = async () => { + if (!createdOrganizationId) return; + await workos.organizations + .deleteOrganization(createdOrganizationId) + .catch((err) => { + console.warn("enableSso: failed to delete orphaned WorkOS org", { + organizationId: createdOrganizationId, + err, + }); }); - } + }; - return { organizationId }; - }); + let persisted: { organizationId: string; reused: boolean }; + try { + persisted = await withTransaction(ctx, async (tx) => { + await requireOwner(tx, ctx); + + const existing = await tx + .select() + .from(workspace) + .where(eq(workspace.id, ctx.workspace.id)) + .get(); + if (!existing) throw new NotFoundError("workspace", ctx.workspace.id); + + // A concurrent enable may have persisted its own org between our read + // and this transaction — keep theirs so there's exactly one. + const organizationId = + existing.workosOrganizationId ?? createdOrganizationId; + if (!organizationId) { + // The org id was cleared between our read and this transaction. + throw new ConflictError("Workspace SSO changed concurrently, retry."); + } + + const updated = await tx + .update(workspace) + .set({ + workosOrganizationId: organizationId, + ssoEnabled: true, + updatedAt: new Date(), + }) + .where(eq(workspace.id, ctx.workspace.id)) + .returning() + .get(); + + if (existing.workosOrganizationId) { + await emitAudit(tx, ctx, { + action: "workspace_sso.update", + entityType: "workspace_sso", + entityId: ctx.workspace.id, + before: existing, + after: updated, + }); + } else { + await emitAudit(tx, ctx, { + action: "workspace_sso.create", + entityType: "workspace_sso", + entityId: ctx.workspace.id, + after: updated, + }); + } + + return { + organizationId, + reused: organizationId !== createdOrganizationId, + }; + }); + } catch (err) { + await discardCreated(); + throw err; + } + + if (persisted.reused) await discardCreated(); + + return { organizationId: persisted.organizationId }; } diff --git a/packages/services/src/status-report/__tests__/status-report.test.ts b/packages/services/src/status-report/__tests__/status-report.test.ts index 7d0179e68..3018cf3f4 100644 --- a/packages/services/src/status-report/__tests__/status-report.test.ts +++ b/packages/services/src/status-report/__tests__/status-report.test.ts @@ -1,4 +1,4 @@ -import { db, eq } from "@openstatus/db"; +import { db, eq, inArray } from "@openstatus/db"; import { page, pageComponent, @@ -56,6 +56,16 @@ const subscriptionSpies = (globalThis as Record) const TEST_PREFIX = "svc-status-report-test"; +function withoutSubscribers(ctx: ServiceContext): ServiceContext { + return { + ...ctx, + workspace: { + ...ctx.workspace, + limits: { ...ctx.workspace.limits, "status-subscribers": false }, + }, + }; +} + let teamCtx: ServiceContext; let freeCtx: ServiceContext; let testPageId: number; @@ -712,6 +722,29 @@ describe("notifyStatusReport", () => { ).rejects.toBeInstanceOf(ForbiddenError); }); }); + + test("returns false without dispatching when the plan has no subscribers", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const { initialUpdate } = await createStatusReport({ + ctx, + input: { + title: `${TEST_PREFIX}-notify-no-subscribers`, + status: "investigating", + message: "m", + date: new Date(), + pageId: testPageId, + pageComponentIds: [], + }, + }); + + const notified = await notifyStatusReport({ + ctx: withoutSubscribers(ctx), + input: { statusReportUpdateId: initialUpdate.id }, + }); + expect(notified).toBe(false); + }); + }); }); // Same derivation as `listMonitors` — a second copy of the logic, so it gets @@ -781,6 +814,52 @@ describe("listStatusReports totalSize", () => { }); }); +describe("listStatusReports ordering", () => { + test("reports created in the same second page in id order", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const ids: number[] = []; + for (let i = 0; i < 4; i++) { + const { statusReport: report } = await createStatusReport({ + ctx, + input: { + title: `${TEST_PREFIX}-same-second-${i}`, + status: "investigating", + message: "m", + date: new Date(), + pageId: otherPageId, + pageComponentIds: [], + }, + }); + ids.push(report.id); + } + await tx + .update(statusReport) + .set({ createdAt: new Date("2026-01-01T00:00:00Z") }) + .where(inArray(statusReport.id, ids)); + + for (const order of ["asc", "desc"] as const) { + const paged: number[] = []; + for (let offset = 0; offset < ids.length; offset++) { + const { items } = await listStatusReports({ + ctx, + input: { + limit: 1, + offset, + statuses: [], + order, + pageId: otherPageId, + }, + }); + paged.push(...items.map((item) => item.id)); + } + const sorted = [...ids].sort((a, b) => a - b); + expect(paged).toEqual(order === "asc" ? sorted : sorted.reverse()); + } + }); + }); +}); + describe("slack actor path", () => { test("createStatusReport succeeds with a slack actor", async () => { await withTestTransaction(async (tx) => { diff --git a/packages/services/src/status-report/list.ts b/packages/services/src/status-report/list.ts index e3cb71318..b7c68a596 100644 --- a/packages/services/src/status-report/list.ts +++ b/packages/services/src/status-report/list.ts @@ -276,10 +276,12 @@ export async function listStatusReports(args: { .select() .from(statusReport) .where(whereClause) + // `createdAt` has second precision, so the `id` tie-break keeps pages + // stable across reports created in the same second. .orderBy( - input.order === "asc" - ? asc(statusReport.createdAt) - : desc(statusReport.createdAt), + ...(input.order === "asc" + ? [asc(statusReport.createdAt), asc(statusReport.id)] + : [desc(statusReport.createdAt), desc(statusReport.id)]), ) .limit(input.limit) .offset(input.offset) diff --git a/packages/services/src/status-report/notify.ts b/packages/services/src/status-report/notify.ts index fcb2f4027..fde67fd63 100644 --- a/packages/services/src/status-report/notify.ts +++ b/packages/services/src/status-report/notify.ts @@ -16,11 +16,14 @@ import { NotifyStatusReportInput } from "./schemas"; * Enforces: * - Workspace owns the target update (via join to the parent report). * - Plan has `status-subscribers` enabled — otherwise no-op. + * + * Returns `true` when the update was dispatched, `false` when the plan + * skipped it. */ export async function notifyStatusReport(args: { ctx: ServiceContext; input: NotifyStatusReportInput; -}): Promise { +}): Promise { const { ctx } = args; requireScope(ctx, "write"); const input = NotifyStatusReportInput.parse(args.input); @@ -51,8 +54,9 @@ export async function notifyStatusReport(args: { if (!ctx.workspace.limits["status-subscribers"]) { // Plan does not enable subscribers — silently skip, matching existing // `emailRouter.sendStatusReport` behaviour. - return; + return false; } await dispatchStatusReportUpdate(input.statusReportUpdateId); + return true; } -- 2.54.0