From e1c98a744b410dd9f31ee80d3f1064fc9d433cd1 Mon Sep 17 00:00:00 2001 From: Timothy Jacobs Date: Wed, 27 Jan 2021 19:03:42 +0000 Subject: [PATCH] App Passwords: Improve validation and sanitization of the application name. Application names are now required to be unique and cannot contain solely whitespace characters. Additionally, invalid characters are now stripped from the application name using `sanitize_text_field()`. Props Boniu91, hellofromTonya, engahmeds3ed, xkon, francina. Fixes #51941. git-svn-id: https://develop.svn.wordpress.org/trunk@50030 602fd350-edb4-49c9-b593-d223f7449a82 --- .../_enqueues/admin/application-passwords.js | 2 +- src/js/_enqueues/admin/auth-app.js | 2 +- .../class-wp-application-passwords.php | 37 +++- ...-rest-application-passwords-controller.php | 2 + .../tests/rest-api/application-passwords.php | 201 ++++++++++++++++++ .../rest-application-passwords-controller.php | 3 +- tests/qunit/fixtures/wp-api-generated.js | 4 + 7 files changed, 247 insertions(+), 4 deletions(-) create mode 100644 tests/phpunit/tests/rest-api/application-passwords.php diff --git a/src/js/_enqueues/admin/application-passwords.js b/src/js/_enqueues/admin/application-passwords.js index 7901ac901b..cab592519b 100644 --- a/src/js/_enqueues/admin/application-passwords.js +++ b/src/js/_enqueues/admin/application-passwords.js @@ -57,7 +57,7 @@ $newAppPassButton.prop( 'disabled', false ); $newAppPassForm.after( tmplNewAppPass( { - name: name, + name: response.name, password: response.password } ) ); $( '.new-application-password-notice' ).focus(); diff --git a/src/js/_enqueues/admin/auth-app.js b/src/js/_enqueues/admin/auth-app.js index b4b8ddbda5..f358dd7e64 100644 --- a/src/js/_enqueues/admin/auth-app.js +++ b/src/js/_enqueues/admin/auth-app.js @@ -98,7 +98,7 @@ .append( '

' + wp.i18n.__( 'Be sure to save this in a safe location. You will not be able to retrieve it.' ) + '

