Skip to content
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

Parse json validation #16923

Merged
merged 16 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog.d/16923.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Adds parse_json servlet function for standardized JSON parsing from query parameters, ensuring enhanced data validation and error handling.
Introduces INVALID_PARAM error response for invalid JSON objects, improving parameter validation feedback.
Adds validation check to prevent 500 internal server error on invalid Json Filter request.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally we try to keep changelog entries short, and acknowledge that the audience is system administrators. Such an audience won't care to know the details of the implementation, but rather than user-facing impact. My suggestion would be:

Suggested change
Adds parse_json servlet function for standardized JSON parsing from query parameters, ensuring enhanced data validation and error handling.
Introduces INVALID_PARAM error response for invalid JSON objects, improving parameter validation feedback.
Adds validation check to prevent 500 internal server error on invalid Json Filter request.
Return `400 M_INVALID_PARAM` upon receiving invalid JSON in query parameters across various client and admin endpoints, rather than an internal server error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the Insights! - I'll keep that in mind. 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you happy to accept my suggestion here? I prefer the suggested version of the changelog for the reasons in my initial comment.

81 changes: 81 additions & 0 deletions synapse/http/servlet.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
""" This module contains base REST classes for constructing REST servlets. """
import enum
import logging
import urllib.parse as urlparse
from http import HTTPStatus
from typing import (
TYPE_CHECKING,
Expand Down Expand Up @@ -428,6 +429,86 @@ def parse_string(
)


def parse_json(
request: Request,
name: str,
default: Optional[dict] = None,
required: bool = False,
encoding: str = "ascii",
) -> Optional[JsonDict]:
"""
Parse a JSON parameter from the request query string.

Args:
request: the twisted HTTP request.
name: the name of the query parameter.
default: value to use if the parameter is absent,
defaults to None.
required: whether to raise a 400 SynapseError if the
parameter is absent, defaults to False.
encoding: The encoding to decode the string content with.

Returns:
A JSON value.

Raises:
SynapseError if the parameter is absent or if the
parameter is present and not a JSON object.
"""
args: Mapping[bytes, Sequence[bytes]] = request.args # type: ignore
return parse_json_from_args(
args,
name,
default,
required=required,
encoding=encoding,
)


def parse_json_from_args(
args: Mapping[bytes, Sequence[bytes]],
name: str,
default: Optional[dict] = None,
required: bool = False,
encoding: str = "ascii",
) -> Optional[JsonDict]:
"""
Parse a JSON parameter from the request query string.

Args:
args: A mapping of request args as bytes to a list of bytes (e.g. request.args).
name: the name of the query parameter.
default: value to use if the parameter is absent,
defaults to None.
required: whether to raise a 400 SynapseError if the
parameter is absent, defaults to False.
encoding: The encoding to decode the string content with.

Returns:
A JSON Object or the default.

Raises:
SynapseError if the parameter is absent and required, or if the
parameter is present and not a JSON object.
"""
name_bytes = name.encode("ascii")

if name_bytes not in args:
if not required:
return default

message = f"Missing required integer query parameter {name}"
raise SynapseError(HTTPStatus.BAD_REQUEST, message, errcode=Codes.MISSING_PARAM)

json_str = parse_string_from_args(args, name, required=True, encoding=encoding)

try:
return json_decoder.decode(urlparse.unquote(json_str))
except Exception:
message = f"Query parameter {name} must be a valid JSON object"
raise SynapseError(HTTPStatus.BAD_REQUEST, message, errcode=Codes.INVALID_PARAM)


EnumT = TypeVar("EnumT", bound=enum.Enum)


Expand Down
34 changes: 12 additions & 22 deletions synapse/rest/admin/rooms.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
assert_params_in_dict,
parse_enum,
parse_integer,
parse_json,
parse_json_object_from_request,
parse_string,
)
Expand Down Expand Up @@ -776,14 +777,8 @@ async def on_GET(
limit = parse_integer(request, "limit", default=10)

# picking the API shape for symmetry with /messages
filter_str = parse_string(request, "filter", encoding="utf-8")
if filter_str:
filter_json = urlparse.unquote(filter_str)
event_filter: Optional[Filter] = Filter(
self._hs, json_decoder.decode(filter_json)
)
else:
event_filter = None
filter_json = parse_json(request, "filter", encoding="utf-8")
event_filter = Filter(self._hs, filter_json) if filter_json else None

event_context = await self.room_context_handler.get_event_context(
requester,
Expand Down Expand Up @@ -914,21 +909,16 @@ async def on_GET(
)
# Twisted will have processed the args by now.
assert request.args is not None

filter_json = parse_json(request, "filter", encoding="utf-8")
event_filter = Filter(self._hs, filter_json) if filter_json else None

as_client_event = b"raw" not in request.args
filter_str = parse_string(request, "filter", encoding="utf-8")
if filter_str:
filter_json = urlparse.unquote(filter_str)
event_filter: Optional[Filter] = Filter(
self._hs, json_decoder.decode(filter_json)
)
if (
event_filter
and event_filter.filter_json.get("event_format", "client")
== "federation"
):
as_client_event = False
else:
event_filter = None
if (
event_filter
and event_filter.filter_json.get("event_format", "client") == "federation"
):
as_client_event = False

msgs = await self._pagination_handler.get_messages(
room_id=room_id,
Expand Down
35 changes: 12 additions & 23 deletions synapse/rest/client/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
parse_boolean,
parse_enum,
parse_integer,
parse_json,
parse_json_object_from_request,
parse_string,
parse_strings_from_args,
Expand All @@ -65,7 +66,6 @@
from synapse.streams.config import PaginationConfig
from synapse.types import JsonDict, Requester, StreamToken, ThirdPartyInstanceID, UserID
from synapse.types.state import StateFilter
from synapse.util import json_decoder
from synapse.util.cancellation import cancellable
from synapse.util.stringutils import parse_and_validate_server_name, random_string

Expand Down Expand Up @@ -703,21 +703,16 @@ async def on_GET(
)
# Twisted will have processed the args by now.
assert request.args is not None

filter_json = parse_json(request, "filter", encoding="utf-8")
event_filter = Filter(self._hs, filter_json) if filter_json else None

as_client_event = b"raw" not in request.args
filter_str = parse_string(request, "filter", encoding="utf-8")
if filter_str:
filter_json = urlparse.unquote(filter_str)
event_filter: Optional[Filter] = Filter(
self._hs, json_decoder.decode(filter_json)
)
if (
event_filter
and event_filter.filter_json.get("event_format", "client")
== "federation"
):
as_client_event = False
else:
event_filter = None
if (
event_filter
and event_filter.filter_json.get("event_format", "client") == "federation"
):
as_client_event = False

msgs = await self.pagination_handler.get_messages(
room_id=room_id,
Expand Down Expand Up @@ -898,14 +893,8 @@ async def on_GET(
limit = parse_integer(request, "limit", default=10)

# picking the API shape for symmetry with /messages
filter_str = parse_string(request, "filter", encoding="utf-8")
if filter_str:
filter_json = urlparse.unquote(filter_str)
event_filter: Optional[Filter] = Filter(
self._hs, json_decoder.decode(filter_json)
)
else:
event_filter = None
filter_json = parse_json(request, "filter", encoding="utf-8")
event_filter = Filter(self._hs, filter_json) if filter_json else None

event_context = await self.room_context_handler.get_event_context(
requester, room_id, event_id, limit, event_filter
Expand Down