Issue-49: Add nonce to wp-login #56
Conversation
WalkthroughThe AlleyVate WordPress plugin has introduced a new Changes
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/alley/wp/alleyvate/features/class-login-nonce.php (1 hunks)
- src/alley/wp/alleyvate/load.php (1 hunks)
Additional comments: 2
src/alley/wp/alleyvate/load.php (2)
- 36-36: The feature key 'login_none' in the summary does not match the actual key 'login_nonce' in the code. Please verify the correct feature key to ensure consistency in documentation and code.
- 'login_none' => new Features\Login_Nonce(), + 'login_nonce' => new Features\Login_Nonce(),
- 36-36: The addition of the 'login_nonce' feature to the list of features is correctly implemented.
| */ | ||
| private function generate_random_nonce_name( string $name ): string { | ||
| $parts = [ $name ]; | ||
| if ( ! empty( $_SERVER ) ) { // phpcs:ignore WordPressVIPMinimum.Variables.ServerVariables.UserControlledHeaders |
There was a problem hiding this comment.
The use of $_SERVER variables should be accompanied by isset checks to prevent PHP notices if these variables are not set.
- if ( ! empty( $_SERVER ) ) {
+ if ( isset( $_SERVER['REMOTE_ADDR'], $_SERVER['HTTP_X_FORWARDED_FOR'], $_SERVER['HTTP_CLIENT_IP'] ) ) {Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if ( ! empty( $_SERVER ) ) { // phpcs:ignore WordPressVIPMinimum.Variables.ServerVariables.UserControlledHeaders | |
| if ( isset( $_SERVER['REMOTE_ADDR'], $_SERVER['HTTP_X_FORWARDED_FOR'], $_SERVER['HTTP_CLIENT_IP'] ) ) { // phpcs:ignore WordPressVIPMinimum.Variables.ServerVariables.UserControlledHeaders |
Using user-controlled headers such as 'HTTP_X_FORWARDED_FOR' and 'HTTP_CLIENT_IP' in the nonce generation process could be a security risk if these headers are spoofed. Consider using server-side generated data that cannot be manipulated by the user.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/alley/wp/alleyvate/load.php (1 hunks)
Additional comments: 1
src/alley/wp/alleyvate/load.php (1)
- 33-39: The addition of the 'login_nonce' feature to the list of features in the
loadfunction appears to be correctly implemented. Ensure that theFeatures\Login_Nonceclass exists, is correctly namespaced, and implements the necessaryFeatureinterface with abootmethod.
| return; | ||
| } | ||
|
|
||
| echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( $this->nonce_timeout ) ); |
There was a problem hiding this comment.
The nonce timeout is stored in minutes but is used directly in the meta refresh tag, which expects seconds.
- echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( $this->nonce_timeout ) );
+ echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( $this->nonce_timeout * 60 ) );Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( $this->nonce_timeout ) ); | |
| echo sprintf( '<meta http-equiv="refresh" content="%d">', esc_attr( $this->nonce_timeout * 60 ) ); |
| * and we know we have a valid login attempt, remove all the filters | ||
| * so we don't risk bypassing our nonce validation down the line. | ||
| */ | ||
| remove_all_filters( 'authenticate' ); |
There was a problem hiding this comment.
The use of remove_all_filters on the 'authenticate' hook could still pose a security risk by removing all authentication checks. Please ensure that this action does not compromise the security of the system.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/alley/wp/alleyvate/features/class-login-nonce.php (1 hunks)
Additional comments: 1
src/alley/wp/alleyvate/features/class-login-nonce.php (1)
- 65-65: The previous comment about the nonce timeout being stored in minutes but used directly in the meta refresh tag, which expects seconds, is incorrect. The code correctly stores the nonce timeout in seconds.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/alley/wp/alleyvate/features/class-login-nonce.php (1 hunks)
- tests/alley/wp/alleyvate/features/test-login-nonce.php (1 hunks)
Additional comments: 2
tests/alley/wp/alleyvate/features/test-login-nonce.php (2)
46-64: The test
test_logins_require_noncecorrectly simulates a login attempt without a nonce and asserts that the result is aWP_Errorwith the expected error code. This ensures that the nonce validation is enforced during login.70-90: The test
test_logins_work_with_nonceproperly simulates a login attempt with a valid nonce and asserts that the result is aWP_Userobject, indicating a successful login. This confirms that the nonce functionality allows logins when the nonce is correct.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/alley/wp/alleyvate/features/class-login-nonce.php (1 hunks)
- tests/alley/wp/alleyvate/features/test-login-nonce.php (1 hunks)
Additional comments: 3
src/alley/wp/alleyvate/features/class-login-nonce.php (1)
- 60-62: The addition of a meta refresh tag to the login page is a good practice to ensure the page refreshes after the nonce timeout, preventing the use of expired nonces.
tests/alley/wp/alleyvate/features/test-login-nonce.php (2)
65-66: The test
test_logins_require_noncecorrectly asserts that aWP_Erroris returned when the nonce is not provided, and the error code is 'nonce_failure'. This ensures that the login process requires a nonce for authentication.94-95: The test
test_logins_work_with_noncecorrectly asserts that aWP_Userobject is returned when a valid nonce is provided, indicating a successful login.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/alley/wp/alleyvate/features/test-login-nonce.php (1 hunks)
Additional comments: 4
tests/alley/wp/alleyvate/features/test-login-nonce.php (4)
51-68: The previous comment about resetting
$_POSTto its original state after the test is still valid and has not been addressed in the provided hunk.62-68: The previous comment about improving the test by asserting the contents of the error message or the
WP_Errorobject is still valid and has not been addressed in the provided hunk.74-101: The previous comment about resetting
$_POSTto its original state after the test is still valid and has not been addressed in the provided hunk.71-101: The previous comment about checking that the nonce field is present in the login form HTML output is still valid and has not been addressed in the provided hunk.
|
@anubisthejackle this is pretty close - see my comment about the log in button name, and I like your suggestion about not making the nonce name random. Let me know when that's done and I'll re-review |
…gin form submission
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/alley/wp/alleyvate/features/class-login-nonce.php (1 hunks)
- tests/alley/wp/alleyvate/features/test-login-nonce.php (1 hunks)
Additional comments: 1
tests/alley/wp/alleyvate/features/test-login-nonce.php (1)
- 83-111: The test
test_logins_work_with_noncecorrectly tests the nonce functionality by adding and then removing a filter, and asserting the HTTP response code. This ensures that a login with a valid nonce is successful.
|
@kevinfodness I've added those changes now. |
| * Initializes the nonce fields. Is only run on `login_init` to restrict nonce data to login page. | ||
| */ | ||
| public static function action__add_nonce_life_filter(): void { | ||
| add_filter( 'nonce_life', fn() => self::NONCE_TIMEOUT ); |
There was a problem hiding this comment.
Could this check the nonce name & action and only set the timeout for this particular nonce? This way it could avoid changing the nonce timeout of any other nonces that may be added to this page. (May be an unlikely edge case, though.)
There was a problem hiding this comment.
It could, but I'm not sure if its necessary, since this is only added on the login_init action, which would restrict it to the login page.
There was a problem hiding this comment.
Yes, I have a hard time imagining the issue here, although being as precise as possible may help prevent some future compatibility issue that we can't predict. I guess possibilities could be other plugins adding a nonce or WP core adding a nonce.
There was a problem hiding this comment.
I'm fine with leaving this as-is. If core ends up adding a nonce to the login screen, this feature would become irrelevant, and we could remove it :)
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/alley/wp/alleyvate/load.php (1 hunks)
Additional comments: 1
src/alley/wp/alleyvate/load.php (1)
- 25-31: The addition of the 'login_nonce' feature to the
available_featuresarray is consistent with the PR's objective to enhance security. This change should be reviewed in the context of the entire login process to ensure that it integrates smoothly and does not introduce any unintended side effects.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/alley/wp/alleyvate/features/class-login-nonce.php (1 hunks)
Additional comments: 1
src/alley/wp/alleyvate/features/class-login-nonce.php (1)
- 112-113: Ensure that the
http_response_code( 403 )is set beforewp_die()to properly indicate a forbidden action. This is correctly implemented.
| * Initializes the nonce fields. Is only run on `login_init` to restrict nonce data to login page. | ||
| */ | ||
| public static function action__add_nonce_life_filter(): void { | ||
| add_filter( 'nonce_life', fn() => self::NONCE_TIMEOUT ); |
There was a problem hiding this comment.
I'm fine with leaving this as-is. If core ends up adding a nonce to the login screen, this feature would become irrelevant, and we could remove it :)
| $nonce = false; | ||
| if ( ! empty( $_POST[ self::NONCE_NAME ] ) ) { | ||
| $nonce = sanitize_key( $_POST[ self::NONCE_NAME ] ); | ||
| } |
There was a problem hiding this comment.
The logic below still works if this defaults to empty string:
| $nonce = false; | |
| if ( ! empty( $_POST[ self::NONCE_NAME ] ) ) { | |
| $nonce = sanitize_key( $_POST[ self::NONCE_NAME ] ); | |
| } | |
| $nonce = sanitize_key( $_POST[ self::NONCE_NAME ] ?? '' ); |
Summary
This PR adds a nonce to wp-login.php as described in issue #49.
Notes for reviewers
Please consider the potential overhead to the login process and the requirement for the page to be uncached when reviewing the changes.
Changelog entries
Added
Changed
Deprecated
Removed
Fixed
Security
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Documentation