Weekly review batch: D1 bind-cap crash, missing error toasts, SDK bugs

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`
 

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:

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 ----
 
 

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<string, unknown> = {};
+    // 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<string, unknown> = 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<string, unknown> = {};
+    // See keysToCamel: a null-prototype target avoids silently dropping a
+    // `__proto__` key.
+    const out: Record<string, unknown> = Object.create(null);
     for (const [k, v] of Object.entries(value)) {
       out[toSnake(k)] = keysToSnake(v);
     }

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<string, unknown>;
+    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__"]);
+  });
+});

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<string, unknown>;
     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<string, unknown>;
+    await client().links.update(1, patch as unknown as UpdateLinkBody);
+    const { init } = lastCall();
+    const body = JSON.parse(init.body as string) as Record<string, unknown>;
+    expect(body["label"]).toBe("new");
+    expect(body["__proto__"]).toEqual({ injected: true });
+  });
 });
 
 // ============================================================

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 ----

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, []);

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<unknown>) {
   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) {