Add validation of query parameter for server requests#1230
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1230 +/- ##
==========================================
- Coverage 89.61% 89.60% -0.01%
==========================================
Files 280 280
Lines 21694 21724 +30
==========================================
+ Hits 19440 19466 +26
- Misses 2254 2258 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
forman
left a comment
There was a problem hiding this comment.
Looks good to me, but please consider my suggestions. We may have to discuss this further in more detail.
| 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) |
There was a problem hiding this comment.
Query parameters should be, afaik, not case-sensitive.
| 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. |
There was a problem hiding this comment.
So we only protect the routes that use this argument.
This is currently only /viewer. An routes of the form /viewer/.../...? What others routes like /datasets?
Suggestion: allowed_query_params can also be a bool. If True, it can have any query params (like it is now). if False, any query parameters are forbidden. The new default would False and we need to check, which other endpoints must be protected.
Co-authored-by: Norman Fomferra <norman.fomferra@brockmann-consult.de>
Summary
This PR adds validation of allowed query parameters for requests to static routes and configures the
/api/viewerroute to reject unknown URL query parameters.This prevents unrelated scanner requests such as
?rest_route=/wp/v2/users/from being served as the Viewer index.html with200 OK.Testing
Run the server locally:
In other terminal run:
Closes #1225.
Checklist:
Add docstrings and API docs for any new/modified user-facing classes and functionsNew/modified features documented indocs/source/*CHANGES.md