From 55a4f62d9ce1b7892c08d09cc38af42fe8661b67 Mon Sep 17 00:00:00 2001 From: Jeff Stieler Date: Thu, 7 Oct 2021 12:37:34 -0400 Subject: [PATCH 1/8] Ensure that API key descriptions are truncated over the REST API. --- includes/class-wc-auth.php | 29 +++++++++-------- tests/php/includes/class-wc-auth-test.php | 38 +++++++++++++++++++++++ 2 files changed, 54 insertions(+), 13 deletions(-) create mode 100644 tests/php/includes/class-wc-auth-test.php diff --git a/includes/class-wc-auth.php b/includes/class-wc-auth.php index 6b313cc1689..23fdf0861ce 100644 --- a/includes/class-wc-auth.php +++ b/includes/class-wc-auth.php @@ -129,7 +129,8 @@ class WC_Auth { 'return_url' => rawurlencode( $this->get_formatted_url( $data['return_url'] ) ), 'callback_url' => rawurlencode( $this->get_formatted_url( $data['callback_url'] ) ), 'scope' => wc_clean( $data['scope'] ), - ), $url + ), + $url ); } @@ -210,14 +211,11 @@ class WC_Auth { global $wpdb; $description = sprintf( - /* translators: 1: app name 2: scope 3: date 4: time */ - __( '%1$s - API %2$s (created on %3$s at %4$s).', 'woocommerce' ), - wc_clean( $app_name ), - $this->get_i18n_scope( $scope ), - date_i18n( wc_date_format() ), - date_i18n( wc_time_format() ) + '%s - API (%s)', + wc_trim_string( wc_clean( $app_name ), 170 ), + gmdate( 'Y-m-d H:i:s' ) ); - $user = wp_get_current_user(); + $user = wp_get_current_user(); // Created API keys. $permissions = in_array( $scope, array( 'read', 'write', 'read_write' ), true ) ? sanitize_text_field( $scope ) : 'read'; @@ -327,13 +325,15 @@ class WC_Auth { // Login endpoint. if ( 'login' === $route && ! is_user_logged_in() ) { wc_get_template( - 'auth/form-login.php', array( + 'auth/form-login.php', + array( 'app_name' => wc_clean( $data['app_name'] ), 'return_url' => add_query_arg( array( 'success' => 0, 'user_id' => wc_clean( $data['user_id'] ), - ), $this->get_formatted_url( $data['return_url'] ) + ), + $this->get_formatted_url( $data['return_url'] ) ), 'redirect_url' => $this->build_url( $data, 'authorize' ), ) @@ -353,13 +353,15 @@ class WC_Auth { } elseif ( 'authorize' === $route && current_user_can( 'manage_woocommerce' ) ) { // Authorize endpoint. wc_get_template( - 'auth/form-grant-access.php', array( + 'auth/form-grant-access.php', + array( 'app_name' => wc_clean( $data['app_name'] ), 'return_url' => add_query_arg( array( 'success' => 0, 'user_id' => wc_clean( $data['user_id'] ), - ), $this->get_formatted_url( $data['return_url'] ) + ), + $this->get_formatted_url( $data['return_url'] ) ), 'scope' => $this->get_i18n_scope( wc_clean( $data['scope'] ) ), 'permissions' => $this->get_permissions_in_scope( wc_clean( $data['scope'] ) ), @@ -386,7 +388,8 @@ class WC_Auth { array( 'success' => 1, 'user_id' => wc_clean( $data['user_id'] ), - ), $this->get_formatted_url( $data['return_url'] ) + ), + $this->get_formatted_url( $data['return_url'] ) ) ) ); diff --git a/tests/php/includes/class-wc-auth-test.php b/tests/php/includes/class-wc-auth-test.php new file mode 100644 index 00000000000..5f6907fec3e --- /dev/null +++ b/tests/php/includes/class-wc-auth-test.php @@ -0,0 +1,38 @@ +getMethod( 'create_keys' ); + $create_keys->setAccessible( true ); + + $app_name = 'This_app_name_is_very_long_and_meant_to_exceed_the_column_length_of_200_characters_'; + $app_name .= $app_name; + $app_user_id = 1; + $scope = 'read_write'; + + $key_data = $create_keys->invoke( $wc_auth, $app_name, $app_user_id, $scope ); + + // Verify the key was inserted successfully. + $this->assertNotEquals( 0, $key_data['key_id'], 'API Key with long description was not written to database.' ); + + // Clean up. + $maybe_delete_key = $reflected_auth->getMethod( 'maybe_delete_key' ); + $maybe_delete_key->setAccessible( true ); + $maybe_delete_key->invoke( $wc_auth, $key_data ); + } +} From 20038828bbfad8732a08a3a1e083d29b680e3761 Mon Sep 17 00:00:00 2001 From: Jeff Stieler Date: Thu, 7 Oct 2021 16:05:00 -0400 Subject: [PATCH 2/8] Limit API key description field to 200 characters. --- includes/admin/settings/views/html-keys-edit.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/admin/settings/views/html-keys-edit.php b/includes/admin/settings/views/html-keys-edit.php index 04da58e51cf..a7df1ac4704 100644 --- a/includes/admin/settings/views/html-keys-edit.php +++ b/includes/admin/settings/views/html-keys-edit.php @@ -23,7 +23,7 @@ defined( 'ABSPATH' ) || exit; - + From 624f2eac4baac1a7d539b554bbc604ff007ff674 Mon Sep 17 00:00:00 2001 From: Jeff Stieler Date: Thu, 7 Oct 2021 16:06:08 -0400 Subject: [PATCH 3/8] Avoid reporting success when API key insertion fails. --- includes/class-wc-ajax.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/includes/class-wc-ajax.php b/includes/class-wc-ajax.php index 456c0509103..5f683f2aeff 100644 --- a/includes/class-wc-ajax.php +++ b/includes/class-wc-ajax.php @@ -2131,6 +2131,10 @@ class WC_AJAX { ) ); + if ( 0 === $wpdb->insert_id ) { + throw new Exception( __( 'There was an error generating your API Key.', 'woocommerce' ) ); + } + $key_id = $wpdb->insert_id; $response = $data; $response['consumer_key'] = $consumer_key; From 1f32776760e679f25239c570a17365ba427447d1 Mon Sep 17 00:00:00 2001 From: Jeff Stieler Date: Thu, 7 Oct 2021 17:01:48 -0400 Subject: [PATCH 4/8] Add test case for API key admin ajax error visibility. --- tests/php/includes/class-wc-ajax-test.php | 30 ++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/php/includes/class-wc-ajax-test.php b/tests/php/includes/class-wc-ajax-test.php index 104b8069d3d..1c60905ba4c 100644 --- a/tests/php/includes/class-wc-ajax-test.php +++ b/tests/php/includes/class-wc-ajax-test.php @@ -8,7 +8,7 @@ /** * Class WC_AJAX_Test file. */ -class WC_AJAX_Test extends \WC_Unit_Test_Case { +class WC_AJAX_Test extends \WP_Ajax_UnitTestCase { /** * Stock should not be reduced from AJAX when an item is added to an order. @@ -84,4 +84,32 @@ class WC_AJAX_Test extends \WC_Unit_Test_Case { } } } + + /** + * Creating an API Key with too long of a description should report failure. + */ + public function test_create_api_key_long_description_failure() { + $this->_setRole( 'administrator' ); + + $description = 'This_description_is_really_very_long_and_is_meant_to_exceed_the_database_column_length_of_200_characters_'; + $description .= $description; + + $_POST['security'] = wp_create_nonce( 'update-api-key' ); + $_POST['key_id'] = 0; + $_POST['user'] = 1; + $_POST['permissions'] = 'read'; + $_POST['description'] = $description; + + try { + $this->_handleAjax( 'woocommerce_update_api_key' ); + } catch ( WPAjaxDieContinueException $e ) { + // wp_die() doesn't actually occur, so we need to clean up WC_AJAX::update_api_key's output buffer. + ob_end_clean(); + } + + $response = json_decode( $this->_last_response, true ); + + $this->assertFalse( $response['success'] ); + $this->assertEquals( $response['data']['message'], 'There was an error generating your API Key.' ); + } } From 7a68bc5ff7f0321a11bb4e3b8d8f47a704b9c66c Mon Sep 17 00:00:00 2001 From: Jeff Stieler Date: Thu, 7 Oct 2021 12:37:34 -0400 Subject: [PATCH 5/8] Ensure that API key descriptions are truncated over the REST API. --- .../woocommerce/includes/class-wc-auth.php | 29 +++++++------- tests/php/includes/class-wc-auth-test.php | 38 +++++++++++++++++++ 2 files changed, 54 insertions(+), 13 deletions(-) create mode 100644 tests/php/includes/class-wc-auth-test.php diff --git a/plugins/woocommerce/includes/class-wc-auth.php b/plugins/woocommerce/includes/class-wc-auth.php index 6b313cc1689..23fdf0861ce 100644 --- a/plugins/woocommerce/includes/class-wc-auth.php +++ b/plugins/woocommerce/includes/class-wc-auth.php @@ -129,7 +129,8 @@ class WC_Auth { 'return_url' => rawurlencode( $this->get_formatted_url( $data['return_url'] ) ), 'callback_url' => rawurlencode( $this->get_formatted_url( $data['callback_url'] ) ), 'scope' => wc_clean( $data['scope'] ), - ), $url + ), + $url ); } @@ -210,14 +211,11 @@ class WC_Auth { global $wpdb; $description = sprintf( - /* translators: 1: app name 2: scope 3: date 4: time */ - __( '%1$s - API %2$s (created on %3$s at %4$s).', 'woocommerce' ), - wc_clean( $app_name ), - $this->get_i18n_scope( $scope ), - date_i18n( wc_date_format() ), - date_i18n( wc_time_format() ) + '%s - API (%s)', + wc_trim_string( wc_clean( $app_name ), 170 ), + gmdate( 'Y-m-d H:i:s' ) ); - $user = wp_get_current_user(); + $user = wp_get_current_user(); // Created API keys. $permissions = in_array( $scope, array( 'read', 'write', 'read_write' ), true ) ? sanitize_text_field( $scope ) : 'read'; @@ -327,13 +325,15 @@ class WC_Auth { // Login endpoint. if ( 'login' === $route && ! is_user_logged_in() ) { wc_get_template( - 'auth/form-login.php', array( + 'auth/form-login.php', + array( 'app_name' => wc_clean( $data['app_name'] ), 'return_url' => add_query_arg( array( 'success' => 0, 'user_id' => wc_clean( $data['user_id'] ), - ), $this->get_formatted_url( $data['return_url'] ) + ), + $this->get_formatted_url( $data['return_url'] ) ), 'redirect_url' => $this->build_url( $data, 'authorize' ), ) @@ -353,13 +353,15 @@ class WC_Auth { } elseif ( 'authorize' === $route && current_user_can( 'manage_woocommerce' ) ) { // Authorize endpoint. wc_get_template( - 'auth/form-grant-access.php', array( + 'auth/form-grant-access.php', + array( 'app_name' => wc_clean( $data['app_name'] ), 'return_url' => add_query_arg( array( 'success' => 0, 'user_id' => wc_clean( $data['user_id'] ), - ), $this->get_formatted_url( $data['return_url'] ) + ), + $this->get_formatted_url( $data['return_url'] ) ), 'scope' => $this->get_i18n_scope( wc_clean( $data['scope'] ) ), 'permissions' => $this->get_permissions_in_scope( wc_clean( $data['scope'] ) ), @@ -386,7 +388,8 @@ class WC_Auth { array( 'success' => 1, 'user_id' => wc_clean( $data['user_id'] ), - ), $this->get_formatted_url( $data['return_url'] ) + ), + $this->get_formatted_url( $data['return_url'] ) ) ) ); diff --git a/tests/php/includes/class-wc-auth-test.php b/tests/php/includes/class-wc-auth-test.php new file mode 100644 index 00000000000..5f6907fec3e --- /dev/null +++ b/tests/php/includes/class-wc-auth-test.php @@ -0,0 +1,38 @@ +getMethod( 'create_keys' ); + $create_keys->setAccessible( true ); + + $app_name = 'This_app_name_is_very_long_and_meant_to_exceed_the_column_length_of_200_characters_'; + $app_name .= $app_name; + $app_user_id = 1; + $scope = 'read_write'; + + $key_data = $create_keys->invoke( $wc_auth, $app_name, $app_user_id, $scope ); + + // Verify the key was inserted successfully. + $this->assertNotEquals( 0, $key_data['key_id'], 'API Key with long description was not written to database.' ); + + // Clean up. + $maybe_delete_key = $reflected_auth->getMethod( 'maybe_delete_key' ); + $maybe_delete_key->setAccessible( true ); + $maybe_delete_key->invoke( $wc_auth, $key_data ); + } +} From 68f1b12797a83a0909a8714fdb6ba9d125428700 Mon Sep 17 00:00:00 2001 From: Jeff Stieler Date: Thu, 7 Oct 2021 16:05:00 -0400 Subject: [PATCH 6/8] Limit API key description field to 200 characters. --- .../includes/admin/settings/views/html-keys-edit.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/woocommerce/includes/admin/settings/views/html-keys-edit.php b/plugins/woocommerce/includes/admin/settings/views/html-keys-edit.php index 04da58e51cf..a7df1ac4704 100644 --- a/plugins/woocommerce/includes/admin/settings/views/html-keys-edit.php +++ b/plugins/woocommerce/includes/admin/settings/views/html-keys-edit.php @@ -23,7 +23,7 @@ defined( 'ABSPATH' ) || exit; - + From 4975a5ea211954cf9fe2ddd13f04875c12229b93 Mon Sep 17 00:00:00 2001 From: Jeff Stieler Date: Thu, 7 Oct 2021 16:06:08 -0400 Subject: [PATCH 7/8] Avoid reporting success when API key insertion fails. --- plugins/woocommerce/includes/class-wc-ajax.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugins/woocommerce/includes/class-wc-ajax.php b/plugins/woocommerce/includes/class-wc-ajax.php index 37731c7e106..5ffbbd92eba 100644 --- a/plugins/woocommerce/includes/class-wc-ajax.php +++ b/plugins/woocommerce/includes/class-wc-ajax.php @@ -2126,6 +2126,10 @@ class WC_AJAX { ) ); + if ( 0 === $wpdb->insert_id ) { + throw new Exception( __( 'There was an error generating your API Key.', 'woocommerce' ) ); + } + $key_id = $wpdb->insert_id; $response = $data; $response['consumer_key'] = $consumer_key; From 3e5e9e8eda0fab3158714ad91c2feac457abfaae Mon Sep 17 00:00:00 2001 From: Jeff Stieler Date: Thu, 7 Oct 2021 17:01:48 -0400 Subject: [PATCH 8/8] Add test case for API key admin ajax error visibility. --- .../tests/php/includes/class-wc-ajax-test.php | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/plugins/woocommerce/tests/php/includes/class-wc-ajax-test.php b/plugins/woocommerce/tests/php/includes/class-wc-ajax-test.php index 104b8069d3d..1c60905ba4c 100644 --- a/plugins/woocommerce/tests/php/includes/class-wc-ajax-test.php +++ b/plugins/woocommerce/tests/php/includes/class-wc-ajax-test.php @@ -8,7 +8,7 @@ /** * Class WC_AJAX_Test file. */ -class WC_AJAX_Test extends \WC_Unit_Test_Case { +class WC_AJAX_Test extends \WP_Ajax_UnitTestCase { /** * Stock should not be reduced from AJAX when an item is added to an order. @@ -84,4 +84,32 @@ class WC_AJAX_Test extends \WC_Unit_Test_Case { } } } + + /** + * Creating an API Key with too long of a description should report failure. + */ + public function test_create_api_key_long_description_failure() { + $this->_setRole( 'administrator' ); + + $description = 'This_description_is_really_very_long_and_is_meant_to_exceed_the_database_column_length_of_200_characters_'; + $description .= $description; + + $_POST['security'] = wp_create_nonce( 'update-api-key' ); + $_POST['key_id'] = 0; + $_POST['user'] = 1; + $_POST['permissions'] = 'read'; + $_POST['description'] = $description; + + try { + $this->_handleAjax( 'woocommerce_update_api_key' ); + } catch ( WPAjaxDieContinueException $e ) { + // wp_die() doesn't actually occur, so we need to clean up WC_AJAX::update_api_key's output buffer. + ob_end_clean(); + } + + $response = json_decode( $this->_last_response, true ); + + $this->assertFalse( $response['success'] ); + $this->assertEquals( $response['data']['message'], 'There was an error generating your API Key.' ); + } }