diff --git a/newsfragments/3252.minor b/newsfragments/3252.minor new file mode 100644 index 000000000..e69de29bb diff --git a/src/allmydata/test/web/test_root.py b/src/allmydata/test/web/test_root.py index 1e324c398..40bb2bf9a 100644 --- a/src/allmydata/test/web/test_root.py +++ b/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", } diff --git a/src/allmydata/web/root.py b/src/allmydata/web/root.py index 99485486a..0e241cfc3 100644 --- a/src/allmydata/web/root.py +++ b/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=" + was/is a feature -- which simply redirects to the more-common + "GET /uri/" with any other query args + preserved. New code should use "/uri/" + """ + 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 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/ 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