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

publicRooms validation error & parse_integer_from_args changes #16918

Closed
TrevisGordan opened this issue Feb 14, 2024 · 0 comments · Fixed by #16920
Closed

publicRooms validation error & parse_integer_from_args changes #16918

TrevisGordan opened this issue Feb 14, 2024 · 0 comments · Fixed by #16920

Comments

@TrevisGordan
Copy link
Contributor

Description

During testing, I encountered some 500 errors due to missing or bad validation.
For example, related to:
#13147
#14223

curl -X GET 'http://matrix.localhost/_matrix/client/v3/publicRooms?limit=-2&access_token=<TOKEN>'

-> limit=-2 causing an error on the DB level (if run with PostgreSQL), missing validation.

I've noticed there are already some motions for better parameter validation on the REST endpoints.
Ill go ahead and create a PR to fix this - My first change would be expanding the parse_integer_from_args function to optionally check and validate negative values. And move the related SynapseError INVALID_PARAM logic into it.

In addition, this change would allow the removal of around 20 duplicate code blocks.

  • Change function
  • Fix publicRooms validation
  • Remove duplicate code lines

Steps to reproduce

Homeserver

local

Synapse Version

1.94.0

Installation Method

Docker (matrixdotorg/synapse)

Database

PostgreSQL

Workers

Multiple workers

Platform

kubernetes Dockerdesktop

Configuration

No response

Relevant log output

"""
2024-01-31 08:26:26,665 - synapse.http.server - 140 - ERROR - GET-122048 - Failed handle request via 'PublicRoomListRestServlet': <XForwardedForRequest at 0x7fffddf62640 method='GET' uri='/_matrix/client/v3/publicRooms?limit=-2&access_token=<redacted>' clientproto='HTTP/1.1' site='8083'>
"""
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/dist-packages/twisted/internet/defer.py", line 1993, in _inlineCallbacks
    result = context.run(
  File "/usr/local/lib/python3.9/dist-packages/twisted/python/failure.py", line 518, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "/usr/local/lib/python3.9/dist-packages/synapse/util/caches/response_cache.py", line 258, in cb
    return await callback(*args, **kwargs)
  File "/usr/local/lib/python3.9/dist-packages/synapse/handlers/room_list.py", line 164, in _get_public_room_list
    results = await self.store.get_largest_public_rooms(
  File "/usr/local/lib/python3.9/dist-packages/synapse/storage/databases/main/room.py", line 518, in get_largest_public_rooms
    ret_val = await self.db_pool.runInteraction(
  File "/usr/local/lib/python3.9/dist-packages/synapse/storage/database.py", line 934, in runInteraction
    return await delay_cancellation(_runInteraction())
  File "/usr/local/lib/python3.9/dist-packages/twisted/internet/defer.py", line 1993, in _inlineCallbacks
    result = context.run(
  File "/usr/local/lib/python3.9/dist-packages/twisted/python/failure.py", line 518, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "/usr/local/lib/python3.9/dist-packages/synapse/storage/database.py", line 900, in _runInteraction
    result = await self.runWithConnection(
  File "/usr/local/lib/python3.9/dist-packages/synapse/storage/database.py", line 1029, in runWithConnection
    return await make_deferred_yieldable(
  File "/usr/local/lib/python3.9/dist-packages/twisted/python/threadpool.py", line 244, in inContext
    result = inContext.theWork()  # type: ignore[attr-defined]
  File "/usr/local/lib/python3.9/dist-packages/twisted/python/threadpool.py", line 260, in <lambda>
    inContext.theWork = lambda: context.call(  # type: ignore[attr-defined]
  File "/usr/local/lib/python3.9/dist-packages/twisted/python/context.py", line 117, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/usr/local/lib/python3.9/dist-packages/twisted/python/context.py", line 82, in callWithContext
    return func(*args, **kw)
  File "/usr/local/lib/python3.9/dist-packages/twisted/enterprise/adbapi.py", line 282, in _runWithConnection
    result = func(conn, *args, **kw)
  File "/usr/local/lib/python3.9/dist-packages/synapse/storage/database.py", line 1022, in inner_func
    return func(db_conn, *args, **kwargs)
  File "/usr/local/lib/python3.9/dist-packages/synapse/storage/database.py", line 762, in new_transaction
    r = func(cursor, *args, **kwargs)
  File "/usr/local/lib/python3.9/dist-packages/synapse/storage/databases/main/room.py", line 509, in _get_largest_public_rooms_txn
    txn.execute(sql, query_args)
  File "/usr/local/lib/python3.9/dist-packages/synapse/storage/database.py", line 421, in execute
    self._do_execute(self.txn.execute, sql, parameters)
  File "/usr/local/lib/python3.9/dist-packages/synapse/storage/database.py", line 473, in _do_execute
    return func(sql, *args, **kwargs)
psycopg2.errors.InvalidRowCountInLimitClause: LIMIT must not be negative

Anything else that would be useful to know?

No response

TrevisGordan added a commit to TrevisGordan/synapse that referenced this issue Feb 14, 2024
fixes element-hq#16918 500 internal server error on negative limit parameter (with PostgreSQL)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant