diff --git a/pkg/cmd/template/utf8_bom_test.go b/pkg/cmd/template/utf8_bom_test.go new file mode 100644 index 00000000..37a3ad9e --- /dev/null +++ b/pkg/cmd/template/utf8_bom_test.go @@ -0,0 +1,122 @@ +// Copyright 2024 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package template_test + +import ( + "testing" + + cmdtpl "carvel.dev/ytt/pkg/cmd/template" + "carvel.dev/ytt/pkg/cmd/ui" + "carvel.dev/ytt/pkg/files" + "github.com/stretchr/testify/require" +) + +const utf8BOM = "\xEF\xBB\xBF" + +// A file saved with a UTF-8 BOM should render exactly like the same file +// without one. Before this was handled the BOM was read as part of the first +// key, so `test: #@ None` rendered as a quoted, escaped key instead of `test`. +func TestUTF8BOMRendersTheSameAsNoBOM(t *testing.T) { + tests := []struct { + name string + template string + expected string + }{ + { + name: "annotated value", + template: "test: #@ None\n", + expected: "test: null\n", + }, + { + name: "explicit document marker", + template: "---\ntest: 1\n", + expected: "test: 1\n", + }, + { + name: "ytt comment first", + template: "#! a comment\ntest: 1\n", + expected: "test: 1\n", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + withoutBOM := renderOneFile(t, []byte(tc.template)) + require.Equal(t, tc.expected, withoutBOM, + "the control case rendered unexpectedly") + + withBOM := renderOneFile(t, []byte(utf8BOM+tc.template)) + require.Equal(t, withoutBOM, withBOM, + "a leading UTF-8 BOM changed the output") + }) + } +} + +// Only a leading BOM is a stream marker. One part way through a file is +// ordinary content, and the emitter keeps it by quoting and escaping it. +func TestUTF8BOMInsideAFileIsLeftAlone(t *testing.T) { + rendered := renderOneFile(t, []byte("test: \"a"+utf8BOM+"b\"\n")) + require.Equal(t, "test: \"a\\uFEFFb\"\n", rendered) +} + +func TestUTF8BOMStarlarkLibraryLoadsTheSameAsNoBOM(t *testing.T) { + renderWithLibrary := func(t *testing.T, libraryData []byte) string { + t.Helper() + + dataYML := "#@ load(\"values.star\", \"value\")\nresult: #@ value\n" + filesToProcess := []*files.File{ + files.MustNewFileFromSource( + files.NewBytesSource("data.yml", []byte(dataYML))), + files.MustNewFileFromSource( + files.NewBytesSource("values.star", libraryData)), + } + + out := cmdtpl.NewOptions().RunWithFiles( + cmdtpl.Input{Files: filesToProcess}, ui.NewTTY(false)) + require.NoError(t, out.Err) + require.Len(t, out.Files, 1, "unexpected number of output files") + + return string(out.Files[0].Bytes()) + } + + const library = "value = \"from starlark\"\n" + withoutBOM := renderWithLibrary(t, []byte(library)) + require.Equal(t, "result: from starlark\n", withoutBOM, + "the control case rendered unexpectedly") + + withBOM := renderWithLibrary(t, []byte(utf8BOM+library)) + require.Equal(t, withoutBOM, withBOM, + "a leading UTF-8 BOM changed how a Starlark library loaded") +} + +func TestUTF8BOMTextTemplateRendersTheSameAsNoBOM(t *testing.T) { + const textTemplate = "result: (@= \"from text template\" @)\n" + + withoutBOM := renderOneNamedFile(t, "data.txt", []byte(textTemplate)) + require.Equal(t, "result: from text template\n", withoutBOM, + "the control case rendered unexpectedly") + + withBOM := renderOneNamedFile(t, "data.txt", []byte(utf8BOM+textTemplate)) + require.Equal(t, withoutBOM, withBOM, + "a leading UTF-8 BOM changed the text-template output") +} + +func renderOneFile(t *testing.T, data []byte) string { + return renderOneNamedFile(t, "data.yml", data) +} + +func renderOneNamedFile(t *testing.T, name string, data []byte) string { + t.Helper() + + filesToProcess := []*files.File{ + files.MustNewFileFromSource(files.NewBytesSource(name, data)), + } + + input := cmdtpl.Input{Files: filesToProcess} + out := cmdtpl.NewOptions().RunWithFiles(input, ui.NewTTY(false)) + require.NoError(t, out.Err) + require.Len(t, out.Files, 1, "unexpected number of output files") + + return string(out.Files[0].Bytes()) +} diff --git a/pkg/files/file.go b/pkg/files/file.go index 6a956919..d22a57d8 100644 --- a/pkg/files/file.go +++ b/pkg/files/file.go @@ -4,6 +4,7 @@ package files import ( + "bytes" "fmt" "os" "path/filepath" @@ -11,6 +12,16 @@ import ( "strings" ) +// utf8BOM is the UTF-8 encoding of U+FEFF. Some editors, and Windows tooling in +// particular, write it at the start of a file to record the encoding. +var utf8BOM = []byte{0xEF, 0xBB, 0xBF} + +// TrimUTF8BOM returns data without a leading UTF-8 byte order mark. It only +// removes the stream marker; a U+FEFF later in the input remains content. +func TrimUTF8BOM(data []byte) []byte { + return bytes.TrimPrefix(data, utf8BOM) +} + var ( yamlExts = []string{".yaml", ".yml"} starlarkExts = []string{".star"} diff --git a/pkg/workspace/template_loader.go b/pkg/workspace/template_loader.go index 539d4eca..9fe695cd 100644 --- a/pkg/workspace/template_loader.go +++ b/pkg/workspace/template_loader.go @@ -227,6 +227,8 @@ func (l *TemplateLoader) EvalText(libraryCtx LibraryExecutionContext, file *file return nil, plainRootNode, nil } + fileBs = files.TrimUTF8BOM(fileBs) + textRoot, err := texttemplate.NewParser().Parse(fileBs, file.RelativePath()) if err != nil { return nil, nil, fmt.Errorf("Parsing text template '%s': %s", file.RelativePath(), err) @@ -262,6 +264,8 @@ func (l *TemplateLoader) EvalStarlark(libraryCtx LibraryExecutionContext, file * l.ui.Debugf("## file %s\n", file.RelativePath()) + fileBs = files.TrimUTF8BOM(fileBs) + instructions := template.NewInstructionSet() compiledTemplate := template.NewCompiledTemplate( file.RelativePath(), template.NewCodeFromBytes(fileBs, instructions), diff --git a/pkg/yamlmeta/document_set.go b/pkg/yamlmeta/document_set.go index 7e494cc7..7f214f26 100644 --- a/pkg/yamlmeta/document_set.go +++ b/pkg/yamlmeta/document_set.go @@ -6,6 +6,8 @@ package yamlmeta import ( "bytes" "io" + + "carvel.dev/ytt/pkg/files" ) type DocSetOpts struct { @@ -18,6 +20,11 @@ type DocSetOpts struct { func NewDocumentSetFromBytes(data []byte, opts DocSetOpts) (*DocumentSet, error) { parserOpts := ParserOpts{WithoutComments: opts.WithoutComments, Strict: opts.Strict} + // Trimmed here as well as in ParseBytes so that the bytes retained below, + // which back AsSourceBytes and the source lines shown in template errors, + // are the same bytes that were parsed. + data = files.TrimUTF8BOM(data) + docSet, err := NewParser(parserOpts).ParseBytes(data, opts.AssociatedName) if err != nil { return nil, err diff --git a/pkg/yamlmeta/parser.go b/pkg/yamlmeta/parser.go index 0eb6a57c..81341cf8 100644 --- a/pkg/yamlmeta/parser.go +++ b/pkg/yamlmeta/parser.go @@ -15,6 +15,7 @@ import ( "strings" "carvel.dev/ytt/pkg/filepos" + "carvel.dev/ytt/pkg/files" "carvel.dev/ytt/pkg/yamlmeta/internal/yaml.v2" ) @@ -42,6 +43,11 @@ func NewParser(opts ParserOpts) *Parser { func (p *Parser) ParseBytes(data []byte, associatedName string) (*DocumentSet, error) { p.associatedName = associatedName + // A BOM marks the encoding of the stream and is not content. It has to go + // before the document marker check below: a BOM in front of "---" hides the + // marker, so the parser prepends its own and the input stops parsing. + data = files.TrimUTF8BOM(data) + // YAML library uses 0-based line numbers for nodes (but, first line in a text file is typically line 1) nodeLineCorrection := 1 // YAML library uses 1-based line numbers for errors diff --git a/pkg/yamlmeta/parser_bom_test.go b/pkg/yamlmeta/parser_bom_test.go new file mode 100644 index 00000000..1139f244 --- /dev/null +++ b/pkg/yamlmeta/parser_bom_test.go @@ -0,0 +1,71 @@ +// Copyright 2024 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package yamlmeta_test + +import ( + "testing" + + "carvel.dev/ytt/pkg/yamlmeta" + "github.com/stretchr/testify/require" +) + +const utf8BOM = "\xEF\xBB\xBF" + +func parseForBOMTest(t *testing.T, data string) *yamlmeta.DocumentSet { + t.Helper() + + opts := yamlmeta.ParserOpts{WithoutComments: false} + docSet, err := yamlmeta.NewParser(opts).ParseBytes([]byte(data), "t.yml") + require.NoError(t, err) + + return docSet +} + +func printForBOMTest(docSet *yamlmeta.DocumentSet) string { + opts := yamlmeta.PrinterOpts{ExcludeRefs: true} + return yamlmeta.NewPrinterWithOpts(nil, opts).PrintStr(docSet) +} + +// A UTF-8 BOM marks the encoding of a stream; it is not part of the first +// key. Left in place it becomes a leading U+FEFF on that key, which then +// renders as a quoted, escaped key in the output. +func TestParserSkipsUTF8BOM(t *testing.T) { + tests := []struct { + name string + data string + }{ + {name: "plain mapping", data: "test: null\n"}, + {name: "document marker", data: "---\ntest: null\n"}, + {name: "leading comment", data: "#! a comment\ntest: null\n"}, + {name: "two documents", data: "test: null\n---\nsecond: null\n"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + withBOM := parseForBOMTest(t, utf8BOM+tc.data) + withoutBOM := parseForBOMTest(t, tc.data) + + require.Equal(t, + printForBOMTest(withoutBOM), printForBOMTest(withBOM), + "a leading UTF-8 BOM changed how the document parsed") + }) + } +} + +// The document marker check runs on the raw input, so a BOM in front of +// "---" hides the marker. The parser then prepends its own marker, which +// shifts reported positions and can make the input fail to parse at all. +func TestParserPositionsAreCorrectAfterUTF8BOM(t *testing.T) { + const data = "---\ntest: null\n" + + withBOM := parseForBOMTest(t, utf8BOM+data) + withoutBOM := parseForBOMTest(t, data) + + require.Equal(t, len(withoutBOM.Items), len(withBOM.Items), + "a leading UTF-8 BOM changed the number of parsed documents") + + wantPos := withoutBOM.Items[0].Position.AsIntString() + require.Equal(t, wantPos, withBOM.Items[0].Position.AsIntString(), + "a leading UTF-8 BOM shifted the reported line number") +}