amadan

rastrillo / fiesta Public

Refuse brackets that do not nest in the CSS readers in check

The declaration reader counted parentheses with a number that could
go below zero. In :root { --rst-review-probe: ); color: red; } the )
took it to -1, so neither ; ended a declaration, and the whole block
read as one new custom property, which the compatibility gate allows.
A browser drops the invalid custom property at the ; and applies
color: red to every 1.0 page. [ and ] were not counted at all.

braceMatchEnd counted braces without reading strings, so a "{" in one
rule and a "}" in a later one paired up, and the reduced-motion block
it cut ran on over the rules between them.

There is now one bracket reader, on cssSpan. checkCSSBrackets refuses
a (, [ or { that is not closed by its own partner before the one
around it, a closer with nothing open, and a { inside ( or [, which a
browser reads as a block inside the value or prelude. Every reader
that strips comments gets it through cssWithoutComments. braceMatchEnd
walks spans with the same rule and returns an error for an unmatched
block instead of the end of the file. cssItems and cssDeclarations
step over a () or [] group whole with it and refuse a stray closer,
so neither keeps a count of its own.

None of the 26 stylesheets in css/, the 1.0 seed and the two releases
is refused, and each reads to the same records.

Red: Astra's input passes TestTheNewRuleGateHasNoKnownHoles; nine
unmatched, unclosed or misplaced bracket inputs read without error in
TestTheReaderRefusesWhatItCannotReadAsABrowserDoes; braceMatchEnd
ends at the quoted brace, 23 for 39. Mutations, each red and
reverted: no bracket check in the comment reader (M1), no stray
closer refusal in cssDeclarations (M2), braceMatchEnd stepping one
byte, not one span (M3), a closer with nothing open let through (M4),
a closer of the wrong kind let through (M5), a { inside brackets let
through (M6).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude d37c5b9efcfbff81304b3f4cc63c6804a382f60a parent 2ed4142
4 files changed, +241 −107
  • check/baseline_test.go +103 −77
  • check/css_test.go +31 −27
  • check/markuphelp_test.go +100 −0
  • check/sizing_test.go +7 −3

Intent

full ledger: fiesta-1-1

diff --git a/check/baseline_test.go b/check/baseline_test.go
index f3e764b..6f077ca 100644
--- a/check/baseline_test.go
+++ b/check/baseline_test.go
@@ -325,42 +325,42 @@ func atRuleName(prelude string) string {
}
// cssItems splits a rule list into its statements, each up to its ;
-// or through its {} block, with strings and parentheses respected so a
-// brace inside content: "{" or a url() is not taken for structure. It
-// and the two readers below see only what cssWithoutComments returned,
-// which has no backslash outside a string, so only skipString reads
-// escapes.
+// or through its {} block. It reads with cssSpan, so a bracket inside
+// content: "{" is text, and steps over a () or [] group whole with
+// braceMatchEnd, so a ; or { inside url() or an attribute selector is
+// not taken for structure. A bracket that closes nothing is an error,
+// not a count below zero.
func cssItems(src string) ([]cssItem, error) {
var items []cssItem
- start, depth := 0, 0
- for i := 0; i < len(src); i++ {
+ start := 0
+ for i := 0; i < len(src); {
+ end, _, err := cssSpan(src, i)
+ if err != nil {
+ return nil, err
+ }
switch c := src[i]; c {
- case '"', '\'':
- i = skipString(src, i)
- case '(':
- depth++
- case ')':
- depth--
- case '{':
- if depth > 0 {
- continue
+ case '(', '[':
+ close, err := braceMatchEnd(src, i+1)
+ if err != nil {
+ return nil, err
}
- end, ok := matchingBrace(src, i)
- if !ok {
- return nil, fmt.Errorf("the block opened after %q is never closed", collapseSpace(src[start:i]))
+ end = close + 1
+ case '{':
+ close, err := braceMatchEnd(src, i+1)
+ if err != nil {
+ return nil, fmt.Errorf("the block opened after %q: %w", collapseSpace(src[start:i]), err)
}
- items = append(items, cssItem{prelude: src[start:i], block: src[i+1 : end], hasBlock: true})
- i, start = end, end+1
- case '}':
- return nil, fmt.Errorf("a } closes nothing after %q", collapseSpace(src[start:i]))
+ items = append(items, cssItem{prelude: src[start:i], block: src[i+1 : close], hasBlock: true})
+ end, start = close+1, close+1
+ case ')', ']', '}':
+ return nil, fmt.Errorf("a %c closes nothing after %q", c, collapseSpace(src[start:i]))
case ';':
- if depth == 0 {
- if p := strings.TrimSpace(src[start:i]); p != "" {
- items = append(items, cssItem{prelude: p})
- }
- start = i + 1
+ if p := strings.TrimSpace(src[start:i]); p != "" {
+ items = append(items, cssItem{prelude: p})
}
+ start = i + 1
}
+ i = end
}
if rest := strings.TrimSpace(src[start:]); rest != "" {
items = append(items, cssItem{prelude: rest})
@@ -368,43 +368,16 @@ func cssItems(src string) ([]cssItem, error) {
return items, nil
}
-// skipString returns the index of the quote that closes the string
-// opening at i.
-func skipString(src string, i int) int {
- q := src[i]
- for i++; i < len(src) && src[i] != q; i++ {
- if src[i] == '\\' {
- i++
- }
- }
- return i
-}
-
-// matchingBrace is the index of the } that closes the { at open.
-func matchingBrace(src string, open int) (int, bool) {
- depth := 0
- for i := open; i < len(src); i++ {
- switch src[i] {
- case '"', '\'':
- i = skipString(src, i)
- case '{':
- depth++
- case '}':
- if depth--; depth == 0 {
- return i, true
- }
- }
- }
- return 0, false
-}
-
// cssDeclarations reads a block's declarations as property, value and
// a non-empty third field when !important, the value's whitespace
-// collapsed. A { in the block is a nested rule, which these files do
-// not use and this reader does not read.
+// collapsed. It reads brackets as cssItems does: a ; inside a string
+// or a () or [] group ends no declaration, and a ) that closes nothing
+// is an error, where a count below zero read every declaration after
+// it as part of the one before. A { in the block is a nested rule,
+// which these files do not use and this reader does not read.
func cssDeclarations(block string) ([][3]string, error) {
var out [][3]string
- start, depth := 0, 0
+ start := 0
flush := func(end int) error {
piece := strings.TrimSpace(block[start:end])
if piece == "" {
@@ -414,32 +387,38 @@ func cssDeclarations(block string) ([][3]string, error) {
if !ok {
return fmt.Errorf("%q is not a declaration", collapseSpace(piece))
}
+ prop = strings.TrimSpace(prop)
value = collapseSpace(value)
important := ""
if v, ok := cutImportant(value); ok {
value, important = v, "!important"
}
- out = append(out, [3]string{strings.TrimSpace(prop), value, important})
+ out = append(out, [3]string{prop, value, important})
return nil
}
- for i := 0; i < len(block); i++ {
- switch block[i] {
- case '"', '\'':
- i = skipString(block, i)
- case '(':
- depth++
- case ')':
- depth--
+ for i := 0; i < len(block); {
+ end, _, err := cssSpan(block, i)
+ if err != nil {
+ return nil, err
+ }
+ switch c := block[i]; c {
+ case '(', '[':
+ close, err := braceMatchEnd(block, i+1)
+ if err != nil {
+ return nil, err
+ }
+ end = close + 1
case '{':
return nil, fmt.Errorf("a nested rule after %q, which this reader does not read", collapseSpace(block[start:i]))
+ case ')', ']', '}':
+ return nil, fmt.Errorf("a %c closes nothing after %q", c, collapseSpace(block[start:i]))
case ';':
- if depth == 0 {
- if err := flush(i); err != nil {
- return nil, err
- }
- start = i + 1
+ if err := flush(i); err != nil {
+ return nil, err
}
+ start = i + 1
}
+ i = end
}
if err := flush(len(block)); err != nil {
return nil, err
@@ -845,6 +824,11 @@ func TestTheNewRuleGateHasNoKnownHoles(t *testing.T) {
{`@media (width: 731px) { @property --rst-\73p-4 { syntax: "<length>"; inherits: false; initial-value: 48px; } }`, outsideAString, true},
{`@media (width: 731px) { @keyframes rst-\73pin {} }`, outsideAString, true},
{`[rst-page-foot] [rst-dropdown] { margin: 0; }`, "[rst-page-foot] [rst-dropdown]", false},
+ // A ) that closes nothing ends no declaration for a browser, so
+ // the ; after it does, and color: red reaches every 1.0 page. A
+ // reader whose bracket count went below zero read the rest of
+ // the block as one new custom property.
+ {`:root { --rst-review-probe: ); color: red; }`, "a ) closes", true},
// A comment marker the reader takes for one and a browser does
// not, either side of a rule: inside a string of either quote,
// after an escaped quote inside the string, or after a
@@ -984,7 +968,27 @@ func TestDeclarationsAreReadInTheirContext(t *testing.T) {
// no string. A backslash inside a string is read as a browser reads
// it.
func TestTheReaderRefusesWhatItCannotReadAsABrowserDoes(t *testing.T) {
- for _, c := range []struct{ css, refusal string }{
+ brackets := []struct{ css, refusal string }{
+ // Brackets must nest. One that closes nothing, closes another
+ // kind or is never closed leaves a browser and a reader
+ // disagreeing about where a declaration or a rule ends.
+ {`a { --b: ); color: red }`, "a ) closes"},
+ {`a { --b: ]; color: red }`, "a ] closes"},
+ {`) a { b: c }`, "a ) closes nothing"},
+ {`a { b: ( ] ) }`, "a ] closes the ("},
+ {`a { --b: (; color: red }`, "a } closes the ("},
+ {`a { --b: [; color: red }`, "a } closes the ["},
+ {`a { b: c`, "a { is never closed"},
+ {`a { b: f(`, "a ( is never closed"},
+ {`a[b`, "a [ is never closed"},
+ // A { inside brackets opens a block a browser reads inside the
+ // value or the prelude, where these readers would take it for
+ // a rule.
+ {`a { b: ({}) }`, "a { opens inside the ("},
+ {`@media (x { a { b: c } }) { d { e: f } }`, "a { opens inside the ("},
+ {`a[b { c: d }`, "a { opens inside the ["},
+ }
+ for _, c := range append([]struct{ css, refusal string }{
{`[rst-snippet] { --rst-gate-a: url(/*); } [rst-page] { padding-inline: 3rem; } [rst-snippet] { --rst-gate-b: url(*/); }`, "url("},
{`a { b: URL(c) }`, "url("},
// An escaped url( hides the rule between as an unescaped one
@@ -1002,11 +1006,23 @@ func TestTheReaderRefusesWhatItCannotReadAsABrowserDoes(t *testing.T) {
{`a { content: "b\`, "string"},
{`a { b: c } /* d`, "comment"},
{`a { padding: 1/**/2px }`, "between two tokens"},
- } {
+ }, brackets...) {
if _, err := declarationsOf("t.css", c.css); err == nil || !strings.Contains(err.Error(), c.refusal) {
t.Errorf("declarationsOf(%q): %v, want an error naming the %s", c.css, err, c.refusal)
}
}
+ // The comment reader the other readers start from refuses them
+ // too, and the declaration reader does on a block read alone.
+ for _, c := range brackets {
+ if _, err := cssWithoutComments(c.css); err == nil || !strings.Contains(err.Error(), c.refusal) {
+ t.Errorf("cssWithoutComments(%q): %v, want an error naming the %s", c.css, err, c.refusal)
+ }
+ }
+ for _, block := range []string{" --rst-review-probe: ); color: red; ", " --b: ]; c: d ", " --b: (; c: d "} {
+ if decls, err := cssDeclarations(block); err == nil {
+ t.Errorf("cssDeclarations(%q) = %q, want an error naming the bracket", block, decls)
+ }
+ }
const css = "a { content: \"/*\"; b: url(\"*/\"); c: url( '/*' ) } d { content: \"e\\\nf\"; g: x-url( /**/ h) } i { k: \"\\\"/*\\\"\"; m: '\\'}'; n: o }"
got, err := declarationsOf("t.css", css)
if err != nil {
@@ -1023,6 +1039,16 @@ func TestTheReaderRefusesWhatItCannotReadAsABrowserDoes(t *testing.T) {
{File: "t.css", Selector: "i", Property: "n", Value: "o"},
}
sameDeclarations(t, got, want)
+
+ // A bracket inside a string is text, in a selector or a value.
+ got, err = declarationsOf("t.css", `a[title="]"] { content: ")"; b: f("(") [g] }`)
+ if err != nil {
+ t.Fatal(err)
+ }
+ sameDeclarations(t, got, []declaration{
+ {File: "t.css", Selector: `a[title="]"]`, Property: "content", Value: `")"`},
+ {File: "t.css", Selector: `a[title="]"]`, Property: "b", Value: `f("(") [g]`},
+ })
}
func TestAtRulesAreReadAsRecords(t *testing.T) {
diff --git a/check/css_test.go b/check/css_test.go
index a678209..b1dc1e8 100644
--- a/check/css_test.go
+++ b/check/css_test.go
@@ -17,8 +17,9 @@ import (
// scheme gates, the selectors the components rely on, and the markup
// contracts the examples hold (the dropdown's native exclusivity,
// the modal as a rendered-open dialog). The CSS readers they share
-// (braceMatchEnd, leafRules, selectorList, blockBody) live here and
-// in contrast_test.go; the tests over markup read the example state
+// (leafRules, selectorList, blockBody) live here and in
+// contrast_test.go, on the scanner and bracket reader in
+// markuphelp_test.go; the tests over markup read the example state
// the manifest's idiom table names.
func TestTokensCSSIsEmbedded(t *testing.T) {
@@ -77,7 +78,10 @@ func shapeLeaks(css string) ([]string, error) {
break
}
start := idx + i + len(header)
- end := braceMatchEnd(css, start)
+ end, err := braceMatchEnd(css, start)
+ if err != nil {
+ return nil, err
+ }
rules, err := readLeafRules(css[start:end])
if err != nil {
return nil, err
@@ -314,33 +318,12 @@ func TestBothSchemesAreAuthoredInEveryTheme(t *testing.T) {
// as a way to silence the failure.
var reducedMotionAllowlist = map[string]bool{}
-// braceMatchEnd returns the index of the '}' matching the '{' implicitly
-// opened at css[start-1] (i.e. depth starts at 1) — tokens.css nests at
-// most one level (a media query wrapping plain rules), and no selector or
-// value in this file contains a literal brace, so simple depth counting
-// is exact here.
-func braceMatchEnd(css string, start int) int {
- depth := 1
- for j := start; j < len(css); j++ {
- switch css[j] {
- case '{':
- depth++
- case '}':
- depth--
- if depth == 0 {
- return j
- }
- }
- }
- return len(css)
-}
-
// reducedMotionBlocks returns the raw contents of every
// @media (prefers-reduced-motion: reduce) { ... } block in css, and rest
// = css with those blocks (header, braces and all) removed — so a
// transition-selector search over rest never mistakes a reduce block's
// own "transition: none" for an active, undisabled transition.
-func reducedMotionBlocks(css string) (blocks []string, rest string) {
+func reducedMotionBlocks(t testing.TB, css string) (blocks []string, rest string) {
const header = "@media (prefers-reduced-motion: reduce) {"
var b strings.Builder
last, idx := 0, 0
@@ -351,7 +334,10 @@ func reducedMotionBlocks(css string) (blocks []string, rest string) {
}
blockStart := idx + i
contentStart := blockStart + len(header)
- end := braceMatchEnd(css, contentStart)
+ end, err := braceMatchEnd(css, contentStart)
+ if err != nil {
+ t.Fatal(err)
+ }
blocks = append(blocks, css[contentStart:end])
b.WriteString(css[last:blockStart])
last = end + 1
@@ -524,10 +510,28 @@ func TestShellCSSReducedMotionDisablesEveryAnimation(t *testing.T) {
assertReducedMotion(t, "shell.css", string(fiesta.ShellCSS()))
}
+// braceMatchEnd reads a quoted brace as text, so the block it finds
+// ends at the brace that closes it, not at one inside a string.
+func TestBraceMatchEndReadsAStringAsText(t *testing.T) {
+ const css = `@media x { a { b: "}"; } c { d: "{"; } } e { f: "}" }`
+ start := strings.Index(css, "{") + 1
+ want := strings.Index(css, "} } e") + 2
+ if got, err := braceMatchEnd(css, start); err != nil || got != want {
+ t.Errorf("braceMatchEnd(%q, %d) = %d, %v; want %d", css, start, got, err, want)
+ }
+ // A brace that closes the wrong bracket, or none, is an error, not
+ // the end of the file.
+ for _, css := range []string{`{ a: ( }`, `{ a: "}"`} {
+ if end, err := braceMatchEnd(css, 1); err == nil {
+ t.Errorf("braceMatchEnd(%q, 1) = %d, want an error", css, end)
+ }
+ }
+}
+
func assertReducedMotion(t *testing.T, name, css string) {
t.Helper()
css = stripCSSComments(t, css)
- reduceBlocks, rest := reducedMotionBlocks(css)
+ reduceBlocks, rest := reducedMotionBlocks(t, css)
if len(reduceBlocks) == 0 {
t.Fatal(name + " declares no @media (prefers-reduced-motion: reduce) block at all")
}
diff --git a/check/markuphelp_test.go b/check/markuphelp_test.go
index b05f7c4..342e632 100644
--- a/check/markuphelp_test.go
+++ b/check/markuphelp_test.go
@@ -112,7 +112,13 @@ func stripCSSComments(t testing.TB, css string) string {
// be made to skip a rule a browser applies: between content: "/*" and
// content: "*/" a whole rule drops out of the reading and still styles
// every page. See cssSpan for what it refuses.
+//
+// It refuses brackets that do not nest, with checkCSSBrackets, so no
+// reader of what it returns has to guess where a bracket ends.
func cssWithoutComments(css string) (string, error) {
+ if err := checkCSSBrackets(css); err != nil {
+ return "", err
+ }
var b strings.Builder
for i := 0; i < len(css); {
end, comment, err := cssSpan(css, i)
@@ -127,6 +133,100 @@ func cssWithoutComments(css string) (string, error) {
return b.String(), nil
}
+// checkCSSBrackets refuses css whose brackets, read outside strings and
+// comments with cssSpan, do not nest: a (, [ or { must be closed, by
+// its own partner, before the one around it is.
+//
+// A browser and a reader that counts brackets disagree about anything
+// else. A ) that closes nothing ends nothing for a browser, so in
+// --rst-x: ); color: red the ; ends the declaration and color: red
+// applies, while a count that went below zero read the rest of the
+// block as one custom property. A ( never closed swallows the rest of
+// the block for a browser, and only for a browser.
+//
+// A { inside ( or [ is refused too. A browser reads it as a block
+// inside the value or the prelude, and a reader that takes every { for
+// a rule would start one there. No stylesheet needs one.
+func checkCSSBrackets(css string) error {
+ var open []int
+ for i := 0; i < len(css); {
+ end, comment, err := cssSpan(css, i)
+ if err != nil {
+ return err
+ }
+ if !comment {
+ if open, err = stepCSSBracket(css, i, open); err != nil {
+ return err
+ }
+ }
+ i = end
+ }
+ if len(open) > 0 {
+ return unclosedCSSBracket(css, open)
+ }
+ return nil
+}
+
+// braceMatchEnd is the index of the bracket that closes the one at
+// css[start-1], a (, [ or {, read the way checkCSSBrackets reads them:
+// a bracket inside a string or a comment is text, and one that closes
+// the wrong bracket, or none, is an error. Counting braces alone, a
+// "{" in one rule and a "}" in a later one paired up, and the block
+// ran on past its end over every rule between.
+func braceMatchEnd(css string, start int) (int, error) {
+ open := []int{start - 1}
+ for i := start; i < len(css); {
+ end, comment, err := cssSpan(css, i)
+ if err != nil {
+ return 0, err
+ }
+ if !comment {
+ if open, err = stepCSSBracket(css, i, open); err != nil {
+ return 0, err
+ }
+ if len(open) == 0 {
+ return i, nil
+ }
+ }
+ i = end
+ }
+ return 0, unclosedCSSBracket(css, open)
+}
+
+// cssPartner is the opening bracket each closing one closes.
+var cssPartner = map[byte]byte{')': '(', ']': '[', '}': '{'}
+
+// stepCSSBracket reads the piece of css at i, which cssSpan has found
+// is not a comment, against open, the indexes of the brackets open
+// there, innermost last. A string or a name starts with no bracket, so
+// only a one-byte piece can be one.
+func stepCSSBracket(css string, i int, open []int) ([]int, error) {
+ line := func(at int) int { return 1 + strings.Count(css[:at], "\n") }
+ switch c := css[i]; c {
+ case '(', '[', '{':
+ if n := len(open); c == '{' && n > 0 && css[open[n-1]] != '{' {
+ return nil, fmt.Errorf("line %d: a { opens inside the %c opened on line %d; close the %c before it", line(i), css[open[n-1]], line(open[n-1]), css[open[n-1]])
+ }
+ return append(open, i), nil
+ case ')', ']', '}':
+ n := len(open)
+ if n == 0 {
+ return nil, fmt.Errorf("line %d: a %c closes nothing", line(i), c)
+ }
+ if css[open[n-1]] != cssPartner[c] {
+ return nil, fmt.Errorf("line %d: a %c closes the %c opened on line %d", line(i), c, css[open[n-1]], line(open[n-1]))
+ }
+ return open[:n-1], nil
+ }
+ return open, nil
+}
+
+// unclosedCSSBracket names the innermost bracket open at the end.
+func unclosedCSSBracket(css string, open []int) error {
+ at := open[len(open)-1]
+ return fmt.Errorf("line %d: a %c is never closed", 1+strings.Count(css[:at], "\n"), css[at])
+}
+
// cssSpan reads the piece of css that starts at i, as a browser's
// tokenizer would, for the readers that need to know what is a comment:
// a comment, a whole string, a run of name characters, or else one
diff --git a/check/sizing_test.go b/check/sizing_test.go
index 6ddef17..80729d5 100644
--- a/check/sizing_test.go
+++ b/check/sizing_test.go
@@ -155,7 +155,7 @@ func TestTheTargetFloorIsOneToken(t *testing.T) {
// holds leaves the control at its old size on a desktop. Named
// rather than counted, so a control that loses its floor fails by
// name, and two floors merged into one rule pass.
- everyWidth := withoutTouchQueries(css)
+ everyWidth := withoutTouchQueries(t, css)
sized := regexp.MustCompile(`(?:^|;)\s*(?:min-)?(?:block|inline)-size:\s*var\(--rst-target\)`)
for _, e := range tapInventory {
if why, ok := floorElsewhere[e.Key]; ok {
@@ -290,14 +290,18 @@ var floorElsewhere = map[string]string{
// withoutTouchQueries is css with every touch query, header, braces and
// all, cut out.
-func withoutTouchQueries(css string) string {
+func withoutTouchQueries(t *testing.T, css string) string {
+ t.Helper()
var b strings.Builder
for {
i := strings.Index(css, touchQuery)
if i < 0 {
break
}
- end := braceMatchEnd(css, i+len(touchQuery))
+ end, err := braceMatchEnd(css, i+len(touchQuery))
+ if err != nil {
+ t.Fatal(err)
+ }
b.WriteString(css[:i])
css = css[end+1:]
}