Derive file S3 location server-side on upload - #150
Merged
wasimxyz merged 4 commits intoJul 16, 2026
Merged
Conversation
The PATCH /api/v1/files/:fileId endpoint accepted arbitrary s3_bucket and
s3_key values, letting any files:update caller repoint a record at any S3
object. Downstream download redirects and Lambda reprocessing trusted those
DB values, enabling data tampering and unauthorized object access.
The watcher only ever echoed back the canonical bucket/key that
request-upload-url already computed, so these fields carried no new
information. Drop them from patchFileBody and rebuild the canonical
{instrumentId}/{runId}/{filename} key from trusted DB state on the uploaded
transition instead of validating client input.
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
wasimxyz
changed the title
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
The upload PATCH no longer sends s3_bucket / s3_key now that the server derives the canonical S3 location itself; the watcher only reports status and content type. Bump the watcher to 0.5.1. Co-authored-by: Cursor <cursoragent@cursor.com>
wasimxyz
deleted the
wasimamiri/eng-1450-arbitrary-s3-location-injection-via-file-patch-enables-data
branch
July 16, 2026 17:28
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.
Summary
The
PATCH /api/v1/files/:fileIdendpoint accepted arbitrarys3_bucket/s3_keyvalues with no validation. Anyfiles:updatecaller (which includes every browser-authenticated user via wildcard session scopes) could repoint a file record at any S3 object. Downstream download redirects and Lambda reprocessing trust those DB values, enabling silent data tampering and unauthorized object access.Rather than validate client input against an allowlist/prefix, this removes client control entirely:
patchFileBodyno longer acceptss3_bucket/s3_key(unknown keys are dropped by the Zod schema; the OpenAPI spec regenerates automatically).→ uploadedtransition the handler derives the canonical{instrumentId}/{runId}/{filename}key from trusted DB state and sets the bucket fromgetS3RawDataBucket().This is safe because the watcher only ever echoed back the same bucket/key that
request-upload-urlalready computed server-side (watcher/src/data_hub_watcher/uploader.py), so these fields carried no new information. Wire behavior is unchanged for legitimate callers; the value derived is byte-for-byte identical.Out of scope (follow-ups)
POST .../runs/:runId/files) still accepts caller-specifieds3_bucket/s3_key— it legitimately registers processed artifacts under non-raw prefixes, so it would need its own allowlist rather than the canonical-key treatment.Test plan
make check-all(ruff, pyright, ultracite, tsc) — cleandetected → uploadedtest to send a hostiles3_bucket/s3_keypayload and assert the record still lands on the derived canonical location (ENG-1450 regression)Made with Cursor