From 4e8a6382c5ce74cb9285baebcacfbbc304f8c50b Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 13:57:55 +0100
Subject: [PATCH 01/13] fix(snapshot s3): reject object keys with ".." segments
 and never overwrite a downloaded object

---
 internal/aws/aws.go      | 50 ++++++++++++++++++++++++++++++++++++++--
 internal/aws/aws_test.go | 23 ++++++++++++++++++
 2 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/internal/aws/aws.go b/internal/aws/aws.go
index 7a410e8fe..44693bce1 100644
--- a/internal/aws/aws.go
+++ b/internal/aws/aws.go
@@ -25,7 +25,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,8 +552,55 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex
 	return s3Data, nil
 }
 
+// localPathForS3Key turns an S3 object key into a path relative to the
+// download directory, or rejects the key. S3 keys are not filesystem paths:
+// they can contain ".." segments and backslashes that a naive filepath.Join
+// would resolve differently than the key names, letting one key's download
+// overwrite another's or, on Windows, escape the download directory.
+//
+// Only what is load-bearing for security is rejected here; everything else
+// (doubled slashes, leading slashes, backslash-containing literal filenames)
+// is accepted and lands exactly where filepath.Join put it before this
+// change, so buckets that snapshot cleanly today keep the same fingerprint.
+func localPathForS3Key(key string) (string, error) {
+	const reason = "object key [%s] cannot be used as a local path: %s; exclude it with --exclude"
+
+	// Segments are split on both '/' and '\' so a key can't smuggle a ".."
+	// past the check using the separator this OS doesn't treat specially.
+	segments := strings.FieldsFunc(key, func(r rune) bool { return r == '/' || r == '\\' })
+	for _, segment := range segments {
+		if segment == ".." {
+			return "", fmt.Errorf(reason, key, `contains a ".." segment`)
+		}
+	}
+
+	rel := strings.TrimLeft(key, "/")
+	if rel == "" || rel == "." {
+		return "", fmt.Errorf(reason, key, "names no file")
+	}
+
+	rel = filepath.FromSlash(rel)
+	if !filepath.IsLocal(rel) {
+		return "", fmt.Errorf(reason, key, "is not a local path")
+	}
+
+	return rel, nil
+}
+
 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)
+	if err := os.MkdirAll(filepath.Dir(dest), 0770); err != nil {
+		return err
+	}
+	// O_EXCL is the second half of the containment fix: two keys that map to
+	// the same local file (a doubled slash vs a single one, a leading slash
+	// vs none, a case-only clash on a case-insensitive filesystem) now fail
+	// loudly instead of the later download silently overwriting the earlier.
+	file, err := os.OpenFile(dest, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
 	if err != nil {
 		return err
 	}
diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go
index 3b0716ea9..c904543ad 100644
--- a/internal/aws/aws_test.go
+++ b/internal/aws/aws_test.go
@@ -1227,6 +1227,29 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientFilterEquivalence() {
 	}
 }
 
+// TestGetS3DataFromClientRejectsKeysWithDotDotSegments reproduces the
+// unfixed bug: an S3 key containing a ".." segment normalises, via
+// filepath.Join, to the same local path as another key. FakeS3Client lists
+// keys in lexicographic order, so "protected/release.bin" downloads first
+// and the traversing key overwrites it with attacker-controlled content
+// before fingerprinting, with no error raised.
+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")
+}
+
 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

From 9bb9ea5c93a63c4b8ec8c21cc1ca8d3fc7f53f86 Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 14:06:18 +0100
Subject: [PATCH 02/13] test(snapshot s3): pin the object-key containment rule

---
 internal/aws/aws_test.go | 157 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 157 insertions(+)

diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go
index c904543ad..598c94227 100644
--- a/internal/aws/aws_test.go
+++ b/internal/aws/aws_test.go
@@ -3,6 +3,8 @@ package aws
 import (
 	"context"
 	"fmt"
+	"os"
+	"path/filepath"
 	"regexp"
 	"testing"
 	"time"
@@ -1250,6 +1252,161 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientRejectsKeysWithDotDotSegments(
 	require.Contains(suite.T(), err.Error(), "uploads/user-a/../../protected/release.bin")
 }
 
+// TestLocalPathForS3Key pins the containment rule from the plan's vector
+// table. Windows-only-reject keys (reserved names, drive-looking segments)
+// are asserted as accepted here because CI runs on Linux; filepath.IsLocal
+// only rejects them on Windows.
+//
+// Accept rows compare the joined path rather than the raw returned rel,
+// because localPathForS3Key deliberately returns the key uncleaned (per
+// slice A) - filepath.Join, not this helper, is what collapses "a//b" or
+// "./a.txt" to the path a bucket snapshot landed on before this change.
+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: "three dots is not a \"..\" segment", key: "...", wantPath: "..."},
+		{name: "three dots as a nested segment", key: "a/.../b", wantPath: "a/.../b"},
+		{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"},
+		{name: "a reserved Windows name is only rejected on Windows", key: "CON", wantPath: "CON"},
+		{name: "a drive-looking segment is only rejected on Windows", key: "C:evil", wantPath: "C:evil"},
+		{name: "a colon segment is only rejected on Windows", key: "a:b", wantPath: "a:b"},
+		{
+			name:       "a traversing key is rejected",
+			key:        "uploads/user-a/../../protected/release.bin",
+			wantErr:    true,
+			wantErrMsg: `contains a ".." segment`,
+		},
+		{
+			name:       "a backslash-separated traversal is rejected",
+			key:        `uploads/user-a/..\..\..\..\Users\Public\kosli-poc.txt`,
+			wantErr:    true,
+			wantErrMsg: `contains a ".." segment`,
+		},
+		{
+			name:       "a short backslash-separated traversal is rejected",
+			key:        `uploads/user-a/..\x`,
+			wantErr:    true,
+			wantErrMsg: `contains a ".." segment`,
+		},
+		{name: "a bare \"..\" is rejected", key: "..", wantErr: true, wantErrMsg: `contains a ".." segment`},
+		{name: "a trailing \"..\" segment is rejected", key: "a/..", wantErr: true, wantErrMsg: `contains a ".." segment`},
+		{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))
+		})
+	}
+}
+
+// TestDownloadFileFromBucketRefusesToOverwrite asserts the O_EXCL half of the
+// containment fix: a destination file that already exists (however it got
+// there) is never silently truncated and replaced.
+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)
+
+	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")
+}
+
+// TestGetS3DataFromClientCollidingKeysAreAnError covers the case rule 1 does
+// not catch: two distinct S3 keys ("a//b" and "a/b") that both land on the
+// same local file. localPathForS3Key accepts both (neither contains a ".."
+// segment), so O_EXCL is what turns the second download into an error
+// instead of a silent overwrite.
+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)
+}
+
+// TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys pins that accepted
+// odd-shaped keys still land exactly where filepath.Join put them before
+// this change, so buckets that snapshot cleanly today keep the same
+// fingerprint. Comparing fingerprints (rather than the temp dir layout
+// directly) is the same technique TestGetS3DataFromClientFilterEquivalence
+// uses above.
+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.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

