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

src/__tests__/handler/api-misc.test.ts

@@ -146,6 +146,26 @@ describe("Redirect", () => {
     expect(res.headers.get("Location")).toBe("https://destination.com/");
   });
 
+  it("redirect carries a short private Cache-Control so disables and retargets reach returning visitors", async () => {
+    // A bare 301 is cached by browsers indefinitely: a returning visitor
+    // never contacts the Worker again, so disabling or retargeting the link
+    // has no effect for them and their repeat clicks go unrecorded. The
+    // short private max-age keeps the SEO semantics of a 301 while forcing
+    // revalidation within seconds.
+    const createRes = await SELF.fetch(
+      authed("/_/admin/api/links", {
+        method: "POST",
+        headers: { "Content-Type": "application/json" },
+        body: JSON.stringify({ url: "https://cached.example.com" }),
+      })
+    );
+    const created = await createRes.json() as any;
+    const slug = created.slugs[0].slug;
+    const res = await SELF.fetch(unauthed(`/${slug}`), { redirect: "manual" });
+    expect(res.status).toBe(301);
+    expect(res.headers.get("Cache-Control")).toBe("private, max-age=90");
+  });
+
   it("should return 404 for a non-existent slug", async () => {
     const res = await SELF.fetch(unauthed("/nonexistent999"));
     expect(res.status).toBe(404);

src/__tests__/service/ownership.test.ts

@@ -1,7 +1,7 @@
 import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
 import { env } from "cloudflare:test";
 import { applyMigrations, resetData } from "../setup";
-import { LinkRepository } from "../../db";
+import { LinkRepository, SlugRepository } from "../../db";
 import { SlugCache } from "../../kv";
 import {
   createLink,
@@ -168,6 +168,25 @@ describe("Slug ownership: remove", () => {
   });
 });
 
+describe("addCustomSlugToLink: concurrent UNIQUE violation returns 409", () => {
+  it("returns 409 rather than 500 when a concurrent request claims the slug between the existence check and the insert", async () => {
+    // Two concurrent requests both pass SlugRepository.exists() before either
+    // commits. The second INSERT hits the UNIQUE constraint. Without a try/catch
+    // in the service layer, the D1 error propagates as an unhandled 500.
+    const link = await createOwnedLink();
+
+    const spy = vi.spyOn(SlugRepository, "addCustom").mockRejectedValueOnce(
+      new Error("D1_ERROR: UNIQUE constraint failed: slugs.slug"),
+    );
+    const result = await addCustomSlugToLink(env as any, link.id, { slug: "raced-slug" }).finally(() => spy.mockRestore());
+
+    expect(result.ok).toBe(false);
+    if (!result.ok) {
+      expect(result.status).toBe(409);
+    }
+  });
+});
+
 describe("Collaboration: adding slugs", () => {
   it("non-owner can add a custom slug to another user's link", async () => {
     const link = await createOwnedLink();
@@ -213,6 +232,33 @@ describe("Link disable via expires_at", () => {
   });
 });
 
+describe("searchLinks: LIKE metacharacter escaping", () => {
+  it("searching for _ does not return links that have no literal underscore", async () => {
+    // Without escaping, the LIKE pattern '%_%' matches any non-empty string,
+    // returning every link. With correct escaping it matches only links whose
+    // label, slug, or URL literally contains an underscore character.
+    await createLink(env as any, { url: "https://example.com", label: "hello world", created_by: OWNER });
+    await createLink(env as any, { url: "https://other.com", label: "other link", created_by: OTHER, allow_duplicate: true });
+
+    const result = await searchLinks(env as any, "_", { includeOwner: true });
+    expect(result.ok).toBe(true);
+    if (result.ok) {
+      expect(result.data).toHaveLength(0);
+    }
+  });
+
+  it("searching for % does not return links that have no literal percent sign", async () => {
+    // Without escaping, the LIKE pattern '%%' matches every string.
+    await createLink(env as any, { url: "https://example.com", label: "hello world", created_by: OWNER });
+
+    const result = await searchLinks(env as any, "%");
+    expect(result.ok).toBe(true);
+    if (result.ok) {
+      expect(result.data).toHaveLength(0);
+    }
+  });
+});
+
 describe("UI search includes created_by", () => {
   it("finds a link when searching by owner email with includeOwner", async () => {
     await createOwnedLink(OWNER);
@@ -355,3 +401,40 @@ describe("disableLink: null return from repository does not throw", () => {
     }
   });
 });
+
+describe("disableSlug: null return from repository does not crash", () => {
+  it("returns 404 rather than throwing when the slug is concurrently deleted", async () => {
+    // SlugRepository.disable() returns null when the slug row vanishes between
+    // the service's existence check and the UPDATE. Without a null guard the
+    // service dereferences disabled!.disabled_at and throws a TypeError.
+    const link = await createOwnedLink();
+    await addCustomSlugToLink(env as any, link.id, { slug: "concurrent-slug" });
+
+    const spy = vi.spyOn(SlugRepository, "disable").mockResolvedValueOnce(null);
+    const result = await disableSlug(env as any, link.id, "concurrent-slug", OWNER).finally(() => spy.mockRestore());
+
+    expect(result.ok).toBe(false);
+    if (!result.ok) {
+      expect(result.status).toBe(404);
+    }
+  });
+});
+
+describe("enableSlug: null return from repository returns 404", () => {
+  it("returns 404 rather than returning null data when the slug is concurrently deleted", async () => {
+    // SlugRepository.enable() returns null when the slug row vanishes between
+    // the service's existence check and the UPDATE. Without a null guard the
+    // service returns ok(null) instead of a proper 404.
+    const link = await createOwnedLink();
+    await addCustomSlugToLink(env as any, link.id, { slug: "concurrent-slug" });
+    await disableSlug(env as any, link.id, "concurrent-slug", OWNER);
+
+    const spy = vi.spyOn(SlugRepository, "enable").mockResolvedValueOnce(null);
+    const result = await enableSlug(env as any, link.id, "concurrent-slug", OWNER).finally(() => spy.mockRestore());
+
+    expect(result.ok).toBe(false);
+    if (!result.ok) {
+      expect(result.status).toBe(404);
+    }
+  });
+});

src/db/link-repository.ts

@@ -167,11 +167,17 @@ export class LinkRepository {
   ): Promise<LinkWithSlugs[]> {
     if (!query.trim()) return [];
 
-    const pattern = `%${query.trim().toLowerCase()}%`;
+    // Escape SQLite LIKE metacharacters so a user query of "_" or "50%" is
+    // treated as a literal string, not a wildcard pattern.
+    const escaped = query.trim().toLowerCase()
+      .replace(/\\/g, "\\\\")
+      .replace(/%/g, "\\%")
+      .replace(/_/g, "\\_");
+    const pattern = `%${escaped}%`;
 
     const where = opts?.includeOwner
-      ? "lower(l.label) LIKE ? OR lower(s.slug) LIKE ? OR lower(l.url) LIKE ? OR lower(l.created_by) LIKE ?"
-      : "lower(l.label) LIKE ? OR lower(s.slug) LIKE ? OR lower(l.url) LIKE ?";
+      ? "lower(l.label) LIKE ? ESCAPE '\\' OR lower(s.slug) LIKE ? ESCAPE '\\' OR lower(l.url) LIKE ? ESCAPE '\\' OR lower(l.created_by) LIKE ? ESCAPE '\\'"
+      : "lower(l.label) LIKE ? ESCAPE '\\' OR lower(s.slug) LIKE ? ESCAPE '\\' OR lower(l.url) LIKE ? ESCAPE '\\'";
 
     const binds = opts?.includeOwner
       ? [pattern, pattern, pattern, pattern]

src/redirect.ts

@@ -86,6 +86,17 @@ export async function handleRedirect(
 
   ctx.waitUntil(recordClick(env, normalizedSlug, data));
 
-  // 6. Redirect
-  return Response.redirect(entry.url, 301);
+  // 6. Redirect. 301 keeps the SEO signal of a permanent move, but a bare
+  // 301 is cached by browsers indefinitely: returning visitors would skip
+  // the Worker forever, so disables and retargets would never reach them
+  // and their repeat clicks would go unrecorded. The short private max-age
+  // forces revalidation within seconds (same approach Bitly uses).
+  // new URL(...).href mirrors Response.redirect's Location serialization.
+  return new Response(null, {
+    status: 301,
+    headers: {
+      Location: new URL(entry.url).href,
+      "Cache-Control": "private, max-age=90",
+    },
+  });
 }

src/services/link-management.ts

@@ -231,7 +231,17 @@ export async function addCustomSlugToLink(
     return fail(409, "Slug already exists");
   }
 
-  const slug = await SlugRepository.addCustom(env.DB, linkId, normalizedSlug);
+  let slug: Slug;
+  try {
+    slug = await SlugRepository.addCustom(env.DB, linkId, normalizedSlug);
+  } catch (e) {
+    // A concurrent request may claim the same slug between the exists() check
+    // and the INSERT, hitting the UNIQUE constraint. Surface it as 409.
+    if (e instanceof Error && e.message.includes("UNIQUE constraint failed")) {
+      return fail(409, "Slug already exists");
+    }
+    throw e;
+  }
 
   await SlugCache.put(env.SLUG_KV, normalizedSlug, {
     url: link.url,
@@ -273,14 +283,15 @@ export async function disableSlug(
   if (!slugObj.is_custom) return fail(400, "Cannot disable the system-generated slug; only custom slugs can be disabled. Disable the whole link instead.");
 
   const disabled = await SlugRepository.disable(env.DB, slug);
+  if (!disabled) return fail(404, "Slug not found");
 
   await SlugCache.put(env.SLUG_KV, slug, {
     url: link.url,
-    disabled_at: disabled!.disabled_at,
+    disabled_at: disabled.disabled_at,
     expires_at: link.expires_at,
   });
 
-  return ok(disabled!);
+  return ok(disabled);
 }
 
 export async function enableSlug(
@@ -297,14 +308,15 @@ export async function enableSlug(
   if (!slugObj) return fail(404, "Slug not found on this link");
 
   const enabled = await SlugRepository.enable(env.DB, slug);
+  if (!enabled) return fail(404, "Slug not found");
 
   await SlugCache.put(env.SLUG_KV, slug, {
     url: link.url,
     disabled_at: null,
     expires_at: link.expires_at,
   });
 
-  return ok(enabled!);
+  return ok(enabled);
 }
 
 export async function removeSlug(