fix: weekly defect review — null crashes in slug ops, LIKE wildcard escape, TOCTOU 409

[DennisAlund]

Weekly defect-hunting review. Three confirmed bugs, each with a regression test. All 901 tests pass.

---

Fix 1: disableSlug / enableSlug — TypeError crash on concurrent delete

What was wrong: disableSlug accessed disabled!.disabled_at (line 279) without a null guard. SlugRepository.disable() returns Slug | null; if the slug row is concurrently deleted between the service's link.slugs.find() check and the UPDATE, the function returns null and the ! assertion throws TypeError: Cannot read properties of null (reading 'disabled_at'). enableSlug had the same pattern — ok(enabled!) with a null return produces { ok: true, data: null } instead of a 404.

Impact: Any concurrent DELETE /links/:id that races a PATCH /slugs/:slug/disable or /enable produces an unhandled 500 instead of a graceful 404.

Fix: Added if (!disabled) return fail(404, "Slug not found") after SlugRepository.disable(), and the same guard after SlugRepository.enable(). Removed the ! non-null assertions. Follows the same pattern already in place for disableLink (added in the prior PR).

Test: ownership.test.ts — mocks SlugRepository.disable/enable to return null and asserts status 404 rather than a throw.

---

Fix 2: addCustomSlugToLink — TOCTOU produces unhandled 500 on concurrent insert

What was wrong: The duplicate-slug check was a separate SlugRepository.exists() call before SlugRepository.addCustom(). Two concurrent requests can both pass the exists() check before either commits. The second INSERT hits the UNIQUE constraint on slugs.slug. addCustom() has no try/catch; addCustomSlugToLink has no try/catch. The D1 error propagates as an unhandled 500 instead of a 409.

Impact: Concurrent requests to add the same custom slug produce a 500 instead of the expected conflict response.

Fix: Wrapped SlugRepository.addCustom() in a try/catch. Errors whose message contains "UNIQUE constraint failed" are translated to fail(409, "Slug already exists").

Test: ownership.test.ts — mocks SlugRepository.addCustom to throw a UNIQUE constraint error and asserts status 409.

---

Fix 3: LinkRepository.search — LIKE metacharacter wildcard

What was wrong: The search pattern was built as `%${query.trim().toLowerCase()}%` with no escaping. SQLite LIKE treats % as "any sequence" and _ as "any single character". Searching for _ creates the pattern %_%, which matches any string with at least one character — effectively returning every link with a non-null label, slug, or URL.

Impact: A search for _ returns every link in the database. A search for 50% or example_com matches rows that do not literally contain those strings. Incorrect results with no error.

Fix: Escape \ → \\, % → \%, _ → \_ before building the pattern, and add ESCAPE '\' to every LIKE comparison in the WHERE clause.

Test: ownership.test.ts — creates links with no underscores or percent signs, then asserts that searching for _ and % returns zero results.

---

Items flagged for developer judgment (PR comments)

See inline comments on this PR for four issues that need design or schema decisions before fixing.

---

Generated by Claude Code

[DennisAlund]

FLAG: Non-atomic delete leaves zombie links on Worker crash

LinkRepository.delete() runs three separate await db.prepare(...).run() calls with no db.batch() wrapper:

// link-repository.ts lines 157-159
await db.prepare("DELETE FROM clicks WHERE slug IN (SELECT slug FROM slugs WHERE link_id = ?)").bind(id).run();
await db.prepare("DELETE FROM slugs WHERE link_id = ?").bind(id).run();
await db.prepare("DELETE FROM links WHERE id = ?").bind(id).run();

If the Worker is killed (CPU/memory eviction, deploy rollover) between any two steps, the DB is left in a partially-cleaned state:

• Crash after DELETE clicks: link and slugs survive with zero click history — the next delete attempt passes the lifetime guard and retries cleanly. Low impact.
• Crash after DELETE slugs: the links row survives permanently with no slug rows and can never be redirected or deleted again (getById assembles a link with empty slugs, total_clicks = 0 passes the guard, but cascading slug deletes is a no-op and the link row orphan stays in the list() output forever).

LinkRepository.create() already uses db.batch() with an explicit comment noting this. delete() should do the same.

Recommended fix: Wrap the three DELETEs in db.batch([...]). This does not change the return type or any public contract. I did not fix this automatically because I could not write a regression test that reliably simulates a mid-execution Worker crash — the fix is correct and safe but not test-provable in the current test harness.

---

Generated by Claude Code

[DennisAlund]

FLAG: HTTP 301 permanent redirect makes link disabling ineffective for cached clients

redirect.ts line 90:

return Response.redirect(entry.url, 301);

HTTP 301 is a permanent redirect. Browsers cache it with no expiry and no revalidation. Once a browser follows a short link, it stores the destination locally. Any subsequent disableLink, enableLink, or updateLink call has no effect on that browser — it redirects directly to the cached URL without contacting the server.

This means:

• Disabling a link stops new clients but does not stop returning visitors.
• Retargeting a short link (changing url) also does not take effect for cached browsers.

