Synapse happily accepts m.push_rules account data

[auscompgeek]

Description

Synapse will store changes to the m.push_rules account data to the account_data table. This then becomes out of sync with your real push rules stored in the push_rules table.

Steps to reproduce

Warning: Recovering from these steps requires database surgery. Do not attempt on a homeserver for which you do not have admin access to the database.

• Open riot-web devtools
• Explore account data
• Open m.push_rules
• Edit and send

Your edited "push rules" gets stored in the account_data table as m.push_rules, which then gets sent out to all of your clients.

Version information

• Version: 1.12.3
• Install method: Debian unstable matrix-synapse package
• Platform: Debian unstable on OVH

[auscompgeek]

If you believe your users have been affected by this, you can identify those users with the following SQL query:

SELECT user_id FROM account_data WHERE account_data_type = 'm.push_rules';

Recovering from this is possible by deleting those rows then editing the real push rules from Riot's notification settings (to trigger a resend of the push rules to every client). Automating the fix for all users is left as an exercise to the reader...

[babolivier]

Heya and thanks for the bug report.

Just FYI, the custom m.push_rules account data isn't actually pushed to clients, as Synapse overwrites whatever's in the m.push_rules account data with the content from the push_rules database here:

synapse/synapse/handlers/sync.py

Lines 1240 to 1264 in 118b58f

	if since_token and not sync_result_builder.full_state:
	(
	account_data,
	account_data_by_room,
	) = await self.store.get_updated_account_data_for_user(
	user_id, since_token.account_data_key
	)
	push_rules_changed = await self.store.have_push_rules_changed_for_user(
	user_id, int(since_token.push_rules_key)
	)
	if push_rules_changed:
	account_data["m.push_rules"] = await self.push_rules_for_user(
	sync_config.user
	)
	else:
	(
	account_data,
	account_data_by_room,
	) = await self.store.get_account_data_for_user(sync_config.user.to_string())
	account_data["m.push_rules"] = await self.push_rules_for_user(
	sync_config.user
	)

(I tested that on a personal account just now and confirmed that, even after clearing the cache in Riot, it was only seeing the unmodified push rules)

The fact that Synapse happily stores that account data without updating the push_rules table (or that it accepts to store it at all), though, definitely sounds like a bug.

[auscompgeek]

Hmm, I guess clear cache in Riot is another way of recovering from that too then. But in my testing, if I change the account data on one client (in my case riot-web), it will get pushed down /sync for my other clients (notably RiotX).

From that snippet, it makes sense since RiotX would've received it in a non-initial sync and the real push rules never changed in the meantime, so it would've indeed received the new account data.

[babolivier]

Oh yes you're right, it's indeed an issue for incremental syncs. My bad.

[erikjohnston]

Fixed by #15555

[clokep]

Fixed by #15555

Note that this is disabled by default (pending the MSC being approved).

[erikjohnston]

Fixed by #15555

Note that this is disabled by default (pending the MSC being approved).

oh boo :(

[clokep]

MSC4010 is ready from my POV! I think it is fairly straightforward.