Conversation
FontFamily and EmbedFont take a font stack from the caller and it was interpolated raw into the `font-family` attribute of `<g>` and into the `@font-face` rule. The ordinary CSS spelling of a stack contains a double quote, which closes the attribute, and an ampersand in a family name is an unterminated entity, so the formatter returned a document that is not well-formed XML. escapeString cannot be reused here: svgEscaper maps a space to &alecthomas#160; for xml:space="preserve" text nodes, which would rename the family. Add an XML escaper for &, <, > and " alongside it and apply it at both sites. The registered svg formatter and the zero-value default contain neither character, so their output is unchanged.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
1 participant
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.
svg.FontFamily(...)andsvg.EmbedFont(...)take a font stack from the caller. It is interpolated raw into thefont-familyattribute of<g>(formatters/svg/svg.go:112) and into the@font-facerule (formatters/svg/svg.go:197). The ordinary CSS spelling of a stack contains a double quote, which closes the attribute; an ampersand in a family name is an unterminated entity. Either way the formatter returns a document that is not well-formed XML, so a strict SVG consumer renders nothing.escapeStringcannot be reused here.svgEscaper(formatters/svg/svg.go:77-84) maps a space to and a tab to four of them, for thexml:space="preserve"text nodes it was written for; running a font stack through it renames the family. So this adds an XML escaper for&,<,>and"next to it and applies it at both sites.The sibling eight lines down already escapes what it puts in an attribute (
formatters/svg/svg.go:168,rect id="%s"). The same class was fixed on the HTML side in 879e833 ("escape configured values in HTML attributes"), which changed four sites, all informatters/html/html.go.The registered
svgformatter uses"Liberation Mono"(formatters/api.go:26) and the zero-value default isConsolas, Monaco, ...(formatters/svg/svg.go:59). Neither contains"or&, so no existing output changes.Verification
go test ./formatters/svg/ -run TestFontFamilyIsEscaped -count=1, Go 1.26.3.md5sum formatters/svg/svg.goon every row; the test file is byte-identical across rows at5e0ba8c2b6ad41c269870f02349cced8.svg.go2d9746d432eb76f310f408de1049fca3HEADfd7cfd7305ef8d88f313b9fafd3cc560113e6f0928b675be34712e88321eca62@font-faceonlyc3b0041fb443c393aefe698501011defescapeStringinstead, both sites9fd197170b9151d2435111f7ebe10bf3&from the new escaper8da314ea4c875640155f8c36361f0ddcFoo & Bar Mono"from the new escaperb0578e88e626ab112be0cbf916bbb838"Fira Code", monospaceRows 3 and 4 are the N-1 rows: each site is independently necessary. The last two rows fail different subtests, so neither input is redundant.
The
escapeStringrow fails on value transparency rather than well-formedness: the document parses, but the family comes back with U+00A0 where the caller wrote U+0020, so the two strings print identically and only the equality check catches it.The test parses the output with
encoding/xmlin strict mode and asserts thefont-familyattribute round-trips byte-identical to the configured value, and that the<style>text containsfont-family: '<value>';.Also run:
go test ./... -count=1exit 0, 7 ok / 0 FAIL.gofmt -l formatters/svg/empty. I did not runjust check-generated; nothing generated was touched. There is no changelog in this repo to update.Not covered: a literal
'inside the family name still ends the CSS string in the@font-facerule. The XML parser hands'back as', so that is a CSS-level quoting question rather than an XML one, and I left it alone. Happy to extend the escaper if you want that handled too.Disclosure: this change was written with AI assistance (Claude). The measurements and mutants above were reproduced and checked by me before opening this PR.