fix(dashboard): cap the contract envelope before decoding it - #111
Closed
juicycleff wants to merge 1 commit into
Closed
juicycleff wants to merge 1 commit into
juicycleff wants to merge 1 commit into
Conversation
The contract transport now refuses a request body over 1 MiB with 413 and BAD_REQUEST. You can change the cap with transport.WithMaxBodyBytes, server.WithMaxBodyBytes, or the dashboard's contract_max_body_bytes setting. Zero or less keeps the default, so there's no way to turn it off. Before this, the handler read and decoded the whole envelope before it looked at the intent's Requires predicate, and it had no limit at all. Any principal, read-only ones included, could post an arbitrarily large body to any intent and the server would work through all of it before saying no. We found this while reviewing chronicle's reports.generateCustom, which takes user-supplied text. The Requires check still runs after the decode, because the intent name only exists inside the body. If the declared Content-Length is over the cap, the handler refuses without reading anything. For a chunked body, http.MaxBytesReader stops one byte past the cap.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Conventional Commits ValidationPR Title: valid |
Contributor
Author
|
Landed on main as 4cee987. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The contract transport now caps the request envelope at 1 MiB. Anything larger gets a 413 with
BAD_REQUEST, and the dispatcher never sees it.If you need a different limit:
transport.WithMaxBodyBytes(n)onNewHandlerorNewHandlerWithCSRFserver.WithMaxBodyBytes(n)for remote contributors built withserver.Newcontract_max_body_bytesin the dashboard config, orWithContractMaxBodyBytes(n)Zero or less keeps the default. There's no setting that turns the cap off.
Why
The handler decoded the whole envelope before it evaluated the intent's
Requirespredicate, and it put no limit on the body. Any principal, a read-only one included, could post an arbitrarily large body to any intent and the server would read and decode all of it before refusing. We ran into this reviewing chronicle's dashboard contract, wherereports.generateCustomaccepts user-supplied text.Moving the
Requirescheck ahead of the decode isn't possible, because the intent name only exists inside the body (the URL carries the envelope version and nothing else). So the cap is what bounds the work. If the declaredContent-Lengthis over the limit, the handler refuses without reading anything. For a chunked body,http.MaxBytesReaderstops one byte past the limit.Tests
http_limit_test.gocovers a 4 MiB chunked body (413, dispatcher not called, at most limit + 1 bytes read off the wire), an over-limit declared length (413, zero bytes read), a body exactly at the default limit (200), and a configured 4096-byte limit at 4096 and 4097 bytes. A server test checks thatserver.WithMaxBodyBytesreaches the dispatch handler.We ran the new tests first against the option with enforcement left out. Every over-limit case came back 200 and was dispatched. With the fix,
go test ./extensions/dashboard/...passes.