Skip to content
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

Fix canonical url for dynamic routes #49512

Merged
merged 6 commits into from
May 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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()
})
})