Skip to content

fix(security): validate usage identifiers #227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ZaaK0913-coder wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(security): validate usage identifiers #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,13 @@ Backend endpoints are taken from the companion documentation page `src/app/docs/
| `/usage` | Usage totals & settlement workflow | `POST /api/v1/usage`, `GET /api/v1/usage/:agent/:serviceId`, `POST /api/v1/settle` |
| `/webhooks` | Webhooks management | _(calls webhooks endpoints in code)_ and displays each webhook registration time relatively with an absolute timestamp tooltip |

### Usage identifier validation

The `/usage` record and query forms trim the agent and service identifiers before
submission. Identifiers must be 1-128 characters and may only contain letters,
numbers, dots, underscores, hyphens, and colons. Query requests still URL-encode
the validated identifiers before placing them in the path.

### API keys page notes

The `/api-keys` page lists each key label, prefix, and created-at age with the absolute ISO timestamp available on hover. If the account has no keys, the page renders a clear "No API keys yet" empty state instead of an empty list while preserving the create, reveal-once, copy, and revoke confirmation flows.
Expand Down
189 changes: 153 additions & 36 deletions src/app/usage/page.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ describe("UsagePage", () => {
it("renders both Record and Query landmarks", () => {
render(<UsagePage />);
expect(
screen.getByRole("heading", { name: /Usage metering/i })
screen.getByRole("heading", { name: /Usage metering/i }),
).toBeInTheDocument();
expect(
screen.getByRole("heading", { name: /Record usage/i })
screen.getByRole("heading", { name: /Record usage/i }),
).toBeInTheDocument();
expect(
screen.getByRole("heading", { name: /Query usage/i })
screen.getByRole("heading", { name: /Query usage/i }),
).toBeInTheDocument();
});

Expand All @@ -34,10 +34,10 @@ describe("UsagePage", () => {

render(<UsagePage />);
fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[0], {
target: { value: "a" },
target: { value: " agent-1 " },
});
fireEvent.change(screen.getAllByLabelText(/^Service ID$/i)[0], {
target: { value: "s" },
target: { value: " svc.alpha:prod " },
});
fireEvent.change(screen.getByLabelText(/^Requests$/i), {
target: { value: "42" },
Expand All @@ -48,12 +48,67 @@ describe("UsagePage", () => {
expect(screen.getByRole("status")).toHaveTextContent(/New total: 42/);
});
expect(apiPostMock).toHaveBeenCalledWith("/api/v1/usage", {
agent: "a",
serviceId: "s",
agent: "agent-1",
serviceId: "svc.alpha:prod",
requests: 42,
});
});

it("blocks record submit for empty, whitespace, or malformed identifiers", async () => {
render(<UsagePage />);
const agentInput = screen.getAllByLabelText(/^Agent$/i)[0];
const serviceInput = screen.getAllByLabelText(/^Service ID$/i)[0];

fireEvent.change(agentInput, { target: { value: " " } });
fireEvent.change(serviceInput, { target: { value: "svc/one" } });
fireEvent.change(screen.getByLabelText(/^Requests$/i), {
target: { value: "1" },
});
fireEvent.submit(screen.getByLabelText(/^Requests$/i).closest("form")!);

await waitFor(() => {
expect(agentInput).toHaveAttribute("aria-invalid", "true");
expect(serviceInput).toHaveAttribute("aria-invalid", "true");
});
expect(screen.getByText("Agent is required.")).toBeInTheDocument();
expect(
screen.getByText(
"Service ID can only use letters, numbers, dots, underscores, hyphens, and colons.",
),
).toBeInTheDocument();
expect(agentInput.getAttribute("aria-describedby")).toBeTruthy();
expect(serviceInput.getAttribute("aria-describedby")).toBeTruthy();
expect(apiPostMock).not.toHaveBeenCalled();
});

it("clears record identifier field errors after editing", async () => {
render(<UsagePage />);
const agentInput = screen.getAllByLabelText(/^Agent$/i)[0];

fireEvent.change(agentInput, { target: { value: "agent one" } });
fireEvent.change(screen.getAllByLabelText(/^Service ID$/i)[0], {
target: { value: "svc-1" },
});
fireEvent.change(screen.getByLabelText(/^Requests$/i), {
target: { value: "1" },
});
fireEvent.submit(screen.getByLabelText(/^Requests$/i).closest("form")!);

expect(
await screen.findByText(
"Agent can only use letters, numbers, dots, underscores, hyphens, and colons.",
),
).toBeInTheDocument();

fireEvent.change(agentInput, { target: { value: "agent-one" } });
expect(
screen.queryByText(
"Agent can only use letters, numbers, dots, underscores, hyphens, and colons.",
),
).not.toBeInTheDocument();
expect(agentInput).toHaveAttribute("aria-invalid", "false");
});

it("surfaces a backend invalid_request as a role=alert", async () => {
apiPostMock.mockRejectedValueOnce({
error: "invalid_request",
Expand Down Expand Up @@ -94,7 +149,7 @@ describe("UsagePage", () => {

await waitFor(() => {
expect(screen.getByRole("alert")).toHaveTextContent(
/requests must be a positive integer/
/requests must be a positive integer/,
);
});
expect(apiPostMock).not.toHaveBeenCalled();
Expand All @@ -109,19 +164,44 @@ describe("UsagePage", () => {

render(<UsagePage />);
fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[1], {
target: { value: "a" },
});
fireEvent.change(screen.getByLabelText(/^Service ID$/i, { selector: 'input[name="queryServiceId"]' }), {
target: { value: "s" },
});
target: { value: " a " },
});
fireEvent.change(
screen.getByLabelText(/^Service ID$/i, {
selector: 'input[name="queryServiceId"]',
}),
{
target: { value: " s " },
},
);
fireEvent.click(screen.getByRole("button", { name: /Query/i }));

await waitFor(() => {
expect(screen.getByRole("status")).toHaveTextContent(/a \/ s: 12 request\(s\)\./i);
expect(screen.getByRole("status")).toHaveTextContent(
/a \/ s: 12 request\(s\)\./i,
);
});
expect(apiGetMock).toHaveBeenCalledWith("/api/v1/usage/a/s");
});

it("blocks query submit for too-long identifiers", async () => {
render(<UsagePage />);
const queryAgentInput = screen.getAllByLabelText(/^Agent$/i)[1];
const queryServiceInput = screen.getAllByLabelText(/^Service ID$/i)[1];

fireEvent.change(queryAgentInput, { target: { value: "agent-ok" } });
fireEvent.change(queryServiceInput, { target: { value: "s".repeat(129) } });
fireEvent.click(screen.getByRole("button", { name: /Query/i }));

await waitFor(() => {
expect(queryServiceInput).toHaveAttribute("aria-invalid", "true");
});
expect(
screen.getByText("Service ID must be 128 characters or fewer."),
).toBeInTheDocument();
expect(apiGetMock).not.toHaveBeenCalled();
});

it("shows a request id when the query request fails", async () => {
apiGetMock.mockRejectedValueOnce({
error: "invalid_request",
Expand All @@ -133,9 +213,14 @@ describe("UsagePage", () => {
fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[1], {
target: { value: "a" },
});
fireEvent.change(screen.getByLabelText(/^Service ID$/i, { selector: 'input[name="queryServiceId"]' }), {
target: { value: "s" },
});
fireEvent.change(
screen.getByLabelText(/^Service ID$/i, {
selector: 'input[name="queryServiceId"]',
}),
{
target: { value: "s" },
},
);
fireEvent.click(screen.getByRole("button", { name: /Query/i }));

await waitFor(() => {
Expand Down Expand Up @@ -195,9 +280,13 @@ describe("UsagePage", () => {
render(<UsagePage />);

// First query
fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[1], { target: { value: "a" } });
fireEvent.change(screen.getAllByLabelText(/^Service ID$/i)[1], { target: { value: "s" } });

fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[1], {
target: { value: "a" },
});
fireEvent.change(screen.getAllByLabelText(/^Service ID$/i)[1], {
target: { value: "s" },
});

const queryButton = screen.getByRole("button", { name: /Query/i });
fireEvent.click(queryButton);

Expand All @@ -213,12 +302,16 @@ describe("UsagePage", () => {
});

await waitFor(() => {
expect(screen.getByRole("status")).toHaveTextContent(/a \/ s: 10 request\(s\)/i);
expect(screen.getByRole("status")).toHaveTextContent(
/a \/ s: 10 request\(s\)/i,
);
});

// Start second query
fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[1], { target: { value: "b" } });

fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[1], {
target: { value: "b" },
});

// Create new promise for second query
let resolveQuery2: (value: unknown) => void;
apiGetMock.mockImplementationOnce(() => {
Expand All @@ -240,7 +333,9 @@ describe("UsagePage", () => {
});

await waitFor(() => {
expect(screen.getByRole("status")).toHaveTextContent(/b \/ s: 20 request\(s\)/i);
expect(screen.getByRole("status")).toHaveTextContent(
/b \/ s: 20 request\(s\)/i,
);
});
});

Expand All @@ -249,12 +344,18 @@ describe("UsagePage", () => {

apiGetMock.mockResolvedValueOnce({ agent: "a", serviceId: "s", total: 10 });

fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[1], { target: { value: "a" } });
fireEvent.change(screen.getAllByLabelText(/^Service ID$/i)[1], { target: { value: "s" } });
fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[1], {
target: { value: "a" },
});
fireEvent.change(screen.getAllByLabelText(/^Service ID$/i)[1], {
target: { value: "s" },
});
fireEvent.click(screen.getByRole("button", { name: /Query/i }));

await waitFor(() => {
expect(screen.getByRole("status")).toHaveTextContent(/a \/ s: 10 request\(s\)/i);
expect(screen.getByRole("status")).toHaveTextContent(
/a \/ s: 10 request\(s\)/i,
);
});

// Now error
Expand All @@ -270,15 +371,25 @@ describe("UsagePage", () => {
});

it("handles an empty/zero total", async () => {
apiGetMock.mockResolvedValueOnce({ agent: "zero", serviceId: "s", total: 0 });
apiGetMock.mockResolvedValueOnce({
agent: "zero",
serviceId: "s",
total: 0,
});

render(<UsagePage />);
fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[1], { target: { value: "zero" } });
fireEvent.change(screen.getAllByLabelText(/^Service ID$/i)[1], { target: { value: "s" } });
fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[1], {
target: { value: "zero" },
});
fireEvent.change(screen.getAllByLabelText(/^Service ID$/i)[1], {
target: { value: "s" },
});
fireEvent.click(screen.getByRole("button", { name: /Query/i }));

await waitFor(() => {
expect(screen.getByRole("status")).toHaveTextContent(/zero \/ s: 0 request\(s\)/i);
expect(screen.getByRole("status")).toHaveTextContent(
/zero \/ s: 0 request\(s\)/i,
);
});
});

Expand All @@ -291,12 +402,18 @@ describe("UsagePage", () => {
});

render(<UsagePage />);
fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[0], { target: { value: "a" } });
fireEvent.change(screen.getAllByLabelText(/^Service ID$/i)[0], { target: { value: "s" } });
fireEvent.change(screen.getByLabelText(/^Requests$/i), { target: { value: "5" } });

fireEvent.change(screen.getAllByLabelText(/^Agent$/i)[0], {
target: { value: "a" },
});
fireEvent.change(screen.getAllByLabelText(/^Service ID$/i)[0], {
target: { value: "s" },
});
fireEvent.change(screen.getByLabelText(/^Requests$/i), {
target: { value: "5" },
});

const recordButton = screen.getByRole("button", { name: /Record/i });

// Using submit to verify the onRecord handler prevents multiple calls
const form = screen.getByLabelText(/^Requests$/i).closest("form")!;
fireEvent.submit(form);
Expand Down
Loading