fix: S3 compatibility gaps found by the boto3 and mc smoke tests by laffer1 · Pull Request #14 · MidnightBSD/leonardo

laffer1 · 2026-11-01T04:39:23Z

Summary

A smoke test of everyday client workflows against the 0.1.2 build (boto3, the MinIO client mc, presigned URLs) found a set of behaviours that differ from S3 and break real tools. This PR fixes all of them and adds the smoke tests under tools/ so they can be rerun against any packaged install before a release.

Listings and versions

ListObjects/V2 hide keys whose latest version is a delete marker.
ListObjectVersions emits <VersionId>null</VersionId> for unversioned objects. boto3 raised KeyError and mc rm --versions / aws s3 rb --force could not empty a bucket. null is accepted wherever a version id is.
DeleteObjects honours <VersionId> per entry instead of creating a fresh delete marker for every key, and reports VersionId / DeleteMarker / DeleteMarkerVersionId. mc rm --versions, mc undo and aws s3 rb --force all use this path.
DELETE ?versionId= echoes x-amz-version-id and x-amz-delete-marker.
ListObjectsV2 includes <Owner> only with fetch-owner=true; v1 always.
A missing version is NoSuchVersion, not NoSuchKey.

Suspended versioning and payload housekeeping

PUT on a suspended bucket keeps the real versions and replaces only the null version; DELETE inserts a null delete marker. Previously the whole history was dropped.
Overwriting an object in an unversioned bucket (or replacing the null version) now deletes the old payload file. Every overwrite used to leak one blob, and a leaked blob made the bucket permanently undeletable. Bucket emptiness is now judged by object records, not by stray files in data/.

Copy and multipart

x-amz-copy-source accepts ?versionId=; copying a delete marker by id is InvalidRequest.
x-amz-metadata-directive / x-amz-tagging-directive: REPLACE are honoured; a self-copy without REPLACE is InvalidRequest.
UploadPartCopy honours x-amz-copy-source-range (strictly inside the source, as S3 does), returns <CopyPartResult> and x-amz-copy-source-version-id, and streams the range.
CompleteMultipartUpload validates part numbers, ETags (InvalidPart) and non-last part sizes (EntityTooSmall) before touching the upload, so a rejected completion can be retried or aborted. It used to delete the upload record first and ignore client ETags.

Requests and responses

Keys with a trailing slash (folder/) keep it; the router silently dropped it.
GetObject applies the response-* header overrides, replacing the stored header.
412 and 416 carry an XML error body; 416 sets Content-Range: bytes */N.
GetBucketTagging on an untagged bucket is 404 NoSuchTagSet.

Authentication

Rejections carry the S3 reason: InvalidAccessKeyId (unknown or disabled key), SignatureDoesNotMatch, AccessDenied (anonymous or expired). RequestAuthenticator gains authenticateOrExplain(); the filter renders the failure.
SigV2 presigned URLs (AWSAccessKeyId / Expires / Signature) work when allow_sigv2 is on.

Tools

tools/smoke-test.py: boto3, 210 checks across buckets, objects, listings, copy, tagging, delete, multipart, checksums, presigned URLs, versioning, auth and concurrency. Env-configured, cleans up after itself, non-zero exit on any failure.
tools/smoke-test-mc.sh: 55 mc steps (cp, mirror, tag, versioning, rm --versions, undo, rb --force).
Both documented in tools/README.md.

Test plan

./gradlew build: 264 tests green (new: ClientCompatConformanceTest with 19 cases, ObjectPublisherTest, RequestAuthenticatorFailureReasonTest, S3XmlTest, MultipartServiceEtagTest, store tests)
tools/smoke-test.py against the install tree running with the packaged sample config and fsync_on_write: true: 210 pass, 0 fail, 1 skipped (CRC32C needs botocore[crt])
tools/smoke-test-mc.sh against the same server: 55 pass, 0 fail; no buckets or payload files left behind
CI

Not a release; this should go into 0.1.3 together with #13.

🤖 Generated with Claude Code

