jhoang304 / jhoang304/Whelp

Image URLs are treated as S3 deletion authority

Open
#58 1 comment 0 reactions 0 assignees View on GitHub
bug
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.