#43667 closed defect (bug) (fixed)
signup_nonce_check does not use wp_verify_nonce.
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (13)
#5
follow-up:
↓ 6
@
5 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 thewpmu_validate_blog_signup
andwpmu_validate_user_signup
filters, both of which pass a$result
array containing anerrors
key which is aWP_Error
object, I think we should instead add that message to thatWP_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
@
5 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 thewpmu_validate_blog_signup
andwpmu_validate_user_signup
filters, both of which pass a$result
array containing anerrors
key which is aWP_Error
object, I think we should instead add that message to thatWP_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.
#8
@
5 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'.
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.