From 456f07504b331bd16627d443b8fea71611952589 Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 14:07:03 +0100
Subject: [PATCH 03/13] docs(snapshot s3): state the object-key rule

---
 cmd/kosli/snapshotS3.go | 1 +
 1 file changed, 1 insertion(+)

diff --git a/cmd/kosli/snapshotS3.go b/cmd/kosli/snapshotS3.go
index 7ed2670c6..20526648a 100644
--- a/cmd/kosli/snapshotS3.go
+++ b/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 containing a ^..^ path segment are rejected and fail the snapshot, naming the key; exclude such a key with ^--exclude^ if it is legitimate. Two keys that resolve to the same local path are also an error.
 
 ` + kosliIgnoreDesc
 

From 9b0e634c7e37529970548b98e1440dae9864b150 Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 14:30:22 +0100
Subject: [PATCH 04/13] fix(snapshot s3): count ".. " and "..." as ".."
 segments and point at --exclude-regex

Windows drops trailing spaces and dots from a name, so a segment such as
".. " can resolve as "..". Trim before comparing so the check does not
depend on the platform running the snapshot.

--exclude matches the raw key with a slash-trimmed pattern, so it cannot
exclude a key that starts with "/"; --exclude-regex always can.

A single filepath.Clean check replaces the two-literal guard for keys that
name no file, and the redundant FromSlash is dropped: IsLocal and Join
accept both separators on every platform.
---
 cmd/kosli/snapshotS3.go  |  2 +-
 internal/aws/aws.go      | 17 ++++++++++-------
 internal/aws/aws_test.go | 21 ++++++++++++---------
 3 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/cmd/kosli/snapshotS3.go b/cmd/kosli/snapshotS3.go
index 20526648a..4b32e4107 100644
--- a/cmd/kosli/snapshotS3.go
+++ b/cmd/kosli/snapshotS3.go
@@ -15,7 +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 containing a ^..^ path segment are rejected and fail the snapshot, naming the key; exclude such a key with ^--exclude^ if it is legitimate. Two keys that resolve to the same local path are also an error.
+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. Exclude such keys with ^--exclude-regex^ if they are legitimate.
 
 ` + kosliIgnoreDesc
 