' ); // We're using .text() to write the variables to avoid any chance of XSS. - $( 'strong', $notice ).text( name ); + $( 'strong', $notice ).text( response.name ); $( 'input', $notice ).val( response.password ); $form.replaceWith( $notice ); diff --git a/src/wp-includes/class-wp-application-passwords.php b/src/wp-includes/class-wp-application-passwords.php index 22ad1c72c8..0fd5d8b40a 100644 --- a/src/wp-includes/class-wp-application-passwords.php +++ b/src/wp-includes/class-wp-application-passwords.php @@ -58,6 +58,7 @@ class WP_Application_Passwords { * Creates a new application password. * * @since 5.6.0 + * @since 5.7.0 Returns WP_Error if application name already exists. * * @param int $user_id User ID. * @param array $args Information about the application password. @@ -65,8 +66,16 @@ class WP_Application_Passwords { * A WP_Error instance is returned on error. */ public static function create_new_application_password( $user_id, $args = array() ) { + if ( ! empty( $args['name'] ) ) { + $args['name'] = sanitize_text_field( $args['name'] ); + } + if ( empty( $args['name'] ) ) { - return new WP_Error( 'application_password_empty_name', __( 'An application name is required to create an application password.' ) ); + return new WP_Error( 'application_password_empty_name', __( 'An application name is required to create an application password.' ), array( 'status' => 400 ) ); + } + + if ( self::application_name_exists_for_user( $user_id, $args['name'] ) ) { + return new WP_Error( 'application_password_duplicate_name', __( 'Each application name should be unique.' ), array( 'status' => 409 ) ); } $new_password = wp_generate_password( static::PW_LENGTH, false ); @@ -162,6 +171,28 @@ class WP_Application_Passwords { return null; } + /** + * Check if application name exists before for this user. + * + * @since 5.7.0 + * + * @param int $user_id User ID. + * @param string $name Application name. + * + * @return bool Provided application name exists or not. + */ + public static function application_name_exists_for_user( $user_id, $name ) { + $passwords = static::get_user_application_passwords( $user_id ); + + foreach ( $passwords as $password ) { + if ( strtolower( $password['name'] ) === strtolower( $name ) ) { + return true; + } + } + + return false; + } + /** * Updates an application password. * @@ -180,6 +211,10 @@ class WP_Application_Passwords { continue; } + if ( ! empty( $update['name'] ) ) { + $update['name'] = sanitize_text_field( $update['name'] ); + } + $save = false; if ( ! empty( $update['name'] ) && $item['name'] !== $update['name'] ) { diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-application-passwords-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-application-passwords-controller.php index dc94348022..622d3617c2 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-application-passwords-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-application-passwords-controller.php @@ -631,6 +631,8 @@ class WP_REST_Application_Passwords_Controller extends WP_REST_Controller { 'type' => 'string', 'required' => true, 'context' => array( 'view', 'edit', 'embed' ), + 'minLength' => 1, + 'pattern' => '.*\S.*', ), 'password' => array( 'description' => __( 'The generated password. Only available after adding an application.' ), diff --git a/tests/phpunit/tests/rest-api/application-passwords.php b/tests/phpunit/tests/rest-api/application-passwords.php new file mode 100644 index 0000000000..b3edab0d72 --- /dev/null +++ b/tests/phpunit/tests/rest-api/application-passwords.php @@ -0,0 +1,201 @@ +user->create( + array( + 'role' => 'administrator', + ) + ); + + if ( is_multisite() ) { + grant_super_admin( self::$user_id ); + } + } + + + /** + * @covers WP_Application_Passwords::create_new_application_password + * @ticket 51941 + * @dataProvider data_create_new_application_password_validation + */ + public function test_create_new_application_password_validation( $expected, array $args = array(), array $names = array() ) { + // Create the existing passwords. + foreach ( $names as $name ) { + WP_Application_Passwords::create_new_application_password( self::$user_id, array( 'name' => $name ) ); + } + + $actual = WP_Application_Passwords::create_new_application_password( self::$user_id, $args ); + + $this->assertInstanceOf( WP_Error::class, $actual ); + $this->assertSame( $expected['error_code'], $actual->get_error_code() ); + $this->assertSame( $expected['error_message'], $actual->get_error_message( $expected['error_code'] ) ); + } + + public function data_create_new_application_password_validation() { + return array( + 'application_password_empty_name when no args' => array( + 'expected' => array( + 'error_code' => 'application_password_empty_name', + 'error_message' => 'An application name is required to create an application password.', + ), + ), + 'application_password_empty_name when no name' => array( + 'expected' => array( + 'error_code' => 'application_password_empty_name', + 'error_message' => 'An application name is required to create an application password.', + ), + 'args' => array( 'app_id' => 1 ), + ), + 'application_password_empty_name when empty name' => array( + 'expected' => array( + 'error_code' => 'application_password_empty_name', + 'error_message' => 'An application name is required to create an application password.', + ), + 'args' => array( 'name' => ' ' ), + ), + 'application_password_empty_name when ' ), + ), + 'application_password_duplicate_name when name exists' => array( + 'expected' => array( + 'error_code' => 'application_password_duplicate_name', + 'error_message' => 'Each application name should be unique.', + ), + 'args' => array( 'name' => 'test2' ), + 'names' => array( 'test1', 'test2' ), + ), + ); + } + + /** + * @covers WP_Application_Passwords::create_new_application_password + * @ticket 51941 + * @dataProvider data_create_new_application_password + */ + public function test_create_new_application_password( array $args, array $names = array() ) { + // Create the existing passwords. + foreach ( $names as $name ) { + WP_Application_Passwords::create_new_application_password( self::$user_id, array( 'name' => $name ) ); + } + + list( $new_password, $new_item ) = WP_Application_Passwords::create_new_application_password( self::$user_id, $args ); + + $this->assertNotEmpty( $new_password ); + $this->assertSame( + array( 'uuid', 'app_id', 'name', 'password', 'created', 'last_used', 'last_ip' ), + array_keys( $new_item ) + ); + $this->assertSame( $args['name'], $new_item['name'] ); + } + + public function data_create_new_application_password() { + return array( + 'should create new password when no passwords exists' => array( + 'args' => array( 'name' => 'test3' ), + ), + 'should create new password when name is unique' => array( + 'args' => array( 'name' => 'test3' ), + 'names' => array( 'test1', 'test2' ), + ), + ); + } + + /** + * @covers WP_Application_Passwords::application_name_exists_for_user + * @ticket 51941 + * @dataProvider data_application_name_exists_for_user + */ + public function test_application_name_exists_for_user( $expected, $name ) { + if ( $expected ) { + WP_Application_Passwords::create_new_application_password( self::$user_id, array( 'name' => $name ) ); + } + + $this->assertSame( $expected, WP_Application_Passwords::application_name_exists_for_user( self::$user_id, $name ) ); + } + + public function data_application_name_exists_for_user() { + return array( + array( false, 'test1' ), + array( false, 'baz' ), + array( false, 'bar' ), + array( true, 'App 1' ), + array( true, 'Some Test' ), + array( true, 'Baz' ), + ); + } + + /** + * @covers WP_Application_Passwords::update_application_password + * @ticket 51941 + * @dataProvider data_update_application_password + */ + public function test_update_application_password( array $update, array $existing ) { + // Create the original item. + list( , $original_item ) = WP_Application_Passwords::create_new_application_password( self::$user_id, $existing ); + $uuid = $original_item['uuid']; + + $actual = WP_Application_Passwords::update_application_password( self::$user_id, $uuid, $update ); + + $this->assertTrue( $actual ); + + // Check updated only given values. + $updated_item = WP_Application_Passwords::get_user_application_password( self::$user_id, $uuid ); + foreach ( $updated_item as $key => $update_value ) { + $expected_value = isset( $update[ $key ] ) ? $update[ $key ] : $original_item[ $key ]; + $this->assertSame( $expected_value, $update_value ); + } + } + + /** + * @covers WP_Application_Passwords::update_application_password + * @ticket 51941 + * @dataProvider data_update_application_password + */ + public function test_update_application_password_when_no_password_found( array $update ) { + $actual = WP_Application_Passwords::update_application_password( self::$user_id, '', $update ); + + $this->assertInstanceOf( WP_Error::class, $actual ); + $this->assertSame( 'application_password_not_found', $actual->get_error_code() ); + $this->assertSame( 'Could not find an application password with that id.', $actual->get_error_message( 'application_password_not_found' ) ); + } + + public function data_update_application_password() { + return array( + 'should not update when no values given to update' => array( + 'update' => array(), + 'existing' => array( 'name' => 'Test' ), + ), + 'should not update when given same name' => array( + 'update' => array( 'name' => 'Test' ), + 'existing' => array( 'name' => 'Test' ), + ), + 'should update name' => array( + 'update' => array( 'name' => 'Test Updated' ), + 'existing' => array( 'name' => 'Test' ), + ), + ); + } +} diff --git a/tests/phpunit/tests/rest-api/rest-application-passwords-controller.php b/tests/phpunit/tests/rest-api/rest-application-passwords-controller.php index 79218e307e..b123101bb3 100644 --- a/tests/phpunit/tests/rest-api/rest-application-passwords-controller.php +++ b/tests/phpunit/tests/rest-api/rest-application-passwords-controller.php @@ -538,6 +538,7 @@ class WP_Test_REST_Application_Passwords_Controller extends WP_Test_REST_Control /** * @ticket 51583 + * @ticket 51941 */ public function test_update_item_cannot_overwrite_app_id() { wp_set_current_user( self::$admin ); @@ -554,7 +555,7 @@ class WP_Test_REST_Application_Passwords_Controller extends WP_Test_REST_Control list( , $item ) = WP_Application_Passwords::create_new_application_password( self::$admin, array( - 'name' => 'App', + 'name' => 'App 2', 'app_id' => $app_id, ) ); diff --git a/tests/qunit/fixtures/wp-api-generated.js b/tests/qunit/fixtures/wp-api-generated.js index e9f6b11d9b..c615bcd274 100644 --- a/tests/qunit/fixtures/wp-api-generated.js +++ b/tests/qunit/fixtures/wp-api-generated.js @@ -4974,6 +4974,8 @@ mockedApiResponse.Schema = { "name": { "description": "The name of the application password.", "type": "string", + "minLength": 1, + "pattern": ".*\\S.*", "required": true } } @@ -5030,6 +5032,8 @@ mockedApiResponse.Schema = { "name": { "description": "The name of the application password.", "type": "string", + "minLength": 1, + "pattern": ".*\\S.*", "required": false } }