diff --git a/src/__tests__/handler/api-misc.test.ts b/src/__tests__/handler/api-misc.test.ts index 1c9efab..c5cb795 100644 --- a/src/__tests__/handler/api-misc.test.ts +++ b/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); diff --git a/src/__tests__/service/ownership.test.ts b/src/__tests__/service/ownership.test.ts index 064bf17..03af5c8 100644 --- a/src/__tests__/service/ownership.test.ts +++ b/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); + } + }); +}); diff --git a/src/db/link-repository.ts b/src/db/link-repository.ts index 502d2aa..d8f5049 100644 --- a/src/db/link-repository.ts +++ b/src/db/link-repository.ts @@ -167,11 +167,17 @@ export class LinkRepository { ): Promise { 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] diff --git a/src/redirect.ts b/src/redirect.ts index 440aa50..e25e382 100644 --- a/src/redirect.ts +++ b/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", + }, + }); } diff --git a/src/services/link-management.ts b/src/services/link-management.ts index 2dd0b9f..068a9b2 100644 --- a/src/services/link-management.ts +++ b/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,6 +308,7 @@ 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, @@ -304,7 +316,7 @@ export async function enableSlug( expires_at: link.expires_at, }); - return ok(enabled!); + return ok(enabled); } export async function removeSlug(