From 869fbe6b2cc1f7b803085c51b69e0d1f23a6d80b Mon Sep 17 00:00:00 2001 From: Tom Most Date: Thu, 20 Oct 2022 23:19:53 -0700 Subject: [PATCH 01/12] Deprecate twisted.web.resource.ErrorPage and spawn --- src/twisted/web/newsfragments/11716.feature | 1 + src/twisted/web/newsfragments/11716.removal | 1 + src/twisted/web/resource.py | 69 +++++++++++++++++---- src/twisted/web/test/test_resource.py | 51 +++++++++++++-- 4 files changed, 106 insertions(+), 16 deletions(-) create mode 100644 src/twisted/web/newsfragments/11716.feature create mode 100644 src/twisted/web/newsfragments/11716.removal diff --git a/src/twisted/web/newsfragments/11716.feature b/src/twisted/web/newsfragments/11716.feature new file mode 100644 index 00000000000..5693458b403 --- /dev/null +++ b/src/twisted/web/newsfragments/11716.feature @@ -0,0 +1 @@ +The twisted.web.pages.ErrorPage, NotFoundPage, and ForbiddenPage IResource implementations provide HTML error pages rendered safely using twisted.web.template. diff --git a/src/twisted/web/newsfragments/11716.removal b/src/twisted/web/newsfragments/11716.removal new file mode 100644 index 00000000000..f4d2b36f415 --- /dev/null +++ b/src/twisted/web/newsfragments/11716.removal @@ -0,0 +1 @@ +The twisted.web.resource.ErrorPage, NoResource, and ForbiddenResource classes have been deprecated in favor of new implementations twisted.web.pages module because they permit HTML injection. diff --git a/src/twisted/web/resource.py b/src/twisted/web/resource.py index 5e6bd83f908..93c780740fc 100644 --- a/src/twisted/web/resource.py +++ b/src/twisted/web/resource.py @@ -1,9 +1,11 @@ -# -*- test-case-name: twisted.web.test.test_web -*- +# -*- test-case-name: twisted.web.test.test_web, twisted.web.test.test_resource -*- # Copyright (c) Twisted Matrix Laboratories. # See LICENSE for details. """ Implementation of the lowest-level Resource class. + +See L{twisted.web.pages} for some utility implementations. """ @@ -21,8 +23,11 @@ from zope.interface import Attribute, Interface, implementer +from incremental import Version + from twisted.python.compat import nativeString from twisted.python.components import proxyForInterface +from twisted.python.deprecate import deprecatedModuleAttribute from twisted.python.reflect import prefixedMethodNames from twisted.web._responses import FORBIDDEN, NOT_FOUND from twisted.web.error import UnsupportedMethod @@ -286,20 +291,25 @@ def _computeAllowedMethods(resource): return allowedMethods -class ErrorPage(Resource): +class _UnsafeErrorPage(Resource): """ - L{ErrorPage} is a resource which responds with a particular + L{_UnsafeErrorPage}, publicly available via the deprecated alias + C{ErrorPage}, is a resource which responds with a particular (parameterized) status and a body consisting of HTML containing some descriptive text. This is useful for rendering simple error pages. + Deprecated in Twisted NEXT because it permits HTML injection; use + L{twisted.pages.ErrorPage} instead. + @ivar template: A native string which will have a dictionary interpolated into it to generate the response body. The dictionary has the following keys: - - C{"code"}: The status code passed to L{ErrorPage.__init__}. - - C{"brief"}: The brief description passed to L{ErrorPage.__init__}. + - C{"code"}: The status code passed to L{_UnsafeErrorPage.__init__}. + - C{"brief"}: The brief description passed to + L{_UnsafeErrorPage.__init__}. - C{"detail"}: The detailed description passed to - L{ErrorPage.__init__}. + L{_UnsafeErrorPage.__init__}. @ivar code: An integer status code which will be used for the response. @type code: C{int} @@ -342,26 +352,61 @@ def getChild(self, chnam, request): return self -class NoResource(ErrorPage): +class _UnsafeNoResource(_UnsafeErrorPage): """ - L{NoResource} is a specialization of L{ErrorPage} which returns the HTTP - response code I{NOT FOUND}. + L{_UnsafeNoResource}, publicly available via the deprecated alias + C{NoResource}, is a specialization of L{_UnsafeErrorPage} which + returns the HTTP response code I{NOT FOUND}. + + Deprecated in Twisted NEXT because it permits HTML injection; use + L{twisted.pages.NotFoundPage} instead. """ def __init__(self, message="Sorry. No luck finding that resource."): ErrorPage.__init__(self, NOT_FOUND, "No Such Resource", message) -class ForbiddenResource(ErrorPage): +class _UnsafeForbiddenResource(_UnsafeErrorPage): """ - L{ForbiddenResource} is a specialization of L{ErrorPage} which returns the - I{FORBIDDEN} HTTP response code. + L{_UnsafeForbiddenResource}, publicly available via the deprecated alias + C{ForbiddenResource} is a specialization of L{_UnsafeErrorPage} which + returns the I{FORBIDDEN} HTTP response code. + + Deprecated in Twisted NEXT because it permits HTML injection; use + L{twisted.pages.ForbiddenPage} instead. """ def __init__(self, message="Sorry, resource is forbidden."): ErrorPage.__init__(self, FORBIDDEN, "Forbidden Resource", message) +# Deliberately undocumented public aliases. See GHSA-vg46-2rrj-3647. +ErrorPage = _UnsafeErrorPage +NoResource = _UnsafeNoResource +ForbiddenResource = _UnsafeForbiddenResource + +deprecatedModuleAttribute( + Version("Twisted", "NEXT", 0, 0), + "Use twisted.pages.ErrorPage instead, which properly escapes HTML.", + __name__, + "ErrorPage", +) + +deprecatedModuleAttribute( + Version("Twisted", "NEXT", 0, 0), + "Use twisted.pages.NotFoundPage instead, which properly escapes HTML.", + __name__, + "NoResource", +) + +deprecatedModuleAttribute( + Version("Twisted", "NEXT", 0, 0), + "Use twisted.pages.ForbiddenPage instead, which properly escapes HTML.", + __name__, + "ForbiddenResource", +) + + class _IEncodingResource(Interface): """ A resource which knows about L{_IRequestEncoderFactory}. diff --git a/src/twisted/web/test/test_resource.py b/src/twisted/web/test/test_resource.py index bd2f90887da..3e83d0efdc2 100644 --- a/src/twisted/web/test/test_resource.py +++ b/src/twisted/web/test/test_resource.py @@ -11,10 +11,10 @@ from twisted.web.resource import ( FORBIDDEN, NOT_FOUND, - ErrorPage, - ForbiddenResource, - NoResource, Resource, + _UnsafeErrorPage as ErrorPage, + _UnsafeForbiddenResource as ForbiddenResource, + _UnsafeNoResource as NoResource, getChildForRequest, ) from twisted.web.test.requesthelper import DummyRequest @@ -22,13 +22,56 @@ class ErrorPageTests(TestCase): """ - Tests for L{ErrorPage}, L{NoResource}, and L{ForbiddenResource}. + Tests for L{_UnafeErrorPage}, L{_UnsafeNoResource}, and + L{_UnsafeForbiddenResource}. """ errorPage = ErrorPage noResource = NoResource forbiddenResource = ForbiddenResource + def test_deprecatedErrorPage(self): + """ + The public C{twisted.web.resource.ErrorPage} alias for the + corresponding C{_Unsafe} class produces a deprecation warning when + imported. + """ + from twisted.web.resource import ErrorPage + + self.assertIs(ErrorPage, self.errorPage) + + [warning] = self.flushWarnings() + self.assertEqual(warning["category"], DeprecationWarning) + self.assertIn("twisted.pages.ErrorPage", warning["message"]) + + def test_deprecatedNoResource(self): + """ + The public C{twisted.web.resource.NoResource} alias for the + corresponding C{_Unsafe} class produces a deprecation warning when + imported. + """ + from twisted.web.resource import NoResource + + self.assertIs(NoResource, self.noResource) + + [warning] = self.flushWarnings() + self.assertEqual(warning["category"], DeprecationWarning) + self.assertIn("twisted.pages.NotFoundPage", warning["message"]) + + def test_deprecatedForbiddenResource(self): + """ + The public C{twisted.web.resource.ForbiddenResource} alias for the + corresponding C{_Unsafe} class produce a deprecation warning when + imported. + """ + from twisted.web.resource import ForbiddenResource + + self.assertIs(ForbiddenResource, self.forbiddenResource) + + [warning] = self.flushWarnings() + self.assertEqual(warning["category"], DeprecationWarning) + self.assertIn("twisted.pages.ForbiddenPage", warning["message"]) + def test_getChild(self): """ The C{getChild} method of L{ErrorPage} returns the L{ErrorPage} it is From 5ce023c4d735a895a03f2eb4a622a2322b8990ec Mon Sep 17 00:00:00 2001 From: Tom Most Date: Thu, 20 Oct 2022 23:38:19 -0700 Subject: [PATCH 02/12] Implement twisted.web.pages --- src/twisted/web/_template_util.py | 6 +- src/twisted/web/newsfragments/11716.feature | 2 +- src/twisted/web/pages.py | 108 ++++++++++++++++++++ src/twisted/web/resource.py | 10 +- src/twisted/web/test/test_pages.py | 106 +++++++++++++++++++ src/twisted/web/test/test_resource.py | 4 +- 6 files changed, 225 insertions(+), 11 deletions(-) create mode 100644 src/twisted/web/pages.py create mode 100644 src/twisted/web/test/test_pages.py diff --git a/src/twisted/web/_template_util.py b/src/twisted/web/_template_util.py index bd081bd54ab..38ebbed1d5b 100644 --- a/src/twisted/web/_template_util.py +++ b/src/twisted/web/_template_util.py @@ -1034,9 +1034,9 @@ class _TagFactory: """ A factory for L{Tag} objects; the implementation of the L{tags} object. - This allows for the syntactic convenience of C{from twisted.web.html import - tags; tags.a(href="linked-page.html")}, where 'a' can be basically any HTML - tag. + This allows for the syntactic convenience of C{from twisted.web.template + import tags; tags.a(href="linked-page.html")}, where 'a' can be basically + any HTML tag. The class is not exposed publicly because you only ever need one of these, and we already made it for you. diff --git a/src/twisted/web/newsfragments/11716.feature b/src/twisted/web/newsfragments/11716.feature index 5693458b403..e8ba00b7ef2 100644 --- a/src/twisted/web/newsfragments/11716.feature +++ b/src/twisted/web/newsfragments/11716.feature @@ -1 +1 @@ -The twisted.web.pages.ErrorPage, NotFoundPage, and ForbiddenPage IResource implementations provide HTML error pages rendered safely using twisted.web.template. +The twisted.web.pages.ErrorPage, notFound, and forbidden IResource implementations provide HTML error pages safely rendered using twisted.web.template. diff --git a/src/twisted/web/pages.py b/src/twisted/web/pages.py new file mode 100644 index 00000000000..8f37b3e45a8 --- /dev/null +++ b/src/twisted/web/pages.py @@ -0,0 +1,108 @@ +# -*- test-case-name: twisted.web.test.test_pages -*- +# Copyright (c) Twisted Matrix Laboratories. +# See LICENSE for details. + +""" +Utility implementations of L{IResource}. +""" + +__all__ = ( + "ErrorPage", + "notFound", + "forbidden", +) + + +from twisted.web import http +from twisted.web.iweb import IRequest +from twisted.web.resource import Resource +from twisted.web.template import renderElement, tags + + +class ErrorPage(Resource): + """ + L{ErrorPage} is a resource that responds to all requests with a particular + (parameterized) HTTP status code and a body consisting of HTML containing + some descriptive text. This is useful for rendering simple error pages. + + @ivar _code: An integer HTTP status code which will be used for the + response. + + @ivar _brief: A short string which will be included in the response body as + the page title. + + @ivar _detail: A longer string which will be included in the response body. + """ + + def __init__(self, code: int, brief: str, detail: str) -> None: + """ + @param code: An integer HTTP status code which will be used for the + response. + + @param brief: A short string which will be included in the response + body as the page title. + + @param detail: A longer string which will be included in the + response body. + """ + super().__init__() + self._code: int = code + self._brief: str = brief + self._detail: str = detail + + def render(self, request: IRequest) -> None: + """ + Respond to all requests with the given HTTP status code and an HTML + document containing the explanatory strings. + """ + request.setResponseCode(self._code) + request.setHeader(b"content-type", b"text/html; charset=utf-8") + renderElement( + request, + tags.html( + tags.head(tags.title(f"{self._code} - {self._brief}")), + tags.body(tags.h1(self._brief), tags.p(self._detail)), + ), + ) + + def getChild(self, path: bytes, request: IRequest) -> Resource: + """ + Handle all requests for which L{ErrorPage} lacks a child by returning + this error page. + + @param path: A path segment. + + @param request: HTTP request + """ + return self + + +def notFound( + brief: str = "No Such Resource", + message: str = "Sorry. No luck finding that resource.", +) -> ErrorPage: + """ + Generate an L{ErrorPage} with a 404 Not Found status code. + + @param brief: A short string displayed as the page title. + + @param brief: A longer string displayed in the page body. + + @returns: An L{ErrorPage} + """ + return ErrorPage(http.NOT_FOUND, brief, message) + + +def forbidden( + brief: str = "Forbidden Resource", message: str = "Sorry, resource is forbidden." +) -> ErrorPage: + """ + Generate an L{ErrorPage} with a 403 Forbidden status code. + + @param brief: A short string displayed as the page title. + + @param brief: A longer string displayed in the page body. + + @returns: An L{ErrorPage} + """ + return ErrorPage(http.FORBIDDEN, brief, message) diff --git a/src/twisted/web/resource.py b/src/twisted/web/resource.py index 93c780740fc..09fc74a89fd 100644 --- a/src/twisted/web/resource.py +++ b/src/twisted/web/resource.py @@ -183,7 +183,7 @@ def getChild(self, path, request): Parameters and return value have the same meaning and requirements as those defined by L{IResource.getChildWithDefault}. """ - return NoResource("No such child resource.") + return _UnsafeNoResource() def getChildWithDefault(self, path, request): """ @@ -359,7 +359,7 @@ class _UnsafeNoResource(_UnsafeErrorPage): returns the HTTP response code I{NOT FOUND}. Deprecated in Twisted NEXT because it permits HTML injection; use - L{twisted.pages.NotFoundPage} instead. + L{twisted.pages.notFound} instead. """ def __init__(self, message="Sorry. No luck finding that resource."): @@ -373,7 +373,7 @@ class _UnsafeForbiddenResource(_UnsafeErrorPage): returns the I{FORBIDDEN} HTTP response code. Deprecated in Twisted NEXT because it permits HTML injection; use - L{twisted.pages.ForbiddenPage} instead. + L{twisted.pages.forbidden} instead. """ def __init__(self, message="Sorry, resource is forbidden."): @@ -394,14 +394,14 @@ def __init__(self, message="Sorry, resource is forbidden."): deprecatedModuleAttribute( Version("Twisted", "NEXT", 0, 0), - "Use twisted.pages.NotFoundPage instead, which properly escapes HTML.", + "Use twisted.pages.notFound instead, which properly escapes HTML.", __name__, "NoResource", ) deprecatedModuleAttribute( Version("Twisted", "NEXT", 0, 0), - "Use twisted.pages.ForbiddenPage instead, which properly escapes HTML.", + "Use twisted.pages.forbidden instead, which properly escapes HTML.", __name__, "ForbiddenResource", ) diff --git a/src/twisted/web/test/test_pages.py b/src/twisted/web/test/test_pages.py new file mode 100644 index 00000000000..d83e0eba3e2 --- /dev/null +++ b/src/twisted/web/test/test_pages.py @@ -0,0 +1,106 @@ +# Copyright (c) Twisted Matrix Laboratories. +# See LICENSE for details. + +""" +Test L{twisted.web.pages} +""" + +from twisted.trial.unittest import SynchronousTestCase +from twisted.web.http_headers import Headers +from twisted.web.pages import ErrorPage, forbidden, notFound +from twisted.web.test.requesthelper import DummyRequest + + +def _render(resource: ErrorPage) -> DummyRequest: + """ + Render a response using the given resource. + + @param resource: The resource to use to handle the request. + + @returns: The request that the resource handled, + """ + request = DummyRequest([b""]) + resource.render(request) + return request + + +class ErrorPageTests(SynchronousTestCase): + """ + Test L{twisted.web.pages.ErrorPage} and its convencience helpers + L{notFound} and L{forbidden}. + """ + + maxDiff = None + + def assertResponse(self, request: DummyRequest, code: int, body: bytes) -> None: + self.assertEqual(request.responseCode, code) + self.assertEqual( + request.responseHeaders, + Headers({b"content-type": [b"text/html; charset=utf-8"]}), + ) + self.assertEqual( + # Decode to str because unittest somehow still doesn't diff bytes + # without truncating them in 2022. + b"".join(request.written).decode("latin-1"), + body.decode("latin-1"), + ) + + def test_escapesHTML(self): + """ + The I{brief} and I{detail} parameters are HTML-escaped on render. + """ + self.assertResponse( + _render(ErrorPage(400, "A & B", "