From 3fa2d38f3137480f5d7a707584461f74b4b765bd Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sat, 6 Jan 2018 12:02:45 -0500 Subject: [PATCH] Initial rewrite of REST class; needs more testing, but should be functional - improves #53 - improves #66 --- CHANGELOG | 5 + UPGRADING | 8 ++ arsse.php | 4 +- lib/REST.php | 97 +++++++++++++++---- lib/REST/{NextCloudNews => }/Exception404.php | 2 +- lib/REST/{NextCloudNews => }/Exception405.php | 2 +- lib/REST/Exception501.php | 10 ++ lib/REST/NextCloudNews/V1_2.php | 2 + lib/REST/Response.php | 65 ------------- tests/bootstrap.php | 1 + tests/cases/REST/TestREST.php | 50 ++++++++++ tests/cases/REST/TestTarget.php | 2 +- tests/lib/AbstractTest.php | 11 ++- tests/phpunit.xml | 1 + 14 files changed, 171 insertions(+), 89 deletions(-) rename lib/REST/{NextCloudNews => }/Exception404.php (80%) rename lib/REST/{NextCloudNews => }/Exception405.php (80%) create mode 100644 lib/REST/Exception501.php delete mode 100644 lib/REST/Response.php create mode 100644 tests/cases/REST/TestREST.php diff --git a/CHANGELOG b/CHANGELOG index 7330065..a446fd3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,11 @@ Version 0.3.0 (2018-??-??) ========================== +Bug fixes: +- Correctly handle %-encoded request URLs +- Overhaul protocol detection to fix various subtle bugs +- Overhaul HTTP response handling for more consistent results + Changes: - Make date strings in TTRSS explicitly UTC diff --git a/UPGRADING b/UPGRADING index df9448c..160574f 100644 --- a/UPGRADING +++ b/UPGRADING @@ -9,6 +9,14 @@ When upgrading between any two versions of The Arsse, the following are usually - If installing from source, update dependencies with `composer install -o --no-dev` +Upgrading from 0.2.1 to 0.3.0 +============================= + +- The following Composer dependencies have been added: + - zendframework/zend-diactoros + - psr/http-message + + Upgrading from 0.2.0 to 0.2.1 ============================= diff --git a/arsse.php b/arsse.php index 1da0966..6468f0b 100644 --- a/arsse.php +++ b/arsse.php @@ -24,5 +24,7 @@ if (\PHP_SAPI=="cli") { Arsse::$conf->importFile(BASE."config.php"); } // handle Web requests - (new REST)->dispatch()->output(); + $emitter = new \Zend\Diactoros\Response\SapiEmitter(); + $response = (new REST)->dispatch(); + $emitter->emit($response); } diff --git a/lib/REST.php b/lib/REST.php index ea8c87d..4ccb3a0 100644 --- a/lib/REST.php +++ b/lib/REST.php @@ -6,8 +6,14 @@ declare(strict_types=1); namespace JKingWeb\Arsse; +use Psr\Http\Message\RequestInterface; +use Psr\Http\Message\ResponseInterface; +use Zend\Diactoros\ServerRequest; +use Zend\Diactoros\ServerRequestFactory; +use Zend\Diactoros\Response\EmptyResponse; + class REST { - protected $apis = [ + const API_LIST = [ // NextCloud News version enumerator 'ncn' => [ 'match' => '/index.php/apps/news/api', @@ -21,7 +27,7 @@ class REST { 'class' => REST\NextCloudNews\V1_2::class, ], 'ttrss_api' => [ // Tiny Tiny RSS https://git.tt-rss.org/git/tt-rss/wiki/ApiReference - 'match' => '/tt-rss/api/', + 'match' => '/tt-rss/api', 'strip' => '/tt-rss/api', 'class' => REST\TinyTinyRSS\API::class, ], @@ -44,40 +50,93 @@ class REST { // NewsBlur http://www.newsblur.com/api // Feedly https://developer.feedly.com/ ]; + protected $apis = []; - public function __construct() { + public function __construct(array $apis = null) { + $this->apis = $apis ?? self::API_LIST; } - public function dispatch(REST\Request $req = null): \Psr\Http\Message\ResponseInterface { - if ($req===null) { - $req = new REST\Request(); - } - $api = $this->apiMatch($req->url, $this->apis); - $req->url = substr($req->url, strlen($this->apis[$api]['strip'])); - $req->refreshURL(); - $class = $this->apis[$api]['class']; - $drv = new $class(); - if ($req->head) { - $res = $drv->dispatch($req); - $res->head = true; - return $res; + public function dispatch(ServerRequestInterface $req = null): ResponseInterface { + // create a request object if not provided + $req = $req ?? ServerRequestFactory::fromGlobals(); + // find the API to handle + list ($api, $target, $class) = $this->apiMatch($req->getRequestTarget(), $this->apis); + // modify the request to have a stripped target + $req = $req->withRequestTarget($target); + // generate a response + $res = $this->handOffRequest($class, $req); + // modify the response so that it has all the required metadata + $res = $this->normalizeResponse($res, $req); + } + + protected function handOffRequest(string $className, ServerRequestInterface $req): ResponseInterface { + // instantiate the API handler + $drv = new $className(); + // perform the request and return the response + if ($req->getMethod()=="HEAD") { + // if the request is a HEAD request, we act exactly as if it were a GET request, and simply remove the response body later + return $drv->dispatch($req->withMethod("GET")); } else { return $drv->dispatch($req); } } - public function apiMatch(string $url, array $map): string { + public function apiMatch(string $url): array { + $map = $this->apis; // sort the API list so the longest URL prefixes come first uasort($map, function ($a, $b) { return (strlen($a['match']) <=> strlen($b['match'])) * -1; }); + // normalize the target URL + $url = REST\Target::normalize($url); // find a match foreach ($map as $id => $api) { + // first try a simple substring match if (strpos($url, $api['match'])===0) { - return $id; + // if it matches, perform a more rigorous match and then strip off any defined prefix + $pattern = "<^".preg_quote($api['match'])."([/\?#]|$)>"; + if ($url==$api['match'] || in_array(substr($api['match'], -1, 1), ["/", "?", "#"]) || preg_match($pattern, $url)) { + $target = substr($url, strlen($api['strip'])); + } else { + // if the match fails we are not able to handle the request + throw new REST\Exception501(); + } + // return the API name, stripped URL, and API class name + return [$id, $target, $api['class']]; } } - // or throw an exception otherwise + // or throw an exception otherwise throw new REST\Exception501(); } + + public function normalizeResponse(ResponseInterface $res, RequestInterface $req = null): ResponseInterface { + // set or clear the Content-Length header field + $body = $res->getBody(); + $bodySize = $body->getSize(); + if ($bodySize || $res->getStatusCode()==200) { + // if there is a message body or the response is 200, make sure Content-Length is included + $res = $res->withHeader("Content-Length", (string) $bodySize); + } else { + // for empty responses of other statuses, omit it + $res = $res->withoutHeader("Content-Length"); + } + // if the response is to a HEAD request, the body should be omitted + if ($req->getMethod()=="HEAD") { + $res = new EmptyResponse($res->getStatusCode(), $res->getHeaders()); + } + // if an Allow header field is present, normalize it + if ($res->hasHeader("Allow")) { + $methods = preg_split("<\s+,\s+>", strtoupper($res->getHeaderLine())); + // if GET is allowed, HEAD should be allowed as well + if (in_array("GET", $methods) && !in_array("HEAD", $methods)) { + $methods[] = "HEAD"; + } + // OPTIONS requests are always allowed by our handlers + if (!in_array("OPTIONS", $methods)) { + $methods[] = "OPTIONS"; + } + $res = $res->withHeader("Allow", implode(", ", $methods)); + } + return $res; + } } diff --git a/lib/REST/NextCloudNews/Exception404.php b/lib/REST/Exception404.php similarity index 80% rename from lib/REST/NextCloudNews/Exception404.php rename to lib/REST/Exception404.php index 325a4f5..8bee192 100644 --- a/lib/REST/NextCloudNews/Exception404.php +++ b/lib/REST/Exception404.php @@ -4,7 +4,7 @@ * See LICENSE and AUTHORS files for details */ declare(strict_types=1); -namespace JKingWeb\Arsse\REST\NextCloudNews; +namespace JKingWeb\Arsse\REST; class Exception404 extends \Exception { } diff --git a/lib/REST/NextCloudNews/Exception405.php b/lib/REST/Exception405.php similarity index 80% rename from lib/REST/NextCloudNews/Exception405.php rename to lib/REST/Exception405.php index b41c0d5..842ccdb 100644 --- a/lib/REST/NextCloudNews/Exception405.php +++ b/lib/REST/Exception405.php @@ -4,7 +4,7 @@ * See LICENSE and AUTHORS files for details */ declare(strict_types=1); -namespace JKingWeb\Arsse\REST\NextCloudNews; +namespace JKingWeb\Arsse\REST; class Exception405 extends \Exception { } diff --git a/lib/REST/Exception501.php b/lib/REST/Exception501.php new file mode 100644 index 0000000..77d1e30 --- /dev/null +++ b/lib/REST/Exception501.php @@ -0,0 +1,10 @@ +code = $code; - $this->payload = $payload; - $this->type = $type; - $this->fields = $extraFields; - } - - public function output() { - if (!headers_sent()) { - foreach ($this->fields as $field) { - header($field); - } - $body = ""; - if (!is_null($this->payload)) { - switch ($this->type) { - case self::T_JSON: - $body = (string) json_encode($this->payload, \JSON_PRETTY_PRINT); - break; - default: - $body = (string) $this->payload; - break; - } - } - if (strlen($body)) { - header("Content-Type: ".$this->type); - header("Content-Length: ".strlen($body)); - } elseif ($this->code==200) { - $this->code = 204; - } - try { - $statusText = Arsse::$lang->msg("HTTP.Status.".$this->code); - } catch (\JKingWeb\Arsse\Lang\Exception $e) { - $statusText = ""; - } - header("Status: ".$this->code." ".$statusText); - if (!$this->head) { - echo $body; - } - } else { - throw new REST\Exception("headersSent"); - } - } -} diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 628bd3d..59c04a1 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -9,4 +9,5 @@ namespace JKingWeb\Arsse; const NS_BASE = __NAMESPACE__."\\"; define(NS_BASE."BASE", dirname(__DIR__).DIRECTORY_SEPARATOR); ini_set("memory_limit", "-1"); +error_reporting(\E_ALL); require_once BASE."vendor".DIRECTORY_SEPARATOR."autoload.php"; diff --git a/tests/cases/REST/TestREST.php b/tests/cases/REST/TestREST.php new file mode 100644 index 0000000..aef736e --- /dev/null +++ b/tests/cases/REST/TestREST.php @@ -0,0 +1,50 @@ +apiMatch($input); + } catch (Exception501 $e) { + $out = []; + } + $this->assertEquals($exp, $out); + } + + public function provideApiMatchData() { + $real = null; + $fake = [ + 'unstripped' => ['match' => "/full/url", 'strip' => "", 'class' => "UnstrippedProtocol"], + ]; + return [ + [$real, "/index.php/apps/news/api/v1-2/feeds", ["ncn_v1-2", "/feeds", \JKingWeb\Arsse\REST\NextCloudNews\V1_2::class]], + [$real, "/index.php/apps/news/api/v1-2", ["ncn", "/v1-2", \JKingWeb\Arsse\REST\NextCloudNews\Versions::class]], + [$real, "/index.php/apps/news/api/", ["ncn", "/", \JKingWeb\Arsse\REST\NextCloudNews\Versions::class]], + [$real, "/index%2Ephp/apps/news/api/", ["ncn", "/", \JKingWeb\Arsse\REST\NextCloudNews\Versions::class]], + [$real, "/index.php/apps/news/", []], + [$real, "/index!php/apps/news/api/", []], + [$real, "/tt-rss/api/index.php", ["ttrss_api", "/index.php", \JKingWeb\Arsse\REST\TinyTinyRSS\API::class]], + [$real, "/tt-rss/api", ["ttrss_api", "", \JKingWeb\Arsse\REST\TinyTinyRSS\API::class]], + [$real, "/tt-rss/API", []], + [$real, "/tt-rss/api-bogus", []], + [$real, "/tt-rss/api bogus", []], + [$real, "/tt-rss/feed-icons/", ["ttrss_icon", "", \JKingWeb\Arsse\REST\TinyTinyRSS\Icon::class]], + [$real, "/tt-rss/feed-icons/", ["ttrss_icon", "", \JKingWeb\Arsse\REST\TinyTinyRSS\Icon::class]], + [$real, "/tt-rss/feed-icons", []], + [$fake, "/full/url/", ["unstripped", "/full/url/", "UnstrippedProtocol"]], + [$fake, "/full/url-not", []], + ]; + } +} \ No newline at end of file diff --git a/tests/cases/REST/TestTarget.php b/tests/cases/REST/TestTarget.php index 08555d8..5577af8 100644 --- a/tests/cases/REST/TestTarget.php +++ b/tests/cases/REST/TestTarget.php @@ -8,7 +8,7 @@ namespace JKingWeb\Arsse\TestCase\REST; use JKingWeb\Arsse\REST\Target; -/** @covers \JKingWeb\Arsse\REST\Target */ +/** @covers \JKingWeb\Arsse\REST\Target */ class TestTarget extends \JKingWeb\Arsse\Test\AbstractTest { /** @dataProvider provideTargetUrls */ diff --git a/tests/lib/AbstractTest.php b/tests/lib/AbstractTest.php index c3188a6..13b5641 100644 --- a/tests/lib/AbstractTest.php +++ b/tests/lib/AbstractTest.php @@ -15,6 +15,14 @@ use Zend\Diactoros\Response\EmptyResponse; /** @coversNothing */ abstract class AbstractTest extends \PHPUnit\Framework\TestCase { + public function setUp() { + $this->clearData(); + } + + public function tearDown() { + $this->clearData(); + } + public function assertException(string $msg = "", string $prefix = "", string $type = "Exception") { if (func_num_args()) { $class = \JKingWeb\Arsse\NS_BASE . ($prefix !== "" ? str_replace("/", "\\", $prefix) . "\\" : "") . $type; @@ -34,10 +42,11 @@ abstract class AbstractTest extends \PHPUnit\Framework\TestCase { protected function assertResponse(ResponseInterface $exp, ResponseInterface $act, string $text = null) { $this->assertEquals($exp->getStatusCode(), $act->getStatusCode(), $text); - $this->assertInstanceOf(get_class($exp), $act); if ($exp instanceof JsonResponse) { $this->assertEquals($exp->getPayload(), $act->getPayload(), $text); $this->assertSame($exp->getPayload(), $act->getPayload(), $text); + } else { + $this->assertEquals((string) $exp->getBody(), (string) $act->getBody(), $text); } $this->assertEquals($exp->getHeaders(), $act->getHeaders(), $text); } diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 167ab87..2652d0a 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -67,6 +67,7 @@ cases/REST/TestTarget.php + cases/REST/TestREST.php cases/REST/NextCloudNews/TestVersions.php