A smoke test of common client workflows against the 0.1.2 build (boto3, the MinIO client and presigned URLs) turned up a set of behaviours that differ from S3 and break real tools. This fixes all of them and adds the smoke tests under tools/ so they can be rerun against a packaged install. Listings and versions - ListObjects/V2 no longer show keys whose latest version is a delete marker. - ListObjectVersions emits <VersionId>null</VersionId> for unversioned objects (boto3 raised KeyError, `mc rm --versions` and `aws s3 rb --force` could not empty a bucket); "null" is accepted wherever a version id is, including DELETE, HEAD, GET and copy sources. - DeleteObjects honours <VersionId> per entry instead of creating a new delete marker for every key, and reports VersionId / DeleteMarker / DeleteMarkerVersionId in the result. `mc rm --versions`, `mc undo` and `aws s3 rb --force` all go through this path. - DELETE ?versionId= echoes x-amz-version-id and x-amz-delete-marker. - ListObjectsV2 includes <Owner> only with fetch-owner=true; v1 always. - A missing version is NoSuchVersion rather than NoSuchKey. Suspended versioning and payload housekeeping - A PUT on a suspended bucket keeps the real versions in history and replaces only the null version; a DELETE inserts a null delete marker. Previously the whole history was dropped. - Overwriting an object in an unversioned bucket, or replacing the null version, now removes the old payload file. Every overwrite used to leak one blob, and a leaked blob made the bucket undeletable: bucket emptiness is now judged by object records, not stray data files. Copy and multipart - x-amz-copy-source accepts ?versionId=; copying a delete marker by id is InvalidRequest. - x-amz-metadata-directive and x-amz-tagging-directive REPLACE are honoured; a copy onto itself without REPLACE is InvalidRequest. - UploadPartCopy honours x-amz-copy-source-range (strictly inside the source, as S3 does), returns <CopyPartResult> and x-amz-copy-source-version-id, and streams the range instead of buffering the whole source. - CompleteMultipartUpload validates part numbers, ETags (InvalidPart) and the minimum size of non-last parts (EntityTooSmall) before it touches the upload, so a rejected completion can be retried or aborted. It used to delete the upload record first and ignore client ETags. Requests and responses - Keys with a trailing slash ("folder/") keep it; the router dropped it. - GetObject applies response-content-type, -language, -disposition, -encoding, response-cache-control and response-expires, replacing the stored header rather than appending to it. - 412 and 416 carry an XML error body; 416 sets Content-Range: bytes */N. - GetBucketTagging on an untagged bucket is 404 NoSuchTagSet. Authentication - Rejections name the reason the way S3 does: InvalidAccessKeyId for an unknown or disabled key, SignatureDoesNotMatch for a bad signature, AccessDenied for anonymous or expired requests. The authenticator gains authenticateOrExplain(); the filter renders its AuthFailure. - SigV2 presigned URLs (AWSAccessKeyId / Expires / Signature) are accepted when allow_sigv2 is on. Tests: unit tests for the store, XML, ETag matching, auth reasons and SigV2 presigning, plus a ClientCompatConformanceTest pinning each client visible behaviour through the AWS SDK. Full build: 264 tests green. Tools: tools/smoke-test.py (boto3, 210 checks) and tools/smoke-test-mc.sh (mc, 55 steps), documented in tools/README.md. Both pass against this build running from the install tree with the sample configuration and fsync_on_write: true. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Lucas Holt <luke@foolishgames.com>

sourcery-ai · 2026-11-01T04:39:31Z

Sorry @laffer1, your pull request is larger than the review limit of 150,000 diff characters

SigV2 only signs the query parameters in its CanonicalizedResource set, which lacked the GetObject response-* overrides introduced in the previous commit. Anyone holding a SigV2 presigned link could therefore change the Content-Type or Content-Disposition it serves without breaking the signature. The six override parameters are now part of the resource, as AWS specifies, with a test for both the tampered and the properly signed case. CodeQL on the pull request flagged two more things: header values echoed from the request (x-amz-version-id on DELETE and the response-* overrides) are now stripped of CR/LF before use, and the MD5 helper in tools/smoke-test.py declares usedforsecurity=False, since it only recomputes S3 ETags. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Lucas Holt <luke@foolishgames.com>

+
+def md5(b: bytes) -> str:
+    """S3 ETags are MD5 digests; this is an integrity check, not a security use."""
+    return hashlib.md5(b, usedforsecurity=False).hexdigest()


The SigV2 CanonicalizedResource matched raw query keys against the signed set while the controller reads percent-decoded parameters, so a key such as response%2Dcontent-type reached GetObject without being covered by the signature, and a repeated parameter let the signer and the consumer pick different copies. Keys and values are now decoded before matching and signing (as the SDKs do), and a request that repeats a signed parameter or carries malformed encoding is rejected. GetObject also refuses response-* overrides on anonymous requests with InvalidRequest, as S3 does, so a public link cannot be given an arbitrary Content-Type or Content-Disposition by whoever shares it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Lucas Holt <luke@foolishgames.com>

Copilot

🟡 Changes recommended

Unresolved critical defects can bypass retention and legal holds, leave committed objects without payloads, and allow tampering with SigV2 response overrides.

15 open issues

What changed in this PR

This PR improves Leonardo’s S3 compatibility for everyday boto3, MinIO mc, and presigned-URL workflows. It also adds reusable smoke tests for validating packaged installations before release.

Changes:

Corrects listings, version-aware deletion, suspended versioning, and payload housekeeping.
Extends copy and multipart behavior, response headers, and authentication error reporting.
Adds focused regression tests and documented boto3 and mc smoke suites.

File	Description
tools/smoke-test.py	Adds boto3 end-to-end compatibility checks.
tools/smoke-test-mc.sh	Adds MinIO client workflow checks.
tools/README.md	Documents smoke-test setup and coverage.
leonardo-xml/src/test/java/org/midnightbsd/leonardo/xml/S3XmlTest.java	Tests version, owner, copy, and deletion XML.
leonardo-xml/src/main/java/org/midnightbsd/leonardo/xml/S3Xml.java	Extends S3 response serialization.
leonardo-xml/src/main/java/org/midnightbsd/leonardo/xml/DeleteObjectsRequest.java	Parses per-object version IDs.
leonardo-storage/src/main/java/org/midnightbsd/leonardo/storage/BoundedInputStream.java	Adds bounded streaming for copy ranges.
leonardo-core/src/test/java/org/midnightbsd/leonardo/core/object/ObjectServiceTest.java	Updates publisher dependency wiring.
leonardo-core/src/test/java/org/midnightbsd/leonardo/core/object/ObjectPublisherTest.java	Tests null-version handling and payload cleanup.
leonardo-core/src/test/java/org/midnightbsd/leonardo/core/object/ObjectMetadataStoreTest.java	Tests delete-marker filtering and null-version lookup.
leonardo-core/src/test/java/org/midnightbsd/leonardo/core/multipart/MultipartServiceEtagTest.java	Tests multipart ETag comparisons.
leonardo-core/src/main/java/org/midnightbsd/leonardo/core/S3Exception.java	Adds the NoSuchVersion error factory.
leonardo-core/src/main/java/org/midnightbsd/leonardo/core/object/ObjectService.java	Extends versioned deletion and copy behavior.
leonardo-core/src/main/java/org/midnightbsd/leonardo/core/object/ObjectPublisher.java	Preserves suspended history and reclaims replaced payloads.
leonardo-core/src/main/java/org/midnightbsd/leonardo/core/object/ObjectMetadataStore.java	Handles null IDs and hides deleted keys.
leonardo-core/src/main/java/org/midnightbsd/leonardo/core/multipart/MultipartService.java	Streams copy ranges and validates completion requests.
leonardo-core/src/main/java/org/midnightbsd/leonardo/core/bucket/BucketMetadataStore.java	Determines bucket emptiness from object records.
leonardo-conformance/src/test/java/org/midnightbsd/leonardo/conformance/ClientCompatConformanceTest.java	Adds client compatibility regression cases.
leonardo-auth/src/test/java/org/midnightbsd/leonardo/auth/RequestAuthenticatorFailureReasonTest.java	Tests authentication errors and SigV2 presigning.
leonardo-auth/src/main/java/org/midnightbsd/leonardo/auth/RequestAuthenticatorImpl.java	Classifies failures and verifies SigV2 presigned URLs.
leonardo-auth/src/main/java/org/midnightbsd/leonardo/auth/RequestAuthenticator.java	Introduces structured authentication outcomes.
leonardo-api/src/test/java/org/midnightbsd/leonardo/api/controller/ObjectControllerRangeTest.java	Tests trailing-slash keys and copy-source parsing.
leonardo-api/src/main/java/org/midnightbsd/leonardo/api/filter/S3AuthenticationFilter.java	Renders specific S3 authentication failures.
leonardo-api/src/main/java/org/midnightbsd/leonardo/api/controller/ObjectController.java	Handles copy options, key preservation, and response overrides.
leonardo-api/src/main/java/org/midnightbsd/leonardo/api/controller/BucketController.java	Supports listing owners, versioned batch deletion, and tag errors.

