From fc3999946a9cd09b4de0f9c6cb473559cfbe0508 Mon Sep 17 00:00:00 2001 From: Kurt Griffiths Date: Mon, 11 Jul 2016 10:09:35 -0600 Subject: [PATCH] fix(api.add_route): Validate uri template fields (#843) Raise ValueError if a field is found that is not a valid Python identifier. This prevents cryptic errors from being raised later on when requests are routed. Co-Authored-By: Enzo Calamia Co-Authored-By: Kurt Griffiths Closes #769 --- falcon/api.py | 24 ++++++++++++++++-------- falcon/routing/compiled.py | 13 +++++++++---- tests/test_default_router.py | 17 ++++++++++++++++- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/falcon/api.py b/falcon/api.py index 3b5539b..557de77 100644 --- a/falcon/api.py +++ b/falcon/api.py @@ -40,17 +40,17 @@ class API(object): class ExampleComponent(object): def process_request(self, req, resp): - \"""Process the request before routing it. + \"\"\"Process the request before routing it. Args: req: Request object that will eventually be routed to an on_* responder method. resp: Response object that will be routed to the on_* responder. - \""" + \"\"\" def process_resource(self, req, resp, resource, params): - \"""Process the request and resource *after* routing. + \"\"\"Process the request and resource *after* routing. Note: This method is only called when the request matches @@ -69,10 +69,10 @@ class API(object): template fields, that will be passed to the resource's responder method as keyword arguments. - \""" + \"\"\" def process_response(self, req, resp, resource) - \"""Post-processing of the response (after routing). + \"\"\"Post-processing of the response (after routing). Args: req: Request object. @@ -80,7 +80,7 @@ class API(object): resource: Resource object to which the request was routed. May be None if no route was found for the request. - \""" + \"\"\" See also :ref:`Middleware `. @@ -249,6 +249,10 @@ class API(object): def add_route(self, uri_template, resource, *args, **kwargs): """Associates a templatized URI path with a resource. + Note: + The following information describes the behavior of + Falcon's default router. + A resource is an instance of a class that defines various "responder" methods, one for each HTTP method the resource allows. Responder names start with `on_` and are named according to @@ -272,6 +276,10 @@ class API(object): field names defined in the template. A field expression consists of a bracketed field name. + Note: + Since field names correspond to argument names in responder + methods, they must be valid Python identifiers. + For example, given the following template:: /user/{name} @@ -281,8 +289,8 @@ class API(object): def on_put(self, req, resp, name): pass - Individual path segments may contain one or more field expressions. - For example:: + Individual path segments may contain one or more field + expressions:: /repos/{org}/{repo}/compare/{usr0}:{branch0}...{usr1}:{branch1} diff --git a/falcon/routing/compiled.py b/falcon/routing/compiled.py index 057cf6e..f4e3058 100644 --- a/falcon/routing/compiled.py +++ b/falcon/routing/compiled.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import keyword import re @@ -41,14 +42,18 @@ class CompiledRouter(object): def add_route(self, uri_template, method_map, resource): """Adds a route between URI path template and resource.""" - # Can't start with a number, since these eventually get passed as - # args to on_* responders - if re.search('{\d', uri_template): - raise ValueError('Field names may not start with a digit.') if re.search('\s', uri_template): raise ValueError('URI templates may not include whitespace.') + # NOTE(kgriffs): Ensure fields are valid Python identifiers, + # since they will be passed as kwargs to responders. + fields = re.findall('{([^}]*)}', uri_template) + for field in fields: + is_identifier = re.match('[A-Za-z_][A-Za-z0-9_]+$', field) + if not is_identifier or field in keyword.kwlist: + raise ValueError('Field names must be valid identifiers.') + path = uri_template.strip('/').split('/') def insert(nodes, path_index=0): diff --git a/tests/test_default_router.py b/tests/test_default_router.py index dec8a8e..9f3c6d5 100644 --- a/tests/test_default_router.py +++ b/tests/test_default_router.py @@ -137,13 +137,28 @@ class TestComplexRouting(testing.TestBase): ) @ddt.data( - '/repos/{org}/{repo}/compare/{simple-vs-complex}', + '/repos/{org}/{repo}/compare/{simple_vs_complex}', '/repos/{complex}.{vs}.{simple}', '/repos/{org}/{repo}/compare/{complex}:{vs}...{complex2}/full', ) def test_non_collision(self, template): self.router.add_route(template, {}, ResourceWithId(-1)) + @ddt.data( + '/{}', + '/{9v}', + '/{@kgriffs}', + '/repos/{simple-thing}/etc', + '/repos/{or g}/{repo}/compare/{thing}', + '/repos/{org}/{repo}/compare/{}', + '/repos/{complex}.{}.{thing}', + '/repos/{complex}.{9v}.{thing}/etc', + ) + def test_invalid_field_name(self, template): + self.assertRaises( + ValueError, + self.router.add_route, template, {}, ResourceWithId(-1)) + def test_dump(self): print(self.router._src)