From b774b054563f956cf9abd6ff91737850d176e7b4 Mon Sep 17 00:00:00 2001 From: m5r Date: Mon, 31 Aug 2026 17:40:07 +0200 Subject: [PATCH] activity: select-only multi-client filter the freetype client field is gone. the picker is a select-only menu of known clients with multi-select, chips for the active set, and a 32-client cap shared with the server. the api accepts a comma-separated client list and filters any-of with bound parameters. the trigger carets come from phosphor icons, newly adopted. bundle budget rises to 900000 bytes for the picker and the icon dependency. --- admin/package-lock.json | 14 + admin/package.json | 1 + admin/scripts/assert-bundle-size.mjs | 2 +- .../activity/ActivityFilters.test.tsx | 445 ++++++++++++++++-- .../src/features/activity/ActivityFilters.tsx | 111 ++--- admin/src/features/activity/ClientFilter.tsx | 365 ++++++++++++++ admin/src/features/activity/search.test.ts | 30 +- admin/src/features/activity/search.ts | 33 +- admin/src/ui/Select.tsx | 4 +- src/storage/repositories/queries_repo.zig | 122 ++++- src/web/handlers/queries.zig | 90 +++- src/web/openapi.yaml | 6 +- 12 files changed, 1093 insertions(+), 130 deletions(-) create mode 100644 admin/src/features/activity/ClientFilter.tsx diff --git a/admin/package-lock.json b/admin/package-lock.json index e9b976f..75029fb 100644 --- a/admin/package-lock.json +++ b/admin/package-lock.json @@ -8,6 +8,7 @@ "name": "nxdns-admin", "version": "0.0.0", "dependencies": { + "@phosphor-icons/react": "^2.1.10", "@stylexjs/stylex": "0.19.0", "@tanstack/react-query": "5.101.4", "@tanstack/react-router": "1.170.18", @@ -1078,6 +1079,19 @@ "node": "^20.19.0 || >=22.12.0" } }, + "node_modules/@phosphor-icons/react": { + "version": "2.1.10", + "resolved": "https://registry.npmjs.org/@phosphor-icons/react/-/react-2.1.10.tgz", + "integrity": "sha512-vt8Tvq8GLjheAZZYa+YG/pW7HDbov8El/MANW8pOAz4eGxrwhnbfrQZq0Cp4q8zBEu8NIhHdnr+r8thnfRSNYA==", + "license": "MIT", + "engines": { + "node": ">=10" + }, + "peerDependencies": { + "react": ">= 16.8", + "react-dom": ">= 16.8" + } + }, "node_modules/@react-types/shared": { "version": "3.36.1", "resolved": "https://registry.npmjs.org/@react-types/shared/-/shared-3.36.1.tgz", diff --git a/admin/package.json b/admin/package.json index 6a4882e..6757e25 100644 --- a/admin/package.json +++ b/admin/package.json @@ -25,6 +25,7 @@ "trailingComma": "all" }, "dependencies": { + "@phosphor-icons/react": "^2.1.10", "@stylexjs/stylex": "0.19.0", "@tanstack/react-query": "5.101.4", "@tanstack/react-router": "1.170.18", diff --git a/admin/scripts/assert-bundle-size.mjs b/admin/scripts/assert-bundle-size.mjs index 0f675d2..62dcb6f 100644 --- a/admin/scripts/assert-bundle-size.mjs +++ b/admin/scripts/assert-bundle-size.mjs @@ -14,7 +14,7 @@ import { readdirSync, statSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; -const BUDGET_BYTES = 850_000; +const BUDGET_BYTES = 900_000; const distDir = join(dirname(dirname(fileURLToPath(import.meta.url))), "dist", "assets"); diff --git a/admin/src/features/activity/ActivityFilters.test.tsx b/admin/src/features/activity/ActivityFilters.test.tsx index eb60e4d..e7c74d6 100644 --- a/admin/src/features/activity/ActivityFilters.test.tsx +++ b/admin/src/features/activity/ActivityFilters.test.tsx @@ -10,14 +10,63 @@ */ import { useCallback, useState } from "react"; -import { afterAll, expect, test, vi } from "vitest"; -import { act, fireEvent, render, screen } from "@testing-library/react"; +import { afterAll, beforeEach, expect, test, vi } from "vitest"; +import { act, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; +import { QueryClientProvider } from "@tanstack/react-query"; +import { createQueryClient } from "@/lib/queryClient"; +import { queryKeys } from "@/lib/queries"; +import type { Client } from "@/lib/types"; import ActivityFilters, { NO_FILTERS, type AppliedFilters } from "./ActivityFilters"; import { unixToDatetimeLocal } from "./datetime"; vi.stubEnv("TZ", "Europe/Paris"); afterAll(() => vi.unstubAllEnvs()); +function client(ip: string, name: string, learnedName: string): Client { + return { + id: 1, + ip, + name, + learned_name: learnedName, + group_id: 1, + group: "default", + hand_edited: name !== "", + first_seen: 1_700_000_000, + last_seen: 1_700_000_100, + }; +} + +/** Two named clients and one bare address: every case the picker has to show. */ +const CLIENTS: Client[] = [ + client("192.0.2.10", "Kitchen Pi", "pi.lan"), + client("192.0.2.11", "", "laptop.lan"), + client("192.0.2.12", "", ""), +]; + +/** The picker reads the clients query the query tables already load. */ +beforeEach(() => { + vi.stubGlobal( + "fetch", + vi.fn((input: RequestInfo | URL) => { + const url = String(input); + if (url === "/api/clients") { + return Promise.resolve( + new Response(JSON.stringify({ clients: CLIENTS }), { + status: 200, + headers: { "content-type": "application/json" }, + }), + ); + } + return Promise.resolve(new Response(JSON.stringify({ error: "not stubbed" }), { status: 404 })); + }), + ); +}); + +/** A household bigger than the chip row and, at 40, bigger than the 32 cap. */ +function manyClients(count: number): Client[] { + return Array.from({ length: count }, (_, index) => client(`198.51.100.${index + 1}`, `Device ${index + 1}`, "")); +} + /** 02:30 does not exist on this date in Paris; the clock jumps 02:00 to 03:00. */ const GAP_WALL_TIME = "2026-03-29T02:30:00"; @@ -42,7 +91,12 @@ function describedText(input: HTMLElement): string { * land them in an order the network and the router can genuinely produce — * an early debounce arriving after a later one, over a draft that has moved on. */ -function renderFilters(initial: AppliedFilters = NO_FILTERS, deferred = false) { +function renderFilters( + initial: AppliedFilters = NO_FILTERS, + deferred = false, + primeClients = true, + clients: Client[] = CLIENTS, +) { const patches: Array> = []; const state: { current: AppliedFilters } = { current: initial }; const setter: { current: ((next: AppliedFilters) => void) | null } = { current: null }; @@ -62,12 +116,75 @@ function renderFilters(initial: AppliedFilters = NO_FILTERS, deferred = false) { return ; } - render(); + const queryClient = createQueryClient(); + // Seeded rather than fetched, so the picker has its options on the first + // render and a test of the toolbar is not also a test of a request landing. + // The one test that is about that arrival opts out and waits for the stub. + if (primeClients) queryClient.setQueryData(queryKeys.clients, clients); + render( + + + , + ); /** The URL moving under the form: a commit landing, or the back button. */ function land(next: AppliedFilters) { act(() => setter.current!(next)); } - return { patches, state, land }; + return { patches, state, land, queryClient }; +} + +/** + * A press, as react-aria hears one. `usePress` works in pointer events, or in + * mouse events where jsdom has no `PointerEvent`; a bare `click` is neither, and + * RAC's own controls do not respond to it. + */ +function press(element: HTMLElement) { + fireEvent.mouseDown(element); + fireEvent.mouseUp(element); + fireEvent.click(element); +} + +/** + * The picker's trigger: the first button of the client group, whatever it reads + * as. Its label is the selection, so it cannot be looked up by a fixed name. + */ +function clientTrigger(): HTMLElement { + return within(screen.getByRole("group", { name: "Clients" })).getAllByRole("button")[0] as HTMLElement; +} + +/** The removable chips, in the order they are shown, by what each one reads as. */ +function clientChips(): string[] { + return within(screen.getByRole("group", { name: "Clients" })) + .getAllByRole("button") + .slice(1) + .map((chip) => chip.textContent?.replace("\u00d7", "") ?? ""); +} + +/** + * Opens the client menu, if it is not open already. + * + * The open state is read from the trigger rather than assumed: an open RAC + * popover hides the rest of the page from the accessibility tree — the trigger + * included — so a blind second click would either fail to find it or shut the + * menu it was meant to open. + */ +function openClientMenu() { + if (clientTrigger().getAttribute("aria-expanded") === "true") return; + fireEvent.click(clientTrigger()); +} + +/** + * Picks one client and shuts the menu again. + * + * A multiple-selection menu stays open on a pick, which is the point of it — but + * an open RAC popover hides the rest of the page from the accessibility tree, so + * a test that wants to read the chips has to close it first, exactly as a reader + * would before looking at them. + */ +function pickClient(name: string) { + openClientMenu(); + press(screen.getByRole("menuitemcheckbox", { name })); + fireEvent.keyDown(screen.getByRole("menu"), { key: "Escape", code: "Escape" }); } function openTimeMenu() { @@ -284,10 +401,7 @@ test("a debounced commit landing late does not roll the field back over newer ty act(() => vi.advanceTimersByTime(400)); fireEvent.change(domain, { target: { value: "ads" } }); act(() => vi.advanceTimersByTime(400)); - expect(patches).toEqual([ - { domain: "ad", client: undefined }, - { domain: "ads", client: undefined }, - ]); + expect(patches).toEqual([{ domain: "ad" }, { domain: "ads" }]); fireEvent.change(domain, { target: { value: "adsx" } }); // The older one lands first. It is this toolbar's own echo, two keystrokes @@ -352,40 +466,293 @@ test("a window moved from outside clears an error left over from the old one", ( expect(screen.queryByRole("alert")).toBeNull(); }); -test("the client field names the exact match it performs", () => { +test("the picker offers every client and names the filter it is not yet applying", () => { renderFilters(); - const client = screen.getByLabelText("Client IP (exact match)") as HTMLInputElement; - expect(client.getAttribute("placeholder")).toBe("Client IP…"); -}); + expect(clientTrigger().textContent).toContain("Clients"); + expect(clientChips()).toEqual([]); -test("an earlier range commit landing late does not unseat the pick that is current", () => { - const first = 1_800_000_000_000; - vi.spyOn(Date, "now").mockReturnValue(first); - const { patches, land } = renderFilters(NO_FILTERS, true); - - openTimeMenu(); - fireEvent.click(screen.getByRole("menuitem", { name: "Past hour" })); - - // A second pick before the first has come back through the URL. - const second = first + 60_000; - vi.spyOn(Date, "now").mockReturnValue(second); - openTimeMenu(); - fireEvent.click(screen.getByRole("menuitem", { name: "Past 24 hours" })); - - expect(patches).toEqual([ - { since: first / 1000 - 3600, until: first / 1000 }, - { since: second / 1000 - 86_400, until: second / 1000 }, + openClientMenu(); + // The name is the way in; the address is still the filter. + expect(screen.getAllByRole("menuitemcheckbox").map((item) => item.textContent)).toEqual([ + "192.0.2.12", + "Kitchen Pi — 192.0.2.10", + "laptop.lan — 192.0.2.11", ]); +}); - // The older window lands first. It is this toolbar's own echo, so it must not - // read as a range someone else chose: doing that would clear the preset and - // throw the custom row open over a window the reader has already moved past. - land({ ...NO_FILTERS, since: first / 1000 - 3600, until: first / 1000 }); - expect(screen.queryByLabelText("Since")).toBeNull(); +test("picking clients commits them to the url at once, as one comma-separated value", () => { + vi.useFakeTimers(); + try { + const { patches, state } = renderFilters(); - land({ ...NO_FILTERS, since: second / 1000 - 86_400, until: second / 1000 }); - expect(screen.getByRole("button", { name: "Time: Past 24 hours" })).toBeTruthy(); - expect(screen.queryByLabelText("Since")).toBeNull(); + pickClient("Kitchen Pi — 192.0.2.10"); + // No debounce: a pick is a decision the reader has finished making, and + // nothing about it can be half-typed. + expect(patches).toEqual([{ client: "192.0.2.10" }]); + expect(state.current.client).toBe("192.0.2.10"); - vi.restoreAllMocks(); + pickClient("laptop.lan — 192.0.2.11"); + expect(patches).toEqual([{ client: "192.0.2.10" }, { client: "192.0.2.10,192.0.2.11" }]); + expect(state.current.client).toBe("192.0.2.10,192.0.2.11"); + + // Nothing lands later either: there is no pending commit behind these. + act(() => vi.advanceTimersByTime(400)); + expect(patches).toHaveLength(2); + } finally { + vi.useRealTimers(); + } +}); + +test("the trigger names a single client and counts several", () => { + const { state } = renderFilters(); + + pickClient("Kitchen Pi — 192.0.2.10"); + expect(clientTrigger().textContent).toContain("Kitchen Pi — 192.0.2.10"); + + pickClient("laptop.lan — 192.0.2.11"); + expect(clientTrigger().textContent).toContain("2 clients"); + expect(state.current.client).toBe("192.0.2.10,192.0.2.11"); +}); + +test("a picked client is a chip, and the chip takes it back off", () => { + const { state } = renderFilters(); + + pickClient("Kitchen Pi — 192.0.2.10"); + pickClient("laptop.lan — 192.0.2.11"); + // A chip is one client beside a row of others, so it wears the short form: the + // address is the half that does not fit, and the menu still spells it out. + expect(clientChips()).toEqual(["Kitchen Pi", "laptop.lan"]); + + fireEvent.click(screen.getByRole("button", { name: "Remove client Kitchen Pi — 192.0.2.10" })); + + expect(state.current.client).toBe("192.0.2.11"); + expect(clientChips()).toEqual(["laptop.lan"]); +}); + +test("an address no client claims still shows as a chip and can still be removed", () => { + // The log names devices the config has never heard of, and a link filtered on + // one has to stay readable and clearable even though the menu cannot offer it. + const { state } = renderFilters({ ...NO_FILTERS, client: "203.0.113.9,192.0.2.10" }); + + expect(clientChips()).toEqual(["203.0.113.9", "Kitchen Pi"]); + expect(clientTrigger().textContent).toContain("2 clients"); + + fireEvent.click(screen.getByRole("button", { name: "Remove client 203.0.113.9" })); + expect(state.current.client).toBe("192.0.2.10"); +}); + +test("the menu ticks the clients the url names, not the ones last picked here", () => { + const { land } = renderFilters(); + pickClient("Kitchen Pi — 192.0.2.10"); + + // The URL moves to a different client from somewhere else. + land({ ...NO_FILTERS, client: "192.0.2.11" }); + expect(clientChips()).toEqual(["laptop.lan"]); + + openClientMenu(); + const ticked = screen + .getAllByRole("menuitemcheckbox") + .filter((item) => item.getAttribute("aria-checked") === "true"); + expect(ticked.map((item) => item.textContent?.replace("✓", ""))).toEqual(["laptop.lan — 192.0.2.11"]); +}); + +test("a rename moves the label and leaves the filter on the address", async () => { + const { state, queryClient } = renderFilters(); + pickClient("Kitchen Pi — 192.0.2.10"); + + act(() => { + queryClient.setQueryData( + queryKeys.clients, + CLIENTS.map((entry) => (entry.ip === "192.0.2.10" ? { ...entry, name: "Hallway Pi" } : entry)), + ); + }); + + // Nothing here holds a label, so nothing here can commit one: the chip is a + // rendering of the address, re-rendered. + await waitFor(() => expect(clientChips()).toEqual(["Hallway Pi"])); + expect(state.current.client).toBe("192.0.2.10"); +}); + +test("Clear empties the picker along with everything else", () => { + const { state } = renderFilters(); + pickClient("Kitchen Pi — 192.0.2.10"); + expect(clientChips()).toHaveLength(1); + + fireEvent.click(screen.getByRole("button", { name: "Clear" })); + + expect(clientChips()).toEqual([]); + expect(state.current).toEqual(NO_FILTERS); +}); + +test("with no clients loaded the trigger is disabled rather than opening on nothing", () => { + renderFilters({ ...NO_FILTERS, client: "192.0.2.10" }, false, false); + + expect(clientTrigger().hasAttribute("disabled")).toBe(true); + // The filter the URL carries is still on screen and still removable, because + // none of that waits on the convenience that names it. + expect(clientChips()).toEqual(["192.0.2.10"]); +}); + +test("past three chips the rest are a count, and the count removes nothing", () => { + const clients = manyClients(12); + renderFilters({ ...NO_FILTERS, client: clients.map((entry) => entry.ip).join(",") }, false, true, clients); + + // Three, and then how many more. A dozen chips are not more visible than + // three: they wrap the toolbar into a block and push the table off the screen. + expect(clientChips()).toEqual(["Device 1", "Device 2", "Device 3"]); + expect(screen.getByText("+9 more")).toBeTruthy(); + expect(clientTrigger().textContent).toContain("12 clients"); + + // The summary is not a button, so there is no fourth thing to remove and no + // pointer target that does nothing. + expect(within(screen.getByRole("group", { name: "Clients" })).getAllByRole("button")).toHaveLength(4); + expect(screen.queryByRole("button", { name: /Remove client Device 4/ })).toBeNull(); +}); + +test("every known client at once commits every one of their addresses", () => { + const { patches, state } = renderFilters(); + + pickClient("Kitchen Pi — 192.0.2.10"); + pickClient("laptop.lan — 192.0.2.11"); + expect(state.current.client).toBe("192.0.2.10,192.0.2.11"); + + // The whole household is still written out. What is picked is what is + // committed, so the ticks, the chips and the URL all say what the reader just + // did — and pruned clients keep history rows, so this is not "no filter". + pickClient("192.0.2.12"); + expect(patches.at(-1)).toEqual({ client: "192.0.2.10,192.0.2.11,192.0.2.12" }); + expect(state.current.client).toBe("192.0.2.10,192.0.2.11,192.0.2.12"); + expect(clientChips()).toEqual(["Kitchen Pi", "laptop.lan", "192.0.2.12"]); +}); + +test("the only client on the network is still a client you can pick", () => { + // The household the owner hit: one known client, so picking it is picking all + // of them. A picker that answered that click by clearing itself would read as + // a control that did nothing at all. + const { state } = renderFilters(NO_FILTERS, false, true, [CLIENTS[0] as Client]); + + pickClient("Kitchen Pi — 192.0.2.10"); + + expect(state.current.client).toBe("192.0.2.10"); + expect(clientChips()).toEqual(["Kitchen Pi"]); + expect(clientTrigger().textContent).toContain("Kitchen Pi — 192.0.2.10"); + + openClientMenu(); + expect(screen.getByRole("menuitemcheckbox", { name: "Kitchen Pi — 192.0.2.10" }).getAttribute("aria-checked")).toBe( + "true", + ); +}); + +test("the menu stops at the cap the server enforces, and says so", () => { + const clients = manyClients(40); + const picked = clients.slice(0, 32).map((entry) => entry.ip); + const { patches } = renderFilters({ ...NO_FILTERS, client: picked.join(",") }, false, true, clients); + + openClientMenu(); + expect(screen.getByText("At most 32 clients at a time.")).toBeTruthy(); + const item = screen.getByRole("menuitemcheckbox", { name: "Device 33 — 198.51.100.33" }); + expect(item.getAttribute("aria-disabled")).toBe("true"); + + // A menu that took a 33rd pick and dropped it would look like it had worked. + press(item); + expect(patches).toEqual([]); + + // What is already picked can still be unpicked, or the reader would be stuck. + const chosen = screen.getByRole("menuitemcheckbox", { name: "Device 1 — 198.51.100.1" }); + expect(chosen.getAttribute("aria-disabled")).toBeNull(); +}); + +test("the cap note appears when the cap binds, however few clients are loaded", () => { + const clients = manyClients(5); + // Two known and thirty unknown: the cap is full while the menu still has three + // rows it will not let anyone pick, and a disabled row with nothing to explain + // it is the one state this must not reach. + const unknown = Array.from({ length: 30 }, (_, index) => `203.0.113.${index + 1}`); + renderFilters( + { ...NO_FILTERS, client: [clients[0]!.ip, clients[1]!.ip, ...unknown].join(",") }, + false, + true, + clients, + ); + + openClientMenu(); + expect(screen.getByText("At most 32 clients at a time.")).toBeTruthy(); + expect( + screen.getByRole("menuitemcheckbox", { name: "Device 3 — 198.51.100.3" }).getAttribute("aria-disabled"), + ).toBe("true"); +}); + +test("with room left the cap note stays out of the menu", () => { + renderFilters(); + openClientMenu(); + + // Three clients and nothing picked: a ceiling nobody can reach is not news. + expect(screen.queryByText("At most 32 clients at a time.")).toBeNull(); +}); + +test("an address no client claims takes a slot in the cap like any other", () => { + const clients = manyClients(40); + // Thirty-one known and one the config has never heard of. The cap is on what + // the request may name, not on what this menu happens to be able to show. + const picked = [...clients.slice(0, 31).map((entry) => entry.ip), "203.0.113.9"]; + renderFilters({ ...NO_FILTERS, client: picked.join(",") }, false, true, clients); + + openClientMenu(); + const item = screen.getByRole("menuitemcheckbox", { name: "Device 33 — 198.51.100.33" }); + expect(item.getAttribute("aria-disabled")).toBe("true"); +}); + +test("select-all is held to the cap, with the addresses already filtered keeping their slots", () => { + const clients = manyClients(40); + const unknown = ["203.0.113.1", "203.0.113.2", "203.0.113.3", "203.0.113.4", "203.0.113.5"]; + const { state } = renderFilters({ ...NO_FILTERS, client: unknown.join(",") }, false, true, clients); + + openClientMenu(); + // Ctrl+A reaches the selection without pressing an item, so the disabled rows + // never see it and the cap has to hold here too. + fireEvent.keyDown(screen.getByRole("menu"), { key: "a", code: "KeyA", ctrlKey: true }); + fireEvent.keyUp(screen.getByRole("menu"), { key: "a", code: "KeyA", ctrlKey: true }); + + const applied = state.current.client?.split(",") ?? []; + expect(applied).toHaveLength(32); + // What was already filtered keeps its slots and the new picks take what is + // left, rather than the list being cut wherever it happened to run out. + expect(applied.slice(0, 5)).toEqual(unknown); + // The rest are known clients, each once. Which 27 is the menu's own order and + // not something this test should restate. + const rest = applied.slice(5); + const addresses = new Set(clients.map((entry) => entry.ip)); + expect(rest.every((ip) => addresses.has(ip))).toBe(true); + expect(new Set(rest).size).toBe(27); +}); + +test("removing a chip hands the focus on rather than dropping it", () => { + const clients = manyClients(5); + const { state } = renderFilters( + { + ...NO_FILTERS, + client: clients + .slice(0, 4) + .map((entry) => entry.ip) + .join(","), + }, + false, + true, + clients, + ); + + // The chip that takes the removed one's place, so a reader clearing three + // clients from the keyboard does not tab back in from the top each time. + fireEvent.click(screen.getByRole("button", { name: "Remove client Device 2 — 198.51.100.2" })); + expect(document.activeElement?.getAttribute("aria-label")).toBe("Remove client Device 3 — 198.51.100.3"); + + // Nothing after it, so the one before it. + fireEvent.click(screen.getByRole("button", { name: "Remove client Device 4 — 198.51.100.4" })); + fireEvent.click(screen.getByRole("button", { name: "Remove client Device 3 — 198.51.100.3" })); + expect(document.activeElement?.getAttribute("aria-label")).toBe("Remove client Device 1 — 198.51.100.1"); + + // No chips left, so the control the row belongs to. + fireEvent.click(screen.getByRole("button", { name: "Remove client Device 1 — 198.51.100.1" })); + expect(document.activeElement).toBe(clientTrigger()); + expect(state.current.client).toBeUndefined(); }); diff --git a/admin/src/features/activity/ActivityFilters.tsx b/admin/src/features/activity/ActivityFilters.tsx index f612ff2..9e00932 100644 --- a/admin/src/features/activity/ActivityFilters.tsx +++ b/admin/src/features/activity/ActivityFilters.tsx @@ -3,9 +3,9 @@ * * The applied state is the URL, never this form: what the reader sees is what * the link they can paste to a housemate will show. So this holds a draft, and - * every control writes through to the URL — the segments and the time presets - * at once, the two text fields after a pause so a five-letter domain is one - * navigation rather than five. + * every control writes through to the URL — the segments, the client picker and + * the time presets at once, the domain field after a pause so a five-letter word + * is one navigation rather than five. * * A time preset writes both bounds as absolute seconds, resolved once at the * click. Neither half may be left open: a bookmark has to describe the same @@ -34,6 +34,7 @@ import * as stylex from "@stylexjs/stylex"; import { Button, Menu, MenuItem, MenuTrigger, Popover, Radio, RadioGroup } from "react-aria-components"; import { styles as shared } from "@/ui/styles"; import { colors } from "@/ui/tokens.stylex"; +import ClientFilter, { joinClients, parseClients, useClientOptions } from "./ClientFilter"; import { datetimeField, editDatetimeField, resolveDatetimeField, type DatetimeField } from "./datetime"; import type { ActivitySearch } from "./search"; @@ -90,11 +91,6 @@ const styles = stylex.create({ width: "100%", paddingInlineStart: "1.875rem", }, - clientInput: { - flexGrow: 0, - flexShrink: 1, - flexBasis: "10rem", - }, /** A button is text-sized by default; this is the hit area around the text. */ hitTarget: { minHeight: HIT_TARGET, @@ -217,12 +213,6 @@ interface Bounds { until: number | undefined; } -/** The two text filters as the URL spells them, with absent written as empty. */ -interface TextPair { - domain: string; - client: string; -} - function sameBounds(a: Bounds, b: Bounds): boolean { return a.since === b.since && a.until === b.until; } @@ -272,19 +262,22 @@ interface ChosenPreset extends Bounds { * What this toolbar knows about the URL, and what it is still waiting to see * come back from it. * - * `url` and `bounds` are the last values looked at, so a change can be told - * apart field by field. `pendingText` and `pendingBounds` are every commit made - * here that the URL has not echoed yet. Both are queues rather than single - * slots: two keystrokes either side of the debounce put two text commits in + * `domain` and `bounds` are the last values looked at, so a change can be told + * apart field by field. `pendingDomain` and `pendingBounds` are every commit + * made here that the URL has not echoed yet. Both are queues rather than single + * slots: two keystrokes either side of the debounce put two domain commits in * flight, and two menu picks in quick succession do the same to the range. In * both cases the older echo landing second must not be mistaken for someone * else's edit — that is what would clear the preset out from under the pick * that is actually current. + * + * Only the domain needs any of this. Every other control commits on the action + * itself, so the URL is never behind what the reader is doing with it. */ interface Sync { - url: TextPair; + domain: string; bounds: Bounds; - pendingText: TextPair[]; + pendingDomain: string[]; pendingBounds: Bounds[]; } @@ -296,41 +289,41 @@ interface Props { } export default function ActivityFilters({ applied, onApply, onClear }: Props) { - const urlText: TextPair = { domain: applied.domain ?? "", client: applied.client ?? "" }; + const urlDomain = applied.domain ?? ""; const urlBounds: Bounds = { since: applied.since, until: applied.until }; - const [domain, setDomain] = useState(urlText.domain); - const [client, setClient] = useState(urlText.client); + const clientOptions = useClientOptions(); + // The picked clients are read straight off the URL: there is no draft to hold, + // because there is no state here the reader can leave half-finished. + const clients = parseClients(applied.client); + const [domain, setDomain] = useState(urlDomain); const [preset, setPreset] = useState(null); const [showCustom, setShowCustom] = useState(applied.since !== undefined || applied.until !== undefined); const [since, setSince] = useState(() => datetimeField(applied.since)); const [until, setUntil] = useState(() => datetimeField(applied.until)); const [error, setError] = useState(null); const [sync, setSync] = useState({ - url: urlText, + domain: urlDomain, bounds: urlBounds, - pendingText: [], + pendingDomain: [], pendingBounds: [], }); - const textMoved = sync.url.domain !== urlText.domain || sync.url.client !== urlText.client; + const domainMoved = sync.domain !== urlDomain; const boundsMoved = !sameBounds(sync.bounds, urlBounds); - if (textMoved || boundsMoved) { - let pendingText = sync.pendingText; + if (domainMoved || boundsMoved) { + let pendingDomain = sync.pendingDomain; let pendingBounds = sync.pendingBounds; - if (textMoved) { + if (domainMoved) { // The newest commit the URL matches, and everything before it, has now // been accounted for; an older one landing later is not new news. - const echo = pendingText.findLastIndex( - (sent) => sent.domain === urlText.domain && sent.client === urlText.client, - ); + const echo = pendingDomain.findLastIndex((sent) => sent === urlDomain); if (echo >= 0) { - pendingText = pendingText.slice(echo + 1); + pendingDomain = pendingDomain.slice(echo + 1); } else { - pendingText = []; - if (sync.url.domain !== urlText.domain) setDomain(urlText.domain); - if (sync.url.client !== urlText.client) setClient(urlText.client); + pendingDomain = []; + setDomain(urlDomain); } } @@ -352,21 +345,18 @@ export default function ActivityFilters({ applied, onApply, onClear }: Props) { } } - setSync({ url: urlText, bounds: urlBounds, pendingText, pendingBounds }); + setSync({ domain: urlDomain, bounds: urlBounds, pendingDomain, pendingBounds }); } - const dirty = textFilter(domain) !== applied.domain || textFilter(client) !== applied.client; + const dirty = textFilter(domain) !== applied.domain; - const commitText = useCallback( + const commitDomain = useCallback( (replace: boolean) => { - const next = { domain: textFilter(domain), client: textFilter(client) }; - setSync((prev) => ({ - ...prev, - pendingText: [...prev.pendingText, { domain: next.domain ?? "", client: next.client ?? "" }], - })); - onApply(next, replace); + const next = textFilter(domain); + setSync((prev) => ({ ...prev, pendingDomain: [...prev.pendingDomain, next ?? ""] })); + onApply({ domain: next }, replace); }, - [domain, client, onApply], + [domain, onApply], ); function commitBounds(bounds: Bounds) { @@ -376,13 +366,13 @@ export default function ActivityFilters({ applied, onApply, onClear }: Props) { useEffect(() => { if (!dirty) return; - const id = setTimeout(() => commitText(true), DEBOUNCE_MS); + const id = setTimeout(() => commitDomain(true), DEBOUNCE_MS); return () => clearTimeout(id); - }, [dirty, commitText]); + }, [dirty, commitDomain]); function flush(event: FormEvent) { event.preventDefault(); - if (dirty) commitText(true); + if (dirty) commitDomain(true); } function selectPreset(label: string) { @@ -439,7 +429,6 @@ export default function ActivityFilters({ applied, onApply, onClear }: Props) { function clear() { setDomain(""); - setClient(""); setPreset(null); setShowCustom(false); setSince(datetimeField(undefined)); @@ -447,7 +436,7 @@ export default function ActivityFilters({ applied, onApply, onClear }: Props) { setError(null); setSync((prev) => ({ ...prev, - pendingText: [], + pendingDomain: [], pendingBounds: [...prev.pendingBounds, { since: undefined, until: undefined }], })); onClear(); @@ -455,7 +444,7 @@ export default function ActivityFilters({ applied, onApply, onClear }: Props) { const active = domain.trim() !== "" || - client.trim() !== "" || + clients.length > 0 || applied.blocked !== undefined || applied.since !== undefined || applied.until !== undefined; @@ -537,20 +526,10 @@ export default function ActivityFilters({ applied, onApply, onClear }: Props) { {...stylex.props(shared.smallInput, styles.field, styles.searchInput, shared.focusRing)} /> - {/* - * The client filter matches one address exactly — the server has no - * substring match for it — so the name says so rather than leaving the - * reader to discover it by typing half an address and getting nothing. - */} - setClient(event.target.value)} - {...stylex.props(shared.smallInput, styles.field, styles.clientInput, shared.focusRing)} + onApply({ client: joinClients(next) })} /> { + const options = [...names.keys()].map((ip) => { + const name = clientLabel(ip, names)?.text ?? null; + return { ip, name, label: optionLabel(ip, name) }; + }); + return options.sort((a, b) => a.label.localeCompare(b.label)); + }, [names]); +} + +/** How an address reads in full: its client's line, or the bare address. */ +export function displayFor(ip: string, options: readonly ClientOption[]): string { + return options.find((option) => option.ip === ip)?.label ?? ip; +} + +/** How an address reads on a chip: its client's name, or the bare address. */ +export function chipFor(ip: string, options: readonly ClientOption[]): string { + return options.find((option) => option.ip === ip)?.name ?? ip; +} + +/** + * The URL carries the selection as one comma-separated value; absent means none. + * + * A plain split, because `validateClients` has already made the value canonical + * — trimmed, no blanks, no repeats, within the cap. Dropping anything a second + * time here is what would let the chips and the request disagree. + */ +export function parseClients(value: string | undefined): string[] { + if (value === undefined) return []; + return value.split(","); +} + +export function joinClients(ips: readonly string[]): string | undefined { + return ips.length === 0 ? undefined : ips.join(","); +} + +/** + * The trigger's label. A single client is named outright, because that is the + * one case where the whole filter fits on the button; more than one would not, + * and the chips beside it say which ones anyway. + */ +function triggerLabel(selected: readonly string[], options: readonly ClientOption[]): string { + if (selected.length === 0) return "Clients"; + if (selected.length === 1) return displayFor(selected[0] as string, options); + return `${selected.length} clients`; +} + +interface Props { + options: readonly ClientOption[]; + selected: readonly string[]; + /** Every change here is a decision already made, so it applies at once. */ + onChange: (next: string[]) => void; +} + +export default function ClientFilter({ options, selected, onChange }: Props) { + const known = new Set(options.map((option) => option.ip)); + // The menu is offered only the addresses it can account for. An address no + // client claims is not in its collection, so handing it over as a selected key + // would be handing over a key that resolves to nothing. + const chosen = new Set(selected.filter((ip) => known.has(ip))); + // Every address the URL carries takes a slot, whether or not a client claims + // it: the cap is on what the request may name, not on what this menu can show. + const atCap = selected.length >= MAX_CLIENTS; + // True only when the cap is actually holding something back. Unknown addresses + // spend slots too, so a short list of clients can be closed off while the menu + // still has room in it — and a disabled row with nothing to explain it is the + // one state this must not reach. + const capBinds = atCap && chosen.size < options.length; + + const chips = useRef(new Map()); + const trigger = useRef(null); + /** Where focus goes once the removed chip is gone; null means the trigger. */ + const focusAfterRemoval = useRef(undefined); + + useEffect(() => { + const next = focusAfterRemoval.current; + if (next === undefined) return; + focusAfterRemoval.current = undefined; + // Removing a chip unmounts the element that had the focus. Left alone the + // browser drops focus to the document, and a reader clearing three clients + // from the keyboard would have to tab back in from the top each time. + (next === null ? trigger.current : (chips.current.get(next) ?? trigger.current))?.focus(); + }); + + function remove(ip: string) { + const next = selected.filter((entry) => entry !== ip); + const visible = next.slice(0, MAX_CHIPS); + const index = selected.indexOf(ip); + // The chip that takes this one's place, or the one before it at the end of + // the row, or the trigger when the row is empty. + focusAfterRemoval.current = visible[index] ?? visible[index - 1] ?? null; + onChange(next); + } + + /** + * A selection from the menu, merged back over the addresses the menu could not + * see and turned into what the URL should carry. + * + * What is picked is what is committed, always — even every known client at + * once. The tick, the chip and the URL then say what the reader did, and a + * picker whose feedback for "you selected everything" is to erase the + * selection reads as a control that ignored the click; with one client on the + * network that is every click it will ever get. Nor is the full set a no-op: + * the history keeps rows for clients the inventory has since pruned, so "all + * known clients" and "no filter" are different questions. + * + * The cap is enforced here as well as on the items, because select-all reaches + * this without passing an item at all. What was already filtered keeps its + * slots and the new picks take what is left, so a selection too big to send is + * cut somewhere the reader can predict rather than wherever the URL ran out. + */ + function apply(next: Set) { + const kept = selected.filter((ip) => !known.has(ip) || next.has(ip)); + const added = [...next].filter((ip) => !chosen.has(ip)); + onChange([...kept, ...added].slice(0, MAX_CLIENTS)); + } + + return ( + // The picker and the chips it fills are one control between them: the group + // says so, and names the chips' "Remove …" buttons as part of it. +
+ + + stylex.props(styles.popover).className ?? ""}> + {/* Said where it is doing something, and nowhere else. */} + {capBinds &&

At most {MAX_CLIENTS} clients at a time.

} + { + // Ctrl/Cmd+A hands back the literal "all"; taking every known + // client is its only reading, and apply() caps the result. + if (keys === "all") apply(new Set(options.map((option) => option.ip))); + else apply(new Set([...keys].map(String))); + }} + autoFocus={false} + {...stylex.props(styles.menu)} + > + {options.map((option) => ( + + stylex.props(styles.item, shared.insetFocusRing, isFocused && styles.itemFocused) + .className ?? "" + } + > + + {option.label} + + ))} + + + + {selected.slice(0, MAX_CHIPS).map((ip) => ( + + ))} + {selected.length > MAX_CHIPS && ( + +{selected.length - MAX_CHIPS} more + )} +
+ ); +} diff --git a/admin/src/features/activity/search.test.ts b/admin/src/features/activity/search.test.ts index 5297ac3..38c1038 100644 --- a/admin/src/features/activity/search.test.ts +++ b/admin/src/features/activity/search.test.ts @@ -1,4 +1,12 @@ -import { validateActivitySearch, validateBlocked, validateMode, validateText, validateTimestamp } from "./search"; +import { + validateActivitySearch, + validateBlocked, + validateMode, + validateText, + validateTimestamp, + MAX_CLIENTS, + validateClients, +} from "./search"; test("mode is the two-value union, defaulting to history", () => { expect(validateMode("live")).toBe("live"); @@ -111,3 +119,23 @@ test("a search of junk applies nothing", () => { blocked: undefined, }); }); + +test("the client list is canonicalized once, so the chips and the request agree", () => { + expect(validateClients("192.0.2.10,192.0.2.11")).toBe("192.0.2.10,192.0.2.11"); + + // Blanks name no client and a repeat asks for the same client twice, so + // neither changes which rows come back: dropping them is the same filter + // written once, not a different one. + expect(validateClients(" 192.0.2.10 , ,192.0.2.11,192.0.2.10,")).toBe("192.0.2.10,192.0.2.11"); + expect(validateClients(",,")).toBeUndefined(); + expect(validateClients("")).toBeUndefined(); + expect(validateClients(null)).toBeUndefined(); +}); + +test("a pasted list past the cap is cut to what the api will accept", () => { + const addresses = Array.from({ length: MAX_CLIENTS + 8 }, (_, index) => `198.51.100.${index + 1}`); + + // The API refuses a longer list outright, so keeping the extra addresses + // would show a filter that cannot be applied at all. + expect(validateClients(addresses.join(","))).toBe(addresses.slice(0, MAX_CLIENTS).join(",")); +}); diff --git a/admin/src/features/activity/search.ts b/admin/src/features/activity/search.ts index 9c0f43a..5d30106 100644 --- a/admin/src/features/activity/search.ts +++ b/admin/src/features/activity/search.ts @@ -57,6 +57,37 @@ export function validateText(value: unknown): string | undefined { return trimmed === "" ? undefined : trimmed; } +/** `queries_repo.max_clients`: past this the API answers 400 rather than filter. */ +export const MAX_CLIENTS = 32; + +/** + * The client filter: a comma-separated list of exact addresses, canonicalized + * here and nowhere else. + * + * This is the one place the value is read, so it is the one place it can be made + * to mean exactly one thing. Everything downstream — the chips that show the + * filter and the request that applies it — reads what this returns, so the two + * cannot disagree about a link somebody pasted. + * + * The rule: entries are trimmed, blanks are dropped, repeats are dropped, and + * the list is cut to the cap. A blank entry names no client and a repeat asks + * for the same client twice, so neither changes which rows come back; dropping + * them is not a different filter, it is the same filter written once. The cut is + * a different filter, and it is the honest one available: the API refuses a + * longer list outright, so keeping the extra addresses would show a filter that + * cannot be applied at all. + */ +export function validateClients(value: unknown): string | undefined { + if (typeof value !== "string") return undefined; + const seen = new Set(); + for (const entry of value.split(",")) { + const trimmed = entry.trim(); + if (trimmed !== "") seen.add(trimmed); + if (seen.size === MAX_CLIENTS) break; + } + return seen.size === 0 ? undefined : [...seen].join(","); +} + /** * The API filter for a validated search, built field by field. * @@ -82,7 +113,7 @@ export function validateActivitySearch(search: Record): Activit since: validateTimestamp(search["since"]), until: validateTimestamp(search["until"]), domain: validateText(search["domain"]), - client: validateText(search["client"]), + client: validateClients(search["client"]), blocked: validateBlocked(search["blocked"]), }; } diff --git a/admin/src/ui/Select.tsx b/admin/src/ui/Select.tsx index c2177e0..e4db14f 100644 --- a/admin/src/ui/Select.tsx +++ b/admin/src/ui/Select.tsx @@ -10,6 +10,7 @@ * models an id converts on both edges. */ +import { CaretDown } from "@phosphor-icons/react"; import * as stylex from "@stylexjs/stylex"; import { Button, @@ -83,6 +84,7 @@ const styles = stylex.create({ whiteSpace: "nowrap", }, chevron: { + display: "inline-flex", color: colors.textMuted, }, description: { @@ -155,7 +157,7 @@ export default function Select({ {description !== undefined && ( diff --git a/src/storage/repositories/queries_repo.zig b/src/storage/repositories/queries_repo.zig index 1d359e5..a76f3b0 100644 --- a/src/storage/repositories/queries_repo.zig +++ b/src/storage/repositories/queries_repo.zig @@ -674,7 +674,10 @@ pub const QueryFilter = struct { /// Matched case-insensitively for ASCII, which is what SQLite's `LIKE` /// does and what a domain search wants. domain_substring: ?[]const u8 = null, - client: ?[]const u8 = null, + /// Exact client addresses, matched any-of. Empty means no client filter; + /// one address is the common case and reads the same as the old single + /// filter did. Never more than `max_clients`. + clients: []const []const u8 = &.{}, blocked: ?bool = null, since: ?i64 = null, until: ?i64 = null, @@ -684,6 +687,12 @@ pub const QueryFilter = struct { /// that forgets cannot ask this connection for the whole table. pub const max_limit: u32 = 1000; +/// How many addresses one client filter may name. The statement is assembled +/// into a fixed buffer, so this is a hard bound rather than a preference: a +/// household picking more than this from a list is not a case worth widening +/// the buffer for. +pub const max_clients: usize = 32; + const select_head = \\SELECT q.id, q.timestamp, d.domain, q.client_ip, q.qtype, q.blocked, \\ q.response_time_us, q.cache_hit, q.upstream, q.qclass, q.rcode, @@ -697,7 +706,10 @@ const like_escape = '\\'; const where_before = " q.id < ?"; const where_domain = " d.domain LIKE ? ESCAPE '\\'"; -const where_client = " q.client_ip = ?"; +const where_client_head = " q.client_ip IN ("; +const where_client_tail = ")"; +/// `?` per address with a comma between, at the widest the cap allows. +const where_client_max = where_client_head.len + 2 * max_clients + where_client_tail.len; const where_blocked = " q.blocked = ?"; const where_since = " q.timestamp >= ?"; const where_until = " q.timestamp < ?"; @@ -716,7 +728,7 @@ const Sql = struct { /// `where_keyword` is longer than `and_keyword` and is used at most once, /// so counting six of it bounds every reachable combination. const capacity = select_head.len + 6 * where_keyword.len + select_tail.len + - where_before.len + where_domain.len + where_client.len + + where_before.len + where_domain.len + where_client_max + where_blocked.len + where_since.len + where_until.len; buf: [capacity]u8 = undefined, @@ -734,6 +746,19 @@ const Sql = struct { self.put(fragment); } + /// One placeholder per address. The addresses themselves are bound, like + /// every other value; only their count reaches this buffer. + fn clientPredicate(self: *Sql, count: usize) void { + self.put(if (self.has_where) and_keyword else where_keyword); + self.has_where = true; + self.put(where_client_head); + for (0..count) |i| { + if (i > 0) self.put(","); + self.put("?"); + } + self.put(where_client_tail); + } + fn text(self: *const Sql) []const u8 { return self.buf[0..self.len]; } @@ -743,11 +768,16 @@ const Sql = struct { /// `arena`, including the list's own storage, so the caller frees the whole /// result by resetting the arena — there is nothing to unwind on failure. pub fn selectQueries(database: *db.Db, arena: Allocator, filter: QueryFilter) db.Error!std.ArrayList(QueryRow) { + // The statement buffer is sized for the cap, so a longer list would be a + // buffer overrun rather than a slow query. The handler rejects it first; + // this is the wall behind that, for a caller that skips the handler. + if (filter.clients.len > max_clients) return error.Misuse; + var sql: Sql = .{}; sql.put(select_head); if (filter.before != null) sql.predicate(where_before); if (filter.domain_substring != null) sql.predicate(where_domain); - if (filter.client != null) sql.predicate(where_client); + if (filter.clients.len > 0) sql.clientPredicate(filter.clients.len); if (filter.blocked != null) sql.predicate(where_blocked); if (filter.since != null) sql.predicate(where_since); if (filter.until != null) sql.predicate(where_until); @@ -765,9 +795,9 @@ pub fn selectQueries(database: *db.Db, arena: Allocator, filter: QueryFilter) db idx += 1; try stmt.bindText(idx, try likePattern(arena, v)); } - if (filter.client) |v| { + for (filter.clients) |client| { idx += 1; - try stmt.bindText(idx, v); + try stmt.bindText(idx, client); } if (filter.blocked) |v| { idx += 1; @@ -2031,11 +2061,11 @@ test "each filter narrows the result on its own" { const by_domain = try selectQueries(&database, arena, .{ .domain_substring = "example.com" }); try testing.expectEqualSlices(i64, &.{ 3, 1 }, ids(by_domain.items, &buf)); - const by_client = try selectQueries(&database, arena, .{ .client = "192.0.2.20" }); + const by_client = try selectQueries(&database, arena, .{ .clients = &.{"192.0.2.20"} }); try testing.expectEqualSlices(i64, &.{2}, ids(by_client.items, &buf)); // An exact match, not a prefix: the seeded clients share the first octets. - const no_client = try selectQueries(&database, arena, .{ .client = "192.0.2" }); + const no_client = try selectQueries(&database, arena, .{ .clients = &.{"192.0.2"} }); try testing.expectEqual(@as(usize, 0), no_client.items.len); const only_blocked = try selectQueries(&database, arena, .{ .blocked = true }); @@ -2049,7 +2079,7 @@ test "each filter narrows the result on its own" { .limit = 10, .before = 3, .domain_substring = "ads", - .client = "192.0.2.20", + .clients = &.{"192.0.2.20"}, .blocked = true, .since = 200, .until = 300, @@ -2125,18 +2155,84 @@ test "the built SQL never carries a filter value and fits its buffer" { sql.put(select_head); sql.predicate(where_before); sql.predicate(where_domain); - sql.predicate(where_client); + sql.clientPredicate(max_clients); sql.predicate(where_blocked); sql.predicate(where_since); sql.predicate(where_until); sql.put(select_tail); - // Every predicate present is the longest reachable statement. + // Every predicate present, with the client list at its cap, is the longest + // reachable statement — which is what the buffer is sized against. try testing.expect(sql.len <= Sql.capacity); try testing.expectEqual(@as(usize, 1), std.mem.count(u8, sql.text(), " WHERE")); try testing.expectEqual(@as(usize, 5), std.mem.count(u8, sql.text(), " AND")); - // Six filters plus the LIMIT, each a bare parameter. - try testing.expectEqual(@as(usize, 7), std.mem.count(u8, sql.text(), "?")); + // Five scalar filters plus the LIMIT, plus one per address, each a bare + // parameter: no value is ever spelled into the statement. + try testing.expectEqual(@as(usize, 6 + max_clients), std.mem.count(u8, sql.text(), "?")); + // The select list has commas of its own, so the client list is measured as + // the difference against the same statement without it. + var without: Sql = .{}; + without.put(select_head); + without.predicate(where_before); + without.predicate(where_domain); + without.predicate(where_blocked); + without.predicate(where_since); + without.predicate(where_until); + without.put(select_tail); + try testing.expectEqual( + @as(usize, max_clients - 1), + std.mem.count(u8, sql.text(), ",") - std.mem.count(u8, without.text(), ","), + ); +} + +test "one client reads as an exact match and several read as any-of" { + var arena_state: std.heap.ArenaAllocator = .init(testing.allocator); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + var database = try openLog(); + defer database.close(); + + var second = plainRow(200, "b.example"); + second.client_ip = "192.0.2.11"; + var third = plainRow(300, "c.example"); + third.client_ip = "192.0.2.12"; + try seed(&database, &.{ plainRow(100, "a.example"), second, third }); + + var buf: [8]i64 = undefined; + + const one = try selectQueries(&database, arena, .{ .clients = &.{"192.0.2.11"} }); + try testing.expectEqualSlices(i64, &.{2}, ids(one.items, &buf)); + + // Newest-first, so the higher id leads however the addresses are ordered. + const two = try selectQueries(&database, arena, .{ .clients = &.{ "192.0.2.12", "192.0.2.10" } }); + try testing.expectEqualSlices(i64, &.{ 3, 1 }, ids(two.items, &buf)); + + // An address nothing was logged from narrows to nothing rather than being + // ignored, which is the difference between a filter and a suggestion. + const absent = try selectQueries(&database, arena, .{ .clients = &.{"198.51.100.1"} }); + try testing.expectEqual(@as(usize, 0), absent.items.len); + + // No addresses at all is no client filter. + const none = try selectQueries(&database, arena, .{}); + try testing.expectEqual(@as(usize, 3), none.items.len); +} + +test "a client list past the cap is refused rather than truncated" { + var arena_state: std.heap.ArenaAllocator = .init(testing.allocator); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + var database = try openLog(); + defer database.close(); + + var too_many: [max_clients + 1][]const u8 = undefined; + for (&too_many) |*slot| slot.* = "192.0.2.10"; + + // Dropping the overflow would narrow the filter silently, which answers a + // question the caller did not ask; the statement buffer could not hold it + // either way. + try testing.expectError(error.Misuse, selectQueries(&database, arena, .{ .clients = &too_many })); } test "likePattern wraps the needle and neutralises every metacharacter" { diff --git a/src/web/handlers/queries.zig b/src/web/handlers/queries.zig index 4388347..ed07075 100644 --- a/src/web/handlers/queries.zig +++ b/src/web/handlers/queries.zig @@ -32,12 +32,34 @@ pub const max_limit: u32 = queries_repo.max_limit; pub const max_domain_len = logger.max_domain_len; pub const max_client_len = logger.max_client_len; +/// How many addresses one `client` parameter may name, the repository's cap. +pub const max_clients = queries_repo.max_clients; + +/// The longest `client` value that can decode to a full list: every address at +/// its width, separated by commas. +pub const max_client_list_len = max_clients * max_client_len + (max_clients - 1); + +/// The buffer that value needs. +/// +/// `queryValue` measures the value as it arrives and only then decodes it in +/// place, so the buffer is sized for the percent-encoded form rather than the +/// decoded one. A browser encodes the separating commas, and every colon of an +/// IPv6 address with them, so a legal near-cap selection of IPv6 clients would +/// be refused by a buffer sized for what it decodes to. Three bytes per byte is +/// the worst `%XX` can do. +const max_client_value_len = 3 * max_client_list_len; + /// Where the two string filters are copied to. The parsed filter borrows them, /// so it must not outlive the buffers — in the handler both live in the same /// stack frame. +/// +/// `client` holds the raw comma-separated value and `clients` indexes into it, +/// so the addresses are never copied a second time: the filter's slices point +/// into the same bytes the query string was decoded into. pub const Buffers = struct { domain: [max_domain_len]u8 = undefined, - client: [max_client_len]u8 = undefined, + client: [max_client_value_len]u8 = undefined, + clients: [max_clients][]const u8 = undefined, }; pub const Page = struct { @@ -82,8 +104,22 @@ pub fn parseFilter(query: []const u8, buffers: *Buffers) FilterError!queries_rep if (domain.len != 0) filter.domain_substring = domain; } - if (http_util.queryValue(query, "client", &buffers.client) catch return error.BadClient) |client| { - if (client.len != 0) filter.client = client; + // A comma-separated list of exact addresses, matched any-of. An empty entry + // is a malformed list rather than a filter to drop: `?client=a,,b` is a + // client bug, and answering it as `a,b` would hide the bug behind an answer + // to a question nobody asked. + if (http_util.queryValue(query, "client", &buffers.client) catch return error.BadClient) |raw| { + if (raw.len != 0) { + var count: usize = 0; + var it = std.mem.splitScalar(u8, raw, ','); + while (it.next()) |entry| { + if (entry.len == 0 or entry.len > max_client_len) return error.BadClient; + if (count == max_clients) return error.BadClient; + buffers.clients[count] = entry; + count += 1; + } + filter.clients = buffers.clients[0..count]; + } } filter.blocked = http_util.queryBool(query, "blocked") catch return error.BadBlocked; @@ -98,7 +134,7 @@ pub fn message(err: FilterError) []const u8 { error.BadLimit => "limit must be between 1 and 1000", error.BadBefore => "before must be a positive row id", error.BadDomain => "domain is not a valid filter", - error.BadClient => "client is not a valid filter", + error.BadClient => "client must be up to 32 comma-separated client addresses", error.BadBlocked => "blocked must be true or false", error.BadSince => "since must be a unix timestamp in seconds", error.BadUntil => "until must be a unix timestamp in seconds", @@ -228,7 +264,8 @@ test "every filter reaches the repository untouched" { try testing.expectEqual(@as(u32, 250), filter.limit); try testing.expectEqual(@as(?i64, 900), filter.before); try testing.expectEqualStrings("ads.example", filter.domain_substring.?); - try testing.expectEqualStrings("192.0.2.10", filter.client.?); + try testing.expectEqual(@as(usize, 1), filter.clients.len); + try testing.expectEqualStrings("192.0.2.10", filter.clients[0]); try testing.expectEqual(@as(?bool, true), filter.blocked); try testing.expectEqual(@as(?i64, 100), filter.since); try testing.expectEqual(@as(?i64, 200), filter.until); @@ -238,7 +275,48 @@ test "an empty string filter is no filter at all" { var buffers: Buffers = .{}; const filter = try parseFilter("domain=&client=", &buffers); try testing.expectEqual(@as(?[]const u8, null), filter.domain_substring); - try testing.expectEqual(@as(?[]const u8, null), filter.client); + try testing.expectEqual(@as(usize, 0), filter.clients.len); +} + +test "a client list is several exact addresses and each entry must be well formed" { + var buffers: Buffers = .{}; + const several = try parseFilter("client=192.0.2.10,192.0.2.11,192.0.2.12", &buffers); + try testing.expectEqual(@as(usize, 3), several.clients.len); + try testing.expectEqualStrings("192.0.2.10", several.clients[0]); + try testing.expectEqualStrings("192.0.2.11", several.clients[1]); + try testing.expectEqualStrings("192.0.2.12", several.clients[2]); + + // An empty entry would silently widen the filter, so it is malformed input. + try testing.expectError(error.BadClient, parseFilter("client=192.0.2.10,", &buffers)); + try testing.expectError(error.BadClient, parseFilter("client=,192.0.2.10", &buffers)); + try testing.expectError(error.BadClient, parseFilter("client=192.0.2.10,,192.0.2.11", &buffers)); + + var many: std.ArrayList(u8) = .empty; + defer many.deinit(testing.allocator); + try many.appendSlice(testing.allocator, "client=192.0.2.1"); + for (0..max_clients) |_| try many.appendSlice(testing.allocator, ",192.0.2.1"); + try testing.expectError(error.BadClient, parseFilter(many.items, &buffers)); +} + +test "a full list of percent-encoded IPv6 clients fits" { + // A browser encodes the separators and every colon it puts between them, so + // the value on the wire is several times the length of what it decodes to. + // Sizing the buffer for the decoded form refuses a selection that is legal. + const address = "2001:0db8:0000:0000:0000:0000:0000:0001"; + const encoded = "2001%3A0db8%3A0000%3A0000%3A0000%3A0000%3A0000%3A0001"; + + var query: std.ArrayList(u8) = .empty; + defer query.deinit(testing.allocator); + try query.appendSlice(testing.allocator, "client="); + for (0..max_clients) |index| { + if (index != 0) try query.appendSlice(testing.allocator, "%2C"); + try query.appendSlice(testing.allocator, encoded); + } + + var buffers: Buffers = .{}; + const filter = try parseFilter(query.items, &buffers); + try testing.expectEqual(@as(usize, max_clients), filter.clients.len); + for (filter.clients) |entry| try testing.expectEqualStrings(address, entry); } test "each malformed parameter names itself in a 400" { diff --git a/src/web/openapi.yaml b/src/web/openapi.yaml index b96cd7f..1baa69b 100644 --- a/src/web/openapi.yaml +++ b/src/web/openapi.yaml @@ -200,8 +200,10 @@ paths: schema: { type: string, maxLength: 253 } - name: client in: query - description: Exact client address. - schema: { type: string, maxLength: 64 } + description: >- + Up to 32 comma-separated exact client addresses, matched any-of. + Each entry is at most 45 characters; an empty entry is a 400. + schema: { type: string, maxLength: 1471 } - name: blocked in: query schema: { type: boolean }