For a URL shortener this is a significant operational gap: an admin who disables a link pointing to stale or harmful content cannot guarantee it stops working.

Recommended fix: Change to 302 (Found) or 307 (Temporary Redirect). 302 is the conventional choice for URL shorteners for exactly this reason. This is a behavioral change that affects all existing short links, so it needs a deliberate decision about whether cached 301s from existing users need to be invalidated (they cannot be, from the server side).

---

Generated by Claude Code

[DennisAlund]

FLAG: disableLink/enableLink share expires_at for both schedule and admin-disable, so re-enabling a link permanently clears a pre-existing expiry

LinkRepository.disable() sets expires_at = now (the current timestamp) to make the link appear expired. LinkRepository.enable() sets expires_at = NULL.

If a link was created with a future expires_at (e.g. 2027-01-01 for a campaign), an admin disables it for review, then re-enables it, the original expiry is permanently lost — the link becomes perpetually active.

// disable: link-repository.ts line 129
await db.prepare("UPDATE links SET expires_at = ? WHERE id = ?").bind(now, id).run();

// enable: link-repository.ts line 136
await db.prepare("UPDATE links SET expires_at = NULL WHERE id = ?").bind(id).run();

The disabled_at field exists on slugs rows for per-slug state, but there is no disabled_at on the links table — the admin-disable state is entirely encoded in expires_at, which it shares with the scheduled expiry.

Recommended fix: Add a disabled_at column to the links table (requires a migration). disable() writes the current timestamp to disabled_at and leaves expires_at untouched. enable() clears disabled_at. redirect.ts and any KV-path checks need to additionally test disabled_at IS NOT NULL. This is a schema change with migration work.

---

Generated by Claude Code

[cloudflare-workers-and-pages]

Deploying with    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	8be21bb	Jun 11 2026, 09:20 AM

[DennisAlund]

FLAG: GET /api/links?owner= allows any read-scope API key to enumerate another user's links

api/links.ts lines 110-115:

linksApp.openapi(listLinksRoute, async (c) => {
  const { owner, range } = c.req.valid("query") as { owner?: string; range?: TimelineRange };
  const result = owner
    ? await listLinksByOwner(c.env, owner, opts)
    : await listLinks(c.env, opts);
  return fromServiceResult(result) as never;
});

The middleware for this route is requireScope("read") only. There is no check that c.var.auth.identity === owner. Any API key with read scope can pass an arbitrary owner value and receive the full link inventory for that identity — including labels, target URLs, click counts, and slug names.

Decision needed: Is listLinksByOwner intended to be owner-gated (only callable by the matching identity) or admin-visible (any authenticated read key can list any owner)? If it should be owner-gated, add a check like:

if (owner && owner !== c.var.auth.identity) return fail(403, "Cannot list links for another owner");

If admin visibility is intentional, no change needed — but this should be documented as a design decision.

---

Generated by Claude Code

[Copilot]

Pull request overview

Fixes three production-facing defects in slug/link operations by hardening service/repository behavior against race conditions and ensuring SQLite LIKE searches treat user input literally.

Changes:

• Add null guards in disableSlug / enableSlug to prevent crashes and return 404 on concurrent slug deletion.
• Translate concurrent unique constraint failures in addCustomSlugToLink into a 409 conflict response.
• Escape SQLite LIKE metacharacters (%, _, \) and add ESCAPE '\' so search queries don’t behave like wildcard patterns.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/services/link-management.ts	Adds race-safe null handling for slug enable/disable and converts UNIQUE insert races into 409s.
src/db/link-repository.ts	Escapes LIKE metacharacters and applies ESCAPE '\' across search predicates.
src/tests/service/ownership.test.ts	Adds regression tests covering the three fixed defect scenarios (404/409 behavior, literal search).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[DennisAlund]

Closing out the four flagged items with decisions from review:

1. Non-atomic LinkRepository.delete: fixed in PR Robustness audit: input validation, atomic mutations, consistency fixes #13 (db.batch around the three DELETEs, plus a no-orphan regression test). Robustness audit: input validation, atomic mutations, consistency fixes #13 rebases onto this PR after merge, so no duplicate fix here.

2. 301 redirect caching: fixed on this branch in 8be21bb. The redirect keeps status 301 for its SEO semantics but now sends Cache-Control: private, max-age=90 (the Bitly approach), so browsers revalidate within seconds. Disables, expiries, and retargets reach returning visitors, and their repeat clicks get recorded again. Location serialization is unchanged; covered by a new handler test.

3. disable/enable sharing expires_at: accepted as-is. The loss only occurs when a future scheduled expiry is overwritten by a temporary mid-campaign disable, a corner case this deployment does not exercise. One mechanism, no migration.

4. GET /_/api/links?owner= enumeration: confirmed by design. Reads (links, bundles, owner listings) are open to any authenticated key; writes stay owner-gated. This mirrors the workspace-shared model already documented in src/services/bundle-management.ts.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.