Respond with useful error codes when `Content-Length` header/s are invalid by devonh · Pull Request #19212 · element-hq/synapse

devonh · 2025-11-21T17:14:28Z

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 handleContentChunk but 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.

This PR also responds with more meaningful error codes now in the case of:

multiple Content-Length headers
invalid Content-Length header value
request content size being larger than the Content-Length value

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct (run the linters)

synapse/http/site.py

tests/server.py

synapse/http/site.py

tests/server.py

tests/http/test_site.py

synapse/http/site.py

tests/rest/client/test_login.py

synapse/http/site.py

tests/server.py

MadLittleMods · 2025-12-01T18:58:11Z

tests/http/test_site.py

        # we should get a 415
        self.assertRegex(transport.value().decode(), r"^HTTP/1\.1 415 ")
+
+    def test_content_length_too_large(self) -> None:


(test hasn't been added yet)

synapse/http/site.py

MadLittleMods · 2025-12-01T19:15:15Z

synapse/http/site.py

+                    command.decode("ascii", errors="replace"),
+                    self.get_redacted_uri(),
+                )
+                self.loseConnection()


I think it would be nice to respond with an actual HTTP status code and a good error explaining why.

In the case of multiple Content-Length headers -> 400 Bad Request

In the case of no Content-Length header -> 411 Length Required

Yeah, in the case of multiple headers we can do that.
In the case of no header, we can't jump to that conclusion so easily. It depends on the HTTP version. Required in HTTP 1, not required if there is a Transfer-Encoding: chunked header in the HTTP 1.1, and not required in HTTP 2.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Length

We could bake in all these additional checks if you think it doesn't blow this PR up. I'm not sure if twisted already handles these cases or not.

I have added functionality for ensuring the Content-Length header is valid when present.

I don't think it is worth adding in further validation to enforce HTTP spec compliance. Doing so adds complexity without addressing any specific issues, and becomes a maintenance burden trying to account for all the HTTP variations and future spec changes.
Also, not all HTTP clients strictly follow the HTTP spec/s so adding in strict spec compliance checks could break existing deployments for no real reason.

synapse/http/site.py

MadLittleMods · 2025-12-01T19:31:31Z

tests/http/test_site.py

        # we should get a 415
        self.assertRegex(transport.value().decode(), r"^HTTP/1\.1 415 ")
+
+    def test_content_length_too_large(self) -> None:


I did a bunch of testing and Synapse (Twisted) handles this by truncating the HTTP request at the specified size of the Content-Length. So the request will go through to the server correctly but the body will be silently truncated. The rest of the bytes are dropped.

Perhaps some request with a basic JSON body that will be cut-off and we expect M_NOT_JSON 🤷

Co-authored-by: Eric Eastwood <erice@element.io>

synapse/http/site.py

MadLittleMods · 2025-12-05T19:41:45Z

synapse/http/site.py

