feat(redirects): honor the query a redirect source was scoped to

packages/blocks/src/sdk/redirects.test.ts

@@ -0,0 +1,73 @@
+/**
+ * Regression tests for self-redirects — rules whose source and target resolve
+ * to the same path, which a browser reports as ERR_TOO_MANY_REDIRECTS.
+ *
+ * The cases below are real rows from a bulk-migration CSV (Fresh -> TanStack)
+ * that took two pages of a production storefront down.
+ */
+import { describe, expect, it } from "vitest";
+import { addRedirects, loadRedirects, matchRedirect, parseRedirectsCsv } from "./redirects";
+
+const fromCsv = (csv: string) => {
+  const map = loadRedirects({});
+  addRedirects(map, parseRedirectsCsv(csv));
+  return map;
+};
+
+describe("self-redirects are dropped at load time", () => {
+  it("drops a row that literally points at itself", () => {
+    expect(parseRedirectsCsv("from,to\n/a,/a\n")).toEqual([]);
+    expect(matchRedirect("/a", fromCsv("from,to\n/a,/a\n"))).toBeNull();
+  });
+
+  it("drops a row whose absolute source collapses onto its target", () => {
+    // Took /aliancas down: normalizePath drops the query, leaving /x -> /x.
+    const map = fromCsv(
+      "from,to,type\nhttps://www.example.com/aliancas?map=category-1,/aliancas,PERMANENT\n",
+    );
+    expect(matchRedirect("/aliancas", map)).toBeNull();
+  });
+
+  it("drops a row whose absolute target collapses onto its source", () => {
+    // Took the home page down: http://blog.example.com/ -> /
+    expect(matchRedirect("/", fromCsv("from,to\nhttp://blog.example.com/,/\n"))).toBeNull();
+  });
+
+  it("drops a pair that differs only by trailing slash", () => {
+    expect(parseRedirectsCsv("from,to\n/a/,/a\n")).toEqual([]);
+    expect(parseRedirectsCsv("from,to\n/a,/a/\n")).toEqual([]);
+  });
+
+  it("drops a pair that differs only by case", () => {
+    // normalizePath lowercases the source, so /A would match a request to /a.
+    expect(parseRedirectsCsv("from,to\n/A,/a\n")).toEqual([]);
+  });
+
+  it("drops self-redirects declared as CMS blocks, not just CSV rows", () => {
+    const map = loadRedirects({
+      r: {
+        __resolveType: "website/loaders/redirect.ts",
+        redirect: { from: "/loop", to: "/loop", type: "permanent" },
+      },
+    });
+    expect(matchRedirect("/loop", map)).toBeNull();
+  });
+
+  it("keeps a same-path redirect that leaves the site", () => {
+    // Same pathname, different host: a real redirect, not a loop.
+    const map = fromCsv("from,to\n/x,https://other.example.com/x\n");
+    expect(matchRedirect("/x", map)).toMatchObject({ to: "https://other.example.com/x" });
+  });
+
+  it("keeps a same-path redirect that only adds a query", () => {
+    const map = fromCsv("from,to\n/x,/x?ref=1\n");
+    expect(matchRedirect("/x", map)).toBeNull();
+  });
+
+  it("leaves ordinary redirects alone", () => {
+    const map = fromCsv("from,to,type\n/old,/new,permanent\n/blog/*,/news/*,permanent\n");
+    expect(matchRedirect("/old", map)).toMatchObject({ to: "/new", status: 301 });
+    expect(matchRedirect("/blog/post-1", map)).toMatchObject({ to: "/news/post-1" });
+    expect(matchRedirect("/unknown", map)).toBeNull();
+  });
+});

packages/blocks/src/sdk/redirects.ts

@@ -40,13 +40,29 @@ export interface Redirect {
   from: string;
   to: string;
   status: 301 | 302;
+  /**
+   * Query the source URL was scoped to, without the leading "?" (e.g.
+   * `map=category-1`). Only set when the CSV/CMS `from` carried one.
+   *
+   * The map is keyed by pathname, so without this a query-scoped rule would
+   * have to widen into a whole-page rule — which is how these rows used to
+   * produce redirect loops. `matchRedirect` fires them only when the incoming
+   * search matches.
+   */
+  search?: string;
 }
 
 export interface RedirectMap {
   /** Exact match redirects for O(1) lookup. */
   exact: Map<string, Redirect>;
   /** Glob/prefix redirects checked sequentially (few in practice). */
   patterns: Array<{ prefix: string; redirect: Redirect }>;
+  /**
+   * Rules whose `from` carried a query, bucketed by pathname. Only reachable
+   * when `matchRedirect` is given the request's search string, so they can
+   * never hijack a bare path.
+   */
+  scoped: Map<string, Redirect[]>;
 }
 
 // -------------------------------------------------------------------------
