Merge pull request #655 from meejah/ticket3252-port-uri-handler

3257: port uri handler

src/allmydata/test/web/test_root.py

@@ -1,8 +1,17 @@
+from mock import Mock
+
 from twisted.trial import unittest
+from twisted.web.test.requesthelper import DummyRequest
 
 from ...storage_client import NativeStorageServer
 from ...web.root import Root
 from ...util.connection_status import ConnectionStatus
+from allmydata.web.root import URIHandler
+from allmydata.web.common import WebError
+
+from hypothesis import given
+from hypothesis.strategies import text
+
 
 class FakeRoot(Root):
     def __init__(self):
@@ -10,6 +19,7 @@ class FakeRoot(Root):
     def now_fn(self):
         return 0
 
+
 class FakeContext(object):
     def __init__(self):
         self.slots = {}
@@ -17,12 +27,62 @@ class FakeContext(object):
     def fillSlots(self, slotname, contents):
         self.slots[slotname] = contents
 
+
+class RenderSlashUri(unittest.TestCase):
+    """
+    Ensure that URIs starting with /uri?uri= only accept valid
+    capabilities
+    """
+
+    def setUp(self):
+        self.request = DummyRequest(b"/uri")
+        self.request.fields = {}
+
+        def prepathURL():
+            return b"http://127.0.0.1.99999/" + b"/".join(self.request.prepath)
+
+        self.request.prePathURL = prepathURL
+        self.client = Mock()
+        self.res = URIHandler(self.client)
+
+    def test_valid(self):
+        """
+        A valid capbility does not result in error
+        """
+        self.request.args[b"uri"] = [(
+            b"URI:CHK:nt2xxmrccp7sursd6yh2thhcky:"
+            b"mukesarwdjxiyqsjinbfiiro6q7kgmmekocxfjcngh23oxwyxtzq:2:5:5874882"
+        )]
+        self.res.render_GET(self.request)
+
+    def test_invalid(self):
+        """
+        A (trivially) invalid capbility is an error
+        """
+        self.request.args[b"uri"] = [b"not a capability"]
+        with self.assertRaises(WebError):
+            self.res.render_GET(self.request)
+
+    @given(
+        text()
+    )
+    def test_hypothesis_error_caps(self, cap):
+        """
+        Let hypothesis try a bunch of invalid capabilities
+        """
+        self.request.args[b"uri"] = [cap.encode('utf8')]
+        with self.assertRaises(WebError):
+            self.res.render_GET(self.request)
+
+
 class RenderServiceRow(unittest.TestCase):
     def test_missing(self):
-        # minimally-defined static servers just need anonymous-storage-FURL
-        # and permutation-seed-base32. The WUI used to have problems
-        # rendering servers that lacked nickname and version. This tests that
-        # we can render such minimal servers.
+        """
+        minimally-defined static servers just need anonymous-storage-FURL
+        and permutation-seed-base32. The WUI used to have problems
+        rendering servers that lacked nickname and version. This tests that
+        we can render such minimal servers.
+        """
         ann = {"anonymous-storage-FURL": "pb://w2hqnbaa25yw4qgcvghl5psa3srpfgw3@tcp:127.0.0.1:51309/vucto2z4fxment3vfxbqecblbf6zyp6x",
                "permutation-seed-base32": "w2hqnbaa25yw4qgcvghl5psa3srpfgw3",
                }

src/allmydata/web/root.py

@@ -1,7 +1,17 @@
-import time, os, json
+import os
+import time
+import json
+import urllib
 
-from twisted.web import http
-from nevow import rend, url, tags as T
+from twisted.web import (
+    http,
+    resource,
+)
+from twisted.web.util import redirectTo
+
+from hyperlink import URL
+
+from nevow import rend, tags as T
 from nevow.inevow import IRequest
 from nevow.static import File as nevow_File # TODO: merge with static.File?
 from nevow.util import resource_filename
