Review request

Vinyl marketplace, anyone signed in can edit anyone else's listing

Solved · 279 viewsasked last month by

Repo or live app

git.example.com/kaitanaka/crate-digger ↗

Unsure about: Security

Small marketplace for record collectors, about 60 sellers. A friend testing it noticed he could change the price on someone else's listing by editing the id in the request. RLS is on for listings and I do have an update policy, so I expected that to be blocked. Clearly I have misunderstood what the policy does.

Comment

2 answers

Marked as helpful by the asker

mira_devlast month

You almost certainly wrote the update policy with only using and no with check. That is the single most common RLS mistake.

using decides which rows you are allowed to update. with check decides what the row is allowed to look like afterwards. With only using, a seller can take their own row and rewrite seller_id to someone else, or in a policy written as using (true) touch anything at all. Write both halves:

create policy "own listings" on listings for update
  using (seller_id = auth.uid())
  with check (seller_id = auth.uid());

Check your insert policy too, since insert only has with check, and a missing one there lets a user create a listing already owned by someone else.

Comment

Added with check. My friend can't change prices anymore.

kai_makes last month

Also stop sellers from changing seller_id itself (a trigger or column-level grant), otherwise they can move a listing to someone else. jb_supa last month

dmitri_vlast month

Worth checking what Postgres actually has now, rather than the migration you think ran. After Mira's fix I'd look at all policies on the table:

Update is fixed. The other two aren't: insert with with check (true) lets anyone create a listing under another seller's id, and delete with using (true) lets any signed-in user delete any listing. The delete one is worse.

alter policy "sellers insert" on listings with check (seller_id = auth.uid());
alter policy "sellers delete" on listings using (seller_id = auth.uid());

For jb_supa's point about seller_id, the simplest version is a column grant, so no policy can ever let someone move a listing:

revoke update on listings from authenticated;
grant update (price, title, description, condition) on listings to authenticated;

Comment

The delete one. I never tested it because there's no delete button on other people's listings. Fixed both, and the column grant is in.

kai_makes last month

The button is never the control. Anyone with the anon key and a login can call the API directly. dmitri_v last month

Keyboard shortcuts

Vinyl marketplace, anyone signed in can edit anyone else's listing

Repo or live app

2 answers

Your answer

Vinyl marketplace, anyone signed in can edit anyone else's listing

Repo or live app

2 answers

Related

Your answer