spiral-project / ihatemoney

A simple shared budget manager web application
https://ihatemoney.org
Other
1.21k stars 270 forks source link

Change settle endpoint to use POST instead of GET #1303

Open zorun opened 8 months ago

zorun commented 8 months ago

This is necessary to avoid XSS. State-changing actions should never be done with a GET.

zorun commented 8 months ago

@TomRoussel FYI, if you are interested to review. There were two security issues:

Tests are failing on purpose because I added a test for this second issue. I'm not yet sure how to fix it, I would prefer if we reuse existing checks (they are done in get_billform_for()). Reusing the Bill form would be best, but we can't do that directly, we need hidden fields. Maybe we could use a custom form but reuse the existing bill creation endpoint. If we don't find anything good, we can just duplicate the validation rules but I don't like it.

You should not feel bad about it, we're quite happy to finally have this feature thanks to your dedication. Let's fix this together :)

TomRoussel commented 8 months ago

Sure, I'm happy to help get this in a better state! I'll have a look at this later this week.

Intuitively, it feels like it should be possible to think of something that reuses the existing checks for this usecase as well. There's no scheme that immediately pops in my head, but to be honest my experience with WTForms is limited to this project.

TomRoussel commented 7 months ago

No comments on the proposed changes. About the second issue: could we create a BillForm object using the information inside the SettlementForm with get_billform_for()? Basically just add the the payer and payed_for fields as keyword arguments to that function, which can then do all the validation checks. Since it validates if the payer and payed_for are both in the project, that should solve this problem neatly without duplicating any logic.