Skip to content

fix: sanitize asset filenames#9737

Merged
patak-cat merged 3 commits intovitejs:mainfrom
dominikg:fix/sanitize-asset-filenames
Aug 19, 2022
Merged

fix: sanitize asset filenames#9737
patak-cat merged 3 commits intovitejs:mainfrom
dominikg:fix/sanitize-asset-filenames

Conversation

@dominikg
Copy link
Copy Markdown
Contributor

@dominikg dominikg commented Aug 18, 2022

Description

Vite supplies a custom assetFileName to rollup, which means that rollup itself won't sanitize it further.

This PR copies the default sanitize function from rollup and applies it to the name.

fixes sveltejs/kit#5843

Additional context

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@dominikg
Copy link
Copy Markdown
Contributor Author

dominikg commented Aug 18, 2022 •
edited
Loading

Ideally the test would test with a .css file too, someone has ideas how to get vite to not bundle-up all css?

There is a small chance for differences w rollup if someone were to set a custom sanitizer in rollup options, but that sits in the danger zone, so this fix is still better than leaving asset names unsanitized.

@benmccann benmccann added bug p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels Aug 18, 2022
benmccann
benmccann previously approved these changes Aug 18, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

thanks for figuring this out!!

@dominikg
Copy link
Copy Markdown
Contributor Author

dominikg commented Aug 18, 2022

Needs an additional test for collision resilience

@dominikg
Copy link
Copy Markdown
Contributor Author

dominikg commented Aug 18, 2022

Collisions are no problem as long as asset hashing isn't turned off. If one would do that, +circle.svg and _circle.svg would get the same assetFileName.

@dominikg
Copy link
Copy Markdown
Contributor Author

dominikg commented Aug 18, 2022

with asset hashing disabled, vite build does complete with overriding assets of the same name, but logs that it did.

vite v3.0.8 building for production...
✓ 6 modules transformed.
The emitted file "_circle.svg" overwrites a previously emitted file of the same name.
dist/_circle.svg                0.13 KiB
dist/index.html                 0.33 KiB
dist/manifest.json              0.22 KiB
dist/assets/index.8dc79d4c.js   1.83 KiB / gzip: 0.69 KiB

I think that is still acceptable because not sanitizing asset filenames at all is a larger risk/problem.

patak-cat
patak-cat reviewed Aug 19, 2022
View reviewed changes
packages/vite/src/node/plugins/asset.ts

case '[name]':
return name
return sanitizeFileName(name)
Copy link
Copy Markdown
Member

@patak-cat patak-cat Aug 19, 2022

Choose a reason for hiding this comment

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

Shouldn't we use output.sanitizeFileName here to support custom functions by the user? (and also avoid the need to replicate the default in our code)
https://rollupjs.org/guide/en/#outputsanitizefilename

Copy link
Copy Markdown
Member

@patak-cat patak-cat Aug 19, 2022

Choose a reason for hiding this comment

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

Discussing with @dominikg in Vite Land, we don't have access to Rollup's default. We can also leave support for custom sanitize functions for another PR. I think we can merge this PR and then work on extending support later.

@patak-cat patak-cat added this to the 3.1 milestone Aug 19, 2022
@patak-cat patak-cat merged commit 2f468bb into vitejs:main Aug 19, 2022
@nickmenow nickmenow mentioned this pull request Aug 20, 2022
Closed
@underfin underfin mentioned this pull request Aug 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patak-cat patak-cat patak-cat approved these changes

@benmccann benmccann benmccann left review comments

Assignees

No one assigned

Labels

p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)

Projects

None yet

Milestone

3.1

Development

Successfully merging this pull request may close these issues.

Invalid CSS file name caused by brackets and +

3 participants

@dominikg @benmccann @patak-cat