Completely revamped NCNv1 REST handler

- URLs are now matched centrally against a whitelist
- %-encoded URLs are still handled correctly
- Dispatched methods now only handle one specific task
- Filler methods (e.g. versionPOST) are no longer required
- Unhandled URLs now return 501 Not Implemented rather than 404 Not Found; this removes some ambiguity in the semantics of 404

lib/REST/AbstractHandler.php

@@ -23,7 +23,7 @@ abstract class AbstractHandler implements Handler {
         return $data;
     }
 
-    protected function validateId($id):bool {
+    protected function validateId($id): bool {
         try {
             $ch1 = strval(intval($id));
             $ch2 = strval($id);

lib/REST/Exception405.php

@@ -0,0 +1,6 @@
+<?php
+declare(strict_types=1);
+namespace JKingWeb\Arsse\REST;
+
+class Exception405 extends \Exception {
+}

lib/REST/Exception501.php

@@ -0,0 +1,6 @@
+<?php
+declare(strict_types=1);
+namespace JKingWeb\Arsse\REST;
+
+class Exception501 extends \Exception {
+}

lib/REST/NextCloudNews/V1_2.php

@@ -7,6 +7,8 @@ use JKingWeb\Arsse\AbstractException;
 use JKingWeb\Arsse\Db\ExceptionInput;
 use JKingWeb\Arsse\Feed\Exception as FeedException;
 use JKingWeb\Arsse\REST\Response;
+use JKingWeb\Arsse\REST\Exception501;
+use JKingWeb\Arsse\REST\Exception405;
 
 class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler {
     function __construct() {
@@ -32,49 +34,91 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler {
         }
         // FIXME: Do query parameters take precedence in NextCloud? Is there a conflict error when values differ?
         $data = array_merge($data, $req->query);
-        // match the path
+        // check to make sure the requested function is implemented
-        if(preg_match("<^/(items|folders|feeds|cleanup|version|status|user)(?:/([^/]+))?(?:/([^/]+))?(?:/([^/]+))?/?$>", $req->path, $url)) {
+        try {
-            // clean up the path
+            $func = $this->chooseCall($req->paths, $req->method);
-            $scope = $url[1];
+        } catch(Exception501 $e) {
-            unset($url[0]);
+            return new Response(501);
-            unset($url[1]);
+        } catch(Exception405 $e) {
-            $url = array_filter($url);
+            return new Response(405, "", "", ["Allow: ".$e->getMessage()]);
-            $url = array_values($url);
-            // decode any % sequences in the URL
-            $url = array_map(function($v){return rawurldecode($v);}, $url);
-            // check to make sure the requested function is implemented
-            $func = $scope.$req->method;
-            if(!method_exists($this, $func)) return new Response(501);
-            // dispatch
-            try {
-                Data::$db->dateFormatDefault("unix");
-                return $this->$func($url, $data);
-            } catch(Exception $e) {
-                // if there was a REST exception return 400
-                return new Response(400);
-            } catch(AbstractException $e) {
-                // if there was any other Arsse exception return 500
-                return new Response(500);
-            }
-        } else {
-            return new Response(404);
         }
+        if(!method_exists($this, $func)) return new Response(501);
+        // dispatch
+        try {
+            Data::$db->dateFormatDefault("unix");
+            return $this->$func($req->paths, $data);
+        } catch(Exception $e) {
+            // if there was a REST exception return 400
+            return new Response(400);
+        } catch(AbstractException $e) {
+            // if there was any other Arsse exception return 500
+            return new Response(500);
+        }
+    }
+
+    protected function chooseCall(array $url, string $method): string {
+        $choices = [
+            'items' => [],
+            'folders' => [
+                ''       => ['GET' => "folderList",   'POST'   => "folderAdd"],
+                '#'      => ['PUT' => "folderRename", 'DELETE' => "folderRemove"],
+                '#/read' => ['PUT' => "folderMarkRead"],
+            ],
+            'feeds' => [
+                ''         => ['GET' => "subscriptionList", 'POST' => "subscriptionAdd"],
+                '#'        => ['DELETE' => "subscriptionRemove"],
+                '#/move'   => ['PUT' => "subscriptionMove"],
+                '#/rename' => ['PUT' => "subscriptionRename"],
+                '#/read'   => ['PUT' => "subscriptionMarkRead"],
+                'all'      => ['GET' => "feedListStale"],
+                'update'   => ['GET' => "feedUpdate"],
+            ],
+            'cleanup' => [],
+            'version' => [
+                '' => ['GET' => "versionReport"],
+            ],
+            'status' => [],
+            'user' => [],
+        ];
+        // the first path element is the overall scope of the request
+        $scope = $url[0];
+        // any URL components which are only digits should be replaced with "#", for easier comparison
+        for($a = 0; $a < sizeof($url); $a++) {
+            if($this->validateId($url[$a])) $url[$a] = "#";
+        }
+        // normalize the HTTP method to uppercase
+        $method = strtoupper($method);
+        // if the scope is not supported, return 501
+        if(!array_key_exists($scope, $choices)) throw new Exception501();
+        // we now evaluate the supplied URL against every supported path for the selected scope
+        // the URL is evaluated as an array so as to avoid decoded escapes turning invalid URLs into valid ones
+        foreach($choices[$scope] as $path => $funcs) {
+            // add the scope to the path to match against and split it
+            $path = (strlen($path)) ? "$scope/$path" : $scope;
+            $path = explode("/", $path);
+            if($path===$url) {
+                // if the path matches, make sure the method is allowed
+                if(array_key_exists($method,$funcs)) {
+                    // if it is allowed, return the object method to run
+                    return $funcs[$method];
+                } else {
+                    // otherwise return 405
+                    throw new Exception405(implode(", ", array_keys($funcs)));
+                }
+            }
+        }
+        // if the path was not found, return 501
+        throw new Exception501();
     }
     
     // list folders
-    protected function foldersGET(array $url, array $data): Response {
+    protected function folderList(array $url, array $data): Response {
-        // if URL is more than '/folders' this is an error
-        if(sizeof($url)==1)  return new Response(405, "", "", ['Allow: PUT, DELETE']);
-        if(sizeof($url) > 1) return new Response(404);
         $folders = Data::$db->folderList(Data::$user->id, null, false)->getAll();
         return new Response(200, ['folders' => $folders]);
     }
 
     // create a folder
-    protected function foldersPOST(array $url, array $data): Response {
+    protected function folderAdd(array $url, array $data): Response {
-        // if URL is more than '/folders' this is an error
-        if(sizeof($url)==1)  return new Response(405, "", "", ['Allow: PUT, DELETE']);
-        if(sizeof($url) > 1) return new Response(404);
         try {
             $folder = Data::$db->folderAdd(Data::$user->id, $data);
         } catch(ExceptionInput $e) {
@@ -93,15 +137,10 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler {
     }
 
     // delete a folder
-    protected function foldersDELETE(array $url, array $data): Response {
+    protected function folderRemove(array $url, array $data): Response {
-        // if URL is more or less than '/folders/$id' this is an error
-        if(sizeof($url) < 1) return new Response(405, "", "", ['Allow: GET, POST']);
-        if(sizeof($url) > 1) return new Response(404);
-        // folder ID must be integer
-        if(!$this->validateId($url[0])) return new Response(404);
         // perform the deletion
         try {
-            Data::$db->folderRemove(Data::$user->id, (int) $url[0]);
+            Data::$db->folderRemove(Data::$user->id, (int) $url[1]);
         } catch(ExceptionInput $e) {
             // folder does not exist
             return new Response(404);
@@ -110,17 +149,12 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler {
     }
 
     // rename a folder (also supports moving nesting folders, but this is not a feature of the API)
-    protected function foldersPUT(array $url, array $data): Response {
+    protected function folderRename(array $url, array $data): Response {
-        // if URL is more or less than '/folders/$id' this is an error
-        if(sizeof($url) < 1) return new Response(405, "", "", ['Allow: GET, POST']);
-        if(sizeof($url) > 1) return new Response(404);
-        // folder ID must be integer
-        if(!$this->validateId($url[0])) return new Response(404);
         // there must be some change to be made
         if(!sizeof($data)) return new Response(422);
         // perform the edit
         try {
-            Data::$db->folderPropertiesSet(Data::$user->id, (int) $url[0], $data);
+            Data::$db->folderPropertiesSet(Data::$user->id, (int) $url[1], $data);
         } catch(ExceptionInput $e) {
             switch($e->getCode()) {
                 // folder does not exist
@@ -138,33 +172,10 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler {
     }
 
     // return the server version
-    protected function versionGET(array $url, array $data): Response {
+    protected function versionReport(array $url, array $data): Response {
-        // if URL is more than '/version' this is an error
-        if(sizeof($url)) return new Response(404);
         return new Response(200, ['version' => \JKingWeb\Arsse\VERSION]);
     }
 
-    // invalid function
-    protected function versionPOST(array $url, array $data): Response {
-        // if URL is more than '/version' this is an error
-        if(sizeof($url)) return new Response(404);
-        return new Response(405, "", "", ['Allow: GET']);
-    }
-
-    // invalid function
-    protected function versionPUT(array $url, array $data): Response {
-        // if URL is more than '/version' this is an error
-        if(sizeof($url)) return new Response(404);
-        return new Response(405, "", "", ['Allow: GET']);
-    }
-
-    // invalid function
-    protected function versionDELETE(array $url, array $data): Response {
-        // if URL is more than '/version' this is an error
-        if(sizeof($url)) return new Response(404);
-        return new Response(405, "", "", ['Allow: GET']);
-    }
-
     protected function feedTranslate(array $feed, bool $overwrite = false): array {
         // cast values
         $feed = $this->mapFieldTypes($feed, [
@@ -185,62 +196,51 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler {
     }
     
     // return list of feeds for the logged-in user
+    protected function subscriptionList(array $url, array $data): Response {
+        $subs = Data::$db->subscriptionList(Data::$user->id);
+        $out = [];
+        foreach($subs as $sub) {
+            $sub = $this->feedTranslate($sub);
+            $out[] = $sub;
+        }
+        $out = ['feeds' => $out];
+        $out['starredCount'] = Data::$db->articleStarredCount(Data::$user->id);
+        $newest = Data::$db->editionLatest(Data::$user->id, ['subscription' => $id]);
+        if($newest) $out['newestItemId'] = $newest;
+        return new Response(200, $out);
+    }
+    
     // return list of feeds which should be refreshed
     // refresh a feed
-    protected function feedsGET(array $url, array $data): Response {
+    protected function feedListStale(array $url, array $data): Response {
-        // URL may be /feeds/[all|update] only
+        // function requires admin rights per spec
-        $args = sizeof($url);
+        if(Data::$user->rightsGet(Data::$user->id)==User::RIGHTS_NONE) return new Response(403);
-        if($args==2 && in_array($url[1], ["rename","move","read"])) return new Response(405, "", "", ['Allow: PUT, DELETE']);
+        // list stale feeds which should be checked for updates
-        if($args > 1)                                               return new Response(404);
+        $feeds = Data::$db->feedListStale();
-        if($args==1 && !in_array($url[0], ["all","update"]))        return new Response(405, "", "", ['Allow: PUT, DELETE']);
+        $out = [];
-        // valid action are listing owned subscriptions or (for admins) listing stale feeds or updating a feed
+        foreach($feeds as $feed) {
-        if($args==1) {
+            // since in our implementation feeds don't belong the users, the 'userId' field will always be an empty string
-            // listing stale feeds for updating and updating itself require admin rights per spec
+            $out[] = ['id' => $feed, 'userId' => ""];
-            if(Data::$user->rightsGet(Data::$user->id)==User::RIGHTS_NONE) return new Response(403);
-            if($url[0]=="all") {
-                // list stale feeds which should be checked for updates
-                $feeds = Data::$db->feedListStale();
-                $out = [];
-                foreach($feeds as $feed) {
-                    // since in our implementation feeds don't belong the users, the 'userId' field will always be an empty string
-                    $out[] = ['id' => $feed, 'userId' => ""];
-                }
-                return new Response(200, ['feeds' => $out]);
-            } elseif($url[0]=="update") {
-                // perform an update of a single feed
-                if(!array_key_exists("feedId", $data) || $data['feedId'] < 1) return new Response(422);
-                try {
-                    Data::$db->feedUpdate($data['feedId']);
-                } catch(ExceptionInput $e) {
-                    return new Response(404);
-                }
-                return new Response(200);
-            }
-        } else { 
-            // list subscriptions for the logged-in user
-            $subs = Data::$db->subscriptionList(Data::$user->id);
-            $out = [];
-            foreach($subs as $sub) {
-                $sub = $this->feedTranslate($sub);
-                $out[] = $sub;
-            }
-            $out = ['feeds' => $out];
-            $out['starredCount'] = Data::$db->articleStarredCount(Data::$user->id);
-            $newest = Data::$db->editionLatest(Data::$user->id, ['subscription' => $id]);
-            if($newest) $out['newestItemId'] = $newest;
-            return new Response(200, $out);
         }
+        return new Response(200, ['feeds' => $out]);
+    }
+    
+    // refresh a feed
+    protected function feedUpdate(array $url, array $data): Response {
+        // perform an update of a single feed
+        if(!array_key_exists("feedId", $data)) return new Response(422);
+        if(!$this->validateId($data['feedId'])) return new Response(404);
+        try {
+            Data::$db->feedUpdate((int) $data['feedId']);
+        } catch(ExceptionInput $e) {
+            return new Response(404);
+        }
+        return new Response(200);
     }
 
     // add a new feed
-    protected function feedsPOST(array $url, array $data): Response {
+    protected function subscriptionAdd(array $url, array $data): Response {
-        // if URL is more than '/feeds' this is an error
+        // normalize the feed URL
-        $args = sizeof($url);
-        if($args==1 && in_array($url[0], ["all","update"]))         return new Response(405, "", "", ['Allow: GET']);
-        if($args==1)                                                return new Response(405, "", "", ['Allow: PUT, DELETE']);
-        if($args==2 && in_array($url[1], ["rename","move","read"])) return new Response(405, "", "", ['Allow: PUT']);
-        if($args)                                                   return new Response(404);
-        // normalize the URL
         if(!array_key_exists("url", $data)) {
             $url = "";
         } else {
@@ -251,7 +251,7 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler {
             $folder = null;
         } else {
             $folder = $data['folderId'];
-            $folder = $folder ? null : $folder;
+            $folder = $folder ? $folder : null;
         }
         // try to add the feed
         $tr = Data::$db->begin();
@@ -281,15 +281,9 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler {
     }
 
     // delete a feed
-    protected function feedsDELETE(array $url, array $data): Response {
+    protected function subscriptionRemove(array $url, array $data): Response {
-        // if URL is more or less than '/feeds/$id' this is an error
-        if(sizeof($url) < 1) return new Response(405, "", "", ['Allow: GET, POST']);
-        if(sizeof($url) > 1) return new Response(404);
-        // folder ID must be integer
-        if(!$this->validateId($url[0])) return new Response(404);
-        // perform the deletion
         try {
-            Data::$db->subscriptionRemove(Data::$user->id, (int) $url[0]);
+            Data::$db->subscriptionRemove(Data::$user->id, (int) $url[1]);
         } catch(ExceptionInput $e) {
             // feed does not exist
             return new Response(404);
@@ -298,44 +292,40 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler {
     }
 
     // rename a feed
-    // move a feed to a folder
+    protected function subscriptionRename(array $url, array $data): Response {
-    // mark items from a feed as read
+        // normalize input
-    protected function feedsPUT(array $url, array $data): Response {
-        // URL may be /feeds/<id>/[rename|move|read]
-        $args = sizeof($url);
-        if(!$args)                                                   return new Response(405, "", "", ['Allow: GET, POST']);
-        if($args > 2)                                                return new Response(404);
-        if(in_array($url[0], ["all", "update"])) {
-            if($args==1)                                             return new Response(405, "", "", ['Allow: GET']);
-                                                                     return new Response(404);
-        }
-        if($args==2 && !in_array($url[1], ["rename","move","read"])) return new Response(404);
-        // if the feed ID is not an integer, this is also an error
-        if(!$this->validateId($url[0])) return new Response(404);
-        // normalize input for move and rename
         $in = [];
         if(array_key_exists("feedTitle", $data)) {
             $in['title'] = $data['feedTitle'];
+        } else {
+            return new Response(422);
         }
+        // perform the renaming
+        try {
+            Data::$db->subscriptionPropertiesSet(Data::$user->id, (int) $url[1], $in);
+        } catch(ExceptionInput $e) {
+            return new Response(404);
+        }
+        return new Response(204);
+    }
+
+    // move a feed to a folder
+    protected function subscriptionMove(array $url, array $data): Response {
+        // normalize input for move and rename
+        $in = [];
         if(array_key_exists("folderId", $data)) {
             $folder = $data['folderId'];
             if(!$this->validateId($folder)) return new Response(422);
             if(!$folder) $folder = null;
             $in['folder'] = $folder;
+        } else {
+            return new Response(422);
         }
-        // perform the move and/or rename
+        // perform the move
-        if($in) {
+        try {
-            try {
+            Data::$db->subscriptionPropertiesSet(Data::$user->id, (int) $url[1], $in);
-                Data::$db->subscriptionPropertiesSet(Data::$user->id, (int) $url[0], $in);
+        } catch(ExceptionInput $e) {
-            } catch(ExceptionInput $e) {
+            return new Response(404);
-                return new Response(404);
-            }
-        }
-        // mark items as read, if requested
-        if(array_key_exists("newestItemId", $data)) {
-            $newest = $data['newestItemId'];
-            if(!$this->validateId($newest)) return new Response(422);
-            // FIXME: do the marking as read
         }
         return new Response(204);
     }

lib/REST/NextCloudNews/Versions.php

@@ -22,7 +22,7 @@ class Versions extends \JKingWeb\Arsse\REST\AbstractHandler {
             return new Response(200, $out);
         } else {
             // if the URL path was anything else, the client is probably trying a version we don't support
-            return new Response(404);
+            return new Response(501);
         }
     }
 }

lib/REST/Request.php

@@ -6,6 +6,7 @@ class Request {
     public $method = "GET";
     public $url = "";
     public $path ="";
+    public $paths = [];
     public $query = "";
     public $type ="";
     public $body = "";
@@ -31,13 +32,14 @@ class Request {
     public function refreshURL() {
         $url = $this->parseURL($this->url);
         $this->path = $url['path'];
+        $this->paths = $url['paths'];
         $this->query = $url['query'];
     }
 
     protected function parseURL(string $url): array {
         // split the query string from the path
         $parts = explode("?", $url);
-        $out = ['path' => $parts[0], 'query' => []];
+        $out = ['path' => $parts[0], 'paths' => [''], 'query' => []];
         // if there is a query string, parse it
         if(isset($parts[1])) {
             // split along & to get key-value pairs
@@ -56,6 +58,17 @@ class Request {
                 $out['query'][$key] = $value;
             }
         }
+        // also include the path as a set of decoded elements
+        // if the path is an empty string or just / nothing needs be done
+        if(!in_array($out['path'],["/",""])) {
+            $paths = explode("/", $out['path']);
+            // remove the first and last empty elements, if present (others should remain)
+            if(!strlen($paths[0])) array_shift($paths);
+            if(!strlen($paths[sizeof($paths)-1])) array_pop($paths);
+            // %-decode each path element
+            $paths = array_map(function($v){return rawurldecode($v);}, $paths);
+            $out['paths'] = $paths;
+        }
         return $out;
     }
 }

tests/REST/NextCloudNews/TestNCNV1_2.php

@@ -29,7 +29,7 @@ class TestNCNV1_2 extends \PHPUnit\Framework\TestCase {
 
     function testRespondToInvalidPaths() {
         $errs = [
-            404 => [
+            501 => [
                 ['GET',    "/"],
                 ['PUT',    "/"],
                 ['POST',   "/"],
@@ -59,10 +59,10 @@ class TestNCNV1_2 extends \PHPUnit\Framework\TestCase {
                 ],
             ],
         ];
-        foreach($errs[404] as $req) {
+        foreach($errs[501] as $req) {
-            $exp = new Response(404);
+            $exp = new Response(501);
             list($method, $path) = $req;
-            $this->assertEquals($exp, $this->h->dispatch(new Request($method, $path)), "$method call to $path did not return 404.");
+            $this->assertEquals($exp, $this->h->dispatch(new Request($method, $path)), "$method call to $path did not return 501.");
         }
         foreach($errs[405] as $allow => $cases) {
             $exp = new Response(405, "", "", ['Allow: '.$allow]);
@@ -130,9 +130,6 @@ class TestNCNV1_2 extends \PHPUnit\Framework\TestCase {
         $exp = new Response(404);
         $this->assertEquals($exp, $this->h->dispatch(new Request("DELETE", "/folders/1")));
         Phake::verify(Data::$db, Phake::times(2))->folderRemove(Data::$user->id, 1);
-        // use a non-integer folder ID
-        $exp = new Response(404);
-        $this->assertEquals($exp, $this->h->dispatch(new Request("DELETE", "/folders/invalid")));
     }
 
     function testRenameAFolder() {

tests/REST/NextCloudNews/TestNCNVersionDiscovery.php

@@ -35,7 +35,7 @@ class TestNCNVersionDiscovery extends \PHPUnit\Framework\TestCase {
     }
 
     function testUseIncorrectPath() {
-        $exp = new Response(404);
+        $exp = new Response(501);
         $h = new Rest\NextCloudNews\Versions();
         $req = new Request("GET", "/ook");
         $res = $h->dispatch($req);