Skip to content

A join is answered from a copy of a room this server has left, and the reason it can succeed is unexplained #342

Description

@hanthor

Seen as an intermittent Complement failure on #341 (a pure file move that cannot touch join rules), after the same protected subset passed on #338, #339 and #340 minutes earlier:

--- FAIL: TestRestrictedRoomsRemoteJoinInMSC3787Room/Join_should_fail_with_mangled_join_rules
    restricted_rooms_test.go:164: MatchFailure got status 200 instead of a failure code

The re-run passed, and the same subtest passes on the plain-restricted variants in the same run, so this is a real intermittency, not a change in #341.

What is established

join in routes.rs tries set_membership_with(..., "join") locally first and only walks make_join/send_join with a resident when the room is UnknownRoom. Nothing on the local path checks that this server still has a member in the room. In the test, bob (hs2) ends up with hs2 holding a copy of the room and no member of its own in it, while alice mangles the join rules on hs1 — which hs1 has no reason to deliver to hs2, because destinations_in fans out only to servers with a joined or invited member. So hs2 answers bob's join from a copy whose join rules are known to be out of date. That much is certainly true and is worth fixing on its own: a server with no joined member in a room is not a resident of it, and a join should go through one.

What was wrong in the first version of this issue

This issue originally claimed the stale copy is why the 200 happens: that hs2's old allow still names the space bob is in, so the restricted check passes locally. That mechanism is disproven. Reproduced across two real servers (the_whole_restricted_room_sequence_holds_across_two_servers, with Complement's exact step order), the local path refuses:

mangled join: 403 {"errcode":"M_FORBIDDEN","error":"cannot join restricted room without
  `join_authorised_via_users_server` field if not invited"}
local copy of join rules: 200 {"allow":[{"room_id":"!…","type":"m.room_membership"}],
  "join_rule":"restricted"}

The stale rules do make restricted_join_nominee reach the vouching check, and bob is in the allowed room on that view — but the nominee has to be a local member who is joined to the restricted room, and hs2 has none. With no join_authorised_via_users_server, ruma's rule for restricted/knock_restricted rejects a join whose current membership is leave (ruma-state-res room_member.rs, the "we cannot authorize" branch). So the stale copy alone does not produce a 200.

The other reading I drew from the CI log — that hs1 was never asked, because its container log shows no check_state_dependent_auth_rules line for a join — does not hold either: that log is truncated (it also shows no auth lines for the joins in the earlier subtests, which certainly happened). Which server answered the failing join is therefore unknown.

What is still open

Why a 200 is possible at all. For the local path to return one, bob's membership in hs2's copy would have to read join or invite at that moment, or a nominee would have to be produced; for the resident path, hs1 would have to hand out a template and accept the join against the mangled rules. Neither has been reproduced. Candidates not yet ruled out:

  • a membership regression on hs2 — an older member event arriving late and being appended after the leave (the ingest dedup only catches an event already in the log by ID);
  • the state lookup in Rooms::authorize, whose closure turns both "not in state" and "stored body missing or unparseable" into None; for join rules ruma then errors out, so it fails closed there, but the same closure feeds the member and power-level lookups, where absence is not always the safe answer.

Next step

Reproduce before fixing: run TestRestrictedRoomsRemoteJoinInMSC3787Room locally in a loop with both servers' logs kept, and confirm which server answers the 200 and with what state. The resident check above is worth landing either way, but it should not be landed as the fix for this failure until the failure is understood.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions