-
Notifications
You must be signed in to change notification settings - Fork 218
docs/runtime: warn on external rewrites, sanitize Content-Disposition #418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2640c27
c9fdd96
96b2916
06a869d
affd28e
c4a128d
8a5f972
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import fs from "node:fs"; | |
| import { randomUUID } from "node:crypto"; | ||
| import { PHASE_DEVELOPMENT_SERVER } from "../shims/constants.js"; | ||
| import { normalizePageExtensions } from "../routing/file-matcher.js"; | ||
| import { isExternalUrl } from "./config-matchers.js"; | ||
|
|
||
| /** | ||
| * Parse a body size limit value (string or number) into bytes. | ||
|
|
@@ -438,6 +439,23 @@ export async function resolveNextConfig( | |
| } | ||
| } | ||
|
|
||
| // Warn about external rewrites that act as reverse proxies | ||
| { | ||
| const allRewrites = [...rewrites.beforeFiles, ...rewrites.afterFiles, ...rewrites.fallback]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Substantive: This warning fires at config-resolution time (during Is that intentional? If someone deploys with external rewrites, they'll see the warning during This isn't a blocker — the build-time warning is already useful — but worth noting for a follow-up. |
||
| const externalRewrites = allRewrites.filter((r) => isExternalUrl(r.destination)); | ||
| if (externalRewrites.length > 0) { | ||
| const listing = externalRewrites.map((r) => ` ${r.source} → ${r.destination}`).join("\n"); | ||
| const noun = externalRewrites.length === 1 ? "external rewrite" : "external rewrites"; | ||
| console.warn( | ||
| `[vinext] Found ${externalRewrites.length} ${noun} that proxy requests to third-party origins:\n` + | ||
| `${listing}\n` + | ||
| `Credential headers (cookie, authorization, proxy-authorization, x-api-key) are stripped from outbound requests. ` + | ||
| `All other request headers are still forwarded. ` + | ||
| `If you need to forward credentials to the external origin, use an API route or route handler where you control exactly which headers are sent.`, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The guidance here is good and actionable. One thought: the message says "use an API route or route handler where you control exactly which headers are sent" — it might be worth adding that credentials can be injected server-side in the API route (e.g., from environment variables or a secrets store) rather than forwarded from the client request. That makes the security benefit more concrete for developers who aren't sure why an API route is better. Not blocking — the current wording is clear enough. |
||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: this suggests "proxying at the CDN level" as an alternative, which is reasonable advice for Cloudflare users. But for non-Cloudflare deployments, this might not be as actionable. Consider mentioning the more general alternative of using server-side fetch in an API route/route handler, which works everywhere. |
||
| } | ||
| } | ||
|
Comment on lines
+442
to
+457
|
||
|
|
||
| // Resolve headers | ||
| let headers: NextHeader[] = []; | ||
| if (config.headers) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -171,7 +171,10 @@ function setImageSecurityHeaders(headers: Headers, config?: ImageConfig): void { | |
| config?.contentSecurityPolicy ?? IMAGE_CONTENT_SECURITY_POLICY, | ||
| ); | ||
| headers.set("X-Content-Type-Options", "nosniff"); | ||
| headers.set("Content-Disposition", config?.contentDispositionType ?? "inline"); | ||
| headers.set( | ||
| "Content-Disposition", | ||
| config?.contentDispositionType === "attachment" ? "attachment" : "inline", | ||
| ); | ||
|
Comment on lines
+174
to
+177
|
||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -599,7 +599,8 @@ async function startAppRouterServer(options: AppRouterServerOptions) { | |
| "Content-Security-Policy": | ||
| imageConfig?.contentSecurityPolicy ?? IMAGE_CONTENT_SECURITY_POLICY, | ||
| "X-Content-Type-Options": "nosniff", | ||
| "Content-Disposition": imageConfig?.contentDispositionType ?? "inline", | ||
| "Content-Disposition": | ||
| imageConfig?.contentDispositionType === "attachment" ? "attachment" : "inline", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The sanitization pattern |
||
| }; | ||
| if (tryServeStatic(req, res, clientDir, params.imageUrl, false, imageSecurityHeaders)) { | ||
| return; | ||
|
|
@@ -772,7 +773,8 @@ async function startPagesRouterServer(options: PagesRouterServerOptions) { | |
| "Content-Security-Policy": | ||
| pagesImageConfig?.contentSecurityPolicy ?? IMAGE_CONTENT_SECURITY_POLICY, | ||
| "X-Content-Type-Options": "nosniff", | ||
| "Content-Disposition": pagesImageConfig?.contentDispositionType ?? "inline", | ||
| "Content-Disposition": | ||
| pagesImageConfig?.contentDispositionType === "attachment" ? "attachment" : "inline", | ||
| }; | ||
| if (tryServeStatic(req, res, clientDir, params.imageUrl, false, imageSecurityHeaders)) { | ||
| return; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -596,3 +596,86 @@ describe("generateBuildId", () => { | |||||||||||||
| expect(b.buildId).toBe("stable-id"); | ||||||||||||||
| }); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| describe("resolveNextConfig external rewrite warning", () => { | ||||||||||||||
| afterEach(() => { | ||||||||||||||
| vi.restoreAllMocks(); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| it("emits a warning when rewrites contain external destinations", async () => { | ||||||||||||||
| const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); | ||||||||||||||
|
|
||||||||||||||
| await resolveNextConfig({ | ||||||||||||||
| rewrites: async () => [ | ||||||||||||||
| { source: "/api/:path*", destination: "https://api.example.com/:path*" }, | ||||||||||||||
| { source: "/internal", destination: "/other" }, | ||||||||||||||
| ], | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| // Should have warned about the external rewrite | ||||||||||||||
| const externalWarning = warn.mock.calls.find( | ||||||||||||||
| (call) => typeof call[0] === "string" && call[0].includes("external rewrite"), | ||||||||||||||
| ); | ||||||||||||||
| expect(externalWarning).toBeDefined(); | ||||||||||||||
| expect(externalWarning![0]).toContain("1 external rewrite that"); | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: The assertion |
||||||||||||||
| expect(externalWarning![0]).toContain("https://api.example.com/:path*"); | ||||||||||||||
| expect(externalWarning![0]).toContain("Credential headers"); | ||||||||||||||
| expect(externalWarning![0]).toContain("stripped"); | ||||||||||||||
| expect(externalWarning![0]).toContain("/api/:path*"); | ||||||||||||||
| expect(externalWarning![0]).toContain("→"); | ||||||||||||||
|
|
||||||||||||||
| // The internal rewrite should not appear in the warning | ||||||||||||||
| expect(externalWarning![0]).not.toContain("/other"); | ||||||||||||||
|
Comment on lines
+615
to
+628
|
||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| it("does not warn when all rewrites are internal", async () => { | ||||||||||||||
| const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); | ||||||||||||||
|
|
||||||||||||||
| await resolveNextConfig({ | ||||||||||||||
| rewrites: async () => [ | ||||||||||||||
| { source: "/old", destination: "/new" }, | ||||||||||||||
| { source: "/a", destination: "/b" }, | ||||||||||||||
| ], | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| const externalWarning = warn.mock.calls.find( | ||||||||||||||
| (call) => typeof call[0] === "string" && call[0].includes("external rewrite"), | ||||||||||||||
| ); | ||||||||||||||
| expect(externalWarning).toBeUndefined(); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| it("warns about multiple external rewrites across beforeFiles, afterFiles, and fallback", async () => { | ||||||||||||||
| const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); | ||||||||||||||
|
|
||||||||||||||
| await resolveNextConfig({ | ||||||||||||||
| rewrites: async () => ({ | ||||||||||||||
| beforeFiles: [{ source: "/proxy1", destination: "https://one.example.com/api" }], | ||||||||||||||
| afterFiles: [{ source: "/proxy2", destination: "https://two.example.com/api" }], | ||||||||||||||
| fallback: [{ source: "/proxy3", destination: "https://three.example.com/api" }], | ||||||||||||||
| }), | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| const externalWarning = warn.mock.calls.find( | ||||||||||||||
| (call) => typeof call[0] === "string" && call[0].includes("external rewrite"), | ||||||||||||||
| ); | ||||||||||||||
| expect(externalWarning).toBeDefined(); | ||||||||||||||
| expect(externalWarning![0]).toContain("3 external rewrites"); | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same nit as above — this correctly tests the plural form, but the assertion
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: consider also asserting that all three sources appear in the listing, to verify that
Suggested change
The destination URLs are already asserted, but asserting the source paths too ensures the |
||||||||||||||
| expect(externalWarning![0]).toContain("https://one.example.com/api"); | ||||||||||||||
| expect(externalWarning![0]).toContain("https://two.example.com/api"); | ||||||||||||||
| expect(externalWarning![0]).toContain("https://three.example.com/api"); | ||||||||||||||
| expect(externalWarning![0]).toContain("/proxy1"); | ||||||||||||||
| expect(externalWarning![0]).toContain("/proxy2"); | ||||||||||||||
| expect(externalWarning![0]).toContain("/proxy3"); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| it("does not warn when no rewrites are configured", async () => { | ||||||||||||||
| const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); | ||||||||||||||
|
|
||||||||||||||
| await resolveNextConfig({ env: {} }); | ||||||||||||||
|
|
||||||||||||||
| const externalWarning = warn.mock.calls.find( | ||||||||||||||
| (call) => typeof call[0] === "string" && call[0].includes("external rewrite"), | ||||||||||||||
| ); | ||||||||||||||
| expect(externalWarning).toBeUndefined(); | ||||||||||||||
| }); | ||||||||||||||
| }); | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4162,7 +4162,7 @@ describe("proxyExternalRequest", () => { | |
| } | ||
| }); | ||
|
|
||
| it("forwards credential headers and strips x-middleware-* headers from proxied requests", async () => { | ||
| it("strips credential and x-middleware-* headers from proxied requests", async () => { | ||
| const { proxyExternalRequest } = | ||
| await import("../packages/vinext/src/config/config-matchers.js"); | ||
|
|
||
|
|
@@ -4190,11 +4190,11 @@ describe("proxyExternalRequest", () => { | |
| try { | ||
| await proxyExternalRequest(request, "https://api.example.com/data"); | ||
| expect(capturedHeaders).toBeDefined(); | ||
| // Credential headers must be forwarded (matching Next.js behavior) | ||
| expect(capturedHeaders!.get("cookie")).toBe("session=secret123"); | ||
| expect(capturedHeaders!.get("authorization")).toBe("Bearer tok_secret"); | ||
| expect(capturedHeaders!.get("x-api-key")).toBe("sk_live_secret"); | ||
| expect(capturedHeaders!.get("proxy-authorization")).toBe("Basic cHJveHk="); | ||
| // Credential headers must be stripped to prevent leaking secrets | ||
| expect(capturedHeaders!.get("cookie")).toBeNull(); | ||
| expect(capturedHeaders!.get("authorization")).toBeNull(); | ||
| expect(capturedHeaders!.get("x-api-key")).toBeNull(); | ||
| expect(capturedHeaders!.get("proxy-authorization")).toBeNull(); | ||
| // Internal middleware headers must be stripped | ||
| expect(capturedHeaders!.get("x-middleware-rewrite")).toBeNull(); | ||
| expect(capturedHeaders!.get("x-middleware-next")).toBeNull(); | ||
|
|
@@ -6988,6 +6988,24 @@ describe("handleImageOptimization", () => { | |
| expect(response.headers.get("Content-Disposition")).toBe("attachment"); | ||
| }); | ||
|
|
||
| it("defaults Content-Disposition to inline when contentDispositionType is invalid", async () => { | ||
| const { handleImageOptimization } = | ||
| await import("../packages/vinext/src/server/image-optimization.js"); | ||
| const request = new Request("http://localhost/_vinext/image?url=%2Fimg.jpg&w=800"); | ||
| const handlers = { | ||
| fetchAsset: async () => | ||
| new Response("image-data", { | ||
| status: 200, | ||
| headers: { "Content-Type": "image/jpeg" }, | ||
| }), | ||
| }; | ||
| const response = await handleImageOptimization(request, handlers, undefined, { | ||
| contentDispositionType: "bogus" as "inline", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good addition — testing the sanitization boundary with an invalid value cast via
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good test — this directly exercises the sanitization boundary. The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good test. The |
||
| }); | ||
| expect(response.status).toBe(200); | ||
| expect(response.headers.get("Content-Disposition")).toBe("inline"); | ||
| }); | ||
|
|
||
| it("applies custom contentSecurityPolicy", async () => { | ||
| const { handleImageOptimization } = | ||
| await import("../packages/vinext/src/server/image-optimization.js"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
CREDENTIAL_HEADERSis re-allocated on every call toproxyExternalRequest. Since this is a constant, consider hoisting it to module scope (similar toHOP_BY_HOP_HEADERSat line 161) so it's allocated once:(moved to module level, and the inline
conston line 957 removed)Not a correctness issue — just a minor consistency/efficiency thing.