Implemented suggestions from Copilot

Author: MitchBradley

examples/ChunkRequest/ChunkRequest.ino

@@ -58,44 +58,51 @@ void handleRequest(AsyncWebServerRequest *request) {
   }
 
   // If request->_tempObject is not null, handleBody already
-  // did the necessary work for a PUT operation
+  // did the necessary work for a PUT operation.  Otherwise,
+  // handleBody was either not called, or did nothing, so we
+  // handle the request later in this routine.  That happens
+  // when a non-chunked PUT has Content-Length: 0.
   auto state = static_cast<RequestState *>(request->_tempObject);
   if (state) {
+    // If handleBody successfully opened the file, whether or not it
+    // wrote data to it, we close it here and send the "created"
+    // response.  If handleBody did not open the file, because the
+    // open attempt failed or because the operation was rejected,
+    // state will be non-null but state->outFile will be false.  In
+    // that case, handleBody has already sent an appropriate
+    // response code.
+
     if (state->outFile) {
       // The file was already opened and written in handleBody so
-      // we are done.  We will handle PUT without body data below.
+      // we close it here and issue the appropriate response.
       state->outFile.close();
       request->send(201);  // Created
     }
+    // Finally, releast the resources used by state
     delete state;
     request->_tempObject = nullptr;
     return;
   }
 
   String path = request->url();
 
-  if (request->method() == HTTP_PUT) {
-    // This PUT code executes if the body was empty, which
-    // can happen if the client creates a zero-length file.
-    // MacOS WebDAVFS does that, then later LOCKs the file
-    // and issues a subsequent PUT with body contents.
+  // This PUT code executes if the body was empty, which
+  // can happen if the client creates a zero-length file.
+  // MacOS WebDAVFS does that, then later LOCKs the file
+  // and issues a subsequent PUT with body contents.
 
 #ifdef ESP32
-    File file = LittleFS.open(path, "w", true);
+  File file = LittleFS.open(path, "w", true);
 #else
-    File file = LittleFS.open(path, "w");
+  File file = LittleFS.open(path, "w");
 #endif
 
-    if (file) {
-      file.close();
-      request->send(201);  // Created
-      return;
-    }
-    request->send(403);
+  if (file) {
+    file.close();
+    request->send(201);  // Created
     return;
   }
-
-  request->send(404);
+  request->send(403);
 }
 
 void handleBody(AsyncWebServerRequest *request, unsigned char *data, size_t len, size_t index, size_t total) {
@@ -141,8 +148,12 @@ void handleBody(AsyncWebServerRequest *request, unsigned char *data, size_t len,
         request->send(403, "text/plain", "Cannot PUT to a directory");
         return;
       }
-      // If we already returned, the File object in request->_tempObject
-      // is the default-contructed one.  The presence of
+      // If we already returned, the File object in
+      // request->_tempObject is the default-constructed one.  The
+      // presence of a non-default-constructed File in state->outFile
+      // indicates that the file was opened successfully and is ready
+      // to receive body data.  The File will be closed later when
+      // handleRequest is called after all calls to handleBody
 
       std::swap(state->outFile, file);
       // Now request->_tempObject contains the actual file object which owns it,

src/WebRequest.cpp

@@ -34,15 +34,16 @@ enum {
   CHUNK_LENGTH,     // Getting chunk length - HHHH[;...] CR LF
   CHUNK_EXTENSION,  // Getting chunk length - HHHH[;...] CR LF
   CHUNK_DATA,       // Handling chunk data
+  CHUNK_ERROR,      // Invalid chunk header
   CHUNK_END,        // Getting chunk end marker  - CR LF
 };
 
 AsyncWebServerRequest::AsyncWebServerRequest(AsyncWebServer *s, AsyncClient *c)
   : _client(c), _server(s), _handler(NULL), _response(NULL), _onDisconnectfn(NULL), _temp(), _parseState(PARSE_REQ_START), _version(0), _method(HTTP_ANY),
     _url(), _host(), _contentType(), _boundary(), _authorization(), _reqconntype(RCT_HTTP), _authMethod(AsyncAuthType::AUTH_NONE), _isMultipart(false),
     _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),
