From cf7a4f480d58d6b1dc3027c6a0bc226252a30a25 Mon Sep 17 00:00:00 2001 From: Thibault Le Ouay Ducasse Date: Sun, 26 Jul 2026 21:03:36 +0200 Subject: [PATCH] api improvment --- .../monitor/__tests__/monitor.test.ts | 135 +++++++++++++ .../src/routes/rpc/handlers/monitor/index.ts | 41 ++-- .../src/routes/rpc/handlers/monitor/limits.ts | 47 +++-- .../routes/rpc/handlers/monitor/validators.ts | 12 +- .../status-page/__tests__/status-page.test.ts | 189 +++++++++++++++++- .../routes/rpc/handlers/status-page/index.ts | 62 +++--- .../__tests__/status-report.test.ts | 65 ++++++ .../rpc/handlers/status-report/index.ts | 11 +- 8 files changed, 483 insertions(+), 79 deletions(-) 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 8b5b8e46..6e37d350 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 @@ -979,6 +979,47 @@ describe("MonitorService.UpdateHTTPMonitor", () => { expect(res.status).toBe(401); }); + + // These three fields have implicit presence in the proto, so an omitted + // field decodes to false/false/"" — a name-only update used to persist that. + test("partial update preserves active, public and description", async () => { + const mon = await db + .insert(monitor) + .values({ + workspaceId: 1, + name: `${TEST_PREFIX}-preserve`, + url: "https://preserve.example.com", + periodicity: "1m", + active: true, + public: true, + description: "keep me", + regions: "ams", + jobType: "http", + }) + .returning() + .get(); + + try { + const res = await connectRequest( + "UpdateHTTPMonitor", + { + id: String(mon.id), + monitor: { name: `${TEST_PREFIX}-preserve-renamed` }, + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(200); + + const data = await res.json(); + expect(data.monitor.name).toBe(`${TEST_PREFIX}-preserve-renamed`); + expect(data.monitor.active).toBe(true); + expect(data.monitor.public).toBe(true); + expect(data.monitor.description).toBe("keep me"); + } finally { + await db.delete(monitor).where(eq(monitor.id, mon.id)); + } + }); }); describe("MonitorService.UpdateTCPMonitor", () => { @@ -1779,6 +1820,100 @@ describe("MonitorService - Limits", () => { const data = await res.json(); expect(data.message).toContain("periodicity"); }); + + // The free plan allows a single monitor, so one row puts workspace 2 at its + // cap — which is the normal state for any workspace on its plan limit. + async function seedFreePlanMonitorAtCap(suffix: string) { + return db + .insert(monitor) + .values({ + workspaceId: 2, + name: `${TEST_PREFIX}-at-cap-${suffix}`, + url: `https://at-cap-${suffix}.example.com`, + periodicity: "10m", + active: true, + regions: "ams", + jobType: "http", + }) + .returning() + .get(); + } + + // Regression: the row-count cap is a create-time check. A workspace sitting + // at its monitor limit must still be able to edit the monitors it has. + test("workspace at its monitor cap can still update an existing monitor", async () => { + const mon = await seedFreePlanMonitorAtCap("update"); + + try { + const res = await connectRequest( + "UpdateHTTPMonitor", + { + id: String(mon.id), + monitor: { periodicity: "PERIODICITY_30M" }, + }, + { "x-openstatus-key": FREE_PLAN_KEY }, + ); + + expect(res.status).toBe(200); + const data = await res.json(); + expect(data.monitor.periodicity).toBe("PERIODICITY_30M"); + } finally { + await db.delete(monitor).where(eq(monitor.id, mon.id)); + } + }); + + test("update still enforces the plan's periodicity limit", async () => { + const mon = await seedFreePlanMonitorAtCap("periodicity"); + + try { + const res = await connectRequest( + "UpdateHTTPMonitor", + { + id: String(mon.id), + monitor: { periodicity: "PERIODICITY_30S" }, + }, + { "x-openstatus-key": FREE_PLAN_KEY }, + ); + + expect(res.status).toBe(403); + const data = await res.json(); + expect(data.message).toContain("periodicity"); + } finally { + await db.delete(monitor).where(eq(monitor.id, mon.id)); + } + }); + + test("update still enforces the plan's max-regions limit", async () => { + const mon = await seedFreePlanMonitorAtCap("regions"); + + try { + const res = await connectRequest( + "UpdateHTTPMonitor", + { + id: String(mon.id), + monitor: { + regions: [ + "REGION_FLY_AMS", + "REGION_FLY_IAD", + "REGION_FLY_SIN", + "REGION_FLY_LHR", + "REGION_FLY_SYD", + "REGION_FLY_NRT", + "REGION_FLY_FRA", + "REGION_FLY_GRU", + ], + }, + }, + { "x-openstatus-key": FREE_PLAN_KEY }, + ); + + expect(res.status).toBe(403); + const data = await res.json(); + expect(data.message).toContain("region"); + } finally { + await db.delete(monitor).where(eq(monitor.id, mon.id)); + } + }); }); describe("MonitorService - Status Field", () => { diff --git a/apps/server/src/routes/rpc/handlers/monitor/index.ts b/apps/server/src/routes/rpc/handlers/monitor/index.ts index 885e3057..fa345834 100644 --- a/apps/server/src/routes/rpc/handlers/monitor/index.ts +++ b/apps/server/src/routes/rpc/handlers/monitor/index.ts @@ -68,7 +68,7 @@ import { responseLogNotFoundError, responseLogsNotEnabledError, } from "./errors"; -import { checkMonitorLimits } from "./limits"; +import { checkMonitorConfigLimits, checkMonitorLimits } from "./limits"; import { toHTTPResponseLogDetail, toHTTPResponseLogListItem, @@ -347,14 +347,11 @@ export const monitorServiceImpl: ServiceImpl = { validateCommonMonitorFields(mon); // Check workspace limits if periodicity or regions are changing - if (mon.periodicity || (mon.regions && mon.regions.length > 0)) { - await checkMonitorLimits( - workspaceId, - limits, - mon.periodicity || undefined, - mon.regions && mon.regions.length > 0 ? mon.regions : undefined, - ); - } + checkMonitorConfigLimits( + limits, + mon.periodicity || undefined, + mon.regions && mon.regions.length > 0 ? mon.regions : undefined, + ); // Build update values - only include fields that are provided const updateValues: Record = @@ -429,14 +426,11 @@ export const monitorServiceImpl: ServiceImpl = { validateCommonMonitorFields(mon); // Check workspace limits if periodicity or regions are changing - if (mon.periodicity || (mon.regions && mon.regions.length > 0)) { - await checkMonitorLimits( - workspaceId, - limits, - mon.periodicity || undefined, - mon.regions && mon.regions.length > 0 ? mon.regions : undefined, - ); - } + checkMonitorConfigLimits( + limits, + mon.periodicity || undefined, + mon.regions && mon.regions.length > 0 ? mon.regions : undefined, + ); // Build update values - only include fields that are provided const updateValues: Record = @@ -482,14 +476,11 @@ export const monitorServiceImpl: ServiceImpl = { validateCommonMonitorFields(mon); // Check workspace limits if periodicity or regions are changing - if (mon.periodicity || (mon.regions && mon.regions.length > 0)) { - await checkMonitorLimits( - workspaceId, - limits, - mon.periodicity || undefined, - mon.regions && mon.regions.length > 0 ? mon.regions : undefined, - ); - } + checkMonitorConfigLimits( + limits, + mon.periodicity || undefined, + mon.regions && mon.regions.length > 0 ? mon.regions : undefined, + ); // Build update values - only include fields that are provided const updateValues: Record = diff --git a/apps/server/src/routes/rpc/handlers/monitor/limits.ts b/apps/server/src/routes/rpc/handlers/monitor/limits.ts index 3a90bcf9..3ab91d52 100644 --- a/apps/server/src/routes/rpc/handlers/monitor/limits.ts +++ b/apps/server/src/routes/rpc/handlers/monitor/limits.ts @@ -9,27 +9,15 @@ import { z } from "zod"; import { periodicityToString, regionsToStrings } from "./converters"; /** - * Check workspace limits for creating a new monitor. + * Check the plan limits that apply to a monitor's configuration. Safe on both + * create and update — it never looks at how many monitors already exist. * Throws ConnectError with PermissionDenied if any limit is exceeded. */ -export async function checkMonitorLimits( - workspaceId: number, +export function checkMonitorConfigLimits( limits: Limits, periodicity: Periodicity | undefined, regions: Region[] | undefined, -): Promise { - // Check monitor count limit - const countResult = await db - .select({ count: sql`count(*)` }) - .from(monitor) - .where(and(eq(monitor.workspaceId, workspaceId), isNull(monitor.deletedAt))) - .get(); - - const count = countResult?.count ?? 0; - if (count >= limits.monitors) { - throw new ConnectError("Upgrade for more monitors", Code.PermissionDenied); - } - +): void { // Check periodicity limit if (periodicity) { const periodicityStr = periodicityToString(periodicity); @@ -63,3 +51,30 @@ export async function checkMonitorLimits( } } } + +/** + * Check workspace limits for creating a new monitor. + * Throws ConnectError with PermissionDenied if any limit is exceeded. + * + * Create-only: the row-count cap must not run on update, or a workspace + * sitting at its limit could no longer edit the monitors it already has. + */ +export async function checkMonitorLimits( + workspaceId: number, + limits: Limits, + periodicity: Periodicity | undefined, + regions: Region[] | undefined, +): Promise { + const countResult = await db + .select({ count: sql`count(*)` }) + .from(monitor) + .where(and(eq(monitor.workspaceId, workspaceId), isNull(monitor.deletedAt))) + .get(); + + const count = countResult?.count ?? 0; + if (count >= limits.monitors) { + throw new ConnectError("Upgrade for more monitors", Code.PermissionDenied); + } + + checkMonitorConfigLimits(limits, periodicity, regions); +} diff --git a/apps/server/src/routes/rpc/handlers/monitor/validators.ts b/apps/server/src/routes/rpc/handlers/monitor/validators.ts index d01b008e..8a6dfe86 100644 --- a/apps/server/src/routes/rpc/handlers/monitor/validators.ts +++ b/apps/server/src/routes/rpc/handlers/monitor/validators.ts @@ -131,15 +131,21 @@ export function getCommonDbValuesForUpdate(mon: { result.degradedAfter = Number(mon.degradedAt); } - if (mon.active !== undefined) { + // `active`, `public` and `description` have implicit presence in the proto, + // so a decoded message always carries false/false/"" when the client omits + // them — `!== undefined` can't tell "omitted" from "sent as the zero value". + // Treat the zero value as omitted, like the fields above, so renaming a + // monitor doesn't also disable it. Cost: these three can't be reset to their + // zero value here until the proto gains explicit presence or an update mask. + if (mon.active !== undefined && mon.active !== false) { result.active = mon.active; } - if (mon.description !== undefined) { + if (mon.description !== undefined && mon.description !== "") { result.description = mon.description; } - if (mon.public !== undefined) { + if (mon.public !== undefined && mon.public !== false) { result.public = mon.public; } 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 2e07ced7..c68838a9 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 @@ -778,14 +778,15 @@ describe("StatusPageService.UpdateStatusPage", () => { .where(eq(page.id, testPageToUpdateId)); }); - test("clears locales when field is omitted", async () => { - // Set some locales + // `locales` is `repeated`, so an omitted field is indistinguishable from an + // empty one — both have to mean "keep", or every partial update would drop + // the page's languages. + test("keeps locales when field is omitted", async () => { await db .update(page) .set({ defaultLocale: "en", locales: ["en", "fr"] }) .where(eq(page.id, testPageToUpdateId)); - // Omitting locales clears them (same as sending []) const res = await connectRequest( "UpdateStatusPage", { @@ -798,7 +799,8 @@ describe("StatusPageService.UpdateStatusPage", () => { expect(res.status).toBe(200); const data = await res.json(); - expect(data.statusPage.locales ?? []).toEqual([]); + expect(data.statusPage.locales).toEqual(["LOCALE_EN", "LOCALE_FR"]); + expect(data.statusPage.defaultLocale).toBe("LOCALE_EN"); // Restore defaults await db @@ -811,14 +813,12 @@ describe("StatusPageService.UpdateStatusPage", () => { .where(eq(page.id, testPageToUpdateId)); }); - test("resets locales to null when empty list is sent", async () => { - // First set some locales + test("keeps locales when an empty list is sent", async () => { await db .update(page) .set({ defaultLocale: "en", locales: ["en", "fr"] }) .where(eq(page.id, testPageToUpdateId)); - // Send empty locales to clear them const res = await connectRequest( "UpdateStatusPage", { @@ -831,7 +831,65 @@ describe("StatusPageService.UpdateStatusPage", () => { expect(res.status).toBe(200); const data = await res.json(); - expect(data.statusPage.locales ?? []).toEqual([]); + expect(data.statusPage.locales).toEqual(["LOCALE_EN", "LOCALE_FR"]); + + // Restore defaults + await db + .update(page) + .set({ defaultLocale: "en", locales: null }) + .where(eq(page.id, testPageToUpdateId)); + }); + + test("keeps the default locale when LOCALE_UNSPECIFIED is sent", async () => { + await db + .update(page) + .set({ defaultLocale: "fr", locales: ["en", "fr"] }) + .where(eq(page.id, testPageToUpdateId)); + + const res = await connectRequest( + "UpdateStatusPage", + { + id: String(testPageToUpdateId), + defaultLocale: "LOCALE_UNSPECIFIED", + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(200); + + const data = await res.json(); + // Must not silently fall back to "en" + expect(data.statusPage.defaultLocale).toBe("LOCALE_FR"); + expect(data.statusPage.locales).toEqual(["LOCALE_EN", "LOCALE_FR"]); + + // Restore defaults + await db + .update(page) + .set({ defaultLocale: "en", locales: null }) + .where(eq(page.id, testPageToUpdateId)); + }); + + test("still replaces locales when a non-empty list is sent", async () => { + await db + .update(page) + .set({ defaultLocale: "en", locales: ["en", "fr"] }) + .where(eq(page.id, testPageToUpdateId)); + + const res = await connectRequest( + "UpdateStatusPage", + { + id: String(testPageToUpdateId), + defaultLocale: "LOCALE_DE", + locales: ["LOCALE_DE", "LOCALE_EN"], + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(200); + + const data = await res.json(); + expect(data.statusPage.defaultLocale).toBe("LOCALE_DE"); + expect(data.statusPage.locales).toEqual(["LOCALE_DE", "LOCALE_EN"]); // Restore defaults await db @@ -1869,6 +1927,121 @@ describe("StatusPageService.UpdateComponent", () => { }); }); +// A group belongs to exactly one page. Workspace scope alone would let a +// component be filed under a group from a sibling page. +describe("StatusPageService — component group must be on the same page", () => { + let otherPageId: number; + let otherGroupId: number; + + beforeAll(async () => { + await db + .delete(pageComponentGroup) + .where(eq(pageComponentGroup.name, `${TEST_PREFIX}-other-group`)); + await db.delete(page).where(eq(page.slug, `${TEST_PREFIX}-other-page-slug`)); + + const otherPage = await db + .insert(page) + .values({ + workspaceId: 1, + title: `${TEST_PREFIX}-other-page`, + slug: `${TEST_PREFIX}-other-page-slug`, + description: "Second page, owns a group of its own", + customDomain: "", + }) + .returning() + .get(); + otherPageId = otherPage.id; + + const otherGroup = await db + .insert(pageComponentGroup) + .values({ + workspaceId: 1, + pageId: otherPageId, + name: `${TEST_PREFIX}-other-group`, + }) + .returning() + .get(); + otherGroupId = otherGroup.id; + }); + + afterAll(async () => { + await db + .delete(pageComponentGroup) + .where(eq(pageComponentGroup.id, otherGroupId)); + await db.delete(page).where(eq(page.id, otherPageId)); + }); + + test("AddMonitorComponent rejects a group from another page", async () => { + const res = await connectRequest( + "AddMonitorComponent", + { + pageId: String(testPageId), + monitorId: String(testMonitorId), + name: `${TEST_PREFIX}-cross-page-monitor`, + groupId: String(otherGroupId), + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(404); + }); + + test("AddStaticComponent rejects a group from another page", async () => { + const res = await connectRequest( + "AddStaticComponent", + { + pageId: String(testPageId), + name: `${TEST_PREFIX}-cross-page-static`, + groupId: String(otherGroupId), + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(404); + }); + + test("UpdateComponent rejects a group from another page", async () => { + const res = await connectRequest( + "UpdateComponent", + { + id: String(testComponentToUpdateId), + groupId: String(otherGroupId), + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(404); + + const stored = await db + .select() + .from(pageComponent) + .where(eq(pageComponent.id, testComponentToUpdateId)) + .get(); + expect(stored?.groupId ?? null).toBe(null); + }); + + test("a group on the same page is still accepted", async () => { + const res = await connectRequest( + "AddStaticComponent", + { + pageId: String(testPageId), + name: `${TEST_PREFIX}-same-page-static`, + groupId: String(testGroupId), + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(200); + + const data = await res.json(); + expect(data.component.groupId).toBe(String(testGroupId)); + + await db + .delete(pageComponent) + .where(eq(pageComponent.id, Number(data.component.id))); + }); +}); + // ========================================================================== // Component Groups // ========================================================================== 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 f83ae4e3..198e1eaa 100644 --- a/apps/server/src/routes/rpc/handlers/status-page/index.ts +++ b/apps/server/src/routes/rpc/handlers/status-page/index.ts @@ -300,6 +300,24 @@ async function getGroupById(id: number, workspaceId: number) { .get(); } +/** + * Resolve a component group and assert it belongs to `pageId`. Workspace scope + * alone isn't enough — a group from a sibling page would otherwise be accepted + * and the component would render under a group it isn't on. Reported as + * not-found so the check doesn't confirm the group exists on another page. + */ +async function getGroupForPage( + groupId: string, + workspaceId: number, + pageId: number, +) { + const group = await getGroupById(Number(groupId), workspaceId); + if (!group || group.pageId !== pageId) { + throw componentGroupNotFoundError(groupId); + } + return group; +} + /** * Helper to get a monitor by ID with workspace scope. */ @@ -786,30 +804,35 @@ export const statusPageServiceImpl: ServiceImpl = { } if (req.icon !== undefined && req.icon) validateIconUrl(req.icon); - // Locale merge + cross-field validation. + // Locale merge + cross-field validation. `default_locale` is an enum and + // `locales` is `repeated`, so an omitted field decodes to UNSPECIFIED / + // `[]` — treat both as "not provided" and keep what is stored, or + // updating a title would silently reset the page's languages. Same + // presence test the theme / access-type fields below use. Trade-off: + // locales can't be cleared over this RPC until the proto carries + // explicit presence. + const reqDefaultLocale = + req.defaultLocale !== undefined && req.defaultLocale !== 0 + ? req.defaultLocale + : undefined; const nextDefaultLocale = - req.defaultLocale !== undefined - ? protoLocaleToDb(req.defaultLocale) + reqDefaultLocale !== undefined + ? protoLocaleToDb(reqDefaultLocale) : existing.defaultLocale; const validLocales = req.locales.filter((l) => l !== 0); const nextLocales = validLocales.length > 0 ? [...new Set(validLocales.map(protoLocaleToDb))] - : null; + : existing.locales; if (nextLocales && !nextLocales.includes(nextDefaultLocale)) { throw new ConnectError( "Default locale must be included in the locales list", Code.InvalidArgument, ); } - // `UpdateStatusPage` syncs locales on every call when the - // workspace has i18n — proto can't distinguish "field omitted" - // from "field = []", so the wire contract is "empty locales - // means clear". Gating on `req.locales.length > 0` meant omit - // and empty both became no-ops, leaving stale locales on the - // page. Skip the call only on plans without i18n, where the - // service would throw `LimitExceededError` regardless. - const localesChanged = limits.i18n === true; + const localesChanged = + limits.i18n === true && + (reqDefaultLocale !== undefined || validLocales.length > 0); const generalChanged = (req.title !== undefined && req.title !== "") || @@ -1024,10 +1047,7 @@ export const statusPageServiceImpl: ServiceImpl = { // Validate group exists if provided if (req.groupId) { - const group = await getGroupById(Number(req.groupId), workspaceId); - if (!group) { - throw componentGroupNotFoundError(req.groupId); - } + await getGroupForPage(req.groupId, workspaceId, pageData.id); } // Create the component @@ -1071,10 +1091,7 @@ export const statusPageServiceImpl: ServiceImpl = { // Validate group exists if provided if (req.groupId) { - const group = await getGroupById(Number(req.groupId), workspaceId); - if (!group) { - throw componentGroupNotFoundError(req.groupId); - } + await getGroupForPage(req.groupId, workspaceId, pageData.id); } // Create the component @@ -1139,10 +1156,7 @@ export const statusPageServiceImpl: ServiceImpl = { // Validate group exists if provided if (req.groupId !== undefined && req.groupId !== "") { - const group = await getGroupById(Number(req.groupId), workspaceId); - if (!group) { - throw componentGroupNotFoundError(req.groupId); - } + await getGroupForPage(req.groupId, workspaceId, component.pageId); } // Build update values diff --git a/apps/server/src/routes/rpc/handlers/status-report/__tests__/status-report.test.ts b/apps/server/src/routes/rpc/handlers/status-report/__tests__/status-report.test.ts index 03f4c58a..8e7477f4 100644 --- a/apps/server/src/routes/rpc/handlers/status-report/__tests__/status-report.test.ts +++ b/apps/server/src/routes/rpc/handlers/status-report/__tests__/status-report.test.ts @@ -366,6 +366,55 @@ describe("StatusReportService.CreateStatusReport", () => { expect(res.status).toBe(404); }); + // `Number("")` is 0 and `Number.parseInt("1.5")` is 1, so a malformed id used + // to be coerced into a real component id rather than rejected — an empty + // string silently targeted component 0. + test("rejects malformed page component ids", async () => { + for (const bad of ["", " ", "1.5", "1e3", "-1", "abc", "1abc"]) { + const res = await connectRequest( + "CreateStatusReport", + { + title: `${TEST_PREFIX}-bad-component-id`, + status: "STATUS_REPORT_STATUS_INVESTIGATING", + message: "Test message", + date: new Date().toISOString(), + pageId: "1", + pageComponentIds: [bad], + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.message).toContain("Invalid page component id"); + } + }); + + test("rejects a malformed component impact id", async () => { + const res = await connectRequest( + "CreateStatusReport", + { + title: `${TEST_PREFIX}-bad-impact-id`, + status: "STATUS_REPORT_STATUS_INVESTIGATING", + message: "Test message", + date: new Date().toISOString(), + pageId: "1", + pageComponentIds: [String(testPageComponentId)], + componentImpacts: [ + { + pageComponentId: "", + impact: "PAGE_COMPONENT_IMPACT_MAJOR_OUTAGE", + }, + ], + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.message).toContain("Invalid page component id"); + }); + test("returns error when page components are from different pages", async () => { const res = await connectRequest( "CreateStatusReport", @@ -1104,6 +1153,22 @@ describe("StatusReportService.UpdateStatusReport", () => { expect(res.status).toBe(404); }); + test("rejects a malformed page component id on update", async () => { + const res = await connectRequest( + "UpdateStatusReport", + { + id: String(testStatusReportToUpdateId), + pageComponentIds: [""], + updatePageComponentIds: true, + }, + { "x-openstatus-key": "1" }, + ); + + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.message).toContain("Invalid page component id"); + }); + test("returns error when updating with components from different pages", async () => { const res = await connectRequest( "UpdateStatusReport", diff --git a/apps/server/src/routes/rpc/handlers/status-report/index.ts b/apps/server/src/routes/rpc/handlers/status-report/index.ts index 66b0553b..e50d07dd 100644 --- a/apps/server/src/routes/rpc/handlers/status-report/index.ts +++ b/apps/server/src/routes/rpc/handlers/status-report/index.ts @@ -32,16 +32,21 @@ function parseDate(dateString: string): Date { return date; } +// Match the digits explicitly: `Number("")` is 0 (finite!), so a blank id used +// to slip through and target component 0, and `Number.parseInt("1.5")` is 1, so +// swapping in parseInt alone would still truncate a malformed id silently. +const PAGE_COMPONENT_ID = /^\d+$/; + function parsePageComponentIds(ids: ReadonlyArray): number[] { return ids.map((id) => { - const n = Number(id); - if (!Number.isFinite(n)) { + const trimmed = id.trim(); + if (!PAGE_COMPONENT_ID.test(trimmed)) { throw new ConnectError( `Invalid page component id: "${id}"`, Code.InvalidArgument, ); } - return n; + return Number(trimmed); }); } -- 2.51.2