remove old SA1019 lint:ignore (#574)

* Fix path traversal in `-proto-out-dir`

A malicious file descriptor name (e.g. from an untrusted protoset or
server reflection) could escape the output directory through ../
sequences, writing attacker-controlled content to arbitrary paths.

Confine all writes to an `os.Root` opened on the output directory.

* assert

* refactor

---------

Co-authored-by: Scott Blum <dragonsinth@gmail.com>

.gitignore

@@ -1,4 +1,5 @@
 dist/
+.claude/
 .idea/
 VERSION
 .tmp/

cmd/grpcurl/grpcurl.go

@@ -15,7 +15,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 required to use APIs in other grpcurl package
+	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/grpcreflect"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"

desc_source.go

@@ -9,11 +9,11 @@ import (
 	"path/filepath"
 	"sync"
 
-	"github.com/golang/protobuf/proto"              //lint:ignore SA1019 we have to import these because some of their types appear in exported API
-	"github.com/jhump/protoreflect/desc"            //lint:ignore SA1019 same as above
-	"github.com/jhump/protoreflect/desc/protoparse" //lint:ignore SA1019 same as above
+	"github.com/golang/protobuf/proto"
+	"github.com/jhump/protoreflect/desc"
+	"github.com/jhump/protoreflect/desc/protoparse"
 	"github.com/jhump/protoreflect/desc/protoprint"
-	"github.com/jhump/protoreflect/dynamic" //lint:ignore SA1019 same as above
+	"github.com/jhump/protoreflect/dynamic"
 	"github.com/jhump/protoreflect/grpcreflect"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -309,24 +309,37 @@ func WriteProtoFiles(outProtoDirPath string, descSource DescriptorSource, symbol
 	for _, filename := range filenames {
 		allFileDescriptors = addFilesToFileDescriptorList(allFileDescriptors, expandedFiles, fds[filename])
 	}
+	return writeProtoFiles(outProtoDirPath, allFileDescriptors)
+}
+
+func writeProtoFiles(outProtoDirPath string, allFileDescriptors []*desc.FileDescriptor) error {
+	if err := os.MkdirAll(outProtoDirPath, 0755); err != nil {
+		return fmt.Errorf("failed to create output directory %q: %w", outProtoDirPath, err)
+	}
+	root, err := os.OpenRoot(outProtoDirPath)
+	if err != nil {
+		return fmt.Errorf("failed to open output directory %q: %w", outProtoDirPath, err)
+	}
+	defer root.Close()
 	pr := protoprint.Printer{}
 	// now we can serialize to files
 	for i := range allFileDescriptors {
-		if err := writeProtoFile(outProtoDirPath, allFileDescriptors[i], &pr); err != nil {
+		if err := writeProtoFile(root, allFileDescriptors[i], &pr); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-func writeProtoFile(outProtoDirPath string, fd *desc.FileDescriptor, pr *protoprint.Printer) error {
-	outFile := filepath.Join(outProtoDirPath, fd.GetFullyQualifiedName())
-	outDir := filepath.Dir(outFile)
-	if err := os.MkdirAll(outDir, 0777); err != nil {
-		return fmt.Errorf("failed to create directory %q: %w", outDir, err)
+func writeProtoFile(root *os.Root, fd *desc.FileDescriptor, pr *protoprint.Printer) error {
+	// root confines all path operations to the output directory, so a
+	// malicious file name (e.g. "../escape.proto") cannot write outside it
+	outFile := fd.GetFullyQualifiedName()
+	if err := root.MkdirAll(filepath.Dir(outFile), 0755); err != nil {
+		return fmt.Errorf("failed to create directory for %q: %w", outFile, err)
 	}
 
-	f, err := os.Create(outFile)
+	f, err := root.Create(outFile)
 	if err != nil {
 		return fmt.Errorf("failed to create proto file %q: %w", outFile, err)
 	}

desc_source_test.go

@@ -3,9 +3,11 @@ package grpcurl
 import (
 	"bytes"
 	"os"
+	"path/filepath"
 	"testing"
 
-	"github.com/golang/protobuf/proto" //lint:ignore SA1019 we have to import this because it appears in exported API
+	"github.com/golang/protobuf/proto"
+	"github.com/jhump/protoreflect/desc"
 	"google.golang.org/protobuf/types/descriptorpb"
 )
 
@@ -60,3 +62,47 @@ func checkWriteProtoset(t *testing.T, descSrc DescriptorSource, protoset *descri
 		t.Fatalf("written protoset not equal to input:\nExpecting: %s\nActual: %s", protoset, &result)
 	}
 }
+
+func writeProtoFileForTest(t *testing.T, dir, fdName string) error {
+	t.Helper()
+	fd, err := desc.CreateFileDescriptor(&descriptorpb.FileDescriptorProto{
+		Name:   proto.String(fdName),
+		Syntax: proto.String("proto3"),
+	})
+	if err != nil {
+		t.Fatalf("failed to create file descriptor: %v", err)
+	}
+	return writeProtoFiles(dir, []*desc.FileDescriptor{fd})
+}
+
+func TestWriteProtoFile_NormalPath(t *testing.T) {
+	dir := t.TempDir()
+	if err := writeProtoFileForTest(t, dir, "foo/bar.proto"); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if _, err := os.Stat(filepath.Join(dir, "foo", "bar.proto")); err != nil {
+		t.Fatalf("expected output file not created: %v", err)
+	}
+}
+
+func TestWriteProtoFile_RejectsPathTraversal(t *testing.T) {
+	dir := t.TempDir()
+	if err := writeProtoFileForTest(t, dir, "../escape.proto"); err == nil {
+		t.Fatal("expected error for path-traversing descriptor name, got nil")
+	}
+	escapePath := filepath.Join(dir, "..", "escape.proto")
+	if _, err := os.Stat(escapePath); err == nil {
+		t.Fatalf("file was created outside output directory at %q", escapePath)
+	}
+}
+
+func TestWriteProtoFile_RejectsDeepPathTraversal(t *testing.T) {
+	dir := t.TempDir()
+	if err := writeProtoFileForTest(t, dir, "foo/../../../escape.proto"); err == nil {
+		t.Fatal("expected error for path-traversing descriptor name, got nil")
+	}
+	escapePath := filepath.Join(dir, "foo", "..", "..", "..", "escape.proto")
+	if _, err := os.Stat(escapePath); err == nil {
+		t.Fatalf("file was created outside output directory at %q", escapePath)
+	}
+}

format.go

@@ -11,10 +11,10 @@ import (
 	"strings"
 	"sync"
 
-	"github.com/golang/protobuf/jsonpb"     //lint:ignore SA1019 we have to import these because some of their types appear in exported API
-	"github.com/golang/protobuf/proto"      //lint:ignore SA1019 same as above
-	"github.com/jhump/protoreflect/desc"    //lint:ignore SA1019 same as above
-	"github.com/jhump/protoreflect/dynamic" //lint:ignore SA1019 same as above
+	"github.com/golang/protobuf/jsonpb"
+	"github.com/golang/protobuf/proto"
+	"github.com/jhump/protoreflect/desc"
+	"github.com/jhump/protoreflect/dynamic"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/status"
@@ -330,7 +330,6 @@ func (r anyResolverWithFallback) Resolve(typeUrl string) (proto.Message, error)
 	if slash := strings.LastIndex(mname, "/"); slash >= 0 {
 		mname = mname[slash+1:]
 	}
-	//lint:ignore SA1019 new non-deprecated API requires other code changes; deferring...
 	mt := proto.MessageType(mname)
 	if mt != nil {
 		return reflect.New(mt.Elem()).Interface().(proto.Message), nil

format_test.go

@@ -7,9 +7,9 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/golang/protobuf/jsonpb"  //lint:ignore SA1019 we have to import these because some of their types appear in exported API
-	"github.com/golang/protobuf/proto"   //lint:ignore SA1019 same as above
-	"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above
+	"github.com/golang/protobuf/jsonpb"
+	"github.com/golang/protobuf/proto"
+	"github.com/jhump/protoreflect/desc"
 	"google.golang.org/grpc/metadata"
 	"google.golang.org/protobuf/types/known/structpb"
 )

grpcurl.go

@@ -23,10 +23,10 @@ import (
 	"sync"
 	"time"
 
-	"github.com/golang/protobuf/proto"   //lint:ignore SA1019 we have to import these because some of their types appear in exported API
-	"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above
+	"github.com/golang/protobuf/proto"
+	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/desc/protoprint"
-	"github.com/jhump/protoreflect/dynamic" //lint:ignore SA1019 same as above
+	"github.com/jhump/protoreflect/dynamic"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/connectivity"
 	"google.golang.org/grpc/credentials"

grpcurl_test.go

@@ -12,9 +12,9 @@ import (
 	"testing"
 	"time"
 
-	"github.com/golang/protobuf/jsonpb"  //lint:ignore SA1019 we have to import these because some of their types appear in exported API
-	"github.com/golang/protobuf/proto"   //lint:ignore SA1019 same as above
-	"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above
+	"github.com/golang/protobuf/jsonpb"
+	"github.com/golang/protobuf/proto"
+	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/grpcreflect"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"

invoke.go

@@ -9,10 +9,10 @@ import (
 	"sync"
 	"sync/atomic"
 
-	"github.com/golang/protobuf/jsonpb"     //lint:ignore SA1019 we have to import these because some of their types appear in exported API
-	"github.com/golang/protobuf/proto"      //lint:ignore SA1019 same as above
-	"github.com/jhump/protoreflect/desc"    //lint:ignore SA1019 same as above
-	"github.com/jhump/protoreflect/dynamic" //lint:ignore SA1019 same as above
+	"github.com/golang/protobuf/jsonpb"
+	"github.com/golang/protobuf/proto"
+	"github.com/jhump/protoreflect/desc"
+	"github.com/jhump/protoreflect/dynamic"
 	"github.com/jhump/protoreflect/dynamic/grpcdynamic"
 	"github.com/jhump/protoreflect/grpcreflect"
 	"google.golang.org/grpc"