Do not reject `/sync` requests with unrecognised filter fields #14369

squahtx · 2022-11-04T15:26:58Z

For forward compatibility, Synapse needs to ignore fields it does not
recognise instead of raising an error.

Signed-off-by: Sean Quah seanq@matrix.org

For forward compatibility, Synapse needs to ignore fields it does not recognise instead of raising an error. Fixes #14365. Signed-off-by: Sean Quah <seanq@matrix.org>

reivilibre

without wanting to ask the annoying question, should this have a Complement test or something that sends a bogus field? (potentially not TBH since you could say the same about every single endpoint ever)

Other annoying question: did this JSON schema come from the spec's JSON schema? Does that need to be fixed?

squahtx · 2022-11-04T17:31:29Z

should this have a Complement test or something that sends a bogus field?

It turns out we have existing trial tests that this change has broken:

synapse/tests/api/test_filtering.py

Lines 48 to 60 in 4283bd1

    
           def test_errors_on_invalid_filters(self): 
        
               invalid_filters = [ 
        
                   {"boom": {}}, 
        
                   {"account_data": "Hello World"}, 
        
                   {"event_fields": [r"\\foo"]}, 
        
                   {"room": {"timeline": {"limit": 0}, "state": {"not_bars": ["*"]}}}, 
        
                   {"event_format": "other"}, 
        
                   {"room": {"not_rooms": ["#foo:pik-test"]}}, 
        
                   {"presence": {"senders": ["@bar;pik.test.com"]}}, 
        
               ] 
        
               for filter in invalid_filters: 
        
                   with self.assertRaises(SynapseError): 
        
                       self.filtering.check_valid_filter(filter)

I'm minded to flip the expectations around in the failing test cases.

did this JSON schema come from the spec's JSON schema? Does that need to be fixed?

Good question. The schema in the spec don't specify additionalProperties for sync filters. I think that means it defaults to true?

https://github.com/matrix-org/matrix-spec/blob/main/data/api/client-server/definitions/event_filter.yaml
https://github.com/matrix-org/matrix-spec/blob/main/data/api/client-server/definitions/sync_filter.yaml
https://github.com/matrix-org/matrix-spec/blob/main/data/api/client-server/definitions/room_event_filter.yaml

squahtx · 2022-11-04T17:54:06Z

I've fixed the failing test and adapted it to create a test for unknown filter fields.

reivilibre · 2022-11-07T13:46:19Z

Good question. The schema in the spec don't specify additionalProperties for sync filters. I think that means it defaults to true?

AFAICT you are correct.

@ashfame

Synapse 1.72.0 (2022-11-22) =========================== Please note that Synapse now only supports PostgreSQL 11+, because PostgreSQL 10 has reached end-of-life, c.f. our [Deprecation Policy](https://github.com/matrix-org/synapse/blob/develop/docs/deprecation_policy.md). Bugfixes -------- - Update forgotten references to legacy metrics in the included Grafana dashboard. ([\matrix-org#14477](matrix-org#14477)) Synapse 1.72.0rc1 (2022-11-16) ============================== Features -------- - Add experimental support for [MSC3912](matrix-org/matrix-spec-proposals#3912): Relation-based redactions. ([\matrix-org#14260](matrix-org#14260)) - Build Debian packages for Ubuntu 22.10 (Kinetic Kudu). ([\matrix-org#14396](matrix-org#14396)) - Add an [Admin API](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html) endpoint for user lookup based on third-party ID (3PID). Contributed by @ashfame. ([\matrix-org#14405](matrix-org#14405)) - Faster joins: include heroes' membership events in the partial join response, for rooms without a name or canonical alias. ([\matrix-org#14442](matrix-org#14442)) Bugfixes -------- - Faster joins: do not block creation of or queries for room aliases during the resync. ([\matrix-org#14292](matrix-org#14292)) - Fix a bug introduced in Synapse 1.64.0rc1 which could cause log spam when fetching events from other homeservers. ([\matrix-org#14347](matrix-org#14347)) - Fix a bug introduced in 1.66 which would not send certain pushrules to clients. Contributed by Nico. ([\matrix-org#14356](matrix-org#14356)) - Fix a bug introduced in v1.71.0rc1 where the power level event was incorrectly created during initial room creation. ([\matrix-org#14361](matrix-org#14361)) - Fix the refresh token endpoint to be under /r0 and /v3 instead of /v1. Contributed by Tulir @ Beeper. ([\matrix-org#14364](matrix-org#14364)) - Fix a long-standing bug where Synapse would raise an error when encountering an unrecognised field in a `/sync` filter, instead of ignoring it for forward compatibility. ([\matrix-org#14369](matrix-org#14369)) - Fix a background database update, introduced in Synapse 1.64.0, which could cause poor database performance. ([\matrix-org#14374](matrix-org#14374)) - Fix PostgreSQL sometimes using table scans for queries against the `event_search` table, taking a long time and a large amount of IO. ([\matrix-org#14409](matrix-org#14409)) - Fix rendering of some HTML templates (including emails). Introduced in v1.71.0. ([\matrix-org#14448](matrix-org#14448)) - Fix a bug introduced in Synapse 1.70.0 where the background updates to add non-thread unique indexes on receipts could fail when upgrading from 1.67.0 or earlier. ([\matrix-org#14453](matrix-org#14453)) Updates to the Docker image --------------------------- - Add all Stream Writer worker types to `configure_workers_and_start.py`. ([\matrix-org#14197](matrix-org#14197)) - Remove references to legacy worker types in the multi-worker Dockerfile. ([\matrix-org#14294](matrix-org#14294)) Improved Documentation ---------------------- - Upload documentation PRs to Netlify. ([\matrix-org#12947](matrix-org#12947), [\matrix-org#14370](matrix-org#14370)) - Add addtional TURN server configuration example based on [eturnal](https://github.com/processone/eturnal) and adjust general TURN server doc structure. ([\matrix-org#14293](matrix-org#14293)) - Add example on how to load balance /sync requests. Contributed by [aceArt](https://aceart.de). ([\matrix-org#14297](matrix-org#14297)) - Edit sample Nginx reverse proxy configuration to use HTTP/1.1. Contributed by Brad Jones. ([\matrix-org#14414](matrix-org#14414)) Deprecations and Removals ------------------------- - Remove support for PostgreSQL 10. ([\matrix-org#14392](matrix-org#14392), [\matrix-org#14397](matrix-org#14397)) Internal Changes ---------------- - Run unit tests against Python 3.11. ([\matrix-org#13812](matrix-org#13812)) - Add TLS support for generic worker endpoints. ([\matrix-org#14128](matrix-org#14128), [\matrix-org#14455](matrix-org#14455)) - Switch to a maintained action for installing Rust in CI. ([\matrix-org#14313](matrix-org#14313)) - Add override ability to `complement.sh` command line script to request certain types of workers. ([\matrix-org#14324](matrix-org#14324)) - Enabling testing of [MSC3874](matrix-org/matrix-spec-proposals#3874) (filtering of `/messages` by relation type) in complement. ([\matrix-org#14339](matrix-org#14339)) - Concisely log a failure to resolve state due to missing `prev_events`. ([\matrix-org#14346](matrix-org#14346)) - Use a maintained Github action to install Rust. ([\matrix-org#14351](matrix-org#14351)) - Cleanup old worker datastore classes. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#14375](matrix-org#14375)) - Test against PostgreSQL 15 in CI. ([\matrix-org#14394](matrix-org#14394)) - Remove unreachable code. ([\matrix-org#14410](matrix-org#14410)) - Clean-up event persistence code. ([\matrix-org#14411](matrix-org#14411)) - Update docstring to clarify that `get_partial_state_events_batch` does not just give you completely arbitrary partial-state events. ([\matrix-org#14417](matrix-org#14417)) - Fix mypy errors introduced by bumping the locked version of `attrs` and `gitpython`. ([\matrix-org#14433](matrix-org#14433)) - Make Dependabot only bump Rust deps in the lock file. ([\matrix-org#14434](matrix-org#14434)) - Fix an incorrect stub return type for `PushRuleEvaluator.run`. ([\matrix-org#14451](matrix-org#14451)) - Improve performance of `/context` in large rooms. ([\matrix-org#14461](matrix-org#14461))

squahtx requested a review from a team as a code owner November 4, 2022 15:26

Do not reject /sync requests with unrecognised filter fields

3128451

For forward compatibility, Synapse needs to ignore fields it does not recognise instead of raising an error. Fixes #14365. Signed-off-by: Sean Quah <seanq@matrix.org>

squahtx force-pushed the squah/allow_unrecognised_sync_filter_fields branch from 062a251 to 3128451 Compare November 4, 2022 15:40

reivilibre approved these changes Nov 4, 2022

View reviewed changes

Update filter tests

3d0e0f0

squahtx requested a review from reivilibre November 4, 2022 17:53

reivilibre approved these changes Nov 7, 2022

View reviewed changes

squahtx merged commit e980982 into develop Nov 7, 2022

squahtx deleted the squah/allow_unrecognised_sync_filter_fields branch November 7, 2022 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not reject `/sync` requests with unrecognised filter fields #14369

Do not reject `/sync` requests with unrecognised filter fields #14369

squahtx commented Nov 4, 2022 •

edited

Loading

reivilibre left a comment •

edited

Loading

squahtx commented Nov 4, 2022

squahtx commented Nov 4, 2022

reivilibre commented Nov 7, 2022

Do not reject /sync requests with unrecognised filter fields #14369

Do not reject /sync requests with unrecognised filter fields #14369

Conversation

squahtx commented Nov 4, 2022 • edited Loading

reivilibre left a comment • edited Loading

Choose a reason for hiding this comment

squahtx commented Nov 4, 2022

squahtx commented Nov 4, 2022

reivilibre commented Nov 7, 2022

Do not reject `/sync` requests with unrecognised filter fields #14369

Do not reject `/sync` requests with unrecognised filter fields #14369

squahtx commented Nov 4, 2022 •

edited

Loading

reivilibre left a comment •

edited

Loading