From 0e1c1066784b903a099f2114a77b9c6b06f96114 Mon Sep 17 00:00:00 2001 From: tryeverything24 <114252040+tryeverything24@users.noreply.github.com> Date: Sat, 25 Jul 2026 07:42:42 +0300 Subject: [PATCH] fix(miners): validate `since` query param so malformed input returns 400, not 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/v1/miners/:githubId/pulls?since=... and /issues?since=... pass the raw query string straight into a ::timestamptz cast. A non-timestamp value (e.g. since=banana, or since= on /issues, which binds the empty string) makes Postgres reject the cast, surfacing as a QueryFailedError 500 instead of a 400 — misclassifying a client mistake as a server failure. Consumers that treat 5xx as retryable (the usual convention) will then hammer a request that can never succeed. The POST variants of the same endpoints (parseSinceByRepo) and the dashboard controller already validate and normalize timestamps to ISO; this brings the two GET endpoints in line: absent/empty keeps the existing default behavior, anything else must parse as a date and is normalized with toISOString() before it reaches SQL. Adds controller unit tests covering rejection, normalization, the default window on /pulls, and the omitted/empty null mode on /issues. --- .../src/api/miners/miners.controller.spec.ts | 92 +++++++++++++++++++ .../das/src/api/miners/miners.controller.ts | 25 ++++- 2 files changed, 115 insertions(+), 2 deletions(-) create mode 100644 packages/das/src/api/miners/miners.controller.spec.ts diff --git a/packages/das/src/api/miners/miners.controller.spec.ts b/packages/das/src/api/miners/miners.controller.spec.ts new file mode 100644 index 0000000..fe19ed2 --- /dev/null +++ b/packages/das/src/api/miners/miners.controller.spec.ts @@ -0,0 +1,92 @@ +import { BadRequestException } from "@nestjs/common"; +import { MinersController } from "./miners.controller"; +import { MinersService } from "./miners.service"; + +// The GET endpoints hand `since` straight to SQL as a `::timestamptz` +// parameter. The controller must reject values Postgres would choke on — +// turning a client mistake into a 400 instead of a QueryFailedError 500 — +// and normalize accepted values to ISO, exactly like the POST endpoints' +// parseSinceByRepo and the dashboard controller already do. +function makeController(): { + controller: MinersController; + getPullRequests: jest.Mock; + getIssues: jest.Mock; +} { + const getPullRequests = jest.fn().mockResolvedValue({}); + const getIssues = jest.fn().mockResolvedValue({}); + const service = { getPullRequests, getIssues } as unknown as MinersService; + return { + controller: new MinersController(service), + getPullRequests, + getIssues, + }; +} + +describe("GET /miners/:githubId/pulls `since` validation", () => { + it("rejects a non-timestamp `since` with 400 before reaching the service", async () => { + const { controller, getPullRequests } = makeController(); + + await expect( + controller.getPullRequests("1", "banana"), + ).rejects.toBeInstanceOf(BadRequestException); + expect(getPullRequests).not.toHaveBeenCalled(); + }); + + it("normalizes a valid `since` to a full ISO timestamp", async () => { + const { controller, getPullRequests } = makeController(); + + await controller.getPullRequests("1", "2026-06-01"); + + expect(getPullRequests).toHaveBeenCalledTimes(1); + expect(getPullRequests.mock.calls[0][1]).toBe("2026-06-01T00:00:00.000Z"); + }); + + it("falls back to the default window when `since` is omitted", async () => { + const { controller, getPullRequests } = makeController(); + + await controller.getPullRequests("1"); + + expect(getPullRequests).toHaveBeenCalledTimes(1); + const since: unknown = getPullRequests.mock.calls[0][1]; + expect(typeof since).toBe("string"); + expect(Number.isNaN(Date.parse(since as string))).toBe(false); + }); +}); + +describe("GET /miners/:githubId/issues `since` validation", () => { + it("rejects a non-timestamp `since` with 400 before reaching the service", async () => { + const { controller, getIssues } = makeController(); + + await expect(controller.getIssues("1", "banana")).rejects.toBeInstanceOf( + BadRequestException, + ); + expect(getIssues).not.toHaveBeenCalled(); + }); + + it("passes null when `since` is omitted (open-issue load counting mode)", async () => { + const { controller, getIssues } = makeController(); + + await controller.getIssues("1"); + + expect(getIssues).toHaveBeenCalledTimes(1); + expect(getIssues.mock.calls[0][1]).toBeNull(); + }); + + it("treats an empty `since` as omitted instead of casting '' to timestamptz", async () => { + const { controller, getIssues } = makeController(); + + await controller.getIssues("1", ""); + + expect(getIssues).toHaveBeenCalledTimes(1); + expect(getIssues.mock.calls[0][1]).toBeNull(); + }); + + it("normalizes a valid `since` to a full ISO timestamp", async () => { + const { controller, getIssues } = makeController(); + + await controller.getIssues("1", "2026-06-01"); + + expect(getIssues).toHaveBeenCalledTimes(1); + expect(getIssues.mock.calls[0][1]).toBe("2026-06-01T00:00:00.000Z"); + }); +}); diff --git a/packages/das/src/api/miners/miners.controller.ts b/packages/das/src/api/miners/miners.controller.ts index 9f633a3..6a8e7df 100644 --- a/packages/das/src/api/miners/miners.controller.ts +++ b/packages/das/src/api/miners/miners.controller.ts @@ -25,6 +25,23 @@ interface SinceByRepoBody { since_by_repo?: Record; } +/** + * Validate an optional `since` query value. Absent or empty resolves to + * undefined so callers apply their own default; anything else must parse as + * a date and is normalized to ISO. The value is bound to a `::timestamptz` + * cast in SQL, so rejecting garbage here turns a client mistake into a 400 + * instead of a QueryFailedError 500 — matching the validation the POST + * endpoints (parseSinceByRepo) and the dashboard controller already apply. + */ +function parseSinceQuery(since?: string): string | undefined { + if (since === undefined || since === "") return undefined; + const parsed = new Date(since); + if (Number.isNaN(parsed.getTime())) { + throw new BadRequestException("`since` must be a valid ISO timestamp"); + } + return parsed.toISOString(); +} + /** * Validate a `{ since_by_repo: { "": "" } }` body * into parallel `repoNames` / `sinceValues` arrays. Repo names are lowercased @@ -143,7 +160,7 @@ export class MinersController { const pagination = parsePaginationQuery(limit, cursor); return this.miners.getPullRequests( githubId, - MinersService.resolveSince(since), + MinersService.resolveSince(parseSinceQuery(since)), pagination, ); } @@ -222,7 +239,11 @@ export class MinersController { @Query("limit") limit?: string, ): Promise { const pagination = parsePaginationQuery(limit, cursor); - return this.miners.getIssues(githubId, since ?? null, pagination); + return this.miners.getIssues( + githubId, + parseSinceQuery(since) ?? null, + pagination, + ); } @Post(":githubId/issues")