fix: overwrite symlinks when extracting tarballs (#867)

1. Fix the bug that symbolic links are not automatically overwritten when extracted from a tar archive to the File store. 2. Rename some functions and variables for better readability. 3. Add corresponding unit tests. 4. Improve docs and error messages. Fixes #865 Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
oras-project · Jan 15, 2025 · 1a9e250 · 1a9e250
1 parent eeb21fc
commit 1a9e250
Show file tree

Hide file tree

Showing 5 changed files with 629 additions and 40 deletions.
diff --git a/content/file/file.go b/content/file/file.go
@@ -394,7 +394,8 @@ func (s *Store) Predecessors(ctx context.Context, node ocispec.Descriptor) ([]oc
 	return s.graph.Predecessors(ctx, node)
 }
 
-// Add adds a file into the file store.
+// Add adds a file or a directory into the file store.
+// Hard links within the directory are treated as regular files.
 func (s *Store) Add(ctx context.Context, name, mediaType, path string) (ocispec.Descriptor, error) {
 	if s.isClosedSet() {
 		return ocispec.Descriptor{}, ErrStoreClosed

diff --git a/content/file/file_test.go b/content/file/file_test.go
@@ -18,6 +18,7 @@ package file
 import (
 	"bytes"
 	"context"
+	_ "crypto/sha256"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -29,8 +30,6 @@ import (
 	"sync/atomic"
 	"testing"
 
-	_ "crypto/sha256"
-
 	"github.com/opencontainers/go-digest"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"golang.org/x/sync/errgroup"
@@ -66,6 +65,21 @@ func (t *storageTracker) Exists(ctx context.Context, target ocispec.Descriptor)
 	return t.Storage.Exists(ctx, target)
 }
 
+type storageMock struct {
+	content.Storage
+
+	OnFetch func(ctx context.Context, desc ocispec.Descriptor) error
+}
+
+func (m *storageMock) Fetch(ctx context.Context, desc ocispec.Descriptor) (io.ReadCloser, error) {
+	if m.OnFetch != nil {
+		if err := m.OnFetch(ctx, desc); err != nil {
+			return nil, err
+		}
+	}
+	return m.Storage.Fetch(ctx, desc)
+}
+
 func TestStoreInterface(t *testing.T) {
 	var store interface{} = &Store{}
 	if _, ok := store.(oras.Target); !ok {
@@ -1615,21 +1629,6 @@ func TestStore_File_Push_RestoreDuplicates_NotFound(t *testing.T) {
 	}
 }
 
-type storageMock struct {
-	content.Storage
-
-	OnFetch func(ctx context.Context, desc ocispec.Descriptor) error
-}
-
-func (m *storageMock) Fetch(ctx context.Context, desc ocispec.Descriptor) (io.ReadCloser, error) {
-	if m.OnFetch != nil {
-		if err := m.OnFetch(ctx, desc); err != nil {
-			return nil, err
-		}
-	}
-	return m.Storage.Fetch(ctx, desc)
-}
-
 func TestStore_File_Push_RestoreDuplicates_DuplicateName(t *testing.T) {
 	mediaType := "test"
 	content := []byte("hello world")
@@ -3202,6 +3201,159 @@ func TestCopyGraph_FileToMemory_PartialCopy(t *testing.T) {
 	}
 }
 
+func TestStore_resolveWritePath_PathTraversal(t *testing.T) {
+	tempDir := t.TempDir()
+
+	tests := []struct {
+		name               string
+		workingDir         string
+		allowPathTraversal bool
+		input              string
+		want               string
+		wantErr            error
+	}{
+		{
+			name:               "good relative path with path traversal disallowed",
+			workingDir:         tempDir,
+			allowPathTraversal: false,
+			input:              "test.txt",
+			want:               filepath.Join(tempDir, "test.txt"),
+			wantErr:            nil,
+		},
+		{
+			name:               "good absolute path with path traversal disallowed",
+			workingDir:         tempDir,
+			allowPathTraversal: false,
+			input:              filepath.Join(tempDir, "test.txt"),
+			want:               filepath.Join(tempDir, "test.txt"),
+			wantErr:            nil,
+		},
+		{
+			name:               "bad absolute path with path traversal disallowed",
+			workingDir:         tempDir,
+			allowPathTraversal: false,
+			input:              filepath.Clean(filepath.Join(tempDir, "../test.txt")),
+			want:               "",
+			wantErr:            ErrPathTraversalDisallowed,
+		},
+		{
+			name:               "bad absolute path with path traversal allowed",
+			workingDir:         tempDir,
+			allowPathTraversal: true,
+			input:              filepath.Clean(filepath.Join(tempDir, "../test.txt")),
+			want:               filepath.Clean(filepath.Join(tempDir, "../test.txt")),
+			wantErr:            nil,
+		},
+		{
+			name:               "bad relative path with path traversal disallowed",
+			workingDir:         tempDir,
+			allowPathTraversal: false,
+			input:              "../test.txt",
+			want:               "",
+			wantErr:            ErrPathTraversalDisallowed,
+		},
+		{
+			name:               "bad relative path with path traversal allowed",
+			workingDir:         tempDir,
+			allowPathTraversal: true,
+			input:              "../test.txt",
+			want:               filepath.Clean(filepath.Join(tempDir, "../test.txt")),
+			wantErr:            nil,
+		},
+		{
+			name:               "bad relative directory path with path traversal disallowed",
+			workingDir:         tempDir,
+			allowPathTraversal: false,
+			input:              "..",
+			want:               "",
+			wantErr:            ErrPathTraversalDisallowed,
+		},
+		{
+			name:               "bad relative directory path with path traversal allowed",
+			workingDir:         tempDir,
+			allowPathTraversal: true,
+			input:              "..",
+			want:               filepath.Clean(filepath.Join(tempDir, "..")),
+			wantErr:            nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s, err := New(tt.workingDir)
+			if err != nil {
+				t.Fatalf("failed to create store: %v", err)
+			}
+			defer s.Close()
+
+			s.AllowPathTraversalOnWrite = tt.allowPathTraversal
+			got, err := s.resolveWritePath(tt.input)
+			if !errors.Is(err, tt.wantErr) {
+				t.Fatalf("resolveWritePath() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			if got != tt.want {
+				t.Errorf("resolveWritePath() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestStore_resolveWritePath_Overwrite(t *testing.T) {
+	t.Run("Target file already exists", func(t *testing.T) {
+		tempDir := t.TempDir()
+
+		s, err := New(tempDir)
+		if err != nil {
+			t.Fatalf("failed to create store: %v", err)
+		}
+		defer s.Close()
+		s.DisableOverwrite = true
+
+		existingFile := filepath.Join(tempDir, "test.txt")
+		if err := os.WriteFile(existingFile, []byte("content"), 0444); err != nil {
+			t.Fatalf("failed to create existing file: %v", err)
+		}
+		if _, err := s.resolveWritePath("test.txt"); !errors.Is(err, ErrOverwriteDisallowed) {
+			t.Errorf("resolveWritePath() error = %v, wantErr %v", err, ErrOverwriteDisallowed)
+		}
+	})
+
+	t.Run("Target file does not exist", func(t *testing.T) {
+		tempDir := t.TempDir()
+
+		s, err := New(tempDir)
+		if err != nil {
+			t.Fatalf("failed to create store: %v", err)
+		}
+		defer s.Close()
+		s.DisableOverwrite = true
+
+		got, err := s.resolveWritePath("test.txt")
+		if err != nil {
+			t.Fatalf("resolveWritePath() error = %v", err)
+		}
+		if want := filepath.Join(tempDir, "test.txt"); got != want {
+			t.Errorf("resolveWritePath() = %v, want %v", got, want)
+		}
+	})
+
+	t.Run("Invalid path", func(t *testing.T) {
+		tempDir := t.TempDir()
+
+		s, err := New(tempDir)
+		if err != nil {
+			t.Fatalf("failed to create store: %v", err)
+		}
+		defer s.Close()
+		s.AllowPathTraversalOnWrite = true
+		s.DisableOverwrite = true
+
+		if _, err := s.resolveWritePath("\x00invalid:path/test.txt"); err == nil {
+			t.Error("resolveWritePath() error = nil, wantErr = true")
+		}
+	})
+}
+
 func equalDescriptorSet(actual []ocispec.Descriptor, expected []ocispec.Descriptor) bool {
 	if len(actual) != len(expected) {
 		return false