Harden upload security, complete TUS protocol support, upgrade to Quarkus 3.38.0 - #2
Merged
Merged
Conversation
JaCoCo (0.8.15), awaitility (4.3.0) and microprofile-openapi-api (4.1.1) are already at their latest stable releases.
Authorization - Close a concatenation ownership bypass: when the ownership-checked merge refused, the request fell through to mergePartialUploadsUnfinished, which had no ownership check at all. A user could name another user's partial uploads, get a 201, and on the next HEAD have the victim's bytes merged into their own upload and the victim's partials deleted. The unfinished merge now takes a requiredOwnerId (SPI change), and postCreate validates every referenced partial up front instead of relying on a blind fallback. - Enforce per-upload ownership on HEAD, PATCH and DELETE. getUploaderId was populated on create but never consulted, so a known upload ID granted full access to any authenticated user. Denied requests return 404 rather than 403 so they cannot be used to probe for other users' upload IDs. - Stop deriving the auth and rate-limit filter path from quarkus.tus.path. The endpoints are a constant @path, so setting that property left them in place but stopped the filters matching, silently serving TUS unauthenticated. The filters now match the real mount point and a build step rejects any other configured value. Packaging - Drop runtime META-INF/beans.xml. It made Quarkus index the whole runtime jar as a bean archive, defeating every conditional registration in TusProcessor. JAX-RS resources and providers are now indexed explicitly and gated on the same config as their bean registration. - Register TusMetricsService only when Capability.METRICS is present. It references Micrometer types and Micrometer is compileOnly, so consumer apps without that extension failed to load the class. - Remove the hard quarkus-smallrye-health-deployment dependency, which forced its own capability check to always be true. Protocol conformance - Reject PATCH on unfinished final concatenations. isFinalConcat means "merge still pending", so the guard's !isFinalConcat() clause let clients write into a placeholder that finalizeConcatenation later overwrote. - Reject a creation-with-upload body longer than Upload-Length, and stop writeInitialData writing past the declared length; it used to clamp only the recorded offset, leaving trailing bytes and firing a false completion. - Reject Upload-Concat values containing an unresolvable partial reference instead of silently merging the subset that parsed. - Return 400 for malformed or unsupported checksums instead of accepting the chunk unverified or reporting 460, and parse before taking the lock so the new early returns cannot leak it. - Stop advertising checksum-trailer; nothing ever populated the trailer map it claimed to read. - Give merged final uploads an owner, lastActivity and expiry so they are no longer permanent files that no cleanup job touches. Tests go from 143 to 154. TusOwnershipTest uses elytron properties-file basic auth for two real principals, since @testsecurity applies one identity per test method. Three existing tests asserted the old checksum behaviour and were updated.
The TUS core protocol requires the server to interpret X-HTTP-Method-Override as the request method. Without it, clients behind proxies or corporate filters that block PATCH and DELETE cannot use the server at all: their POST with the override header was routed to upload creation instead. TusMethodOverrideFilter is @PreMatching, so the rewritten method drives resource matching and every later filter — including authentication and rate limiting — sees the effective method rather than the one on the wire. The override is scoped to the TUS endpoints so it cannot change how the rest of the application dispatches requests, and the target method must be one of the standard HTTP methods; anything else is rejected with 400 rather than silently routed. Enabled by default per the spec, and disableable via quarkus.tus.method-override-enabled for deployments whose proxy enforces method-based rules. Also extracts the duplicated TUS path check into TusUtils.isTusPath, now shared by the auth, rate-limit and method-override filters. Tests go from 193 to 202: six conformance tests covering PATCH, HEAD, DELETE, case-insensitive values, rejection of unsupported values and the no-header path; one ownership test confirming the override cannot bypass the per-upload authorization check; and two covering the disabled configuration.
The concatenation extension requires HEAD on a final upload to return the
Upload-Concat header "as received in the upload creation request", but the
value was rebuilt from the parsed partial IDs as "final;/tus/{id} ...". A
client that sent absolute URLs or different spacing got back a different
string than it sent.
Both merge methods now take the raw Upload-Concat header and store it
verbatim (SPI change). Rebuilding from the IDs remains only as a fallback for
callers that reach the SPI without a header value.
The existing test only asserted the value starts with "final;", which is why
this went unnoticed; the two new tests assert an exact echo, covering both the
complete-merge and unfinished-merge paths.
…tore The offset was validated before the lock was acquired, so two requests could both pass validation and then write in turn. With chunks of different sizes the second write overwrote part of the first while both returned 204, leaving a silently corrupted upload that no checksum could detect: each chunk is validated in isolation. A client retrying an in-flight PATCH was enough to trigger it, no attacker required. The lock is now taken before any state-dependent validation, and the offset, deferred length and content-length checks all run under it. Lock ownership passes to the async write pipeline, whose eventually() releases it; every earlier exit releases in a finally. The store no longer trusts the caller's offset either. writeChunkAsync compares it to the upload's current offset and fails with the new spi.OffsetMismatchException, which the resource maps to 409 carrying the real Upload-Offset so the client can resume. This matters for third-party stores, which would otherwise inherit the assumption that the caller got it right. Cheap stateless rejections (chunk size, malformed or unsupported checksum) still happen before the lock so a bad request never contends for it. testConcurrentPatchOnSameUpload asserted that all four losing requests get 423. That only held because validation preceded locking; a loser that acquires the lock after the winner finishes now correctly sees a moved offset and gets 409, which is more useful since it carries the offset to resume from. The test now asserts the real invariant — exactly one winner, losers rejected with 423 or 409 — and additionally that the stored bytes are one writer's chunk rather than a blend, which is the corruption this fix prevents.
discardUpload removed the entry, deleted the data file and dropped the lock unconditionally, without holding it. A DELETE or a cleanup job landing while a PATCH was mid-write therefore unlinked the file underneath the in-flight write, which continued against the now-unlinked fd and then persisted metadata for an upload whose data file was gone — recreating a .meta that only a restart would reap, and potentially firing TusUploadCompletedEvent for a file that no longer existed. The client got 204 with an offset for bytes that were never durably stored. discardUpload now acquires the lock and returns false if it cannot, leaving the upload untouched. DELETE answers 423 when an upload exists but is being written; deleting something that never existed stays idempotent at 204. The merge and finalization paths already hold every partial's lock, so they use a private discardLockedUpload — acquisition is not reentrant, and routing them through the locking variant makes them silently fail to delete the partials, which the existing concatenation tests catch. Cleanup jobs inherit the same protection for free, skipping an in-use upload and retrying on their next run. They now also return and log only the uploads actually removed, rather than counting skipped ones as cleaned. writeChunkAsync additionally drops its result if the upload was discarded while the write was in flight, so a discard that slips through some other path cannot resurrect metadata or fire a completion event for a deleted upload.
The throttle keyed anonymous clients on the X-Forwarded-For header, which the client controls. Varying it per request bought a fresh burst allowance every time, making the limiter a no-op for anyone who bothered to set it, and each distinct value also inserted a bucket into a map only swept after an hour of idleness — so the bypass doubled as a memory-growth vector. With no header at all, every anonymous client instead shared one "anonymous" bucket and could lock the others out. Clients are now identified by the authenticated principal, falling back to the peer address of the connection. Deployments behind a reverse proxy should set quarkus.http.proxy.proxy-address-forwarding (with trusted-proxies): Quarkus verifies the immediate peer is a trusted proxy before resolving the forwarded address into the request's remote address, so the throttle applies per forwarded client without trusting arbitrary senders. TusRateLimitTest depended on X-Forwarded-For to isolate its buckets, which is the behaviour being removed; it now enables proxy-address-forwarding and so covers the behind-a-proxy deployment. The new TusRateLimitSpoofingTest covers the untrusted case: distinct forged values must not escape the burst limit. It is deliberately a single test, since every request in that profile shares the one bucket keyed by the peer address. Rate limiting was undocumented; configuration.adoc now covers the three properties and the proxy requirement.
Reaching the final offset was treated as the completion trigger, but a PATCH at the final offset with an empty body satisfies every check on the way there: the offset matches, nothing exceeds the declared length, the zero-length checksum guard skips validation, and a zero-byte write succeeds. Each such request fired TusUploadCompletedEvent again. Observers of that event typically move the file to permanent storage, insert a row, run a virus scan, call a webhook or bill for the upload, so any client could replay all of it indefinitely, with no attacker sophistication required. Completion is now latched on UploadInfo: markCompletionFired() returns true only for the caller that makes the transition, and every firing site — the chunk write, creation-with-upload, and the in-memory reference store — goes through it. The latch is persisted in the .meta sidecar so a restart cannot grant a completed upload one more event; metadata written before the field existed defaults to unfired, which risks one extra event rather than suppressing a genuine one. Zero-length uploads are the reason this is a latch rather than a rejection of PATCH on a complete upload: their offset already equals the declared length, so an empty PATCH is their only chance to complete, and it must still work exactly once.
Neither stream checked who was asking. Both reveal an upload's declared size and live byte counts, and the progress endpoint did not even validate that the path segment was a UUID, so any string became a map key. Worse, registering a sink overwrote the map entry: subscribing to another user's upload silently displaced the legitimate subscriber, whose connection was then unreachable and never closed. Neither map is swept, so subscribing to attacker-chosen keys grew them without limit. Both endpoints now require the upload to exist and, when auth is enabled, to belong to the caller, answering 404 in every other case so a denial cannot be distinguished from a missing upload. The progress endpoint validates the ID format like the event stream already did, which also bounds the maps to real uploads. The ownership rule moves to TusUploadAuthorizer, shared with TusUploadResource rather than duplicated — three endpoints deciding access separately is how such rules drift apart. Registration also closes any sink it displaces, so a reconnecting client replaces its own stream instead of leaking the previous connection.
Each reference in an Upload-Concat: final header was summed and copied separately, so repeating one partial inflated it into a file many times its size — a 10 MB partial referenced 200 times produced 2 GB from a single small request, well under the default max-size and so passing every existing check. The only real ceiling was the HTTP header size limit. Repeating a partial is now rejected outright rather than de-duplicated: the client asked for something that cannot be honoured, and silently merging a different set than requested is how the earlier invalid-reference bug behaved. The complete-partial path happened to already fail, because merging locks every referenced partial and locking the same one twice does not succeed — but resting a size guarantee on lock non-reentrancy is fragile, and the deferred path took no locks and had nothing protecting it: it declared a final length of the summed duplicates, claiming more bytes than were ever uploaded. That path is what the new test for incomplete partials covers; the complete-partial test passes today and pins the behaviour so it survives the reasoning above. Also caps how many partials one merge may reference, via quarkus.tus.max-concat-parts (default 1000, comfortably above what the header size limit allows in practice).
The core protocol requires the header on every response except OPTIONS. Resource methods set it themselves, but responses produced by the container never reach them — a media-type mismatch on PATCH, an unsupported method — so those went out without it. A global response filter fills only the gaps, leaving a header a resource already set untouched since it may legitimately differ from the configured version. A request to a path matching no route at all is still answered by the router before JAX-RS sees it, and remains outside this guarantee.
The checksum extension required Upload-Checksum as a header, which forces a client streaming a large chunk to buffer the whole thing just to hash it before sending. The checksum-trailer extension exists precisely to avoid that, letting the value arrive after the body. The body is fully read by the time the resource method runs, so a trailer is available there. PATCH now falls back to the trailer when the header is absent, and the same validation applies to both: a malformed value or an unsupported algorithm is a 400, a mismatch is a 460 with the chunk discarded. This depends on vertx-core being able to read request trailers, which stock Vert.x cannot (eclipse-vertx/vert.x#5253). The build forces a vertx-core built from the request-trailers branch; the extension is advertised only because of that, and the documentation says so. The tests write the chunked request onto a socket directly, since RestAssured cannot send trailers.
The checksum-trailer extension needs a vertx-core that can read HTTP request trailers, which stock Vert.x cannot, so the build forces a 4.5.32-SNAPSHOT resolved from the local Maven repository. CI has no such repository and would fail to resolve it. Both jobs now clone the request-trailers-4.x branch and install it. The result is cached against the branch's head revision, so the Maven build only runs when that branch moves rather than on every push. The whole block is marked temporary: it goes away, along with the force in build.gradle and the warning in configuration.adoc, once the change ships upstream in Vert.x.
It only ran on push to master, so a change that broke the native build would surface after merging rather than on the pull request. That is a poor trade for this extension in particular: native image depends on how beans and JAX-RS classes are registered, and the runtime module registers them explicitly because it ships no beans.xml.
nativeIntegrationTest set quarkus.native.enabled as a test system property, which configures the test JVM but not the Quarkus build. quarkusBuild therefore produced a JVM application and the task reported success in seconds without ever invoking native-image, so the job has never verified anything. The property now goes on the Gradle invocation so quarkusBuild produces a native binary, and the task fails with an explanatory message if no runner executable is present, so this cannot silently pass again.
Quarkus refuses to emit both a native image and a JAR from one build, so quarkus.native.enabled alone fails dependency resolution for quarkusBuild.
checkout, cache, setup-java, upload-artifact and setup-gradle all targeted Node 20, which GitHub now force-runs on Node 24 while warning that the compatibility shim will go away. checkout v4 to v7, setup-java v4 to v5, setup-gradle v4 to v6, cache v4 to v6, upload-artifact v4 to v7. graalvm/setup-graalvm stays on v1, which still tracks its current release.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upgrades the build, closes every finding from a four-dimension review of the upgraded tree, and completes TUS protocol support. 13 commits, each independently reviewable. Tests go from 143 to 229.
1. Upgrade
Quarkus 3.36.2 → 3.38.0, Gradle wrapper 9.5.1 → 9.6.1 (with its pinned SHA-256 updated). JaCoCo, awaitility and microprofile-openapi-api were already current.
2. Authorization
Concatenation ownership bypass. When the ownership-checked merge refused,
postCreatefell through tomergePartialUploadsUnfinished, which had no ownership check at all. An authenticated user could name another user's partial upload IDs, get a 201, and on the nextHEADhave the victim's bytes merged into their own upload while the victim's partials were deleted. Both merge methods now take arequiredOwnerId, and every referenced partial is validated up front rather than via a blind fallback.Missing per-upload authorization (IDOR).
getUploaderIdwas populated on create but never consulted, so a known upload ID granted any authenticated user full read/write/delete.HEAD,PATCH,DELETEand the SSE streams now enforce ownership through a sharedTusUploadAuthorizer, answering404rather than403so responses can't be used to probe for other users' IDs.quarkus.tus.pathcould silently disable authentication. The endpoints are a constant@Path, but the auth and rate-limit filters gated on the configured value — so changing it left the endpoints live while the filters stopped matching them. Filters now match the real mount point, and a build step rejects any other value.3. Concurrency
PATCH offset TOCTOU. The offset was validated before the lock was acquired, so two requests could both pass validation and then write in turn. With differently sized chunks the second overwrote part of the first while both returned 204 — silent corruption no checksum could catch, reachable by a client retrying an in-flight PATCH. Validation now happens under the lock, and the store rejects a write at a stale offset with
409instead of trusting the caller.discardUploadignored the lock. A DELETE or cleanup job landing mid-write unlinked the file underneath the in-flight write, which then reported success for bytes no longer stored. It now acquires the lock and returnsfalseif it can't; DELETE answers423while a write is in progress.4. Abuse resistance
X-Forwarded-For— a client varying it per request got a fresh burst every time, making the limiter a no-op and growing an unbounded bucket map. Clients are now keyed on principal, else the peer address; deployments behind a proxy setquarkus.http.proxy.proxy-address-forwarding.TusUploadCompletedEventevery time, letting any client replay whatever observers do (move files, insert rows, call webhooks, bill). Completion is now latched and the latch is persisted.quarkus.tus.max-concat-partscaps the count.5. Packaging — affects consumers of the published extension
runtime/.../META-INF/beans.xml. It made Quarkus index the whole runtime jar, defeating every conditionalAdditionalBeanBuildItem. Resources and providers are now registered explicitly viaAdditionalIndexedClassesBuildItem, gated on the same config, so the conditionals are real.TusMetricsServicegated onCapability.METRICS— it references Micrometer types while Micrometer iscompileOnly, so consumers without that extension failed on first upload.quarkus-smallrye-health-deploymentdependency, which forced its own capability check to always be true.6. Protocol conformance — now complete
X-HTTP-Method-Override(a core MUST, previously absent),Upload-Concatechoed verbatim as received,Tus-Resumableguaranteed on container-generated responses, PATCH rejected on unfinished final concatenations, creation-with-upload bodies bounded byUpload-Length, invalid concat references rejected instead of silently dropped, and malformed/unsupported checksums returning400instead of being accepted unverified.All nine TUS extensions are now supported, including
checksum-trailer— see the caveat below.Caveat: patched Vert.x required
checksum-trailerneeds avertx-corethat can read HTTP request trailers, which stock Vert.x cannot (eclipse-vertx/vert.x#5253, PR #6278). The build forces a4.5.32-SNAPSHOTbuilt from that change, and CI clones and installs it (cached against the branch revision). A local build needsmvn installin that checkout first. Once the change ships upstream, remove the force inbuild.gradle, the CI block, and the warning inconfiguration.adoc.Testing
143 → 229 tests. New tests were checked against the old behaviour rather than assumed: the ownership, TOCTOU, discard-lock, XFF, completion-latch, SSE and trailer tests each fail when the corresponding fix is reverted. Three existing tests encoded non-conformant checksum behaviour and were updated;
testConcurrentPatchOnSameUploadasserted an implementation detail (losers always get 423) and now asserts the real invariant plus data integrity.Native image is not covered — that job only runs on push to
master, and this branch changed exactly the machinery native builds are sensitive to (beans.xmlremoval, explicit indexing, four new beans/providers). Worth verifying before or immediately after merge.