Repository: twisted/twisted

Issue:
twisted.web.Resource.putChild works unexpectedly with Unicode path
|[<img alt="derwolfe's avatar" src="https://avatars.githubusercontent.com/u/641357?s=50" width="50" height="50">](https://github.com/derwolfe)| @derwolfe reported|
|-|-|
|Trac ID|trac#8985|
|Type|defect|
|Created|2017-01-10 19:17:12Z|

IResource [https://github.com/twisted/twisted/blob/twisted-16.5.0/src/twisted/web/resource.py#L61-L71] states that the `path` param should be `bytes`. However, if a user passes in a unicode string, no warning is displayed, nor is any exception thrown. On python3, this is especially problematic, as new users might be following the documentation how-to for twisted web [https://twistedmatrix.com/documents/current/web/howto/using-twistedweb.html#resource-trees] and not understand why their unicode path isn't returning the resource that they would expect.

Currently, Resource's implementation [https://github.com/twisted/twisted/blob/twisted-16.5.0/src/twisted/web/resource.py#L209-L220] of `putChild` happily accepts unicode or bytes. 

For example of how this fails, the following twisted site will return `hi` when curl'ed (`curl http://localhost:8888`) and running on python2, but will return a 404 when run with python3.

```
from twisted.internet import reactor
from twisted.web.server import Site
from twisted.web.resource import Resource


class Page(Resource):

    def render_GET(self, request):
        return b"hi"


if __name__ == '__main__':

    root = Resource()
    root.putChild("", Page()) # on python3, "" should be b""

    factory = Site(root)

    reactor.listenTCP(8888, factory)
    reactor.run()
```



<details><summary>Searchable metadata</summary>

```
trac-id__8985 8985
type__defect defect
reporter__derwolfe derwolfe
priority__normal normal
milestone__None None
branch__ 
branch_author__ 
status__new new
resolution__None None
component__web web
keywords__None None
time__1484075832128825 1484075832128825
changetime__1511669305479914 1511669305479914
version__None None
owner__rodrigc rodrigc
cc__derwolfe
```
</details>


Discussion:
twm: please review
twm: The Codecov diff coverage complaint appears to be nonsense.
twm: Thank you for the review @adiroiban! I've rebased and will land this when it goes green.

Patch:
diff --git a/src/twisted/web/distrib.py b/src/twisted/web/distrib.py
index 56f83fe279..3e77abfe42 100644
--- a/src/twisted/web/distrib.py
+++ b/src/twisted/web/distrib.py
@@ -10,9 +10,8 @@ by each subprocess and not by the main web server (i.e. GET, POST etc.).
 """
 
 import copy
-
-# System Imports
 import os
+import sys
 
 try:
     import pwd
@@ -24,8 +23,6 @@ from xml.dom.minidom import getDOMImplementation
 from twisted.internet import address, reactor
 from twisted.logger import Logger
 from twisted.persisted import styles
-
-# Twisted Imports
 from twisted.spread import pb
 from twisted.spread.banana import SIZE_LIMIT
 from twisted.web import http, resource, server, static, util
@@ -355,18 +352,20 @@ class UserDirectory(resource.Resource):
         return htmlDoc.encode("utf-8")
 
     def getChild(self, name, request):
-        if name == "":
+        if name == b"":
             return self
 
-        td = ".twistd"
+        td = b".twistd"
 
-        if name[-len(td) :] == td:
+        if name.endswith(td):
             username = name[: -len(td)]
             sub = 1
         else:
             username = name
             sub = 0
         try:
+            # Decode using the filesystem encoding to reverse a transformation
+            # done in the pwd module.
             (
                 pw_name,
                 pw_passwd,
@@ -375,7 +374,7 @@ class UserDirectory(resource.Resource):
                 pw_gecos,
                 pw_dir,
                 pw_shell,
-            ) = self._pwd.getpwnam(username)
+            ) = self._pwd.getpwnam(username.decode(sys.getfilesystemencoding()))
         except KeyError:
             return resource.NoResource()
         if sub:
diff --git a/src/twisted/web/newsfragments/8985.removal b/src/twisted/web/newsfragments/8985.removal
new file mode 100644
index 0000000000..534af29144
--- /dev/null
+++ b/src/twisted/web/newsfragments/8985.removal
@@ -0,0 +1,1 @@
+twisted.web.resource.Resource.putChild now raises TypeError when the path argument is not bytes, rather than issuing a deprecation warning.
diff --git a/src/twisted/web/resource.py b/src/twisted/web/resource.py
index 5e6bd83f90..0e0d1a7a96 100644
--- a/src/twisted/web/resource.py
+++ b/src/twisted/web/resource.py
@@ -59,16 +59,18 @@ class IResource(Interface):
         @type request: L{twisted.web.server.Request}
         """
 
-    def putChild(path, child):
+    def putChild(path: bytes, child: "IResource") -> None:
         """
-        Put a child IResource implementor at the given path.
+        Put a child L{IResource} implementor at the given path.
 
         @param path: A single path component, to be interpreted relative to the
             path this resource is found at, at which to put the given child.
             For example, if resource A can be found at I{http://example.com/foo}
             then a call like C{A.putChild(b"bar", B)} will make resource B
             available at I{http://example.com/foo/bar}.
-        @type path: C{bytes}
+
+            The path component is I{not} URL-encoded -- pass C{b'foo bar'}
+            rather than C{b'foo%20bar'}.
         """
 
     def render(request):
@@ -103,7 +105,7 @@ class Resource:
     """
     Define a web-accessible resource.
 
-    This serves 2 main purposes; one is to provide a standard representation
+    This serves two main purposes: one is to provide a standard representation
     for what HTTP specification calls an 'entity', and the other is to provide
     an abstract directory structure for URL retrieval.
     """
@@ -199,12 +201,17 @@ class Resource:
         return self.getChild(path, request)
 
     def getChildForRequest(self, request):
+        """
+        Deprecated in favor of L{getChildForRequest}.
+
+        @see: L{twisted.web.resource.getChildForRequest}.
+        """
         warnings.warn(
             "Please use module level getChildForRequest.", DeprecationWarning, 2
         )
         return getChildForRequest(self, request)
 
-    def putChild(self, path, child):
+    def putChild(self, path: bytes, child: IResource) -> None:
         """
         Register a static child.
 
@@ -213,24 +220,18 @@ class Resource:
         path to be ''.
 
         @param path: A single path component.
-        @type path: L{bytes}
 
         @param child: The child resource to register.
-        @type child: L{IResource}
 
         @see: L{IResource.putChild}
         """
         if not isinstance(path, bytes):
-            warnings.warn(
-                "Path segment must be bytes; "
-                "passing {} has never worked, and "
-                "will raise an exception in the future.".format(type(path)),
-                category=DeprecationWarning,
-                stacklevel=2,
-            )
+            raise TypeError(f"Path segment must be bytes, but {path!r} is {type(path)}")
 
         self.children[path] = child
-        child.server = self.server
+        # IResource is incomplete and doesn't mention this server attribute, see
+        # https://github.com/twisted/twisted/issues/11717
+        child.server = self.server  # type: ignore[attr-defined]
 
     def render(self, request):
         """


Test patch:
diff --git a/src/twisted/web/test/requesthelper.py b/src/twisted/web/test/requesthelper.py
index e233ad8b70..a3b0904427 100644
--- a/src/twisted/web/test/requesthelper.py
+++ b/src/twisted/web/test/requesthelper.py
@@ -5,11 +5,12 @@
 Helpers related to HTTP requests, used by tests.
 """
 
+from __future__ import annotations
 
 __all__ = ["DummyChannel", "DummyRequest"]
 
 from io import BytesIO
-from typing import Optional
+from typing import Dict, List, Optional
 
 from zope.interface import implementer, verify
 
@@ -206,6 +207,11 @@ class DummyRequest:
     uri = b"http://dummy/"
     method = b"GET"
     client: Optional[IAddress] = None
+    sitepath: List[bytes]
+    written: List[bytes]
+    prepath: List[bytes]
+    args: Dict[bytes, List[bytes]]
+    _finishedDeferreds: List[Deferred[None]]
 
     def registerProducer(self, prod, s):
         """
@@ -225,7 +231,12 @@ class DummyRequest:
     def unregisterProducer(self):
         self.go = 0
 
-    def __init__(self, postpath, session=None, client=None):
+    def __init__(
+        self,
+        postpath: list[bytes],
+        session: Optional[Session] = None,
+        client: Optional[IAddress] = None,
+    ) -> None:
         self.sitepath = []
         self.written = []
         self.finished = 0
diff --git a/src/twisted/web/test/test_distrib.py b/src/twisted/web/test/test_distrib.py
index 840ec43539..6f5272a4f9 100644
--- a/src/twisted/web/test/test_distrib.py
+++ b/src/twisted/web/test/test_distrib.py
@@ -29,7 +29,7 @@ from twisted.trial.unittest import TestCase
 from twisted.web import client, distrib, resource, server, static
 from twisted.web.http_headers import Headers
 from twisted.web.test._util import _render
-from twisted.web.test.test_web import DummyChannel, DummyRequest
+from twisted.web.test.requesthelper import DummyChannel, DummyRequest
 
 
 class MySite(server.Site):
@@ -395,7 +395,7 @@ class UserDirectoryTests(TestCase):
         """
         self.assertTrue(verifyObject(resource.IResource, self.directory))
 
-    def _404Test(self, name):
+    async def _404Test(self, name: bytes) -> None:
         """
         Verify that requesting the C{name} child of C{self.directory} results
         in a 404 response.
@@ -403,28 +403,24 @@ class UserDirectoryTests(TestCase):
         request = DummyRequest([name])
         result = self.directory.getChild(name, request)
         d = _render(result, request)
+        await d
+        self.assertEqual(request.responseCode, 404)
 
-        def cbRendered(ignored):
-            self.assertEqual(request.responseCode, 404)
-
-        d.addCallback(cbRendered)
-        return d
-
-    def test_getInvalidUser(self):
+    async def test_getInvalidUser(self):
         """
         L{UserDirectory.getChild} returns a resource which renders a 404
         response when passed a string which does not correspond to any known
         user.
         """
-        return self._404Test("carol")
+        await self._404Test(b"carol")
 
-    def test_getUserWithoutResource(self):
+    async def test_getUserWithoutResource(self):
         """
         L{UserDirectory.getChild} returns a resource which renders a 404
         response when passed a string which corresponds to a known user who has
         neither a user directory nor a user distrib socket.
         """
-        return self._404Test("alice")
+        await self._404Test(b"alice")
 
     def test_getPublicHTMLChild(self):
         """
@@ -436,7 +432,7 @@ class UserDirectoryTests(TestCase):
         public_html = home.child("public_html")
         public_html.makedirs()
         request = DummyRequest(["bob"])
-        result = self.directory.getChild("bob", request)
+        result = self.directory.getChild(b"bob", request)
         self.assertIsInstance(result, static.File)
         self.assertEqual(result.path, public_html.path)
 
@@ -450,7 +446,7 @@ class UserDirectoryTests(TestCase):
         home.makedirs()
         web = home.child(".twistd-web-pb")
         request = DummyRequest(["bob"])
-        result = self.directory.getChild("bob.twistd", request)
+        result = self.directory.getChild(b"bob.twistd", request)
         self.assertIsInstance(result, distrib.ResourceSubscription)
         self.assertEqual(result.host, "unix")
         self.assertEqual(abspath(result.port), web.path)
diff --git a/src/twisted/web/test/test_resource.py b/src/twisted/web/test/test_resource.py
index bd2f90887d..864447dc90 100644
--- a/src/twisted/web/test/test_resource.py
+++ b/src/twisted/web/test/test_resource.py
@@ -182,19 +182,8 @@ class ResourceTests(TestCase):
         resource = Resource()
         child = Resource()
         sibling = Resource()
-        resource.putChild("foo", child)
-        warnings = self.flushWarnings([self.test_staticChildPathType])
-        self.assertEqual(len(warnings), 1)
-        self.assertIn("Path segment must be bytes", warnings[0]["message"])
-        # We expect an error here because "foo" != b"foo" on Python 3+
-        self.assertIsInstance(
-            resource.getChildWithDefault(b"foo", DummyRequest([])), ErrorPage
-        )
-
-        resource.putChild(None, sibling)
-        warnings = self.flushWarnings([self.test_staticChildPathType])
-        self.assertEqual(len(warnings), 1)
-        self.assertIn("Path segment must be bytes", warnings[0]["message"])
+        self.assertRaises(TypeError, resource.putChild, "foo", child)
+        self.assertRaises(TypeError, resource.putChild, None, sibling)
 
     def test_defaultHEAD(self):
         """
diff --git a/src/twisted/web/test/test_template.py b/src/twisted/web/test/test_template.py
index e227324759..e2e45f5ea1 100644
--- a/src/twisted/web/test/test_template.py
+++ b/src/twisted/web/test/test_template.py
@@ -761,7 +761,7 @@ class DummyRenderRequest(DummyRequest):  # type: ignore[misc]
     """
 
     def __init__(self) -> None:
-        super().__init__([""])
+        super().__init__([b""])
         self.site = FakeSite()