Repository navigation
Security: bound request parameter parsing - #1419
Open
crimson-knight wants to merge 20 commits into
Open
crimson-knight wants to merge 20 commits into
crimson-knight wants to merge 20 commits into
Conversation
Crystal 1.22 validates HTTP messages at construction time:
HTTP::Request.new rejects an empty resource ("Invalid HTTP resource"),
and the multipart parser rejects a body line that appears where a
header is expected ("Invalid header name").
- Build spec requests with the resource "/" instead of "". Request#path
already reported "/" for an empty resource, so behavior is unchanged.
- Add the blank line that separates part headers from the body in the
file upload spec's multipart fixture.
Amber itself never constructs a request with an empty resource:
Amber::Testing::ContextBuilder defaults its path to "/".
Indexed parameter keys are converted with a checked conversion and must fall within params.max_array_index and params.max_array_index_gap, with gap padding capped per request by params.max_array_padding. Parameter count, key nesting depth, key length, field value length, and URL-encoded body size get explicit defaults. All limits live in the new typed `params:` configuration section (AMBER_PARAMS_* overrides) and apply to the router's form and multipart parsers and to the schema query, JSON bracket, and multipart parsers. A request beyond a limit raises Amber::Exceptions::ParameterLimitExceeded, answered with 400 by the pipeline and the Error pipe, and reported as the parameter_limit_exceeded code by schema contracts. The WebSocket binary decoder now rejects length prefixes and field counts larger than the message.
Addresses the review of the first parameter-limit commit:
- URL-encoded bodies and query strings are read pair by pair, so count,
key, value and body-size limits reject before the body is held; both
FormData.parse_part overloads and QueryParser.parse_query_string use
the same checked reader, and router query access (Params#query,
fetch_all) is checked once per request.
- Multipart bodies are capped by max_multipart_body_bytes and each file
by max_file_bytes while streaming; the schema multipart paths read
uploads through the same bounded policy, and one ParamBudget covers
fields and files together.
- The WebSocket binary decoder caps the frame before Base64 decoding
(max_frame_bytes, default 1 MiB) and rejects trailing bytes.
- JSONParser.parse_multipart counts and size-checks array elements.
- Gap and padding checks use subtraction, the count check compares
before incrementing, and ParamsConfig#validate! enforces a range for
every limit.
- The loader validates the effective params after YAML and AMBER_PARAMS_*
overrides; the V1 settings path now carries `params:` and the same
overrides with validation.
- A null-padded array slot is replaced with a container before
descending, so items[0][name]=alice yields [{"name" => "alice"}].
Resolves the second review of the parameter-limit change: - HTTP::Request#param_budget is shared by the query string, the router's form and multipart parses, and schema conversion; cached router fields are not charged twice. HTTP::Request#query_params checks the raw query before parsing, on every access until it succeeds, so schema, WebSocket and error-page readers are bounded too. - URL-encoded reads never pass the remaining body allowance, and raw keys and values are size-checked before decoding. Key depth is counted without splitting, before any value is parsed. - Multipart part headers are bounded by max_part_header_bytes while read (BoundedIO trims peeks to its allowance), the epilogue is drained within the body cap, and upload tempfiles are removed when a later part is rejected and when the request finishes. Tempfiles use a fixed name. - max_file_bytes is limited to 1 GiB, which the schema's in-memory read can hold. The WebSocket binary decoder also checks the decoded frame size.
Resolves the third review: - WebSocket routes read through Amber::WebSockets::BoundedProtocol, which caps each message across frames and fragments as they arrive (params.max_websocket_message_bytes, 2 MiB) and closes with 1009. - Every multipart tempfile is tracked; a duplicate part name deletes the replaced tempfile at once, and a rejection deletes them all. - Router::File copies through the handle File.tempfile created, closes it on success and failure, reports the bytes copied as `size`, and verifies the tempfile's identity before the schema reads it. - FormScanner reads URL-encoded input byte by byte with & and ; as separators and enforces raw key, value and body sizes as bytes arrive. - Malformed multipart framing or metadata raises Amber::Exceptions::BadRequest (400); ParameterLimitExceeded is now a BadRequest. - The negative-index check applies only to bracketed segments. - The query budget reads the raw request target before URI parsing, and method-override detection no longer copies the query.
Resolves the fourth review: - ParamBudget#read_bounded reads through a fixed scratch buffer and keeps only permitted bytes, so an over-limit value never grows the result past its limit. - JSON, XML and fallback document bodies (schema parser, JSONParser, XMLParser, router JSON params) are bounded by params.max_document_body_bytes (8 MiB). - The WebSocket handler checks the handshake query before switching protocols. - BinaryDecoder defaults to the ingress limit; FrameTooLargeError closes the socket with 1009. - JSONParser array-form keys (`a[b][]`) get the same depth and index checks as nested keys. - Upload tempfiles live in a private 0700 directory, and cleanup checks the tempfile's identity before deleting.
Fixes the fifth review's findings: - HTTP::Request#form_params and #form_params? reuse Amber's bounded, cached form parse for every request method, so a middleware or action read of a URL-encoded body is bounded and shares one result. - XML extraction reads each node's text once; every extracted key and value is checked (length, depth) and charged to a total of four times max_document_body_bytes, including XPath extraction. - ParamBudget#admit! checks key shape and bracket indices, so query, form and multipart pairs are rejected before any params reader sees them. - Uploads stay readable through the action: File#file opens the tempfile on first use, and Amber closes and deletes it after the response. File#move_to and File#retain! transfer ownership for later or asynchronous use; cleanup skips an upload it no longer owns. - Context#websocket? matches the WebSocket handler (case-insensitive Upgrade, Upgrade in Connection). A socket-only route no longer raises NilAssertionError: upgrades reach the handler, plain requests get 404. - Uploads use one stable per-user directory, <tmpdir>/amber-uploads-<uid>, mode 0700 and owner-checked, reused by every worker and restart.
Fixes the sixth review's in-scope findings: - File#move_to across file systems copies to a new file beside the destination, renames it into place only after the copy succeeds, and deletes the tempfile only after the rename. A failed copy removes the partial file and leaves the destination and the tempfile intact. - XML element#text again holds all descendant text, as in 76954f0. ExtractionAllowance measures each node's text once (with the whitespace strip would remove), checks max_value_bytes before building a value, charges every copy to the total cap, and copies each value once. XPath values get the same checks, and the element#text key is checked against the key limits. - URL-encoded and multipart bodies are recognized by their media type, compared case-insensitively, in Params, method override detection and schema parsing.
Fixes the seventh review's in-scope findings: - Params keeps a body rejection and raises it again on every later read, so a caller that catches it never sees a parse of the remaining bytes. - XML values are written straight into a string of their stripped size, so each value is copied once and the charge matches the copy. - New params.max_document_nodes (default 100,000): a parsed XML document is counted before extraction, which bounds extraction's caches. - If another local user has claimed the shared upload directory, the process uses a new private directory with an unpredictable name. - File.discard deletes a tempfile even when closing it fails, for both the upload copy and the cross-device staging file. - A cross-device move that completed reports success even if removing the original tempfile fails; cleanup removes the leftover later.
… request Fixes the eighth review's in-scope findings: - A well-formed XML body is counted against max_document_nodes while it streams (XML::Reader), before libxml builds a tree; a recovered malformed document is counted by following libxml's links, with no per-node allocation, before extraction. - ExtractionAllowance walks the tree by pointer and measures text and attribute values in libxml's buffers, decoding UTF-8 whitespace without allocating, so an over-limit value is rejected before it is copied. - HTTP::Request#guard_body_read remembers a body rejection on the request; params, form_params, schema document reads (JSON, XML, fallback, CBOR, COSE) and schema multipart reads all raise it again. - Upload cleanup closes and deletes every tempfile even when one handle fails to close or one file fails to delete, logging the failure.
…rse errors Fixes the ninth review's findings: - XML request bodies may not contain a document type declaration: the streaming count and the recovered-tree count both reject one, so request XML never defines or expands entities. The tree count also counts the nodes inside attribute values. - ParserRegistry.parse_request, JSONParser.parse_request and XMLParser.parse_request run the whole parse through guard_body_read, which now also keeps Amber::Schema::RequestParseError, so rejections raised after the body is read (node limit, CBOR/COSE errors) stay. - Pipe::Pipeline and Pipe::Error answer RequestParseError with its own status, so a direct parse from an action that hits a limit gets 400. - Multipart rejection cleanup deletes every tempfile, logging failures and keeping the original error; File#delete_tempfile clears ownership only after a successful delete, so a failed delete can be retried.
…cached reads Fixes the tenth review's findings: - QueryParser.set_nested_value takes a block; the schema multipart and form converters check each key and reserve its array slot before they read an upload or parse a value, so a rejected key never loads the file. - Params and HTTP::Request#form_params? raise a remembered body rejection before serving a cached parse. - guard_body_read keeps the outermost rejection, so a repeated schema parse raises the same RequestParseError. - A tempfile that cannot be deleted while a multipart parse fails is handed to the request, which retries when the request finishes.
… errors Fixes the eleventh review's findings: - A schema multipart part skipped for a conflicting key is drained under its own max_value_bytes or max_file_bytes cap. - ParserRegistry.parse_request computes each request's schema data once (HTTP::Request#schema_request_data) and returns copies, so repeated schema reads charge padding and counts once. - File.discard never raises: a tempfile that cannot be deleted is kept in a bounded list that File.retry_leftover_deletions retries when a request finishes, and the original rejection is preserved. - A declared form charset is decoded to UTF-8 while the body streams; an unknown charset gets 400.
- Bound the raw bytes of a charset-declared form, so a UTF-16 body cannot carry twice max_form_body_bytes. - Check a schema's media type on every parse, before cached request data is returned. - Hand upload tempfiles that cannot be deleted at end of request to the shared retry list, and drop the per-request list. - Read CBOR and COSE bodies under max_document_body_bytes, keeping the limit error intact through the CBOR decoder. - Pass the creation identity to discard so a closed staging file is still tracked; log, rather than evict, past 10,000 leftovers. - Specs for each, plus a nested-copy check for cached schema data.
- Defer deletion of an upload's original tempfile after a move across file systems, so a failed delete is retried by a later request. - Check CBOR and COSE bodies against max_document_body_bytes inside the CBOR reader, before a declared length is allocated, and read long values in chunks; COSE passes the limit error through. - Raise a remembered RequestParseError for malformed JSON in a schema read, so a second parse cannot see an empty body. - Specs for each, including a cross-device move with a locked upload directory and allocation bounds for a declared CBOR length.
- Grow CBOR arrays as elements arrive instead of reserving the declared count. - Check the request's document limit before the CBOR decoder's own caps, including for a trailing byte, so an over-limit body is always reported as parameter_limit_exceeded. - Let the development exception page render a rejected query's error without reading the query again. - Specs for each, with allocation bounds for declared array counts.
- HTML-escape the error message on the production HTML error page, and the method, path and title on the development exception page. - Leave the client's declared charset out of the form decoding error. - Show the request-line method on the development exception page when resolving _method would read a rejected body again, so the 400 stays a 400. - Compare a declared CBOR length with the remaining allowance, without UInt64 overflow, and count framing bytes only once they are read. - Specs for each, including a script tag in the request path and query on both error pages.
- Check bracketed array indices only where a schema parser reserves array slots, so a form that names records by id in brackets (such as users[1000001][name]) reads as ordinary params again. Admission keeps the key, value, count and depth checks. - Choose the error response's media type (JSON, HTML or plain text) from the first Accept entry without its parameters, so an HTML Accept header with a charset gets the escaped HTML page and the client's header is never echoed as the Content-Type. - Specs for both, and the admission specs updated to the slot-site index check.
crimson-knight
marked this pull request as ready for review
October 25, 2026 03:05
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
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.
Summary
Bounds how Amber parses request parameters. A request beyond a configured limit gets
400 Bad Request, and parsing never allocates in proportion to a number written in the request or to an unchecked body size.items[N]): the index uses a checked conversion and must be at mostparams.max_array_index(default 100) and at mostparams.max_array_index_gap(default 32) past the end of its array. Gap padding is capped per request byparams.max_array_padding(default 1,000).params: 8 MiBFormScannerreads query strings and URL-encoded bodies byte by byte, with∧as separators. Raw key, value and body sizes are enforced as the bytes arrive; the query is read from the raw request target before URI parsing. Multipart bodies, part headers and files are counted while they stream, and the epilogue is drained within the body cap. Bounded reads go through a fixed scratch buffer and keep no more than their limit.HTTP::Request#param_budgetis shared by the query string, the router's form and multipart parses, and schema conversion. Cached router fields are charged once.HTTP::Request#query_paramschecks the raw query before Crystal parses it, so every reader is bounded (schema contracts, WebSocket reconnection, the error page, application code). A rejected query raises on every access.HTTP::Request#form_paramsand#form_params?reuse Amber's bounded, cached form parse for any request method, so middleware and action reads are bounded and share one result withparams.paramsreaders see the same limits as schema contracts. Bracketed array indices are checked where a schema parser reserves array slots, so a field named by record id (users[1000001][name]) stays an ordinary parameter.element#textkeeps its base behavior (descendant text). Text and attribute values are measured in libxml's buffers before they are copied, checked againstmax_value_bytesand key limits, and charged to a total of four timesmax_document_body_bytes, XPath fields included. Documents are held tomax_document_nodes(default 100,000): counted while they stream, and recovered malformed trees are counted before extraction.HTTP::Request#guard_body_readremembers a body rejection, including one raised after the body was read (node limit, CBOR/COSE decoding), so laterparams,form_paramsand schema reads raise it again instead of parsing the remaining bytes.RequestParseErrorfrom an action is answered with its own status (400) by the pipeline and the Error pipe.max_document_body_bytes, checked before a declared length is allocated; long values are read as their bytes arrive and arrays grow as elements arrive.charsetis decoded to UTF-8 while the body streams;max_form_body_bytescounts the raw bytes, and an unknown charset gets 400.Acceptentry without its parameters), the production HTML error page HTML-escapes the message, and the development exception page escapes the method, path and title. A development error page for a rejected query or form shows the request-line method and no query parameters, so the 400 stays a 400.<!DOCTYPE ...>gets 400, so request XML never defines or expands entities.params:section (Amber::Configuration::ParamsConfig,AMBER_PARAMS_*overrides). Every limit has a range that keeps it clear of integer overflow.Amber::Exceptions::BadRequest(status 400) covers malformed multipart framing and metadata;ParameterLimitExceededis aBadRequest. Both are answered by the pipeline and the Error pipe. Schema contracts report them asparameter_limit_exceededorinvalid_request_body.<tmpdir>/amber-uploads-<uid>, owner-checked, reused by every worker and restart).sizeis the bytes copied, not the client's declared size. During the actionparams.files[name].fileis a readable handle, opened on first use. Amber closes it and deletes the tempfile when the request finishes;File#move_to(path)andFile#retain!transfer ownership for later or asynchronous use, and cleanup skips an upload it no longer owns. A move across file systems copies to a file beside the destination and renames it into place, so a failed copy leaves both files intact. If another local user has claimed the shared upload directory, the process uses a new private directory with an unpredictable name. A duplicate part name deletes the replaced file at once, and a rejection deletes them all. A tempfile that cannot be deleted (for example in a read-only directory) is logged and retried when a later request finishes; past 10,000 such files each further path is logged as an error.Upgrade,UpgradeinConnection). A socket-only route no longer raisesNilAssertionError: upgrades reach the handler, and a plain request gets 404.Amber::WebSockets::BoundedProtocol, which counts each message across frames and fragments as they arrive and closes with 1009 when it passes the limit. The handshake query is checked before the 101 response.BinaryDecoderdefaultsmax_frame_bytes:to the ingress limit and closes with 1009 when a smaller limit trips. It checks the size before and after Base64 decoding, and rejects length prefixes or field counts larger than the frame, plus any trailing bytes.items[0][name]=alicebuilds[{"name" => "alice"}]inJSONParser.Configuration loading decisions
paramssection after YAML andAMBER_PARAMS_*overrides, so an unusable limit fails at boot. It does not call the fullAppConfig#validate!, because v2-dev's loader validates no section today. Turning that on would also start enforcing server, session, jobs, logging and mailer rules in existing apps, which is a separate change.params:mapping and the sameAMBER_PARAMS_*overrides, and validates them at load.Compatibility
params.max_array_index/params.max_array_index_gapfor those applications.max_file_bytesaccepts up to 1 GiB, because schema contracts hold uploads in memory.amber-uploadname with the client's plain extension. Amber deletes them when the request finishes; callmove_to(path)orretain!during the action to keep one (for example before passing its path to a background job).File#filenow opens the tempfile read-only.Acceptother than JSON or HTML are nowtext/plain, rather than echoing theAcceptvalue as theContent-Type.params.max_document_nodesnodes (default 100,000), and may not contain a DOCTYPE.FormData.parse,Multipart.parseandMultipartParser.parse_multipart_requesttake an optionalParamBudget(defaulting to the request's) where they previously took nothing or aParamsConfig.params.max_document_body_bytesfor larger APIs.params.max_websocket_message_bytesif needed.Tests
GC.stats.total_bytes). Request-path specs go through routing, the CSRF pipe and a schema contract, including ordinary query access and upload cleanup at the end of a request.dfd55de0): 2589 examples, 0 failures. CI is green on ubuntu-latest and macos-latest. Warnings are unchanged: the 25 existingParams#validationdeprecation warnings inspec/amber/validations.crystal-alpha tool format --checkis clean.cdd58b34) on the changed files: no new findings.dfd55de0, scoped to findings this diff introduces) found no introduced findings: "No INTRODUCED findings identified. Mergeable as written." Every pass's findings were fixed or, when they predate this change, recorded as follow-ups.