mirror of
https://github.com/versity/versitygw.git
synced 2026-09-23 16:34:18 +00:00
DavvyyandGitHub
d03ac3c299
* fix(s3api): reject a PUT whose body ends before Content-Length
With a plain (non-aws-chunked) request body, a PutObject whose body ends
before Content-Length bytes have arrived is committed as a complete but
shorter object. Real S3 rejects this with IncompleteBody and the object
never becomes visible.
Nothing on that path compares bytes received against Content-Length:
fasthttp reports a connection closed mid-body as a plain io.EOF (the
conversion to io.ErrUnexpectedEOF exists only on the chunked
transfer-encoding branch), the authentication middleware leaves an
UNSIGNED-PAYLOAD body unwrapped, and io.Copy treats io.EOF as a normal
end of stream.
ErrIncompleteBody already exists and is enforced by the chunk readers.
This adds the equivalent check for plain bodies, in the controller so
that every backend is covered, and only when the body is not already an
aws-chunked reader.
POST-Object is deliberately untouched: it is a separate handler whose
ContentLength is an upper bound, so a byte-count check there would
break browser form uploads.
* test(s3api): cover ContentLengthReader
Complete and empty bodies, truncated bodies (including an EOF delivered
together with the final bytes, and one byte per Read), a body longer
than announced, and pass-through of a non-EOF error.
* fix(s3api): check plain bodies against Content-Length, not the decoded length
Review catch: the controller replaces contentLength with
X-Amz-Decoded-Content-Length whenever that header is present, regardless
of payload type, and the previous commit fed that value to
ContentLengthReader. A complete plain upload whose decoded header is
larger than Content-Length was then rejected with IncompleteBody, where
it had succeeded before.
AWS S3 ignores X-Amz-Decoded-Content-Length on a plain body and stores
Content-Length bytes. The decoded length only describes aws-chunked
payloads, and those skip this wrapper anyway, so the check now reads the
raw Content-Length header.
If the header is missing or unparseable the body is left unwrapped: an
aws-chunked request without Content-Length is already rejected earlier
with ErrMissingContentLength, so there is nothing to check here.
* test(integration): cover aborted uploads and the decoded-length case
Three tests, as requested in review.
PutObject_aborted_plain_body plain PUT, body ends early -> no object
PutObject_aborted_streaming_body same for an aws-chunked upload
PutObject_plain_body_with_decoded_length
complete plain PUT carrying
X-Amz-Decoded-Content-Length still
succeeds and stores Content-Length bytes
The abort is driven by a reader that fails partway, so the transport
tears the connection down mid-body - that is what a client that dies or
cancels looks like on the wire. Sending a short body with a normal
reader would not reproduce it: net/http would simply report the
mismatch itself.
The third test is the regression the review found. It fails without the
accompanying fix.
* test(integration): replace the bogus streaming abort test with a real one
The reviewer is right: putObjectAborted always sends
x-amz-content-sha256: UNSIGNED-PAYLOAD, and the gateway decides aws-chunked
from that header alone, so PutObject_aborted_streaming_body took the same
plain path as the test above it. It duplicated the plain test instead of
covering the streaming one.
Dropped it and added UnsignedStreamingPayloadTrailer_aborted_connection in
tests/integration/unsigned_streaming_payload_trailer.go, where it belongs.
UnsignedStreamingPayloadTrailer_incomplete_body already covers malformed and
truncated framing, but every case there is a COMPLETE request. The new test is
the other shape: valid framing whose bytes simply stop arriving.
54 lines
1.9 KiB
Go
54 lines
1.9 KiB
Go
// Copyright 2026 Versity Software
|
|
// This file is licensed under the Apache License, Version 2.0
|
|
// (the "License"); you may not use this file except in compliance
|
|
// with the License. You may obtain a copy of the License at
|
|
//
|
|
// http://www.apache.org/licenses/LICENSE-2.0
|
|
//
|
|
// Unless required by applicable law or agreed to in writing,
|
|
// software distributed under the License is distributed on an
|
|
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
|
|
// KIND, either express or implied. See the License for the
|
|
// specific language governing permissions and limitations
|
|
// under the License.
|
|
|
|
package utils
|
|
|
|
import (
|
|
"errors"
|
|
"io"
|
|
|
|
"github.com/versity/versitygw/debuglogger"
|
|
"github.com/versity/versitygw/s3err"
|
|
)
|
|
|
|
// ContentLengthReader turns an EOF that arrives before Content-Length bytes
|
|
// were read into an IncompleteBody error.
|
|
//
|
|
// fasthttp's request stream reports a client that closed the connection in
|
|
// the middle of a fixed-length body as a plain io.EOF (only the chunked
|
|
// transfer-encoding path is converted to io.ErrUnexpectedEOF), and io.Copy
|
|
// treats io.EOF as a normal end of stream. Without this check an aborted
|
|
// upload is committed as a complete, shorter object. aws-chunked bodies do
|
|
// not need this: their chunk readers already validate the framing.
|
|
type ContentLengthReader struct {
|
|
r io.Reader
|
|
remaining int64
|
|
}
|
|
|
|
func NewContentLengthReader(r io.Reader, contentLength int64) *ContentLengthReader {
|
|
return &ContentLengthReader{r: r, remaining: contentLength}
|
|
}
|
|
|
|
func (cr *ContentLengthReader) Read(p []byte) (int, error) {
|
|
n, err := cr.r.Read(p)
|
|
cr.remaining -= int64(n)
|
|
if cr.remaining > 0 && (errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF)) {
|
|
debuglogger.Logf("request body ended %v bytes short of Content-Length", cr.remaining)
|
|
return n, s3err.GetAPIError(s3err.ErrIncompleteBody)
|
|
}
|
|
return n, err
|
|
}
|
|
|
|
var _ io.Reader = &ContentLengthReader{}
|