Skip to content

fix: reject aws-chunked frames beyond decoded length - #246

Open
perfloop[bot] wants to merge 2 commits into
mainfrom
perfloop-pr-open-4nvq6fhy5h
Open

perfloop[bot] wants to merge 2 commits into
mainfrom
perfloop-pr-open-4nvq6fhy5h

Conversation

@perfloop

@perfloop perfloop Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Signing-mode PutObject now rejects an AWS data-frame header whose size exceeds the remaining X-Amz-Decoded-Content-Length, before yielding bytes from that frame. This prevents a forwarded prefix from being reported as a complete upload.

If the upstream responds before the request-body writer reaches later frames, TAG waits for the writer to stop and checks unread frames on the shared decoder before relaying a 2xx. The post-response read uses the existing upstream request timeout; a timeout returns an error. A zero-length stream is checked before the body is replaced with http.NoBody. Validation does not add unread bytes to the write-through tee.

If TAG has received an upstream 2xx before a later frame exceeds the declared length, it withholds that response. The ordinary proxy cache path invalidates the key again when caching is enabled and does not warm from the partial body. Tiered mode requires a pre-forward metadata snapshot for positive-length signing-forwarded AWS-chunked object PUTs; if the snapshot cannot be read, TAG returns an error before forwarding. After a known upstream success, tiered cleanup removes only the captured prior version so a newer local write survives.

The routed early-response test no longer waits for the client's WroteRequest callback before releasing the gateway body gate. It uses the upstream-prefix schedule instead, so it does not depend on the remaining 1 MiB request fitting in TCP buffers. No public S3 operation, API, or setting is added.

Regression tests cover:

  • TestAWSChunkedReaderRejectsOversizedHeaderBeforePayload and TestRoutedAWSChunkedDecodedExtent for rejecting overlong headers before payload and preserving valid, empty, zero-length, and missing-length cases.
  • TestRoutedAWSChunkedExtentWaitsForEarlyUpstreamResponse, TestSuccessfulChunkedValidationReadUsesDeadline, and TestResponseTrackerUnwrapsReadDeadline for early upstream responses and bounded validation reads through the handler wrappers.
  • TestHandlePutObject_ReinvalidatesAfterUpstreamSuccessAndValidationError, TestTieredPutInvalidatesDisplacedPriorOnlyAfterKnownUpstreamSuccess, and TestTieredSignedChunkedPutRequiresPriorSnapshotBeforeForward for cache cleanup, newer-write preservation, and fail-closed snapshot reads.
  • TestHandlePutObject_WriteThroughTee_IncompleteBufferFallsBackToWarm for the existing tee fallback.

The recorded checks passed: make build, make lint, make lint-ci, make test, make test-race, make test-coverage, and make test-integration TESTRUN=^TestSDK_PutObject_ChunkedEncoding.

The correctness comparison uses 4df832ad38e8d9cc6cea919603be1ccd7657b75f and records violated there and satisfied at the final source. It uses sh -c "exec \"$PERFLOOP_BUILD_OUTPUT_DIR/aws-chunk-extent-proof\"".

Recorded proof ran with retained support that is not included in this pull request.

The proof uses loopback clients and upstreams. It does not measure production frequency or effects on a real storage backend. Earlier valid frames may already have reached upstream; TAG does not roll them back.

Target CI also runs Test / test, Lint PR Title / lint, CLA Assistant / cla, S3 Compatibility Tests / s3-compat-tests (default), S3 Compatibility Tests / s3-compat-tests (tiered), and S3 Compatibility Tests (Cluster) / s3-compat-tests-cluster (default).


Generated by Perfloop. Measurements and checks. Reproduce this comparison (tenant access required).

@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5 Tier: plus

[High risk] Adds validation to AWS chunked upload frame parsing.

The PR is not yet safe to merge because a timed-out zero-length frame probe can result in a successful empty PUT.

Fix All in Claude CodeFindings

  1. P1 Zero-length probe ignores read errors ▶

Summary

The PR checks AWS-chunked frame sizes against the declared decoded length, validates unread frames before relaying an early upstream success, and adds cache cleanup for withheld successes.

  • Adds regression coverage for routed uploads, deadlines, cache invalidation, and tiered metadata snapshots.
  • The zero-length probe still needs to reject read failures rather than forward an empty upload.

Reviews (2) · Last reviewed commit: "fix: gate tiered chunked PUT forwarding"

Comment thread proxy/forwarder.go Outdated
Comment thread proxy/forwarder.go Outdated
Comment thread handlers/aws_chunked_early_response_test.go Outdated
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
@perfloop
perfloop Bot force-pushed the perfloop-pr-open-4nvq6fhy5h branch from 533fa93 to 014e012 Compare October 3, 2026 01:39
Comment thread proxy/forwarder.go
Comment on lines +690 to +696
n, err := fwdReq.Body.Read(probe[:])
if n > 0 {
return errAWSChunkExceedsDecodedLength
}
if errors.Is(err, errAWSChunkExceedsDecodedLength) {
return err
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Zero-length probe ignores read errors

For an AWS-chunked PUT declaring zero decoded bytes, the new probe waits for the first frame header. If that read times out, this code ignores the error and replaces the body with http.NoBody. TAG can then forward an empty PUT and relay upstream success for an incomplete upload. Return the read error instead.

Suggested change
n, err := fwdReq.Body.Read(probe[:])
if n > 0 {
return errAWSChunkExceedsDecodedLength
}
if errors.Is(err, errAWSChunkExceedsDecodedLength) {
return err
}
n, err := fwdReq.Body.Read(probe[:])
if n > 0 {
return errAWSChunkExceedsDecodedLength
}
if err != nil && !errors.Is(err, io.EOF) {
return err
}

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid: the zero-length probe must distinguish clean EOF from a failed or timed-out body read. I will propagate non-EOF read errors and keep the existing handling for an empty stream.

Source feedback

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 014e012. Configure here.

Comment thread proxy/forwarder.go
return tracker.readErr
}
if tracker.eof || tracker.readErr != nil || tracker.validator == nil {
return nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Body-read timeout still relays success

Medium Severity

validateDecodedExtent only rejects readErr when it is an extent violation. A request-body timeout that already stopped the transport writer hits the generic readErr != nil branch and returns success, so an early upstream 2xx is relayed. Comments on this helper say read timeouts reject that 2xx, and the later validateRemainingDecodedExtent path does apply isRequestBodyReadTimeout.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 014e012. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid: when a body-read timeout stops the transport writer, an early upstream 2xx must not be relayed as success. I will make the early-response validation reject that timeout consistently with the later-frame path.

Source feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant