Fix: escape double quote of url value in CSS `url()` #7718

Shinyaigeek · 2022-02-15T19:58:00Z

↪️ Pull Request

Hi team! 👋

In css transforming, CSSTransformer encloses URL value in CSS url() with " double quote, so I fixed this simply by escaping ” double quote in url value in url() , but honestly I'm worried that this way is too straightforward and I overlooked some context.

If there is something I missed, please be free to point out or close this PR 🙇

💻 Examples

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8" />
    <title>CSS Double</title>
  </head>
  <body>
    <h1>CSS Double</h1>
    <div class="background"></div>
    <style>
      .background {
        background-image: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" version="1.1" width="32" height="24" viewBox="0 0 32 24"><polygon points="0,0 32,0 16,24" style="fill: rgb%2819, 80, 91%29"></polygon></svg>');
        width: 100px;
        height: 100px;
      }
    </style>
  </body>
</html>

🚨 Test instructions

simply, I checked with the case base64 encoded url value with "

✔️ PR Todo

Added/updated unit tests for this change
Filled out test instructions (In case there aren't any unit tests)
Included links to related issues/PRs

height · 2022-02-15T19:58:03Z

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

devongovett · 2022-03-01T16:17:05Z

packages/transformers/css-experimental/src/CSSTransformer.js

@@ -83,7 +83,8 @@ export default (new Transformer({
            symbols: new Map([['*', {local: '*', isWeak: true, loc}]]),
          });
        } else if (dep.type === 'url') {
-          asset.addURLDependency(dep.url, {
+          // if dependency imported with url() includes ", escape " double quotes because this url value will be enclosed with double quote "
+          asset.addURLDependency(dep.url.replace(/"/g, '\\"'), {


hmm I think this might cause an issue for resolvers because they won't expect pre-escaped values. Perhaps this replacement should be done in the packager. The util here is what does the replacement of placeholders to resolved urls. It might need an option to escape the url.

Oh, indeed. As you say, escaping the background image’s URL in Transformer will cause the not found error in resolver. I will fix this 👍 Thank you for your advice!

I think escaping the quote in URL is needed in anyway because

if URL is not enclosed with quotes, it is not a valid property that the quote exists in the URL

if URL is enclosed with quotes, the quote should be escaped

#7564 as you can see in this PR, I think we need to enclose the URL in quotes and then escape the quotes.

I was thinking about this issue, and I notice that parcel's resolver cannot resolve the filepath of the image where the quotes are escaped though such a URL is the correct URL in the first place. So I think escape should be done with CSSTransformer and the resolver should be modified.

I thought about making the resolver able to handle escaped quotes, but I was concerned about performance degradation due to increased computation, so I made a change to remove escaped quotes from URLs with the CSSTransformer and re-escape them with the CSSPackager. ea56294

devongovett · 2022-03-20T16:44:31Z

packages/transformers/css/src/CSSTransformer.js

-              placeholder: dep.placeholder,
+          asset.addURLDependency(
+            // when URL is escaped, parcel resolver cannot resolve URL, so we need to unescape it in transformer
+            dep.url.replace(/\\"/g, '"').replace(/\\'/g, "'"),


I don't think this is necessary. The url will already be unescaped before it is passed to us.

devongovett · 2022-03-20T16:46:13Z

Thanks for getting this started @Shinyaigeek! I refactored it a little so that we handle the escaping differently depending on the asset type. For example, in HTML, quotes become ". Also supported escaping \ itself in CSS. Hopefully the changes make sense.

packages/core/utils/src/replaceBundleReferences.js

Shinyaigeek · 2022-03-21T03:16:01Z

Thank you for your help @devongovett !! I saw your commit, and feels reasonable. Sure, escaping only CSS text was short-circuited and should have been able to escape other files in the parcel pipeline! I learned a lot from that 👍

* upstream/v2: Upgrade Flow to 0.174.1 (#7849) v2.4.0 v2.4.0 changelog Bump Parcel CSS Dynamic imports priority fix closes #6980 (#7061) fix(transformers): errors.map is not a function (#7672) Make NodeResolver check realpath before resolving with `source` entry (#7846) docs: fix wrong location documents (#7689) Fix: escape double quote of url value in CSS `url()` (#7718) Update @parcel/css and add diagnostic for url dependencies in custom properties (#7845) Use relative path for bundle labels in bundle analysis (#7737) Allow use react-jsx transform in React 16.14.0 (#7728) Move to @parcel/css by default (#7821) Feature: pick PORT number also from .env file (#7819) Enable parsing static initialization blocks (#7839) Bump swc and prevent pure comment removal (#7833) Bump swc (#7777) Human readable file size in bundle analyzer report (#7766) Improve emoji support detection (#7775)

wbinnssmith approved these changes Feb 17, 2022

View reviewed changes

devongovett reviewed Mar 1, 2022

View reviewed changes

Shinyaigeek force-pushed the fix/escape-url-value-in-css-url branch 2 times, most recently from 7e8ae38 to ea56294 Compare March 6, 2022 10:03

Shinyaigeek force-pushed the fix/escape-url-value-in-css-url branch from 6980ae3 to 0e3f3bd Compare March 20, 2022 04:55

Feature: escape deps url on CSSPackager

483258c

Shinyaigeek force-pushed the fix/escape-url-value-in-css-url branch from 0e3f3bd to 483258c Compare March 20, 2022 05:33

Shinyaigeek and others added 5 commits March 20, 2022 14:48

Update: escape single quote

b36d7c2

Chore: add comment for CSSTransformer

e11d400

Update: not replace single quote

c2fafb3

Chore: add test case for CSS quote handling

6c954f1

Refactor and also escape urls in other file types

43da65b

devongovett reviewed Mar 20, 2022

View reviewed changes

Revert transformer change

03ad4ea

mischnic reviewed Mar 20, 2022

View reviewed changes

packages/core/utils/src/replaceBundleReferences.js Outdated Show resolved Hide resolved

devongovett added 2 commits March 20, 2022 14:05

Make it not a breaking change

39d40fe

Merge branch 'v2' into fix/escape-url-value-in-css-url

f8a3cb2

devongovett merged commit 7d2c1ef into parcel-bundler:v2 Mar 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: escape double quote of url value in CSS `url()` #7718

Fix: escape double quote of url value in CSS `url()` #7718

Shinyaigeek commented Feb 15, 2022 •

edited

Loading

height bot commented Feb 15, 2022

devongovett Mar 1, 2022

Shinyaigeek Mar 2, 2022

Shinyaigeek Mar 2, 2022

Shinyaigeek Mar 6, 2022

devongovett Mar 20, 2022

devongovett commented Mar 20, 2022

Shinyaigeek commented Mar 21, 2022

Fix: escape double quote of url value in CSS url() #7718

Fix: escape double quote of url value in CSS url() #7718

Conversation

Shinyaigeek commented Feb 15, 2022 • edited Loading

↪️ Pull Request

💻 Examples

🚨 Test instructions

✔️ PR Todo

height bot commented Feb 15, 2022

devongovett Mar 1, 2022

Choose a reason for hiding this comment

Shinyaigeek Mar 2, 2022

Choose a reason for hiding this comment

Shinyaigeek Mar 2, 2022

Choose a reason for hiding this comment

Shinyaigeek Mar 6, 2022

Choose a reason for hiding this comment

devongovett Mar 20, 2022

Choose a reason for hiding this comment

devongovett commented Mar 20, 2022

Shinyaigeek commented Mar 21, 2022

Fix: escape double quote of url value in CSS `url()` #7718

Fix: escape double quote of url value in CSS `url()` #7718

Shinyaigeek commented Feb 15, 2022 •

edited

Loading