@@ -83,6 +99,7 @@ export function registerRedirectResolveType(resolveType: string): void {
 export function loadRedirects(blocks: Record<string, unknown>): RedirectMap {
   const exact = new Map<string, Redirect>();
   const patterns: Array<{ prefix: string; redirect: Redirect }> = [];
+  const scoped = new Map<string, Redirect[]>();
 
   for (const [_key, block] of Object.entries(blocks)) {
     if (!block || typeof block !== "object") continue;
@@ -103,22 +120,21 @@ export function loadRedirects(blocks: Record<string, unknown>): RedirectMap {
     for (const entry of list) {
       if (!entry.from || !entry.to) continue;
 
+      const { path, search } = splitSource(entry.from);
+      if (!search && isSelfRedirect(entry.from, entry.to)) continue;
+
       const redirect: Redirect = {
-        from: normalizePath(entry.from),
+        from: path,
         to: entry.to,
         status: entry.type === "permanent" ? 301 : 302,
+        ...(search ? { search } : {}),
       };
 
-      if (redirect.from.includes("*")) {
-        const prefix = redirect.from.replace(/\*+$/, "");
-        patterns.push({ prefix, redirect });
-      } else {
-        exact.set(redirect.from, redirect);
-      }
+      addToMap({ exact, patterns, scoped }, redirect);
     }
   }
 
-  return { exact, patterns };
+  return { exact, patterns, scoped };
 }
 
 // -------------------------------------------------------------------------
@@ -149,10 +165,14 @@ export function parseRedirectsCsv(csv: string): Redirect[] {
     // header, and for a header repeated when multiple files are concatenated.
     if (from.toLowerCase() === "from" && to.toLowerCase() === "to") continue;
 
+    const { path, search } = splitSource(from);
+    if (!search && isSelfRedirect(from, to)) continue;
+
     redirects.push({
-      from: normalizePath(from),
+      from: path,
       to,
       status: type === "permanent" || type === "301" ? 301 : 302,
+      ...(search ? { search } : {}),
     });
   }
 
@@ -164,12 +184,23 @@ export function parseRedirectsCsv(csv: string): Redirect[] {
  */
 export function addRedirects(map: RedirectMap, redirects: Redirect[]): void {
   for (const redirect of redirects) {
-    if (redirect.from.includes("*")) {
-      const prefix = redirect.from.replace(/\*+$/, "");
-      map.patterns.push({ prefix, redirect });
-    } else {
-      map.exact.set(redirect.from, redirect);
-    }
+    addToMap(map, redirect);
+  }
+}
+
+/** Route a redirect into the bucket that matches its shape. */
+function addToMap(map: RedirectMap, redirect: Redirect): void {
+  if (redirect.search) {
+    const bucket = map.scoped.get(redirect.from);
+    if (bucket) bucket.push(redirect);
+    else map.scoped.set(redirect.from, [redirect]);
+    return;
+  }
+  if (redirect.from.includes("*")) {
+    const prefix = redirect.from.replace(/\*+$/, "");
+    map.patterns.push({ prefix, redirect });
+  } else {
+    map.exact.set(redirect.from, redirect);
   }
 }
 
@@ -183,9 +214,19 @@ export function addRedirects(map: RedirectMap, redirects: Redirect[]): void {
  * Checks exact matches first (O(1)), then glob patterns (O(n), but
  * typically few patterns exist).
  */
-export function matchRedirect(pathname: string, map: RedirectMap): Redirect | null {
+export function matchRedirect(
+  pathname: string,
+  map: RedirectMap,
+  search?: string,
+): Redirect | null {
   const normalized = normalizePath(pathname);
 
+  // Query-scoped rules are the more specific match, so they win over a
+  // bare-path rule for the same pathname. Callers that pass no `search`
+  // never reach them.
+  const scopedMatch = matchScoped(normalized, map, search);
+  if (scopedMatch) return scopedMatch;
+
   const exactMatch = map.exact.get(normalized);
   if (exactMatch) return exactMatch;
 
@@ -200,10 +241,111 @@ export function matchRedirect(pathname: string, map: RedirectMap): Redirect | nu
   return null;
 }
 
+/**
+ * A query-scoped rule matches when every `key=value` it was defined with is
+ * present in the request. Subset rather than equality, so appended tracking
+ * params (utm_*, gclid, fbclid) do not defeat the rule.
+ */
+function matchScoped(pathname: string, map: RedirectMap, search?: string): Redirect | null {
+  if (!search) return null;
+  const bucket = map.scoped.get(pathname);
+  if (!bucket) return null;
+
+  const actual = new URLSearchParams(search.startsWith("?") ? search.slice(1) : search);
+  for (const redirect of bucket) {
+    let matches = true;
+    for (const [key, value] of new URLSearchParams(redirect.search)) {
+      if (actual.get(key) !== value) {
+        matches = false;
+        break;
+      }
+    }
+    if (matches) return redirect;
+  }
+  return null;
+}
+
 // -------------------------------------------------------------------------
 // Helpers
 // -------------------------------------------------------------------------
 
+/**
+ * Split a redirect source into pathname and query.
+ *
+ * The map is keyed by pathname, so the query has to travel beside it rather
+ * than being dropped — dropping it widens a query-scoped rule into a rule for
+ * the whole page (see `Redirect.search`).
+ */
+function splitSource(from: string): { path: string; search?: string } {
+  const raw = from.trim();
+
+  if (isAbsolute(raw)) {
+    try {
+      const url = new URL(raw);
+      return {
+        path: normalizePath(url.pathname),
+        search: url.search ? url.search.slice(1) : undefined,
+      };
+    } catch {
+      // malformed URL: fall through to the string-level split
+    }
+  }
+
+  const qIdx = raw.indexOf("?");
+  if (qIdx >= 0) {
+    return { path: normalizePath(raw.slice(0, qIdx)), search: raw.slice(qIdx + 1) || undefined };
+  }
+  return { path: normalizePath(raw) };
+}
+
+const isAbsolute = (u: string) => u.startsWith("http://") || u.startsWith("https://");
+
+/** origin + normalized pathname, or null when the URL is malformed. */
+function absoluteKey(url: string): string | null {
+  try {
+    const parsed = new URL(url);
+    return parsed.origin + normalizePath(parsed.pathname);
+  } catch {
+    return null;
+  }
+}
+
+/**
+ * A rule whose source and target resolve to the same page redirects to itself:
+ * the response sends the browser back to the URL it just asked for, which it
+ * reports as ERR_TOO_MANY_REDIRECTS.
+ *
+ * These are routine in bulk migration exports, and `normalizePath` manufactures
+ * more of them: it reduces an absolute `from` to `new URL(p).pathname`, so a
+ * rule scoped to one query (`https://site/x?map=category-1 -> /x`) collapses
+ * onto the bare path it points at.
+ *
+ * Host is compared whenever both sides carry one. A relative target is compared
+ * on path alone — the map is keyed by pathname, so a rule reached from any host
+ * lands the visitor back on this site's copy of that path.
+ *
+ * The Fresh loader this SDK replaced (deco-cx/apps
+ * `website/loaders/redirectsFromCsv.ts`) carried the same `from === to` guard.
+ */
+function isSelfRedirect(from: string, to: string): boolean {
+  const src = from.trim();
+  const dst = to.trim();
+
+  // Both absolute: same site AND same path is a loop; a different host is not.
+  if (isAbsolute(src) && isAbsolute(dst)) {
+    const a = absoluteKey(src);
+    return a !== null && a === absoluteKey(dst);
+  }
+
+  // Target on another origin (absolute or protocol-relative) always goes
+  // somewhere else — the host is what differs, and we cannot know ours.
+  if (isAbsolute(dst) || dst.startsWith("//")) return false;
+
+  const qIdx = dst.indexOf("?");
+  const dstPath = normalizePath(qIdx >= 0 ? dst.slice(0, qIdx) : dst);
+  return dstPath === normalizePath(src);
+}
+
 function normalizePath(path: string): string {
   let p = path.trim();