Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Incorrect SQL query construction in _construct_room_type_where_clause #14051

Closed
DMRobertson opened this issue Oct 4, 2022 · 1 comment · Fixed by #14053
Closed

Incorrect SQL query construction in _construct_room_type_where_clause #14051

DMRobertson opened this issue Oct 4, 2022 · 1 comment · Fixed by #14053
Assignees
Labels
O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Sentry Issue was discovered by looking at Sentry reports on Matrix.org

Comments

@DMRobertson
Copy link
Contributor

To reproduce:

curl -H "Authorization: Bearer $TOKEN" -X POST https://matrix-client.matrix.org/_matrix/client/v3/publicRooms -d '{"filter": {"generic_search_term": "debian-ou", "room_types": [null]}, "third_party_instance_id": "irc-w3c|w3" }'

which yields

{"errcode":"M_UNKNOWN","error":"Internal server error"}

Internal users: example Sentry traceback.

I believe the cause is faulty logic in:

def _construct_room_type_where_clause(
self, room_types: Union[List[Union[str, None]], None]
) -> Tuple[Union[str, None], List[str]]:
if not room_types:
return None, []
else:
# We use None when we want get rooms without a type
is_null_clause = ""
if None in room_types:
is_null_clause = "OR room_type IS NULL"
room_types = [value for value in room_types if value is not None]
list_clause, args = make_in_list_sql_clause(
self.database_engine, "room_type", room_types
)
return f"({list_clause} {is_null_clause})", args

Suggest:

  • unit test to reproduce this
  • something like the following:
# within the else block:
conditions = []
args = []
specific_room_types = [value for value in room_types if value is not None]
if None in room_types:
    conditions.append("room_type IS NULL")
if specific_room_types:
    condition, condition_args =make_in_list_sql_clause(..., specific_room_types)
    conditions.append(clause)
    args.append(condition_args)

return f"({ ' OR '.join(conditions) })", args

Likely an oversight in #13031.

@DMRobertson DMRobertson added S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. O-Occasional Affects or can be seen by some users regularly or most users rarely labels Oct 4, 2022
@clokep clokep self-assigned this Oct 4, 2022
@clokep
Copy link
Member

clokep commented Oct 4, 2022

There's a couple of things going wrong here:

  • The handling of [None] was not quite correct (as you'd end up with the wrong number of arguments).
  • The ordering of arguments when providing both a network tuple and room type field was incorrect.

@DMRobertson DMRobertson added the Z-Sentry Issue was discovered by looking at Sentry reports on Matrix.org label Oct 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Sentry Issue was discovered by looking at Sentry reports on Matrix.org
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants