From 531cdb591ee5c7a641a9db3f3fc5521be3beffd3 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Fri, 8 Mar 2024 21:16:16 +0100 Subject: [PATCH] Fix metadata url cases should not append with trailing slash (#63050) ### What Exclude the cases like external urls and relative urls with query from appending trailing slash when it's needed. The process is: - If it's a uncertain string path (relative url, could start with `'./'` or `/`), convert to relative that starts with `/`; - then we covert the url (string or URL) to string url - We do the check if we need to append the trailing slash ### Why In #62109 we added functionality that can only append trailing slash when we appended trailing slash to some metadata url like `canonical` url and open graph url when the config is enabled. For urls with queries, the trailing slash can also be omitted. For the external urls (different origin comparing to `metadataBase`) we don't need to append trailing slash as they're not the same web app. x-ref: [slack thread](https://vercel.slack.com/archives/C0676QZBWKS/p1709845946033929) Closes NEXT-2762 Closes NEXT-2753 --- .../metadata/resolvers/resolve-url.test.ts | 66 ++++++++++++++++++- .../src/lib/metadata/resolvers/resolve-url.ts | 35 +++++++--- .../trailingslash/app/metadata/page.js | 14 ++++ .../trailingslash/trailingslash.test.ts | 10 +++ 4 files changed, 116 insertions(+), 9 deletions(-) create mode 100644 test/e2e/app-dir/trailingslash/app/metadata/page.js diff --git a/packages/next/src/lib/metadata/resolvers/resolve-url.test.ts b/packages/next/src/lib/metadata/resolvers/resolve-url.test.ts index 532440d0c74a5..ca0478b798dc0 100644 --- a/packages/next/src/lib/metadata/resolvers/resolve-url.test.ts +++ b/packages/next/src/lib/metadata/resolvers/resolve-url.test.ts @@ -1,4 +1,4 @@ -import { resolveUrl } from './resolve-url' +import { resolveUrl, resolveAbsoluteUrlWithPathname } from './resolve-url' // required to be resolved as URL with resolveUrl() describe('metadata: resolveUrl', () => { @@ -42,3 +42,67 @@ describe('metadata: resolveUrl', () => { ) }) }) + +describe('resolveAbsoluteUrlWithPathname', () => { + describe('trailingSlash is false', () => { + const metadataBase = new URL('https://example.com/') + const opts = { + trailingSlash: false, + pathname: '/', + } + const resolver = (url: string | URL) => + resolveAbsoluteUrlWithPathname(url, metadataBase, opts) + it('should resolve absolute internal url', () => { + expect(resolver('https://example.com/foo')).toBe( + 'https://example.com/foo' + ) + }) + }) + + describe('trailingSlash is true', () => { + const metadataBase = new URL('https://example.com/') + const opts = { + trailingSlash: true, + pathname: '/', + } + const resolver = (url: string | URL) => + resolveAbsoluteUrlWithPathname(url, metadataBase, opts) + it('should add trailing slash to relative url', () => { + expect(resolver('/foo')).toBe('https://example.com/foo/') + }) + + it('should add trailing slash to absolute internal url', () => { + expect(resolver('https://example.com/foo')).toBe( + 'https://example.com/foo/' + ) + expect(resolver(new URL('https://example.com/foo'))).toBe( + 'https://example.com/foo/' + ) + }) + + it('should not add trailing slash to external url', () => { + expect(resolver('https://external.org/foo')).toBe( + 'https://external.org/foo' + ) + expect(resolver(new URL('https://external.org/foo'))).toBe( + 'https://external.org/foo' + ) + }) + + it('should not add trailing slash to absolute internal url with query', () => { + expect(resolver('https://example.com/foo?bar')).toBe( + 'https://example.com/foo?bar' + ) + expect(resolver(new URL('https://example.com/foo?bar'))).toBe( + 'https://example.com/foo?bar' + ) + }) + + it('should not add trailing slash to relative url with query', () => { + expect(resolver('/foo?bar')).toBe('https://example.com/foo?bar') + expect(resolver(new URL('/foo?bar', metadataBase))).toBe( + 'https://example.com/foo?bar' + ) + }) + }) +}) diff --git a/packages/next/src/lib/metadata/resolvers/resolve-url.ts b/packages/next/src/lib/metadata/resolvers/resolve-url.ts index 8ddc6aa85c36c..49c888a40edd7 100644 --- a/packages/next/src/lib/metadata/resolvers/resolve-url.ts +++ b/packages/next/src/lib/metadata/resolvers/resolve-url.ts @@ -82,21 +82,40 @@ function resolveAbsoluteUrlWithPathname( metadataBase: URL | null, { trailingSlash, pathname }: MetadataContext ): string { + // Resolve url with pathname that always starts with `/` url = resolveRelativeUrl(url, pathname) - // Get canonicalUrl without trailing slash - let canonicalUrl = '' + // Convert string url or URL instance to absolute url string, + // if there's case needs to be resolved with metadataBase + let resolvedUrl = '' const result = metadataBase ? resolveUrl(url, metadataBase) : url if (typeof result === 'string') { - canonicalUrl = result + resolvedUrl = result } else { - canonicalUrl = result.pathname === '/' ? result.origin : result.href + resolvedUrl = result.pathname === '/' ? result.origin : result.href } - // Add trailing slash if it's enabled - return trailingSlash && !canonicalUrl.endsWith('/') - ? `${canonicalUrl}/` - : canonicalUrl + // Add trailing slash if it's enabled for urls matches the condition + // - Not external, same origin with metadataBase + // - Doesn't have query + if (trailingSlash && !resolvedUrl.endsWith('/')) { + let isRelative = resolvedUrl.startsWith('/') + let isExternal = false + let hasQuery = resolvedUrl.includes('?') + if (!isRelative) { + try { + const parsedUrl = new URL(resolvedUrl) + isExternal = + metadataBase != null && parsedUrl.origin !== metadataBase.origin + } catch { + // If it's not a valid URL, treat it as external + isExternal = true + } + if (!isExternal && !hasQuery) return `${resolvedUrl}/` + } + } + + return resolvedUrl } export { diff --git a/test/e2e/app-dir/trailingslash/app/metadata/page.js b/test/e2e/app-dir/trailingslash/app/metadata/page.js new file mode 100644 index 0000000000000..5b86d09b10339 --- /dev/null +++ b/test/e2e/app-dir/trailingslash/app/metadata/page.js @@ -0,0 +1,14 @@ +export default function Page() { + return 'page' +} + +export const metadata = { + openGraph: { + // external url with different domain + url: 'http://trailingslash-another.com/metadata', + }, + alternates: { + // relative url with query string + canonical: '/metadata?query=string', + }, +} diff --git a/test/e2e/app-dir/trailingslash/trailingslash.test.ts b/test/e2e/app-dir/trailingslash/trailingslash.test.ts index 2d71308630f4e..b1731aa254589 100644 --- a/test/e2e/app-dir/trailingslash/trailingslash.test.ts +++ b/test/e2e/app-dir/trailingslash/trailingslash.test.ts @@ -48,5 +48,15 @@ createNextDescribe( .waitForElementByCss('#a-page') expect(await browser.waitForElementByCss('#a-page').text()).toBe('A page') }) + + it('should not add trailing slash to external url or relative url with query', async () => { + const $ = await next.render$('/metadata') + expect($('[rel="canonical"]').attr('href')).toBe( + 'http://trailingslash.com/metadata?query=string' + ) + expect($('[property="og:url"]').attr('content')).toBe( + 'http://trailingslash-another.com/metadata' + ) + }) } )