diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index a84890ed11..1dd002703d 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -17365,6 +17365,9 @@ } } }, + "400": { + "description": "Malformed installation id" + }, "404": { "description": "Installation health not found" } @@ -17407,6 +17410,9 @@ } } }, + "400": { + "description": "Malformed installation id" + }, "404": { "description": "Installation health not found" } @@ -17449,6 +17455,9 @@ } } }, + "400": { + "description": "Malformed installation id" + }, "404": { "description": "Installation not found" } @@ -24011,6 +24020,9 @@ "200": { "description": "Installation health" }, + "400": { + "description": "Malformed installation id" + }, "401": { "description": "Not signed in" }, @@ -24052,6 +24064,9 @@ "200": { "description": "Repair plan" }, + "400": { + "description": "Malformed installation id" + }, "401": { "description": "Not signed in" }, @@ -24093,6 +24108,9 @@ "200": { "description": "Repair plan recomputed" }, + "400": { + "description": "Malformed installation id" + }, "401": { "description": "Not signed in" }, @@ -24136,7 +24154,7 @@ "description": "Settings applied" }, "400": { - "description": "Malformed settings" + "description": "Malformed installation id or settings" }, "401": { "description": "Not signed in" diff --git a/src/api/routes.ts b/src/api/routes.ts index 9d2c71494a..f09b71fbfb 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -2698,7 +2698,7 @@ export function createApp() { app.get("/v1/installations/:id/health", async (c) => { const installationId = Number(c.req.param("id")); - if (!Number.isFinite(installationId)) return c.json({ error: "invalid_installation_id" }, 400); + if (!Number.isInteger(installationId) || installationId <= 0) return c.json({ error: "invalid_installation_id" }, 400); const health = await getInstallationHealth(c.env, installationId); if (!health) return c.json({ error: "installation_health_not_found" }, 404); return c.json(enrichInstallationHealth(health)); @@ -2706,7 +2706,7 @@ export function createApp() { app.get("/v1/installations/:id/repair", async (c) => { const installationId = Number(c.req.param("id")); - if (!Number.isFinite(installationId)) return c.json({ error: "invalid_installation_id" }, 400); + if (!Number.isInteger(installationId) || installationId <= 0) return c.json({ error: "invalid_installation_id" }, 400); const health = await getInstallationHealth(c.env, installationId); if (!health) return c.json({ error: "installation_health_not_found" }, 404); return c.json(await buildInstallationRepairDiagnostics(c.env, health)); @@ -2714,7 +2714,7 @@ export function createApp() { app.post("/v1/installations/:id/repair/refresh", async (c) => { const installationId = Number(c.req.param("id")); - if (!Number.isFinite(installationId)) return c.json({ error: "invalid_installation_id" }, 400); + if (!Number.isInteger(installationId) || installationId <= 0) return c.json({ error: "invalid_installation_id" }, 400); const refreshed = await refreshInstallationHealthForInstallation(c.env, installationId); if (!refreshed) return c.json({ error: "installation_not_found" }, 404); const health = await getInstallationHealth(c.env, installationId); @@ -2744,7 +2744,7 @@ export function createApp() { const resolved = await resolveAppInstallationScope(c); if (resolved instanceof Response) return resolved; const installationId = Number(c.req.param("id")); - if (!Number.isFinite(installationId)) return c.json({ error: "invalid_installation_id" }, 400); + if (!Number.isInteger(installationId) || installationId <= 0) return c.json({ error: "invalid_installation_id" }, 400); const health = await getInstallationHealth(c.env, installationId); if (!health) return c.json({ error: "installation_health_not_found" }, 404); if (!installationRecordInScope(resolved.scope, health)) return c.json({ error: "forbidden_installation" }, 403); @@ -2755,7 +2755,7 @@ export function createApp() { const resolved = await resolveAppInstallationScope(c); if (resolved instanceof Response) return resolved; const installationId = Number(c.req.param("id")); - if (!Number.isFinite(installationId)) return c.json({ error: "invalid_installation_id" }, 400); + if (!Number.isInteger(installationId) || installationId <= 0) return c.json({ error: "invalid_installation_id" }, 400); const health = await getInstallationHealth(c.env, installationId); if (!health) return c.json({ error: "installation_health_not_found" }, 404); if (!installationRecordInScope(resolved.scope, health)) return c.json({ error: "forbidden_installation" }, 403); @@ -2766,7 +2766,7 @@ export function createApp() { const resolved = await resolveAppInstallationScope(c); if (resolved instanceof Response) return resolved; const installationId = Number(c.req.param("id")); - if (!Number.isFinite(installationId)) return c.json({ error: "invalid_installation_id" }, 400); + if (!Number.isInteger(installationId) || installationId <= 0) return c.json({ error: "invalid_installation_id" }, 400); // Ownership is enforced BEFORE the refresh side effect so a tenant can never trigger repair on an // installation they don't own; the existing health record supplies the account the scope is checked against. const existing = await getInstallationHealth(c.env, installationId); @@ -2788,7 +2788,7 @@ export function createApp() { const resolved = await resolveAppInstallationScope(c); if (resolved instanceof Response) return resolved; const installationId = Number(c.req.param("id")); - if (!Number.isFinite(installationId)) return c.json({ error: "invalid_installation_id" }, 400); + if (!Number.isInteger(installationId) || installationId <= 0) return c.json({ error: "invalid_installation_id" }, 400); const installation = await getInstallation(c.env, installationId); if (!installation) return c.json({ error: "installation_not_found" }, 404); if (!installationRecordInScope(resolved.scope, { installationId: installation.id, accountLogin: installation.accountLogin })) { diff --git a/src/openapi/orb-and-control-route-specs.ts b/src/openapi/orb-and-control-route-specs.ts index f0a8bcd7e9..2dac5570c7 100644 --- a/src/openapi/orb-and-control-route-specs.ts +++ b/src/openapi/orb-and-control-route-specs.ts @@ -174,7 +174,7 @@ const APP_ROUTES: SpecEntry[] = [ tags: ["Control panel"], summary: "Return one installation's health summary", auth: "session", - responses: { 200: { description: "Installation health" }, 404: { description: "No such installation" }, ...SESSION_AUTH_RESPONSES }, + responses: { 200: { description: "Installation health" }, 400: { description: "Malformed installation id" }, 404: { description: "No such installation" }, ...SESSION_AUTH_RESPONSES }, }, { method: "get", @@ -183,7 +183,7 @@ const APP_ROUTES: SpecEntry[] = [ tags: ["Control panel"], summary: "Return the repair plan for an unhealthy installation", auth: "session", - responses: { 200: { description: "Repair plan" }, 404: { description: "No such installation" }, ...SESSION_AUTH_RESPONSES }, + responses: { 200: { description: "Repair plan" }, 400: { description: "Malformed installation id" }, 404: { description: "No such installation" }, ...SESSION_AUTH_RESPONSES }, }, { method: "post", @@ -192,7 +192,7 @@ const APP_ROUTES: SpecEntry[] = [ tags: ["Control panel"], summary: "Recompute an installation's repair plan", auth: "session", - responses: { 200: { description: "Repair plan recomputed" }, 404: { description: "No such installation" }, ...SESSION_AUTH_RESPONSES }, + responses: { 200: { description: "Repair plan recomputed" }, 400: { description: "Malformed installation id" }, 404: { description: "No such installation" }, ...SESSION_AUTH_RESPONSES }, }, { method: "put", @@ -201,7 +201,7 @@ const APP_ROUTES: SpecEntry[] = [ tags: ["Control panel", "Agent automation"], summary: "Apply agent settings across every repo in an installation", auth: "session", - responses: { 200: { description: "Settings applied" }, 400: { description: "Malformed settings" }, 404: { description: "No such installation" }, ...SESSION_AUTH_RESPONSES }, + responses: { 200: { description: "Settings applied" }, 400: { description: "Malformed installation id or settings" }, 404: { description: "No such installation" }, ...SESSION_AUTH_RESPONSES }, }, { method: "get", diff --git a/src/openapi/spec.ts b/src/openapi/spec.ts index 83f03ed765..3c12240af0 100644 --- a/src/openapi/spec.ts +++ b/src/openapi/spec.ts @@ -540,6 +540,7 @@ export function buildOpenApiSpec() { request: { params: z.object({ id: z.string() }) }, responses: { 200: { description: "GitHub App installation health", content: { "application/json": { schema: InstallationHealthSchema } } }, + 400: { description: "Malformed installation id" }, 404: { description: "Installation health not found" }, }, }); @@ -552,6 +553,7 @@ export function buildOpenApiSpec() { request: { params: z.object({ id: z.string() }) }, responses: { 200: { description: "GitHub App installation repair diagnostics", content: { "application/json": { schema: InstallationRepairSchema } } }, + 400: { description: "Malformed installation id" }, 404: { description: "Installation health not found" }, }, }); @@ -564,6 +566,7 @@ export function buildOpenApiSpec() { request: { params: z.object({ id: z.string() }) }, responses: { 200: { description: "Refreshed GitHub App installation repair diagnostics", content: { "application/json": { schema: InstallationRepairSchema } } }, + 400: { description: "Malformed installation id" }, 404: { description: "Installation not found" }, }, }); diff --git a/test/integration/app-installations-selfservice.test.ts b/test/integration/app-installations-selfservice.test.ts index 62e110c16c..8eddab15bb 100644 --- a/test/integration/app-installations-selfservice.test.ts +++ b/test/integration/app-installations-selfservice.test.ts @@ -170,6 +170,40 @@ describe("tenant self-service installation health/repair (#7661)", () => { await expect(missing.json()).resolves.toMatchObject({ error: "installation_health_not_found" }); }); + it("#9716: rejects a fractional/exponent/zero/negative installation id with 400 on every id route (Number.isFinite let those through)", async () => { + const app = createApp(); + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "" }); + await seedFleet(env); + + // A GitHub installation id is always a positive integer. Number.isFinite accepted "1.5"->1.5 (fractional) and + // "0"/"-1" (non-positive), binding them straight into the D1 lookup; each must now be a 400. ("1e3"->1000 is a + // genuine integer and is deliberately NOT rejected -- it is exponent notation for a valid id, asserted below.) + const badIds = ["1.5", "0", "-1"]; + const routes: Array<{ path: (id: string) => string; method: "GET" | "POST" | "PUT"; body?: string }> = [ + { path: (id) => `/v1/installations/${id}/health`, method: "GET" }, + { path: (id) => `/v1/installations/${id}/repair`, method: "GET" }, + { path: (id) => `/v1/installations/${id}/repair/refresh`, method: "POST" }, + { path: (id) => `/v1/app/installations/${id}/health`, method: "GET" }, + { path: (id) => `/v1/app/installations/${id}/repair`, method: "GET" }, + { path: (id) => `/v1/app/installations/${id}/repair/refresh`, method: "POST" }, + { path: (id) => `/v1/app/installations/${id}/agent/bulk-settings`, method: "PUT", body: JSON.stringify({ autonomy: {} }) }, + ]; + for (const route of routes) { + for (const badId of badIds) { + const res = await app.request( + route.path(badId), + { method: route.method, headers: apiHeaders(env), ...(route.body !== undefined ? { body: route.body } : {}) }, + env, + ); + expect(res.status, `${route.method} ${route.path(badId)}`).toBe(400); + await expect(res.json()).resolves.toMatchObject({ error: "invalid_installation_id" }); + } + } + + // A valid positive integer id still passes the guard and reaches the handler (200 for the owned installation). + expect((await app.request("/v1/app/installations/600/health", { headers: apiHeaders(env) }, env)).status).toBe(200); + }); + it("scopes per-installation repair diagnostics and their error branches", async () => { const app = createApp(); const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "" });