fix(storage): send explicit content-length on S3 requests - #235
Conversation
Strict S3-compatible endpoints (e.g. Google Cloud Storage's XML API) reject requests that go out with no Content-Length (chunked transfer) with a 411, which broke every multipart upload to those backends. Stream sizes are measured when getSize() returns null but the stream is seekable. Fixes appwrite/appwrite#13548
 This PR is not safe to merge because unknown-size uploads can remain chunked in the default cURL transport, and the implementation-coupled tests must also be replaced to satisfy the repository requirement.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $headers['host'] = $this->host; | ||
| $headers['date'] = gmdate('D, d M Y H:i:s T'); | ||
| $headers['content-md5'] = $md5; | ||
| $headers['content-length'] = (string) $this->bodyLength($body); |
There was a problem hiding this comment.
For seekable streams whose getSize() is unknown—the exact case this change intends to fix—setting this header does not give the default cURL transport the measured size. The transport calls getSize() again, leaves CURLOPT_INFILESIZE unset, and configures an HTTP/1.1 upload with an unknown length. It can therefore still use chunked transfer encoding despite the signed Content-Length, causing strict S3-compatible endpoints to continue rejecting these uploads. Pass the measured length to the transport or otherwise ensure it uses this value for request framing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/storage/src/Storage/Device/S3.php
Line: 750
Comment:
**Unknown uploads remain chunked**
For seekable streams whose `getSize()` is unknown—the exact case this change intends to fix—setting this header does not give the default cURL transport the measured size. The transport calls `getSize()` again, leaves `CURLOPT_INFILESIZE` unset, and configures an HTTP/1.1 upload with an unknown length. It can therefore still use chunked transfer encoding despite the signed `Content-Length`, causing strict S3-compatible endpoints to continue rejecting these uploads. Pass the measured length to the transport or otherwise ensure it uses this value for request framing.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| // createMultipartUpload (POST, empty body), two uploadPart PUTs, | ||
| // exists() probe (HEAD), completeMultipartUpload (POST, XML body) | ||
| $this->assertCount(5, $client->requests); | ||
| $this->assertSame('0', $client->requests[0]->getHeaderLine('content-length')); | ||
| $this->assertSame('10', $client->requests[1]->getHeaderLine('content-length')); | ||
| $this->assertSame('6', $client->requests[2]->getHeaderLine('content-length')); | ||
| $this->assertSame('0', $client->requests[3]->getHeaderLine('content-length')); | ||
| $completeLength = (int) $client->requests[4]->getHeaderLine('content-length'); | ||
| $this->assertGreaterThan(0, $completeLength); | ||
| $this->assertSame(\strlen((string) $client->requests[4]->getBody()), $completeLength); |
There was a problem hiding this comment.
This test mirrors the implementation's exact five-request sequence, ordering, and body lengths instead of testing observable endpoint behavior. That violates the repository directive against implementation-coupled tests. The same coupling appears in the authorization-string assertion and the unknown-size-stream header assertion. More importantly, the capturing fake bypasses the real cURL framing behavior, allowing the transport regression to pass. This repository requirement must be satisfied before merging; replace these checks with a protocol-level test using the actual adapter and a strict endpoint.
Context Used: Call out and harshly judge implementation-coupled ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/storage/tests/Storage/Device/S3Test.php
Line: 751-760
Comment:
**Tests mirror request internals**
This test mirrors the implementation's exact five-request sequence, ordering, and body lengths instead of testing observable endpoint behavior. That violates the repository directive against implementation-coupled tests. The same coupling appears in the authorization-string assertion and the unknown-size-stream header assertion. More importantly, the capturing fake bypasses the real cURL framing behavior, allowing the transport regression to pass. This repository requirement must be satisfied before merging; replace these checks with a protocol-level test using the actual adapter and a strict endpoint.
**Context Used:** Call out and harshly judge implementation-coupled ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Fixes appwrite/appwrite#13548
Problem
A user reported on appwrite/appwrite today that file uploads to S3-compatible storage (GCP Cloud Storage's XML API in their case) fail with
411 Length Requiredas soon as the file is big enough to be chunked. Strict S3-compatible endpoints reject any request that goes out without an explicitContent-Length, and multipartuploadPartPUTs were exactly that: onlycontent-type,content-md5, host/date and the SigV4 headers were set, so the transport fell back to chunked transfer encoding.Root cause
S3::call()builds the PSR-7 request without aContent-Lengthheader. When the body stream reports a size the transport may add one, but upload pipelines frequently hand over streams whosegetSize()isnull(decorated/pump streams), so the request leaves withTransfer-Encoding: chunkedand the endpoint 411s.Fix
S3::call()now setscontent-lengthon every request, measured from the body stream. WhengetSize()returnsnullbut the stream is seekable (whichhashBody()already requires), the size is measured by seeking to the end and back. The header is included in the SigV4 signed headers, which AWS and compatible endpoints accept.+25 lines in
packages/storage/src/Storage/Device/S3.php, plus 3 regression tests.Evidence
Tests added in
packages/storage/tests/Storage/Device/S3Test.phpwith a capturing PSR-18 client that records the requests the realS3::call()puts on the wire:testWriteSendsContentLength- single PUT carriescontent-length: 11for an 11-byte body, and the header is covered by the signaturetestMultipartUploadSendsContentLengthForEveryRequest- every request in the multipart flow (createMultipartUpload, both uploadParts, the exists probe, completeMultipartUpload) carries a correctcontent-lengthtestContentLengthIsMeasuredForSeekableStreamWithUnknownSize- a stream reportinggetSize() === nullstill yieldscontent-length: 11bin/monorepo check storage --fix(Pint + PHPStan + Rector) passes clean. The docker-backed E2E suite was not runnable locally; the unit suite is fully green.