Add very basic domain validation for DomainSpecificString.

[clokep]

There seems to be a client attempting to join multiple rooms at once by sending a comma separated list of room aliases to the join room API. roomIdOrAlias ends up being something like #alias:test,#another:server, which is invalid, but gets parsed as trying to join #test on a server named test,#another:server.

Something in the stack causes the error to only see the first bit before the #:

RequestSendFailedsynapse: Failed to send request: ValueError: invalid hostname: test,

Anyway, these are obviously invalid domain names when we attempt to create a RoomAlias object and we should reject them. We could do a lot more validation here, but it is surprisingly hard to define what a "valid" domain name is (in terms of things you see on the Internet). The fix given here is very specific, but should solve the current error.

Fixes https://sentry.matrix.org/sentry/synapse-matrixorg/issues/119622/

[clokep]

There's a bunch of rules about valid domain names that we could enforce (< 253 characters, made up of labels of 63 characters, etc) but I'm no confident in those not being broke by same real domain names out there. Commas shouldn't be allowed, as well as other general punctuation (:!@#$%^&*(), etc.)... I can expand this if we'd like. 😄

[richvdh]

: absolutely can go in the domain part of a domain specific string, since they can include ipv6 literals, with all the fun that entails.

There already exists a function parse_and_validate_server_name which does what I think you want here.

[richvdh]

I'm trying to remember the reason we didn't already do more validation here. I think the reason is some combination of:

• efficiency: we don't necessarily want to validate an identifier every time we pull it out of the database or otherwise wrap it in a DomainSpecificString. However I'm prepared to believe that increased robustness from better checking makes that a price worth paying.
• there may be cases where there are existing events which contain malformed identifiers, but which we have already been accepted into the event DAG of a room. Changing the rules now could have unforseen consequences.

If you want to give it a go, I won't object. Just trying to give some background.

[clokep]

Right, IP literals complicate things a bit. 😢

Pulling things out of the database might not need validation, but we should be validating new data coming in. I might be able to do something a bit more specific for that endpoint to fix the Sentry issue, but that feels more like a hack. I could believe there's some broken aliases and such already in events, unfortunately. I'll think a bit about.

Thanks for the pointer to parse_and_validate_server_name!

[clokep]

I made some changes based on this conversation -- the additional validation is only done on is_valid and not with from_string. I did a bit of auditing and it looks like is_valid is never called with data pulled from the database so I hope this is safer!

[richvdh]

looks great, thanks.