Skip to content

Commit

Permalink
Fix canonical url for dynamic routes (#49512)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
huozhi committed May 9, 2023
1 parent 064e0c2 commit ef2b8f8
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 16 deletions.
25 changes: 12 additions & 13 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,12 @@ function findDynamicParamFromRouterState(
export async function renderToHTMLOrFlight(
req: IncomingMessage,
res: ServerResponse,
pathname: string,
pagePath: string,
query: NextParsedUrlQuery,
renderOpts: RenderOpts
): Promise<RenderResult> {
const isFlight = req.headers[RSC.toLowerCase()] !== undefined
const pathname = validateURL(req.url)

const {
buildManifest,
Expand Down Expand Up @@ -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}"`
)
}

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1297,7 +1296,7 @@ export async function renderToHTMLOrFlight(
{styles}
<AppRouter
assetPrefix={assetPrefix}
initialCanonicalUrl={initialCanonicalUrl}
initialCanonicalUrl={pathname}
initialTree={initialTree}
initialHead={
<>
Expand Down Expand Up @@ -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 ({
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -1571,7 +1570,7 @@ export async function renderToHTMLOrFlight(

const renderResult = new RenderResult(
await bodyResult({
asNotFound: pathname === '/404',
asNotFound: pagePath === '/404',
})
)

Expand Down Expand Up @@ -1625,7 +1624,7 @@ export async function renderToHTMLOrFlight(
() =>
StaticGenerationAsyncStorageWrapper.wrap(
staticGenerationAsyncStorage,
{ pathname, renderOpts },
{ pathname: pagePath, renderOpts },
() => wrappedRender()
)
)
Expand Down
13 changes: 10 additions & 3 deletions packages/next/src/server/app-render/validate-url.tsx
Original file line number Diff line number Diff line change
@@ -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)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return 'alternate-child-dynamic-slug'
}
5 changes: 5 additions & 0 deletions test/e2e/app-dir/metadata/metadata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
13 changes: 13 additions & 0 deletions test/unit/validate-url.test.ts
Original file line number Diff line number Diff line change
@@ -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()
})
})

0 comments on commit ef2b8f8

Please sign in to comment.