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: sanitize asset filenames #9737

Merged
merged 3 commits into from
Aug 19, 2022

Conversation

dominikg
Copy link
Contributor

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
Contributor Author

dominikg commented Aug 18, 2022

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
Copy link
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
Contributor Author

Needs an additional test for collision resilience

@dominikg
Copy link
Contributor Author

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
Contributor Author

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.

@@ -324,7 +324,7 @@ export function assetFileNamesToFileName(
return hash

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

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
Member

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-dev patak-dev added this to the 3.1 milestone Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid CSS file name caused by brackets and +
3 participants