From bb890834444e815a5710a27b0eba01c3fcec6f4f Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sat, 30 Jan 2021 21:37:19 -0500 Subject: [PATCH] Perform strict validation of query parameters This is in fact stricter than Miniflux, which ignores duplicate values and does not validate anything other than the string enumerations --- lib/Conf.php | 10 +--------- lib/Misc/ValueInfo.php | 12 ++++++++++++ lib/REST/Miniflux/V1.php | 41 ++++++++++++++++++++++++++++------------ locale/en.php | 1 + 4 files changed, 43 insertions(+), 21 deletions(-) diff --git a/lib/Conf.php b/lib/Conf.php index c6fd33c1..dfe35e29 100644 --- a/lib/Conf.php +++ b/lib/Conf.php @@ -113,14 +113,6 @@ class Conf { /** @var \DateInterval|null (OBSOLETE) Number of seconds for SQLite to wait before returning a timeout error when trying to acquire a write lock on the database (zero does not wait) */ public $dbSQLite3Timeout = null; // previously 60.0 - - protected const TYPE_NAMES = [ - Value::T_BOOL => "boolean", - Value::T_STRING => "string", - Value::T_FLOAT => "float", - VALUE::T_INT => "integer", - Value::T_INTERVAL => "interval", - ]; protected const EXPECTED_TYPES = [ 'dbTimeoutExec' => "double", 'dbTimeoutLock' => "double", @@ -318,7 +310,7 @@ class Conf { return $value; } catch (ExceptionType $e) { $type = $this->types[$key]['const'] & ~(Value::M_STRICT | Value::M_DROP | Value::M_NULL | Value::M_ARRAY); - throw new Conf\Exception("typeMismatch", ['param' => $key, 'type' => self::TYPE_NAMES[$type], 'file' => $file, 'nullable' => $nullable]); + throw new Conf\Exception("typeMismatch", ['param' => $key, 'type' => Value::TYPE_NAMES[$type], 'file' => $file, 'nullable' => $nullable]); } } diff --git a/lib/Misc/ValueInfo.php b/lib/Misc/ValueInfo.php index e9af5272..9977e786 100644 --- a/lib/Misc/ValueInfo.php +++ b/lib/Misc/ValueInfo.php @@ -35,6 +35,17 @@ class ValueInfo { public const M_DROP = 1 << 29; // drop the value (return null) if the type doesn't match public const M_STRICT = 1 << 30; // throw an exception if the type doesn't match public const M_ARRAY = 1 << 31; // the value should be a flat array of values of the specified type; indexed and associative are both acceptable + public const TYPE_NAMES = [ + self::T_MIXED => "mixed", + self::T_NULL => "null", + self::T_BOOL => "boolean", + self::T_INT => "integer", + self::T_FLOAT => "float", + self::T_DATE => "date", + self::T_STRING => "string", + self::T_ARRAY => "array", + self::T_INTERVAL => "interval", + ]; // symbolic date and time formats protected const DATE_FORMATS = [ // in out 'iso8601' => ["!Y-m-d\TH:i:s", "Y-m-d\TH:i:s\Z" ], // NOTE: ISO 8601 dates require special input processing because of varying formats for timezone offsets @@ -48,6 +59,7 @@ class ValueInfo { 'float' => ["U.u", "U.u" ], ]; + public static function normalize($value, int $type, string $dateInFormat = null, $dateOutFormat = null) { $allowNull = ($type & self::M_NULL); $strict = ($type & (self::M_STRICT | self::M_DROP)); diff --git a/lib/REST/Miniflux/V1.php b/lib/REST/Miniflux/V1.php index 17864bbc..c6761213 100644 --- a/lib/REST/Miniflux/V1.php +++ b/lib/REST/Miniflux/V1.php @@ -8,6 +8,7 @@ namespace JKingWeb\Arsse\REST\Miniflux; use JKingWeb\Arsse\Arsse; use JKingWeb\Arsse\Feed; +use JKingWeb\Arsse\ExceptionType; use JKingWeb\Arsse\Feed\Exception as FeedException; use JKingWeb\Arsse\AbstractException; use JKingWeb\Arsse\Context\Context; @@ -118,7 +119,7 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler { 'DELETE' => ["deleteCategory", false, true, false, false, []], ], '/categories/1/entries' => [ - 'GET' => ["getCategoryEntries", false, true, false, false, []], + 'GET' => ["getCategoryEntries", false, true, false, true, []], ], '/categories/1/entries/1' => [ 'GET' => ["getCategoryEntry", false, true, false, false, []], @@ -155,7 +156,7 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler { 'DELETE' => ["deleteFeed", false, true, false, false, []], ], '/feeds/1/entries' => [ - 'GET' => ["getFeedEntries", false, true, false, false, []], + 'GET' => ["getFeedEntries", false, true, false, true, []], ], '/feeds/1/entries/1' => [ 'GET' => ["getFeedEntry", false, true, false, false, []], @@ -226,7 +227,7 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler { return new ErrorResponse("401", 401); } // get the request path only; this is assumed to already be normalized - $target = parse_url($req->getRequestTarget())['path'] ?? ""; + $target = parse_url($req->getRequestTarget(), \PHP_URL_PATH) ?? ""; $method = $req->getMethod(); // handle HTTP OPTIONS requests if ($method === "OPTIONS") { @@ -270,7 +271,7 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler { $args[] = $data; } if ($reqQuery) { - $args[] = $req->getQueryParams(); + $args[] = $this->normalizeQuery(parse_url($req->getRequestTarget(), \PHP_URL_QUERY) ?? ""); } try { return $this->$func(...$args); @@ -330,9 +331,9 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler { } elseif (gettype($body[$k]) !== $t) { return new ErrorResponse(["InvalidInputType", 'field' => $k, 'expected' => $t, 'actual' => gettype($body[$k])], 422); } elseif ( - (in_array($k, ["keeplist_rules", "blocklist_rules"]) && !Rule::validate($body[$k])) || - (in_array($k, ["url", "feed_url"]) && !URL::absolute($body[$k])) || - ($k === "category_id" && $body[$k] < 1) + (in_array($k, ["keeplist_rules", "blocklist_rules"]) && !Rule::validate($body[$k])) + || (in_array($k, ["url", "feed_url"]) && !URL::absolute($body[$k])) + || ($k === "category_id" && $body[$k] < 1) ) { return new ErrorResponse(["InvalidInputValue", 'field' => $k], 422); } @@ -359,7 +360,7 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler { return $body; } - protected function normalizeQuery(string $query): array { + protected function normalizeQuery(string $query) { // fill an array with all valid keys $out = []; foreach (self::VALID_QUERY as $k => $t) { @@ -376,10 +377,26 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler { } $t = self::VALID_QUERY[$k] & ~V::M_ARRAY; $a = self::VALID_QUERY[$k] >= V::M_ARRAY; - if ($a) { - $out[$k][] = V::normalize($v, $t + V::M_DROP, "unix"); - } elseif (!isset($out[$k])) { - $out[$k] = V::normalize($v, $t + V::M_DROP, "unix"); + try { + if ($a) { + $out[$k][] = V::normalize($v, $t + V::M_STRICT, "unix"); + } elseif (!isset($out[$k])) { + $out[$k] = V::normalize($v, $t + V::M_STRICT, "unix"); + } else { + return new ErrorResponse(["DuplicateInputValue", 'field' => $k], 400); + } + } catch (ExceptionType $e) { + return new ErrorResponse(["InvalidInputValue", 'field' => $k], 400); + } + if ( + // TODO: does the "starred" param accept 0/1, or just true/false? + (in_array($k, ["category_id", "before_entry_id", "after_entry_id"]) && $v < 1) + || (in_array($k, ["limit", "offset"]) && $v < 0) + || ($k === "direction" && !in_array($v, ["asc", "desc"])) + || ($k === "order" && !in_array($v, ["id", "status", "published_at", "category_title", "category_id"])) + || ($k === "status" && !in_array($v, ["read", "unread", "removed"])) + ) { + return new ErrorResponse(["InvalidInputValue", 'field' => $k], 400); } } return $out; diff --git a/locale/en.php b/locale/en.php index b44d7490..03b0579f 100644 --- a/locale/en.php +++ b/locale/en.php @@ -12,6 +12,7 @@ return [ 'API.Miniflux.Error.403' => 'Access Forbidden', 'API.Miniflux.Error.404' => 'Resource Not Found', 'API.Miniflux.Error.MissingInputValue' => 'Required key "{field}" was not present in input', + 'API.Miniflux.Error.DuplicateInputValue' => 'Key "{field}" accepts only one value', 'API.Miniflux.Error.InvalidBodyJSON' => 'Invalid JSON payload: {0}', 'API.Miniflux.Error.InvalidInputType' => 'Input key "{field}" of type {actual} was expected as {expected}', 'API.Miniflux.Error.InvalidInputValue' => 'Supplied value is not valid for input key "{field}"',