Skip to content

Improve arbitrary value validation when parsing candidates #17361

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

Merged
merged 4 commits into from
Mar 28, 2025

Conversation

thecrypticace
Copy link
Contributor

@thecrypticace thecrypticace commented Mar 24, 2025

Fixes #17357

This affects the CDN and Play

Now candidates like these no longer parse and emit CSS:

  • [--foo:1rem;--bar:2rem]
  • [&{color:red}]:flex
  • data-[a]{color:red}foo[a]:flex

@thecrypticace thecrypticace requested a review from a team as a code owner March 24, 2025 18:29
@thecrypticace thecrypticace changed the title Feat/validate arbitrary values Improve arbitrary value validation when parsing candidates Mar 24, 2025
@@ -552,6 +564,9 @@ export function parseVariant(variant: string, designSystem: DesignSystem): Varia

let selector = decodeArbitraryValue(variant.slice(1, -1))
Copy link
Member

Choose a reason for hiding this comment

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

kinda bothers me that this var is called selector and not arbitraryValue like in the 5 other places that look identical haha

Copy link
Member

Choose a reason for hiding this comment

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

I mean, in this case it is a selector because of the variant :P

[.foo]:…

Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

bg-[url(https://example.com?q=;)] will be thrown out but works in postcss so we might allow this too

@RobinMalfait
Copy link
Member

RobinMalfait commented Mar 25, 2025

Our parser doesn't handle it properly so needs a separate fix (same PR or different PR). But semicolon without quotes work:

https://jsfiddle.net/cg6v57mp/1/

In Play this works: https://play.tailwindcss.com/hCXYekpfy6
But this doesn't: https://play.tailwindcss.com/hIwEPHkssN

Looked into this, and it looks like a Tailwind Play issue because Tailwind CSS itself does properly generate the correct CSS:

div {
  background-color: url(https://github.com/tailwindlabs.png?q=;);
}

Output:

div {
  background-color: url(https://github.com/tailwindlabs.png?q=;);
}

Even running it through Lightning CSS keeps working as expected:

div {
  background-color: url("https://github.com/tailwindlabs.png?q=;");
}

@thecrypticace thecrypticace force-pushed the feat/validate-arbitrary-values branch from f4026bd to 8839dd3 Compare March 27, 2025 15:23
@thecrypticace thecrypticace force-pushed the feat/validate-arbitrary-values branch from 8839dd3 to 6276ab3 Compare March 27, 2025 15:27
Copy link
Member

@RobinMalfait RobinMalfait left a comment

Choose a reason for hiding this comment

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

Aha I see, inlined it. Makes sense 👍

@thecrypticace
Copy link
Contributor Author

I'll fix the Play thing separately. I'm pretty sure I know what the problem is there.

Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

LGTM!

@thecrypticace thecrypticace merged commit ab868c6 into main Mar 28, 2025
6 checks passed
@thecrypticace thecrypticace deleted the feat/validate-arbitrary-values branch March 28, 2025 16:38
philipp-spiess pushed a commit that referenced this pull request Mar 28, 2025
Fixes #17357

This affects the CDN and Play

Now candidates like these no longer parse and emit CSS:
- `[--foo:1rem;--bar:2rem]`
- `[&{color:red}]:flex`
- `data-[a]{color:red}foo[a]:flex`
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.

Declaring multiple CSS variables doesn't work with Vite
3 participants