Opened 5 years ago
Last modified 2 years ago
#46748 accepted defect (bug)
authenticate filter hook does not behave as expected for priority values less than 20
Reported by: | robert.peake | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 3.7 |
Component: | Login and Registration | Keywords: | has-unit-tests early needs-patch dev-feedback |
Focuses: | Cc: |
Description
Returning null or a WP_Error object from functions bound to the authenticate filter at priority values less than 20 does not prohibit a user from logging in.
Consider the following snippet:
<?php <?php /* Plugin Name: Prohibit Login Description: Proves that the authenticate filter does not work as expected with lower priority Author: Robert Peake Version: 0.1 */ function prohibit_login($user_or_email, $username = null, $password = null) { return new WP_Error('authentication_failed','Prohibit Login plugin prohibited login'); } add_filter('authenticate', 'prohibit_login', 20, 3);
This code when activated as a plugin prohibits an admin user from logging in using wp-login.php and displays the message "Prohibit Login plugin prohibited login" as expected.
Changing the value from 20 to e.g. 19 on the final line does not prohibit an admin user from logging in using wp-login.php. No message is displayed, and the login proceeds.
(Note: this has been tested with all other plugins deactivated with the Twentynineteen theme using the latest nightly build.)
While this is not a security problem in itself, because it is undocumented behaviour it could lead to security issues in plugins where an author assumes that, like other filter hooks, e.g. the default priority of 10 can be explicitly stated without side-effects.
For this reason, I initially submitted a ticket on the HackerOne platform just to be sure, but the ticket was closed as being a "hypothetical" vulnerability with out a "clear PoC", so I am filing this bug report instead.
Attachments (2)
Change History (22)
#2
in reply to:
↑ 1
@
5 years ago
- Version set to trunk
Replying to swissspidy:
wp_authenticate_username_password
runs on priority 20, so I guess this happens because the function does not immediately return$user
if it's aWP_Error
.
That makes sense.
Since most other filter and action hooks work as expected more-or-less irrespective of priority in relation to core functionality, I would suggest at minimum that the documentation be updated to reflect the need for a priority of 20+ in order to prevent logins. As you know, the default is 10.
#3
@
5 years ago
In the longer term, could wp_authenticate_username_password
have its priority lowered (and all other auth-specific core functionality that needs to fire before it be likewise lowered) so that hooking into authenticate
with the default priority value of 10 behaves as expected?
Could a proof-of-concept conceivably be created as a plugin to demonstrate this functionality? Or is it too intrinsic to core?
#7
@
4 years ago
- Keywords has-patch added; needs-patch removed
- Milestone changed from Awaiting Review to 5.8
- Owner set to SergeyBiryukov
- Status changed from new to accepted
This ticket was mentioned in Slack in #core by chaion07. View the logs.
3 years ago
#9
@
3 years ago
Hi, I tested the patch. It still applies against trunk and it works fine in my testing: it returns $user
/$input_user
when the result is a WP_Error
object.
I'm not sure about which unit tests we should add here, though.
#10
@
3 years ago
- Milestone changed from 5.8 to 5.9
Thank you @audrasjb!
Today is 5.8 Beta 1. As this one needs tests, punting to 5.9 to continue progress towards resolution.
This ticket was mentioned in PR #1903 on WordPress/wordpress-develop by costdev.
3 years ago
#11
- Keywords has-unit-tests added; needs-unit-tests removed
This PR:
- Adds the changes from the existing patch on the ticket.
- Adds unit tests for
wp_authenticate_username_password()
with 100% line coverage.
Trac ticket: https://core.trac.wordpress.org/ticket/46748
@
3 years ago
Test Report: Confirmed reproducible. Apply PR 1903 and confirmed the filter less than 20 priority now works ✅
#12
@
3 years ago
The whole authentication stack is pretty janky to be honest. I think this would be a good candidate for an early
ticket.
#13
@
3 years ago
- Keywords early added
- Milestone changed from 5.9 to 6.0
As per the comment above, moving to the next milestone with early
keyword so it could get a proper review.
#14
@
3 years ago
- Keywords needs-patch added; has-patch removed
Hold on. In testing further, 46748.diff blocks email login in wp_authenticate_email_password()
. The PR adds an is_email()
check. This needs further investigation to determine if the changes are appropriate or if something else is going on. I agree with @TimothyBlynJacobs to punt this to 6.0. Give it time for more discussion and investigation.
Marking as needs-patch
as the current patches do not fully resolve the reported issue: i.e. works for username login, but not email login.
This ticket was mentioned in Slack in #core by costdev. View the logs.
2 years ago
#16
@
2 years ago
- Keywords dev-feedback added
Per the discussion in the bug scrub, I'm adding dev-feedback
to this ticket to clarify its current state.
@SergeyBiryukov has kindly offered to take a look at the issue to help move this one forward. Everyone reading this ticket is encouraged to take a look through the discussion above and this discussion on the PR. All input and feedback is welcome to help us land this in 6.0.
This ticket was mentioned in Slack in #core by costdev. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by chaion07. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by chaion07. View the logs.
2 years ago
#20
@
2 years ago
- Milestone changed from 6.0 to Future Release
Thanks @robertpeake for reporting this. We recently reviewed this ticket during a bug-scrub session. Due to the lack of discussion and the progress we are moving it to Future Release.
Props to @costdev & @peterwilsoncc
Cheers!
wp_authenticate_username_password
runs on priority 20, so I guess this happens because the function does not immediately return$user
if it's aWP_Error
.