-    _chunkedParseState(CHUNK_NONE), _tempObject(NULL) {
+    _itemSize(0), _itemName(), _itemFilename(), _itemType(), _itemValue(), _itemBuffer(0), _itemBufferIndex(0), _itemIsFile(false), _chunkStartIndex(0),
+    _chunkOffset(0), _chunkSize(0), _chunkedParseState(CHUNK_NONE), _tempObject(NULL) {
   c->onError(
     [](void *r, AsyncClient *c, int8_t error) {
       (void)c;
@@ -389,25 +390,45 @@ bool AsyncWebServerRequest::_parseChunkedBytes(uint8_t *buf, size_t len) {
       if (_chunkedParseState == CHUNK_LENGTH) {
         // Incrementally decode a hex number
         if (data >= '0' && data <= '9') {
-          _chunkSize = (_chunkSize * 16) + (data - '0');
+          if (_chunkSize >= 0x1000000) {
+            _chunkedParseState = CHUNK_ERROR;
+          } else {
+            _chunkSize = (_chunkSize * 16) + (data - '0');
+          }
         } else if (data >= 'A' && data <= 'F') {
-          _chunkSize = (_chunkSize * 16) + (data - 'A' + 10);
+          if (_chunkSize >= 0x1000000) {
+            _chunkedParseState = CHUNK_ERROR;
+          } else {
+            _chunkSize = (_chunkSize * 16) + (data - 'A' + 10);
+          }
         } else if (data >= 'a' && data <= 'f') {
-          _chunkSize = (_chunkSize * 16) + (data - 'a' + 10);
+          if (_chunkSize >= 0x1000000) {
+            _chunkedParseState = CHUNK_ERROR;
+          } else {
+            _chunkSize = (_chunkSize * 16) + (data - 'a' + 10);
+          }
         } else if (data == ';') {
           _chunkedParseState = CHUNK_EXTENSION;
+        } else if (data == '\r') {
+          // Ignore CR; wait for LF
         } else if (data == '\n') {
           _chunkOffset = 0;
           _chunkedParseState = CHUNK_DATA;
+        } else {
+          // Invalid hex character
+          _chunkedParseState = CHUNK_ERROR;
         }
       } else if (_chunkedParseState == CHUNK_EXTENSION) {
+        // Chunk extensions appear after a semicolon.
+        // We ignore them because their use cases are
+        // specialized and obscure.
         if (data == '\n') {
-          // A zero length chunk marks the end of the chunk stream
           _chunkOffset = 0;
           _chunkedParseState = CHUNK_DATA;
         }
       } else if (_chunkedParseState == CHUNK_END) {
         if (data == '\n') {
+          // A zero length chunk marks the end of the chunk stream
           if (_chunkSize == 0) {
             // If we needed to support trailers, we would switch to
             // TRAILER state, but since we have no use case for them,
@@ -417,6 +438,13 @@ bool AsyncWebServerRequest::_parseChunkedBytes(uint8_t *buf, size_t len) {
           _chunkSize = 0;
           _chunkedParseState = CHUNK_LENGTH;
         }
+      } else if (_chunkedParseState == CHUNK_ERROR) {
+        // If there was an error when parsing the chunk length, the
+        // rest of the data stream is unreliable.  Ideally we should
+        // close the connection, but that risks leaving things dangling
+        // (e.g. an open file), so it is probably best to just ignore
+        // the rest of the data and give handleRequest a chance to
+        // clean up.
       }
     }
   }

src/literals.h

@@ -216,7 +216,7 @@ DECLARE_STR(T_HTTP_CODE_502, "Bad Gateway");
 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");
+DECLARE_STR(T_HTTP_CODE_507, "Insufficient Storage");
 DECLARE_STR(T_HTTP_CODE_ANY, "Unknown code");
 
 static constexpr const char *T_only_once_headers[] = {