+                self.get_redacted_uri(),
+            )
+            error_response_json = {
+                "errcode": Codes.UNKNOWN,


Optional: We could have our own errcode for the known scenarios.

IO.ELEMENT.INVALID_CONTENT_LENGTH, etc

I think using M_UNKNOWN here is more in the spirit of the spec https://spec.matrix.org/v1.16/client-server-api/#standard-error-response
We are trying to provide an HTTP error here since this is an HTTP issue, not a matrix endpoint issue. Assigning it a custom matrix errcode carries with it the implication that this is a matrix error.

From the spec:
When encountering the error code M_UNKNOWN, clients should prefer the HTTP status code as a more reliable reference for what the issue was.
if the client were to receive an error code of M_UNKNOWN with a 400 Bad Request, the client should assume that the request being made was invalid

I think that works. But I also think we could do better ⏩

By the same logic, M_TOO_LARGE would be inappropriate here then. Bottom-line is that we just need better error codes in the spec and that could start with our own namespaced versions which can be parsed by downstream code.

M_TOO_LARGE happens due to an internal Synapse limit based on the size of transactions from the matrix spec (or config setting). So it's easy to argue that it's a matrix based error here, not a generic HTTP error.

Related discussion: #19539 (comment)

synapse/http/site.py

MadLittleMods · 2025-12-05T19:56:34Z

synapse/http/site.py

+            return
+
+        if content_length is not None and self.content is not None:
+            if content_length < self.content.tell():


What about the opposite case: content_length > self.content.tell()?

It wasn't clear to me from the spec that this is always an error.
But we can treat it that way and be spec compliant. So I have added it for completeness.

I also refactored things now that we have so many comparisons to reduce duplication.

synapse/http/site.py

tests/test_server.py

MadLittleMods · 2025-12-05T20:04:06Z

tests/test_server.py

        self.assertEqual(channel.code, 200)
        self.assertNotIn("body", channel.result)

+    def test_content_too_large(self) -> None:


Test for the opposite case?

synapse/http/site.py

Co-authored-by: Eric Eastwood <erice@element.io>

synapse/http/site.py

tests/test_server.py

Co-authored-by: Eric Eastwood <erice@element.io>

synapse/http/site.py

Add HTTP 413 response when incoming request is too large

40d60c8

devonh requested a review from a team as a code owner November 21, 2025 17:14

devonh and others added 2 commits November 21, 2025 10:15

Add changelog entry

1e8eaa1

Merge branch 'develop' into devon/too-large-http-response

27b5918

devonh commented Nov 21, 2025

View reviewed changes

synapse/http/site.py Show resolved Hide resolved

devonh commented Nov 21, 2025

View reviewed changes

tests/server.py Show resolved Hide resolved

devonh commented Nov 21, 2025

View reviewed changes

synapse/http/site.py Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Nov 21, 2025

View reviewed changes

synapse/http/site.py Fixed Show fixed Hide fixed

devonh added 2 commits November 21, 2025 12:11

Log redacted uri

c3be074

Log redacted uri in content-type check

26a2c8f

devonh commented Nov 21, 2025

View reviewed changes

synapse/http/site.py Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Nov 21, 2025

View reviewed changes

synapse/http/site.py Fixed Show fixed Hide fixed

synapse/http/site.py Fixed Show fixed Hide fixed

MadLittleMods added A-Federation A-to-device-messages EDU messages sent exactly once to a specific set of devices. Related to E2EE labels Nov 24, 2025

MadLittleMods reviewed Nov 25, 2025

View reviewed changes

MadLittleMods added the A-Validation label Nov 25, 2025

devonh added 2 commits November 26, 2025 13:56

Address review comments

5cf7b96

Upade comment wording

85d84f5

github-advanced-security bot found potential problems Nov 26, 2025

View reviewed changes

synapse/http/site.py Fixed Show fixed Hide fixed

synapse/http/site.py Dismissed Show dismissed Hide dismissed

devonh added 4 commits November 26, 2025 14:09

Rejig max_request_size usage to use common constants everywhere

9c4536b

Add ref to upstream twisted changes needed

5f4a097

Don't add duplicate Content-Length header in tests

21b114c

Remove unused variable

426b676

devonh commented Nov 27, 2025

View reviewed changes

tests/rest/client/test_login.py Show resolved Hide resolved

devonh requested a review from MadLittleMods November 27, 2025 00:06

MadLittleMods reviewed Dec 1, 2025

View reviewed changes

devonh and others added 4 commits December 2, 2025 00:11

Update synapse/http/site.py

296ed42

Co-authored-by: Eric Eastwood <erice@element.io>

Log rejection at info level

7e86d3d

Temp changes for incorrect Content-Length

e8d8a3b

Merge branch 'develop' into devon/too-large-http-response

36973e0

devonh added 5 commits December 4, 2025 08:55

Fix linter errors

7648b3b

Add comment about why we reject multiple content-length headers

72c3ab3

Fix mypy errors

c2b8d2c

Fix test content-length header addition logic

5ab4736

Refactor error response writing to be reusable

7b0b9b2

github-advanced-security bot found potential problems Dec 4, 2025

View reviewed changes

synapse/http/site.py Fixed Show fixed Hide fixed

synapse/http/site.py Fixed Show fixed Hide fixed

devonh added 2 commits December 4, 2025 12:43

Add return type

b2c219e

Move log

2a2cd4b

github-advanced-security bot found potential problems Dec 4, 2025

View reviewed changes

synapse/http/site.py Fixed Show fixed Hide fixed

Update log

485283c

devonh changed the title ~~Add HTTP 413 response when incoming request is too large~~ Respond with useful error codes when Content-Length header/s are invalid Dec 4, 2025

devonh added 2 commits December 4, 2025 16:40

Add changelog entry

b095437

Refactor extracted function to be class member

b5b4643

devonh requested a review from MadLittleMods December 5, 2025 00:31

MadLittleMods reviewed Dec 5, 2025

View reviewed changes

devonh and others added 3 commits December 5, 2025 20:53

Update synapse/http/site.py

e7fe25f

Co-authored-by: Eric Eastwood <erice@element.io>

Update tests/test_server.py

bea38d4

Co-authored-by: Eric Eastwood <erice@element.io>

Better error messages

e07fd8b

github-advanced-security bot found potential problems Dec 5, 2025

View reviewed changes

synapse/http/site.py Fixed Show fixed Hide fixed

synapse/http/site.py Fixed Show fixed Hide fixed

synapse/http/site.py Fixed Show fixed Hide fixed

devonh and others added 2 commits December 5, 2025 15:28

Add validation for content_length too large

93f58e6

Merge branch 'develop' into devon/too-large-http-response

070356d

github-advanced-security bot found potential problems Dec 5, 2025

View reviewed changes

synapse/http/site.py Fixed Show fixed Hide fixed

synapse/http/site.py Fixed Show fixed Hide fixed

devonh requested a review from MadLittleMods December 5, 2025 23:24

MadLittleMods approved these changes Dec 8, 2025

View reviewed changes

synapse/http/site.py Outdated Show resolved Hide resolved

tests/test_server.py Outdated Show resolved Hide resolved

devonh and others added 2 commits December 8, 2025 20:44

Update tests/test_server.py

525bdbd

Co-authored-by: Eric Eastwood <erice@element.io>

Use SynapseError type

adb4359

github-advanced-security bot found potential problems Dec 8, 2025

View reviewed changes

synapse/http/site.py Dismissed Show dismissed Hide dismissed

synapse/http/site.py Dismissed Show dismissed Hide dismissed

devonh merged commit 8b0083c into develop Dec 8, 2025
45 checks passed

devonh deleted the devon/too-large-http-response branch December 8, 2025 21:39

MadLittleMods mentioned this pull request Mar 10, 2026

MSC4140: update error responses #19539

Open

3 tasks

Conversation

devonh commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MadLittleMods Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MadLittleMods Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devonh commented Nov 21, 2025 •

edited

Loading

MadLittleMods Dec 1, 2025 •

edited

Loading

MadLittleMods Dec 5, 2025 •

edited

Loading

MadLittleMods Dec 5, 2025 •

edited

Loading