-
Notifications
You must be signed in to change notification settings - Fork 90
Support chunked encoding in requests #377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| // On the final zero-length chunk, _chunkSize - _chunkOffset | ||
| // will be zero, so we will call handleBody with a zero size, | ||
| // marking the end of the data stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MitchBradley : is it a valid use case to have a client send a chunkd request with no data (curlLen being 0 at the first request) ? And if yes, then handleBody is called with index and len both at 0 right ?
Note: I am thinking of an empty file... I see that is seems to be handled in your example - just wanted to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PUT an empty file is certainly a thing; MacOS does it routinely, first creating an empty file, then locking it before writing data and related hidden metadata files. Mac uses Content-Length for the initial empty file, then chunked for the later ops. I do not know of any clients that send chunked zero length files, but it is certainly possible to do it with curl -T - http://192.168.4.1/foo </dev/null. That works in my test example as expected, with no special handling needed.
I will work through the copilot comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for HTTP chunked transfer encoding in incoming requests to the ESPAsyncWebServer library. This enables clients to send request bodies without knowing the total content length in advance, which is particularly useful for WebDAV operations and large file uploads.
Changes:
- Adds chunked transfer encoding parsing state machine to handle Transfer-Encoding: chunked requests
- Introduces support for the non-standard X-Expected-Entity-Length header used by macOS WebDAVFS
- Adds HTTP 507 "Insufficient Storage" status code for space-related errors
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/literals.h | Adds X-Expected-Entity-Length header constant and HTTP 507 status code string |
| src/ESPAsyncWebServer.h | Declares new member variables for chunk parsing state (_chunkStartIndex, _chunkOffset, _chunkSize, _chunkedParseState) and the _parseChunkedBytes method |
| src/WebRequest.cpp | Implements chunked encoding state machine, adds Transfer-Encoding header detection, initializes chunked state variables, and routes chunked requests through new parsing logic |
| examples/ChunkRequest/ChunkRequest.ino | Provides example demonstrating chunked PUT requests with file storage, including space checking and error handling |
| platformio.ini | Adds reference to the new ChunkRequest example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // is the default-contructed one. The presence of | ||
|
|
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "If we already returned, the File object in request->_tempObject is the default-contructed one. The presence of" appears to be incomplete and ends mid-sentence. Please complete this comment to explain the intended meaning.
| // is the default-contructed one. The presence of | |
| // is the default-constructed one. The presence of a non-default-constructed | |
| // File in state->outFile below is used as a flag that the file was opened | |
| // successfully and is ready to receive body data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, it is impressive how well it figured that out.
| return; | ||
| } | ||
| // If we already returned, the File object in request->_tempObject | ||
| // is the default-contructed one. The presence of |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "default-contructed" should be "default-constructed".
| // is the default-contructed one. The presence of | |
| // is the default-constructed one. The presence of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
src/WebRequest.cpp
Outdated
| // A zero length chunk marks the end of the chunk stream | ||
| _chunkOffset = 0; | ||
| _chunkedParseState = CHUNK_DATA; | ||
| } | ||
| } else if (_chunkedParseState == CHUNK_END) { | ||
| if (data == '\n') { |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "A zero length chunk marks the end of the chunk stream" is in the wrong place. This comment is in the CHUNK_EXTENSION state handler, but the actual check for a zero-length chunk ending the stream is at line 411 in the CHUNK_END state. This comment should be moved or removed to avoid confusion.
| // A zero length chunk marks the end of the chunk stream | |
| _chunkOffset = 0; | |
| _chunkedParseState = CHUNK_DATA; | |
| } | |
| } else if (_chunkedParseState == CHUNK_END) { | |
| if (data == '\n') { | |
| _chunkOffset = 0; | |
| _chunkedParseState = CHUNK_DATA; | |
| } | |
| } else if (_chunkedParseState == CHUNK_END) { | |
| if (data == '\n') { | |
| // A zero length chunk marks the end of the chunk stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MitchBradley : to be verified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
| _chunkedParseState = CHUNK_EXTENSION; | ||
| } else if (data == '\n') { | ||
| _chunkOffset = 0; | ||
| _chunkedParseState = CHUNK_DATA; |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chunk length parser doesn't handle invalid characters. If a character that is not a hex digit (0-9, A-F, a-f), semicolon, or newline is encountered in CHUNK_LENGTH state, it is silently ignored. According to RFC 9112, invalid characters should cause the request to be rejected. Consider adding error handling for invalid characters, for example by transitioning to a PARSE_ERROR state or aborting the connection.
| _chunkedParseState = CHUNK_DATA; | |
| _chunkedParseState = CHUNK_DATA; | |
| } else if (data != '\r') { | |
| // Invalid character in chunk length: reject the request | |
| _parseState = PARSE_REQ_FAIL; | |
| return true; |
| return true; | ||
| } | ||
| _chunkSize = 0; | ||
| _chunkedParseState = CHUNK_LENGTH; |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CHUNK_END state parser doesn't handle invalid characters. If a character other than newline is encountered in CHUNK_END state (which expects CRLF), it is silently ignored. This could lead to malformed chunks being accepted. Consider adding error handling for unexpected characters in this state.
| _chunkedParseState = CHUNK_LENGTH; | |
| _chunkedParseState = CHUNK_LENGTH; | |
| } else { | |
| // Unexpected character in CHUNK_END state: abort parsing. | |
| if (_client) { | |
| _client->close(); | |
| } | |
| return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tough one. I added a CHUNK_ERROR state that will ignore the rest of the data stream, but I'm not sure how to signal the error to the application layer (handleBody). Any ideas?
src/WebRequest.cpp
Outdated
| _isPlainPost(false), _expectingContinue(false), _contentLength(0), _parsedLength(0), _multiParseState(0), _boundaryPosition(0), _itemStartIndex(0), | ||
| _itemSize(0), _itemName(), _itemFilename(), _itemType(), _itemValue(), _itemBuffer(0), _itemBufferIndex(0), _itemIsFile(false), _tempObject(NULL) { | ||
| _itemSize(0), _itemName(), _itemFilename(), _itemType(), _itemValue(), _itemBuffer(0), _itemBufferIndex(0), _itemIsFile(false), | ||
| _chunkedParseState(CHUNK_NONE), _tempObject(NULL) { |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chunk-related member variables (_chunkStartIndex, _chunkOffset, _chunkSize) should be initialized in the constructor. Although _chunkSize and _chunkStartIndex are set when Transfer-Encoding is detected, and _chunkOffset is set before being used in the state machine, it's better practice to initialize all member variables for safety and clarity.
| _chunkedParseState(CHUNK_NONE), _tempObject(NULL) { | |
| _chunkedParseState(CHUNK_NONE), _chunkStartIndex(0), _chunkOffset(0), _chunkSize(0), _tempObject(NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
src/literals.h
Outdated
| DECLARE_STR(T_HTTP_CODE_503, "Service Unavailable"); | ||
| DECLARE_STR(T_HTTP_CODE_504, "Gateway Time-out"); | ||
| DECLARE_STR(T_HTTP_CODE_505, "HTTP Version Not Supported"); | ||
| DECLARE_STR(T_HTTP_CODE_507, "Insufficient storage"); |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HTTP 507 status message should be "Insufficient Storage" (with capital 'S') according to RFC 4918. The lowercase 's' in "storage" is inconsistent with other status messages in this file which use title case.
| DECLARE_STR(T_HTTP_CODE_507, "Insufficient storage"); | |
| DECLARE_STR(T_HTTP_CODE_507, "Insufficient Storage"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
| if (_chunkedParseState == CHUNK_LENGTH) { | ||
| // Incrementally decode a hex number | ||
| if (data >= '0' && data <= '9') { | ||
| _chunkSize = (_chunkSize * 16) + (data - '0'); | ||
| } else if (data >= 'A' && data <= 'F') { | ||
| _chunkSize = (_chunkSize * 16) + (data - 'A' + 10); | ||
| } else if (data >= 'a' && data <= 'f') { | ||
| _chunkSize = (_chunkSize * 16) + (data - 'a' + 10); | ||
| } else if (data == ';') { | ||
| _chunkedParseState = CHUNK_EXTENSION; | ||
| } else if (data == '\n') { | ||
| _chunkOffset = 0; | ||
| _chunkedParseState = CHUNK_DATA; | ||
| } |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chunk size parsing doesn't protect against integer overflow. A malicious client could send a very large hex number (e.g., FFFFFFFFFFFFFFFF) which would cause _chunkSize to overflow when multiplied by 16 repeatedly. Consider adding a check to limit the chunk size to a reasonable maximum (e.g., checking if _chunkSize would overflow before multiplying, or limiting the number of hex digits accepted).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a check for overflowing a 32-bit number. As with the invalid length character thing, it's unclear how best to handle at the application level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.... And even if handled, this is still an MCU so subject to be abused in a lot of ways (memory or file system).
When serving request if I remember to have seen that a . If that can help. At least it limits to 65k. Here a size_t is used. I don't know what the spec says about the max length of a chunk ?uin16_t is used represent chunk sizes
I've checked and I think I'm wrong. Besides, the http spec does not set a limit on chunks so on request side it could be big...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change I made just checks for overflow, by erroring out if you try to add a hex digit to a number that is already >= 0x10000000. It's unclear just how valuable that is in practice, but whatever. Appease our AI overlords. If the chunk is too large to be actually handled, the application can take care of that - and the example does so.
|
I think it's ready for the next step. The remaining comments from Copilot don't seem worthwhile. |
No description provided.