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(postcss-svgo): make sure "new URL" does not crash on relative urls #1050

Closed
wants to merge 1 commit into from
Closed

Conversation

Neunerlei
Copy link

I'm using cssnano not directly, but as a dependency through "css-minimizer-webpack-plugin".
After the update to "css-minimizer-webpack-plugin" 2.0.0 and in turn cssnano 5.0.0 webpack fails with an error:

css/bundle.css from Css Minimizer
TypeError [ERR_INVALID_URL]: Invalid URL: ../assets/th-bingen-9472d38cacd57ee37863.eot?#iefix
    at W:\work\th_bingen\website_relaunch\typo\app\src\css\bundle.css:13:3
    at onParseError (internal/url.js:256:9)
    at new URL (internal/url.js:332:5)
    at W:\work\th_bingen\website_relaunch\typo\app\src\node_modules\postcss-svgo\dist\index.js:32:17
    at walk (W:\work\th_bingen\website_relaunch\typo\app\src\node_modules\postcss-value-parser\lib\walk.js:7:16)
    at ValueParser.walk (W:\work\th_bingen\website_relaunch\typo\app\src\node_modules\postcss-value-parser\lib\index.js:18:3)
    at minify (W:\work\th_bingen\website_relaunch\typo\app\src\node_modules\postcss-svgo\dist\index.js:22:23)
    at W:\work\th_bingen\website_relaunch\typo\app\src\node_modules\postcss-svgo\dist\index.js:116:9
    at W:\work\th_bingen\website_relaunch\typo\app\src\node_modules\postcss\lib\container.js:91:18
    at W:\work\th_bingen\website_relaunch\typo\app\src\node_modules\postcss\lib\container.js:74:18
    at AtRule.each (W:\work\th_bingen\website_relaunch\typo\app\src\node_modules\postcss\lib\container.js:60:16)

To Reproduce
In my Stylesheet I have a part of SASS code:

$thb-icon-web-directory: "../fonts" !default
@font-face
  font-family: "th-bingen"
  src: url("#{$thb-icon-web-directory}/th-bingen.eot")
  src: url("#{$thb-icon-web-directory}/th-bingen.eot?#iefix") format("embedded-opentype"),
  url("#{$thb-icon-web-directory}/th-bingen.woff") format("woff"),
  url("#{$thb-icon-web-directory}/th-bingen.ttf") format("truetype"),
  url("#{$thb-icon-web-directory}/th-bingen.svg#th-bingen") format("svg")
  font-weight: normal
  font-style: normal

That gets processed by webpack. Because the svg is less then 10kb it gets inlined, so the final output Looks like this:

 url(../assets/th-bingen-9472d38cacd57ee37863.eot?#iefix) format("embedded-opentype"), url(../assets/th-bingen-933669eb3949465b904e.woff) format("woff"), url(../assets/th-bingen-cb9e4a0dd4b20e03338a.ttf) format("truetype"), url("data:image/svg+xml,%3cs
vg xmlns='http://www.w3.org/2000/svg'%3e%3cdefs%3e%3cfont id='th-bingen' horiz-adv-x='512'%3e%3cfont-face font-family='th-bingen' units-per-em='512' ascent='480' descent='-32'/%3e%3cglyph glyph-name='map-pin' unicode='a' d='M256 512c-94 0-171-77-171-171 0-90 156-31
6 162-326l9-12 9 12c6 10 162 236 162 326 0 94-77 171-171 171zm0-472c-33 50-149 229-149 301 0 83 67 150 149 150s149-67 149-150c0-72-116-251-149-301zm0 376c-41 0-75-33-75-75 0-41 34-74 75-74s75 33 75 74c0 42-34 75-75 75zm0-128c-29 0-53 24-53 53 0 30 24 54 53 54s53-24
 53-54c0-29-24-53-53-53z'/%3e%3cglyph glyph-name='arrow-right' unicode='b' d='M273 436l160-161H17v-32h416L273 82l22-23 200 200-200 199z'/%3e%3cglyph glyph-name='arrow-left' unicode='c' d='M239 82L79 243h416v32H79l160 161-22 22L17 259 217 59z'/%3e%3cglyph glyph-name
='arrow-down' unicode='d' d='M433 242L272 81v417h-32V81L79 242l-22-23L256 20l199 199z'/%3e%3cglyph glyph-name='arrow-up' unicode='e' d='M79 276l161 160V20h32v416l161-160 22 22-199 200L57 298z'/%3e%3c/font%3e%3c/defs%3e%3c/svg%3e#th-bingen") format("svg")

When I build the script it fails at this "new URL", because it does process each entry in the url list and URL does not work with relative paths.

This fix should resolve this issue, by only creating a new URL if a base64 encoded url is used.

@alexander-akait
Copy link
Member

Thanks, can you add test case?

@skytt
Copy link

skytt commented May 20, 2021

The same problems. Successfully figured it out by doing the same modification as this issue.
Will this PR be published?

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Please add test

@Neunerlei
Copy link
Author

Sorry, but I don't have time to do that in the foreseeable future.
Feel free to close the PR

@internalsystemerror
Copy link

internalsystemerror commented May 25, 2021

I have the same issue, but I don't believe it's caused by relative URLs, but by the # in the url, I with the same error but:

TypeError [ERR_INVALID_URL]: Invalid URL: /assets/fonts/fa-brands-400.eot?#iefix

The similarities being the #iefix.

@Neunerlei
Copy link
Author

@internalsystemerror

The issue is not the relative url, but that it does not contain a hostname

@internalsystemerror
Copy link

The issue is not the relative url, but that it does not contain a hostname

Oh I see. That makes more sense now. I tried forking the code and adding a test prior to this patch being implemented but my jest skills are pretty basic. I was unable to reproduce the issue this way and I'm currently investigating a way to provide a failing test. Any ideas where I should look? This is what I tried adding but this passes:

test('should not crash on urls containing a relative URL', async () => {
  const css = `@font-face {
  src: url("../assets/font.eot#iefix") format("eot");
  }`;
  const result = await postcss(plugin()).process(css, { from: undefined });
  expect(result.messages.length).toBe(0);
});

@ludofischer
Copy link
Collaborator

Hi all, I have created another PR to fix this issue as I wanted to refactor to make the control flow more readable. I've also added a failing test.

ludofischer added a commit that referenced this pull request May 25, 2021
Fix #1050

Ensure it never starts processing an URL before making
sure it is an inine SVG.
ludofischer added a commit that referenced this pull request May 25, 2021
Fix #1050

Ensure it never starts processing an URL before making
sure it is an inine SVG.
ludofischer added a commit that referenced this pull request May 25, 2021
Fix #1050

Ensure it never starts processing an URL before making
sure it is an inine SVG.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants