diff --git a/.changeset/warm-redirects-deploy-warning.md b/.changeset/warm-redirects-deploy-warning.md new file mode 100644 index 0000000000..ac9c23ebdf --- /dev/null +++ b/.changeset/warm-redirects-deploy-warning.md @@ -0,0 +1,12 @@ +--- +"@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. + + diff --git a/packages/deploy-helpers/src/deploy/helpers/assets.ts b/packages/deploy-helpers/src/deploy/helpers/assets.ts index 1a372246a4..c731248f95 100644 --- a/packages/deploy-helpers/src/deploy/helpers/assets.ts +++ b/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,26 @@ 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); + 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, diff --git a/packages/workers-shared/utils/configuration/constructConfiguration.ts b/packages/workers-shared/utils/configuration/constructConfiguration.ts index 6505e535bd..e7e7d5516b 100644 --- a/packages/workers-shared/utils/configuration/constructConfiguration.ts +++ b/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) ); } diff --git a/packages/wrangler/src/__tests__/deploy/assets.test.ts b/packages/wrangler/src/__tests__/deploy/assets.test.ts index 791a0fa647..bacab26314 100644 --- a/packages/wrangler/src/__tests__/deploy/assets.test.ts +++ b/packages/wrangler/src/__tests__/deploy/assets.test.ts @@ -858,6 +858,41 @@ 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 (#14694)", 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: "" }, + ]; + writeAssets(assets); + writeWranglerConfig({ + assets: { directory: "assets" }, + }); + const bodies: AssetManifest[] = []; + await mockAUSRequest(bodies); + mockSubDomainRequest(); + mockUploadWorkerRequest({ + expectedAssets: { + jwt: "<>", + // 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 resolve assets directory relative to cwd if using cli", async ({