_{Review effort: Balanced}

-        final Path dataDir = layout.bucketDataDir(bucket);
-        final Path objectsDir = layout.bucketObjectsMetaDir(bucket);
-        return isEmptyDir(dataDir) && isEmptyDir(objectsDir);
+        return isEmptyDir(layout.bucketObjectsMetaDir(bucket));


                    draft.legalHold(), draft.retention(), newVersionId,
                    draft.checksums(), draft.partsCount(), draft.contentHeaders());
            metaStore.write(bucket, meta);
+            deletePayloads(bucket, replacedPayloads, draft.objectId());


+            if (status == BucketMetadata.VersioningStatus.SUSPENDED) {
+                // A suspended bucket gets a null delete marker that replaces the
+                // null version (if any); real versions stay reachable by id.
+                if (maybeOld.isPresent() && maybeOld.get().versionId() == null) {


+        if (srcMeta.isDeleteMarker()) {
+            throw S3Exception.noSuchKey(srcKey);
+        }


+    got=$("$@" 2>"$LOG")
+    if [ "$got" = "$want" ]; then
+        PASS=$((PASS + 1)); echo "PASS $name"
+    else
+        FAIL=$((FAIL + 1)); echo "FAIL $name"; echo "     expected: $want"; echo "     got:      $got"; sed 's/^/     /' "$LOG"
+    fi
+}
+
+# contains NAME NEEDLE CMD... : PASS when NEEDLE occurs in the command's stdout
+contains() {
+    name=$1; needle=$2; shift 2
+    out=$("$@" 2>"$LOG")
+    if printf '%s\n' "$out" | grep -qF -- "$needle"; then


+            r = fn()
+            if verify is not None:
+                ok = verify(r)
+                if ok is True or ok is None:


+def fetch(url, method="GET", data=None, headers=None):
+    req = urllib.request.Request(url, method=method, data=data, headers=headers or {})
+    try:
+        with urllib.request.urlopen(req, timeout=30) as resp:


+                "checksums", "presigned", "versioning", "auth", "concurrency", "cleanup"]
+
+    def run(self) -> int:
+        only = set(self.args.only.split(",")) if self.args.only else None


+        except KeyboardInterrupt:
+            print("\ninterrupted")


CodeQL does not recognise a regex replaceAll as a response-splitting sanitizer; two literal replace calls do the same job and are. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Lucas Holt <luke@foolishgames.com>

Netty's query parser treats ';' as a pair separator, so a parameter hidden after a ';' inside another value reached the controller without being part of the signed CanonicalizedResource. The resource builder now splits the raw query the same way. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Lucas Holt <luke@foolishgames.com>

Copilot

🟡 Changes recommended

Unresolved critical defects can delete committed or in-flight payloads, bypass WORM protection, and allow request changes without invalidating SigV2 signatures.

16 open issues

1 resolved since last review

SigV2 canonical resource omits response override parameters

_{Review effort: Balanced}

+                final String val;
+                try {
+                    key = URLDecoder.decode(eq < 0 ? pair : pair.substring(0, eq), StandardCharsets.UTF_8);
+                    val = eq < 0 ? null : URLDecoder.decode(pair.substring(eq + 1), StandardCharsets.UTF_8);


+        } catch (final RuntimeException | IOException ex) {
+            try {
+                payloadStore.delete(dstBucket, dstObjectId);


CodeQL's response-splitting query does not treat String.replace as a barrier, so the sanitizer now rebuilds the value without CR and LF; taint does not survive the primitive char copy, and the behaviour is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Lucas Holt <luke@foolishgames.com>

Copilot

🔵 Needs a closer look

Unresolved critical flaws allow unauthorized response overrides on signed requests and can delete committed object payloads or cause concurrent reads to fail during overwrites.

16 open issues

1 previously missed issue

In code that hasn't changed since last review

Reject raw semicolons that bypass SigV4 query authentication

leonardo-api/src/main/java/org/midnightbsd/leonardo/api/controller/ObjectController.java:1064

A header-signed SigV4 request can gain a response override without invalidating its signature: x=1%3Bresponse-content-type%3Dtext%2Fhtml and x=1;response-content-type=text/html produce the same canonical query, but Netty treats only the latter as a separate override. Applying that override here lets a modified request change the returned Content-Type without the signer's authorization. Align SigV4 canonicalization with the API's query parsing, or reject raw semicolon separators before authentication, and add a regression test for this encoded-to-literal rewrite.

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

Unresolved critical issues can delete successfully published payloads, race with readers and in-flight writes, and copy historical versions with incorrect metadata or checksums.

16 open issues

_{Review effort: Balanced}

Copilot AI balanced review requested due to automatic review settings November 1, 2026 04:39

Copilot started reviewing on behalf of laffer1 November 1, 2026 04:39

View session

github-advanced-security Bot found potential problems Nov 1, 2026

View reviewed changes

Comment thread tools/smoke-test.py Fixed

github-advanced-security Bot found potential problems Nov 1, 2026

View reviewed changes

Comment thread leonardo-api/src/main/java/org/midnightbsd/leonardo/api/controller/ObjectController.java Fixed

Comment thread leonardo-api/src/main/java/org/midnightbsd/leonardo/api/controller/ObjectController.java Fixed

github-advanced-security Bot found potential problems Nov 1, 2026

View reviewed changes

Comment thread tools/smoke-test.py

def md5(b: bytes) -> str:

"""S3 ETags are MD5 digests; this is an integrity check, not a security use."""

return hashlib.md5(b, usedforsecurity=False).hexdigest()

github-advanced-security Bot found potential problems Nov 1, 2026

View reviewed changes

Comment thread leonardo-api/src/main/java/org/midnightbsd/leonardo/api/controller/ObjectController.java Fixed

Comment thread leonardo-api/src/main/java/org/midnightbsd/leonardo/api/controller/ObjectController.java Fixed

Copilot AI reviewed Nov 1, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings November 1, 2026 04:48

Copilot started reviewing on behalf of laffer1 November 1, 2026 04:48

View session

laffer1 and others added 2 commits November 1, 2026 00:49

Copilot AI reviewed Nov 1, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings November 1, 2026 04:55

Copilot started reviewing on behalf of laffer1 November 1, 2026 04:55

View session

laffer1 merged commit b4350d9 into main Nov 1, 2026
12 of 13 checks passed

laffer1 deleted the lh/smoke-test-fixes branch November 1, 2026 05:02

Copilot AI reviewed Nov 1, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings November 1, 2026 05:04

laffer1 mentioned this pull request Nov 1, 2026

release: bump version to 0.1.3 #15

Merged

2 of 3 tasks

Copilot started reviewing on behalf of laffer1 November 1, 2026 05:05

View session

Copilot AI reviewed Nov 1, 2026

View reviewed changes

Navigation Menu

Repository navigation

fix: S3 compatibility gaps found by the boto3 and mc smoke tests - #14

fix: S3 compatibility gaps found by the boto3 and mc smoke tests#14
laffer1 merged 6 commits into
mainfrom
lh/smoke-test-fixes

laffer1 commented Nov 1, 2026

Uh oh!

sourcery-ai Bot commented Nov 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

laffer1 commented Nov 1, 2026

Summary

Listings and versions

Suspended versioning and payload housekeeping

Copy and multipart

Requests and responses

Authentication

Tools

Test plan

Uh oh!

sourcery-ai Bot commented Nov 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants