diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e0c95c419..92e6722c1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -436,7 +436,7 @@ jobs: # run sliding sync and point it at the postgres container and synapse container. # the postgres container needs to be above this to make sure it has started prior to this service. slidingsync: - image: "ghcr.io/matrix-org/sliding-sync:v0.99.0" + image: "ghcr.io/matrix-org/sliding-sync:v0.99.2" env: SYNCV3_SERVER: "http://synapse:8008" SYNCV3_SECRET: "SUPER_CI_SECRET" diff --git a/crates/matrix-sdk/src/sliding_sync/list/mod.rs b/crates/matrix-sdk/src/sliding_sync/list/mod.rs index 6f8a8b356..81ea9cf89 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/mod.rs @@ -615,7 +615,7 @@ fn apply_sync_operations( // > knew about entries in this range. v4::SlidingOp::Sync => { // Extract `start` and `end` from the operation's range. - let (start, mut end) = operation + let (start, end) = operation .range .ok_or_else(|| { Error::BadResponse( @@ -629,23 +629,6 @@ fn apply_sync_operations( ) })?; - // The `end` bound of the range might not be correct… At the time of writing, - // there is a bug in the Sliding Sync Proxy that can return - // ranges greater than the `room_list` size. - // - // For example, if the client asks for a range `0..=9`, and there is only one - // room, the server will reply with one `room_id` (which is correct) but with - // the range `0..=9` instead of `0..=0`. - // - // So, a safe workaround is to take the minimum between `end` and the - // `room_list`'s length. - // - // The “safety” is ensured by the fact we also compare the size of the new range - // with the size of the `operation.room_ids` length later on. - // - // See https://github.com/matrix-org/sliding-sync/issues/52. - end = min(end, room_list.len()); - // Range is invalid. if start > end { return Err(Error::BadResponse(format!( @@ -654,7 +637,16 @@ fn apply_sync_operations( ))); } - let mut room_entry_range = start..end; + // Range is too big. + if end > room_list.len() { + return Err(Error::BadResponse(format!( + "`range` is out of the `rooms_list`'s bounds ({} > {})", + end, + room_list.len(), + ))); + } + + let room_entry_range = start..end; // `room_ids` is absent. if operation.room_ids.is_empty() { @@ -667,18 +659,6 @@ fn apply_sync_operations( // Mismatch between the `range` and `room_ids`. if room_entry_range.len() != room_ids.len() { - // Because of https://github.com/matrix-org/sliding-sync/issues/52, we - // can't trust the `range` returned by the server. That's a - // problem. Let's try to work around - // that. - // - // Let's pretend the `start` bound of the range is… correct. - room_entry_range = start..room_ids.len(); - - // Once the bug is fixed on the Sliding Sync Proxy side, we - // can remove this code, and uncomment - // the code below. - /* return Err(Error::BadResponse( format!( "There is a mismatch between the number of items in `range` and `room_ids` ({} != {})", @@ -686,7 +666,6 @@ fn apply_sync_operations( room_ids.len(), ) )); - */ } // Update parts `room_list`. @@ -783,7 +762,7 @@ fn apply_sync_operations( // > arrive from the server. v4::SlidingOp::Invalidate => { // Extract `start` and `end` from the operation's range. - let (start, mut end) = operation + let (start, end) = operation .range .ok_or_else(|| { Error::BadResponse( @@ -797,23 +776,6 @@ fn apply_sync_operations( ) })?; - // The `end` bound of the range might not be correct… At the time of writing, - // there is a bug in the Sliding Sync Proxy that can return - // ranges greater than the `room_list` size. - // - // For example, if the client asks for a range `0..=9`, and there is only one - // room, the server will reply with one `room_id` (which is correct) but with - // the range `0..=9` instead of `0..=0`. - // - // So, a safe workaround is to take the minimum between `end` and the - // `room_list`'s length. - // - // The “safety” is ensured by the fact we also compare the size of the new range - // with the size of the `operation.room_ids` length later on. - // - // See https://github.com/matrix-org/sliding-sync/issues/52. - end = min(end, room_list.len()); - // Range is invalid. if start > end { return Err(Error::BadResponse(format!( @@ -822,6 +784,15 @@ fn apply_sync_operations( ))); } + // Range is too big. + if end > room_list.len() { + return Err(Error::BadResponse(format!( + "`range` is out of the `room_list`' bounds ({} > {})", + end, + room_list.len(), + ))); + } + let room_entry_range = start..end; // Invalidate parts of `room_list`. @@ -1781,6 +1752,7 @@ mod tests { macro_rules! assert_sync_operations { ( + $assert_description:literal : room_list = [ $( $room_list_entries:tt )* ], sync_operations = [ $( @@ -1817,8 +1789,13 @@ mod tests { let result = apply_sync_operations(operations, &mut room_list, &mut rooms_that_have_received_an_update); - assert!(result.$result()); - assert_eq!(*room_list, entries![ $( $expected_room_list_entries )* ]); + assert!(result.$result(), "{}; assert the `Result`", $assert_description); + assert_eq!( + *room_list, + entries![ $( $expected_room_list_entries )* ], + "{}; asserting the `room_list`", + $assert_description, + ); $( #[allow(unused_mut)] @@ -1830,15 +1807,20 @@ mod tests { )* } - assert_eq!(rooms_that_have_received_an_update, expected_rooms_that_have_received_an_update); + assert_eq!( + rooms_that_have_received_an_update, + expected_rooms_that_have_received_an_update, + "{}; asserting the rooms that have received an update", + $assert_description, + ); )? }; } #[test] fn test_sync_operations_sync() { - // All room list is updated. assert_sync_operations! { + "All room list is updated": room_list = [E, E, E, F("!r3:x.y")], sync_operations = [ { @@ -1854,8 +1836,8 @@ mod tests { rooms = ["!r3:x.y"], }; - // Partial update. assert_sync_operations! { + "Partial update": room_list = [E, E, E], sync_operations = [ { @@ -1868,7 +1850,9 @@ mod tests { result = is_ok, room_list = [F("!r0:x.y"), F("!r1:x.y"), E], }; + assert_sync_operations! { + "Partial update": room_list = [E, E, E], sync_operations = [ { @@ -1882,26 +1866,23 @@ mod tests { room_list = [E, F("!r1:x.y"), F("!r2:x.y")], }; - // The range returned by the server is too large compared to the `room_ids` but - // we can fix it on-the-fly. - // - // See https://github.com/matrix-org/sliding-sync/issues/52. assert_sync_operations! { + "The range returned by the server is too large compared to the `room_ids`": room_list = [E], sync_operations = [ { "op": SlidingOp::Sync, - "range": [0, 9], // <- it should be [0, 0] + "range": [0, 2], // <- it should be [0, 0] "room_ids": ["!r0:x.y"], } ] => - result = is_ok, // <- because we have fixed it - room_list = [F("!r0:x.y")], + result = is_err, + room_list = [E], }; - // Missing `range`. assert_sync_operations! { + "Missing `range`": room_list = [E, E, E], sync_operations = [ { @@ -1914,8 +1895,8 @@ mod tests { room_list = [E, E, E], }; - // Invalid `range`. assert_sync_operations! { + "Invalid `range`": room_list = [E, E, E], sync_operations = [ { @@ -1929,8 +1910,8 @@ mod tests { room_list = [E, E, E], }; - // Missing `room_ids`. assert_sync_operations! { + "Missing `room_ids`": room_list = [E, E, E], sync_operations = [ { @@ -1943,9 +1924,9 @@ mod tests { room_list = [E, E, E], }; - // Out of bounds operation. assert_sync_operations! { - room_list = [E, E, E], + "Out of bounds operation": + room_list = [E, F("!r1:x.y"), E], sync_operations = [ { "op": SlidingOp::Sync, @@ -1954,19 +1935,12 @@ mod tests { } ] => - // As soon https://github.com/matrix-org/sliding-sync/issues/52 - // is fixed, let's uncomment the real test. - result = is_ok, - room_list = [E, E, E], - /* result = is_err, - room_list = [E, E, E], - */ + room_list = [E, F("!r1:x.y"), E], }; - // The server replies with a particular range, but some room IDs are - // missing. assert_sync_operations! { + "The server replies with a particular range, but some room IDs are missing": room_list = [E, E, E], sync_operations = [ { @@ -1976,19 +1950,12 @@ mod tests { } ] => - // As soon https://github.com/matrix-org/sliding-sync/issues/52 - // is fixed, let's uncomment the real test. - result = is_ok, - room_list = [F("!r0:x.y"), E, E], - /* result = is_err, room_list = [E, E, E], - */ }; - // The server replies with a particular range, but there is too much - // room IDs. assert_sync_operations! { + "The server replies with a particular range, but there is too much room IDs": room_list = [E, E, E], sync_operations = [ { @@ -1998,21 +1965,15 @@ mod tests { } ] => - // As soon https://github.com/matrix-org/sliding-sync/issues/52 - // is fixed, let's uncomment the real test. - result = is_ok, - room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!extra:x.y")], - /* result = is_err, room_list = [E, E, E], - */ }; } #[test] fn test_sync_operations_delete() { - // Delete a room entry in the middle. assert_sync_operations! { + "Delete a room entry in the middle": room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], sync_operations = [ { @@ -2027,8 +1988,8 @@ mod tests { rooms = ["!r0:x.y"], }; - // Delete a room entry at the beginning. assert_sync_operations! { + "Delete a room entry at the beginning": room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], sync_operations = [ { @@ -2041,8 +2002,8 @@ mod tests { room_list = [F("!r1:x.y"), F("!r2:x.y")], }; - // Delete a room entry at the end. assert_sync_operations! { + "Delete a room entry at the end": room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], sync_operations = [ { @@ -2055,8 +2016,8 @@ mod tests { room_list = [F("!r0:x.y"), F("!r1:x.y")], }; - // Delete an out of bounds room entry. assert_sync_operations! { + "Delete an out of bounds room entry": room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], sync_operations = [ { @@ -2072,8 +2033,8 @@ mod tests { #[test] fn test_sync_operations_insert() { - // Insert a room entry in the middle. assert_sync_operations! { + "Insert a room entry in the middle": room_list = [E, E, E], sync_operations = [ { @@ -2089,8 +2050,8 @@ mod tests { rooms = ["!r0:x.y"], }; - // Insert a room entry at the beginning. assert_sync_operations! { + "Insert a room entry at the beginning": room_list = [E, E, E], sync_operations = [ { @@ -2104,8 +2065,8 @@ mod tests { room_list = [F("!r0:x.y"), E, E, E], }; - // Insert a room entry at the end assert_sync_operations! { + "Insert a room entry at the end": room_list = [E, E, E], sync_operations = [ { @@ -2119,8 +2080,8 @@ mod tests { room_list = [E, E, E, F("!r3:x.y")], }; - // Insert an out of bounds room entry. assert_sync_operations! { + "Insert an out of bounds room entry": room_list = [E, F("!r1:x.y"), E], sync_operations = [ { @@ -2137,8 +2098,8 @@ mod tests { #[test] fn test_sync_operations_invalidate() { - // Invalidating an empty room. assert_sync_operations! { + "Invalidating an empty room": room_list = [E, F("!r1:x.y")], sync_operations = [ { @@ -2153,8 +2114,8 @@ mod tests { rooms = ["!r1:x.y"], }; - // Invalidating a filled room. assert_sync_operations! { + "Invalidating a filled room": room_list = [F("!r0:x.y"), F("!r1:x.y")], sync_operations = [ { @@ -2169,8 +2130,8 @@ mod tests { rooms = ["!r1:x.y"], }; - // Invalidating an invalidated room. assert_sync_operations! { + "Invalidating an invalidated room": room_list = [I("!r0:x.y"), F("!r1:x.y")], sync_operations = [ { @@ -2185,8 +2146,8 @@ mod tests { rooms = ["!r1:x.y"], }; - // Partial update. assert_sync_operations! { + "Partial update from the beginning": room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], sync_operations = [ { @@ -2198,7 +2159,9 @@ mod tests { result = is_ok, room_list = [I("!r0:x.y"), I("!r1:x.y"), F("!r2:x.y")], }; + assert_sync_operations! { + "Partial update from the end": room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], sync_operations = [ { @@ -2211,8 +2174,8 @@ mod tests { room_list = [F("!r0:x.y"), I("!r1:x.y"), I("!r2:x.y")], }; - // Full update. assert_sync_operations! { + "Full update": room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], sync_operations = [ { @@ -2225,11 +2188,8 @@ mod tests { room_list = [I("!r0:x.y"), I("!r1:x.y"), I("!r2:x.y")], }; - // The range returned by the server is too large compared to the `room_lists` - // but we can fix it on-the-fly. - // - // See https://github.com/matrix-org/sliding-sync/issues/52. assert_sync_operations! { + "The range returned by the server is too large compared to the `room_lists`": room_list = [F("!r0:x.y")], sync_operations = [ { @@ -2238,16 +2198,16 @@ mod tests { } ] => - result = is_ok, // <- because we have fixed it - room_list = [I("!r0:x.y")], + result = is_err, + room_list = [F("!r0:x.y")], }; - // Missing `range`. assert_sync_operations! { + "Missing `range`": room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], sync_operations = [ { - "op": SlidingOp::Invalidate, + "op": SlidingOp::Delete, } ] => @@ -2255,12 +2215,12 @@ mod tests { room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], }; - // Invalid `range`. assert_sync_operations! { + "Invalid `range`": room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], sync_operations = [ { - "op": SlidingOp::Invalidate, + "op": SlidingOp::Delete, "range": [12, 0], } ] @@ -2268,5 +2228,19 @@ mod tests { result = is_err, room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], }; + + assert_sync_operations! { + "Out of bounds operation": + room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], + sync_operations = [ + { + "op": SlidingOp::Delete, + "range": [2, 3], + } + ] + => + result = is_err, + room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], + }; } } diff --git a/testing/sliding-sync-integration-test/assets/docker-compose.yml b/testing/sliding-sync-integration-test/assets/docker-compose.yml index f906b9edd..3b86c7182 100644 --- a/testing/sliding-sync-integration-test/assets/docker-compose.yml +++ b/testing/sliding-sync-integration-test/assets/docker-compose.yml @@ -28,7 +28,7 @@ services: - ./data/db:/var/lib/postgresql/data sliding-sync-proxy: - image: ghcr.io/matrix-org/sliding-sync:v0.99.1 + image: ghcr.io/matrix-org/sliding-sync:v0.99.2 depends_on: postgres: condition: service_healthy