@@ -28,31 +38,53 @@ from allmydata.web.common import (
 from allmydata.web.private import (
     create_private_tree,
 )
+from allmydata import uri
 
-class URIHandler(RenderMixin, rend.Page):
-    # I live at /uri . There are several operations defined on /uri itself,
-    # mostly involved with creation of unlinked files and directories.
+class URIHandler(resource.Resource, object):
+    """
+    I live at /uri . There are several operations defined on /uri itself,
+    mostly involved with creation of unlinked files and directories.
+    """
 
     def __init__(self, client):
-        rend.Page.__init__(self, client)
+        super(URIHandler, self).__init__()
         self.client = client
 
-    def render_GET(self, ctx):
-        req = IRequest(ctx)
-        uri = get_arg(req, "uri", None)
-        if uri is None:
+    def render_GET(self, req):
+        """
+        Historically, accessing this via "GET /uri?uri=<capabilitiy>"
+        was/is a feature -- which simply redirects to the more-common
+        "GET /uri/<capability>" with any other query args
+        preserved. New code should use "/uri/<cap>"
+        """
+        uri_arg = req.args.get(b"uri", [None])[0]
+        if uri_arg is None:
             raise WebError("GET /uri requires uri=")
-        there = url.URL.fromContext(ctx)
-        there = there.clear("uri")
-        # I thought about escaping the childcap that we attach to the URL
-        # here, but it seems that nevow does that for us.
-        there = there.child(uri)
-        return there
 
-    def render_PUT(self, ctx):
-        req = IRequest(ctx)
-        # either "PUT /uri" to create an unlinked file, or
-        # "PUT /uri?t=mkdir" to create an unlinked directory
+        # shennanigans like putting "%2F" or just "/" itself, or ../
+        # etc in the <cap> might be a vector for weirdness so we
+        # validate that this is a valid capability before proceeding.
+        cap = uri.from_string(uri_arg)
+        if isinstance(cap, uri.UnknownURI):
+            raise WebError("Invalid capability")
+
+        # so, using URL.from_text(req.uri) isn't going to work because
+        # it seems Nevow was creating absolute URLs including
+        # host/port whereas req.uri is absolute (but lacks host/port)
+        redir_uri = URL.from_text(req.prePathURL().decode('utf8'))
+        redir_uri = redir_uri.child(urllib.quote(uri_arg).decode('utf8'))
+        # add back all the query args that AREN'T "?uri="
+        for k, values in req.args.items():
+            if k != b"uri":
+                for v in values:
+                    redir_uri = redir_uri.add(k.decode('utf8'), v.decode('utf8'))
+        return redirectTo(redir_uri.to_text().encode('utf8'), req)
+
+    def render_PUT(self, req):
+        """
+        either "PUT /uri" to create an unlinked file, or
+        "PUT /uri?t=mkdir" to create an unlinked directory
+        """
         t = get_arg(req, "t", "").strip()
         if t == "":
             file_format = get_format(req, "CHK")
@@ -63,15 +95,18 @@ class URIHandler(RenderMixin, rend.Page):
                 return unlinked.PUTUnlinkedCHK(req, self.client)
         if t == "mkdir":
             return unlinked.PUTUnlinkedCreateDirectory(req, self.client)
-        errmsg = ("/uri accepts only PUT, PUT?t=mkdir, POST?t=upload, "
-                  "and POST?t=mkdir")
+        errmsg = (
+            "/uri accepts only PUT, PUT?t=mkdir, POST?t=upload, "
+            "and POST?t=mkdir"
+        )
         raise WebError(errmsg, http.BAD_REQUEST)
 
-    def render_POST(self, ctx):
-        # "POST /uri?t=upload&file=newfile" to upload an
-        # unlinked file or "POST /uri?t=mkdir" to create a
-        # new directory
-        req = IRequest(ctx)
+    def render_POST(self, req):
+        """
+        "POST /uri?t=upload&file=newfile" to upload an
+        unlinked file or "POST /uri?t=mkdir" to create a
+        new directory
+        """
         t = get_arg(req, "t", "").strip()
         if t in ("", "upload"):
             file_format = get_format(req)
@@ -92,14 +127,20 @@ class URIHandler(RenderMixin, rend.Page):
                   "and POST?t=mkdir")
         raise WebError(errmsg, http.BAD_REQUEST)
 
-    def childFactory(self, ctx, name):
-        # 'name' is expected to be a URI
+    def getChild(self, name, req):
+        """
+        Most requests look like /uri/<cap> so this fetches the capability
+        and creates and appropriate handler (depending on the kind of
+        capability it was passed).
+        """
         try:
             node = self.client.create_node_from_uri(name)
             return directory.make_handler_for(node, self.client)
         except (TypeError, AssertionError):
-            raise WebError("'%s' is not a valid file- or directory- cap"
-                           % name)
+            raise WebError(
+                "'{}' is not a valid file- or directory- cap".format(name)
+            )
+
 
 class FileHandler(rend.Page):
     # I handle /file/$FILECAP[/IGNORED] , which provides a URL from which a