Change display names/avatar URLs to None if they contain null bytes before storing in DB

[H-Shay]

Hopefully fixes #10886, the change seems quite simple but I could be missing something here!

[DMRobertson]

Seems reasonable. Have we had a chance to test this? Is there some kind of test we could write to cover this?

[clokep]

I wonder if we should update the name of this now that it does more than ensuring it is a string? 🤷 It's a pretty simple function so not a big deal.

[H-Shay]

I updated the name, let me know if you think it's a little clearer.

[H-Shay]

Seems reasonable. Have we had a chance to test this? Is there some kind of test we could write to cover this?

I did test locally that the function that was changed, str_or_none, does correctly bounce strings with null bytes in them, bounces other non-string objects, and lets through acceptable strings. I can look deeper into adding a test to test that function in conjunction with the database insertion function, although it might be a little tricky to test that. I'll give it a whirl.

[H-Shay]

Seems reasonable. Have we had a chance to test this? Is there some kind of test we could write to cover this?

@DMRobertson: I added a test that I don't think we should permanently add, it's more of a proof-of-concept that the change works. The test is pretty ugly-I couldn't figure out how to get access to the function I was testing so I copied it over. Nevertheless, the test as it is breaks (on postgres only) in the same way as the error message of the issue that is being addressed without the code change, and passes when the change is added. Let me know if you think this is sufficient!

[DMRobertson]

@DMRobertson: I added a test that I don't think we should permanently add, it's more of a proof-of-concept that the change works. The test is pretty ugly-I couldn't figure out how to get access to the function I was testing so I copied it over.

I had in mind a test that would try to reproduce the issue from the "outside". My (rough!) understanding of the issue is that

• We try to join a room where someone's name or avatar has a NULL codepoint
• That name/avatar was set as part of an event at some point in the room's history
• Synapse parses the JSON description of that event into a object e in memory
• Synapse queues e for persistence to the DB
• When e comes to be persisted, psycopg2/postgres complains about the NULL codepoint.

The last two bullets are what we're keen to test here. From a bit of reading, it looks like we can call EventsPersistenceStorage.persist_event with the event and context you have in your test already. We should then be able to remove your copy of _store_room_members_txn from the test body.

I think the test wants to do something like

self.get_success(
    self.hs.get_storage().persistence.persist_event(...)
)

If I understand right, that should fail before your change and succeed after your change. The test should also confirm that the user's event and avatar are correctly stored to the DB, using e.g. RoomMemberWorkerStore.get_users_in_room_with_profiles.

---

Does that make sense?

Being honest, I think the above is probably overkill for a small change like this. (I just like to have a test that proves we've made the right change.) If the above is tricky and just eating up time, we can drop it.

[reivilibre]

An alternative approach with the testing would be to do it quite a long way from the outside using the test helpers that do the REST requests — I know it's not very unit test-like, but it's useful information to know that the overall request doesn't fail if a user joins a room, sets their name & avatar URL to have null bytes in, and so on. (Thinking about it, that could be a useful addition to Complement since other HS implementations might also get bitten by null bytes.)

[reivilibre]

Suggested change

	"store room members", _store_room_members_txn, [event], False
	"store room members", self.hs.get_storage().persistence.persist_events_store._store_room_members_txn, [event], False

That one was hard to dig up, but doing this lets you avoid copying the function (so there won't be any chance we forget to copy it into the test if we change/break it in the future)

[reivilibre]

I don't know if you really need to test this function explicitly/individually if you properly demonstrate that self.helper.change_membership( (as discussed in another thread) works (assuming it didn't work before!).

[H-Shay]

Thanks for showing how to access _store_room_members_txn-even though it seems like it's not necessary to call now-I really scratched my head over how to get access to it so I appreciate being shown the answer!

[reivilibre]

Haha, no worries, it took me a little bit of digging too :). Everytime I look in the data storage stuff I find out about a new angle I hadn't seen before.
I think the reason is that the event persistence store is separate from the rest, since it was hoped at once point that it could be split out into a separate database?

[H-Shay]

Oh interesting-thanks for pointing that out.

[reivilibre]

I usually prefer to avoid injecting events if you can avoid it (reserve that for when you need invalid events to be persisted for some reason, or to pretend you received one over federation).

Since you have alice's access token and everything, you can use something like

        new_profile = {"displayname": "ali\u0000ce"}
        self.helper.change_membership(
            room, self.u_alice, self.u_alice, "join", extra_data=new_profile, tok=self.t_alice
        )

[H-Shay]

The call to change_membership does indeed break without the change I made to _store_room_members_txn and passes with the change, so hopefully this is sufficient? Thanks for pointing out the use cases for injecting events, it's helpful to be told these sorts of things :)

[reivilibre]

Maybe it would be worth also querying the state of the database and checking that they contain Nones instead of something else.
(It just feels weird to have an assert-less test in some strange way, but it's also worth noting that the behaviour is replacing the name with None rather than e.g. leaving it as it was before, if that makes sense?)

[H-Shay]

Totally makes sense, I shall give that a go.

[reivilibre]

The test looks good other than relying on accidental ordering, though :)

Thanks!

[reivilibre]

Bah, slight problem here is that this is probably fine for tests, but it's relying on the database returning the rows in insertion order.

SQLite secretly maintains a 'rowid' (row ID) per row and stores rows in a B* tree keyed by those, so I can see it returning them in insertion order.
I assume Postgres does something similar — basically, returning them in order of insertion is 'easiest' for this query (it'd be more work to jumble them up) so I reckon this test will work!
(If the database was using an index to retrieve the rows, then it would probably return them in order of that index)

However, nothing guarantees that these databases will do that. I think a notable time that databases will violate this insertion order is if some rows get deleted and the database shuffles things around (e.g. Postgres' VACUUM; SQLite can recycles rowids afaik and SQLite will generate rowids somewhat randomly if you reach the limit).

I was curious about SQLite's VACUUM and in my case it kept the order when vacuuming, seemingly just rewriting rowids, oh well (I was hoping for some rows to be dramatically reordered! :P).

sqlite> create table tab1 (val TEXT);
sqlite> insert into tab1 values ("a"), ("b"), ("c");
sqlite> select rowid, val from tab1;
1|a
2|b
3|c
sqlite> delete from tab1 where val = "b";
sqlite> insert into tab1 values ("d"), ("e"), ("f");
sqlite> select rowid, val from tab1;
1|a
3|c
4|d
5|e
6|f
sqlite> vacuum;
sqlite> select rowid, val from tab1;
1|a
2|c
3|d
4|e
5|f

Anyway, excuse the tangent, but this is the kind of thing that has the tendency to bite you in the bum :(.
I think what I'd do here is:

• select from room_memberships before changing Alice's name
  • check your preconditions and make a note of the event_id
• update the name
• select from room_memberships again, filtering out the row with the event_id you had before and check your postcondition

Check the expected number of rows at each step to make sure that if we get more rows for some reason, they don't cause confusion in the future when the test breaks and some poor soul has to figure out why :p.

I'd probably use a list comprehension to do the filtering in Python (mostly since doing it in SQL here is a pain and it's not as though it's performance sensitive since it's a test), something like that:

rows = [
	row for row in rows if row["event_id"] != first_event_id
]

[reivilibre]

Thank you!