Keep proto fixtures LF on Windows checkouts

Fix overly strict error handling causing valid protos to be skipped

.gitattributes

@@ -0,0 +1 @@
+pkg/protodump/fixtures/* text eol=lf

.github/workflows/build.yml

@@ -1,24 +1,48 @@
 on:
   - push
-  - pull_request_target
+  - pull_request
+
+permissions:
+  contents: read
 
 jobs:
   build:
-    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        os: [ubuntu-latest, macos-latest, windows-latest]
+    runs-on: ${{ matrix.os }}
+    defaults:
+      run:
+        shell: bash
     steps:
       - uses: actions/checkout@v4
       - name: Set up Go
         uses: actions/setup-go@v5
         with:
-          go-version: '1.22'
+          go-version-file: go.mod
       - name: Install Protoc
         run: |
-          curl -LO https://github.com/protocolbuffers/protobuf/releases/download/v28.1/protoc-28.1-linux-x86_64.zip
-          unzip protoc-28.1-linux-x86_64.zip -d $HOME/.local
-          export PATH="$PATH:$HOME/.local/bin"
-          protoc --version
-          go install google.golang.org/protobuf/cmd/protoc-gen-go@latest
+          case "$RUNNER_OS" in
+            Linux) archive=linux-x86_64 ;;
+            macOS)
+              if [[ "$(uname -m)" == "arm64" ]]; then
+                archive=osx-aarch_64
+              else
+                archive=osx-x86_64
+              fi
+              ;;
+            Windows) archive=win64 ;;
+          esac
+          curl -fLsS -o "$RUNNER_TEMP/protoc.zip" "https://github.com/protocolbuffers/protobuf/releases/download/v28.1/protoc-28.1-$archive.zip"
+          python -m zipfile -e "$RUNNER_TEMP/protoc.zip" "$RUNNER_TEMP/protoc"
+          chmod +x "$RUNNER_TEMP/protoc/bin/"*
+          echo "$RUNNER_TEMP/protoc/bin" >> "$GITHUB_PATH"
+          "$RUNNER_TEMP/protoc/bin/protoc" --version
+          go install google.golang.org/protobuf/cmd/protoc-gen-go
       - name: Test
-        run: make test
+        run: go test -race ./...
+      - name: Vet
+        run: go vet ./...
       - name: Build
-        run: make build
+        run: go build ./cmd/protodump

Makefile

@@ -1,5 +1,5 @@
 build:
-	go build -o bin/protodump cmd/protodump/main.go
+	go build -o bin/protodump ./cmd/protodump
 
 fmt:
 	go fmt ./...

cmd/protodump/main.go

@@ -5,8 +5,6 @@ import (
 	"fmt"
 	"log"
 	"os"
-	"path"
-	"path/filepath"
 	"strings"
 
 	"github.com/arkadiyt/protodump/pkg/protodump"
@@ -21,47 +19,6 @@ func Debug(str string, a ...any) (int, error) {
 	return 0, nil
 }
 
-func writeFile(outputDir string, filename string, content []byte) (string, error) {
-	outputDirAbs, err := filepath.Abs(outputDir)
-	if err != nil {
-		return "", fmt.Errorf("couldn't get absolute dir for %s: %v", outputDir, err)
-	}
-
-	fileDir, fileBase := filepath.Split(filename)
-
-	parts := strings.Split(path.Clean(fileDir), string(filepath.Separator))
-	var i int
-	for i = 0; i < len(parts); i++ {
-		_, err := os.Stat(filepath.Join(outputDirAbs, filepath.Join(parts[:i+1]...)))
-		if os.IsNotExist(err) {
-			break
-		}
-	}
-
-	eval := filepath.Join(outputDirAbs, filepath.Join(parts[:i]...))
-	base, err := filepath.EvalSymlinks(eval)
-	if err != nil {
-		return "", fmt.Errorf("failed to evalsymlinks on %s: %v", eval, err)
-	}
-
-	if !strings.HasPrefix(base, outputDirAbs) {
-		return "", fmt.Errorf("invalid filepath: %s", base)
-	}
-
-	rest := filepath.Join(parts[i:]...)
-	err = os.MkdirAll(filepath.Join(base, rest), 0700)
-	if err != nil {
-		return "", fmt.Errorf("failed to mkdirall on %s: %v", rest, err)
-	}
-
-	final := filepath.Join(base, rest, fileBase)
-	err = os.WriteFile(final, content, 0700)
-	if err != nil {
-		return "", fmt.Errorf("failed to write file %s: %v", final, err)
-	}
-	return final, nil
-}
-
 func main() {
 	cwd, err := os.Getwd()
 	if err != nil {
@@ -84,10 +41,11 @@ func main() {
 		log.Fatalf("Got error scanning: %v\n", err)
 	}
 
-	err = os.MkdirAll(*output, 0700)
+	outputRoot, err := openOutputDir(*output)
 	if err != nil {
-		log.Fatalf("Failed to create output folder %s: %v\n", *output, err)
+		log.Fatalf("Failed to open output folder %q: %v\n", *output, err)
 	}
+	defer outputRoot.Close()
 
 	for _, result := range results {
 		definition, err := protodump.NewFromBytes(result)
@@ -97,11 +55,11 @@ func main() {
 
 			filename := definition.Filename()
 			if strings.HasSuffix(filename, ".proto") {
-				final, err := writeFile(*output, filename, []byte(definition.String()))
+				final, err := writeFile(outputRoot, filename, []byte(definition.String()))
 				if err != nil {
-					fmt.Printf("Failed to write %s: %v\n", final, err)
+					fmt.Printf("Failed to write %q: %v\n", filename, err)
 				} else {
-					fmt.Printf("Wrote %s\n", final)
+					fmt.Printf("Wrote %q\n", final)
 				}
 			} else {
 				// Need to investigate further

cmd/protodump/output.go

@@ -0,0 +1,55 @@
+package main
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strings"
+)
+
+func openOutputDir(dir string) (*os.Root, error) {
+	// These platforms do not provide handle-based, race-resistant roots.
+	if runtime.GOOS == "js" || runtime.GOOS == "plan9" {
+		return nil, fmt.Errorf("secure output is not supported on %s", runtime.GOOS)
+	}
+
+	dir, err := filepath.Abs(dir)
+	if err != nil {
+		return nil, err
+	}
+	if err := os.MkdirAll(dir, 0700); err != nil {
+		return nil, err
+	}
+	return os.OpenRoot(dir)
+}
+
+func writeFile(output *os.Root, filename string, content []byte) (string, error) {
+	// Descriptor names must be slash-separated relative paths on every platform.
+	if strings.ContainsAny(filename, `\:`) || !strings.HasSuffix(filename, ".proto") {
+		return "", fmt.Errorf("invalid proto filename: %q", filename)
+	}
+	name, err := filepath.Localize(filename)
+	if err != nil {
+		return "", fmt.Errorf("invalid proto filename %q: %w", filename, err)
+	}
+
+	// Both operations enforce containment, even if symlinks change between them.
+	if err := output.MkdirAll(filepath.Dir(name), 0700); err != nil {
+		return "", fmt.Errorf("couldn't create directories for %q: %w", filename, err)
+	}
+	file, err := output.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0644)
+	if err != nil {
+		return "", fmt.Errorf("couldn't create %q: %w", filename, err)
+	}
+
+	_, writeErr := file.Write(content)
+	closeErr := file.Close()
+	if writeErr != nil {
+		return "", fmt.Errorf("couldn't write %q: %w", filename, writeErr)
+	}
+	if closeErr != nil {
+		return "", fmt.Errorf("couldn't close %q: %w", filename, closeErr)
+	}
+	return filepath.Join(output.Name(), name), nil
+}

cmd/protodump/output_test.go

@@ -0,0 +1,191 @@
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+var protoContent = []byte("syntax = \"proto3\";\n")
+
+func testOutput(t *testing.T) *os.Root {
+	t.Helper()
+	root, err := openOutputDir(filepath.Join(t.TempDir(), "output"))
+	require.NoError(t, err)
+	t.Cleanup(func() { require.NoError(t, root.Close()) })
+	return root
+}
+
+func TestWriteFileCreatesProto(t *testing.T) {
+	root := testOutput(t)
+	for _, name := range []string{"service.proto", "api/v1/service.proto", "with spaces/service.proto"} {
+		t.Run(name, func(t *testing.T) {
+			filename, err := writeFile(root, name, protoContent)
+			require.NoError(t, err)
+			require.Equal(t, filepath.Join(root.Name(), filepath.FromSlash(name)), filename)
+			content, err := os.ReadFile(filename)
+			require.NoError(t, err)
+			require.Equal(t, protoContent, content)
+		})
+	}
+}
+
+func TestWriteFileRejectsInvalidNamesBeforeCreatingDirectories(t *testing.T) {
+	for _, name := range []string{
+		"", ".", "..", "../outside.proto", "/absolute.proto",
+		"api/../service.proto", "api/./service.proto", "api//service.proto",
+		`api\service.proto`, `C:/service.proto`, `C:service.proto`,
+		`//server/share/service.proto`, `\\server\share\service.proto`,
+		"api/stream:service.proto", "api/service.txt", "api/service.proto/",
+		"api/\x00.proto", "api/\xff.proto",
+	} {
+		t.Run(name, func(t *testing.T) {
+			root := testOutput(t)
+			_, err := writeFile(root, name, protoContent)
+			require.Error(t, err)
+			entries, err := os.ReadDir(root.Name())
+			require.NoError(t, err)
+			require.Empty(t, entries)
+		})
+	}
+}
+
+func TestWriteFileRejectsWindowsDevices(t *testing.T) {
+	if runtime.GOOS != "windows" {
+		t.Skip("Windows device names are platform-specific")
+	}
+	for _, name := range []string{"NUL/service.proto", "CON/service.proto", "COM1/service.proto"} {
+		t.Run(name, func(t *testing.T) {
+			root := testOutput(t)
+			_, err := writeFile(root, name, protoContent)
+			require.Error(t, err)
+			entries, err := os.ReadDir(root.Name())
+			require.NoError(t, err)
+			require.Empty(t, entries)
+		})
+	}
+}
+
+func TestWriteFilePreservesExistingFiles(t *testing.T) {
+	root := testOutput(t)
+	original := []byte("original contents")
+	filename := filepath.Join(root.Name(), "existing.proto")
+	require.NoError(t, os.WriteFile(filename, original, 0600))
+
+	_, err := writeFile(root, "existing.proto", protoContent)
+	require.ErrorIs(t, err, os.ErrExist)
+	content, err := os.ReadFile(filename)
+	require.NoError(t, err)
+	require.Equal(t, original, content)
+}
+
+func TestWriteFileRejectsExternalDirectoryLinks(t *testing.T) {
+	for _, relative := range []bool{false, true} {
+		name := "absolute"
+		if relative {
+			name = "relative"
+		}
+		t.Run(name, func(t *testing.T) {
+			root := testOutput(t)
+			outside := t.TempDir()
+			target := outside
+			if relative {
+				var err error
+				target, err = filepath.Rel(root.Name(), outside)
+				require.NoError(t, err)
+			}
+			require.NoError(t, os.Symlink(target, filepath.Join(root.Name(), "linked")))
+
+			_, err := writeFile(root, "linked/new/service.proto", protoContent)
+			require.Error(t, err)
+			entries, err := os.ReadDir(outside)
+			require.NoError(t, err)
+			require.Empty(t, entries)
+		})
+	}
+}
+
+func TestWriteFilePreservesFileLinks(t *testing.T) {
+	for _, kind := range []string{"symlink", "dangling symlink", "hard link"} {
+		t.Run(kind, func(t *testing.T) {
+			root := testOutput(t)
+			target := filepath.Join(t.TempDir(), "target.proto")
+			original := []byte("original contents")
+			if kind != "dangling symlink" {
+				require.NoError(t, os.WriteFile(target, original, 0600))
+			}
+			link := filepath.Join(root.Name(), "linked.proto")
+			if kind == "hard link" {
+				require.NoError(t, os.Link(target, link))
+			} else {
+				require.NoError(t, os.Symlink(target, link))
+			}
+
+			_, err := writeFile(root, "linked.proto", protoContent)
+			require.Error(t, err)
+			content, err := os.ReadFile(target)
+			if kind == "dangling symlink" {
+				require.ErrorIs(t, err, os.ErrNotExist)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, original, content)
+			}
+		})
+	}
+}
+
+func TestWriteFileAllowsInternalDirectoryLinks(t *testing.T) {
+	root := testOutput(t)
+	require.NoError(t, root.Mkdir("schemas", 0700))
+	require.NoError(t, os.Symlink("schemas", filepath.Join(root.Name(), "linked")))
+
+	_, err := writeFile(root, "linked/v1/service.proto", protoContent)
+	require.NoError(t, err)
+	content, err := root.ReadFile(filepath.FromSlash("schemas/v1/service.proto"))
+	require.NoError(t, err)
+	require.Equal(t, protoContent, content)
+}
+
+func TestOpenOutputDirAllowsUserSelectedSymlink(t *testing.T) {
+	target := t.TempDir()
+	link := filepath.Join(t.TempDir(), "output")
+	require.NoError(t, os.Symlink(target, link))
+	root, err := openOutputDir(link)
+	require.NoError(t, err)
+	defer root.Close()
+
+	_, err = writeFile(root, "service.proto", protoContent)
+	require.NoError(t, err)
+	content, err := os.ReadFile(filepath.Join(target, "service.proto"))
+	require.NoError(t, err)
+	require.Equal(t, protoContent, content)
+}
+
+func TestOutputRootRetainsDirectoryHandle(t *testing.T) {
+	root := testOutput(t)
+	original := root.Name()
+	moved := original + "-moved"
+	err := os.Rename(original, moved)
+	if runtime.GOOS == "windows" {
+		// Windows prevents renaming a directory while its root handle is open.
+		require.Error(t, err)
+		moved = original
+	} else {
+		require.NoError(t, err)
+		require.NoError(t, os.Mkdir(original, 0700))
+	}
+
+	_, err = writeFile(root, "service.proto", protoContent)
+	require.NoError(t, err)
+	content, err := os.ReadFile(filepath.Join(moved, "service.proto"))
+	require.NoError(t, err)
+	require.Equal(t, protoContent, content)
+	if moved != original {
+		entries, err := os.ReadDir(original)
+		require.NoError(t, err)
+		require.Empty(t, entries)
+	}
+}

go.mod

@@ -1,6 +1,6 @@
 module github.com/arkadiyt/protodump
 
-go 1.19
+go 1.26.8
 
 require (
 	github.com/stretchr/testify v1.8.1

pkg/protodump/fixtures/edition.proto

@@ -0,0 +1,12 @@
+edition = "2023";
+
+package demo;
+
+option go_package = "./;editions";
+
+message Sample {
+  int32 id = 1;
+  string name = 2;
+  repeated int32 values = 3;
+}
+

pkg/protodump/fixtures/escaping.proto2

@@ -0,0 +1,13 @@
+syntax = "proto2";
+
+package escaping;
+
+option java_package = "a\"b\\c";
+option go_package = "./;escaping";
+
+message M {
+  reserved "ev\"il";
+  optional string note = 1 [default = "evil\";\n}\nmessage Injected {\n  string pwned = 1;\n}\n//"];
+  optional bytes blob = 2 [default = "A\377\001hi"];
+}
+

pkg/protodump/proto.go

@@ -23,6 +23,40 @@ type ProtoDefinition struct {
 	filename    string
 }
 
+func escapeProtoString(s string) string {
+	var b strings.Builder
+	for i := 0; i < len(s); i++ {
+		switch c := s[i]; {
+		case c == '\\':
+			b.WriteString(`\\`)
+		case c == '"':
+			b.WriteString(`\"`)
+		case c == '\n':
+			b.WriteString(`\n`)
+		case c == '\r':
+			b.WriteString(`\r`)
+		case c == '\t':
+			b.WriteString(`\t`)
+		case c < 0x20 || c >= 0x7f:
+			fmt.Fprintf(&b, `\%03o`, c)
+		default:
+			b.WriteByte(c)
+		}
+	}
+	return b.String()
+}
+
+func editionString(d protoreflect.FileDescriptor) (string, error) {
+	edition := protodesc.ToFileDescriptorProto(d).GetEdition().String()
+	if strings.HasPrefix(edition, "EDITION_") {
+		rest := strings.TrimPrefix(edition, "EDITION_")
+		if _, err := strconv.Atoi(rest); err == nil {
+			return rest, nil
+		}
+	}
+	return "unknown", fmt.Errorf("not a source edition: %v", edition)
+}
+
 func (pd *ProtoDefinition) indent() {
 	pd.indendation += 1
 }
@@ -40,6 +74,12 @@ func (pd *ProtoDefinition) write(s string) {
 	pd.builder.WriteString(s)
 }
 
+func (pd *ProtoDefinition) writeQuoted(s string) {
+	pd.write("\"")
+	pd.write(escapeProtoString(s))
+	pd.write("\"")
+}
+
 func (pd *ProtoDefinition) String() string {
 	return pd.builder.String()
 }
@@ -143,7 +183,9 @@ func (pd *ProtoDefinition) writeField(field protoreflect.FieldDescriptor) {
 		pd.write(" [default = ")
 		kind := field.Kind().String()
 		if kind == "string" {
-			pd.write(fmt.Sprintf("\"%s\"", field.Default().String()))
+			pd.writeQuoted(field.Default().String())
+		} else if kind == "bytes" {
+			pd.writeQuoted(string(field.Default().Bytes()))
 		} else if kind == "enum" {
 			pd.write(string(field.DefaultEnumValue().Name()))
 		} else {
@@ -181,9 +223,9 @@ func (pd *ProtoDefinition) writeMessage(message protoreflect.MessageDescriptor)
 
 	for i := 0; i < message.ReservedNames().Len(); i++ {
 		name := message.ReservedNames().Get(i)
-		pd.writeIndented("reserved \"")
-		pd.write(string(name))
-		pd.write("\";\n")
+		pd.writeIndented("reserved ")
+		pd.writeQuoted(string(name))
+		pd.write(";\n")
 	}
 
 	for i := 0; i < message.ReservedRanges().Len(); i++ {
@@ -234,17 +276,16 @@ func (pd *ProtoDefinition) writeImport(fileImport protoreflect.FileImport) {
 	if fileImport.IsPublic {
 		pd.write("public ")
 	}
-	pd.write("\"")
-	pd.write(fileImport.Path())
-	pd.write("\";\n")
+	pd.writeQuoted(fileImport.Path())
+	pd.write(";\n")
 }
 
 func (pd *ProtoDefinition) writeStringFileOptions(name string, value string) {
 	pd.write("option ")
 	pd.write(name)
-	pd.write(" = \"")
-	pd.write(strings.ReplaceAll(value, "\\", "\\\\"))
-	pd.write("\";\n")
+	pd.write(" = ")
+	pd.writeQuoted(value)
+	pd.write(";\n")
 }
 
 func (pd *ProtoDefinition) writeBoolFileOptions(name string, value bool) {
@@ -304,9 +345,16 @@ func (pd *ProtoDefinition) writeFileOptions() {
 }
 
 func (pd *ProtoDefinition) writeFileDescriptor() {
-	pd.write("syntax = \"")
-	pd.write(pd.descriptor.Syntax().String())
-	pd.write("\";\n\n")
+	if pd.descriptor.Syntax() == protoreflect.Editions {
+		pd.write("edition = \"")
+		edition, _ := editionString(pd.descriptor)
+		pd.write(edition)
+		pd.write("\";\n\n")
+	} else {
+		pd.write("syntax = \"")
+		pd.write(pd.descriptor.Syntax().String())
+		pd.write("\";\n\n")
+	}
 
 	packageName := pd.descriptor.FullName()
 	if packageName != "" {

pkg/protodump/proto_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/descriptorpb"
 )
@@ -44,19 +45,19 @@ func convertProtoToFileDescriptor(filePath string) (*descriptorpb.FileDescriptor
 
 func TestDefinitions(t *testing.T) {
 	files, err := os.ReadDir(FIXTURES)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	for _, file := range files {
 		t.Run(file.Name(), func(t *testing.T) {
 			filePath := path.Join(FIXTURES, file.Name())
 			descriptor, err := convertProtoToFileDescriptor(filePath)
-			assert.NoError(t, err)
+			require.NoError(t, err)
 
 			expected, err := os.ReadFile(filePath)
-			assert.NoError(t, err)
+			require.NoError(t, err)
 
 			actual, err := NewFromDescriptor(descriptor)
-			assert.NoError(t, err)
+			require.NoError(t, err)
 
 			assert.Equal(t, string(expected), actual.String())
 		})

pkg/protodump/scan.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"os"
+	"strings"
 
 	"google.golang.org/protobuf/encoding/protowire"
 )
@@ -15,12 +16,22 @@ func consumeBytes(data []byte, position int) (int, error) {
 	start := position
 	consumedFieldOne := false
 	for {
-		number, _, len := protowire.ConsumeField(data[position:])
-		if len < 0 {
-			err := protowire.ParseError(len)
+		number, _, length := protowire.ConsumeField(data[position:])
+		if length < 0 {
+			err := protowire.ParseError(length)
+			// Treat "invalid field number" as end of data, not an error
+			if strings.Contains(err.Error(), "invalid field number") {
+				return position - start, nil
+			}
+			// Return other parse errors as actual errors
 			return position - start, fmt.Errorf("couldn't consume proto bytes: %w", err)
 		}
 
+		// Prevent infinite loop - if we can't consume any bytes, we're done
+		if length == 0 {
+			return position - start, nil
+		}
+
 		// Only consume Field 1 once (to handle the case where protobuf definitions are adjacent
 		// in program memory)
 		if number == 1 {
@@ -30,7 +41,12 @@ func consumeBytes(data []byte, position int) (int, error) {
 			consumedFieldOne = true
 		}
 
-		position += len
+		position += length
+		
+		// Additional safety check - don't read beyond data bounds
+		if position-start >= len(data[start:]) {
+			return position - start, nil
+		}
 	}
 }