From 93a2b97d697b5ccbbe41e4600b81a3cfa9f41160 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Wed, 23 Oct 2024 08:17:13 +0200 Subject: [PATCH 1/5] add failing test case, dedup code --- tst/Bootstrap.php | 11 +++++++++ tst/Data/FilesystemTest.php | 4 +--- tst/RequestTest.php | 48 +++++++++++++++++++++---------------- 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/tst/Bootstrap.php b/tst/Bootstrap.php index 28b56422..8d8e61c6 100644 --- a/tst/Bootstrap.php +++ b/tst/Bootstrap.php @@ -238,6 +238,17 @@ class Helper return json_encode(self::getCommentPost()); } + /** + * Returns 16 random hexadecimal characters. + * + * @return string + */ + public static function getRandomId() + { + // 8 binary bytes are 16 characters long in hex + return bin2hex(random_bytes(8)); + } + /** * delete directory and all its contents recursively * diff --git a/tst/Data/FilesystemTest.php b/tst/Data/FilesystemTest.php index 390cb66d..1e4f2b93 100644 --- a/tst/Data/FilesystemTest.php +++ b/tst/Data/FilesystemTest.php @@ -141,9 +141,7 @@ class FilesystemTest extends TestCase $commentid = Helper::getCommentId(); $ids = array(); for ($i = 0, $max = 10; $i < $max; ++$i) { - // PHPs mt_rand only supports 32 bit or up 0x7fffffff on 64 bit systems to be precise :-/ - $dataid = str_pad(dechex(mt_rand(0, mt_getrandmax())), 8, '0', STR_PAD_LEFT) . - str_pad(dechex(mt_rand(0, mt_getrandmax())), 8, '0', STR_PAD_LEFT); + $dataid = Helper::getRandomId(); $storagedir = $this->_path . DIRECTORY_SEPARATOR . substr($dataid, 0, 2) . DIRECTORY_SEPARATOR . substr($dataid, 2, 2) . DIRECTORY_SEPARATOR; $ids[$dataid] = $storagedir; diff --git a/tst/RequestTest.php b/tst/RequestTest.php index 2207fa7e..eb22655f 100644 --- a/tst/RequestTest.php +++ b/tst/RequestTest.php @@ -12,18 +12,6 @@ class RequestTest extends TestCase $_POST = array(); } - /** - * Returns 16 random hexadecimal characters. - * - * @access public - * @return string - */ - public function getRandomId() - { - // 8 binary bytes are 16 characters long in hex - return bin2hex(random_bytes(8)); - } - /** * Returns random query safe characters. * @@ -54,7 +42,25 @@ class RequestTest extends TestCase public function testRead() { $this->reset(); - $id = $this->getRandomId(); + $id = Helper::getRandomId(); + $_SERVER['REQUEST_METHOD'] = 'GET'; + $_SERVER['QUERY_STRING'] = $id; + $_GET[$id] = ''; + $request = new Request; + $this->assertFalse($request->isJsonApiCall(), 'is HTML call'); + $this->assertEquals($id, $request->getParam('pasteid')); + $this->assertEquals('read', $request->getOperation()); + } + + /** + * paste IDs are 8 bytes hex encoded strings, if unlucky, this turns into + * a numeric string that PHP will cast to an int, for example in array keys + * @see https://www.php.net/manual/en/language.types.array.php + */ + public function testReadNumeric() + { + $this->reset(); + $id = '1234567812345678'; $_SERVER['REQUEST_METHOD'] = 'GET'; $_SERVER['QUERY_STRING'] = $id; $_GET[$id] = ''; @@ -67,7 +73,7 @@ class RequestTest extends TestCase public function testDelete() { $this->reset(); - $id = $this->getRandomId(); + $id = Helper::getRandomId(); $_SERVER['REQUEST_METHOD'] = 'GET'; $_GET['pasteid'] = $id; $_GET['deletetoken'] = 'bar'; @@ -110,7 +116,7 @@ class RequestTest extends TestCase public function testApiRead() { $this->reset(); - $id = $this->getRandomId(); + $id = Helper::getRandomId(); $_SERVER['REQUEST_METHOD'] = 'GET'; $_SERVER['HTTP_ACCEPT'] = 'application/json, text/javascript, */*; q=0.01'; $_SERVER['QUERY_STRING'] = $id; @@ -124,7 +130,7 @@ class RequestTest extends TestCase public function testApiDelete() { $this->reset(); - $id = $this->getRandomId(); + $id = Helper::getRandomId(); $_SERVER['REQUEST_METHOD'] = 'POST'; $_SERVER['HTTP_X_REQUESTED_WITH'] = 'JSONHttpRequest'; $_SERVER['QUERY_STRING'] = $id; @@ -155,7 +161,7 @@ class RequestTest extends TestCase public function testReadWithNegotiation() { $this->reset(); - $id = $this->getRandomId(); + $id = Helper::getRandomId(); $_SERVER['REQUEST_METHOD'] = 'GET'; $_SERVER['HTTP_ACCEPT'] = 'text/html,text/html; charset=UTF-8,application/xhtml+xml, application/xml;q=0.9,*/*;q=0.8, text/csv,application/json'; $_SERVER['QUERY_STRING'] = $id; @@ -169,7 +175,7 @@ class RequestTest extends TestCase public function testReadWithXhtmlNegotiation() { $this->reset(); - $id = $this->getRandomId(); + $id = Helper::getRandomId(); $_SERVER['REQUEST_METHOD'] = 'GET'; $_SERVER['HTTP_ACCEPT'] = 'application/xhtml+xml,text/html,text/html; charset=UTF-8, application/xml;q=0.9,*/*;q=0.8, text/csv,application/json'; $_SERVER['QUERY_STRING'] = $id; @@ -183,7 +189,7 @@ class RequestTest extends TestCase public function testApiReadWithNegotiation() { $this->reset(); - $id = $this->getRandomId(); + $id = Helper::getRandomId(); $_SERVER['REQUEST_METHOD'] = 'GET'; $_SERVER['HTTP_ACCEPT'] = 'text/plain,text/csv, application/xml;q=0.9, application/json, text/html,text/html; charset=UTF-8,application/xhtml+xml, */*;q=0.8'; $_SERVER['QUERY_STRING'] = $id; @@ -197,7 +203,7 @@ class RequestTest extends TestCase public function testReadWithFailedNegotiation() { $this->reset(); - $id = $this->getRandomId(); + $id = Helper::getRandomId(); $_SERVER['REQUEST_METHOD'] = 'GET'; $_SERVER['HTTP_ACCEPT'] = 'text/plain,text/csv, application/xml;q=0.9, */*;q=0.8'; $_SERVER['QUERY_STRING'] = $id; @@ -211,7 +217,7 @@ class RequestTest extends TestCase public function testPasteIdExtraction() { $this->reset(); - $id = $this->getRandomId(); + $id = Helper::getRandomId(); $queryParams = array($id); $queryParamCount = random_int(1, 5); for ($i = 0; $i < $queryParamCount; ++$i) { From cf83e3825fe7cdb8c1bf3260c8ee36c1730556d7 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Wed, 23 Oct 2024 08:23:02 +0200 Subject: [PATCH 2/5] ensure key is cast to string, fixes #1435 --- CHANGELOG.md | 1 + lib/Request.php | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3f8cc01..377741f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * CHANGED: Simpler PostgreSQL table lookup query (#1361) * CHANGED: SRI hashes are now configurable, no longer hardcoded in templates (#1365) * CHANGED: Upgrading libraries to: DOMpurify 3.1.7 +* FIXED: Numeric array keys being cast to integer causing failures under strict type checking (#1435) ## 1.7.4 (2024-07-09) * CHANGED: Saving markdown pastes uses `.md` extension instead of `.txt` (#1293) diff --git a/lib/Request.php b/lib/Request.php index 336b69f1..4e6d1f1a 100644 --- a/lib/Request.php +++ b/lib/Request.php @@ -83,6 +83,7 @@ class Request { foreach ($_GET as $key => $value) { // only return if value is empty and key is 16 hex chars + $key = (string) $key; if (($value === '') && strlen($key) === 16 && ctype_xdigit($key)) { return $key; } From e468f076265ee323ca1027a93a2e2b35fc4ce102 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Fri, 25 Oct 2024 06:58:05 +0200 Subject: [PATCH 3/5] avoid clever key manipulation, all we need are incremental numbers starting at 1 --- lib/Data/Database.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/Data/Database.php b/lib/Data/Database.php index 3837e6ff..2fe59033 100644 --- a/lib/Data/Database.php +++ b/lib/Data/Database.php @@ -506,8 +506,8 @@ class Database extends AbstractData private function _exec($sql, array $params) { $statement = $this->_db->prepare($sql); - foreach ($params as $key => &$parameter) { - $position = $key + 1; + $position = 1; + foreach ($params as &$parameter) { if (is_int($parameter)) { $statement->bindParam($position, $parameter, PDO::PARAM_INT); } elseif (is_string($parameter) && strlen($parameter) >= 4000) { @@ -515,6 +515,7 @@ class Database extends AbstractData } else { $statement->bindParam($position, $parameter); } + ++$position; } $result = $statement->execute(); $statement->closeCursor(); From 0268e01ab5b03c9eca18376fabe023dd57b292c9 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Fri, 25 Oct 2024 07:09:13 +0200 Subject: [PATCH 4/5] experiment: add return types to a unit test facility --- tst/Bootstrap.php | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/tst/Bootstrap.php b/tst/Bootstrap.php index 8d8e61c6..8e9e8cda 100644 --- a/tst/Bootstrap.php +++ b/tst/Bootstrap.php @@ -116,7 +116,7 @@ class Helper * * @return string */ - public static function getPasteId() + public static function getPasteId(): string { return self::$pasteid; } @@ -128,7 +128,7 @@ class Helper * @param array $meta * @return array */ - public static function getPaste($version = 2, array $meta = array()) + public static function getPaste($version = 2, array $meta = array()): array { $example = self::getPasteWithAttachment($version, $meta); // v1 has the attachment stored in a separate property @@ -145,7 +145,7 @@ class Helper * @param array $meta * @return array */ - public static function getPasteWithAttachment($version = 2, array $meta = array()) + public static function getPasteWithAttachment($version = 2, array $meta = array()): array { $example = $version === 1 ? self::$pasteV1 : self::$pasteV2; $example['meta']['salt'] = ServerSalt::generate(); @@ -160,7 +160,7 @@ class Helper * @param array $meta * @return array */ - public static function getPastePost($version = 2, array $meta = array()) + public static function getPastePost($version = 2, array $meta = array()): array { $example = self::getPaste($version, $meta); if ($version == 2) { @@ -176,9 +176,9 @@ class Helper * * @param int $version * @param array $meta - * @return array + * @return string */ - public static function getPasteJson($version = 2, array $meta = array()) + public static function getPasteJson($version = 2, array $meta = array()): string { return json_encode(self::getPastePost($version, $meta)); } @@ -188,7 +188,7 @@ class Helper * * @return string */ - public static function getCommentId() + public static function getCommentId(): string { return self::$commentid; } @@ -200,7 +200,7 @@ class Helper * @param array $meta * @return array */ - public static function getComment($version = 2, array $meta = array()) + public static function getComment($version = 2, array $meta = array()): array { $example = $version === 1 ? self::$commentV1 : self::$pasteV2; if ($version === 2) { @@ -220,7 +220,7 @@ class Helper * @param int $version * @return array */ - public static function getCommentPost() + public static function getCommentPost(): array { $example = self::getComment(); unset($example['meta']); @@ -231,9 +231,9 @@ class Helper * get example comment, as received via POST by user * * @param int $version - * @return array + * @return string */ - public static function getCommentJson() + public static function getCommentJson(): string { return json_encode(self::getCommentPost()); } @@ -243,7 +243,7 @@ class Helper * * @return string */ - public static function getRandomId() + public static function getRandomId(): string { // 8 binary bytes are 16 characters long in hex return bin2hex(random_bytes(8)); @@ -254,8 +254,9 @@ class Helper * * @param string $path * @throws Exception + * @return void */ - public static function rmDir($path) + public static function rmDir($path): void { if (is_dir($path)) { $path .= DIRECTORY_SEPARATOR; @@ -283,7 +284,7 @@ class Helper * * @return void */ - public static function confBackup() + public static function confBackup(): void { if (!is_file(CONF . '.bak') && is_file(CONF)) { rename(CONF, CONF . '.bak'); @@ -298,7 +299,7 @@ class Helper * * @return void */ - public static function confRestore() + public static function confRestore(): void { if (is_file(CONF . '.bak')) { rename(CONF . '.bak', CONF); @@ -314,7 +315,7 @@ class Helper * @param string $pathToFile * @param array $values */ - public static function createIniFile($pathToFile, array $values) + public static function createIniFile($pathToFile, array $values): void { if (count($values)) { @unlink($pathToFile); @@ -357,7 +358,7 @@ class Helper * @param bool $return * @return void|string */ - public static function varExportMin($var, $return = false) + public static function varExportMin($var, $return = false): string { if (is_array($var)) { $toImplode = array(); @@ -380,7 +381,7 @@ class Helper * * @return void */ - public static function updateSubresourceIntegrity() + public static function updateSubresourceIntegrity(): void { foreach (new GlobIterator(PATH . 'js' . DIRECTORY_SEPARATOR . '*.js') as $file) { if ($file->getBasename() == 'common.js') { From 8752354d633d9e70e75d0aec019e0491d735d74e Mon Sep 17 00:00:00 2001 From: El RIDO Date: Fri, 25 Oct 2024 07:12:30 +0200 Subject: [PATCH 5/5] apply StyleCI fixes --- lib/Data/Database.php | 2 +- tst/Data/FilesystemTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Data/Database.php b/lib/Data/Database.php index 2fe59033..f9e0b8b5 100644 --- a/lib/Data/Database.php +++ b/lib/Data/Database.php @@ -506,7 +506,7 @@ class Database extends AbstractData private function _exec($sql, array $params) { $statement = $this->_db->prepare($sql); - $position = 1; + $position = 1; foreach ($params as &$parameter) { if (is_int($parameter)) { $statement->bindParam($position, $parameter, PDO::PARAM_INT); diff --git a/tst/Data/FilesystemTest.php b/tst/Data/FilesystemTest.php index 1e4f2b93..99966c4c 100644 --- a/tst/Data/FilesystemTest.php +++ b/tst/Data/FilesystemTest.php @@ -141,7 +141,7 @@ class FilesystemTest extends TestCase $commentid = Helper::getCommentId(); $ids = array(); for ($i = 0, $max = 10; $i < $max; ++$i) { - $dataid = Helper::getRandomId(); + $dataid = Helper::getRandomId(); $storagedir = $this->_path . DIRECTORY_SEPARATOR . substr($dataid, 0, 2) . DIRECTORY_SEPARATOR . substr($dataid, 2, 2) . DIRECTORY_SEPARATOR; $ids[$dataid] = $storagedir;