[wrangler] Surface _redirects validation warnings at deploy time (#14694)

.changeset/warm-redirects-deploy-warning.md

@@ -0,0 +1,11 @@
+---
+"wrangler": patch
+"@cloudflare/workers-shared": patch
+"@cloudflare/deploy-helpers": patch
+---
+
+Surface `_redirects` validation warnings at `wrangler deploy` time
+
+Previously, invalid `_redirects` rules (e.g. a duplicate rule for the same path, or a file that exceeds the 100-rule dynamic-rule budget and has its remaining lines silently dropped) were only reported as a warning in `wrangler dev`. `wrangler deploy` uploaded the raw `_redirects` file without any client-side validation, so the same issues went completely unreported at deploy time.
+
+`wrangler deploy` now parses `_redirects` for validation purposes and warns about any invalid rules, using the same messages already shown by `wrangler dev`. This does not change what gets uploaded — the raw file is still uploaded as-is, and the asset worker remains the authoritative parser at runtime.

packages/deploy-helpers/src/deploy/helpers/assets.ts

@@ -2,6 +2,8 @@ import assert from "node:assert";
 import { createReadStream } from "node:fs";
 import { readdir, readFile, stat } from "node:fs/promises";
 import * as path from "node:path";
+import { formatInvalidRedirectsWarning } from "@cloudflare/workers-shared/utils/configuration/constructConfiguration";
+import { parseRedirects } from "@cloudflare/workers-shared/utils/configuration/parseRedirects";
 import { parseStaticRouting } from "@cloudflare/workers-shared/utils/configuration/parseStaticRouting";
 import {
 	CF_ASSETS_IGNORE_FILENAME,
@@ -554,6 +556,28 @@ export function resolveAssetOptions(
 		? maybeGetFile(path.join(directory, HEADERS_FILENAME))
 		: undefined;
 
+	// The _redirects and _headers files are parsed in Miniflare in dev, and the
+	// asset worker is the source of truth for the parsed/enforced config at
+	// runtime, so we don't need the parsed result here for deploy. We do still
+	// parse `_redirects` for validation purposes so that authors get the same
+	// "invalid rule" / dynamic-rule-limit warnings at deploy time that they'd
+	// otherwise only see in `wrangler dev` (see issue #14694). This never
+	// changes what gets uploaded -- the raw file content is still uploaded as-is
+	// below, unchanged.
+	if (_redirects) {
+		const parsedRedirects = parseRedirects(_redirects, {
+			htmlHandling: config.assets?.html_handling,
+		});
+		if (parsedRedirects.invalid.length > 0) {
+			logger.warn(
+				formatInvalidRedirectsWarning(
+					parsedRedirects.invalid,
+					REDIRECTS_FILENAME
+				)
+			);
+		}
+	}
+
 	// defaults are set in asset worker
 	const assetConfig: AssetConfig = {
 		html_handling: config.assets?.html_handling,

packages/workers-shared/utils/configuration/constructConfiguration.ts

@@ -11,7 +11,38 @@ import type {
 	MetadataRedirects,
 	MetadataStaticRedirects,
 } from "../types";
-import type { Logger, ParsedHeaders, ParsedRedirects } from "./types";
+import type {
+	InvalidRedirectRule,
+	Logger,
+	ParsedHeaders,
+	ParsedRedirects,
+} from "./types";
+
+/**
+ * Build the human-readable warning text for a list of invalid `_redirects` rules.
+ * Shared between the dev-time construction path (`constructRedirects`) and any
+ * other caller (e.g. deploy) that wants to surface the same validation warnings
+ * without re-implementing the formatting.
+ */
+export function formatInvalidRedirectsWarning(
+	invalid: InvalidRedirectRule[],
+	relativePath: string
+): string {
+	let invalidRedirectRulesList = ``;
+
+	for (const { line, lineNumber, message } of invalid) {
+		invalidRedirectRulesList += `▶︎ ${message}\n`;
+
+		if (line) {
+			invalidRedirectRulesList += `    at ${relativePath}${lineNumber ? `:${lineNumber}` : ""} | ${line}\n\n`;
+		}
+	}
+
+	return (
+		`Found ${invalid.length} invalid redirect rule${invalid.length === 1 ? "" : "s"}:\n` +
+		`${invalidRedirectRulesList}`
+	);
+}
 
 export function constructRedirects({
 	redirects,
@@ -40,19 +71,8 @@ export function constructRedirects({
 	);
 
 	if (num_invalid > 0) {
-		let invalidRedirectRulesList = ``;
-
-		for (const { line, lineNumber, message } of redirects.invalid) {
-			invalidRedirectRulesList += `▶︎ ${message}\n`;
-
-			if (line) {
-				invalidRedirectRulesList += `    at ${redirectsRelativePath}${lineNumber ? `:${lineNumber}` : ""} | ${line}\n\n`;
-			}
-		}
-
 		logger.warn(
-			`Found ${num_invalid} invalid redirect rule${num_invalid === 1 ? "" : "s"}:\n` +
-				`${invalidRedirectRulesList}`
+			formatInvalidRedirectsWarning(redirects.invalid, redirectsRelativePath)
 		);
 	}
 

packages/wrangler/src/__tests__/deploy/assets.test.ts

@@ -858,6 +858,106 @@ describe("deploy", () => {
 				  },
 				}
 			`);
+			// A valid _redirects file should not produce any deploy-time warning.
+			expect(std.warn).toMatchInlineSnapshot(`""`);
+		});
+
+		it("should warn about invalid _redirects rules at deploy time without altering what is uploaded", async ({
+			expect,
+		}) => {
+			// A duplicate rule for the same path is invalid and should be flagged,
+			// but until now this warning only ever surfaced in `wrangler dev`.
+			const redirectsContent = "/foo /bar\n/foo /baz";
+			const assets = [
+				{ filePath: "_redirects", content: redirectsContent },
+				{ filePath: "index.html", content: "<html></html>" },
+			];
+			writeAssets(assets);
+			writeWranglerConfig({
+				assets: { directory: "assets" },
+			});
+			const bodies: AssetManifest[] = [];
+			await mockAUSRequest(bodies);
+			mockSubDomainRequest();
+			mockUploadWorkerRequest({
+				expectedAssets: {
+					jwt: "<<aus-completion-token>>",
+					// The raw (unparsed, unmodified) _redirects file is still uploaded
+					// as-is -- the deploy-time validation is warning-only.
+					config: {
+						_redirects: redirectsContent,
+					},
+				},
+				expectedType: "none",
+			});
+			await runWrangler("deploy");
+			expect(std.warn).toContain("Found 1 invalid redirect rule");
+			expect(std.warn).toContain("Ignoring duplicate rule for path /foo.");
+		});
+
+		it("should not warn about a false infinite-loop when html_handling is 'none'", async ({
+			expect,
+		}) => {
+			// `/ /index.html` looks like an infinite-loop redirect only when HTML
+			// handling is enabled. Projects that explicitly disable it (html_handling:
+			// "none") must not get a false warning at deploy time -- the deploy-time
+			// validation has to respect the same option the Vite plugin already passes.
+			const redirectsContent = "/ /index.html";
+			const assets = [
+				{ filePath: "_redirects", content: redirectsContent },
+				{ filePath: "index.html", content: "<html></html>" },
+			];
+			writeAssets(assets);
+			writeWranglerConfig({
+				assets: { directory: "assets", html_handling: "none" },
+			});
+			const bodies: AssetManifest[] = [];
+			await mockAUSRequest(bodies);
+			mockSubDomainRequest();
+			mockUploadWorkerRequest({
+				expectedAssets: {
+					jwt: "<<aus-completion-token>>",
+					config: {
+						_redirects: redirectsContent,
+						html_handling: "none",
+					},
+				},
+				expectedType: "none",
+			});
+			await runWrangler("deploy");
+			expect(std.warn).not.toContain("Infinite loop detected");
+			expect(std.warn).toMatchInlineSnapshot(`""`);
+		});
+
+		it("should still warn about a real infinite-loop when html_handling is enabled (default)", async ({
+			expect,
+		}) => {
+			// Sanity check for the other side of the fix: when HTML handling is left
+			// at its default (enabled), the same rule genuinely is an infinite loop
+			// and should still be flagged.
+			const redirectsContent = "/ /index.html";
+			const assets = [
+				{ filePath: "_redirects", content: redirectsContent },
+				{ filePath: "index.html", content: "<html></html>" },
+			];
+			writeAssets(assets);
+			writeWranglerConfig({
+				assets: { directory: "assets" },
+			});
+			const bodies: AssetManifest[] = [];
+			await mockAUSRequest(bodies);
+			mockSubDomainRequest();
+			mockUploadWorkerRequest({
+				expectedAssets: {
+					jwt: "<<aus-completion-token>>",
+					config: {
+						_redirects: redirectsContent,
+					},
+				},
+				expectedType: "none",
+			});
+			await runWrangler("deploy");
+			expect(std.warn).toContain("Infinite loop detected");
 		});
 
 		it("should resolve assets directory relative to cwd if using cli", async ({