fix: Prevent race condition on unique-submission forms #1841

susnux · 2023-12-21T12:57:19Z

Currently a series of requests can be crafted so that multiple submissions of the same user are inserted.
As the unique property of a submission is only checked once so that if multiple requests arrive at the server right in the same time they will all be inserted.

To prevent this the time window is minimized by placing the check right before the insert and check afterwards if the submission really is unique.

codecov · 2023-12-21T13:12:44Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (f24a595) 44.62% compared to head (655762a) 44.61%.
Report is 6 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1841      +/-   ##
============================================
- Coverage     44.62%   44.61%   -0.01%     
- Complexity      658      662       +4     
============================================
  Files            58       58              
  Lines          2593     2600       +7     
============================================
+ Hits           1157     1160       +3     
- Misses         1436     1440       +4

Chartman123 · 2023-12-21T13:24:47Z

@susnux Looks good :) Just a little question: You check all submissions against the new submission for uniqueness. If we have a form with lots of submissions, isn't this very resource consuming?

susnux · 2023-12-21T13:36:07Z

@Chartman123 I pushed a performance fix, what do you think?

Chartman123 · 2023-12-21T13:39:53Z

Yes, something like this was on my mind 👍🏻

nfebe · 2023-12-21T18:04:53Z

lib/Controller/ApiController.php

 		// Insert new submission
 		$this->submissionMapper->insert($submission);
 		$submissionId = $submission->getId();

