fix(build): decode urls in CSS files (fix #15109) by ndv99 · Pull Request #15246 · vitejs/vite

ndv99 · 2023-12-05T11:45:28Z

Description

Call decodeURI in urlReplacer in the CSS plugin to decode CSS urls

Additional context

Previously, url() calls in CSS were not decoded at build time. Web browsers do decode this call, so any url() call with an encoded string as a parameter would not resolve at build time, resulting in the following error:

`\n${url} referenced in ${id} didn't resolve at build time, it will remain unchanged to be resolved at runtime`

This is important for some font files, which contain special characters in their names for variable weight/width. This PR fixes this by calling decodeURI at the top of urlReplacer, converting characters like %5B and %5D into their actual characters [ and ] respectively.

More information on this bug can be found here: #15109

QA steps

Clone this branch and go to packages/vite, and run pnpm run dev
Create a new Vite project in a different directory (I tested with React + TypeScript)
Follow the instructions in CONTRIBUTING.MD to link this branch to the new Vite project
Create a subfolder fonts and place a font file inside, ensuring the name contains [ and ]
In index.css, define the font face, but replace [ and ] with %5B and %5D respectively in the src property. Example:

@font-face {
  font-family: 'Ubuntu variable';
  font-stretch: 100%; /*  min and max value for the width axis, expressed as percentage */
  font-style: normal;
  font-weight: 100 800; /*  min and max value for the weight axis */
  src: url('/fonts/f1ea362b-Ubuntu%5Bwdth,wght%5D-latin-v0.896a.woff2')
    format('woff2-variations');
}

Run pnpm run build, and ensure the font file is resolved.

Note: The font file will resolve with underscores replacing special characters in the build output directory - this behaviour can be mitigated by setting build.rollupOptions.output.sanitizeName to false.

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Fixes

Fixes #15109

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

Read the Contributing Guidelines, especially the Pull Request Guidelines.
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).
~~Update the corresponding documentation if needed.~~
Ideally, include relevant tests that fail without this PR but pass with it.

bolt-new-by-stackblitz · 2023-12-05T11:45:31Z

Run & review this pull request in StackBlitz Codeflow.

sapphi-red

Thank you!

Would you add a test in playground/assets? Around

vite/playground/assets/css/css-url.css

Lines 8 to 11 in 0f9e1bf

    
           .css-url-relative { 
        
             background: url(../nested/asset.png); 
        
             background-size: 10px; 
        
           }

vite/playground/assets/__tests__/assets.spec.ts

Lines 152 to 154 in 0f9e1bf

    
           test('relative', async () => { 
        
             expect(await getBg('.css-url-relative')).toMatch(assetMatch) 
        
           })

ndv99 · 2023-12-05T14:56:07Z

Thank you!

Would you add a test in playground/assets?

Absolutely! Could you help me out with the assetMatch regex? I've created a file asset[copy].png to test this with (which is just a copy of asset.png), I'm guessing it should look something like this:

const encodedAssetMatch = isBuild
  ? /\/foo\/bar\/assets\/asset\[copy\]-[-\w]{8}\.png/
  : '/foo/bar/nested/asset[copy].png'

sapphi-red · 2023-12-05T15:12:32Z

Of course! I guess /\/foo\/bar\/assets\/asset\[copy\]-[-\w]{8}\.png/ needs to be /\/foo\/bar\/assets\/asset_copy_-[-\w]{8}\.png/ because we are not setting build.rollupOptions.output.sanitizeName.

ndv99 · 2023-12-05T17:02:55Z

Struggling to get that failing test to pass (in build mode) - for some reason, even though I've set the background URL to the image with special characters in its name, it still seems to be getting the normal asset/png.

test('encoded', async () => {
  const bg = await getBg('.css-url-encoded');
  console.log(bg);
  expect(await getBg('.css-url-encoded')).toMatch(encodedAssetMatch)
})

.css-url-encoded {
  background: url(/nested/asset%5Bcopy%5D.png);
  background-size: 10px;
}

<div class="css-url-encoded">
  <span style="background: #fff">CSS background (encoded)</span>
</div>

The output of the console.log is url("http://localhost:4173/foo/bar/assets/asset-tYbLOGqm.png"), when I'd expect it to be url("http://localhost:4173/foo/bar/assets/asset_copy_-tYbLOGqm.png")

Gotta log off for the day, but I'll keep going tomorrow morning and hopefully figure it out

sapphi-red · 2023-12-06T05:31:49Z

Ah, I guess that's because Vite dedupes assets with same content. Changing the image slightly will fix it.

ndv99 · 2023-12-06T08:57:16Z

Ah, I guess that's because Vite dedupes assets with same content. Changing the image slightly will fix it.

Damn, I never knew that! Just made the file smaller (600px --> 500px) and it looks like it's passing locally now. Changes pushed 👍

patak-cat

Thanks for the PR @ndv99!

@sapphi-red I'm good merging this one now, but I think it opens the door for a regression. If the url is name%2525, when we decode it, it becomes name%25 (imagine this is a real file). If we use this url as the replacement, the browser will send it back and we will decode it again, becoming name% and then the file will not be resolved.

