Try and catch concurrency errors (#3233)

* Try and catch concurrency errors when creating new short urls, updating click count and logging redirects
* Code refactoring
* Couple more tests while at it

Fixes #2538
Fixes #765
Fixes #1974
Fixes #2829

4 files changed, 152 insertions, 102 deletions

diff --git a/includes/functions-shorturls.php b/includes/functions-shorturls.php
index 87e46f64..cbff13ed 100644
--- a/includes/functions-shorturls.php
+++ b/includes/functions-shorturls.php
@@ -12,14 +12,17 @@
  *    status: string, 'success' or 'fail'
  *    message: string, a descriptive localized message of what happened in any case
  *    code: string, a short descriptivish and untranslated message describing what happened
- *
- * Depending on the operation, it will contain any of the following keys:
  *    errorCode: string, a HTTP status code
- *    url: array, the short URL creation information, with the following keys: 'keyword', 'url', 'title', 'date', 'ip'
+ *    statusCode: string, a HTTP status code
+ * Depending on the operation, it will contain any of the following keys:
+ *    url: array, the short URL creation information, with keys: 'keyword', 'url', 'title', 'date', 'ip', 'clicks'
  *    title: string, the URL title
  *    shorturl: string, the proper short URL in full (eg 'http://sho.rt/abc')
  *    html: string, the HTML part used by the ajax to update the page display if any
  *
+ * For compatibility with early consumers and third parties, when people asked for various data and data formats
+ * before the internal API was really structured, the return array now collects several redundant information.
+ *
  * @param  string $url      URL to shorten
  * @param  string $keyword  optional "keyword"
  * @param  string $title    option title
@@ -28,15 +31,30 @@
 function yourls_add_new_link( $url, $keyword = '', $title = '' ) {
     // Allow plugins to short-circuit the whole function
     $pre = yourls_apply_filter( 'shunt_add_new_link', false, $url, $keyword, $title );
-    if ( false !== $pre )
+    if ( false !== $pre ) {
         return $pre;
+    }
 
+    /**
+     * The result array.
+     */
+    $return = [
+        // Always present :
+        'status' => '',
+        'code'   => '',
+        'message' => '',
+        'errorCode' => '',
+        'statusCode' => '',
+    ];
+
+    // Sanitize URL
     $url = yourls_sanitize_url( $url );
     if ( !$url || $url == 'http://' || $url == 'https://' ) {
         $return['status']    = 'fail';
         $return['code']      = 'error:nourl';
         $return['message']   = yourls__( 'Missing or malformed URL' );
-        $return['errorCode'] = '400';
+        $return['errorCode'] = $return['statusCode'] = '400'; // 400 Bad Request
+
         return yourls_apply_filter( 'add_new_link_fail_nourl', $return, $url, $keyword, $title );
     }
 
@@ -45,106 +63,104 @@ function yourls_add_new_link( $url, $keyword = '', $title = '' ) {
     yourls_check_IP_flood( $ip );
 
     // Prevent internal redirection loops: cannot shorten a shortened URL
-    if( yourls_get_relative_url( $url ) ) {
-        if( yourls_is_shorturl( $url ) ) {
-            $return['status']    = 'fail';
-            $return['code']      = 'error:noloop';
-            $return['message']   = yourls__( 'URL is a short URL' );
-            $return['errorCode'] = '400';
-            return yourls_apply_filter( 'add_new_link_fail_noloop', $return, $url, $keyword, $title );
-        }
+    if (yourls_is_shorturl($url)) {
+        $return['status']    = 'fail';
+        $return['code']      = 'error:noloop';
+        $return['message']   = yourls__( 'URL is a short URL' );
+        $return['errorCode'] = $return['statusCode'] = '400'; // 400 Bad Request
+        return yourls_apply_filter( 'add_new_link_fail_noloop', $return, $url, $keyword, $title );
     }
 
     yourls_do_action( 'pre_add_new_link', $url, $keyword, $title );
 
-    $return = array();
+    // Check if URL was already stored and we don't accept duplicates
+    if ( !yourls_allow_duplicate_longurls() && ($url_exists = yourls_long_url_exists( $url )) ) {
+        yourls_do_action( 'add_new_link_already_stored', $url, $keyword, $title );
 
-    // duplicates allowed or new URL => store it
-    if( yourls_allow_duplicate_longurls() || !( $url_exists = yourls_long_url_exists( $url ) ) ) {
+        $return['status']   = 'fail';
+        $return['code']     = 'error:url';
+        $return['url']      = array( 'keyword' => $url_exists->keyword, 'url' => $url, 'title' => $url_exists->title, 'date' => $url_exists->timestamp, 'ip' => $url_exists->ip, 'clicks' => $url_exists->clicks );
+        $return['message']  = /* //translators: eg "http://someurl/ already exists" */ yourls_s( '%s already exists in database', yourls_trim_long_string( $url ) );
+        $return['title']    = $url_exists->title;
+        $return['shorturl'] = yourls_link($url_exists->keyword);
 
-        if( isset( $title ) && !empty( $title ) ) {
-            $title = yourls_sanitize_title( $title );
-        } else {
-            $title = yourls_get_remote_title( $url );
-        }
-        $title = yourls_apply_filter( 'add_new_title', $title, $url, $keyword );
-
-        // Custom keyword provided
-        if ( $keyword ) {
-
-            yourls_do_action( 'add_new_link_custom_keyword', $url, $keyword, $title );
-
-            $keyword = yourls_sanitize_keyword( $keyword, true );
-            $keyword = yourls_apply_filter( 'custom_keyword', $keyword, $url, $title );
-
-            if ( !yourls_keyword_is_free( $keyword ) ) {
-                // This shorturl either reserved or taken already
-                $return['status']  = 'fail';
-                $return['code']    = 'error:keyword';
-                $return['message'] = yourls_s( 'Short URL %s already exists in database or is reserved', $keyword );
-            } else {
-                // all clear, store !
-                yourls_insert_link_in_db( $url, $keyword, $title );
-                $return['url']      = array('keyword' => $keyword, 'url' => $url, 'title' => $title, 'date' => date('Y-m-d H:i:s'), 'ip' => $ip );
-                $return['status']   = 'success';
-                $return['message']  = /* //translators: eg "http://someurl/ added to DB" */ yourls_s( '%s added to database', yourls_trim_long_string( $url ) );
-                $return['title']    = $title;
-                $return['html']     = yourls_table_add_row( $keyword, $url, $title, $ip, 0, time() );
-                $return['shorturl'] = yourls_link($keyword);
-            }
+        return yourls_apply_filter( 'add_new_link_already_stored_filter', $return, $url, $keyword, $title );
+    }
 
-        // Create random keyword
-        } else {
+    // Sanitize provided title, or fetch one
+    if( isset( $title ) && !empty( $title ) ) {
+        $title = yourls_sanitize_title( $title );
+    } else {
+        $title = yourls_get_remote_title( $url );
+    }
+    $title = yourls_apply_filter( 'add_new_title', $title, $url, $keyword );
+
+    // Custom keyword provided : sanitize and make sure it's free
+    if ($keyword) {
+        yourls_do_action( 'add_new_link_custom_keyword', $url, $keyword, $title );
+
+        $keyword = yourls_sanitize_keyword( $keyword, true );
+        $keyword = yourls_apply_filter( 'custom_keyword', $keyword, $url, $title );
 
-            yourls_do_action( 'add_new_link_create_keyword', $url, $keyword, $title );
-
-            $timestamp = date( 'Y-m-d H:i:s' );
-            $id = yourls_get_next_decimal();
-            $ok = false;
-            do {
-                $keyword = yourls_int2string( $id );
-                $keyword = yourls_apply_filter( 'random_keyword', $keyword, $url, $title );
-                if ( yourls_keyword_is_free($keyword) ) {
-                    if (yourls_insert_link_in_db( $url, $keyword, $title )){
-                        // everything ok, populate needed vars
-                        $return['url']      = array('keyword' => $keyword, 'url' => $url, 'title' => $title, 'date' => $timestamp, 'ip' => $ip );
-                        $return['status']   = 'success';
-                        $return['message']  = /* //translators: eg "http://someurl/ added to DB" */ yourls_s( '%s added to database', yourls_trim_long_string( $url ) );
-                        $return['title']    = $title;
-                        $return['html']     = yourls_table_add_row( $keyword, $url, $title, $ip, 0, time() );
-                        $return['shorturl'] = yourls_link($keyword);
-                    } else {
-                        // database error, couldnt store result
-                        $return['status']   = 'fail';
-                        $return['code']     = 'error:db';
-                        $return['message']  = yourls_s( 'Error saving url to database' );
-                    }
-                    $ok = true;
-                }
-                $id++;
-            } while ( !$ok );
-            @yourls_update_next_decimal( $id );
+        if ( !yourls_keyword_is_free( $keyword ) ) {
+            // This shorturl either reserved or taken already
+            $return['status']  = 'fail';
+            $return['code']    = 'error:keyword';
+            $return['message'] = yourls_s( 'Short URL %s already exists in database or is reserved', $keyword );
+            $return['errorCode'] = $return['statusCode'] = '400'; // 400 Bad Request
+
+            return yourls_apply_filter( 'add_new_link_keyword_exists', $return, $url, $keyword, $title );
         }
 
-    // URL was already stored
+        // Create random keyword
     } else {
+        yourls_do_action( 'add_new_link_create_keyword', $url, $keyword, $title );
 
-        yourls_do_action( 'add_new_link_already_stored', $url, $keyword, $title );
+        $id = yourls_get_next_decimal();
 
-        $return['status']   = 'fail';
-        $return['code']     = 'error:url';
-        $return['url']      = array( 'keyword' => $url_exists->keyword, 'url' => $url, 'title' => $url_exists->title, 'date' => $url_exists->timestamp, 'ip' => $url_exists->ip, 'clicks' => $url_exists->clicks );
-        $return['message']  = /* //translators: eg "http://someurl/ already exists" */ yourls_s( '%s already exists in database', yourls_trim_long_string( $url ) );
-        $return['title']    = $url_exists->title;
-        $return['shorturl'] = yourls_link($url_exists->keyword);
+        do {
+            $keyword = yourls_int2string( $id );
+            $keyword = yourls_apply_filter( 'random_keyword', $keyword, $url, $title );
+            $id++;
+        } while ( !yourls_keyword_is_free($keyword) );
+
+        yourls_update_next_decimal($id);
+    }
+
+    // We should be all set now. Store the short URL !
+
+    $timestamp = date( 'Y-m-d H:i:s' );
+
+    try {
+        if (yourls_insert_link_in_db( $url, $keyword, $title )){
+            // everything ok, populate needed vars
+            $return['url']      = array('keyword' => $keyword, 'url' => $url, 'title' => $title, 'date' => $timestamp, 'ip' => $ip );
+            $return['status']   = 'success';
+            $return['message']  = /* //translators: eg "http://someurl/ added to DB" */ yourls_s( '%s added to database', yourls_trim_long_string( $url ) );
+            $return['title']    = $title;
+            $return['html']     = yourls_table_add_row( $keyword, $url, $title, $ip, 0, time() );
+            $return['shorturl'] = yourls_link($keyword);
+            $return['statusCode'] = 200; // 200 OK
+        } else {
+            // unknown database error, couldn't store result
+            $return['status']   = 'fail';
+            $return['code']     = 'error:db';
+            $return['message']  = yourls_s( 'Error saving url to database' );
+            $return['errorCode'] = $return['statusCode'] = '500'; // 500 Internal Server Error
+        }
+    } catch (Exception $e) {
+        // Keyword supposed to be free but the INSERT caused an exception: most likely we're facing a
+        // concurrency problem. See Issue 2538.
+        $return['status']  = 'fail';
+        $return['code']    = 'error:concurrency';
+        $return['message'] = $e->getMessage();
+        $return['errorCode'] = $return['statusCode'] = '503'; // 503 Service Unavailable
     }
 
     yourls_do_action( 'post_add_new_link', $url, $keyword, $title, $return );
 
-    $return['statusCode'] = 200; // regardless of result, this is still a valid request
     return yourls_apply_filter( 'add_new_link', $return, $url, $keyword, $title );
 }
-
 /**
  * Determine the allowed character set in short URLs
  *
@@ -544,7 +560,6 @@ function yourls_get_keyword_stats( $shorturl ) {
     $shorturl  = yourls_sanitize_keyword( $shorturl );
 
     $res = yourls_get_db()->fetchObject("SELECT * FROM `$table_url` WHERE `keyword` = :keyword", array('keyword' => $shorturl));
-    $return = array();
 
     if( !$res ) {
         // non existent link
diff --git a/includes/functions.php b/includes/functions.php
index 06f75f86..7b0a34b7 100644
--- a/includes/functions.php
+++ b/includes/functions.php
@@ -53,7 +53,7 @@ function yourls_get_next_decimal() {
  *
  * Note: this function relies upon yourls_update_option(), which will return either true or false
  * depending if there has been an actual MySQL query updating the DB.
- * In other words, this function may return false yet this would not mean it has functionnaly failed
+ * In other words, this function may return false yet this would not mean it has functionally failed
  * In other words I'm not sure we really need this function to return something :face_with_eyes_looking_up:
  * See issue 2621 for more on this.
  *
@@ -86,18 +86,30 @@ function yourls_xml_encode( $array ) {
 function yourls_update_clicks( $keyword, $clicks = false ) {
 	// Allow plugins to short-circuit the whole function
 	$pre = yourls_apply_filter( 'shunt_update_clicks', false, $keyword, $clicks );
-	if ( false !== $pre )
-		return $pre;
+	if ( false !== $pre ) {
+        return $pre;
+    }
 
 	$keyword = yourls_sanitize_keyword( $keyword );
 	$table = YOURLS_DB_TABLE_URL;
-	if ( $clicks !== false && is_int( $clicks ) && $clicks >= 0 )
-		$update = yourls_get_db()->fetchAffected( "UPDATE `$table` SET `clicks` = :clicks WHERE `keyword` = :keyword", [ 'clicks' => $clicks, 'keyword' => $keyword ] );
-	else
-		$update = yourls_get_db()->fetchAffected( "UPDATE `$table` SET `clicks` = clicks + 1 WHERE `keyword` = :keyword", [ 'keyword' => $keyword ] );
+	if ( $clicks !== false && is_int( $clicks ) && $clicks >= 0 ) {
+        $update = "UPDATE `$table` SET `clicks` = :clicks WHERE `keyword` = :keyword";
+        $values = [ 'clicks' => $clicks, 'keyword' => $keyword ];
+    } else {
+        $update = "UPDATE `$table` SET `clicks` = clicks + 1 WHERE `keyword` = :keyword";
+        $values = [ 'keyword' => $keyword ];
+    }
 
-	yourls_do_action( 'update_clicks', $keyword, $update, $clicks );
-	return $update;
+	// Try and update click count. An error probably means a concurrency problem : just skip the update
+    try {
+        $result = yourls_get_db()->fetchAffected($update, $values);
+    } catch (Exception $e) {
+	    $result = 0;
+    }
+
+	yourls_do_action( 'update_clicks', $keyword, $result, $clicks );
+
+	return $result;
 }
 
 /**
@@ -249,7 +261,7 @@ function yourls_redirect( $location, $code = 301 ) {
 function yourls_redirect_shorturl($url, $keyword) {
     yourls_do_action( 'redirect_shorturl', $url, $keyword );
 
-    // Update click count in main table
+    // Attempt to update click count in main table
     yourls_update_clicks( $keyword );
 
     // Update detailed log for stats
@@ -449,11 +461,13 @@ function yourls_get_HTTP_status( $code ) {
 function yourls_log_redirect( $keyword ) {
 	// Allow plugins to short-circuit the whole function
 	$pre = yourls_apply_filter( 'shunt_log_redirect', false, $keyword );
-	if ( false !== $pre )
-		return $pre;
+	if ( false !== $pre ) {
+        return $pre;
+    }
 
-	if ( !yourls_do_log_redirect() )
-		return true;
+	if (!yourls_do_log_redirect()) {
+        return true;
+    }
 
 	$table = YOURLS_DB_TABLE_LOG;
     $ip = yourls_get_IP();
@@ -466,7 +480,14 @@ function yourls_log_redirect( $keyword ) {
         'location' => yourls_geo_ip_to_countrycode($ip),
     ];
 
-    return yourls_get_db()->fetchAffected("INSERT INTO `$table` (click_time, shorturl, referrer, user_agent, ip_address, country_code) VALUES (:now, :keyword, :referrer, :ua, :ip, :location)", $binds );
+    // Try and log. An error probably means a concurrency problem : just skip the logging
+    try {
+        $result = yourls_get_db()->fetchAffected("INSERT INTO `$table` (click_time, shorturl, referrer, user_agent, ip_address, country_code) VALUES (:now, :keyword, :referrer, :ua, :ip, :location)", $binds );
+    } catch (Exception $e) {
+        $result = 0;
+    }
+
+    return $result;
 }
 
 /**
diff --git a/tests/tests/pages/pages.php b/tests/tests/pages/pages.php
index 0c9281af..54f20050 100644
--- a/tests/tests/pages/pages.php
+++ b/tests/tests/pages/pages.php
@@ -12,10 +12,19 @@ class Pages_Tests extends PHPUnit\Framework\TestCase {
         $this->assertTrue( yourls_keyword_is_reserved('examplepage') );
     }
 
+    public function test_examplepage() {
+        $this->assertTrue(yourls_is_page('examplepage'));
+    }
+
+    public function test_no_page() {
+        $this->assertFalse(yourls_is_page(rand_str()));
+    }
+
     public function test_create_page_and_check_is_reserved() {
         $page = rand_str();
         if( touch(YOURLS_PAGEDIR . "/$page.php") ) {
             $this->assertTrue( yourls_keyword_is_reserved($page) );
+            $this->assertTrue( yourls_is_page($page) );
             unlink(YOURLS_PAGEDIR . "/$page.php");
         } else {
             $this->markTestSkipped( "Cannot create 'pages/$page'" );
diff --git a/tests/tests/shorturl/misc.php b/tests/tests/shorturl/misc.php
index 16307a4a..aad8e94e 100644
--- a/tests/tests/shorturl/misc.php
+++ b/tests/tests/shorturl/misc.php
@@ -36,4 +36,9 @@ class ShortURL_Misc_Tests extends PHPUnit\Framework\TestCase {
         $this->assertEquals( 0, yourls_get_keyword_clicks( $rand ) );
     }
 
+    public function test_get_shorturl_charset() {
+        $this->assertIsString(yourls_get_shorturl_charset());
+        $this->assertNotEmpty(yourls_get_shorturl_charset());
+    }
+
 }