fix: attribute selector with no valid attribute name - #335
Open
theRizwan wants to merge 1 commit into
Open
Conversation
`attribute()` read `next[TOKEN.TYPE]` in four places without checking that a next token existed, so an attribute selector whose last token before `]` was `*`, `$`, `^`, `~` or `|` threw a raw TypeError out of the parser. Two other reads of `next` in the same file already guarded with `next &&`. Guarding those four alone moved the failure rather than fixing it: with no attribute name captured, `Attribute#toString` interpolated the missing value and emitted a selector containing the literal text `undefined`. That path was already reachable without any crash, for example `[ * ]` produced `[ *|undefined]`. So the token loop now also rejects a bracket pair that supplied no attribute name, which matches the error `[*]` already produced. Across 43,200 generated attribute selectors compared against 7.1.5: 341 raw TypeErrors and 596 outputs containing `undefined` are both eliminated, and no input that previously parsed produces different output. Refs postcss#334 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #334.
The crash
attribute()readnext[TOKEN.TYPE]in four places without checking that a next token existed:case tokens.asteriskcase tokens.caret(also reached bytokens.dollarvia fall-through)case tokens.combinator, the~checkcase tokens.combinator, the|checkSo an attribute selector whose last token before
]was*,$,^,~or|threw a rawTypeErrorout of the parser rather than the parser's own error.[ns|*],[a$],[a|]and eight other shapes were affected.Two other reads of
next[TOKEN.TYPE]in the same file already guard withnext &&, and theelse ifdirectly below the asterisk case guards with&& next, so this follows the pattern already in the file rather than introducing one.Why the guards alone were not enough
Adding the four guards moved the failure instead of fixing it. With no attribute name captured,
Attribute#toStringinterpolates the missing value, so[ns|*]became the string[ns|undefined].That path was already reachable without any crash. On 7.1.5:
[*]on its own already threwExpected an attribute., so the two were inconsistent. The token loop now also rejects a bracket pair that supplied no attribute name, pointing at the opening bracket.Verification
Compared against pristine 7.1.5 across 43,200 generated attribute selectors, varying the name, namespace separator and its surrounding whitespace, operator, value, quoting, insensitivity flag, and padding:
TypeErrorundefinedThe 596 that now raise
Expected an attribute.are exactly the 596 that previously emittedundefined. No input that previously parsed produces different output.npm testis green, includingoxlint, the type check, and the coverage thresholds: 94.98% lines, 95.54% branches, 97.71% functions against gates of 94/94/96. Test count 789 to 801.Test placement
The newly-erroring cases are in
exceptions.mjs, asserted by message rather than by type, for the reason recorded in that file in #330: the default{instanceOf: Error}check is satisfied by aTypeError, so a type-only assertion would not have caught this.The five inputs that now parse instead of crashing (
[href*],[href$],[href^],[href~],[href|], each yielding[href]) are inattributes.mjsusingavadirectly rather than thetesthelper.testadds an automatic round-trip assertion, and these are deliberately not round-trips: the trailing token is dropped, matching how[href=]already drops an operator with no value. Usingtestwould have asserted the lossy output is correct.Scope
[ns | ]still returns[ns ]rather than treatingnsas a namespace. That is the whitespace-before-|lookahead in #229 and is left alone here.Preserving a trailing
*/$/^/~/|so those five round-trip is also out of scope. It is the same lossy behaviour as[href=]and would be a separate change.Disclosure
I used AI assistance while investigating and writing this, including for the differential fuzz harness. I have read and can account for every line, and the reasoning above is mine. Flagging it because of the preference stated on #333; happy to walk through any part of it.