I think it is ok because we have this issue in general IIUC, we should be encoding all the URLs before writing them to JS, HTML and CSS files and we aren't doing that right now.

sapphi-red · 2023-12-06T14:35:17Z

Yeah, I think it's ok about that.

But maybe it's safer to return url instead of decodedUrl here?
https://github.com/vitejs/vite/pull/15246/files#diff-2cfbd4f4d8c32727cd8e1a561cffbde0b384a3ce0789340440e144f9d64c10f6L295
We're doing like that in another place.

vite/packages/vite/src/node/plugins/html.ts

Lines 491 to 497 in 8ad81b4

    
           async ({ url }) => { 
        
             const decodedUrl = decodeURI(url) 
        
             if (!isExcludedUrl(decodedUrl)) { 
        
               const result = await processAssetUrl(url) 
        
               return result !== decodedUrl ? result : url 
        
             } 
        
             return url

ndv99 · 2023-12-06T14:58:45Z

Yeah, I think it's ok about that.

But maybe it's safer to return url instead of decodedUrl here?

Just tested this locally, and the build still works in my test project, resolving the encoded URL for the font file.

Edit: Might also be a good idea to use url instead of decodedUrl here for debug logging

ndv99 · 2023-12-11T16:59:11Z

Not really sure what's causing the failure on the Windows pipeline, might work itself out if a re-run is triggered.

fix(build): decode urls in CSS files

d52b413

ndv99 changed the title ~~fix(build): decode urls in CSS files (fix #15109[,#15109])~~ fix(build): decode urls in CSS files (fix #15109) Dec 5, 2023

sapphi-red added feat: css p3-minor-bug An edge case that only affects very specific usage (priority) labels Dec 5, 2023

sapphi-red reviewed Dec 5, 2023

View reviewed changes

ndv99 added 2 commits December 5, 2023 15:19

test(assets): add test for encoded css urls

499f965

test(assets): add missing div in index.html

c0734ca

fix: make asset[copy] smaller, rename to asset[small]

b0c3ce9

ndv99 requested a review from sapphi-red December 6, 2023 09:59

sapphi-red previously approved these changes Dec 6, 2023

View reviewed changes

patak-cat previously approved these changes Dec 6, 2023

View reviewed changes

fix: return url instead of decodedUrl for safety

011daff

ndv99 dismissed stale reviews from patak-cat and sapphi-red via 011daff December 11, 2023 16:46

ndv99 requested review from patak-cat and sapphi-red December 11, 2023 16:46

patak-cat mentioned this pull request Dec 11, 2023

fix: encode URLs correctly (fix #15298) #15311

Merged

9 tasks

sapphi-red approved these changes Dec 12, 2023

View reviewed changes

patak-cat merged commit ea6a7a6 into vitejs:main Dec 12, 2023

ndv99 mentioned this pull request Dec 12, 2023

chore: Migrate maas-ui from CRA to Vite canonical/maas-ui#5207

Merged

9 tasks

twk3 mentioned this pull request Dec 14, 2023

Proposal for switching desktop-client to vite actualbudget/actual#2084

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(build): decode urls in CSS files (fix #15109)#15246

fix(build): decode urls in CSS files (fix #15109)#15246
patak-cat merged 5 commits intovitejs:mainfrom
ndv99:fix/decode-css-urls

ndv99 commented Dec 5, 2023 •

edited

Loading

Uh oh!

bolt-new-by-stackblitz bot commented Dec 5, 2023

Uh oh!

sapphi-red left a comment

Uh oh!

ndv99 commented Dec 5, 2023

Uh oh!

sapphi-red commented Dec 5, 2023 •

edited

Loading

Uh oh!

ndv99 commented Dec 5, 2023

Uh oh!

sapphi-red commented Dec 6, 2023

Uh oh!

ndv99 commented Dec 6, 2023

Uh oh!

patak-cat left a comment

Uh oh!

sapphi-red commented Dec 6, 2023

Uh oh!

ndv99 commented Dec 6, 2023 •

edited

Loading

Uh oh!

ndv99 commented Dec 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	.css-url-relative {
	background: url(../nested/asset.png);
	background-size: 10px;
	}

	test('relative', async () => {
	expect(await getBg('.css-url-relative')).toMatch(assetMatch)
	})

Uh oh!

Conversation

ndv99 commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

QA steps

What is the purpose of this pull request?

Fixes

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

Uh oh!

bolt-new-by-stackblitz bot commented Dec 5, 2023

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

ndv99 commented Dec 5, 2023

Uh oh!

sapphi-red commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndv99 commented Dec 5, 2023

Uh oh!

sapphi-red commented Dec 6, 2023

Uh oh!

ndv99 commented Dec 6, 2023

Uh oh!

patak-cat left a comment

Choose a reason for hiding this comment

Uh oh!

sapphi-red commented Dec 6, 2023

Uh oh!

ndv99 commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndv99 commented Dec 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ndv99 commented Dec 5, 2023 •

edited

Loading

sapphi-red commented Dec 5, 2023 •

edited

Loading

ndv99 commented Dec 6, 2023 •

edited

Loading