Skip leading UTF-8 BOMs in YAML, Starlark, and text templates

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())
+}

pkg/files/file.go

@@ -4,13 +4,24 @@
 package files
 
 import (
+	"bytes"
 	"fmt"
 	"os"
 	"path/filepath"
 	"sort"
 	"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"}

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),

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

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

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")
+}