Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion apps/loopover-ui/public/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -17365,6 +17365,9 @@
}
}
},
"400": {
"description": "Malformed installation id"
},
"404": {
"description": "Installation health not found"
}
Expand Down Expand Up @@ -17407,6 +17410,9 @@
}
}
},
"400": {
"description": "Malformed installation id"
},
"404": {
"description": "Installation health not found"
}
Expand Down Expand Up @@ -17449,6 +17455,9 @@
}
}
},
"400": {
"description": "Malformed installation id"
},
"404": {
"description": "Installation not found"
}
Expand Down Expand Up @@ -24011,6 +24020,9 @@
"200": {
"description": "Installation health"
},
"400": {
"description": "Malformed installation id"
},
"401": {
"description": "Not signed in"
},
Expand Down Expand Up @@ -24052,6 +24064,9 @@
"200": {
"description": "Repair plan"
},
"400": {
"description": "Malformed installation id"
},
"401": {
"description": "Not signed in"
},
Expand Down Expand Up @@ -24093,6 +24108,9 @@
"200": {
"description": "Repair plan recomputed"
},
"400": {
"description": "Malformed installation id"
},
"401": {
"description": "Not signed in"
},
Expand Down Expand Up @@ -24136,7 +24154,7 @@
"description": "Settings applied"
},
"400": {
"description": "Malformed settings"
"description": "Malformed installation id or settings"
},
"401": {
"description": "Not signed in"
Expand Down
14 changes: 7 additions & 7 deletions src/api/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2698,23 +2698,23 @@ 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));
});

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));
});

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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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 })) {
Expand Down
8 changes: 4 additions & 4 deletions src/openapi/orb-and-control-route-specs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down
3 changes: 3 additions & 0 deletions src/openapi/spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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" },
},
});
Expand All @@ -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" },
},
});
Expand All @@ -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" },
},
});
Expand Down
34 changes: 34 additions & 0 deletions test/integration/app-installations-selfservice.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: "" });
Expand Down
Loading