Image URLs are treated as S3 deletion authority
- Dominant language
- TypeScript
- Stars
- 0
- Forks
- 0
- Avg merge
- 2h 19m
- Merged PRs (30d)
- 30
Description
Raised by @greptile during review of #55, where it surfaced on the new bulk
delete. The root cause is older than that PR and reachable through three
separate routes, so it wants its own fix.
## The problem
An image row stores **a URL the caller typed**, not a key this app minted.
`RestaurantImageForm.url` is a bare `StringField(validators=[DataRequired()])`,
and nothing checks that the caller uploaded the object the URL points at — or
that they own the restaurant they are attaching it to.
Separately, every S3 cleanup path turns that stored URL straight back into an
object key (`aws_helpers.is_s3_url` + prefix strip) and deletes it. So a URL
someone typed becomes authority to delete the object it names.
Because uploads land in one shared bucket and are publicly readable, any
object's URL is discoverable.
## Three ways in
1. **Profile picture — two calls, no restaurant needed.**
`PUT /api/users/profile` with `profile_image_url` set to a stranger's S3
URL, then again with `""`. `app/api/user_routes.py:104-109` deletes the
*old* URL's object on change, so the second call removes their picture.
2. **Restaurant image delete.** Attach any URL to any restaurant
(`create_restaurant_image` does not check ownership), then
`DELETE /api/restaurant-images/` → `remove_file_from_s3(image.url)`
(`app/api/restaurant_image_routes.py`).
3. **Restaurant delete.** Same attach, then delete the restaurant. #55 added
this path and mitigated it there — it now skips URLs another
`restaurant_images` or `review_images` row still references — but that is a
band-aid, not a fix, and routes 1 and 2 have no such guard.
Review images (`create_image_by_review_id`) take a free-text URL the same way.
## Suggested fix
Stop deriving the key from the URL. Roughly:
- Give the image tables (and `users.profile_image_url`'s companion) an
app-controlled `s3_key` column, written **only** by `/api/images/upload` for
the user who uploaded it. Needs a migration; existing rows can backfill from
the URL where `is_s3_url` matches, since those were genuinely ours.
- Delete by `s3_key`, never by a URL read back out of a row. A row whose
`s3_key` is null is an external/hot-linked image and is never deleted.
- Check ownership on attach: `create_restaurant_image` should require the
caller to own the restaurant, or at minimum refuse a URL whose `s3_key` is
not one they uploaded.
## Notes
- Nothing here is exploitable for data *disclosure* — objects are already
public. The impact is destruction of other users' images.
- If you would rather not have this described in a public issue, it converts
cleanly to a private security advisory (Security → Advisories → New draft)
and I can delete this one.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by tracing the three cleanup routes in app/api/user_routes.py:104-109 and app/api/restaurant_image_routes.py, then inspect create_restaurant_image, create_image_by_review_id, /api/images/upload, and the image models. Define the migration and ownership behavior before changing the deletion paths. Done means caller-supplied URLs cannot authorize S3 deletion, external images are retained, and owned uploads remain removable through all three routes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- aws, python
- Domain
- api, backend, database, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100