From ef2b8f8696d7e93bd69945f0f1b5acedb0a5298c Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Wed, 10 May 2023 01:31:06 +0200 Subject: [PATCH] Fix canonical url for dynamic routes (#49512) The `pathname` in app-render was actually "page" (e.g. `/blog/[slug]`), with special url conventions. Instead we should use actual request url as pathname. Rename previous `pathname` arg to `pagePath` for better readability Also improved the url validation --- .../next/src/server/app-render/app-render.tsx | 25 +++++++++---------- .../src/server/app-render/validate-url.tsx | 13 +++++++--- .../app/alternates/child/[slug]/page.tsx | 3 +++ test/e2e/app-dir/metadata/metadata.test.ts | 5 ++++ test/unit/validate-url.test.ts | 13 ++++++++++ 5 files changed, 43 insertions(+), 16 deletions(-) create mode 100644 test/e2e/app-dir/metadata/app/alternates/child/[slug]/page.tsx create mode 100644 test/unit/validate-url.test.ts diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index 6a21eda6f013b..f1a9c2f5a6fab 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -138,11 +138,12 @@ function findDynamicParamFromRouterState( export async function renderToHTMLOrFlight( req: IncomingMessage, res: ServerResponse, - pathname: string, + pagePath: string, query: NextParsedUrlQuery, renderOpts: RenderOpts ): Promise { const isFlight = req.headers[RSC.toLowerCase()] !== undefined + const pathname = validateURL(req.url) const { buildManifest, @@ -699,7 +700,7 @@ export async function renderToHTMLOrFlight( !isValidElementType(Component) ) { throw new Error( - `The default export is not a React Component in page: "${pathname}"` + `The default export is not a React Component in page: "${page}"` ) } @@ -1179,7 +1180,7 @@ export async function renderToHTMLOrFlight( injectedCSS: new Set(), injectedFontPreloadTags: new Set(), rootLayoutIncluded: false, - asNotFound: pathname === '/404' || options?.asNotFound, + asNotFound: pagePath === '/404' || options?.asNotFound, }) ).map((path) => path.slice(1)) // remove the '' (root) segment @@ -1216,8 +1217,6 @@ export async function renderToHTMLOrFlight( Uint8Array > = new TransformStream() - const initialCanonicalUrl = validateURL(req.url) - // Get the nonce from the incoming request if it has one. const csp = req.headers['content-security-policy'] let nonce: string | undefined @@ -1297,7 +1296,7 @@ export async function renderToHTMLOrFlight( {styles} @@ -1361,13 +1360,13 @@ export async function renderToHTMLOrFlight( ) } - getTracer().getRootSpanAttributes()?.set('next.route', pathname) + getTracer().getRootSpanAttributes()?.set('next.route', pagePath) const bodyResult = getTracer().wrap( AppRenderSpan.getBodyResult, { - spanName: `render route (app) ${pathname}`, + spanName: `render route (app) ${pagePath}`, attributes: { - 'next.route': pathname, + 'next.route': pagePath, }, }, async ({ @@ -1497,8 +1496,8 @@ export async function renderToHTMLOrFlight( } if (err.digest === NEXT_DYNAMIC_NO_SSR_CODE) { warn( - `Entire page ${pathname} deopted into client-side rendering. https://nextjs.org/docs/messages/deopted-into-client-rendering`, - pathname + `Entire page ${pagePath} deopted into client-side rendering. https://nextjs.org/docs/messages/deopted-into-client-rendering`, + pagePath ) } if (isNotFoundError(err)) { @@ -1571,7 +1570,7 @@ export async function renderToHTMLOrFlight( const renderResult = new RenderResult( await bodyResult({ - asNotFound: pathname === '/404', + asNotFound: pagePath === '/404', }) ) @@ -1625,7 +1624,7 @@ export async function renderToHTMLOrFlight( () => StaticGenerationAsyncStorageWrapper.wrap( staticGenerationAsyncStorage, - { pathname, renderOpts }, + { pathname: pagePath, renderOpts }, () => wrappedRender() ) ) diff --git a/packages/next/src/server/app-render/validate-url.tsx b/packages/next/src/server/app-render/validate-url.tsx index c465690cf8af0..f43b1061b4398 100644 --- a/packages/next/src/server/app-render/validate-url.tsx +++ b/packages/next/src/server/app-render/validate-url.tsx @@ -1,11 +1,18 @@ +const DUMMY_ORIGIN = 'http://n' +const INVALID_URL_MESSAGE = 'Invalid request URL' + export function validateURL(url: string | undefined): string { if (!url) { - throw new Error('Invalid request URL') + throw new Error(INVALID_URL_MESSAGE) } try { - new URL(url, 'http://n') + const parsed = new URL(url, DUMMY_ORIGIN) + // Avoid origin change by extra slashes in pathname + if (parsed.origin !== DUMMY_ORIGIN) { + throw new Error(INVALID_URL_MESSAGE) + } return url } catch { - throw new Error('Invalid request URL') + throw new Error(INVALID_URL_MESSAGE) } } diff --git a/test/e2e/app-dir/metadata/app/alternates/child/[slug]/page.tsx b/test/e2e/app-dir/metadata/app/alternates/child/[slug]/page.tsx new file mode 100644 index 0000000000000..d323469af8cbd --- /dev/null +++ b/test/e2e/app-dir/metadata/app/alternates/child/[slug]/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return 'alternate-child-dynamic-slug' +} diff --git a/test/e2e/app-dir/metadata/metadata.test.ts b/test/e2e/app-dir/metadata/metadata.test.ts index 4e35dbf9e767d..a0d3d6f5cd3c4 100644 --- a/test/e2e/app-dir/metadata/metadata.test.ts +++ b/test/e2e/app-dir/metadata/metadata.test.ts @@ -337,6 +337,11 @@ createNextDescribe( rel: 'alternate', href: 'https://example.com/alternates/child/de-DE', }) + + await browser.loadPage(next.url + '/alternates/child/123') + await matchDom('link', 'rel="canonical"', { + href: 'https://example.com/alternates/child/123', + }) }) it('should support robots tags', async () => { diff --git a/test/unit/validate-url.test.ts b/test/unit/validate-url.test.ts new file mode 100644 index 0000000000000..a82e9fe985ab1 --- /dev/null +++ b/test/unit/validate-url.test.ts @@ -0,0 +1,13 @@ +import { validateURL } from 'next/dist/server/app-render/validate-url' + +describe('validateUrl', () => { + it('should return valid pathname', () => { + expect(validateURL('/')).toBe('/') + expect(validateURL('/abc')).toBe('/abc') + }) + + it('should throw for invalid pathname', () => { + expect(() => validateURL('//**y/\\')).toThrow() + expect(() => validateURL('//google.com')).toThrow() + }) +})