fix(snapshot s3): reject object keys with ".." segments and never overwrite a downloaded object

cmd/kosli/snapshotS3.go

@@ -15,6 +15,7 @@ const snapshotS3ShortDesc = `Report a snapshot of the content of an AWS S3 bucke
 const snapshotS3LongDesc = snapshotS3ShortDesc + awsAuthDesc + `
 You can report the entire bucket content, or filter some of the content using ^--include^ / ^--exclude^ (literal prefix match) or ^--include-regex^ / ^--exclude-regex^ (Go regular expressions matched against the full object key).
 In all cases, the content is reported as one artifact. If you wish to report separate files/dirs within the same bucket as separate artifacts, you need to run the command twice.
+Object keys that cannot be stored as a local file, such as keys containing a ^..^ path segment, are rejected and fail the snapshot, naming the key. Two keys that resolve to the same local file are also an error. A legitimate key of that shape can be left out with ^--exclude-regex^ (anchor and escape it, since the pattern is a regular expression matched against the whole key); when ^--include^ or ^--include-regex^ is set, exclude filters are ignored, so narrow the include filter instead.
 
 ` + kosliIgnoreDesc
 

internal/aws/aws.go

@@ -4,12 +4,15 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/hex"
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"regexp"
 	"strings"
 	"sync"
+	"syscall"
 	"time"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -25,7 +28,6 @@ import (
 	"github.com/kosli-dev/cli/internal/digest"
 	"github.com/kosli-dev/cli/internal/filters"
 	"github.com/kosli-dev/cli/internal/logger"
-	"github.com/kosli-dev/cli/internal/utils"
 )
 
 // EcsEnvRequest represents the PUT request body to be sent to kosli from ECS
@@ -553,11 +555,63 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex
 	return s3Data, nil
 }
 
+// localPathForS3Key turns an S3 object key into a path under the download
+// directory, or rejects it. A key holding a ".." segment resolves onto a path
+// it does not name, taking another key's place or leaving the directory.
+func localPathForS3Key(key string) (string, error) {
+	// Windows separates on '\\' and drops trailing dots and spaces from a
+	// name, so ".. " and "..." resolve as ".." there.
+	segments := strings.FieldsFunc(key, func(r rune) bool { return r == '/' || r == '\\' })
+	for _, segment := range segments {
+		if strings.HasPrefix(segment, "..") && strings.TrimRight(segment, ". ") == "" {
+			return "", unusableS3KeyError(key, `contains a segment that resolves to ".."`)
+		}
+	}
+
+	// A leading '\\' is left for filepath.IsLocal: rooted on Windows, an
+	// ordinary filename elsewhere.
+	rel := strings.TrimLeft(key, "/")
+	if filepath.Clean(rel) == "." {
+		return "", unusableS3KeyError(key, "names no file")
+	}
+	if !filepath.IsLocal(rel) {
+		return "", unusableS3KeyError(key, "is not a local path")
+	}
+
+	return rel, nil
+}
+
+// unusableS3KeyError is only for failures the key itself causes. Advising
+// exclusion on a machine fault such as a full disk would drop a legitimate
+// object from the snapshot.
+func unusableS3KeyError(key, reason string) error {
+	return fmt.Errorf("object key [%s] cannot be stored as a local file: %s; exclude it with --exclude-regex, or narrow the include filter if one is set", key, reason)
+}
+
 func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket string, logger *logger.Logger) error {
-	file, err := utils.CreateFile(filepath.Join(dirName, key))
+	rel, err := localPathForS3Key(key)
 	if err != nil {
 		return err
 	}
+	dest := filepath.Join(dirName, rel)
+	err = os.MkdirAll(filepath.Dir(dest), 0770)
+	if errors.Is(err, syscall.ENOTDIR) {
+		// Legal in S3, impossible on disk: an object "a" and a key under "a/".
+		return unusableS3KeyError(key, "one of its parent prefixes has already been downloaded as an object")
+	}
+	if err != nil {
+		return fmt.Errorf("object key [%s]: %w", key, err)
+	}
+	// O_EXCL fails the snapshot when two keys map to one file rather than
+	// letting the second overwrite the first. Directories are not covered:
+	// "A/x" and "a/y" share one on a case-insensitive filesystem.
+	file, err := os.OpenFile(dest, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
+	if errors.Is(err, fs.ErrExist) {
+		return unusableS3KeyError(key, "another object already downloaded to the same local path")
+	}
+	if err != nil {
+		return fmt.Errorf("object key [%s]: %w", key, err)
+	}
 	defer func() {
 		if err := file.Close(); err != nil {
 			logger.Warn("failed to close file %s: %v", file.Name(), err)
@@ -570,7 +624,7 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin
 		WriterAt: file,
 	})
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to download object key [%s]: %w", key, err)
 	}
 	if result.ContentLength != nil {
 		logger.Debug("downloaded", file.Name(), *result.ContentLength, "bytes")

internal/aws/aws_test.go

@@ -3,7 +3,11 @@ package aws
 import (
 	"context"
 	"fmt"
+	"io/fs"
+	"os"
+	"path/filepath"
 	"regexp"
+	"runtime"
 	"testing"
 	"time"
 
@@ -1227,6 +1231,221 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientFilterEquivalence() {
 	}
 }
 
+func (suite *AWSTestSuite) TestGetS3DataFromClientRejectsKeysWithDotDotSegments() {
+	trustedBody := []byte("trusted release content\n")
+	attackerBody := []byte("attacker controlled content\n")
+
+	poisoned := &FakeS3Client{
+		Bucket: fakeS3TestBucketName,
+		Objects: map[string][]byte{
+			"protected/release.bin":                      trustedBody,
+			"uploads/user-a/../../protected/release.bin": attackerBody,
+		},
+	}
+
+	_, err := getS3DataFromClient(poisoned, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger())
+	require.Error(suite.T(), err, "a key containing a \"..\" segment must fail the snapshot instead of silently overwriting another object's download")
+	require.Contains(suite.T(), err.Error(), "uploads/user-a/../../protected/release.bin")
+}
+
+// Accept rows compare joined paths because the helper returns the key
+// uncleaned; filepath.Join is what collapses "a//b" and "./a.txt".
+func (suite *AWSTestSuite) TestLocalPathForS3Key() {
+	for _, t := range []struct {
+		name       string
+		key        string
+		wantPath   string // accept: the path filepath.Join(dir, key) produced before this change
+		wantErr    bool
+		wantErrMsg string
+	}{
+		{name: "an ordinary nested key", key: "protected/release.bin", wantPath: "protected/release.bin"},
+		{name: "a plain filename", key: "a.txt", wantPath: "a.txt"},
+		{name: "a short nested key", key: "a/z", wantPath: "a/z"},
+		{name: "a dotfile", key: ".kosli_ignore", wantPath: ".kosli_ignore"},
+		{name: "a key with spaces", key: "file with spaces.txt", wantPath: "file with spaces.txt"},
+		{name: "a key with punctuation", key: "weird!*'().txt", wantPath: "weird!*'().txt"},
+		{name: "a unicode key", key: "ünïcödé/файл.txt", wantPath: "ünïcödé/файл.txt"},
+		{name: "a dot followed by a space is a literal name", key: ". ", wantPath: ". "},
+		{name: "a name that merely starts with two dots", key: "..hidden", wantPath: "..hidden"},
+		{name: "a backslash key is a literal filename on this OS", key: `dir\file.txt`, wantPath: `dir\file.txt`},
+		{name: "a leading slash is trimmed", key: "/etc/passwd", wantPath: "etc/passwd"},
+		{name: "doubled leading slashes are trimmed", key: "//x", wantPath: "x"},
+		{name: "a leading dot segment is dropped by Join", key: "./a.txt", wantPath: "a.txt"},
+		{name: "a doubled interior slash is collapsed by Join", key: "a//b", wantPath: "a/b"},
+		{name: "a dot segment is dropped by Join", key: "a/./b", wantPath: "a/b"},
+		// filepath.IsLocal rejects reserved device names and colons on Windows only.
+		{name: "a reserved Windows name", key: "CON", wantPath: "CON", wantErr: runtime.GOOS == "windows", wantErrMsg: "is not a local path"},
+		{name: "a drive-looking segment", key: "C:evil", wantPath: "C:evil", wantErr: runtime.GOOS == "windows", wantErrMsg: "is not a local path"},
+		{name: "a colon segment", key: "a:b", wantPath: "a:b", wantErr: runtime.GOOS == "windows", wantErrMsg: "is not a local path"},
+		{
+			name:       "a traversing key is rejected",
+			key:        "uploads/user-a/../../protected/release.bin",
+			wantErr:    true,
+			wantErrMsg: `resolves to ".."`,
+		},
+		{
+			name:       "a backslash-separated traversal is rejected",
+			key:        `uploads/user-a/..\..\..\..\Users\Public\kosli-poc.txt`,
+			wantErr:    true,
+			wantErrMsg: `resolves to ".."`,
+		},
+		{
+			name:       "a short backslash-separated traversal is rejected",
+			key:        `uploads/user-a/..\x`,
+			wantErr:    true,
+			wantErrMsg: `resolves to ".."`,
+		},
+		{name: "a bare \"..\" is rejected", key: "..", wantErr: true, wantErrMsg: `resolves to ".."`},
+		// Windows drops trailing spaces and dots from a name, so these resolve as "..".
+		{name: "a \"..\" with a trailing space is rejected", key: "a/.. /x", wantErr: true, wantErrMsg: `resolves to ".."`},
+		{name: "three dots are rejected", key: "a/.../b", wantErr: true, wantErrMsg: `resolves to ".."`},
+		{name: "a bare \"./.\" is rejected", key: "./.", wantErr: true, wantErrMsg: "names no file"},
+		{name: "a trailing \"..\" segment is rejected", key: "a/..", wantErr: true, wantErrMsg: `resolves to ".."`},
+		{name: "an empty key is rejected", key: "", wantErr: true, wantErrMsg: "names no file"},
+		{name: "a bare slash is rejected", key: "/", wantErr: true, wantErrMsg: "names no file"},
+		{name: "doubled slashes with nothing else are rejected", key: "//", wantErr: true, wantErrMsg: "names no file"},
+		{name: "a bare dot is rejected", key: ".", wantErr: true, wantErrMsg: "names no file"},
+	} {
+		suite.Run(t.name, func() {
+			got, err := localPathForS3Key(t.key)
+			if t.wantErr {
+				require.Error(suite.T(), err)
+				require.Contains(suite.T(), err.Error(), t.key)
+				require.Contains(suite.T(), err.Error(), t.wantErrMsg)
+				return
+			}
+			require.NoError(suite.T(), err)
+			require.Equal(suite.T(), filepath.Join("base", t.wantPath), filepath.Join("base", got))
+		})
+	}
+}
+
+func (suite *AWSTestSuite) TestDownloadFileFromBucketRefusesToOverwrite() {
+	tempDir := suite.T().TempDir()
+	preexisting := filepath.Join(tempDir, "README.md")
+	require.NoError(suite.T(), os.WriteFile(preexisting, []byte("pre-existing content\n"), 0666))
+
+	client := &FakeS3Client{
+		Bucket: fakeS3TestBucketName,
+		Objects: map[string][]byte{
+			"README.md": []byte(fakeReadmeBody),
+		},
+	}
+
+	err := downloadFileFromBucket(client, tempDir, "README.md", fakeS3TestBucketName, logger.NewStandardLogger())
+	require.Error(suite.T(), err)
+	require.Contains(suite.T(), err.Error(), "object key [README.md]")
+	require.Contains(suite.T(), err.Error(), "--exclude-regex")
+
+	content, readErr := os.ReadFile(preexisting)
+	require.NoError(suite.T(), readErr)
+	require.Equal(suite.T(), "pre-existing content\n", string(content),
+		"the pre-existing file must be left untouched, not truncated")
+}
+
+// Neither key holds a ".." segment, so O_EXCL is what catches this pair.
+func (suite *AWSTestSuite) TestGetS3DataFromClientCollidingKeysAreAnError() {
+	client := &FakeS3Client{
+		Bucket: fakeS3TestBucketName,
+		Objects: map[string][]byte{
+			"a//b": []byte(fakeReadmeBody),
+			"a/b":  []byte(fakeTemplateBody),
+		},
+	}
+
+	_, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger())
+	require.Error(suite.T(), err)
+	// '/' sorts before 'b', so "a//b" downloads first and "a/b" collides.
+	require.Contains(suite.T(), err.Error(), "object key [a/b]", "the error must name the key that collided")
+	require.Contains(suite.T(), err.Error(), "--exclude-regex")
+}
+
+// An object "a" alongside the prefix "a/" is legal in S3 and impossible on a
+// filesystem, so MkdirAll fails with ENOTDIR once "a" lands first.
+func (suite *AWSTestSuite) TestGetS3DataFromClientObjectAndPrefixCollideAreAnError() {
+	client := &FakeS3Client{
+		Bucket: fakeS3TestBucketName,
+		Objects: map[string][]byte{
+			"a":   []byte(fakeReadmeBody),
+			"a/b": []byte(fakeTemplateBody),
+		},
+	}
+
+	_, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger())
+	require.Error(suite.T(), err)
+	require.Contains(suite.T(), err.Error(), "object key [a/b]")
+	require.Contains(suite.T(), err.Error(), "--exclude-regex")
+}
+
+func (suite *AWSTestSuite) TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnFilesystemErrors() {
+	if runtime.GOOS == "windows" {
+		suite.T().Skip("chmod on a directory does not block file creation on Windows")
+	}
+	if os.Getuid() == 0 {
+		suite.T().Skip("root ignores directory permissions")
+	}
+	tempDir := suite.T().TempDir()
+	require.NoError(suite.T(), os.Chmod(tempDir, 0500))
+	suite.T().Cleanup(func() { _ = os.Chmod(tempDir, 0700) })
+
+	client := &FakeS3Client{
+		Bucket: fakeS3TestBucketName,
+		Objects: map[string][]byte{
+			"README.md":     []byte(fakeReadmeBody),
+			"sub/README.md": []byte(fakeReadmeBody),
+		},
+	}
+
+	// A bare key fails in OpenFile; a nested one has a directory left to
+	// create, so it fails in MkdirAll.
+	for _, key := range []string{"README.md", "sub/README.md"} {
+		suite.Run(key, func() {
+			err := downloadFileFromBucket(client, tempDir, key, fakeS3TestBucketName, logger.NewStandardLogger())
+			require.Error(suite.T(), err)
+			require.ErrorIs(suite.T(), err, fs.ErrPermission)
+			require.Contains(suite.T(), err.Error(), fmt.Sprintf("object key [%s]", key))
+			require.NotContains(suite.T(), err.Error(), "--exclude-regex")
+		})
+	}
+}
+
+// Equal fingerprints mean the odd-shaped keys landed on the same paths as the
+// plain ones.
+func (suite *AWSTestSuite) TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys() {
+	unusualBody := []byte("unusual key content\n")
+	otherBody := []byte("other content\n")
+	thirdBody := []byte("third content\n")
+
+	unusual := &FakeS3Client{
+		Bucket: fakeS3TestBucketName,
+		Objects: map[string][]byte{
+			"/lead.txt": unusualBody,
+			"a//b":      otherBody,
+			"./c.txt":   thirdBody,
+			`d\e.txt`:   []byte(fakeNotesBody),
+		},
+	}
+	today := &FakeS3Client{
+		Bucket: fakeS3TestBucketName,
+		Objects: map[string][]byte{
+			"lead.txt": unusualBody,
+			"a/b":      otherBody,
+			"c.txt":    thirdBody,
+			`d\e.txt`:  []byte(fakeNotesBody),
+		},
+	}
+
+	unusualData, err := getS3DataFromClient(unusual, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger())
+	require.NoError(suite.T(), err)
+	todayData, err := getS3DataFromClient(today, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger())
+	require.NoError(suite.T(), err)
+	require.Len(suite.T(), unusualData, 1)
+	require.Len(suite.T(), todayData, 1)
+
+	require.Equal(suite.T(), todayData[0].Digests, unusualData[0].Digests,
+		"odd-shaped keys accepted by the containment rule must still fingerprint identically to the plain keys they land on")
+}
+
 func skipIfCredsUnset(T *testing.T, requireEnvVars bool, creds *AWSStaticCreds) {
 	if requireEnvVars {
 		// skips the test case if it requires env vars and they are not set

internal/aws/s3_contract_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"os"
 	"path/filepath"
+	"slices"
 	"testing"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -43,6 +44,24 @@ func runS3ContractTests(t *testing.T, client S3API, bucket, existingKey string)
 		}
 	})
 
+	// Which of two colliding keys fails a snapshot follows from this order.
+	// Paginated because a single page can hold one key, sorted either way.
+	t.Run("ListObjectsV2 returns keys in lexicographic order", func(t *testing.T) {
+		var keys []string
+		paginator := s3.NewListObjectsV2Paginator(client, &s3.ListObjectsV2Input{
+			Bucket: aws.String(bucket),
+		})
+		for paginator.HasMorePages() {
+			page, err := paginator.NextPage(context.TODO())
+			require.NoError(t, err)
+			for _, object := range page.Contents {
+				keys = append(keys, *object.Key)
+			}
+		}
+		require.GreaterOrEqual(t, len(keys), 2, "the bucket must hold at least two objects for order to be checked")
+		require.True(t, slices.IsSorted(keys), "S3 returns keys in UTF-8 binary order: %v", keys)
+	})
+
 	t.Run("ListObjectsV2 with MaxKeys paginates via ContinuationToken", func(t *testing.T) {
 		// Request one object per page to force pagination. The paginator only
 		// follows a page when IsTruncated is true AND NextContinuationToken is