diff --git a/changelog.d/space-hierarchy-cycle.bugfix.md b/changelog.d/space-hierarchy-cycle.bugfix.md new file mode 100644 index 000000000..f962a7612 --- /dev/null +++ b/changelog.d/space-hierarchy-cycle.bugfix.md @@ -0,0 +1 @@ +Fixed the client space hierarchy endpoint returning a 500 "Space hierarchy is unreasonably large" error for cyclic space graphs (e.g. a space containing itself). Rooms are now deduplicated during traversal as required by the spec, and the traversal depth is bounded even when the client does not specify `max_depth`. diff --git a/src/api/client/space.rs b/src/api/client/space.rs index ed23f5897..1cd0bcbf5 100644 --- a/src/api/client/space.rs +++ b/src/api/client/space.rs @@ -19,9 +19,14 @@ pub(crate) async fn get_hierarchy_route( // there's no reasonable way to handle a space hierarchy changing during // pagination. - let max_depth = body - .max_depth - .map(|max_depth| max_depth.min(UInt::from(MAX_MAX_DEPTH))); + // Default to MAX_MAX_DEPTH when the client doesn't specify one, so an + // unbounded traversal can never happen. + let max_depth = Some( + body.max_depth + .map_or(UInt::from(MAX_MAX_DEPTH), |max_depth| { + max_depth.min(UInt::from(MAX_MAX_DEPTH)) + }), + ); let hierarchy = services .rooms diff --git a/src/service/rooms/summary/mod.rs b/src/service/rooms/summary/mod.rs index 1d1c40679..b3bd9e350 100644 --- a/src/service/rooms/summary/mod.rs +++ b/src/service/rooms/summary/mod.rs @@ -210,6 +210,10 @@ pub async fn get_room_hierarchy_for_user( let mut summaries = vec![root_summary.summary]; let mut inaccessible_children: HashSet<_> = root_summary.inaccessible_children.into_iter().collect(); + // Rooms already included in the response. The spec requires each room to + // appear at most once, and this also terminates traversal of cyclic space + // graphs (e.g. a space containing itself). + let mut visited: HashSet = HashSet::from([room_id]); // TODO refactor this with Vec::peek_mut once it's stabilized while let Some(layer) = queue.last_mut() { @@ -224,6 +228,11 @@ pub async fn get_room_hierarchy_for_user( continue; } + // Skip rooms already present in the response + if !visited.insert(room_id.clone()) { + continue; + } + let summary = match self .get_room_summary_and_children_for_user( Some(querying_user), @@ -269,7 +278,7 @@ pub async fn get_room_hierarchy_for_user( // suggest you reconsider some of the choices you made which led you to this // point. if queue.len() > 50 { - return Err!("Space hierarchy is unreasonably large"); + return Err!(Request(TooLarge("Space hierarchy is unreasonably large"))); } // Add accessible children as a new layer