Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#43667 closed defect (bug) (fixed)

signup_nonce_check does not use wp_verify_nonce.

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

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 7 years ago.
signup_nonce_check.2.patch (628 bytes) - added by herregroen 7 years ago.
43667.diff (3.6 KB) - added by flixos90 7 years ago.

Download all attachments as: .zip

Change History (13)

#1 @herregroen
7 years 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
7 years ago

  • Keywords has-patch added

#3 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 5.0

#4 @flixos90
7 years ago

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

#5 follow-up: @flixos90
7 years 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
7 years 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
7 years ago

  • Keywords needs-refresh removed

@flixos90
7 years ago

#8 @flixos90
7 years 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
7 years 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
7 years ago

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