diff --git a/internal/aws/aws.go b/internal/aws/aws.go
index 44693bce1..18e69b59e 100644
--- a/internal/aws/aws.go
+++ b/internal/aws/aws.go
@@ -563,23 +563,23 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex
 // is accepted and lands exactly where filepath.Join put it before this
 // change, so buckets that snapshot cleanly today keep the same fingerprint.
 func localPathForS3Key(key string) (string, error) {
-	const reason = "object key [%s] cannot be used as a local path: %s; exclude it with --exclude"
+	const reason = "object key [%s] cannot be used as a local path: %s; exclude it with --exclude-regex"
 
 	// Segments are split on both '/' and '\' so a key can't smuggle a ".."
 	// past the check using the separator this OS doesn't treat specially.
+	// Windows drops trailing spaces and dots from a name, so ".. " and "..."
+	// can resolve as "..", which is why the comparison trims them first.
 	segments := strings.FieldsFunc(key, func(r rune) bool { return r == '/' || r == '\\' })
 	for _, segment := range segments {
-		if segment == ".." {
+		if strings.HasPrefix(segment, "..") && strings.TrimRight(segment, ". ") == "" {
 			return "", fmt.Errorf(reason, key, `contains a ".." segment`)
 		}
 	}
 
 	rel := strings.TrimLeft(key, "/")
-	if rel == "" || rel == "." {
+	if filepath.Clean(rel) == "." {
 		return "", fmt.Errorf(reason, key, "names no file")
 	}
-
-	rel = filepath.FromSlash(rel)
 	if !filepath.IsLocal(rel) {
 		return "", fmt.Errorf(reason, key, "is not a local path")
 	}
@@ -598,8 +598,11 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin
 	}
 	// O_EXCL is the second half of the containment fix: two keys that map to
 	// the same local file (a doubled slash vs a single one, a leading slash
-	// vs none, a case-only clash on a case-insensitive filesystem) now fail
-	// loudly instead of the later download silently overwriting the earlier.
+	// vs none, two filenames differing only in case on a case-insensitive
+	// filesystem) now fail loudly instead of the later download silently
+	// overwriting the earlier. Directories are not covered: on a
+	// case-insensitive filesystem "A/x" and "a/y" still share one directory,
+	// as they did before this change.
 	file, err := os.OpenFile(dest, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
 	if err != nil {
 		return err
diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go
index 598c94227..56e0daae4 100644
--- a/internal/aws/aws_test.go
+++ b/internal/aws/aws_test.go
@@ -1252,15 +1252,14 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientRejectsKeysWithDotDotSegments(
 	require.Contains(suite.T(), err.Error(), "uploads/user-a/../../protected/release.bin")
 }
 
-// TestLocalPathForS3Key pins the containment rule from the plan's vector
-// table. Windows-only-reject keys (reserved names, drive-looking segments)
-// are asserted as accepted here because CI runs on Linux; filepath.IsLocal
-// only rejects them on Windows.
+// TestLocalPathForS3Key pins the containment rule. Windows-only-reject keys
+// (reserved names, drive-looking segments) are asserted as accepted here
+// because CI runs on Linux; filepath.IsLocal only rejects them on Windows.
 //
 // Accept rows compare the joined path rather than the raw returned rel,
-// because localPathForS3Key deliberately returns the key uncleaned (per
-// slice A) - filepath.Join, not this helper, is what collapses "a//b" or
-// "./a.txt" to the path a bucket snapshot landed on before this change.
+// because the helper returns the key uncleaned: filepath.Join, not the
+// helper, is what collapses "a//b" or "./a.txt" to the path a bucket
+// snapshot landed on before this change.
 func (suite *AWSTestSuite) TestLocalPathForS3Key() {
 	for _, t := range []struct {
 		name       string
@@ -1276,8 +1275,8 @@ func (suite *AWSTestSuite) TestLocalPathForS3Key() {
 		{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: "three dots is not a \"..\" segment", key: "...", wantPath: "..."},
-		{name: "three dots as a nested segment", key: "a/.../b", wantPath: "a/.../b"},
+		{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"},
@@ -1306,6 +1305,10 @@ func (suite *AWSTestSuite) TestLocalPathForS3Key() {
 			wantErrMsg: `contains a ".." segment`,
 		},
 		{name: "a bare \"..\" is rejected", key: "..", wantErr: true, wantErrMsg: `contains a ".." segment`},
+		// 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: `contains a ".." segment`},
+		{name: "three dots are rejected", key: "a/.../b", wantErr: true, wantErrMsg: `contains a ".." segment`},
+		{name: "a bare \"./.\" is rejected", key: "./.", wantErr: true, wantErrMsg: "names no file"},
 		{name: "a trailing \"..\" segment is rejected", key: "a/..", wantErr: true, wantErrMsg: `contains a ".." segment`},
 		{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"},

From d77b4ebcf846d648b18a99a719bda4b043c123d8 Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 14:56:35 +0100
Subject: [PATCH 05/13] fix(snapshot s3): name the object key when a download
 collides or its directory cannot be made

The rule error already named the key and pointed at --exclude-regex; the
O_EXCL and MkdirAll errors surfaced a bare temp-dir path instead. One
helper now builds every unusable-key error, and the collision test pins
the key and the advice.
---
 internal/aws/aws.go      | 19 ++++++++++++-------
 internal/aws/aws_test.go |  6 ++++++
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/internal/aws/aws.go b/internal/aws/aws.go
index 18e69b59e..a1cd7da34 100644
--- a/internal/aws/aws.go
+++ b/internal/aws/aws.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -563,8 +564,6 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex
 // is accepted and lands exactly where filepath.Join put it before this
 // change, so buckets that snapshot cleanly today keep the same fingerprint.
 func localPathForS3Key(key string) (string, error) {
-	const reason = "object key [%s] cannot be used as a local path: %s; exclude it with --exclude-regex"
-
 	// Segments are split on both '/' and '\' so a key can't smuggle a ".."
 	// past the check using the separator this OS doesn't treat specially.
 	// Windows drops trailing spaces and dots from a name, so ".. " and "..."
@@ -572,21 +571,27 @@ func localPathForS3Key(key string) (string, error) {
 	segments := strings.FieldsFunc(key, func(r rune) bool { return r == '/' || r == '\\' })
 	for _, segment := range segments {
 		if strings.HasPrefix(segment, "..") && strings.TrimRight(segment, ". ") == "" {
-			return "", fmt.Errorf(reason, key, `contains a ".." segment`)
+			return "", unusableS3KeyError(key, errors.New(`contains a ".." segment`))
 		}
 	}
 
 	rel := strings.TrimLeft(key, "/")
 	if filepath.Clean(rel) == "." {
-		return "", fmt.Errorf(reason, key, "names no file")
+		return "", unusableS3KeyError(key, errors.New("names no file"))
 	}
 	if !filepath.IsLocal(rel) {
-		return "", fmt.Errorf(reason, key, "is not a local path")
+		return "", unusableS3KeyError(key, errors.New("is not a local path"))
 	}
 
 	return rel, nil
 }
 
+// unusableS3KeyError names the object key so the operator can act on it; the
+// temp-dir path inside a filesystem error means nothing to them.
+func unusableS3KeyError(key string, cause error) error {
+	return fmt.Errorf("object key [%s] cannot be stored as a local file: %w; exclude it with --exclude-regex", key, cause)
+}
+
 func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket string, logger *logger.Logger) error {
 	rel, err := localPathForS3Key(key)
 	if err != nil {
@@ -594,7 +599,7 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin
 	}
 	dest := filepath.Join(dirName, rel)
 	if err := os.MkdirAll(filepath.Dir(dest), 0770); err != nil {
-		return err
+		return unusableS3KeyError(key, err)
 	}
 	// O_EXCL is the second half of the containment fix: two keys that map to
 	// the same local file (a doubled slash vs a single one, a leading slash
@@ -605,7 +610,7 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin
 	// as they did before this change.
 	file, err := os.OpenFile(dest, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
 	if err != nil {
-		return err
+		return unusableS3KeyError(key, err)
 	}
 	defer func() {
 		if err := file.Close(); err != nil {
diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go
index 56e0daae4..a685e7b17 100644
--- a/internal/aws/aws_test.go
+++ b/internal/aws/aws_test.go
@@ -1369,6 +1369,10 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientCollidingKeysAreAnError() {
 
 	_, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger())
 	require.Error(suite.T(), err)
+	// FakeS3Client lists lexicographically and '/' sorts before 'b', so "a//b"
+	// downloads first and "a/b" is the key that 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")
 }
 
 // TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys pins that accepted
@@ -1405,6 +1409,8 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKey
 	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")

From 11e4a0fa011317ea81e334642f7a074b0411b300 Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 15:07:16 +0100
Subject: [PATCH 06/13] fix(snapshot s3): only advise excluding a key when the
 key is the cause

A disk-full or permission error was carrying "exclude it with
--exclude-regex"; following that on a legitimate object would record a
snapshot with the object silently missing. Filesystem errors now name the
key and wrap the cause with no advice. The rule rejections and an O_EXCL
collision, which the key does cause, keep the advice.

Pins the object-and-prefix case ("a" plus "a/b") and an unwritable
download directory, and documents that only leading "/" is trimmed.
---
 internal/aws/aws.go      | 27 ++++++++++++++--------
 internal/aws/aws_test.go | 48 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/internal/aws/aws.go b/internal/aws/aws.go
index a1cd7da34..f5c4383f5 100644
--- a/internal/aws/aws.go
+++ b/internal/aws/aws.go
@@ -6,6 +6,7 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -571,25 +572,30 @@ func localPathForS3Key(key string) (string, error) {
 	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, errors.New(`contains a ".." segment`))
+			return "", unusableS3KeyError(key, `contains a ".." segment`)
 		}
 	}
 
+	// Only leading '/' is trimmed, mirroring what filepath.Join did before.
+	// A leading '\' is left for filepath.IsLocal, which rejects it as rooted
+	// on Windows and accepts it as a literal filename elsewhere.
 	rel := strings.TrimLeft(key, "/")
 	if filepath.Clean(rel) == "." {
-		return "", unusableS3KeyError(key, errors.New("names no file"))
+		return "", unusableS3KeyError(key, "names no file")
 	}
 	if !filepath.IsLocal(rel) {
-		return "", unusableS3KeyError(key, errors.New("is not a local path"))
+		return "", unusableS3KeyError(key, "is not a local path")
 	}
 
 	return rel, nil
 }
 
-// unusableS3KeyError names the object key so the operator can act on it; the
-// temp-dir path inside a filesystem error means nothing to them.
-func unusableS3KeyError(key string, cause error) error {
-	return fmt.Errorf("object key [%s] cannot be stored as a local file: %w; exclude it with --exclude-regex", key, cause)
+// unusableS3KeyError is for failures the key itself causes, so the advice to
+// exclude it is sound. A filesystem error about the machine (disk full, a
+// read-only temp dir) must not carry that advice: excluding a legitimate
+// object on it would record a snapshot with the object silently missing.
+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", key, reason)
 }
 
 func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket string, logger *logger.Logger) error {
@@ -599,7 +605,7 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin
 	}
 	dest := filepath.Join(dirName, rel)
 	if err := os.MkdirAll(filepath.Dir(dest), 0770); err != nil {
-		return unusableS3KeyError(key, err)
+		return fmt.Errorf("object key [%s]: %w", key, err)
 	}
 	// O_EXCL is the second half of the containment fix: two keys that map to
 	// the same local file (a doubled slash vs a single one, a leading slash
@@ -609,8 +615,11 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin
 	// case-insensitive filesystem "A/x" and "a/y" still share one directory,
 	// as they did before this change.
 	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 unusableS3KeyError(key, err)
+		return fmt.Errorf("object key [%s]: %w", key, err)
 	}
 	defer func() {
 		if err := file.Close(); err != nil {
diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go
index a685e7b17..9c8d3f0c9 100644
--- a/internal/aws/aws_test.go
+++ b/internal/aws/aws_test.go
@@ -3,6 +3,7 @@ package aws
 import (
 	"context"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -1375,6 +1376,53 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientCollidingKeysAreAnError() {
 	require.Contains(suite.T(), err.Error(), "--exclude-regex")
 }
 
+// TestGetS3DataFromClientObjectAndPrefixCollideAreAnError covers a bucket
+// holding both an object "a" and objects under the prefix "a/": legal in S3,
+// impossible on a filesystem. "a" downloads first and becomes a file, so
+// MkdirAll for "a/b" fails. The error must name "a/b" but must not tell the
+// operator to exclude it, because the failure is not the key's fault alone.
+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.NotContains(suite.T(), err.Error(), "--exclude-regex")
+}
+
+// TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnFilesystemErrors pins
+// that an error about the machine rather than the key (here an unwritable
+// download directory) names the key for context but does not suggest
+// excluding it: following that advice would record a snapshot with a
+// legitimate object silently missing.
+func (suite *AWSTestSuite) TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnFilesystemErrors() {
+	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),
+		},
+	}
+
+	err := downloadFileFromBucket(client, tempDir, "README.md", fakeS3TestBucketName, logger.NewStandardLogger())
+	require.Error(suite.T(), err)
+	require.ErrorIs(suite.T(), err, fs.ErrPermission)
+	require.Contains(suite.T(), err.Error(), "object key [README.md]")
+	require.NotContains(suite.T(), err.Error(), "--exclude-regex")
+}
+
 // TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys pins that accepted
 // odd-shaped keys still land exactly where filepath.Join put them before
 // this change, so buckets that snapshot cleanly today keep the same

From 5d37f006fa6afaec7d86975807364412cce7b154 Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 15:15:24 +0100
Subject: [PATCH 07/13] docs(snapshot s3): say that include filters override
 exclude when advising on a rejected key

---
 cmd/kosli/snapshotS3.go | 2 +-
 internal/aws/aws.go     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmd/kosli/snapshotS3.go b/cmd/kosli/snapshotS3.go
index 4b32e4107..7e5ee9e07 100644
--- a/cmd/kosli/snapshotS3.go
+++ b/cmd/kosli/snapshotS3.go
@@ -15,7 +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. Exclude such keys with ^--exclude-regex^ if they are legitimate.
+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^; when ^--include^ or ^--include-regex^ is set, exclude filters are ignored, so narrow the include filter instead.
 
 ` + kosliIgnoreDesc
 
diff --git a/internal/aws/aws.go b/internal/aws/aws.go
index f5c4383f5..fb2358765 100644
--- a/internal/aws/aws.go
+++ b/internal/aws/aws.go
@@ -595,7 +595,7 @@ func localPathForS3Key(key string) (string, error) {
 // read-only temp dir) must not carry that advice: excluding a legitimate
 // object on it would record a snapshot with the object silently missing.
 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", key, reason)
+	return fmt.Errorf("object key [%s] cannot be stored as a local file: %s; exclude it with --exclude-regex, or narrow --include", key, reason)
 }
 
 func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket string, logger *logger.Logger) error {

From a270b52e444492ada2e1550de1b55375618dacad Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 15:24:27 +0100
Subject: [PATCH 08/13] fix(snapshot s3): advise on an object-and-prefix clash
 and name the key on download failure

os.MkdirAll returns ENOTDIR portably when a parent of the destination is
already a file, which only a bucket holding both "a" and "a/b" can cause,
so that error now carries the exclusion advice like an O_EXCL collision.
The advice no longer presumes an include filter is in use.
---
 internal/aws/aws.go      | 12 +++++++++---
 internal/aws/aws_test.go |  6 +++---
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/internal/aws/aws.go b/internal/aws/aws.go
index fb2358765..be6d893b5 100644
--- a/internal/aws/aws.go
+++ b/internal/aws/aws.go
@@ -12,6 +12,7 @@ import (
 	"regexp"
 	"strings"
 	"sync"
+	"syscall"
 	"time"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -595,7 +596,7 @@ func localPathForS3Key(key string) (string, error) {
 // read-only temp dir) must not carry that advice: excluding a legitimate
 // object on it would record a snapshot with the object silently missing.
 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 --include", key, reason)
+	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 {
@@ -604,7 +605,12 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin
 		return err
 	}
 	dest := filepath.Join(dirName, rel)
-	if err := os.MkdirAll(filepath.Dir(dest), 0770); err != nil {
+	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 is the second half of the containment fix: two keys that map to
@@ -633,7 +639,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")
diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go
index 9c8d3f0c9..fe3a3b69d 100644
--- a/internal/aws/aws_test.go
+++ b/internal/aws/aws_test.go
@@ -1379,8 +1379,8 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientCollidingKeysAreAnError() {
 // TestGetS3DataFromClientObjectAndPrefixCollideAreAnError covers a bucket
 // holding both an object "a" and objects under the prefix "a/": legal in S3,
 // impossible on a filesystem. "a" downloads first and becomes a file, so
-// MkdirAll for "a/b" fails. The error must name "a/b" but must not tell the
-// operator to exclude it, because the failure is not the key's fault alone.
+// MkdirAll for "a/b" fails with ENOTDIR. That is a property of the bucket, not
+// the machine, so the error names "a/b" and advises excluding one of the two.
 func (suite *AWSTestSuite) TestGetS3DataFromClientObjectAndPrefixCollideAreAnError() {
 	client := &FakeS3Client{
 		Bucket: fakeS3TestBucketName,
@@ -1393,7 +1393,7 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientObjectAndPrefixCollideAreAnErr
 	_, 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.NotContains(suite.T(), err.Error(), "--exclude-regex")
+	require.Contains(suite.T(), err.Error(), "--exclude-regex")
 }
 
 // TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnFilesystemErrors pins

From e81d898be109c55d54ec178f454d3f5911f169c6 Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 15:32:57 +0100
Subject: [PATCH 09/13] test(snapshot s3): say a segment resolves to "..", skip
 the chmod test on Windows, pin the overwrite message

---
 internal/aws/aws.go      |  2 +-
 internal/aws/aws_test.go | 20 +++++++++++++-------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/internal/aws/aws.go b/internal/aws/aws.go
index be6d893b5..1b267477c 100644
--- a/internal/aws/aws.go
+++ b/internal/aws/aws.go
@@ -573,7 +573,7 @@ func localPathForS3Key(key string) (string, error) {
 	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`)
+			return "", unusableS3KeyError(key, `contains a segment that resolves to ".."`)
 		}
 	}
 
diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go
index fe3a3b69d..0ff65d167 100644
--- a/internal/aws/aws_test.go
+++ b/internal/aws/aws_test.go
@@ -7,6 +7,7 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"runtime"
 	"testing"
 	"time"
 
@@ -1291,26 +1292,26 @@ func (suite *AWSTestSuite) TestLocalPathForS3Key() {
 			name:       "a traversing key is rejected",
 			key:        "uploads/user-a/../../protected/release.bin",
 			wantErr:    true,
-			wantErrMsg: `contains a ".." segment`,
+			wantErrMsg: `resolves to ".."`,
 		},
 		{
 			name:       "a backslash-separated traversal is rejected",
 			key:        `uploads/user-a/..\..\..\..\Users\Public\kosli-poc.txt`,
 			wantErr:    true,
-			wantErrMsg: `contains a ".." segment`,
+			wantErrMsg: `resolves to ".."`,
 		},
 		{
 			name:       "a short backslash-separated traversal is rejected",
 			key:        `uploads/user-a/..\x`,
 			wantErr:    true,
-			wantErrMsg: `contains a ".." segment`,
+			wantErrMsg: `resolves to ".."`,
 		},
-		{name: "a bare \"..\" is rejected", key: "..", wantErr: true, wantErrMsg: `contains a ".." segment`},
+		{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: `contains a ".." segment`},
-		{name: "three dots are rejected", key: "a/.../b", wantErr: true, wantErrMsg: `contains a ".." segment`},
+		{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: `contains a ".." segment`},
+		{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"},
@@ -1347,6 +1348,8 @@ func (suite *AWSTestSuite) TestDownloadFileFromBucketRefusesToOverwrite() {
 
 	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)
@@ -1402,6 +1405,9 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientObjectAndPrefixCollideAreAnErr
 // excluding it: following that advice would record a snapshot with a
 // legitimate object silently missing.
 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")
 	}

From bed75675ec5fd5846ff41293a83c7b5047a3bb75 Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 15:40:54 +0100
Subject: [PATCH 10/13] test(snapshot s3): assert the Windows-only rejections
 per OS; note that --exclude-regex needs escaping

---
 cmd/kosli/snapshotS3.go  |  2 +-
 internal/aws/aws_test.go | 14 ++++++++------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/cmd/kosli/snapshotS3.go b/cmd/kosli/snapshotS3.go
index 7e5ee9e07..eea892832 100644
--- a/cmd/kosli/snapshotS3.go
+++ b/cmd/kosli/snapshotS3.go
@@ -15,7 +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^; when ^--include^ or ^--include-regex^ is set, exclude filters are ignored, so narrow the include filter instead.
+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
 
diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go
index 0ff65d167..1aa4032b4 100644
--- a/internal/aws/aws_test.go
+++ b/internal/aws/aws_test.go
@@ -1254,9 +1254,9 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientRejectsKeysWithDotDotSegments(
 	require.Contains(suite.T(), err.Error(), "uploads/user-a/../../protected/release.bin")
 }
 
-// TestLocalPathForS3Key pins the containment rule. Windows-only-reject keys
-// (reserved names, drive-looking segments) are asserted as accepted here
-// because CI runs on Linux; filepath.IsLocal only rejects them on Windows.
+// TestLocalPathForS3Key pins the containment rule. The Windows-only rejections
+// (reserved names, colons) are asserted per OS, so the table is the only
+// Windows-side coverage filepath.IsLocal has; CI itself runs on Linux.
 //
 // Accept rows compare the joined path rather than the raw returned rel,
 // because the helper returns the key uncleaned: filepath.Join, not the
@@ -1285,9 +1285,11 @@ func (suite *AWSTestSuite) TestLocalPathForS3Key() {
 		{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"},
-		{name: "a reserved Windows name is only rejected on Windows", key: "CON", wantPath: "CON"},
-		{name: "a drive-looking segment is only rejected on Windows", key: "C:evil", wantPath: "C:evil"},
-		{name: "a colon segment is only rejected on Windows", key: "a:b", wantPath: "a:b"},
+		// filepath.IsLocal rejects reserved device names and colons on Windows
+		// only; elsewhere these are ordinary filenames.
+		{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",

From 4a2370b935e387af125a152afa9c584040b5b49a Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 15:50:25 +0100
Subject: [PATCH 11/13] test(aws): pin lexicographic ListObjectsV2 order in the
 S3 contract suite

---
 internal/aws/s3_contract_test.go | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/internal/aws/s3_contract_test.go b/internal/aws/s3_contract_test.go
index ec9ea96d3..d3156d513 100644
--- a/internal/aws/s3_contract_test.go
+++ b/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,21 @@ func runS3ContractTests(t *testing.T, client S3API, bucket, existingKey string)
 		}
 	})
 
+	// Two tests in aws_test.go assert which of two colliding keys an error
+	// names, which follows from the listing order, so the order is a checked
+	// contract rather than an implementation detail of the fake.
+	t.Run("ListObjectsV2 returns keys in lexicographic order", func(t *testing.T) {
+		out, err := client.ListObjectsV2(context.TODO(), &s3.ListObjectsV2Input{
+			Bucket: aws.String(bucket),
+		})
+		require.NoError(t, err)
+		keys := make([]string, 0, len(out.Contents))
+		for _, object := range out.Contents {
+			keys = append(keys, *object.Key)
+		}
+		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

From b9cc318d56d880d22be563f7038f4f5d15129ce8 Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 16:00:41 +0100
Subject: [PATCH 12/13] test(aws): check listing order across pages and cover
 the MkdirAll error wrap

The contract row read one page, and the fake serves one key per page in
the contract suite, so a one-element slice was always sorted. Walking the
paginator checks the order the snapshot actually consumes; reversing the
fake's sort now fails it. A nested key in the permissions test reaches
the MkdirAll wrap, which a bare key never did.
---
 internal/aws/aws_test.go         | 19 +++++++++++++------
 internal/aws/s3_contract_test.go | 17 ++++++++++++-----
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go
index 1aa4032b4..4fd79c4e1 100644
--- a/internal/aws/aws_test.go
+++ b/internal/aws/aws_test.go
@@ -1420,15 +1420,22 @@ func (suite *AWSTestSuite) TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnF
 	client := &FakeS3Client{
 		Bucket: fakeS3TestBucketName,
 		Objects: map[string][]byte{
-			"README.md": []byte(fakeReadmeBody),
+			"README.md":     []byte(fakeReadmeBody),
+			"sub/README.md": []byte(fakeReadmeBody),
 		},
 	}
 
-	err := downloadFileFromBucket(client, tempDir, "README.md", fakeS3TestBucketName, logger.NewStandardLogger())
-	require.Error(suite.T(), err)
-	require.ErrorIs(suite.T(), err, fs.ErrPermission)
-	require.Contains(suite.T(), err.Error(), "object key [README.md]")
-	require.NotContains(suite.T(), err.Error(), "--exclude-regex")
+	// A bare key fails in OpenFile; a nested one fails in MkdirAll, since only
+	// then is there a directory left to create. Both wraps must behave alike.
+	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")
+		})
+	}
 }
 
 // TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys pins that accepted
diff --git a/internal/aws/s3_contract_test.go b/internal/aws/s3_contract_test.go
index d3156d513..aaa689e00 100644
--- a/internal/aws/s3_contract_test.go
+++ b/internal/aws/s3_contract_test.go
@@ -47,15 +47,22 @@ func runS3ContractTests(t *testing.T, client S3API, bucket, existingKey string)
 	// Two tests in aws_test.go assert which of two colliding keys an error
 	// names, which follows from the listing order, so the order is a checked
 	// contract rather than an implementation detail of the fake.
+	// Walked through the paginator so the order is checked across pages, which
+	// is how getS3DataFromClient consumes the listing; a single page from the
+	// fake holds one key and would pass vacuously.
 	t.Run("ListObjectsV2 returns keys in lexicographic order", func(t *testing.T) {
-		out, err := client.ListObjectsV2(context.TODO(), &s3.ListObjectsV2Input{
+		var keys []string
+		paginator := s3.NewListObjectsV2Paginator(client, &s3.ListObjectsV2Input{
 			Bucket: aws.String(bucket),
 		})
-		require.NoError(t, err)
-		keys := make([]string, 0, len(out.Contents))
-		for _, object := range out.Contents {
-			keys = append(keys, *object.Key)
+		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)
 	})
 

From 59799e17c980e8f21268d391c21979578eec3858 Mon Sep 17 00:00:00 2001
From: Alex Kantor <alex@kosli.com>
Date: Wed, 9 Sep 2026 20:04:14 +0100
Subject: [PATCH 13/13] refactor(snapshot s3): hold the new comments to the
 comment standard

Cut the test doc comments that restated their test names, the history and
the reference to a sibling test, and reduced each remaining comment to the
fact a reader cannot get from the code.
---
 internal/aws/aws.go              | 41 ++++++++----------------
 internal/aws/aws_test.go         | 55 +++++++-------------------------
 internal/aws/s3_contract_test.go |  8 ++---
 3 files changed, 26 insertions(+), 78 deletions(-)

diff --git a/internal/aws/aws.go b/internal/aws/aws.go
index 1b267477c..9b750e4f6 100644
--- a/internal/aws/aws.go
+++ b/internal/aws/aws.go
@@ -555,21 +555,12 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex
 	return s3Data, nil
 }
 
-// localPathForS3Key turns an S3 object key into a path relative to the
-// download directory, or rejects the key. S3 keys are not filesystem paths:
-// they can contain ".." segments and backslashes that a naive filepath.Join
-// would resolve differently than the key names, letting one key's download
-// overwrite another's or, on Windows, escape the download directory.
-//
-// Only what is load-bearing for security is rejected here; everything else
-// (doubled slashes, leading slashes, backslash-containing literal filenames)
-// is accepted and lands exactly where filepath.Join put it before this
-// change, so buckets that snapshot cleanly today keep the same fingerprint.
+// 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) {
-	// Segments are split on both '/' and '\' so a key can't smuggle a ".."
-	// past the check using the separator this OS doesn't treat specially.
-	// Windows drops trailing spaces and dots from a name, so ".. " and "..."
-	// can resolve as "..", which is why the comparison trims them first.
+	// 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, ". ") == "" {
@@ -577,9 +568,8 @@ func localPathForS3Key(key string) (string, error) {
 		}
 	}
 
-	// Only leading '/' is trimmed, mirroring what filepath.Join did before.
-	// A leading '\' is left for filepath.IsLocal, which rejects it as rooted
-	// on Windows and accepts it as a literal filename elsewhere.
+	// 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")
@@ -591,10 +581,9 @@ func localPathForS3Key(key string) (string, error) {
 	return rel, nil
 }
 
-// unusableS3KeyError is for failures the key itself causes, so the advice to
-// exclude it is sound. A filesystem error about the machine (disk full, a
-// read-only temp dir) must not carry that advice: excluding a legitimate
-// object on it would record a snapshot with the object silently missing.
+// 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)
 }