+		// Ensure the form is unique if needed.
+		// If we can not submit anymore then the submission must be unique
+		if (!$this->formsService->canSubmit($form) && !$this->submissionService->isUniqueSubmission($submission)) {


Suggested change

if (!$this->formsService->canSubmit($form) && !$this->submissionService->isUniqueSubmission($submission)) {

if (!$this->formsService->canSubmit($form) && !$this->submissionService->isUniqueSubmission($submission)) {

Since the first part of this condition already exists up exactly and throws an exception, if this exception exists this part of the code would never be reached?

Or does canSubmit result change after the first insertion?

This is a calculated form prop https://github.com/nextcloud/forms/blob/main/lib%2FService%2FFormsService.php#L271

The idea is here if the user still can submit after insert then multiple submissions are allowed. If not submissions have to be unique and we need to verify the answer is the only one.

While writing this: Is this not a (unrelated to this PR) bug? If unique submissions are enabled why should the owner be allowed to submit multiple?

why should the owner be allowed to submit multiple?

Good question, the only reason I could think of: The owner fills the form for someone else who can't access the online form for some reason but needs to?

This part of the code was added by @jotoeri 2 years ago. So let's ask him what was his intention :)

Hmm, i'm not 100% sure, but i think we shortly discussed that somewhere that once. Otherwise that change wouldn't have gone through just like that.^^
Anyways: I would interpret the Owner as some kind of an administrator for the form, being allowed to do everything with it. The owner could also just remove the submitOnce, submit and then re-set it, which is just annoying handling. So why should we block the owner of doing whatever he/she wants to do with the form?
In fact it's mostly a UX-Question wrt. the owner, no?

@fenn-cs do the comments help you to finish your review?

nfebe

Left and inline remark/question to help me review.

Chartman123 · 2023-12-26T23:17:04Z

by placing the check right before the insert

@susnux One thing that just came to my mind: by moving the check right before the insert, we would check for valid submission data even if a user were no longer allowed to submit. This creates some unnecessary checks in that case.

susnux · 2024-01-04T14:47:55Z

@susnux One thing that just came to my mind: by moving the check right before the insert, we would check for valid submission data even if a user were no longer allowed to submit. This creates some unnecessary checks in that case.

Well this only happens when using API and not our frontend as the frontend already checks that. Meaning only API usage from external application will cause this.
But yes both cases are not triggered by our frontend but only by (malicious) external usage, so we can discuss both ways.

If we keep the order of the checks the time frame for the attack is a bit wider but as we sanitize afterwards it would be reverted anyways.

Chartman123 · 2024-01-05T11:12:09Z

For me it's ok to keep it the way you implemented it currently 👍🏻

Chartman123

Just squash the fixup commit, then we can merge it from my point of view :)

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

susnux · 2024-01-08T20:29:37Z

Just squash the fixup commit, then we can merge it from my point of view :)

Done

susnux · 2024-01-08T20:29:48Z

/backport to stable3

susnux added bug Something isn't working 3. to review Waiting for reviews feature: 📝 submitting responses labels Dec 21, 2023

susnux requested a review from Chartman123 December 21, 2023 12:57

susnux force-pushed the fix/race-condition branch 2 times, most recently from a44fcdd to 81e7ba9 Compare December 21, 2023 13:09

susnux force-pushed the fix/race-condition branch from 724e1b6 to 7b7b92b Compare December 21, 2023 13:39

susnux requested review from nickvergessen and nfebe December 21, 2023 14:04

nfebe reviewed Dec 21, 2023

View reviewed changes

Chartman123 approved these changes Jan 5, 2024

View reviewed changes

Chartman123 added this to the 4.1 milestone Jan 8, 2024

fix: Prevent race condition on unique-submission forms

655762a

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

susnux force-pushed the fix/race-condition branch from 7b7b92b to 655762a Compare January 8, 2024 20:29

backportbot-nextcloud bot added the backport-request Pending backport by the backport-bot label Jan 8, 2024

susnux merged commit 9a7d696 into main Jan 9, 2024
45 of 47 checks passed

susnux deleted the fix/race-condition branch January 9, 2024 17:13

backportbot-nextcloud bot mentioned this pull request Jan 9, 2024

[stable3] fix: Prevent race condition on unique-submission forms #1870

Merged

backportbot-nextcloud bot removed the backport-request Pending backport by the backport-bot label Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Prevent race condition on unique-submission forms #1841

fix: Prevent race condition on unique-submission forms #1841

susnux commented Dec 21, 2023 •

edited

Loading

codecov bot commented Dec 21, 2023 •

edited

Loading

Chartman123 commented Dec 21, 2023

susnux commented Dec 21, 2023

Chartman123 commented Dec 21, 2023

nfebe Dec 21, 2023

Chartman123 Dec 21, 2023

susnux Dec 21, 2023 •

edited

Loading

Chartman123 Dec 22, 2023

jotoeri Dec 23, 2023 •

edited

Loading

Chartman123 Jan 5, 2024

nfebe left a comment

Chartman123 commented Dec 26, 2023

susnux commented Jan 4, 2024

Chartman123 commented Jan 5, 2024

Chartman123 left a comment

susnux commented Jan 8, 2024

susnux commented Jan 8, 2024

	if (!$this->formsService->canSubmit($form) && !$this->submissionService->isUniqueSubmission($submission)) {
	if (!$this->formsService->canSubmit($form) && !$this->submissionService->isUniqueSubmission($submission)) {

fix: Prevent race condition on unique-submission forms #1841

fix: Prevent race condition on unique-submission forms #1841

Conversation

susnux commented Dec 21, 2023 • edited Loading

codecov bot commented Dec 21, 2023 • edited Loading

Codecov Report

Chartman123 commented Dec 21, 2023

susnux commented Dec 21, 2023

Chartman123 commented Dec 21, 2023

nfebe Dec 21, 2023

Choose a reason for hiding this comment

Chartman123 Dec 21, 2023

Choose a reason for hiding this comment

susnux Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Chartman123 Dec 22, 2023

Choose a reason for hiding this comment

jotoeri Dec 23, 2023 • edited Loading

Choose a reason for hiding this comment

Chartman123 Jan 5, 2024

Choose a reason for hiding this comment

nfebe left a comment

Choose a reason for hiding this comment

Chartman123 commented Dec 26, 2023

susnux commented Jan 4, 2024

Chartman123 commented Jan 5, 2024

Chartman123 left a comment

Choose a reason for hiding this comment

susnux commented Jan 8, 2024

susnux commented Jan 8, 2024

susnux commented Dec 21, 2023 •

edited

Loading

codecov bot commented Dec 21, 2023 •

edited

Loading

susnux Dec 21, 2023 •

edited

Loading

jotoeri Dec 23, 2023 •

edited

Loading