Merge pull request #338 from virtualtam/fix/unique-uniqid

Session ID: extend the regex to match possible hash representations

application/Utils.php

@@ -140,11 +140,16 @@ function checkPHPVersion($minVersion, $curVersion)
 
 /**
  * Validate session ID to prevent Full Path Disclosure.
+ *
  * See #298.
+ * The session ID's format depends on the hash algorithm set in PHP settings
  *
  * @param string $sessionId Session ID
  *
  * @return true if valid, false otherwise.
+ *
+ * @see http://php.net/manual/en/function.hash-algos.php
+ * @see http://php.net/manual/en/session.configuration.php
  */
 function is_session_id_valid($sessionId)
 {
@@ -156,7 +161,7 @@ function is_session_id_valid($sessionId)
         return false;
     }
 
-    if (!preg_match('/^[a-z0-9]{2,32}$/i', $sessionId)) {
+    if (!preg_match('/^[a-zA-Z0-9,-]{2,128}$/', $sessionId)) {
         return false;
     }
 

index.php

@@ -92,16 +92,18 @@ ini_set('session.use_only_cookies', 1);
 // Prevent PHP form using sessionID in URL if cookies are disabled.
 ini_set('session.use_trans_sid', false);
 
-// Regenerate session id if invalid or not defined in cookie.
-if (isset($_COOKIE['shaarli']) && !is_session_id_valid($_COOKIE['shaarli'])) {
-    $_COOKIE['shaarli'] = uniqid();
-}
 session_name('shaarli');
 // Start session if needed (Some server auto-start sessions).
 if (session_id() == '') {
     session_start();
 }
 
+// Regenerate session ID if invalid or not defined in cookie.
+if (isset($_COOKIE['shaarli']) && !is_session_id_valid($_COOKIE['shaarli'])) {
+    session_regenerate_id(true);
+    $_COOKIE['shaarli'] = session_id();
+}
+
 include "inc/rain.tpl.class.php"; //include Rain TPL
 raintpl::$tpl_dir = $GLOBALS['config']['RAINTPL_TPL']; // template directory
 raintpl::$cache_dir = $GLOBALS['config']['RAINTPL_TMP']; // cache directory

tests/UtilsTest.php

@@ -4,12 +4,28 @@
  */
 
 require_once 'application/Utils.php';
+require_once 'tests/utils/ReferenceSessionIdHashes.php';
+
+// Initialize reference data before PHPUnit starts a session
+ReferenceSessionIdHashes::genAllHashes();
+
 
 /**
  * Unitary tests for Shaarli utilities
  */
 class UtilsTest extends PHPUnit_Framework_TestCase
 {
+    // Session ID hashes
+    protected static $sidHashes = null;
+
+    /**
+     * Assign reference data
+     */
+    public static function setUpBeforeClass()
+    {
+        self::$sidHashes = ReferenceSessionIdHashes::getHashes();
+    }
+
     /**
      * Represent a link by its hash
      */
@@ -152,11 +168,41 @@ class UtilsTest extends PHPUnit_Framework_TestCase
     }
 
     /**
-     * Test is_session_id_valid with a valid ID.
+     * Test is_session_id_valid with a valid ID - TEST ALL THE HASHES!
+     *
+     * This tests extensively covers all hash algorithms / bit representations
      */
-    public function testIsSessionIdValid()
+    public function testIsAnyHashSessionIdValid()
     {
-        $this->assertTrue(is_session_id_valid('azertyuiop123456789AZERTYUIOP1aA'));
+        foreach (self::$sidHashes as $algo => $bpcs) {
+            foreach ($bpcs as $bpc => $hash) {
+                $this->assertTrue(is_session_id_valid($hash));
+            }
+        }
+    }
+
+    /**
+     * Test is_session_id_valid with a valid ID - SHA-1 hashes
+     */
+    public function testIsSha1SessionIdValid()
+    {
+        $this->assertTrue(is_session_id_valid(sha1('shaarli')));
+    }
+
+    /**
+     * Test is_session_id_valid with a valid ID - SHA-256 hashes
+     */
+    public function testIsSha256SessionIdValid()
+    {
+        $this->assertTrue(is_session_id_valid(hash('sha256', 'shaarli')));
+    }
+
+    /**
+     * Test is_session_id_valid with a valid ID - SHA-512 hashes
+     */
+    public function testIsSha512SessionIdValid()
+    {
+        $this->assertTrue(is_session_id_valid(hash('sha512', 'shaarli')));
     }
 
     /**
@@ -166,6 +212,8 @@ class UtilsTest extends PHPUnit_Framework_TestCase
     {
         $this->assertFalse(is_session_id_valid(''));
         $this->assertFalse(is_session_id_valid(array()));
-        $this->assertFalse(is_session_id_valid('c0ZqcWF3VFE2NmJBdm1HMVQ0ZHJ3UmZPbTFsNGhkNHI='));
+        $this->assertFalse(
+            is_session_id_valid('c0ZqcWF3VFE2NmJBdm1HMVQ0ZHJ3UmZPbTFsNGhkNHI=')
+        );
     }
 }

tests/utils/ReferenceSessionIdHashes.php

@@ -0,0 +1,55 @@
+<?php
+/**
+ * Testing the untestable - Session ID generation
+ */
+class ReferenceSessionIdHashes
+{
+    // Session ID hashes
+    protected static $sidHashes = null;
+
+    /**
+     * Generates session ID hashes for all algorithms & bit representations
+     */
+    public static function genAllHashes()
+    {
+        foreach (hash_algos() as $algo) {
+            self::$sidHashes[$algo] = array();
+
+            foreach (array(4, 5, 6) as $bpc) {
+                self::$sidHashes[$algo][$bpc] = self::genSidHash($algo, $bpc);
+            }
+        }
+    }
+
+    /**
+     * Generates a session ID for a given hash algorithm and bit representation
+     *
+     * @param string $function           name of the hash function
+     * @param int    $bits_per_character representation type
+     *
+     * @return string the generated session ID
+     */
+    protected static function genSidHash($function, $bits_per_character)
+    {
+        if (session_id()) {
+            session_destroy();
+        }
+
+        ini_set('session.hash_function', $function);
+        ini_set('session.hash_bits_per_character', $bits_per_character);
+
+        session_start();
+        return session_id();
+    }
+
+    /**
+     * Returns the reference hash array
+     *
+     * @return array session IDs generated for all available algorithms and bit
+     *               representations
+     */
+    public static function getHashes()
+    {
+        return self::$sidHashes;
+    }
+}