From 17d92ef4cd1e2fad536f638549c4ebd700073a3e Mon Sep 17 00:00:00 2001 From: Tim Carr Date: Sat, 27 Apr 2024 16:18:11 +0100 Subject: [PATCH 1/4] Add logging to `request` method, and remove duplicate log calls --- src/class-convertkit-api.php | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/class-convertkit-api.php b/src/class-convertkit-api.php index 2a56b99..4d99b8d 100644 --- a/src/class-convertkit-api.php +++ b/src/class-convertkit-api.php @@ -528,8 +528,6 @@ public function unsubscribe_by_email( string $email_address ) { */ public function get_all_posts( $posts_per_request = 50 ) { - $this->log( 'API: get_all_posts()' ); - // Sanitize some parameters. $posts_per_request = absint( $posts_per_request ); @@ -592,8 +590,6 @@ public function get_all_posts( $posts_per_request = 50 ) { */ public function get_posts( $page = 1, $per_page = 10 ) { - $this->log( 'API: get_posts()' ); - // Sanitize some parameters. $page = absint( $page ); $per_page = absint( $per_page ); @@ -652,8 +648,6 @@ public function get_posts( $page = 1, $per_page = 10 ) { */ public function get_post( $post_id ) { - $this->log( 'API: get_post(): [ post_id: ' . $post_id . ']' ); - // Send request. $response = $this->get( sprintf( 'posts/%s', $post_id ), @@ -709,8 +703,6 @@ public function get_products() { */ public function subscriber_authentication_send_code( $email, $redirect_url ) { - $this->log( 'API: subscriber_authentication_send_code(): [ email: ' . $email . ', redirect_url: ' . $redirect_url . ']' ); - // Sanitize some parameters. $email = trim( $email ); $redirect_url = trim( $redirect_url ); @@ -766,7 +758,6 @@ public function subscriber_authentication_send_code( $email, $redirect_url ) { */ public function subscriber_authentication_verify( $token, $subscriber_code ) { - $this->log( 'API: subscriber_authentication_verify(): [ token: ' . $this->mask_string( $token ) . ', subscriber_code: ' . $this->mask_string( $subscriber_code ) . ']' ); // Sanitize some parameters. $token = trim( $token ); @@ -818,8 +809,6 @@ public function subscriber_authentication_verify( $token, $subscriber_code ) { */ public function profile( $signed_subscriber_id ) { - $this->log( 'API: profile(): [ signed_subscriber_id: ' . $this->mask_string( $signed_subscriber_id ) . ' ]' ); - // Trim some parameters. $signed_subscriber_id = trim( $signed_subscriber_id ); @@ -1072,6 +1061,14 @@ private function is_json( $json_string ) { */ public function request( $endpoint, $method = 'get', $params = array(), $retry_if_rate_limit_hit = true ) { + // Log request. + $this->log( sprintf( + 'API: %s %s [%s]', + $method, + $endpoint, + wp_json_encode( $params ) + ) ); + // Send request. switch ( strtolower( $method ) ) { case 'get': From 64a7ed87bd34bacc43e41223660cec1ee97a5981 Mon Sep 17 00:00:00 2001 From: Tim Carr Date: Sat, 27 Apr 2024 16:36:26 +0100 Subject: [PATCH 2/4] Add logging of all requests with masking values --- src/class-convertkit-api.php | 39 ++++++++++++++++++++++++++++++++++-- tests/wpunit/APITest.php | 16 ++++++++++----- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/class-convertkit-api.php b/src/class-convertkit-api.php index 4d99b8d..02523c5 100644 --- a/src/class-convertkit-api.php +++ b/src/class-convertkit-api.php @@ -1063,10 +1063,10 @@ public function request( $endpoint, $method = 'get', $params = array(), $retry_i // Log request. $this->log( sprintf( - 'API: %s %s [%s]', + 'API: %s %s: %s', $method, $endpoint, - wp_json_encode( $params ) + wp_json_encode( $this->mask_params( $params ) ) ) ); // Send request. @@ -1445,6 +1445,41 @@ private function get_error_message( $key ) { } + /** + * Helper method to mask values for specific keys in an array. + * + * @since 2.0.0 + * + * @param string $str String to mask. + * @return string Masked string + */ + private function mask_params( $params ) { + + $keys = array( + 'first_name', + 'token', + 'subscriber_code', + 'signed_subscriber_id', + ); + + foreach ( $params as $key => $value ) { + // Skip if not a string + if ( ! is_string( $value ) ) { + continue; + } + + // Skip if the key isn't one we need to mask the value of. + if ( ! in_array( $key, $keys ) ) { + continue; + } + + $params[ $key ] = $this->mask_string( $value ); + } + + return $params; + + } + /** * Helper method to mask all but the last 4 characters of a string. * diff --git a/tests/wpunit/APITest.php b/tests/wpunit/APITest.php index a021648..c5c7a05 100644 --- a/tests/wpunit/APITest.php +++ b/tests/wpunit/APITest.php @@ -135,18 +135,23 @@ public function tearDown(): void */ public function testLog() { - $this->markTestIncomplete(); - // Define location for log file. define( 'CONVERTKIT_PLUGIN_PATH', $_ENV['WP_ROOT_FOLDER'] . '/wp-content/uploads' ); // Create a log.txt file. $this->tester->writeToFile(CONVERTKIT_PLUGIN_PATH . '/log.txt', 'historical log file'); - // Initialize API. - $api = new ConvertKit_API( $_ENV['CONVERTKIT_API_KEY'], $_ENV['CONVERTKIT_API_SECRET'], true ); + // Initialize API with logging enabled. + $api = new ConvertKit_API( + $_ENV['CONVERTKIT_OAUTH_CLIENT_ID'], + $_ENV['CONVERTKIT_OAUTH_REDIRECT_URI'], + $_ENV['CONVERTKIT_OAUTH_ACCESS_TOKEN'], + $_ENV['CONVERTKIT_OAUTH_REFRESH_TOKEN'], + true + ); // Perform actions that will write sensitive data to the log file. + /* $api->form_subscribe( $_ENV['CONVERTKIT_API_FORM_ID'], $_ENV['CONVERTKIT_API_SUBSCRIBER_EMAIL'], @@ -155,6 +160,7 @@ public function testLog() 'last_name' => 'Last', ) ); + */ $api->profile($_ENV['CONVERTKIT_API_SIGNED_SUBSCRIBER_ID']); // Confirm the historical log.txt file has been deleted. @@ -168,7 +174,7 @@ public function testLog() // Confirm the contents of the log file have masked the email address, name and signed subscriber ID. $this->tester->openFile(CONVERTKIT_PLUGIN_PATH . '/log/log.txt'); - $this->tester->seeInThisFile('API: form_subscribe(): [ form_id: ' . $_ENV['CONVERTKIT_API_FORM_ID'] . ', email: o****@n********.c**, first_name: ******Name ]'); + //$this->tester->seeInThisFile('API: form_subscribe(): [ form_id: ' . $_ENV['CONVERTKIT_API_FORM_ID'] . ', email: o****@n********.c**, first_name: ******Name ]'); $this->tester->seeInThisFile('API: profile(): [ signed_subscriber_id: ********************'); $this->tester->dontSeeInThisFile($_ENV['CONVERTKIT_API_SUBSCRIBER_EMAIL']); $this->tester->dontSeeInThisFile('First Name'); From 316717b4ddc532ac59d8a619f54fed94d8f69a34 Mon Sep 17 00:00:00 2001 From: Tim Carr Date: Mon, 29 Apr 2024 13:26:52 +0100 Subject: [PATCH 3/4] Mask endpoint URIs where required; reinstate `testLog` to confirm sensitive data masked --- src/class-convertkit-api.php | 64 +++++++++++++++++++++++++++++------- tests/wpunit/APITest.php | 6 ++-- 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/src/class-convertkit-api.php b/src/class-convertkit-api.php index bb43afe..d27154f 100644 --- a/src/class-convertkit-api.php +++ b/src/class-convertkit-api.php @@ -857,7 +857,6 @@ public function subscriber_authentication_send_code( $email, $redirect_url ) { */ public function subscriber_authentication_verify( $token, $subscriber_code ) { - // Sanitize some parameters. $token = trim( $token ); $subscriber_code = trim( $subscriber_code ); @@ -1178,12 +1177,19 @@ private function is_json( $json_string ) { public function request( $endpoint, $method = 'get', $params = array(), $retry_if_rate_limit_hit = true ) { // Log request. - $this->log( sprintf( - 'API: %s %s: %s', - $method, - $endpoint, - wp_json_encode( $this->mask_params( $params ) ) - ) ); + $log = sprintf( + 'API: %s %s', + $method, + $this->mask_endpoint( $endpoint ) + ); + if ( count( $params ) ) { + $log = sprintf( + '%s: %s', + $log, + wp_json_encode( $this->mask_params( $params ) ) + ); + } + $this->log( $log ); // Send request. switch ( strtolower( $method ) ) { @@ -1300,6 +1306,8 @@ public function request( $endpoint, $method = 'get', $params = array(), $retry_i break; } + $this->log( 'API: Error: ' . $error ); + return new WP_Error( 'convertkit_api_error', $error, @@ -1561,31 +1569,63 @@ private function get_error_message( $key ) { } + /** + * Helper method to mask the given endpoint URL where it contains + * sensitive data, such as a signed subscriber ID. + * + * @since 2.0.0 + * + * @param string $endpoint String to mask. + * @return string Masked string + */ + private function mask_endpoint( $endpoint ) { + + // Endpoints to mask string. + $keys = array( + 'profile/', + ); + + foreach ( $keys as $key => $value ) { + // Skip if the endpoint isn't one we need to mask. + if ( strpos( $endpoint, $value ) === false ) { + continue; + } + + // Mask after the key e.g. profile/******abcd. + return $value . $this->mask_string( str_replace( $value, '', $endpoint ) ); + + } + + // Just return the endpoint, as it doesn't need masking. + return $endpoint; + + } + /** * Helper method to mask values for specific keys in an array. * * @since 2.0.0 * - * @param string $str String to mask. - * @return string Masked string + * @param array $params Array of parameters to mask. + * @return array Array of parameters with masked values where applicable */ private function mask_params( $params ) { + // Keys to mask values for. $keys = array( 'first_name', 'token', 'subscriber_code', - 'signed_subscriber_id', ); foreach ( $params as $key => $value ) { - // Skip if not a string + // Skip if not a string. if ( ! is_string( $value ) ) { continue; } // Skip if the key isn't one we need to mask the value of. - if ( ! in_array( $key, $keys ) ) { + if ( ! in_array( $key, $keys, true ) ) { continue; } diff --git a/tests/wpunit/APITest.php b/tests/wpunit/APITest.php index 9281713..bf89cbb 100644 --- a/tests/wpunit/APITest.php +++ b/tests/wpunit/APITest.php @@ -151,7 +151,6 @@ public function testLog() ); // Perform actions that will write sensitive data to the log file. - /* $api->form_subscribe( $_ENV['CONVERTKIT_API_FORM_ID'], $_ENV['CONVERTKIT_API_SUBSCRIBER_EMAIL'], @@ -160,7 +159,6 @@ public function testLog() 'last_name' => 'Last', ) ); - */ $api->profile($_ENV['CONVERTKIT_API_SIGNED_SUBSCRIBER_ID']); // Confirm the historical log.txt file has been deleted. @@ -174,8 +172,8 @@ public function testLog() // Confirm the contents of the log file have masked the email address, name and signed subscriber ID. $this->tester->openFile(CONVERTKIT_PLUGIN_PATH . '/log/log.txt'); - //$this->tester->seeInThisFile('API: form_subscribe(): [ form_id: ' . $_ENV['CONVERTKIT_API_FORM_ID'] . ', email: o****@n********.c**, first_name: ******Name ]'); - $this->tester->seeInThisFile('API: profile(): [ signed_subscriber_id: ********************'); + $this->tester->seeInThisFile('API: POST subscribers: {"email_address":"o****@n********.c**","first_name":"******Name","state":"active","fields":{"last_name":"Last"}}'); + $this->tester->seeInThisFile('API: GET profile/*****************************************'); $this->tester->dontSeeInThisFile($_ENV['CONVERTKIT_API_SUBSCRIBER_EMAIL']); $this->tester->dontSeeInThisFile('First Name'); $this->tester->dontSeeInThisFile($_ENV['CONVERTKIT_API_SIGNED_SUBSCRIBER_ID']); From 5b8837371fd3099db9491dc14cca4577c73a9cff Mon Sep 17 00:00:00 2001 From: Tim Carr Date: Mon, 29 Apr 2024 14:10:00 +0100 Subject: [PATCH 4/4] Fix failing test --- tests/wpunit/APINoDataTest.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/wpunit/APINoDataTest.php b/tests/wpunit/APINoDataTest.php index 41c2285..4853ead 100644 --- a/tests/wpunit/APINoDataTest.php +++ b/tests/wpunit/APINoDataTest.php @@ -238,7 +238,9 @@ public function testGetAllPostsNoData() public function testGetProducts() { $result = $this->api->get_products(); - $this->assertNoData($result, 'products'); + $this->assertNotInstanceOf(WP_Error::class, $result); + $this->assertIsArray($result); + $this->assertCount(0, $result); } /**