Add check_media_file_for_spam spam checker hook

[erikjohnston]

This is to allow us to reject known bad data, or e.g. do some anti virus scanning.

Some notes:

• This responds with generic 404 for remote content, but a 400 "Bad Content" for uploads. Is that what we want?
• If we reject remote media we'll keep downloading and then deleting the data for each request.

An example usage:

synapse/tests/rest/media/v1/test_media_storage.py

Lines 433 to 437 in d71524f

	async def check_media_file_for_spam(self, file_wrapper, file_info) -> bool:
	buf = BytesIO()
	await file_wrapper.write_to_io(buf)
	return b"evil" in buf.getvalue()

[erikjohnston]

The rationale here is that we don't want to give the raw path to the spam checker so that people don't just do file.read(), potentially blocking the reactor while we do large reads. Instead we do what FileSender does and read small chunks, yielding to the reactor between reads. I'm not 100% if that's the right way of doing things, but at least it's what we currently do for file downloads.

[richvdh]

it's really not the right way of doing things. Irrespective of whether non-blocking IO is perfect, it's better than doing regular IO on the reactor thread.

This is probably good enough for a quick hack, but it would be nice to come back to it and either make a non-blocking-io thing which integrates with the twisted reactor, or give up and uses threads.

[erikjohnston]

Have filed #9312. Doing it like this is at least not making anything worse

[daenney]

I don't think a 400 is appropriate. "Bad request" generally means there's something malformed about the request the client did, not "the content itself is bad/undesirable".

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400

The HyperText Transfer Protocol (HTTP) 400 Bad Request response status code indicates that the server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing).

The 422, unprocessable entity, is probably the closest thing
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/422

The HyperText Transfer Protocol (HTTP) 422 Unprocessable Entity response status code indicates that the server understands the content type of the request entity, and the syntax of the request entity is correct, but it was unable to process the contained instructions.

[erikjohnston]

I don't think a 400 is appropriate. "Bad request" generally means there's something malformed about the request the client did, not "the content itself is bad/undesirable".

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400

The HyperText Transfer Protocol (HTTP) 400 Bad Request response status code indicates that the server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing).

The 422, unprocessable entity, is probably the closest thing
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/422

The HyperText Transfer Protocol (HTTP) 422 Unprocessable Entity response status code indicates that the server understands the content type of the request entity, and the syntax of the request entity is correct, but it was unable to process the contained instructions.

I think if we are going to make it obvious to the clients they have been blocked by the spam checking module then 400 is probably the more widely expected code, I think 422 is just going to freak people out. Eh, I don't know.

[daenney]

I’m not sure the purpose of status codes is to scare clients/users. Shouldn’t that be up to the user agent, like Ele web? A 422 could more easily be rendered as a “hey this file is really bad” in the UI compared to a 400 that could mean “we broke something at the http request level”

[clokep]

I think this looks good overall, but I'm unsure if @richvdh is happy with the IO conversation.

[clokep]

Should we include some info about what was blocked while logging?

[erikjohnston]

We don't actually know very much about the media annoyingly, e.g. we don't know the media ID. I hope we can extract enough info from the request logs.

[clokep]

Doesn't file_info have some stuff in it?

[erikjohnston]

Yeah, it has some stuff, but not the media ID. We could log if its a remote download or not, but I'm not sure that is particularly useful given you can get that from the request logs?

[clokep]

Oh, I see all that info is pretty much in the requests logs. 👍

[richvdh]

I think this looks good overall, but I'm unsure if @richvdh is happy with the IO conversation.

I've not gone through the code, but from what I saw I'm happy with this under a "no worse than what we have" banner.