From f010bc617b6aceef4c95a58ccbb20ea18f87a23b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 19:42:04 +0000 Subject: [PATCH 1/6] Fix per-link analytics crashing past D1's 100-bound-parameter cap getStats, getTimeline, getLinkBreakdown, getLinkBreakdownPage, and compareLinkStats each fetched every slug on a link and bound the whole list into a slug IN (?,?,...) clause. D1 caps a prepared statement at 100 bound parameters, so a link with more than ~99 custom slugs (nothing prevents adding that many) failed every one of its analytics queries with SQLITE_ERROR: too many SQL variables. 0.37.1 fixed the identical defect for bundle-scoped queries via bundleSlugScope(), a subquery that resolves membership inside SQLite and costs exactly one bound parameter regardless of member count. This applies the same technique per link (linkSlugScope()) to the five methods above, which had been missed in that pass. Adds a describe block mirroring the existing bundle-cap regression tests: a link with 101 slugs exercises all five methods and asserts correct aggregation instead of a thrown SQLITE_ERROR. --- .../repository/click-repository.test.ts | 84 +++++++++++++++++++ src/db/click-repository.ts | 51 ++++++----- 2 files changed, 113 insertions(+), 22 deletions(-) diff --git a/src/__tests__/repository/click-repository.test.ts b/src/__tests__/repository/click-repository.test.ts index cf57ba86..af128563 100644 --- a/src/__tests__/repository/click-repository.test.ts +++ b/src/__tests__/repository/click-repository.test.ts @@ -657,6 +657,90 @@ describe("ClickRepository bundle analytics past the D1 bind cap", () => { }); }); +// The same D1 bound-parameter cap applies per link: a link's own slug count +// (not just a bundle's member slugs) can cross 100 once enough custom slugs +// are added, and every per-link analytics query used to bind one parameter +// per slug fetched for that link. +describe("ClickRepository per-link analytics past the D1 bind cap", () => { + const SLUGS = 101; // 1 auto slug + 100 custom => 101 slugs, past D1's 100-parameter cap + + async function wideLink() { + const link = await LinkRepository.create(env.DB, { url: "https://a.com/wide", slug: "wide-0", createdBy: "a@b" }); + const slugs = ["wide-0"]; + for (let i = 1; i < SLUGS; i++) { + await SlugRepository.addCustom(env.DB, link.id, `wide-${i}`); + slugs.push(`wide-${i}`); + } + return { link, slugs }; + } + + it("getStats aggregates a link whose slug count exceeds the cap", async () => { + const { link, slugs } = await wideLink(); + expect(slugs.length).toBeGreaterThan(100); + + const now = Math.floor(Date.now() / 1000); + for (const slug of slugs) { + await recordClick(slug, now - 60, { country: "US" }); + } + await recordClick(slugs[0], now - 60, { country: "ID" }); + + const stats = await ClickRepository.getStats(env.DB, link.id); + expect(stats.total_clicks).toBe(SLUGS + 1); + expect(stats.countries.find((c) => c.name === "US")?.count).toBe(SLUGS); + expect(stats.slug_clicks).toHaveLength(SLUGS); + }); + + it("getTimeline aggregates a link whose slug count exceeds the cap", async () => { + const { link, slugs } = await wideLink(); + const now = Math.floor(Date.now() / 1000); + for (const slug of slugs) { + await recordClick(slug, now - 60); + } + + const timeline = await ClickRepository.getTimeline(env.DB, link.id, "30d", now); + expect(timeline.summary.last_30d).toBe(SLUGS); + expect(timeline.buckets.some((b) => b.count > 0)).toBe(true); + }); + + it("getLinkBreakdown aggregates a link whose slug count exceeds the cap", async () => { + const { link, slugs } = await wideLink(); + const now = Math.floor(Date.now() / 1000); + for (const slug of slugs) { + await recordClick(slug, now - 60, { country: "US" }); + } + + const breakdown = await ClickRepository.getLinkBreakdown(env.DB, link.id, "country", "30d", 10); + expect(breakdown).toEqual([{ name: "US", count: SLUGS }]); + }); + + it("getLinkBreakdownPage pages a link whose slug count exceeds the cap", async () => { + const { link, slugs } = await wideLink(); + const now = Math.floor(Date.now() / 1000); + await recordClick(slugs[0], now - 60, { country: "US" }); + await recordClick(slugs[1], now - 60, { country: "US" }); + await recordClick(slugs[2], now - 60, { country: "ID" }); + + const page = await ClickRepository.getLinkBreakdownPage(env.DB, link.id, "countries", "30d", 0, 10); + expect(page.total).toBe(2); + expect(page.items).toEqual([ + { name: "US", count: 2 }, + { name: "ID", count: 1 }, + ]); + }); + + it("compareLinkStats aggregates a link whose slug count exceeds the cap", async () => { + const { link, slugs } = await wideLink(); + const now = Math.floor(Date.now() / 1000); + for (const slug of slugs) { + await recordClick(slug, now - 60, { country: "US" }); + } + + const result = await ClickRepository.compareLinkStats(env.DB, link.id, "30d"); + expect(result.total_clicks).toBe(SLUGS); + expect(result.top_country).toBe("US"); + }); +}); + describe("ClickRepository.getBundleSummariesBulk", () => { it("returns empty map when called with no bundles", async () => { const res = await ClickRepository.getBundleSummariesBulk(env.DB, []); diff --git a/src/db/click-repository.ts b/src/db/click-repository.ts index c7f7277e..5983ed21 100644 --- a/src/db/click-repository.ts +++ b/src/db/click-repository.ts @@ -72,6 +72,18 @@ function bundleSlugScope(column = "slug"): string { return `${column} IN (SELECT s_.slug FROM bundle_links bl_ JOIN slugs s_ ON s_.link_id = bl_.link_id WHERE bl_.bundle_id = ?)`; } +/** + * WHERE fragment matching every click on a slug that belongs to a link, + * costing exactly one bound parameter (the link id). Same rationale as + * `bundleSlugScope`: a naive `slug IN (?,?,...)` list built from every slug + * on the link blows D1's 100-bound-parameter cap once a link accumulates + * more than ~99 custom slugs, and fails the whole per-link analytics query + * with SQLITE_ERROR. + */ +function linkSlugScope(column = "slug"): string { + return `${column} IN (SELECT slug FROM slugs WHERE link_id = ?)`; +} + export type { ClickFilters } from "./filters"; export class ClickRepository { @@ -129,9 +141,8 @@ export class ClickRepository { }; if (slugs.length === 0) return empty; - const placeholders = slugs.map(() => "?").join(","); - let where = `slug IN (${placeholders})`; - const binds: (string | number)[] = [...slugs]; + let where = linkSlugScope(); + const binds: (string | number)[] = [linkId]; if (range && range !== "all") { const now = Math.floor(Date.now() / 1000); @@ -211,9 +222,8 @@ export class ClickRepository { }; if (slugs.length === 0) return empty; - const placeholders = slugs.map(() => "?").join(","); const filterFrag = clickFilterSql(filters); - const where = `slug IN (${placeholders})${filterFrag}`; + const where = `${linkSlugScope()}${filterFrag}`; // Summary counts const t24h = ts - 86400; @@ -222,11 +232,11 @@ export class ClickRepository { const t90d = ts - 90 * 86400; const t1y = ts - 365 * 86400; const [last24h, last7d, last30d, last90d, last1y] = await Promise.all([ - db.prepare(`SELECT COUNT(*) as cnt FROM clicks WHERE ${where} AND clicked_at >= ?`).bind(...slugs, t24h).first<{ cnt: number }>(), - db.prepare(`SELECT COUNT(*) as cnt FROM clicks WHERE ${where} AND clicked_at >= ?`).bind(...slugs, t7d).first<{ cnt: number }>(), - db.prepare(`SELECT COUNT(*) as cnt FROM clicks WHERE ${where} AND clicked_at >= ?`).bind(...slugs, t30d).first<{ cnt: number }>(), - db.prepare(`SELECT COUNT(*) as cnt FROM clicks WHERE ${where} AND clicked_at >= ?`).bind(...slugs, t90d).first<{ cnt: number }>(), - db.prepare(`SELECT COUNT(*) as cnt FROM clicks WHERE ${where} AND clicked_at >= ?`).bind(...slugs, t1y).first<{ cnt: number }>(), + db.prepare(`SELECT COUNT(*) as cnt FROM clicks WHERE ${where} AND clicked_at >= ?`).bind(linkId, t24h).first<{ cnt: number }>(), + db.prepare(`SELECT COUNT(*) as cnt FROM clicks WHERE ${where} AND clicked_at >= ?`).bind(linkId, t7d).first<{ cnt: number }>(), + db.prepare(`SELECT COUNT(*) as cnt FROM clicks WHERE ${where} AND clicked_at >= ?`).bind(linkId, t30d).first<{ cnt: number }>(), + db.prepare(`SELECT COUNT(*) as cnt FROM clicks WHERE ${where} AND clicked_at >= ?`).bind(linkId, t90d).first<{ cnt: number }>(), + db.prepare(`SELECT COUNT(*) as cnt FROM clicks WHERE ${where} AND clicked_at >= ?`).bind(linkId, t1y).first<{ cnt: number }>(), ]); const summary = { @@ -270,7 +280,7 @@ export class ClickRepository { // Pick granularity based on actual data span const earliestRow = await db .prepare(`SELECT MIN(clicked_at) as t FROM clicks WHERE ${where}`) - .bind(...slugs) + .bind(linkId) .first<{ t: number | null }>(); allEarliest = earliestRow?.t ?? ts; const spanDays = Math.max(1, Math.floor((ts - allEarliest) / 86400)); @@ -290,7 +300,7 @@ export class ClickRepository { } const timeFilter = sinceTs !== null ? ` AND clicked_at >= ?` : ""; - const binds = sinceTs !== null ? [...slugs, sinceTs] : [...slugs]; + const binds = sinceTs !== null ? [linkId, sinceTs] : [linkId]; const rows = await db .prepare( @@ -451,9 +461,8 @@ export class ClickRepository { const slugs = (slugRows.results ?? []).map((r) => r.slug); if (slugs.length === 0) return []; - const placeholders = slugs.map(() => "?").join(","); - let where = `slug IN (${placeholders}) AND ${dimension} IS NOT NULL`; - const binds: (string | number)[] = [...slugs]; + let where = `${linkSlugScope()} AND ${dimension} IS NOT NULL`; + const binds: (string | number)[] = [linkId]; if (range && range !== "all") { const now = Math.floor(Date.now() / 1000); @@ -502,9 +511,8 @@ export class ClickRepository { const slugs = (slugRows.results ?? []).map((r) => r.slug); if (slugs.length === 0) return { items: [], total: 0 }; - const placeholders = slugs.map(() => "?").join(","); - let where = `slug IN (${placeholders})`; - const binds: (string | number)[] = [...slugs]; + let where = linkSlugScope(); + const binds: (string | number)[] = [linkId]; if (range && range !== "all") { where += " AND clicked_at >= ?"; binds.push(Math.floor(Date.now() / 1000) - (RANGE_SECONDS[range] ?? 0)); @@ -552,9 +560,8 @@ export class ClickRepository { return { total_clicks: 0, top_country: null, top_referrer: null }; } - const placeholders = slugs.map(() => "?").join(","); - let where = `slug IN (${placeholders})`; - const binds: (string | number)[] = [...slugs]; + let where = linkSlugScope(); + const binds: (string | number)[] = [linkId]; if (range && range !== "all") { const now = Math.floor(Date.now() / 1000); @@ -669,7 +676,7 @@ export class ClickRepository { /** * Returns a fixed-size counts series for the selected range — used to render * sparklines on KPI cards. Buckets are daily for ranges >= 7d, hourly for 24h, - * weekly for 1y, and monthly for all. + * and monthly (12 points) for 1y and all. */ static async getSparkline( db: D1Database, From 27ef8437ee2c2be3fece2ca5609328b2fcaa3048 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 19:42:11 +0000 Subject: [PATCH 2/6] Toast the remaining API failures that carry no JSON body quickShorten, createLink, and createDuplicate parsed the failure body's error field with no .catch() fallback, so a non-JSON error response (an edge-level 502/524 HTML page, a bare 500) rejected the promise unhandled and left the button silent with nothing shown to the user. 0.37.1 added this same fallback to fourteen other handlers but missed these three, the two most common actions in the app (shortening and creating a link). The regression-guard test that is supposed to catch a handler missing this fallback had a blind spot: it matched res.json().then( and toast( on the same source line, but these three handlers spread the call across multiple lines in the project's usual style, so the guard could never see both tokens together. Replaced it with a parser that walks the balanced parentheses of the .then(...) call to find the whole thing regardless of line wrapping, and narrowed it to blocks that read a .error field so it doesn't also flag success-path toasts. Added the three handlers to the existing per-handler empty-body/HTML-body/error-message test matrix. --- .../unit/client-api-error-toasts.test.ts | 74 +++++++++++++++---- src/client.ts | 6 +- 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/src/__tests__/unit/client-api-error-toasts.test.ts b/src/__tests__/unit/client-api-error-toasts.test.ts index 389ae175..66b7341f 100644 --- a/src/__tests__/unit/client-api-error-toasts.test.ts +++ b/src/__tests__/unit/client-api-error-toasts.test.ts @@ -12,6 +12,50 @@ import { describe, expect, it } from "vitest"; import { adminClientScript } from "../../client"; import type { Translations } from "../../i18n/types"; +// Finds every `res.json().then(...)` call that reports the parsed body's +// `error` field but has no `.catch(...)` immediately after it. Every such +// block in this file follows the same shape: `toast(data.error || t(...), +// 'error')` (or `body.error`) — the `.error` access is what marks a block as +// reporting the API's failure body, as opposed to a success-path `.then(...)` +// that also happens to call toast() (e.g. "link created"). +// +// Regression: an earlier version of this guard matched line by line +// (`/res\.json\(\)\.then\(/` and `/toast\(/` on the *same* line, with no +// `.catch(` on that line). quickShorten, createLink, and createDuplicate all +// spread the `.then(function(data) { ... })` call across three lines in the +// project's usual multi-line style, so the line-based check could never see +// `res.json().then(` and `toast(` together and missed all three being +// unguarded. This walks the balanced parentheses of the `.then(...)` call +// instead, so it sees the whole call regardless of how it's wrapped. +function findUnguardedJsonThenToast(script: string): string[] { + const unguarded: string[] = []; + const callStart = /res\.json\(\)\.then\(/g; + let match: RegExpExecArray | null; + while ((match = callStart.exec(script))) { + const openParenIdx = match.index + match[0].length - 1; + let depth = 0; + let closeParenIdx = -1; + for (let i = openParenIdx; i < script.length; i++) { + if (script[i] === "(") depth++; + else if (script[i] === ")") { + depth--; + if (depth === 0) { + closeParenIdx = i; + break; + } + } + } + if (closeParenIdx === -1) continue; + const block = script.slice(match.index, closeParenIdx + 1); + if (!/toast\(/.test(block) || !/\.error\b/.test(block)) continue; + const after = script.slice(closeParenIdx + 1, closeParenIdx + 20); + if (!/^\s*\.catch\(/.test(after)) { + unguarded.push(block.split("\n")[0]); + } + } + return unguarded; +} + function extractTopLevelChunk(source: string, startPattern: RegExp): string { const lines = source.split("\n"); const startIdx = lines.findIndex((l) => startPattern.test(l)); @@ -43,15 +87,25 @@ const HANDLERS: Array<{ name: string; invoke: (h: Handlers) => void }> = [ { name: "doCreateBundle", invoke: (h) => h.doCreateBundle() }, { name: "doUpdateBundle", invoke: (h) => h.doUpdateBundle(1) }, { name: "doAddLinkToBundle", invoke: (h) => h.doAddLinkToBundle(1, 2) }, + { name: "quickShorten", invoke: (h) => h.quickShorten() }, + { name: "createLink", invoke: (h) => h.createLink() }, + { name: "createDuplicate", invoke: (h) => h.createDuplicate("https://example.com") }, ]; // Enough of a DOM for the handlers that read form fields before calling the // API. Every field reads as non-empty so none of them bail out early. +// quickShorten gates on isUrl() before ever calling the API, so its URL +// field needs a real http(s) value; every other field just needs to be +// non-empty. function fakeDocument() { - const field = { value: "x", focus() {}, style: {} }; + const urlIds = new Set(["quick-url", "m-url"]); return { - getElementById: () => field, - querySelector: () => field, + getElementById: (id: string) => ({ + value: urlIds.has(id) ? "https://example.com" : "x", + focus() {}, + style: {}, + }), + querySelector: () => ({ value: "x", focus() {}, style: {} }), querySelectorAll: () => [], }; } @@ -61,6 +115,9 @@ function fakeDocument() { function loadHandlers(json: () => Promise) { const script = adminClientScript("1.0.0", {} as unknown as Translations); const code = [ + // quickShorten calls the standalone isUrl() helper before it ever + // reaches the API, so that helper has to be in scope too. + extractTopLevelChunk(script, /^function isUrl\(/), ...HANDLERS.map((h) => extractTopLevelChunk(script, new RegExp(`^function ${h.name}\\(`)), ), @@ -114,16 +171,7 @@ describe("admin action error toasts", () => { it("leaves no failure path parsing a JSON error body without a fallback", () => { const script = adminClientScript("1.0.0", {} as unknown as Translations); - const unguarded = script - .split("\n") - .filter( - (line) => - /res\.json\(\)\.then\(/.test(line) && - /toast\(/.test(line) && - !/\.catch\(/.test(line), - ); - - expect(unguarded).toEqual([]); + expect(findUnguardedJsonThenToast(script)).toEqual([]); }); for (const { name, invoke } of HANDLERS) { diff --git a/src/client.ts b/src/client.ts index 070a4b7d..6f0d9d4a 100644 --- a/src/client.ts +++ b/src/client.ts @@ -189,7 +189,7 @@ function quickShorten() { } else { return res.json().then(function(data) { toast(data.error || t('client.createLinkError'), 'error'); - }); + }).catch(function() { toast(t('client.createLinkError'), 'error'); }); } }); } @@ -267,7 +267,7 @@ function createLink() { } else { return res.json().then(function(data) { toast(data.error || t('client.createLinkError'), 'error'); - }); + }).catch(function() { toast(t('client.createLinkError'), 'error'); }); } }); } @@ -285,7 +285,7 @@ function createDuplicate(url) { } else { return res.json().then(function(data) { toast(data.error || t('client.createLinkError'), 'error'); - }); + }).catch(function() { toast(t('client.createLinkError'), 'error'); }); } }); } From ba2dfb3ff3737902121486fdb8298f443fd437a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 19:42:15 +0000 Subject: [PATCH 3/6] Fix add_link_to_bundle's description claiming an ownership check it doesn't enforce MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tool description said "Only the bundle owner can add," but addLinkToBundle() deliberately skips the ownership gate every other bundle-mutation function enforces — adding a link to a bundle is open to any authenticated caller by design (see the comment in bundle-management.ts and the existing 'any authenticated caller can add a link to any bundle' test). An agent trusting the description could believe the call is caller-scoped and add another user's link to a bundle it doesn't own, or reason incorrectly about the security model. Same class of doc/behavior mismatch as the list_bundles fix in 0.37.0. Documentation correction only, no behavior change. Adds a regression test mirroring the existing list_bundles description test. --- src/__tests__/handler/mcp.test.ts | 30 ++++++++++++++++++++++++++++++ src/mcp/server.ts | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/__tests__/handler/mcp.test.ts b/src/__tests__/handler/mcp.test.ts index bdf64385..93c80266 100644 --- a/src/__tests__/handler/mcp.test.ts +++ b/src/__tests__/handler/mcp.test.ts @@ -715,6 +715,36 @@ describe("MCP tool descriptions", () => { await client.close(); } }); + + // Regression: add_link_to_bundle's description claimed "only the bundle + // owner can add", but addLinkToBundle() deliberately skips the ownership + // gate every other bundle-mutation function enforces (see + // bundle-management.ts and the "any authenticated caller can add a link to + // any bundle" test in bundle-service.test.ts). A tool description that + // promises a check that does not exist could lead an agent to add a link + // it does not own, believing the call would be rejected if it were unsafe. + it("add_link_to_bundle does not claim an ownership check it does not enforce", async () => { + const agent = Object.create(ShrtnrMCP.prototype) as ShrtnrMCP; + agent.server = new McpServer({ name: "shrtnr", version: "test" }); + await agent.init(); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "test-client", version: "test" }); + await Promise.all([ + client.connect(clientTransport), + agent.server.connect(serverTransport), + ]); + + try { + const { tools } = await client.listTools(); + const addLinkToBundle = tools.find((t) => t.name === "add_link_to_bundle"); + expect(addLinkToBundle).toBeDefined(); + expect(addLinkToBundle?.description).not.toMatch(/only the bundle owner can add/i); + expect(addLinkToBundle?.description).toMatch(/any authenticated caller/i); + } finally { + await client.close(); + } + }); }); // ---- create_link trailing-slash normalization ---- diff --git a/src/mcp/server.ts b/src/mcp/server.ts index f3e83689..fc0607aa 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -873,7 +873,7 @@ export class ShrtnrMCP extends McpAgent, Props> { { title: "Add link to bundle", description: - "Add a link to a bundle. Idempotent: adding the same link twice is a no-op. Only the bundle owner can add.", + "Add a link to a bundle. Idempotent: adding the same link twice is a no-op. Open to any authenticated caller: adding does not require owning the bundle or the link.", inputSchema: { bundle_id: z.number().int().positive(), link_id: z.number().int().positive(), From c0fca071a672856bd7d7054a0d09d9ba9eb83b49 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 19:42:23 +0000 Subject: [PATCH 4/6] TS SDK: fix keysToSnake/keysToCamel silently dropping a __proto__ key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both functions built their output on a plain {} target and assigned converted keys through bracket notation: out[key] = value. When key is the literal string "__proto__", that assignment doesn't create an own property on a plain object — it reassigns the object's prototype, because {} inherits the __proto__ accessor from Object.prototype. A request body assembled from parsed/untrusted JSON (which, unlike object-literal syntax, can carry __proto__ as a real own enumerable property) silently lost that key with no error, e.g. client.links.update(id, JSON.parse(patch)). Switched both to Object.create(null) targets, which have no such accessor to intercept the assignment. Added a focused unit test for keysToSnake (the exploitable direction: outbound request bodies) and a client-level integration test showing a __proto__ key now survives onto the wire. keysToCamel doesn't have a matching direct repro since its camelCase transform never maps a source key to the literal string "__proto__", but carries the same fix for symmetry and defense in depth. --- sdk/typescript/src/internal/case.ts | 11 +++++++-- sdk/typescript/tests/case.test.ts | 36 +++++++++++++++++++++++++++++ sdk/typescript/tests/client.test.ts | 16 +++++++++++++ 3 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 sdk/typescript/tests/case.test.ts diff --git a/sdk/typescript/src/internal/case.ts b/sdk/typescript/src/internal/case.ts index f231024c..d3add50f 100644 --- a/sdk/typescript/src/internal/case.ts +++ b/sdk/typescript/src/internal/case.ts @@ -27,7 +27,12 @@ export function keysToCamel(value: unknown): unknown { return value.map(keysToCamel); } if (isPlainObject(value)) { - const out: Record = {}; + // Object.create(null) rather than `{}`: assigning through `out["__proto__"] + // = v` on a `{}` (which inherits Object.prototype's `__proto__` accessor) + // sets the object's prototype instead of creating an own property, so a + // source key literally named `__proto__` silently vanished from the + // output. A null-prototype target has no such accessor to intercept it. + const out: Record = Object.create(null); for (const [k, v] of Object.entries(value)) { out[toCamel(k)] = keysToCamel(v); } @@ -42,7 +47,9 @@ export function keysToSnake(value: unknown): unknown { return value.map(keysToSnake); } if (isPlainObject(value)) { - const out: Record = {}; + // See keysToCamel: a null-prototype target avoids silently dropping a + // `__proto__` key. + const out: Record = Object.create(null); for (const [k, v] of Object.entries(value)) { out[toSnake(k)] = keysToSnake(v); } diff --git a/sdk/typescript/tests/case.test.ts b/sdk/typescript/tests/case.test.ts new file mode 100644 index 00000000..e286d6b1 --- /dev/null +++ b/sdk/typescript/tests/case.test.ts @@ -0,0 +1,36 @@ +// Copyright 2026 Oddbit (https://oddbit.id) +// SPDX-License-Identifier: Apache-2.0 + +import { describe, it, expect } from "vitest"; +import { keysToSnake } from "../src/internal/case"; + +// Regression: assigning through `out["__proto__"] = v` on a plain `{}` +// target doesn't create an own property — it reassigns the object's +// prototype, because `{}` inherits the `__proto__` accessor from +// Object.prototype. A source object built from JSON.parse (unlike an +// object literal) can carry `__proto__` as a real own enumerable +// property, so a request body assembled from untrusted/dynamic JSON with +// that key silently lost it with no error. Object.create(null) has no +// such accessor to intercept the assignment. +// +// keysToCamel does not need the equivalent case here: its camelCase +// transform never maps a source key to the literal string "__proto__" +// (toCamel("__proto__") produces "_Proto__"), so the wire-response path +// cannot collide with the setter. It still uses the same null-prototype +// target as keysToSnake for symmetry and defense in depth. +describe("keysToSnake: __proto__ key handling", () => { + it("preserves a literal __proto__ own property instead of silently dropping it", () => { + const input = JSON.parse('{"url":"https://good.example.com","__proto__":{"label":"x"}}') as Record< + string, + unknown + >; + expect(Object.keys(input)).toContain("__proto__"); + + const out = keysToSnake(input) as Record; + expect(Object.keys(out)).toContain("__proto__"); + expect(out["__proto__"]).toEqual({ label: "x" }); + expect(out["url"]).toBe("https://good.example.com"); + // The conversion must not have polluted the *actual* prototype chain. + expect(Object.getPrototypeOf(out)).not.toBe(input["__proto__"]); + }); +}); diff --git a/sdk/typescript/tests/client.test.ts b/sdk/typescript/tests/client.test.ts index 75164da3..68b3958a 100644 --- a/sdk/typescript/tests/client.test.ts +++ b/sdk/typescript/tests/client.test.ts @@ -4,6 +4,7 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; import { ShrtnrClient } from "../src/index"; import { ShrtnrError } from "../src/index"; +import type { UpdateLinkBody } from "../src/index"; const BASE = "https://shrtnr.test"; const API_KEY = "sk_abc"; @@ -376,6 +377,21 @@ describe("Case transformation", () => { const body = JSON.parse(init.body as string) as Record; expect(body["expires_at"]).toBe(expiresAt.toISOString()); }); + + it("does not drop a __proto__ key from a request body built from parsed JSON", async () => { + mockFetch(200, { id: 1, url: "https://example.com", label: null, created_at: 1000, expires_at: null, created_via: null, created_by: "u", total_clicks: 0, slugs: [] }); + // JSON.parse (unlike object-literal syntax) creates "__proto__" as a real + // own property, simulating a caller that forwards an untrusted/dynamic + // patch body. That key used to vanish from the outgoing request with no + // error because `out["__proto__"] = v` on a plain object reassigns its + // prototype instead of adding an own property. + const patch = JSON.parse('{"label":"new","__proto__":{"injected":true}}') as Record; + await client().links.update(1, patch as unknown as UpdateLinkBody); + const { init } = lastCall(); + const body = JSON.parse(init.body as string) as Record; + expect(body["label"]).toBe("new"); + expect(body["__proto__"]).toEqual({ injected: true }); + }); }); // ============================================================ From 3ec744447fe02f88d47d29581ee8f15bf2a66892 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 19:42:28 +0000 Subject: [PATCH 5/6] Python SDK: raise ShrtnrError on a JSON null 2xx body instead of crashing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A non-204 2xx response whose body is the literal null (4 bytes) is non-empty and valid JSON, so it passed both existing guards in parse_json_response and returned None. Every single-object resource method's SomeModel.from_dict(...) then crashed on it with a bare AttributeError instead of the documented ShrtnrError — the same failure mode the 1.1.2 empty-body fix exists to prevent, just reached from a non-empty body that happens to parse to None. parse_json_response now raises ShrtnrError when the parsed body is None. Scoped to exactly that value (not every non-dict shape) so list()'s legitimate JSON-array responses are unaffected. Adds a regression test mirroring the existing empty-body test. --- sdk/python/src/shrtnr/_base.py | 13 ++++++++++++- sdk/python/tests/test_client.py | 14 ++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/sdk/python/src/shrtnr/_base.py b/sdk/python/src/shrtnr/_base.py index c779b95b..339f7ef8 100644 --- a/sdk/python/src/shrtnr/_base.py +++ b/sdk/python/src/shrtnr/_base.py @@ -80,9 +80,20 @@ def parse_json_response(response: httpx.Response) -> Any: if not response.content: raise ShrtnrError(response.status_code, "Empty response body") try: - return response.json() + parsed = response.json() except Exception as exc: raise ShrtnrError(response.status_code, f"Invalid JSON response: {exc}") from exc + # A body that is valid JSON but is the literal `null` (4 bytes, so it + # passes the empty-body check above, and valid JSON, so it passes the + # parse above) used to reach here as a bare `None`. Every single-object + # resource method's `SomeModel.from_dict(...)` expects a dict and crashed + # on it with a bare AttributeError instead of the documented ShrtnrError — + # the same failure mode the empty-body check exists to prevent, just + # reached from a non-empty body. Arrays are left alone: list() endpoints + # legitimately parse to a JSON array, not a dict. + if parsed is None: + raise ShrtnrError(response.status_code, "Response body is JSON null") + return parsed def parse_text_response(response: httpx.Response) -> str: diff --git a/sdk/python/tests/test_client.py b/sdk/python/tests/test_client.py index 4a74b004..a5fede9c 100644 --- a/sdk/python/tests/test_client.py +++ b/sdk/python/tests/test_client.py @@ -748,6 +748,20 @@ def test_empty_body_2xx_raises_shrtnr_error(client: Shrtnr) -> None: assert exc_info.value.status == 200 +@respx.mock +def test_null_body_2xx_raises_shrtnr_error(client: Shrtnr) -> None: + """A non-204 2xx response whose body is the JSON literal `null` must also + raise ShrtnrError. content is non-empty (4 bytes) and valid JSON, so it + slips past both the empty-body check and the JSON-parse check above, and + used to reach `SomeModel.from_dict(None)` as a bare AttributeError.""" + respx.delete(f"{BASE_URL}/_/api/links/5").mock( + return_value=httpx.Response(200, content=b"null", headers={"content-type": "application/json"}), + ) + with pytest.raises(ShrtnrError) as exc_info: + client.links.delete(5) + assert exc_info.value.status == 200 + + # ---- links.qr: size accepts int ---- From 1216f571f530f1aba2832c4f302a6dc169d94536 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 19:42:32 +0000 Subject: [PATCH 6/6] Dart SDK: list BreakdownPage in the README's Key types BreakdownPage is the return type of both LinksResource.breakdown and BundlesResource.breakdown, shipped in 2.1.0 alongside the other two SDKs, but the Dart README's Key types list never picked it up. TypeScript and Python's READMEs already list BreakdownDimension, BreakdownPage together; this brings Dart's list into parity. --- sdk/dart/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/dart/README.md b/sdk/dart/README.md index 0586d046..aeeb7f36 100644 --- a/sdk/dart/README.md +++ b/sdk/dart/README.md @@ -130,7 +130,7 @@ Key types exported from `package:shrtnr/shrtnr.dart`: - `Link`, `Slug`, `Bundle`, `BundleWithSummary`, `BundleTopLink` - `ClickStats`, `TimelineData`, `TimelineBucket`, `TimelineSummary`, `NameCount` -- `DateCount`, `SlugCount` +- `DateCount`, `SlugCount`, `BreakdownPage` - `DeletedResult`, `AddedResult`, `RemovedResult` - Enums: `TimelineRange`, `BundleAccent`, `BreakdownDimension`, `BundleArchivedFilter`