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(deps): upgrade rollup 4.22.4+ to ensure avoiding XSS #18180

Merged
merged 8 commits into from
Sep 30, 2024

Conversation

delihiros
Copy link
Contributor

Description

rollup before version 4.22.4 has a DOM Clobbering vulnerability which leads to XSS.
Considering the risk, we would like to upgrade the version.

Copy link

stackblitz bot commented Sep 24, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@delihiros
Copy link
Contributor Author

delihiros commented Sep 24, 2024

Hi, I've resolved the conflict. Could you please review it again so it can be merged? Thank you.

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait until rollup/rollup#5672 is fixed, otherwise this will break many apps. (note that the CVE only affects non-ESM outputs)

For now, you can simply bump the transitive dependency on your side.

package.json Outdated Show resolved Hide resolved
packages/vite/package.json Outdated Show resolved Hide resolved
@delihiros
Copy link
Contributor Author

The type definition has changed in estree 1.0.6, and vite needs to be addressed in order to successfully build the commit.

You can view the relevant commit here: DefinitelyTyped Commit.

One possible solution to this issue is to use a type assertion, as I understand that the local variable is of type Identifier.
We could modify the code in vite/packages/vite/src/node/plugins/importAnalysis.ts as follows:

  const importedName = (spec.local as Identifier).name;

@bluwy
Copy link
Member

bluwy commented Sep 29, 2024

In case it's not clear for those blocked by this, you can already use your package manager to update Vite's Rollup dependency transitively, you don't need Vite to bump it for the version to be bumped in your project, so this shouldn't block anyone.

@sapphi-red
Copy link
Member

The type definition has changed in estree 1.0.6, and vite needs to be addressed in order to successfully build the commit.

You can view the relevant commit here: DefinitelyTyped Commit.

One possible solution to this issue is to use a type assertion, as I understand that the local variable is of type Identifier. We could modify the code in vite/packages/vite/src/node/plugins/importAnalysis.ts as follows:

  const importedName = (spec.local as Identifier).name;

For this PR, I think this change is fine to focus on updating rollup.

@sapphi-red sapphi-red changed the title fix: upgrade rollup 4.22.4 to avoid XSS fix(deps): upgrade rollup 4.22.4 to avoid XSS Sep 30, 2024
@sapphi-red sapphi-red changed the title fix(deps): upgrade rollup 4.22.4 to avoid XSS fix(deps): upgrade rollup 4.22.4+ to avoid XSS Sep 30, 2024
@sapphi-red sapphi-red added the dependencies Pull requests that update a dependency file label Sep 30, 2024
@sapphi-red sapphi-red changed the title fix(deps): upgrade rollup 4.22.4+ to avoid XSS fix(deps): upgrade rollup 4.22.4+ to ensure avoiding XSS Sep 30, 2024
@sapphi-red sapphi-red merged commit ea1d0b9 into vitejs:main Sep 30, 2024
15 checks passed
@delihiros delihiros deleted the fix/rollup-4.22.4 branch September 30, 2024 05:43
@BreakBB
Copy link

BreakBB commented Nov 11, 2024

Are there any plans to cherry-pick this to a v5 update? Currently the vulnerability is only fixed in the v6 beta versions.

@sapphi-red
Copy link
Member

No, because it's not necessary to do so.
Quoting the message above:

In case it's not clear for those blocked by this, you can already use your package manager to update Vite's Rollup dependency transitively, you don't need Vite to bump it for the version to be bumped in your project, so this shouldn't block anyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants