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 <dev@sagebl.eu>
Co-Authored-By: Kurt Griffiths <inbox@kgriffs.com>

Closes #769

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 <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}
 

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):

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)