Reported by @ry2811 as GHSA-6jh6-gvj5-pv8v.
A resumable upload declares its size, and that declaration is what store()
weighs against the maximum file size and the client's storage quota. Only
the assembled file was ever held to it. The parts in between were bounded
one request at a time and never added up, so a client could declare one
byte and then stream parts: ten thousand part numbers at twice a 20 MB
part is about 400 GB, per session, and the number of sessions was not
bounded either. None of it counted against anything, because nothing
becomes a File row until the upload completes and ClientStorageUsage sums
File rows. A client with a 1 MB quota could fill the volume and repeat.
putPart()'s own comment described this defect and treated the per-part cap
as the answer to it: "without a cap here the exposure is a day's worth of
disk". A cap on one request bounds one request. The exposure was a day's
worth of disk multiplied by however many requests somebody cared to make.
Three limits, and each one exists because the other two do not cover it.
A session may not stage more than it declared. The room for a part is
claimed before the body is read — a body's length is not known until it
has arrived, and by then it is on the disk being protected — and the write
is then capped at exactly what was claimed, so an over-long body is cut
off mid-stream as it always was, against a smaller number. The claim is a
read and a conditional update under a per-session lock, the same shape
complete() already uses: the protocol sends parts in parallel and how many
is the client's choice, so an unlocked read lets every part in flight
claim the same room, while an atomic claim alone refuses the honest
parallel upload instead. Whatever the part really weighs is settled back
afterwards, in a finally, or a client's own retries would exhaust a
session with room to spare.
Open sessions count against the quota at the size they declared. A quota
measured against finished files alone is spent twice by opening sessions
one after another — each is told there is room, because the ones before it
have not finished. The cost is that an abandoned transfer holds its share
until it is cancelled or swept, so the sweeper now runs hourly rather than
daily: that gap is now somebody unable to upload, which it was not before.
And a cap on open sessions, because for anyone with no quota to spend —
staff, and clients on an installation that sets none — the session count
is the only thing between a declared size and any multiple of it.
Four tests fail on the unfixed code, and three existing ones had to change:
they declared a tiny size and sent a large part deliberately, to reach the
re-checks at complete(). That route is now closed at putPart(), so they
reach those re-checks the way a real install would instead — the file-size
limit or the quota moving while a long transfer is running, which is the
reason complete() re-asks rather than trusting what store() decided.
The staged-byte total is BIGINT UNSIGNED, and the suite runs SQLite, which
has no unsigned integers. The first version of the bounds read
`staged_bytes + :delta BETWEEN 0 AND size` and raised SQLSTATE 22003 on
MySQL for any refund — in the comparison, so the bound written to prevent
the underflow was the statement that underflowed. Every SQLite test passed
on it. Both bounds are now arranged so the column is never inside a
subtraction, and UploadSessionStagedBytesMysqlTest skips loudly unless the
connection is MySQL. Verified against 8.4, as was the report itself: three
sessions declaring one byte each put 6 MB on the volume of a client with a
1 MB quota before, and nothing at all after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx
complete() holds a lock whose comment promises "the lock's TTL releases
the claim if a completion dies mid-flight, so a later retry still works".
A retry has nothing to work from but the parts, and assemble() unlinked
each one inside the loop that read it -- so everything that can fail
afterwards took the retry with it.
Measured on main, with a disk refusing the write (the case the guard forty
lines further down was written for, found against a real GCS bucket):
first complete → 422, 0 parts left, the half-written copy left behind
retry → 422 "Upload is incomplete: missing parts."
For good: listParts() is empty, so no later attempt can ever succeed, and
the client has to send the whole file again. The abandoned copy sat in the
session directory until the sweeper came round.
The parts now go when abort() clears the session directory -- which
already ran on success -- and a failure deletes only the half-written copy
it made. The cost is temp space: peak usage during assembly is the whole
file twice over rather than the file plus one part. The docblock says so.
Also checked while here: every read and every write in the concatenation.
A failing fwrite is loud in practice, since Laravel's error handler turns
the warning into an ErrorException, but loud there is a 500 carrying a PHP
message where this method's other storage failure is a sentence the person
uploading can act on. A short write arriving without a warning would be
worse: the byte count and the checksum describe the buffer that was read,
so an unchecked one records a truncated file with a checksum matching
bytes that were never stored.
Two tests: the retry after a refused write now succeeds, and a temporary
directory that refuses writes (/dev/full, skipped where it does not exist)
fails the upload with this method's own message. Without the fix both go
red.
A full parallel run failed once and passed on retry while I was doing the
#1703 follow-up. A flake is worse than a steady failure: it trains you to
re-run rather than look, and it quietly weakens every green run reported
beside it.
Upload parts are real files under storage_path('app/uploads-tmp/{session_id}'),
not a faked disk. Every parallel worker gets its own database, so session
ids restart at 1 in each of them, and two workers writing parts land in
the same directory. On top of that ChunkedUploadsTest's afterEach deleted
the whole tree rather than its own share, for everybody. Six test files
write parts, so this was reachable without anything I added.
The same collision exists inside one worker: RefreshDatabase rolls back,
so ids restart at 1 for every test, and a run that died before its
cleanup leaves parts sitting under the id the next test is about to
claim.
LocalPartStore now reads its root from config, defaulting to exactly
where it always was -- an installation with UPLOAD_PARTS_PATH unset
behaves identically. Tests\TestCase points it at a per-worker directory
and empties that directory per test, which closes the cross-worker, the
cross-run and the intra-worker versions together. ChunkedUploadsTest's
cleanup and its two directory assertions read the configured root rather
than the hardcoded path, so they can no longer reach into a neighbour.
Verified with eight consecutive parallel runs, green, and by watching the
per-worker directories appear separately (w1, w2, w4 … w14) rather than
one shared tree. The isolation itself cannot be asserted from inside a
single test; what a test can pin is the mechanism it rests on, so one
does: parts go where the configured root says.
The failure message names the disk, which reads as a credentials problem
even when the real cause is a bucket name that was never changed — the
exact confusion produced by switching an existing S3 configuration over
to Google and leaving the old bucket in the field.
Logged rather than shown, because 'throw' => false means the reason is
already gone by the time this code runs, and because the message goes to
whoever was uploading. That can be a client, and a bucket name is not
theirs to see.
Two bugs a green suite could not find, both from pointing the
application at a real Google Cloud Storage bucket.
The adapter attaches a legacy per-object ACL to every write, and a
bucket with uniform bucket-level access — which our own setup
instructions require, and which Google recommends — refuses it:
"Cannot insert legacy ACL for an object when uniform bucket-level access
is enabled". So the default configuration could not write to the
recommended bucket. The library ships
UniformBucketLevelAccessVisibility for exactly this, and nothing is
lost by never setting an ACL: every object here is private and every
read is a signed URL.
The second is worse and was never about Google. Both file disks are
configured 'throw' => false, so a refused write returns false rather
than raising, and LocalPartStore ignored the return. The upload reported
success, the File row was written, and the bytes were nowhere — the
listing showed a file whose download could never work. An expired S3
credential did the same thing. It now checks, and the controller already
turns that into a validation error rather than a 500, so the person
uploading is told.
Verified against a live bucket with a key scoped to
roles/storage.objectAdmin: the probe lists, writes land, reads
round-trip byte for byte, and a signed URL comes back 200 carrying
"Informe año.pdf" intact through both the ASCII and RFC 8187 forms of
Content-Disposition.
Client file sharing, rebuilt from the ground up: a private area per
client, resumable uploads, folders, groups and categories, sharing with
expiry dates and download limits, comments, file versions, an activity
log, a REST API, and sixteen languages.
This repository begins here. ProjectSend 2 was developed privately, and
that development history is not published — the previous generation
remains available, with its own history, at projectsend/legacy.
Free software under the GNU General Public License v2, or (at your
option) any later version.
ignacionelson