Skip to content

Use safer escaping for css url strings#236303

Merged
mjbvz merged 2 commits intomicrosoft:mainfrom
mjbvz:embarrassing-pigeon
Dec 17, 2024
Merged

Use safer escaping for css url strings#236303
mjbvz merged 2 commits intomicrosoft:mainfrom
mjbvz:embarrassing-pigeon

Conversation

@mjbvz
Copy link
Copy Markdown
Collaborator

@mjbvz mjbvz commented Dec 17, 2024

Fixes #236122

For css \0000xx escaping, if I'm understanding the spec correctly it turns out they eat the space character following them: https://www.w3.org/TR/CSS2/syndata.html#characters

My previous fix didn't account for this. Instead I think a safer fix is to switch to use Css.escape. This often over-escapes the string by converting characters that don't need it but should be safe and it fixes the original issue

Fixes microsoft#236122

For css `\0000xx` escaping, if I'm understanding the spec correctly it turns out they eat the space character following them: https://www.w3.org/TR/CSS2/syndata.html#characters

My previous fix didn't account for this. Instead I think a safer fix is to switch to use `Css.escape`. This often over-escapes the string by converting characters that don't need it but should be safe and it fixes the original issue
@mjbvz mjbvz self-assigned this Dec 17, 2024
@mjbvz mjbvz added the bug Issue identified by VS Code Team member as probable bug label Dec 17, 2024
@mjbvz mjbvz enabled auto-merge December 17, 2024 01:06
@vs-code-engineering vs-code-engineering bot added this to the January 2025 milestone Dec 17, 2024
justschen
justschen previously approved these changes Dec 17, 2024
@justschen
Copy link
Copy Markdown
Collaborator

@mjbvz looks like a unit test failed, something about a type error 😢

@mjbvz mjbvz merged commit 754f888 into microsoft:main Dec 17, 2024
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Jan 31, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Issue identified by VS Code Team member as probable bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Did something change in Uri?

3 participants