From 395d8aec246309450a64266592924cb98507ba4d Mon Sep 17 00:00:00 2001 From: Arkadiy Tetelman Date: Sat, 26 Sep 2026 17:44:07 -0700 Subject: [PATCH] Confine proto output writes with os.Root --- .github/workflows/build.yml | 44 ++++++-- Makefile | 2 +- cmd/protodump/main.go | 61 ++--------- cmd/protodump/output.go | 55 ++++++++++ cmd/protodump/output_test.go | 191 +++++++++++++++++++++++++++++++++++ go.mod | 2 +- 6 files changed, 288 insertions(+), 67 deletions(-) create mode 100644 cmd/protodump/output.go create mode 100644 cmd/protodump/output_test.go diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index fc3c886..bb37a08 100644 --- a/.github/workflows/build.yml +++ b/.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 diff --git a/Makefile b/Makefile index 28d433f..ecca6e0 100644 --- a/Makefile +++ b/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 ./... diff --git a/cmd/protodump/main.go b/cmd/protodump/main.go index d79f98a..9776e17 100644 --- a/cmd/protodump/main.go +++ b/cmd/protodump/main.go @@ -5,8 +5,6 @@ import ( "fmt" "log" "os" - "path" - "path/filepath" "strings" "github.com/arkadiyt/protodump/pkg/protodump" @@ -21,54 +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 base != outputDirAbs && !strings.HasPrefix(base, outputDirAbs+string(filepath.Separator)) { - 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) - - file, err := os.OpenFile(final, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0644) - if err != nil { - return "", fmt.Errorf("failed to open file %s: %v", final, err) - } - defer file.Close() - _, err = file.Write(content) - 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 { @@ -91,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) @@ -104,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 diff --git a/cmd/protodump/output.go b/cmd/protodump/output.go new file mode 100644 index 0000000..86a7fce --- /dev/null +++ b/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 +} diff --git a/cmd/protodump/output_test.go b/cmd/protodump/output_test.go new file mode 100644 index 0000000..1d47fab --- /dev/null +++ b/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) + } +} diff --git a/go.mod b/go.mod index 4c3731f..a59fab8 100644 --- a/go.mod +++ b/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