Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change settle endpoint to use POST instead of GET #1303

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

zorun
Copy link
Collaborator

@zorun zorun commented Mar 31, 2024

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

@zorun
Copy link
Collaborator Author

zorun commented Mar 31, 2024

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

  • changing state with a GET, this is a bad idea in general. You could easily create an URL with a fake settlement and make somebody click on it to trick them into creating a wrong bill. This PR adresses that.
  • insufficient validation of payer and payee, allowing to create a bill involving members of another project. This is caused by a weakness in the Ihatemoney data model, same as GHSA-67j9-c52g-w2q9, and must be prevented with appropriate validation (and unit tests).

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
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants