-
Notifications
You must be signed in to change notification settings - Fork 28.7k
fix(#53190): add missing crossOrigin to assetsPrefix resources #56311
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
Changes from all 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,7 @@ | ||
export default function RootLayout({ children }) { | ||
return ( | ||
<html> | ||
<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,37 @@ | ||
import { createNextDescribe } from 'e2e-utils' | ||
|
||
createNextDescribe( | ||
'app dir - crossOrigin config', | ||
{ | ||
files: __dirname, | ||
skipDeployment: true, | ||
}, | ||
({ next, isNextStart }) => { | ||
if (isNextStart) { | ||
it('skip in start mode', () => {}) | ||
return | ||
} | ||
it('should render correctly with assetPrefix: "/"', async () => { | ||
const $ = await next.render$('/') | ||
// 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() | ||
}) | ||
}) | ||
} | ||
) |
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', | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
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.