-
Notifications
You must be signed in to change notification settings - Fork 21
Add validation of query parameter for server requests #1230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
896af49
3bba90c
b21b3cb
20cb143
d3a05bf
8a215ef
fa2983e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,7 +186,11 @@ def optional_apis(self) -> tuple[str]: | |
| return self._optional_apis | ||
|
|
||
| def static_route( | ||
| self, path: str, default_filename: Optional[str] = None, **openapi_metadata | ||
| self, | ||
| path: str, | ||
| default_filename: Optional[str] = None, | ||
| allowed_query_params: Optional[Sequence[str]] = None, | ||
| **openapi_metadata, | ||
| ): | ||
| """Decorator that adds static route to this API. | ||
|
|
||
|
|
@@ -198,6 +202,9 @@ def static_route( | |
| path: The route path. | ||
| default_filename: Optional default filename, e.g., | ||
| "index.html". | ||
| allowed_query_params: Optional names of query parameters allowed | ||
| for this static route. If given, other query parameters should | ||
| be rejected by web framework implementations. | ||
|
Comment on lines
+205
to
+207
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we only protect the routes that use this argument. This is currently only |
||
| **openapi_metadata: Optional OpenAPI GET operation metadata. | ||
| """ | ||
|
|
||
|
|
@@ -210,6 +217,7 @@ def decorator_func(get_root_path: Callable[[], Optional[str]]): | |
| str(root_path), | ||
| api_name=self.name, | ||
| default_filename=default_filename, | ||
| allowed_query_params=allowed_query_params, | ||
| openapi_metadata=openapi_metadata, | ||
| ) | ||
| ) | ||
|
|
@@ -825,6 +833,9 @@ class ApiStaticRoute: | |
| default_filename: Optional default filename, e.g., "index.html". | ||
| api_name: Optional name of the API to which this route belongs | ||
| to. | ||
| allowed_query_params: Optional names of query parameters allowed | ||
| for this static route. If given, other query parameters should | ||
| be rejected by web framework implementations. | ||
| openapi_metadata: Optional OpenAPI operation metadata. | ||
| """ | ||
|
|
||
|
|
@@ -834,6 +845,7 @@ def __init__( | |
| dir_path: str, | ||
| default_filename: Optional[str] = None, | ||
| api_name: Optional[str] = None, | ||
| allowed_query_params: Optional[Sequence[str]] = None, | ||
| openapi_metadata: Optional[dict[str, Any]] = None, | ||
| ): | ||
| assert_instance(path, str, name="path") | ||
|
|
@@ -843,11 +855,17 @@ def __init__( | |
| ) | ||
| assert_instance(default_filename, (type(None), str), name="default_filename") | ||
| assert_instance(api_name, (type(None), str), name="api_name") | ||
| if allowed_query_params is not None: | ||
| assert_true( | ||
| all(isinstance(param, str) for param in allowed_query_params), | ||
| message="allowed_query_params must contain strings", | ||
| ) | ||
| assert_instance(openapi_metadata, (type(None), dict), name="openapi_metadata") | ||
| self.path = path | ||
| self.dir_path = dir_path | ||
| self.default_filename = default_filename | ||
| self.api_name = api_name | ||
| self.allowed_query_params = tuple(allowed_query_params or ()) | ||
| self.openapi_metadata = openapi_metadata | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,45 @@ | |
|
|
||
| SERVER_CTX_ATTR_NAME = "__xcube_server_ctx" | ||
|
|
||
| def _assert_allowed_query_params( | ||
| request: tornado.httputil.HTTPServerRequest, allowed_query_params: Sequence[str] | ||
| ): | ||
| unknown_query_params = set(request.query_arguments) - set(allowed_query_params) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Query parameters should be, afaik, not case-sensitive. |
||
| if unknown_query_params: | ||
| raise tornado.web.HTTPError( | ||
| 404, | ||
| reason=( | ||
| f"Unknown query parameter(s): {', '.join(sorted(unknown_query_params))}" | ||
| ), | ||
| ) | ||
|
|
||
| class QueryValidatingRedirectHandler(tornado.web.RedirectHandler): | ||
| def initialize( | ||
| self, | ||
| url: str, | ||
| permanent: bool = True, | ||
| allowed_query_params: Sequence[str] = (), | ||
| ): | ||
| super().initialize(url, permanent=permanent) | ||
| self._allowed_query_params = allowed_query_params | ||
|
|
||
| def prepare(self): | ||
| _assert_allowed_query_params(self.request, self._allowed_query_params) | ||
|
|
||
|
|
||
| class QueryValidatingStaticFileHandler(tornado.web.StaticFileHandler): | ||
| def initialize( | ||
| self, | ||
| path: str, | ||
| default_filename: Optional[str] = None, | ||
| allowed_query_params: Sequence[str] = (), | ||
| ): | ||
| super().initialize(path=path, default_filename=default_filename) | ||
| self._allowed_query_params = allowed_query_params | ||
|
|
||
| def prepare(self): | ||
| _assert_allowed_query_params(self.request, self._allowed_query_params) | ||
|
|
||
|
|
||
| class TornadoFramework(Framework): | ||
| """ | ||
|
|
@@ -103,15 +142,30 @@ def add_static_routes(self, api_routes: Sequence[ApiStaticRoute], url_prefix: st | |
| for api_route in api_routes: | ||
| base_url = f"{url_prefix}{api_route.path}" | ||
| default_filename = api_route.default_filename | ||
| allowed_query_params = api_route.allowed_query_params | ||
| if allowed_query_params: | ||
| redirect_handler = QueryValidatingRedirectHandler | ||
| static_file_handler = QueryValidatingStaticFileHandler | ||
| redirect_kwargs = { | ||
| "url": f"{base_url}/", | ||
| "allowed_query_params": allowed_query_params, | ||
| } | ||
| static_file_kwargs = { | ||
| "path": api_route.dir_path, | ||
| "default_filename": default_filename, | ||
| "allowed_query_params": allowed_query_params, | ||
| } | ||
| else: | ||
| redirect_handler = tornado.web.RedirectHandler | ||
| static_file_handler = tornado.web.StaticFileHandler | ||
| redirect_kwargs = {"url": f"{base_url}/"} | ||
| static_file_kwargs = { | ||
| "path": api_route.dir_path, | ||
| "default_filename": default_filename, | ||
| } | ||
| handlers.append((f"{base_url}", redirect_handler, redirect_kwargs)) | ||
| handlers.append( | ||
| (f"{base_url}", tornado.web.RedirectHandler, {"url": f"{base_url}/"}) | ||
| ) | ||
| handlers.append( | ||
| ( | ||
| f"{base_url}/(.*)", | ||
| tornado.web.StaticFileHandler, | ||
| {"path": api_route.dir_path, "default_filename": default_filename}, | ||
| ) | ||
| (f"{base_url}/(.*)", static_file_handler, static_file_kwargs) | ||
| ) | ||
| LOG.log( | ||
| LOG_LEVEL_DETAIL, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.