Skip to content

Weekly defect review: bundle listing past D1 bind cap, all-or-nothing settings update - #75

Open
DennisAlund wants to merge 2 commits into
mainfrom
claude/adoring-dirac-x1ilai
Open

DennisAlund wants to merge 2 commits into
mainfrom
claude/adoring-dirac-x1ilai

Conversation

@DennisAlund

Copy link
Copy Markdown
Member

Weekly defect-hunting review. Two defects fixed, each with a regression test that fails before the fix and passes after. yarn test: 102 files, 1598 tests green. No change under src/pages/, src/admin/, src/client.ts, src/styles.ts or admin routes, so yarn e2e was not run.

Fixes

1. Bundle listing exceeds D1's 100 bound-parameter cap (905a871)

  • Wrong: listBundles and ClickRepository.getBundleSummariesBulk bound one parameter per bundle id in IN (...) lists.
  • Impact: with about 100 or more active bundles, the bundles page and GET /bundles fail with SQLITE_ERROR: too many SQL variables.
  • Fix: link counts group without an IN list; summaries run in slices of 50 ids and merge.
  • Test: bundle-service.test.ts, "lists and summarises more bundles than one statement can bind" (120 bundles, with and without a range).

2. Settings update applied partially on a rejected request (second commit)

  • Wrong: updateAppSettings validated and wrote one field at a time.
  • Impact: a request answered with 400 (for example an unknown theme) had already stored earlier fields such as slug_default_length or filter_bots.
  • Fix: validate all fields first, then write.
  • Test: admin-service.test.ts, "stores no field when a later field in the same request is invalid".

Spec hashes are unchanged (no spec or SDK surface change), and all three SDK hashes match ./scripts/spec-hash.sh.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GtFhVu3q2zrePMFkLtF5hs


Generated by Claude Code

claude added 2 commits October 2, 2026 19:15
listBundles and getBundleSummariesBulk bound one parameter per bundle id.
D1 rejects a statement past 100 parameters, so a deployment with roughly
100 or more active bundles got SQLITE_ERROR (too many SQL variables) on the
bundles page and GET /bundles. Link counts now group without an IN list and
the summaries run in slices of 50 ids, merged afterwards.

Regression test lists 120 bundles with and without a range.
updateAppSettings validated and wrote one field at a time, so a request
rejected with 400 on a later field (an unknown theme, a non-boolean filter)
had already stored the earlier ones, while telling the caller nothing was
saved. All fields are now validated first and written only when the whole
request is valid.

Regression test sends a valid slug length and filter with an invalid theme
and checks that neither is stored.

Copy link
Copy Markdown
Member Author

Flagged for developer judgment. None of these are changed in this PR.

Dependency drift

  • yarn audit --level moderate reports 99 findings (40 high, 49 moderate, 10 low). The ones I inspected sit in dev tooling: @cloudflare/vitest-pool-workers > miniflare > undici (patched in >=7.29.1). The fix is a tooling bump, not a drop-in patch, so I left it.
  • Behind latest within semver range: hono 4.13.5 (4.13.12), jose 6.2.10 (6.2.12), zod 4.5.1 (4.6.5), @hono/zod-openapi 1.6.1 (1.6.3), wrangler 4.127.1 (4.147.0). hono and jose are on the request path and worth a patch bump. I did not confirm whether any runtime advisory applies.
  • Major jumps available, not recommended without a plan: typescript 7, vitest 5, @modelcontextprotocol/sdk 1.32, agents 0.26.

Admin JSON handlers accept wrong field types and return 500
handleCreateBundle and handleUpdateBundle (src/api/bundles.ts) pass raw request.json() to the service. createBundle and updateBundle call .trim() on name, so {"name": 123} or a null body throws a TypeError, and non-string description or icon reach D1 unchecked. The public API validates through zod; only the admin path is affected, and only authenticated callers can reach it. The fix means picking a validation style for the raw-JSON handlers (zod versus hand checks, as validateLabel does for links), so I left the choice to you.

extractTitle decodes entities in a fixed order (src/title-fetch.ts)
&amp; is decoded before &lt; and &gt;, so &amp;lt; becomes < (a double decode). Numeric references go through String.fromCharCode, which mangles code points above U+FFFF. Labels render through JSX escaping, so I found no XSS path. The effect is a wrong label only. Fixing it changes visible label text, so it is your call.

hasScope does not trim (src/auth.ts)
scope.split(",") is exact. Stored scopes come from the fixed VALID_SCOPES list, so this is not reachable today. Noted in case scopes ever become free-form.

In-memory rate limiter never evicts expired windows (src/rate-limit.ts)
Keys accumulate for the life of the isolate. It only guards /_/setup and isolates are short-lived, so I treated this as accepted per the file's own header.

Reviewed with no finding: redirect and KV cache handling, link and slug repositories (batch atomicity, click guards), slug generation (no modulo bias), SSRF guard in title-fetch, Access JWT verification, API key handling, client-side escaping in client.ts, QR encoding.


Generated by Claude Code

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
shrtnr e78e874 Oct 02 2026, 07:20 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants