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 220505808..9bf3933b3 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 @@ -2212,6 +2212,16 @@ describe("MonitorService - strict id parsing", () => { } }); + test("an id with CR/LF is InvalidArgument, not Internal", async () => { + const res = await connectRequest( + "GetMonitor", + { id: `${testHttpMonitorId}\r\nx-injected: 1` }, + { "x-openstatus-key": "1" }, + ); + expect(res.status).toBe(400); + expect(res.headers.get("error-reason")).toBe("MONITOR_ID_INVALID"); + }); + test("still accepts a plain decimal id", async () => { const res = await connectRequest( "GetMonitor", diff --git a/apps/server/src/routes/rpc/handlers/monitor/errors.ts b/apps/server/src/routes/rpc/handlers/monitor/errors.ts index 0055f50ec..ff5e383e6 100644 --- a/apps/server/src/routes/rpc/handlers/monitor/errors.ts +++ b/apps/server/src/routes/rpc/handlers/monitor/errors.ts @@ -92,7 +92,8 @@ export function monitorIdInvalidError(monitorId: string): ConnectError { `Invalid monitor id: "${monitorId}"`, Code.InvalidArgument, ErrorReason.MONITOR_ID_INVALID, - { "monitor-id": monitorId }, + // Raw input may hold CR/LF, which `Headers` rejects. + { "monitor-id": encodeURIComponent(monitorId) }, ); } 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 d30102b53..93c60ca83 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 @@ -771,4 +771,18 @@ describe("PrivateLocationService - strict id parsing", () => { .all(); expect(created.length).toBe(0); }); + + test("CreatePrivateLocation: a monitor id with CR/LF is InvalidArgument", async () => { + const res = await connectRequest( + "CreatePrivateLocation", + { + name: `${TEST_PREFIX}-crlf-monitor-ids`, + monitorIds: [`${testMonitorId}\r\nx-injected: 1`], + }, + { "x-openstatus-key": authKey }, + ); + + expect(res.status).toBe(400); + expect(res.headers.get("error-reason")).toBe("INVALID_MONITOR_ID"); + }); }); diff --git a/apps/server/src/routes/rpc/handlers/private-location/errors.ts b/apps/server/src/routes/rpc/handlers/private-location/errors.ts index 612981bbe..9068867bd 100644 --- a/apps/server/src/routes/rpc/handlers/private-location/errors.ts +++ b/apps/server/src/routes/rpc/handlers/private-location/errors.ts @@ -54,6 +54,7 @@ export function invalidMonitorIdError(monitorId: string): ConnectError { `Invalid monitor id: "${monitorId}"`, Code.InvalidArgument, ErrorReason.INVALID_MONITOR_ID, - { "monitor-id": monitorId }, + // Raw input may hold CR/LF, which `Headers` rejects. + { "monitor-id": encodeURIComponent(monitorId) }, ); } diff --git a/apps/server/src/routes/v1/pages/put.test.ts b/apps/server/src/routes/v1/pages/put.test.ts index 88ef01e54..d34c68145 100644 --- a/apps/server/src/routes/v1/pages/put.test.ts +++ b/apps/server/src/routes/v1/pages/put.test.ts @@ -119,3 +119,17 @@ test("update the page keeping its own custom domain", async () => { expect(res.status).toBe(200); }); + +test("update a missing page with a taken custom domain should return 404", async () => { + const res = await app.request("/v1/page/999999", { + 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(404); +}); diff --git a/apps/server/src/routes/v1/pages/put.ts b/apps/server/src/routes/v1/pages/put.ts index b76eb905f..a5cd2d837 100644 --- a/apps/server/src/routes/v1/pages/put.ts +++ b/apps/server/src/routes/v1/pages/put.ts @@ -70,22 +70,6 @@ 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 @@ -129,6 +113,22 @@ export function registerPutPage(api: typeof pagesApi) { }); } + try { + await assertCustomDomainAvailable({ + tx: db, + customDomain: input.customDomain, + excludePageId: _page.id, + }); + } catch (err) { + if (err instanceof ConflictError) { + throw new OpenStatusApiError({ + code: "CONFLICT", + message: err.message, + }); + } + throw err; + } + if (input.slug && _page.slug !== input.slug) { if (subdomainSafeList.includes(input.slug)) { throw new OpenStatusApiError({ diff --git a/packages/api/src/router/page.custom-domain.test.ts b/packages/api/src/router/page.custom-domain.test.ts new file mode 100644 index 000000000..4f16bd64d --- /dev/null +++ b/packages/api/src/router/page.custom-domain.test.ts @@ -0,0 +1,48 @@ +import { db, eq } from "@openstatus/db"; +import { + page, + selectWorkspaceSchema, + workspace, +} from "@openstatus/db/src/schema"; +import { expect } from "@std/expect"; +import { test } from "@std/testing/bdd"; +import { TRPCError } from "@trpc/server"; + +import { edgeRouter } from "../edge"; +import { createInnerTRPCContext } from "../trpc"; + +test("page.updateCustomDomain rejects a taken domain before touching Vercel", async () => { + const ctx = createInnerTRPCContext({ + req: undefined, + session: { user: { id: "1" } }, + workspace: selectWorkspaceSchema.parse( + await db.select().from(workspace).where(eq(workspace.id, 1)).get(), + ), + }); + const caller = edgeRouter.createCaller(ctx); + const before = await db.select().from(page).where(eq(page.id, 1)).get(); + + const realFetch = globalThis.fetch; + let fetchCalls = 0; + globalThis.fetch = (...args) => { + fetchCalls++; + return realFetch(...args); + }; + try { + await caller.page.updateCustomDomain({ + id: 1, + // Seeded on page 2. + customDomain: "status.acme.com", + }); + throw new Error("Should have thrown"); + } catch (e) { + expect(e).toBeInstanceOf(TRPCError); + expect((e as TRPCError).code).toBe("CONFLICT"); + } finally { + globalThis.fetch = realFetch; + } + expect(fetchCalls).toBe(0); + + const after = await db.select().from(page).where(eq(page.id, 1)).get(); + expect(after?.customDomain).toBe(before?.customDomain); +}); diff --git a/packages/api/src/router/page.ts b/packages/api/src/router/page.ts index b66e80d18..67549ee06 100644 --- a/packages/api/src/router/page.ts +++ b/packages/api/src/router/page.ts @@ -3,6 +3,7 @@ import { locales } from "@openstatus/locales"; import { LimitExceededError, NotFoundError } from "@openstatus/services"; import { getUptimeHistory } from "@openstatus/services/frozen-uptime"; import { + assertCustomDomainAvailable, createPage, type CreatePageInput, // `CreatePageInput` re-exports the drizzle insert schema so routers @@ -230,6 +231,14 @@ export const pageRouter = createTRPCRouter({ // doesn't lowercase, so a case-only re-save must not re-add either if (newDomain.toLowerCase() === oldDomain.toLowerCase()) return; + // Before touching Vercel: a conflict after the add/remove below + // would leave Vercel out of sync with the row. + await assertCustomDomainAvailable({ + tx: ctx.db, + customDomain: newDomain, + excludePageId: input.id, + }); + if (newDomain) { await addDomainToVercel(newDomain); } diff --git a/packages/api/src/router/statusPage.subscribe.test.ts b/packages/api/src/router/statusPage.subscribe.test.ts index 67559e284..c64c8a546 100644 --- a/packages/api/src/router/statusPage.subscribe.test.ts +++ b/packages/api/src/router/statusPage.subscribe.test.ts @@ -15,6 +15,10 @@ let workspaceId: number; let pageId: number; let slug: string; +// No external email I/O in tests: capture outgoing requests instead. +const realFetch = globalThis.fetch; +const sent: { url: string; body: string }[] = []; + function caller() { return appRouter.createCaller( createInnerTRPCContext({ @@ -25,6 +29,16 @@ function caller() { } beforeAll(async () => { + globalThis.fetch = (input, init) => { + const url = input instanceof Request ? input.url : String(input); + sent.push({ url, body: String(init?.body ?? "") }); + return Promise.resolve( + new Response(JSON.stringify({ id: "email_test" }), { + status: 200, + headers: { "content-type": "application/json" }, + }), + ); + }; const fixture = await createTestWorkspace(); workspaceId = fixture.workspace.id; const _page = await createPage(workspaceId); @@ -33,6 +47,7 @@ beforeAll(async () => { }); afterAll(async () => { + globalThis.fetch = realFetch; await db.delete(pageSubscriber).where(eq(pageSubscriber.pageId, pageId)); await db.delete(page).where(eq(page.workspaceId, workspaceId)); }); @@ -55,6 +70,10 @@ describe("statusPage.subscribe", () => { expect(row?.token).toBeTruthy(); expect(row?.acceptedAt).toBeNull(); expect(JSON.stringify(result)).not.toContain(row?.token ?? ""); + + // The verification link went to the subscriber's mailbox instead. + const email = sent.find((r) => r.body.includes("someone-else@example.com")); + expect(email?.body).toContain(`/verify/${row?.token}`); }); test("the public token-based verification email endpoint is gone", () => { diff --git a/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts b/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts index 6c7b39540..c0174f80d 100644 --- a/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts +++ b/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts @@ -130,6 +130,47 @@ describe("writeIncidentsPhase idempotency key", () => { }); }); +describe("writeIncidentsPhase first-update match", () => { + test("a later update at the same instant doesn't make a new incident a duplicate", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const pageRow = await insertPage(tx, ctx.workspace.id, "later-update"); + const title = `${TEST_PREFIX}-elevated-errors`; + const pc = { ctx, tx, provider: "statuspage" as const }; + + // First incident: starts Jan 1, its last update is Jan 2. + const first: PhaseResult = { + phase: "incidents", + status: "completed", + resources: [ + incident("inc_a", title, pageRow.id, ctx.workspace.id, [ + new Date("2024-01-01T00:00:00Z"), + new Date("2024-01-02T00:00:00Z"), + ]), + ], + }; + await writeIncidentsPhase(pc, first, pageRow.id, new Map()); + + // A different incident that *starts* when the first one last updated. + const second: PhaseResult = { + phase: "incidents", + status: "completed", + resources: [ + incident("inc_b", title, pageRow.id, ctx.workspace.id, [ + new Date("2024-01-02T00:00:00Z"), + ]), + ], + }; + await writeIncidentsPhase(pc, second, pageRow.id, new Map()); + + expect(second.resources[0].status).toBe("created"); + expect(second.resources[0].openstatusId).not.toBe( + first.resources[0].openstatusId, + ); + }); + }); +}); + // 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. @@ -563,6 +604,7 @@ describe("writePagePhase with a taken custom domain", () => { ); const created = await tx.select().from(page).where(eq(page.id, id)).get(); expect(created?.customDomain).toBe(""); + expect(phase.resources[0].error).toContain("already used"); }); }); }); diff --git a/packages/services/src/import/phase-writers.ts b/packages/services/src/import/phase-writers.ts index 986f0ce6d..9d0cf3bce 100644 --- a/packages/services/src/import/phase-writers.ts +++ b/packages/services/src/import/phase-writers.ts @@ -1,4 +1,4 @@ -import { and, count, eq, isNull } from "@openstatus/db"; +import { and, asc, count, eq, isNull } from "@openstatus/db"; import { maintenance, maintenanceUpdate, @@ -120,18 +120,6 @@ 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 @@ -168,6 +156,21 @@ export async function writePagePhase( return existingBySlug.id; } + // Same tolerance when another page already holds the domain. Checked only + // on the insert path: a skipped existing page never writes the domain. + if (data.customDomain) { + try { + await assertCustomDomainAvailable({ + tx, + customDomain: data.customDomain, + }); + } catch (err) { + if (!(err instanceof ConflictError)) throw err; + resource.error = `Custom domain ${data.customDomain} is already used by another status page; created without it.`; + data.customDomain = ""; + } + } + const [inserted] = await tx .insert(page) .values({ @@ -392,6 +395,49 @@ export async function writeComponentsPhase( phase.status = computePhaseStatus(phase.resources); } +// A same-title report counts as this incident only when its *earliest* +// update is at `firstUpdateDate` — a later update of another incident +// landing on that instant must not match. +async function findReportByFirstUpdate( + tx: DB, + args: { + pageId: number; + workspaceId: number; + title: string; + firstUpdateDate: Date; + }, +): Promise<{ id: number } | undefined> { + const candidates = await tx + .selectDistinct({ id: statusReport.id }) + .from(statusReport) + .innerJoin( + statusReportUpdate, + eq(statusReportUpdate.statusReportId, statusReport.id), + ) + .where( + and( + eq(statusReport.pageId, args.pageId), + eq(statusReport.workspaceId, args.workspaceId), + eq(statusReport.title, args.title), + eq(statusReportUpdate.date, args.firstUpdateDate), + ), + ) + .all(); + for (const candidate of candidates) { + const earliest = await tx + .select({ date: statusReportUpdate.date }) + .from(statusReportUpdate) + .where(eq(statusReportUpdate.statusReportId, candidate.id)) + .orderBy(asc(statusReportUpdate.date)) + .limit(1) + .get(); + if (earliest?.date.getTime() === args.firstUpdateDate.getTime()) { + return candidate; + } + } + return undefined; +} + export async function writeIncidentsPhase( pc: PhaseContext, phase: PhaseResult, @@ -427,22 +473,12 @@ export async function writeIncidentsPhase( // 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 findReportByFirstUpdate(tx, { + pageId, + workspaceId, + title: data.report.title, + firstUpdateDate, + }) : await tx .select({ id: statusReport.id }) .from(statusReport) diff --git a/packages/services/src/import/preview.ts b/packages/services/src/import/preview.ts index 90d6d818f..862a91cfb 100644 --- a/packages/services/src/import/preview.ts +++ b/packages/services/src/import/preview.ts @@ -1,9 +1,9 @@ -import { and, db as defaultDb, eq } from "@openstatus/db"; -import { page } from "@openstatus/db/src/schema"; +import { db as defaultDb } from "@openstatus/db"; import type { ImportSummary } from "@openstatus/importers"; import type { ServiceContext } from "../context"; -import { NotFoundError, ValidationError } from "../errors"; +import { ValidationError } from "../errors"; +import { getPageInWorkspace } from "../page/internal"; import { addLimitWarnings } from "./limits"; import { buildProviderConfig, createProvider } from "./provider"; import { PreviewImportInput } from "./schemas"; @@ -22,16 +22,12 @@ 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); + if (input.pageId !== undefined) { + await getPageInWorkspace({ + tx: db, + id: input.pageId, + workspaceId: ctx.workspace.id, + }); } const provider = createProvider(input.provider); diff --git a/packages/services/src/import/run.ts b/packages/services/src/import/run.ts index 9eab9de5c..dcd553419 100644 --- a/packages/services/src/import/run.ts +++ b/packages/services/src/import/run.ts @@ -1,10 +1,11 @@ -import { and, count, db as defaultDb, eq } from "@openstatus/db"; -import { page, pageComponent } from "@openstatus/db/src/schema"; +import { count, db as defaultDb, eq } from "@openstatus/db"; +import { pageComponent } from "@openstatus/db/src/schema"; import type { ImportSummary } from "@openstatus/importers"; import { requireScope } from "../auth"; import type { ServiceContext } from "../context"; -import { NotFoundError, ValidationError } from "../errors"; +import { ValidationError } from "../errors"; +import { getPageInWorkspace } from "../page/internal"; import { addLimitWarnings } from "./limits"; import { type PhaseContext, @@ -48,16 +49,12 @@ export async function runImport(args: { // Verify pageId belongs to the workspace before doing any provider work. // Was previously duplicated at the router layer — owning this in the // service means all callers (tRPC / Slack / future) get the same check. - if (input.pageId) { - const existing = await tx - .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); + if (input.pageId !== undefined) { + await getPageInWorkspace({ + tx: tx, + id: input.pageId, + workspaceId: ctx.workspace.id, + }); } const provider = createProvider(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 9798e7b98..1b8b4e09a 100644 --- a/packages/services/src/incident/__tests__/postmortem-draft.test.ts +++ b/packages/services/src/incident/__tests__/postmortem-draft.test.ts @@ -114,12 +114,58 @@ describe("collectChannelTranscript", () => { const { text, truncated } = await collectChannelTranscript(client, "C1"); const lines = text.split("\n"); expect(lines).toHaveLength(500); + // An oversized thread keeps its parent and its newest replies. expect(lines[0]).toContain("start"); - expect(text).toContain("reply 498"); - expect(text).not.toContain("reply 499"); + expect(lines[1]).toContain("reply 101"); + expect(lines.at(-1)).toContain("reply 599"); + expect(text).not.toContain("reply 100\n"); expect(truncated).toBe(true); }); + test("a thread that doesn't fit under the cap is dropped whole", async () => { + // Newest: 490 single messages. Older: one parent with 20 replies. + const top = [ + { ts: "100.0", text: "old parent", user: "U1", reply_count: 20 }, + ...Array.from({ length: 490 }, (_, i) => ({ + ts: `${200 + i}.0`, + text: `message ${i}`, + user: "U1", + })), + ]; + const client = history({ + top, + threads: { + "100.0": Array.from({ length: 20 }, (_, i) => ({ + ts: `100.${i + 1}`, + text: `old reply ${i}`, + user: "U2", + })), + }, + }); + const { text, truncated } = await collectChannelTranscript(client, "C1"); + expect(text).not.toContain("old parent"); + expect(text).not.toContain("old reply"); + expect(text.split("\n")).toHaveLength(490); + expect(truncated).toBe(true); + }); + + test("a skipped message with no content doesn't mark the transcript truncated", async () => { + const top = [ + { ts: "99.0", text: "Incident card", bot_id: "B1" }, + ...Array.from({ length: 500 }, (_, i) => ({ + ts: `${100 + i}.0`, + text: `message ${i}`, + user: "U1", + })), + ]; + const { text, truncated } = await collectChannelTranscript( + history({ top }), + "C1", + ); + expect(text.split("\n")).toHaveLength(500); + expect(truncated).toBe(false); + }); + test("over the cap the oldest messages go, the newest stay", async () => { const top = Array.from({ length: 600 }, (_, i) => ({ ts: `${1000 + i}.0`, @@ -167,6 +213,23 @@ describe("collectChannelTranscript", () => { expect(lines[2]).toContain(": 4 x"); expect(truncated).toBe(true); }); + + test("the character cap drops a parent together with its replies", async () => { + const big = "x".repeat(400_000); + const client = history({ + top: [ + { ts: "100.0", text: `old ${big}`, user: "U1", reply_count: 1 }, + { ts: "101.0", text: `new ${big}`, user: "U1" }, + { ts: "102.0", text: `newer ${big}`, user: "U1" }, + ], + threads: { "100.0": [{ ts: "100.5", text: "old reply", user: "U2" }] }, + }); + const { text, truncated } = await collectChannelTranscript(client, "C1"); + expect(text).not.toContain("old reply"); + expect(text).not.toContain(": old x"); + expect(text.split("\n")).toHaveLength(2); + 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 148d67b30..af47dff07 100644 --- a/packages/services/src/incident/postmortem-draft.ts +++ b/packages/services/src/incident/postmortem-draft.ts @@ -68,8 +68,8 @@ 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 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. + * the oldest top-level messages first, each with its whole thread: the end of + * an incident (mitigation, resolution) matters more than its start. */ export async function collectChannelTranscript( client: SlackHistoryClient, @@ -92,57 +92,68 @@ export async function collectChannelTranscript( } } while (cursor); - // 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. + // Walk newest first, one block (a parent and its thread) at a time, so the + // caps drop the oldest blocks whole and never split a parent from its + // replies. Only an oversized newest block is cut, keeping its parent and + // its newest replies. const blocks: string[][] = []; let count = 0; - collect: for (const message of top) { - if (count >= MAX_MESSAGES) { + for (const message of top) { + const block: string[] = []; + // A bot parent (the incident card) is left out, its human replies are not. + if (!message.bot_id && message.text) block.push(line(message)); + if (message.reply_count && message.ts) { + let replyCursor: string | undefined; + do { + const thread = await client.conversations.replies({ + channel, + ts: message.ts, + cursor: replyCursor, + limit: 200, + }); + for (const reply of thread.messages ?? []) { + if (reply.ts === message.ts || reply.bot_id || !reply.text) continue; + block.push(` ${line(reply)}`); + } + replyCursor = thread.has_more + ? thread.response_metadata?.next_cursor + : undefined; + } while (replyCursor); + } + if (block.length === 0) continue; + if (count + block.length > MAX_MESSAGES) { truncated = true; + if (blocks.length === 0) blocks.push(keepNewest(block, MAX_MESSAGES)); break; } - 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; - let replyCursor: string | undefined; - do { - const thread = await client.conversations.replies({ - channel, - ts: message.ts, - cursor: replyCursor, - limit: 200, - }); - for (const reply of thread.messages ?? []) { - if (reply.ts === message.ts || reply.bot_id || !reply.text) continue; - if (!push(` ${line(reply)}`)) break collect; - } - replyCursor = thread.has_more - ? thread.response_metadata?.next_cursor - : undefined; - } while (replyCursor); + count += block.length; + } + const ordered = blocks.reverse(); + const size = (block: string[]) => + block.reduce((sum, entry) => sum + entry.length + 1, 0); + // Over the character cap, drop whole blocks from the oldest end. + let length = ordered.reduce((sum, block) => sum + size(block), 0) - 1; + while (length > MAX_TRANSCRIPT_CHARS && ordered.length > 1) { + length -= size(ordered.shift() ?? []); + truncated = true; } - 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; + const lines = ordered.flat(); + // A single block still over the cap loses its oldest replies, then lines. + while (length > MAX_TRANSCRIPT_CHARS && lines.length > 0) { + const drop = lines.length > 1 && !lines[0].startsWith(" ") ? 1 : 0; + length -= lines[drop].length + 1; + lines.splice(drop, 1); truncated = true; } - const text = lines.slice(first).join("\n"); - return { text, truncated }; + return { text: lines.join("\n"), truncated }; +} + +/** A block's parent (when it has one) plus its newest lines, `max` in all. */ +function keepNewest(block: string[], max: number): string[] { + const hasParent = !block[0].startsWith(" "); + if (!hasParent) return block.slice(-max); + return [block[0], ...block.slice(1).slice(-(max - 1))]; } const SYSTEM = `You write blameless incident postmortems for an engineering team. diff --git a/packages/services/src/invitation/accept.ts b/packages/services/src/invitation/accept.ts index 826dce9cf..e1b297bae 100644 --- a/packages/services/src/invitation/accept.ts +++ b/packages/services/src/invitation/accept.ts @@ -19,7 +19,7 @@ import { UnauthorizedError, } from "../errors"; import type { Workspace } from "../types"; -import { UNLIMITED_MEMBERS_EFFECTIVE_CAP } from "./create"; +import { UNLIMITED_MEMBERS_EFFECTIVE_CAP } from "./constants"; import { AcceptInvitationInput } from "./schemas"; /** diff --git a/packages/services/src/invitation/constants.ts b/packages/services/src/invitation/constants.ts new file mode 100644 index 000000000..ed8edaba8 --- /dev/null +++ b/packages/services/src/invitation/constants.ts @@ -0,0 +1,6 @@ +/** + * "Unlimited" members resolves to this effective cap — matches the legacy + * behavior in `invitationRouter.create`. Large enough that enterprise plans + * never brush against it in practice. + */ +export const UNLIMITED_MEMBERS_EFFECTIVE_CAP = 420; diff --git a/packages/services/src/invitation/create.ts b/packages/services/src/invitation/create.ts index 1baea693e..c320c6056 100644 --- a/packages/services/src/invitation/create.ts +++ b/packages/services/src/invitation/create.ts @@ -11,15 +11,9 @@ import { type ServiceContext, withTransaction } from "../context"; import { env } from "../env"; import { LimitExceededError } from "../errors"; import type { Invitation } from "../types"; +import { UNLIMITED_MEMBERS_EFFECTIVE_CAP } from "./constants"; import { CreateInvitationInput } from "./schemas"; -/** - * "Unlimited" members resolves to this effective cap — matches the legacy - * behavior in `invitationRouter.create`. Large enough that enterprise plans - * never brush against it in practice. - */ -export const UNLIMITED_MEMBERS_EFFECTIVE_CAP = 420; - /** * Invite a user to the caller's workspace. * diff --git a/packages/services/src/page-subscriber/slack.ts b/packages/services/src/page-subscriber/slack.ts index e335b1c7c..23e2c670a 100644 --- a/packages/services/src/page-subscriber/slack.ts +++ b/packages/services/src/page-subscriber/slack.ts @@ -36,6 +36,15 @@ function channelLabel(channelId: string, channelName?: string): string { return channelName ? `#${channelName}` : channelId; } +// A channel can't present a password, email or IP, so only public pages. +function assertSlackSubscribable(accessType: string | null) { + if (accessType !== "public") { + throw new PreconditionFailedError( + "Protected status pages can't be subscribed to from Slack.", + ); + } +} + /** * Self-signup for a Slack channel via `/openstatus subscribe `. There is no * authenticated workspace at the call site — both workspace and audit actor @@ -58,12 +67,7 @@ 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.", - ); - } + assertSlackSubscribable(pageData.accessType); const workspace = parseWorkspaceForContext(pageData.workspace); assertSubscribersAllowed(workspace); @@ -75,6 +79,16 @@ export async function createSlackSubscriber(args: { const name = channelLabel(input.channelId, input.channelName); return withTransaction({ db: args.db } as ServiceContext, async (tx) => { + // Re-read inside the write so a page protected after the read above + // can't still gain a subscriber. + const current = await tx + .select({ accessType: page.accessType }) + .from(page) + .where(eq(page.id, input.pageId)) + .get(); + if (!current) throw new NotFoundError("page", input.pageId); + assertSlackSubscribable(current.accessType); + const existing = await tx.query.pageSubscriber.findFirst({ where: and( eq(pageSubscriber.pageId, input.pageId), diff --git a/packages/services/src/sso/__tests__/sso.test.ts b/packages/services/src/sso/__tests__/sso.test.ts index 1a990164e..cb35addfb 100644 --- a/packages/services/src/sso/__tests__/sso.test.ts +++ b/packages/services/src/sso/__tests__/sso.test.ts @@ -219,6 +219,34 @@ describe("enableSso", () => { }); }); + test("refuses to enable when the add-on is removed mid-enable", async () => { + await withTestTransaction(async (tx) => { + const deleted: string[] = []; + const workos = fakeWorkOS({ organizationId: "org_downgraded", deleted }); + // A downgrade drops the add-on while the WorkOS call is in flight. + workos.organizations.createOrganization = async () => { + await tx + .update(workspace) + .set({ limits: JSON.stringify({ sso: false }) }) + .where(eq(workspace.id, scaleCtx.workspace.id)); + return { id: "org_downgraded" }; + }; + + await expect( + enableSso({ ctx: { ...scaleCtx, db: tx, workos } }), + ).rejects.toBeInstanceOf(PreconditionFailedError); + expect(deleted).toEqual(["org_downgraded"]); + + const row = await tx + .select() + .from(workspace) + .where(eq(workspace.id, scaleCtx.workspace.id)) + .get(); + expect(row?.ssoEnabled).toBe(false); + expect(row?.workosOrganizationId).toBeNull(); + }); + }); + test("keeps a concurrently persisted org and deletes its own", async () => { await withTestTransaction(async (tx) => { const deleted: string[] = []; diff --git a/packages/services/src/sso/enable.ts b/packages/services/src/sso/enable.ts index 1309e7d21..ca7de60b0 100644 --- a/packages/services/src/sso/enable.ts +++ b/packages/services/src/sso/enable.ts @@ -1,5 +1,5 @@ -import { eq } from "@openstatus/db"; -import { workspace } from "@openstatus/db/src/schema"; +import { db as defaultDb, eq } from "@openstatus/db"; +import { selectWorkspaceSchema, workspace } from "@openstatus/db/src/schema"; import { emitAudit } from "../audit"; import { requireScope } from "../auth"; @@ -74,6 +74,12 @@ export async function enableSso(args: { .where(eq(workspace.id, ctx.workspace.id)) .get(); if (!existing) throw new NotFoundError("workspace", ctx.workspace.id); + // The add-on may have been removed (downgrade) while WorkOS was called. + if (!selectWorkspaceSchema.parse(existing).limits.sso) { + throw new PreconditionFailedError( + "SSO has not been added to this workspace", + ); + } // A concurrent enable may have persisted its own org between our read // and this transaction — keep theirs so there's exactly one. @@ -118,7 +124,17 @@ export async function enableSso(args: { }; }); } catch (err) { - await discardCreated(); + // The commit may have landed even though we saw an error (e.g. a lost + // response) — never delete an org the workspace now references. + const row = await (ctx.db ?? defaultDb) + .select({ workosOrganizationId: workspace.workosOrganizationId }) + .from(workspace) + .where(eq(workspace.id, ctx.workspace.id)) + .get() + .catch(() => undefined); + if (row && row.workosOrganizationId !== createdOrganizationId) { + await discardCreated(); + } throw err; } 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 3018cf3f4..3bb4ecab9 100644 --- a/packages/services/src/status-report/__tests__/status-report.test.ts +++ b/packages/services/src/status-report/__tests__/status-report.test.ts @@ -745,6 +745,34 @@ describe("notifyStatusReport", () => { expect(notified).toBe(false); }); }); + + test("returns false when the report has no page", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const { statusReport: report, initialUpdate } = await createStatusReport({ + ctx, + input: { + title: `${TEST_PREFIX}-notify-no-page`, + status: "investigating", + message: "m", + date: new Date(), + pageId: testPageId, + pageComponentIds: [], + }, + }); + await tx + .update(statusReport) + .set({ pageId: null }) + .where(eq(statusReport.id, report.id)) + .run(); + + const notified = await notifyStatusReport({ + ctx, + input: { statusReportUpdateId: initialUpdate.id }, + }); + expect(notified).toBe(false); + }); + }); }); // Same derivation as `listMonitors` — a second copy of the logic, so it gets diff --git a/packages/services/src/status-report/notify.ts b/packages/services/src/status-report/notify.ts index fde67fd63..1e1ab98b9 100644 --- a/packages/services/src/status-report/notify.ts +++ b/packages/services/src/status-report/notify.ts @@ -17,8 +17,8 @@ import { NotifyStatusReportInput } from "./schemas"; * - 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. + * Returns `true` when the update was dispatched, `false` when it was skipped + * (plan without subscribers, or a report not attached to a page). */ export async function notifyStatusReport(args: { ctx: ServiceContext; @@ -33,6 +33,7 @@ export async function notifyStatusReport(args: { .select({ updateId: statusReportUpdate.id, reportWorkspaceId: statusReport.workspaceId, + reportPageId: statusReport.pageId, }) .from(statusReportUpdate) .innerJoin( @@ -56,6 +57,8 @@ export async function notifyStatusReport(args: { // `emailRouter.sendStatusReport` behaviour. return false; } + // The dispatcher silently drops reports without a page. + if (!row.reportPageId) return false; await dispatchStatusReportUpdate(input.statusReportUpdateId); return true;