-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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(#53190): add missing crossOrigin to assetsPrefix resources #56311
Changes from 2 commits
b20b130
b1772a3
51c5033
3081081
8ebab8f
73ce3f9
a344c2d
dbda43d
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 |
---|---|---|
|
@@ -5,24 +5,35 @@ import ReactDOM from 'react-dom' | |
export function getRequiredScripts( | ||
buildManifest: BuildManifest, | ||
assetPrefix: string, | ||
crossOrigin: string | undefined, | ||
SRIManifest: undefined | Record<string, string>, | ||
qs: string, | ||
nonce: string | undefined | ||
): [() => void, string | { src: string; integrity: string }] { | ||
): [ | ||
() => void, | ||
{ src: string; integrity?: string; crossOrigin?: string | undefined } | ||
] { | ||
let preinitScripts: () => void | ||
let preinitScriptCommands: string[] = [] | ||
let bootstrapScript: string | { src: string; integrity: string } = '' | ||
const bootstrapScript: { | ||
src: string | ||
integrity?: string | ||
crossOrigin?: string | undefined | ||
} = { | ||
src: '', | ||
crossOrigin, | ||
} | ||
Comment on lines
+18
to
+25
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. Although the |
||
|
||
const files = buildManifest.rootMainFiles | ||
if (files.length === 0) { | ||
throw new Error( | ||
'Invariant: missing bootstrap script. This is a bug in Next.js' | ||
) | ||
} | ||
if (SRIManifest) { | ||
bootstrapScript = { | ||
src: `${assetPrefix}/_next/` + files[0] + qs, | ||
integrity: SRIManifest[files[0]], | ||
} | ||
bootstrapScript.src = `${assetPrefix}/_next/` + files[0] + qs | ||
bootstrapScript.integrity = SRIManifest[files[0]] | ||
|
||
for (let i = 1; i < files.length; i++) { | ||
const src = `${assetPrefix}/_next/` + files[i] + qs | ||
const integrity = SRIManifest[files[i]] | ||
|
@@ -34,12 +45,14 @@ export function getRequiredScripts( | |
ReactDOM.preinit(preinitScriptCommands[i], { | ||
as: 'script', | ||
integrity: preinitScriptCommands[i + 1], | ||
crossOrigin, | ||
nonce, | ||
}) | ||
} | ||
} | ||
} else { | ||
bootstrapScript = `${assetPrefix}/_next/` + files[0] + qs | ||
bootstrapScript.src = `${assetPrefix}/_next/` + files[0] + qs | ||
|
||
for (let i = 1; i < files.length; i++) { | ||
const src = `${assetPrefix}/_next/` + files[i] + qs | ||
preinitScriptCommands.push(src) | ||
|
@@ -50,6 +63,7 @@ export function getRequiredScripts( | |
ReactDOM.preinit(preinitScriptCommands[i], { | ||
as: 'script', | ||
nonce, | ||
crossOrigin, | ||
}) | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,7 +101,7 @@ export type ChildProp = { | |
segment: Segment | ||
} | ||
|
||
export type RenderOptsPartial = { | ||
export interface RenderOptsPartial { | ||
err?: Error | null | ||
dev?: boolean | ||
buildId: string | ||
|
@@ -111,6 +111,7 @@ export type RenderOptsPartial = { | |
runtime?: ServerRuntime | ||
serverComponents?: boolean | ||
assetPrefix?: string | ||
crossOrigin?: '' | 'anonymous' | 'use-credentials' | undefined | ||
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.
|
||
nextFontManifest?: NextFontManifest | ||
isBot?: boolean | ||
incrementalCache?: import('../lib/incremental-cache').IncrementalCache | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
export const metadata = { | ||
title: 'Next.js', | ||
description: 'Generated by Next.js', | ||
} | ||
|
||
export default function RootLayout({ children }) { | ||
return ( | ||
<html lang="en"> | ||
<body>{children}</body> | ||
</html> | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export default function Index(props) { | ||
return <p id="title">IndexPage</p> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
module.exports = { | ||
/** | ||
* The "assetPrefix" here doesn't needs to be real as we doesn't load the page in the browser in this test, | ||
* we only care about if all assets prefixed with the "assetPrefix" are having correct "crossOrigin". | ||
*/ | ||
assetPrefix: 'https://example.vercel.sh', | ||
|
||
/** | ||
* According to HTML5 Spec (https://html.spec.whatwg.org/multipage/urls-and-fetching.html#cors-settings-attributes), | ||
* crossorigin="" and crossorigin="anonymous" has the same effect. And ReactDOM's preload methods (preload, preconnect, etc.) | ||
* will prefer crossorigin="" to save bytes. | ||
* | ||
* So we use "use-credentials" here for easier testing. | ||
*/ | ||
crossOrigin: 'use-credentials', | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/* eslint-env jest */ | ||
import { join } from 'path' | ||
import { | ||
killApp, | ||
findPort, | ||
launchApp, | ||
renderViaHTTP, | ||
File, | ||
} from 'next-test-utils' | ||
import cheerio from 'cheerio' | ||
|
||
const appDir = join(__dirname, '../') | ||
|
||
describe('App crossOrigin config', () => { | ||
let appPort | ||
let app | ||
|
||
it('should render correctly with assetPrefix: "/"', async () => { | ||
try { | ||
appPort = await findPort() | ||
app = await launchApp(appDir, appPort) | ||
|
||
const html = await renderViaHTTP(appPort, '/') | ||
|
||
const $ = cheerio.load(html) | ||
|
||
// Only potential external (assetPrefix) <script /> and <link /> should have crossorigin attribute | ||
$( | ||
'script[src*="https://example.vercel.sh"], link[href*="https://example.vercel.sh"]' | ||
).each((_, el) => { | ||
const crossOrigin = $(el).attr('crossorigin') | ||
expect(crossOrigin).toBe('use-credentials') | ||
}) | ||
|
||
// Inline <script /> (including RSC payload) and <link /> should not have crossorigin attribute | ||
$('script:not([src]), link:not([href])').each((_, el) => { | ||
const crossOrigin = $(el).attr('crossorigin') | ||
expect(crossOrigin).toBeUndefined() | ||
}) | ||
|
||
// Same origin <script /> and <link /> should not have crossorigin attribute either | ||
$('script[src^="/"], link[href^="/"]').each((_, el) => { | ||
const crossOrigin = $(el).attr('crossorigin') | ||
expect(crossOrigin).toBeUndefined() | ||
}) | ||
} finally { | ||
killApp(app) | ||
} | ||
}) | ||
}) |
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.
Here I simplify the handling of
polyfills
by making its type asJSX.IntrinsicElements['script'][]
, so it can be spread ({...polyfill}
) on the JSX later.