diff options
author | Lakin Wecker <devnull@localhost> | 2008-08-04 16:31:30 +0000 |
---|---|---|
committer | Lakin Wecker <devnull@localhost> | 2008-08-04 16:31:30 +0000 |
commit | f9f6b551a37a16b55f32a2edfa2ce0a0452d7a80 (patch) | |
tree | 522eab3f9ea9070cf2ed769f057a536acc941889 | |
parent | 6b0d4bee07c28468a9475c6ba733d3d2dcc95138 (diff) | |
download | cherrypy-f9f6b551a37a16b55f32a2edfa2ce0a0452d7a80.tar.gz |
#733 - Return a 404 when query parameters passed to a handler are incorect. Similarly return a 404 when path atoms are incorrectly passed to a handler. Alternatively return a 400 when body params are incorrectly passed to a handler. Includes tests.
-rw-r--r-- | cherrypy/_cpdispatch.py | 122 | ||||
-rw-r--r-- | cherrypy/test/test_core.py | 124 | ||||
-rw-r--r-- | cherrypy/test/test_objectmapping.py | 5 |
3 files changed, 238 insertions, 13 deletions
diff --git a/cherrypy/_cpdispatch.py b/cherrypy/_cpdispatch.py index 4e7104bf..c29fb05b 100644 --- a/cherrypy/_cpdispatch.py +++ b/cherrypy/_cpdispatch.py @@ -21,7 +21,127 @@ class PageHandler(object): self.kwargs = kwargs def __call__(self): - return self.callable(*self.args, **self.kwargs) + try: + return self.callable(*self.args, **self.kwargs) + except TypeError, x: + test_callable_spec(self.callable, self.args, self.kwargs) + raise + +def test_callable_spec(callable, callable_args, callable_kwargs): + """ + Inspect callable and test to see if the given args are suitable for it. + + When an error occurs during the handler's invoking stage there are 2 + erroneous cases: + 1. Too many parameters passed to a function which doesn't define + one of *args or **kwargs. + 2. Too little parameters are passed to the function. + + There are 3 sources of parameters to a cherrypy handler. + 1. query string parameters are passed as keyword parameters to the handler. + 2. body parameters are also passed as keyword parameters. + 3. when partial matching occurs, the final path atoms are passed as + positional args. + Both the query string and path atoms are part of the URI. If they are + incorrect, then a 404 Not Found should be raised. Conversely the body + parameters are part of the request; if they are invalid a 400 Bad Request. + """ + (args, varargs, varkw, defaults) = inspect.getargspec(callable) + + if args and args[0] == 'self': + args = args[1:] + + arg_usage = dict([(arg, 0,) for arg in args]) + vararg_usage = 0 + varkw_usage = 0 + extra_kwargs = set() + + for i, value in enumerate(callable_args): + try: + arg_usage[args[i]] += 1 + except IndexError: + vararg_usage += 1 + + for key in callable_kwargs.keys(): + try: + arg_usage[key] += 1 + except KeyError: + varkw_usage += 1 + extra_kwargs.add(key) + + for i, val in enumerate(defaults or []): + # Defaults take effect only when the arg hasn't been used yet. + if arg_usage[args[i]] == 0: + arg_usage[args[i]] += 1 + + missing_args = [] + multiple_args = [] + for key, usage in arg_usage.iteritems(): + if usage == 0: + missing_args.append(key) + elif usage > 1: + multiple_args.append(key) + + if missing_args: + # In the case where the method allows body arguments + # there are 3 potential errors: + # 1. not enough query string parameters -> 404 + # 2. not enough body parameters -> 400 + # 3. not enough path parts (partial matches) -> 404 + # + # We can't actually tell which case it is, + # so I'm raising a 404 because that covers 2/3 of the + # possibilities + # + # In the case where the method does not allow body + # arguments it's definitely a 404. + raise cherrypy.HTTPError(404, + message="Missing parameters: %s" % ",".join(missing_args)) + + # the extra positional arguments come from the path - 404 Not Found + if not varargs and vararg_usage > 0: + raise cherrypy.HTTPError(404) + + body_params = cherrypy.request.body_params or {} + body_params = set(body_params.keys()) + qs_params = set(callable_kwargs.keys()) - body_params + + if multiple_args: + + if qs_params.intersection(set(multiple_args)): + # If any of the multiple parameters came from the query string then + # it's a 404 Not Found + error = 404 + else: + # Otherwise it's a 400 Bad Request + error = 400 + + raise cherrypy.HTTPError(error, + message="Multiple values for parameters: "\ + "%s" % ",".join(multiple_args)) + + if not varkw and varkw_usage > 0: + + # If there were extra query string parameters, it's a 404 Not Found + extra_qs_params = set(qs_params).intersection(extra_kwargs) + if extra_qs_params: + raise cherrypy.HTTPError(404, + message="Unexpected query string "\ + "parameters: %s" % ", ".join(extra_qs_params)) + + # If there were any extra body parameters, it's a 400 Not Found + extra_body_params = set(body_params).intersection(extra_kwargs) + if extra_body_params: + raise cherrypy.HTTPError(400, + message="Unexpected body parameters: "\ + "%s" % ", ".join(extra_body_params)) + + +try: + import inspect +except ImportError: + test_callable_spec = lambda callable, args, kwargs: None + class LateParamPageHandler(PageHandler): diff --git a/cherrypy/test/test_core.py b/cherrypy/test/test_core.py index 0bcda108..f181893a 100644 --- a/cherrypy/test/test_core.py +++ b/cherrypy/test/test_core.py @@ -98,6 +98,40 @@ def setup_server(): def default(self, *args, **kwargs): return "args: %s kwargs: %s" % (args, kwargs) + class ParamErrors(Test): + + def one_positional(self, param1): + return "data" + one_positional.exposed = True + + def one_positional_args(self, param1, *args): + return "data" + one_positional_args.exposed = True + + def one_positional_args_kwargs(self, param1, *args, **kwargs): + return "data" + one_positional_args_kwargs.exposed = True + + def one_positional_kwargs(self, param1, **kwargs): + return "data" + one_positional_kwargs.exposed = True + + def no_positional(self): + return "data" + no_positional.exposed = True + + def no_positional_args(self, *args): + return "data" + no_positional_args.exposed = True + + def no_positional_args_kwargs(self, *args, **kwargs): + return "data" + no_positional_args_kwargs.exposed = True + + def no_positional_kwargs(self, **kwargs): + return "data" + no_positional_kwargs.exposed = True + class Status(Test): @@ -443,15 +477,12 @@ class CoreRequestHandlingTest(helper.CPWebCase): self.getPage("/params/?thing=a&thing=b&thing=c") self.assertBody("['a', 'b', 'c']") - + # Test friendly error message when given params are not accepted. - ignore = helper.webtest.ignored_exceptions - ignore.append(TypeError) - try: - self.getPage("/params/?notathing=meeting") - self.assertInBody("index() got an unexpected keyword argument 'notathing'") - finally: - ignore.pop() + self.getPage("/params/?notathing=meeting") + self.assertInBody("Missing parameters: thing") + self.getPage("/params/?thing=meeting¬athing=meeting") + self.assertInBody("Unexpected query string parameters: notathing") # Test "% HEX HEX"-encoded URL, param keys, and values self.getPage("/params/%d4%20%e3/cheese?Gruy%E8re=Bulgn%e9ville") @@ -466,7 +497,82 @@ class CoreRequestHandlingTest(helper.CPWebCase): # Test coordinates sent by <img ismap> self.getPage("/params/ismap?223,114") self.assertBody("Coordinates: 223, 114") - + + def testParamErrors(self): + + # test that all of the handlers work when given + # the correct parameters in order to ensure that the + # errors below aren't coming from some other source. + for uri in ( + '/paramerrors/one_positional?param1=foo', + '/paramerrors/one_positional_args?param1=foo', + '/paramerrors/one_positional_args/foo', + '/paramerrors/one_positional_args/foo/bar/baz', + '/paramerrors/one_positional_args_kwargs?param1=foo¶m2=bar', + '/paramerrors/one_positional_args_kwargs/foo?param2=bar¶m3=baz', + '/paramerrors/one_positional_args_kwargs/foo/bar/baz?param2=bar¶m3=baz', + '/paramerrors/one_positional_kwargs?param1=foo¶m2=bar¶m3=baz', + '/paramerrors/one_positional_kwargs/foo?param4=foo¶m2=bar¶m3=baz', + '/paramerrors/no_positional', + '/paramerrors/no_positional_args/foo', + '/paramerrors/no_positional_args/foo/bar/baz', + '/paramerrors/no_positional_args_kwargs?param1=foo¶m2=bar', + '/paramerrors/no_positional_args_kwargs/foo?param2=bar', + '/paramerrors/no_positional_args_kwargs/foo/bar/baz?param2=bar¶m3=baz', + '/paramerrors/no_positional_kwargs?param1=foo¶m2=bar', + ): + self.getPage(uri) + self.assertStatus(200) + + # query string parameters are part of the URI, so if they are wrong + # for a particular handler, the status MUST be a 404. + for uri in ( + '/paramerrors/one_positional', + '/paramerrors/one_positional?foo=foo', + '/paramerrors/one_positional/foo/bar/baz', + '/paramerrors/one_positional/foo?param1=foo', + '/paramerrors/one_positional/foo?param1=foo¶m2=foo', + '/paramerrors/one_positional_args/foo?param1=foo¶m2=foo', + '/paramerrors/one_positional_args/foo/bar/baz?param2=foo', + '/paramerrors/one_positional_args_kwargs/foo/bar/baz?param1=bar¶m3=baz', + '/paramerrors/one_positional_kwargs/foo?param1=foo¶m2=bar¶m3=baz', + '/paramerrors/no_positional/boo', + '/paramerrors/no_positional?param1=foo', + '/paramerrors/no_positional_args/boo?param1=foo', + '/paramerrors/no_positional_kwargs/boo?param1=foo', + ): + self.getPage(uri) + self.assertStatus(404) + + # if body parameters are wrong, a 400 must be returned. + for uri, body in ( + ('/paramerrors/one_positional/foo', 'param1=foo',), + ('/paramerrors/one_positional/foo', 'param1=foo¶m2=foo',), + ('/paramerrors/one_positional_args/foo', 'param1=foo¶m2=foo',), + ('/paramerrors/one_positional_args/foo/bar/baz', 'param2=foo',), + ('/paramerrors/one_positional_args_kwargs/foo/bar/baz', 'param1=bar¶m3=baz',), + ('/paramerrors/one_positional_kwargs/foo', 'param1=foo¶m2=bar¶m3=baz',), + ('/paramerrors/no_positional', 'param1=foo',), + ('/paramerrors/no_positional_args/boo', 'param1=foo',), + ): + self.getPage(uri, method='POST', body=body) + self.assertStatus(400) + + + # even if body parameters are wrong, if we get the uri wrong, then + # it's a 404 + for uri, body in ( + ('/paramerrors/one_positional?param2=foo', 'param1=foo',), + ('/paramerrors/one_positional/foo/bar', 'param2=foo',), + ('/paramerrors/one_positional_args/foo/bar?param2=foo', 'param3=foo',), + ('/paramerrors/one_positional_kwargs/foo/bar', 'param2=bar¶m3=baz',), + ('/paramerrors/no_positional?param1=foo', 'param2=foo',), + ('/paramerrors/no_positional_args/boo?param2=foo', 'param1=foo',), + ): + self.getPage(uri, method='POST', body=body) + self.assertStatus(404) + + def testStatus(self): self.getPage("/status/") self.assertBody('normal') diff --git a/cherrypy/test/test_objectmapping.py b/cherrypy/test/test_objectmapping.py index 764a3513..65346021 100644 --- a/cherrypy/test/test_objectmapping.py +++ b/cherrypy/test/test_objectmapping.py @@ -268,11 +268,10 @@ class ObjectMappingTest(helper.CPWebCase): self.getPage("/dir1/dir2/5/3/sir") self.assertBody("default for dir1, param is:('dir2', '5', '3', 'sir')") - # test that extra positional args raises an error. - # 500 for now, maybe 404 in the future. + # test that extra positional args raises an 404 Not Found # See http://www.cherrypy.org/ticket/733. self.getPage("/dir1/dir2/script_name/extra/stuff") - self.assertStatus(500) + self.assertStatus(404) def testExpose(self): # Test the cherrypy.expose function/decorator |