Fix parsing url(…) with special characters such as ; or {}

[RobinMalfait]

This PR fixes an issue where some special characters (with an actual meaning CSS) were used inside of the url(…) function, would result in incorrectly parsed CSS.

For example, when we encounter a {, then we would start a new "block" for nesting purposes. If we encounter an }, then the block would end. If we encounter a ;, then that would be the end of a declaration.

All of that is true, unless we are in a url(…) function. In that case, we should ignore all of those characters and treat them as part of the URL.

This is only an issue because:

1. We are allowed to use these characters in URLs.
2. We can write an url inside url(…) without quotes. With quotes, this would not be an issue.

[philipp-spiess]

Can you use the closingBracketStack for this?

[thecrypticace]

Also this is a number but should be a string if for some reason we can't use closingBracketStack

[RobinMalfait]

That's what I did originally, but you would run into issues relatively quickly. When you are parsing CSS like this:

.foo {
  background: url(data:image/png;base64,abc==);
}

The moment you reach the ; character, then the closingBracketStack looks like this: {( where { is coming from the selector .foo {, and ( is coming from the url( function. Meaning that if the last character is ( we know that we are not top-level anymore, and we can safely ignore the ;.

However, if we are parsing CSS like this:

.foo {
  background: url(https://example-image-search.org?q={query;limit=5}&ids=[1,2,3]);
}

If we now reach the ;, then the closingBracketStack looks like this: {({, where the first {, comes from the selector .foo {, the ( comes from the url( function and the last { comes from the query string.

In this case we can't just look at the last character in the closingBracketStack, but have to check if any ( exists to know that we can safely ignore the ;. This means that our O(1) lookup turns into an O(n) lookup. Sure, it's a small n, but still not ideal.

A second issue is that we validate that the closingBracketStack is empty at the end of the parsing. Which ensures that every opening bracket has a closing bracket and thus properly balanced.

However, brackets inside the url, don't need to be balanced. This is valid CSS:

.foo {
  background: url(https://example-image-search.org?curlies=}});
}

As you can see, the request is actually made with the curlies in the query param.

If we use the closingBracketStack, in this case it would look like this:

1. { — from the selector .foo {
2. {( — from the start of the declaration value url(
3. {(} — from curlies=}, this would also error because improperly balanced. But let's assume it
   didn't error.
4. {(}} — from curlies=}}.
5. {(}}) — from closing the url), but the closing bracket is not balanced and in a strange
   state.
6. {(}})} — from closing the .foo { block }.

Because we don't care about balancing curlies, and because we don't want to use O(n) instead of O(1) lookups, I moved it to a separate variable.

@thecrypticace, I picked a number because this value is only concerned with parentheses, and not with any other type of brackets. Since we don't care about the other types of brackets sandwiched between the parentheses (e.g.: {(})), a number is a bit nicer to work with because we can just increment/decrement the value.

Eventually we do want the url(…) to be closed though, so properly closing the parentheses is still important, but this basically means that the number should be 0 at the end, which we already account for.

If we don't like this solution, and still prefer to use the closingBracketStack, we could also apply this rule instead:

If we see (, ignore every other special character like ; or } until we see an ), because that is essentially exactly what we are trying to achieve. If we then encounter an { or }, we don't even touch the closingBracketStack at all.

It does mean that we have to work with strings instead of numbers a bit more, but that's fine I think? It does reduce the amount of concepts in the parser which is a benefit.

Diff would then look like this:

diff --git a/packages/tailwindcss/src/css-parser.ts b/packages/tailwindcss/src/css-parser.ts
index b91011e0c..3b6e283e5 100644
--- a/packages/tailwindcss/src/css-parser.ts
+++ b/packages/tailwindcss/src/css-parser.ts
@@ -42,7 +42,6 @@ export function parse(input: string) {
 
   let buffer = ''
   let closingBracketStack = ''
-  let parenStack = 0
 
   let peekChar
 
@@ -332,7 +331,10 @@ export function parse(input: string) {
     // }
     // ```
     //
-    else if (currentChar === SEMICOLON && parenStack <= 0) {
+    else if (
+      currentChar === SEMICOLON &&
+      closingBracketStack[closingBracketStack.length - 1] !== ')'
+    ) {
       let declaration = parseDeclaration(buffer)
       if (parent) {
         parent.nodes.push(declaration)
@@ -344,7 +346,10 @@ export function parse(input: string) {
     }
 
     // Start of a block.
-    else if (currentChar === OPEN_CURLY && parenStack <= 0) {
+    else if (
+      currentChar === OPEN_CURLY &&
+      closingBracketStack[closingBracketStack.length - 1] !== ')'
+    ) {
       closingBracketStack += '}'
 
       // At this point `buffer` should resemble a selector or an at-rule.
@@ -369,7 +374,10 @@ export function parse(input: string) {
     }
 
     // End of a block.
-    else if (currentChar === CLOSE_CURLY && parenStack <= 0) {
+    else if (
+      currentChar === CLOSE_CURLY &&
+      closingBracketStack[closingBracketStack.length - 1] !== ')'
+    ) {
       if (closingBracketStack === '') {
         throw new Error('Missing opening {')
       }
@@ -459,17 +467,17 @@ export function parse(input: string) {
 
     // `(`
     else if (currentChar === OPEN_PAREN) {
-      parenStack += 1
+      closingBracketStack += ')'
       buffer += '('
     }
 
     // `)`
     else if (currentChar === CLOSE_PAREN) {
-      if (parenStack <= 0) {
+      if (closingBracketStack[closingBracketStack.length - 1] !== ')') {
         throw new Error('Missing opening (')
       }
 
-      parenStack -= 1
+      closingBracketStack = closingBracketStack.slice(0, -1)
       buffer += ')'
     }

---

Note: I know the closingBracketStack actually holds the inverse of the actual brackets, but it's a bit easier to reason about it this way.

[thecrypticace]

I SWEAR when I read parenStack += … I saw a (. My brain is fried. Carry on…

[RobinMalfait]

Haha, no worries!

Implemented that last change here: 9177c68

[philipp-spiess]

Thanks for the detailed info. I do think the diff you ended up and the concept of "(" will not cause any other stack to be created until it's closed again seems good and it feels like the diff is lighter this way. Awesome!