escape dimension units starting with e and a digit on serialize - #77
affan-arch wants to merge 2 commits into
Conversation
Out of curiosity, why did you do that? |
|
round-trip stability is a property i lean on: sanitizers and rewriters parse, serialize, and re-parse css, so serialize -> re-parse should land on the same tokens. feeding the serializer's output back through the parser is the cheapest way to check that invariant holds, and the only non-error mismatches all reduced to this e-plus-digit unit case. |
|
I mean, what’s your use case in your real life? |
|
honestly it's not behind a product i ship day to day. i do robustness and security work on css handling, and round-trip fuzzing of parser/serializer pairs is a habit from that kind of work. tinycss2 sits under a lot of sanitizers and rewriters, so a serialize step that quietly changes a token's type is the sort of thing i flag even when the trigger is narrow. happy to drop the patch if you feel it's too edge-case to carry. |
OK. Quite complex topics for someone who joined GitHub yesterday!
Interesting, I didn’t know. Do you know some of them?
What is strange is that we just got a fix very close to yours, a few weeks ago, with the same-ish test: #74. Either humans are suddenly fond of CSS parsing/serializing round-trips (probably not), or bots are really going through all the open source projects to report the same strange bugs and flood poor maintainers (probably). Do you think that |
|
you're reading it right: i do use ai tooling to help find and prep these. the account is mine and i'm responsible for what it submits, so i'd rather be judged on whether the fix is correct than on how it surfaced. on consumers, the clearest one is bleach, whose css sanitizer (bleach[css]) parses and re-serializes style attributes through tinycss2. weasyprint depends on it too, though that's rendering more than sanitizing. i don't have an exhaustive list beyond those. and yes, went and read #74 after you mentioned it, same round-trip territory. i switched the guard to your version, it folds the bare e/E, e-/e+ and e-plus-digit cases into one condition and drops the extra clause i had. full suite still passes. |
Well, but it’s been deprecated more than 3 years ago.
That’s no sanitizing at all. I’m sad, I thought I would discover "a lot of sanitizers and rewriters"!
The single line of the fix has now been written by me, there’s nothing left to judge. So… The ends justify the means? Here’s another philosophical question: what’s the point of merging a pull request with your name, when it doesn’t fix a problem for you, when you didn’t write the original code, and when you didn’t write the final code? Here’s my answer: I’ll write the fix myself. That will avoid bots to come again and again to fix this very important bug. I know someone who calls that AIDD ‑ Artificial Intelligence Driven Development. I now work on useless topics chosen by the "contributors" bots. We have guidelines. Please don’t open a new pull request if it’s not for a real-life problem. |
DimensionToken._serialize_toalready knows a unit that begins with ane/Eis dangerous: written verbatim after the numeric representation it can be read back as scientific notation, so it escapes the leading letter as\65for units equal toe/Eor starting withe-/E-. The guard misses the case wheree/Eis followed directly by a digit, which is reachable from escaped input like1\65 5(unite5) or2\65 3px(unite3px). Those serialize to1e5and2e3px, and re-tokenize as the number100000and as2000px, so a parse -> serialize -> parse round-trip silently changes the token type and value. I hit it fuzzing the serializer against re-parsing, where every non-error mismatch reduced to this family. The fix just widens the existing condition to also escape when the unit starts withe/Eand a digit, so the leading letter is emitted as\65and the dimension parses back unchanged. Common units likeem,exandpxare untouched, and I added a small round-trip test coveringe5,E5ande3px.