Add HTTP 413 response when incoming request is too large #19212
Conversation
| ) | ||
| self.responseHeaders.setRawHeaders(b"content-length", [b"0"]) | ||
|
|
||
| logger.warning( |
There was a problem hiding this comment.
Should this be an info level log?
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
MadLittleMods
added
A-Federation
A-to-device-messages
tests/server.py
Outdated
| # `max_request_body_size` copied from `synapse/app/_base.py -> max_request_body_size()` | ||
| req = request( | ||
| channel, | ||
| site, | ||
| our_server_name="test_server", | ||
| max_request_body_size=200 * MAX_PDU_SIZE, |
There was a problem hiding this comment.
We should just have a constant that is shared
There was a problem hiding this comment.
I added a new constant and moved the reasoning comment along with it.
synapse/http/site.py
Outdated
| # FIXME: This can be removed once we Twisted releases a fix and we update to a | ||
| # version that is patched |
There was a problem hiding this comment.
Where is the PR that added this feature? I see the commit: 4b7154c
I'm looking for the context as is there a Twisted issue to track?
There was a problem hiding this comment.
Context is in https://github.com/matrix-org/internal-config/issues/1572
Security Advisory: GHSA-rfq8-j7rh-8hf2
Which links to twisted/twisted#4688 (comment) but not exactly the best thing to link to. Perhaps, we can just link to the security advisory ⏩
synapse/http/site.py
Outdated
| command.decode("ascii", errors="replace"), | ||
| self.get_redacted_uri(), | ||
| ) | ||
| self.write(b"") |
There was a problem hiding this comment.
Can we write a better standard error response with errcode and error (human readable description of the problem)?
There was a problem hiding this comment.
I added a better error response. Should be good now.
| # old versions of twisted don't do form-parsing without a valid | ||
| # content-length header. | ||
| ("Content-Length", str(len(content))), |
There was a problem hiding this comment.
make_request already adds a Content-Length header for this same reason. These all result in duplicate headers.
Related to #17035, when Synapse receives a request that is larger than the maximum size allowed, it aborts the connection without ever sending back a HTTP response.
I dug into our usage of twisted and how best to try and report such an error and this is what I came up with.
It would be ideal to be able to report the status from within
handleContentChunkbut that is called too early on in the twisted http handling code, before things have been setup enough to be able to properly write a response.I tested this change out locally (both with C-S and S-S apis) and they do receive a 413 response now in addition to the connection being closed.
Hopefully this will aid in being able to quickly detect when #17035 is occurring as the current situation makes it very hard to narrow things down to that specific issue without making a lot of assumptions.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.