backup: reject leading-slash S3 object keys (PR #718, round 5)

Codex P1 round 5 (commit 2f87b84):

`safeJoinUnderRoot` permitted an empty first segment so leading-slash
keys like `/a` were accepted and then normalised by `filepath.Join`
to the same output path as `a`. S3 treats `/a` and `a` as two
distinct keys (the literal byte '/' is part of the key), so a
bucket containing both produced last-flush-wins corruption with no
KEYMAP record. The "absolute path collapses safely under bucket
dir" comfort the previous behaviour leaned on was false comfort:
the collapse silently merged distinct user data.

Now empty segments are refused everywhere — leading, mid-path, and
trailing — with ErrS3MalformedKey. Operators with leading-slash
keys must rename them in S3 first.

The previous test `TestS3_AbsolutePathObjectKeyConfinedUnderBucket`
(which asserted the wrong behaviour) is replaced by
`TestS3_LeadingSlashObjectKeyRejected`.

Author: bootjp

internal/backup/s3.go

@@ -672,23 +672,22 @@ func safeJoinUnderRoot(root, rel string) (string, error) {
 	// them onto one filesystem path; without explicit rejection,
 	// distinct user keys would silently overwrite each other at
 	// finalize (Codex P1 #614).
+	//
+	// Empty segments are rejected wherever they appear — including
+	// the leading position. A leading "/" produces an initial empty
+	// segment (segs[0] == "") which filepath.Join would otherwise
+	// strip, collapsing "/a" onto the same output path as "a".
+	// Because S3 treats those as two distinct keys, last-flush wins
+	// and silently overwrites the other (Codex P1 round 5).
 	segs := strings.Split(rel, "/")
-	for i, seg := range segs {
+	for _, seg := range segs {
 		switch seg {
 		case ".", "..":
 			return "", errors.Wrapf(ErrS3MalformedKey,
 				"object name has dot segment %q (S3 treats it literally; rename in S3 first)", rel)
 		case "":
-			// A leading "/" produces an initial empty segment
-			// (segs[0] == ""); filepath.Join handles that case
-			// safely by stripping the prefix, matching the
-			// already-tested "absolute path collapses under
-			// bucket dir" behaviour. Reject empty segments
-			// anywhere else (mid-path "//" or trailing "/").
-			if i != 0 {
-				return "", errors.Wrapf(ErrS3MalformedKey,
-					"object name has empty path segment %q", rel)
-			}
+			return "", errors.Wrapf(ErrS3MalformedKey,
+				"object name has empty path segment %q", rel)
 		}
 	}
 	cleanRoot := filepath.Clean(root)

internal/backup/s3_test.go

@@ -284,26 +284,21 @@ func TestS3_PathTraversalAttemptRejected(t *testing.T) {
 	}
 }
 
-func TestS3_AbsolutePathObjectKeyConfinedUnderBucket(t *testing.T) {
+func TestS3_LeadingSlashObjectKeyRejected(t *testing.T) {
 	t.Parallel()
-	// filepath.Join normalises a leading "/" on the second arg, so
-	// "/etc/host" becomes "<bucketDir>/etc/host" — under the bucket
-	// root, not at filesystem root. This is safe (the user gets a
-	// surprising-but-confined path) and matches what `aws s3 sync`
-	// would round-trip back. We assert the safe outcome rather than
-	// rejecting; rejection would surprise operators with legitimate
-	// keys whose first byte is '/'.
-	enc, root := newS3Encoder(t)
-	emitObject(t, enc, "b", 1, "/etc/host-confined", []byte("ok"), "")
-	if err := enc.Finalize(); err != nil {
-		t.Fatal(err)
-	}
-	got, err := os.ReadFile(filepath.Join(root, "s3", "b", "etc", "host-confined")) //nolint:gosec
-	if err != nil {
-		t.Fatalf("absolute-path key must end up under the bucket dir: %v", err)
-	}
-	if string(got) != "ok" {
-		t.Fatalf("body=%q", got)
+	// Codex P1 round 5: S3 treats "/a" and "a" as two distinct keys
+	// (the literal byte '/' is part of the key). filepath.Join would
+	// silently strip the leading "/" and collapse both onto the same
+	// output path, so a bucket containing both objects would produce
+	// last-flush-wins corruption with no KEYMAP record. The encoder
+	// must refuse any key whose first segment is empty rather than
+	// "confine and merge" them. Operators with such keys must rename
+	// in S3 first.
+	enc, _ := newS3Encoder(t)
+	emitObject(t, enc, "b", 1, "/etc/host-attack", []byte("ok"), "")
+	err := enc.Finalize()
+	if !errors.Is(err, ErrS3MalformedKey) {
+		t.Fatalf("err=%v want ErrS3MalformedKey for leading-slash key", err)
 	}
 }