@@ -613,13 +602,9 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin
 	if err != nil {
 		return fmt.Errorf("object key [%s]: %w", key, err)
 	}
-	// O_EXCL is the second half of the containment fix: two keys that map to
-	// the same local file (a doubled slash vs a single one, a leading slash
-	// vs none, two filenames differing only in case on a case-insensitive
-	// filesystem) now fail loudly instead of the later download silently
-	// overwriting the earlier. Directories are not covered: on a
-	// case-insensitive filesystem "A/x" and "a/y" still share one directory,
-	// as they did before this change.
+	// 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")
diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go
index 4fd79c4e1..1b15e5932 100644
--- a/internal/aws/aws_test.go
+++ b/internal/aws/aws_test.go
@@ -1231,12 +1231,6 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientFilterEquivalence() {
 	}
 }
 
-// TestGetS3DataFromClientRejectsKeysWithDotDotSegments reproduces the
-// unfixed bug: an S3 key containing a ".." segment normalises, via
-// filepath.Join, to the same local path as another key. FakeS3Client lists
-// keys in lexicographic order, so "protected/release.bin" downloads first
-// and the traversing key overwrites it with attacker-controlled content
-// before fingerprinting, with no error raised.
 func (suite *AWSTestSuite) TestGetS3DataFromClientRejectsKeysWithDotDotSegments() {
 	trustedBody := []byte("trusted release content\n")
 	attackerBody := []byte("attacker controlled content\n")
@@ -1254,14 +1248,8 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientRejectsKeysWithDotDotSegments(
 	require.Contains(suite.T(), err.Error(), "uploads/user-a/../../protected/release.bin")
 }
 
-// TestLocalPathForS3Key pins the containment rule. The Windows-only rejections
-// (reserved names, colons) are asserted per OS, so the table is the only
-// Windows-side coverage filepath.IsLocal has; CI itself runs on Linux.
-//
-// Accept rows compare the joined path rather than the raw returned rel,
-// because the helper returns the key uncleaned: filepath.Join, not the
-// helper, is what collapses "a//b" or "./a.txt" to the path a bucket
-// snapshot landed on before this change.
+// 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
@@ -1285,8 +1273,7 @@ func (suite *AWSTestSuite) TestLocalPathForS3Key() {
 		{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; elsewhere these are ordinary filenames.
+		// 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"},
@@ -1333,9 +1320,6 @@ func (suite *AWSTestSuite) TestLocalPathForS3Key() {
 	}
 }
 
-// TestDownloadFileFromBucketRefusesToOverwrite asserts the O_EXCL half of the
-// containment fix: a destination file that already exists (however it got
-// there) is never silently truncated and replaced.
 func (suite *AWSTestSuite) TestDownloadFileFromBucketRefusesToOverwrite() {
 	tempDir := suite.T().TempDir()
 	preexisting := filepath.Join(tempDir, "README.md")
@@ -1359,11 +1343,7 @@ func (suite *AWSTestSuite) TestDownloadFileFromBucketRefusesToOverwrite() {
 		"the pre-existing file must be left untouched, not truncated")
 }
 
-// TestGetS3DataFromClientCollidingKeysAreAnError covers the case rule 1 does
-// not catch: two distinct S3 keys ("a//b" and "a/b") that both land on the
-// same local file. localPathForS3Key accepts both (neither contains a ".."
-// segment), so O_EXCL is what turns the second download into an error
-// instead of a silent overwrite.
+// Neither key holds a ".." segment, so O_EXCL is what catches this pair.
 func (suite *AWSTestSuite) TestGetS3DataFromClientCollidingKeysAreAnError() {
 	client := &FakeS3Client{
 		Bucket: fakeS3TestBucketName,
@@ -1375,17 +1355,13 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientCollidingKeysAreAnError() {
 
 	_, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger())
 	require.Error(suite.T(), err)
-	// FakeS3Client lists lexicographically and '/' sorts before 'b', so "a//b"
-	// downloads first and "a/b" is the key that collides.
+	// '/' 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")
 }
 
-// TestGetS3DataFromClientObjectAndPrefixCollideAreAnError covers a bucket
-// holding both an object "a" and objects under the prefix "a/": legal in S3,
-// impossible on a filesystem. "a" downloads first and becomes a file, so
-// MkdirAll for "a/b" fails with ENOTDIR. That is a property of the bucket, not
-// the machine, so the error names "a/b" and advises excluding one of the two.
+// 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,
@@ -1401,11 +1377,6 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientObjectAndPrefixCollideAreAnErr
 	require.Contains(suite.T(), err.Error(), "--exclude-regex")
 }
 
-// TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnFilesystemErrors pins
-// that an error about the machine rather than the key (here an unwritable
-// download directory) names the key for context but does not suggest
-// excluding it: following that advice would record a snapshot with a
-// legitimate object silently missing.
 func (suite *AWSTestSuite) TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnFilesystemErrors() {
 	if runtime.GOOS == "windows" {
 		suite.T().Skip("chmod on a directory does not block file creation on Windows")
@@ -1425,8 +1396,8 @@ func (suite *AWSTestSuite) TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnF
 		},
 	}
 
-	// A bare key fails in OpenFile; a nested one fails in MkdirAll, since only
-	// then is there a directory left to create. Both wraps must behave alike.
+	// 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())
@@ -1438,12 +1409,8 @@ func (suite *AWSTestSuite) TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnF
 	}
 }
 
-// TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys pins that accepted
-// odd-shaped keys still land exactly where filepath.Join put them before
-// this change, so buckets that snapshot cleanly today keep the same
-// fingerprint. Comparing fingerprints (rather than the temp dir layout
-// directly) is the same technique TestGetS3DataFromClientFilterEquivalence
-// uses above.
+// 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")
diff --git a/internal/aws/s3_contract_test.go b/internal/aws/s3_contract_test.go
index aaa689e00..d01e03e2d 100644
--- a/internal/aws/s3_contract_test.go
+++ b/internal/aws/s3_contract_test.go
@@ -44,12 +44,8 @@ func runS3ContractTests(t *testing.T, client S3API, bucket, existingKey string)
 		}
 	})
 
-	// Two tests in aws_test.go assert which of two colliding keys an error
-	// names, which follows from the listing order, so the order is a checked
-	// contract rather than an implementation detail of the fake.
-	// Walked through the paginator so the order is checked across pages, which
-	// is how getS3DataFromClient consumes the listing; a single page from the
-	// fake holds one key and would pass vacuously.
+	// 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{