WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 19 months ago

Last modified 13 months ago

#43667 closed defect (bug) (fixed)

signup_nonce_check does not use wp_verify_nonce.

Reported by: herregroen Owned by: flixos90
Milestone: 5.1 Priority: normal
Severity: normal Version: 5.1
Component: Login and Registration Keywords: has-patch has-unit-tests
Focuses: multisite Cc:
PR Number:

Description

Currently in multisite setups a nonce check is added to the singup form.

This check does not use the wp_verify_nonce function but instead creates a new nonce and expects an exact match. Due to the nature of wp_nonce_tick this means it's possible to generate nonces that are valid for only a few seconds twice a day.

The error message to try again could also use improvement. Most users will simply click the back button to try again, which will not generate a new nonce but simply restore the old form with the old nonce from browser memory.

Attachments (3)

signup_nonce_check.patch (805 bytes) - added by herregroen 20 months ago.
signup_nonce_check.2.patch (628 bytes) - added by herregroen 20 months ago.
43667.diff (3.6 KB) - added by flixos90 19 months ago.

Download all attachments as: .zip

Change History (13)

#1 @herregroen
20 months ago

I've added a patch which uses the wp_verify_nonce function and expands the error message to include a link to the signup form. Clicking this link will make a new request which will generate a new nonce.

#2 @herregroen
20 months ago

  • Keywords has-patch added

#3 @SergeyBiryukov
20 months ago

  • Milestone changed from Awaiting Review to 5.0

#4 @flixos90
20 months ago

  • Owner set to flixos90
  • Status changed from new to reviewing

#5 follow-up: @flixos90
20 months ago

  • Keywords needs-refresh added

Some thoughts:

  • Do we need to add the link to the registration form? When clicking that, the user will have to re-enter their data. Without a link present, the user would likely hit the browser's back button, still having their data present.
  • It's clear that the above isn't user-friendly in either case. I just noticed that when this error happens, the wp_die() is executed in the HTML content, causing ridiculously invalid markup. Since the method is hooked into the wpmu_validate_blog_signup and wpmu_validate_user_signup filters, both of which pass a $result array containing an errors key which is a WP_Error object, I think we should instead add that message to that WP_Error instance. This should cause it to be printed out in the content correctly, and the process will still fail. In that case, of course a link is no longer necessary anyway.

While the issue described under the second point is not caused by this patch, I think while we fix this one issue, we might as well fix the other as it's clearly broken.

#6 in reply to: ↑ 5 @herregroen
20 months ago

Replying to flixos90:

Some thoughts:

  • Do we need to add the link to the registration form? When clicking that, the user will have to re-enter their data. Without a link present, the user would likely hit the browser's back button, still having their data present.
  • It's clear that the above isn't user-friendly in either case. I just noticed that when this error happens, the wp_die() is executed in the HTML content, causing ridiculously invalid markup. Since the method is hooked into the wpmu_validate_blog_signup and wpmu_validate_user_signup filters, both of which pass a $result array containing an errors key which is a WP_Error object, I think we should instead add that message to that WP_Error instance. This should cause it to be printed out in the content correctly, and the process will still fail. In that case, of course a link is no longer necessary anyway.

While the issue described under the second point is not caused by this patch, I think while we fix this one issue, we might as well fix the other as it's clearly broken.

I added the link explicitly to avoid that behaviour. If the user goes back all his information is indeed still filled in, including the faulty nonce inside a hidden input. Meaning the same error will just occur regardless.

That said, I agree that simply returning an error is clearly the desired option. I'll update the patch to add an error instead. On error the signup form is output again in any case, including a new nonce, so the above is moot.

#7 @herregroen
20 months ago

  • Keywords needs-refresh removed

@flixos90
19 months ago

#8 @flixos90
19 months ago

  • Keywords has-unit-tests added

43667.diff includes 4 unit tests calling wpmu_validate_blog_signup() and wpmu_validate_user_signup(), verifying that the hooked-in function works properly. I also made a minor change, making the error code more verbose with 'invalid_nonce' instead of 'nonce'.

#9 @flixos90
19 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 42976:

Multisite: Verify the signup nonce using wp_verify_nonce() in signup_nonce_check().

Prior to this change, the nonce passed from wp-signup.php was verified with a simple comparison. Furthermore in case of failures, wp_die() would be called right during the HTML markup being already printed. Now the error message is returned properly, modifying the WP_Error object in the passed $result.

Props herregroen.
Fixes #43667.

#10 @flixos90
13 months ago

  • Milestone changed from 5.0 to 5.1
Note: See TracTickets for help on using tickets.