From 581e73f0bfc4bbfd3f19a23536c1b2849cd634cf Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 23 Sep 2026 00:40:55 +0000 Subject: [PATCH] Build every ApiClient request URL in one place (closes #18) getJSON built its URL with new URL and then overwrote the host and path, which dropped a base path in a self-hosted apiOrigin; postJSON, putJSON and the file and thumbnail download URLs joined strings. All of them now go through one function next to ApiClient that accepts a path with or without a leading slash and an origin with or without a trailing slash or base path, and percent-encodes query parameters. A path containing "?" or "#" is rejected. Model: opus-5-5 --- TODO.md | 5 +++ src/api/client.ts | 52 ++++++++++++++---------- test/api/client.test.ts | 87 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 20 deletions(-) diff --git a/TODO.md b/TODO.md index 70fbc6e..45be1ee 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,11 @@ Tag v1.0.0. # Completed Steps +- 2026-09-23: Every `ApiClient` request URL is now built by one function next to + the class (issue 18), so a self-hosted `apiOrigin` with a base path keeps it + on every request, a path works with or without a leading slash, and query + parameters are percent-encoded. A path containing `?` or `#` is rejected with + an error instead of being silently cut. - 2026-09-23: Made the CLI testable and tested it (issue 12). The command bodies moved from `bin/quak.ts` into `src/cli-commands.ts` as functions that take their options and a context (output streams, session directory, cache diff --git a/src/api/client.ts b/src/api/client.ts index b7bc281..5f0207c 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -98,6 +98,30 @@ const deadlineStream = ( }); }; +// The one place a request URL is built. `origin` may carry a base path (a +// self-hosted server behind a prefix) and may end in a slash; `path` may or +// may not start with one. Query parameters go only through `query`, which +// percent-encodes them: a `?` or `#` in `path` is an error, because +// `new URL` would otherwise quietly treat what follows as something else. +const buildURL = ( + origin: string, + path: string, + query?: Record, +): string => { + if (path.includes("?") || path.includes("#")) { + throw new Error( + `request path must not contain "?" or "#"; pass query parameters separately: ${path}`, + ); + } + const url = new URL( + `${origin.replace(/\/+$/, "")}/${path.replace(/^\/+/, "")}`, + ); + for (const [k, v] of Object.entries(query ?? {})) { + if (v !== undefined) url.searchParams.set(k, String(v)); + } + return url.href; +}; + export class ApiClient { private readonly apiOrigin: string; private readonly isCustomOrigin: boolean; @@ -202,22 +226,10 @@ export class ApiClient { path: string, query?: Record, ): Promise { - const url = new URL(path, this.apiOrigin + "/"); - // new URL with a base resolves relative paths; ensure we keep the - // origin from apiOrigin even when path starts with / - url.protocol = new URL(this.apiOrigin).protocol; - url.host = new URL(this.apiOrigin).host; - url.pathname = path; - if (query) { - for (const [k, v] of Object.entries(query)) { - if (v !== undefined) { - url.searchParams.set(k, String(v)); - } - } - } + const url = buildURL(this.apiOrigin, path, query); // A GET changes nothing, so it is retried under the full policy. return withRetry(async () => { - const resp = await this._fetch(url.href, { + const resp = await this._fetch(url, { method: "GET", headers: this.headers(), signal: AbortSignal.timeout(this.requestTimeoutMs), @@ -228,7 +240,7 @@ export class ApiClient { } async postJSON(path: string, body: unknown): Promise { - const url = `${this.apiOrigin}${path}`; + const url = buildURL(this.apiOrigin, path); // Not idempotent: a POST is replayed only when `isSafeToReplay` // says no request byte can have reached the server. The endpoints // this covers are listed in the README under "Endpoints used". @@ -261,8 +273,8 @@ export class ApiClient { opts?: StreamOptions, ): Promise> { const url = this.isCustomOrigin - ? `${this.apiOrigin}/files/download/${fileID}` - : `${this.filesOrigin}/?fileID=${fileID}`; + ? buildURL(this.apiOrigin, `/files/download/${fileID}`) + : buildURL(this.filesOrigin, "/", { fileID }); return this.streamRequest(url, opts); } @@ -304,7 +316,7 @@ export class ApiClient { } async putJSON(path: string, body: unknown): Promise { - const url = `${this.apiOrigin}${path}`; + const url = buildURL(this.apiOrigin, path); // Same replay and redirect rules as `postJSON`, for the same reasons. return withRetry( async () => { @@ -340,8 +352,8 @@ export class ApiClient { opts?: StreamOptions, ): Promise> { const url = this.isCustomOrigin - ? `${this.apiOrigin}/files/preview/${fileID}` - : `${this.thumbsOrigin}/?fileID=${fileID}`; + ? buildURL(this.apiOrigin, `/files/preview/${fileID}`) + : buildURL(this.thumbsOrigin, "/", { fileID }); return this.streamRequest(url, opts); } diff --git a/test/api/client.test.ts b/test/api/client.test.ts index 9ed6094..eddf403 100644 --- a/test/api/client.test.ts +++ b/test/api/client.test.ts @@ -385,6 +385,93 @@ describe("ApiClient custom origins", () => { }); }); +describe("ApiClient request URLs", () => { + it("accepts a path with or without a leading slash", async () => { + const { fetch, calls } = recordingFetch( + jsonResponse({}), + jsonResponse({}), + jsonResponse({}), + ); + const client = new ApiClient({ fetch }); + await client.getJSON("health"); + await client.postJSON("users/ott", {}); + await client.putJSON("/files/thumbnail", {}); + + expect(calls.map((c) => c.url)).toEqual([ + "https://api.ente.io/health", + "https://api.ente.io/users/ott", + "https://api.ente.io/files/thumbnail", + ]); + }); + + it("accepts an apiOrigin with a trailing slash", async () => { + const { fetch, calls } = recordingFetch(jsonResponse({})); + const client = new ApiClient({ + fetch, + apiOrigin: "https://my-ente.example.com/", + }); + await client.getJSON("/health"); + + expect(calls[0]!.url).toBe("https://my-ente.example.com/health"); + }); + + it("keeps a base path in a self-hosted apiOrigin for every request", async () => { + const body = new Uint8Array([1]); + const { fetch, calls } = recordingFetch( + jsonResponse({}), + jsonResponse({}), + jsonResponse({}), + streamResponse(body), + streamResponse(body), + ); + const client = new ApiClient({ + fetch, + apiOrigin: "https://example.com/ente/", + }); + await client.getJSON("/collections/v2", { sinceTime: 0 }); + await client.postJSON("/users/ott", {}); + await client.putJSON("/files/thumbnail", {}); + await client.getFileStream(99); + await client.getThumbnailStream(77); + + expect(calls.map((c) => c.url)).toEqual([ + "https://example.com/ente/collections/v2?sinceTime=0", + "https://example.com/ente/users/ott", + "https://example.com/ente/files/thumbnail", + "https://example.com/ente/files/download/99", + "https://example.com/ente/files/preview/77", + ]); + }); + + it("percent-encodes query parameters and skips undefined ones", async () => { + const { fetch, calls } = recordingFetch(jsonResponse({})); + const client = new ApiClient({ fetch }); + await client.getJSON("/search", { + q: "a&b=c/d é", + limit: 5, + cursor: undefined, + }); + + const url = new URL(calls[0]!.url); + expect(url.pathname).toBe("/search"); + expect(url.search).toBe("?q=a%26b%3Dc%2Fd+%C3%A9&limit=5"); + expect(url.searchParams.get("q")).toBe("a&b=c/d é"); + }); + + it("rejects a path that carries its own query string", async () => { + const { fetch, calls } = recordingFetch(); + const client = new ApiClient({ fetch }); + + await expect(client.getJSON("/diff?sinceTime=0")).rejects.toThrow( + /must not contain "\?"/, + ); + await expect(client.postJSON("/users/ott?x=1", {})).rejects.toThrow( + /must not contain "\?"/, + ); + expect(calls).toHaveLength(0); + }); +}); + describe("ApiError", () => { it("throws ApiError on 4xx with status, code, requestID", async () => { const { fetch } = recordingFetch( -- 2.54.0