Initial rewrite of REST class; needs more testing, but should be functional

- improves #53
- improves #66

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
 

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
 =============================
 

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);
 }

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 {
+    public function dispatch(ServerRequestInterface $req = null): ResponseInterface {
-        if ($req===null) {
+        // create a request object if not provided
-            $req = new REST\Request();
+        $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);
     }
-        $api = $this->apiMatch($req->url, $this->apis);
+
-        $req->url = substr($req->url, strlen($this->apis[$api]['strip']));
+    protected function handOffRequest(string $className, ServerRequestInterface $req): ResponseInterface {
-        $req->refreshURL();
+        // instantiate the API handler
-        $class = $this->apis[$api]['class'];
+        $drv = new $className();
-        $drv = new $class();
+        // perform the request and return the response
-        if ($req->head) {
+        if ($req->getMethod()=="HEAD") {
-            $res =  $drv->dispatch($req);
+            // if the request is a HEAD request, we act exactly as if it were a GET request, and simply remove the response body later
-            $res->head = true;
+            return $drv->dispatch($req->withMethod("GET"));
-            return $res;
         } 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 
         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;
+    }
 }

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 {
 }

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 {
 }

lib/REST/Exception501.php

@@ -0,0 +1,10 @@
+<?php
+/** @license MIT
+ * Copyright 2017 J. King, Dustin Wilson et al.
+ * See LICENSE and AUTHORS files for details */
+
+declare(strict_types=1);
+namespace JKingWeb\Arsse\REST;
+
+class Exception501 extends \Exception {
+}

lib/REST/NextCloudNews/V1_2.php

@@ -16,6 +16,8 @@ use JKingWeb\Arsse\AbstractException;
 use JKingWeb\Arsse\Db\ExceptionInput;
 use JKingWeb\Arsse\Feed\Exception as FeedException;
 use JKingWeb\Arsse\REST\Target;
+use JKingWeb\Arsse\REST\Exception404;
+use JKingWeb\Arsse\REST\Exception405;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Message\ResponseInterface;
 use Zend\Diactoros\Response\JsonResponse as Response;

lib/REST/Response.php

@@ -1,65 +0,0 @@
-<?php
-/** @license MIT
- * Copyright 2017 J. King, Dustin Wilson et al.
- * See LICENSE and AUTHORS files for details */
-
-declare(strict_types=1);
-namespace JKingWeb\Arsse\REST;
-
-use JKingWeb\Arsse\Arsse;
-
-class Response {
-    const T_JSON = "application/json";
-    const T_XML  = "application/xml";
-    const T_TEXT = "text/plain";
-
-    public $head = false;
-    public $code;
-    public $payload;
-    public $type;
-    public $fields;
-
-
-    public function __construct(int $code, $payload = null, string $type = self::T_JSON, array $extraFields = []) {
-        $this->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");
-        }
-    }
-}

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";

tests/cases/REST/TestREST.php

@@ -0,0 +1,50 @@
+<?php
+/** @license MIT
+ * Copyright 2017 J. King, Dustin Wilson et al.
+ * See LICENSE and AUTHORS files for details */
+
+declare(strict_types=1);
+namespace JKingWeb\Arsse\TestCase\REST;
+
+use JKingWeb\Arsse\REST;
+use JKingWeb\Arsse\REST\Exception501;
+
+/** @covers \JKingWeb\Arsse\REST */
+class TestREST extends \JKingWeb\Arsse\Test\AbstractTest {
+
+    /** @dataProvider provideApiMatchData */
+    public function testMatchAUrlToAnApi($apiList, string $input, array $exp) {
+        $r = new REST($apiList);
+        try {
+            $out = $r->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",                       []],
+        ];
+    }
+}

tests/cases/REST/TestTarget.php

@@ -8,7 +8,7 @@ namespace JKingWeb\Arsse\TestCase\REST;
 
 use JKingWeb\Arsse\REST\Target;
 
-/** @covers \JKingWeb\Arsse\REST\Target<extended> */
+/** @covers \JKingWeb\Arsse\REST\Target */
 class TestTarget extends \JKingWeb\Arsse\Test\AbstractTest {
 
     /** @dataProvider provideTargetUrls */

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);
     }

tests/phpunit.xml

@@ -67,6 +67,7 @@
     </testsuite>
     <testsuite name="REST">
         <file>cases/REST/TestTarget.php</file>
+        <file>cases/REST/TestREST.php</file>
     </testsuite>
     <testsuite name="NCNv1">
         <file>cases/REST/NextCloudNews/TestVersions.php</file>