From bf6810b7814cde838b6689b9075bacca0a6af091 Mon Sep 17 00:00:00 2001 From: Brad Arant Date: Sun, 23 Aug 2026 15:07:09 -0700 Subject: [PATCH] Rebuild CGIFormData on Parser/StreamReader, fix a 2-byte-lookahead bug CGIFormData now inherits Parser instead of MString and parses a CGI request body (urlencoded or multipart/form-data) resumably against the shared StreamReader, exposing the result via operator[] - matching every other Parser in this redesign. multipart/form-data reuses IMFBoundary and IMFHeader via checkParse() (chained through the same StreamReader) for each part's boundary line and header block; urlencoded bodies wait for the full Content-Length before delegating to the existing CGIFormattedData parser. File-upload parts (Content-Disposition with a filename= parameter) are recognized and skipped, matching this codebase's convention that uploads get a separate mechanism. Values are stored under an indexed "name:N" key (N = 0-based occurrence count for that name), matching the :N-suffix convention JetCore's Tag::getVariable already applies to its $[:name] CGI variable syntax (defaulting to :0) - this also naturally supports repeated field names (checkboxes, multi-select). operator[] applies the same :0 default so a bare name still works when called directly, not just through that convention layer. Two real resumability bugs found and fixed via a byte-at-a-time test (feeding one byte per StreamReader::feed() call, the harshest possible fragmentation): - CGIFormData's opening boundary marker ("--boundary", no leading CRLF per RFC 2046 for the first boundary) crammed "find+consume the marker" and "disambiguate -- vs CRLF" into one state; when the disambiguation needed more data, INCOMPLETE was returned without recording that the marker was already consumed, so retrying re-searched for it past where it had already been consumed, silently skipping content forever. Split into two states (INIT / OPENING_TAIL), the same fix already applied once to IMFBoundary's PREFIX/TAIL split. - IMFHeader's FIELD state checked eod() (a 1-byte availability check) immediately before an ifNext("\r\n") call that needs 2 bytes - a lone trailing \r with no \n yet available was wrongly treated as malformed instead of "wait for more data". Fixed to check getLength() < 2. Verified via a dedicated test (64 assertions total across this and the earlier Parser/StreamReader test) covering urlencoded, multipart with a skipped file upload, and the byte-at-a-time stress case - plus a real end-to-end run through jet-2.0 with a simulated CGI multipart POST request (see the matching JetCore commit). Co-Authored-By: Claude Sonnet 5 --- CGIFormData.cpp | 196 +++++++++++++++++++++++++++++++++++++----------- CGIFormData.h | 72 +++++++++++++++--- IMFHeader.cpp | 6 +- 3 files changed, 218 insertions(+), 56 deletions(-) diff --git a/CGIFormData.cpp b/CGIFormData.cpp index b199b86..6c79679 100644 --- a/CGIFormData.cpp +++ b/CGIFormData.cpp @@ -1,58 +1,168 @@ #include "CGIFormData.h" -#include "malloc.h" +#include "CGIFormattedData.h" +#include "Exception.h" namespace coreutils { - - CGIFormData::CGIFormData(int fd, ZString contentType, int contentLength, ZString boundary) - : contentType(contentType) { - if(contentType.startsWith("application/x-www-form-urlencoded")) { - read(fd, contentLength); - formdata = new CGIFormattedData(*this); - } else if(contentType.startsWith("multipart/form-data")) { - read(fd, contentLength); - formdata = new CGIFormattedData(); - MString delimiter; - delimiter << "--" << boundary; - ZString body(*this); - body.split(delimiter); + CGIFormData::CGIFormData(StreamReader &reader, ZString contentType, int contentLength, ZString boundary) + : Parser(reader), contentType(contentType), contentLength(contentLength), boundary(boundary) { + if (contentType.startsWith("application/x-www-form-urlencoded")) + mode = URLENCODED; + else if (contentType.startsWith("multipart/form-data")) + mode = MULTIPART; + else + mode = UNKNOWN; + } - for(int ix = 0; ix < body.getList().size(); ++ix) { - ZString part = body[ix]; - if(part.getLength() == 0 || part.startsWith("--")) - continue; + CGIFormData::~CGIFormData() { + delete headerParser; + delete boundaryParser; + } - int headerEnd = part.find("\r\n\r\n"); - if(headerEnd == -1) - continue; + void CGIFormData::extractPartName(IMFHeader &header) { + auto &fields = header.getHeaders(); + MString disposition = fields[MString("Content-Disposition")]; + ZString dz(disposition.getData(), disposition.getLength()); - ZString headers = part.substring(0, headerEnd); - if(headers.find("filename=\"") != -1) - // file uploads are out of scope here; handled via a separate upload mechanism. - continue; + currentSkip = (dz.find(ZString("filename=\"")) != -1); - int namePos = headers.find("name=\""); - if(namePos == -1) - continue; - - ZString afterName = headers.substring(namePos + 6); - int nameEnd = afterName.find("\""); - if(nameEnd == -1) - continue; - ZString name = afterName.substring(0, nameEnd); - - ZString value = part.substring(headerEnd + 4); - value.trimCRLF(); - - formdata->data[name] = value; - } + int namePos = dz.find(ZString("name=\"")); + if (namePos != -1) { + ZString afterName = dz.substring(namePos + 6); + int nameEnd = afterName.find(ZString("\"")); + if (nameEnd != -1) + currentName = MString(afterName.substring(0, nameEnd)); } } + Parser::Status CGIFormData::parse() { + MString &buffer = reader.getBuffer(); + + if (mode == UNKNOWN) { + state = DONE; + return Status::COMPLETE; + } + + switch (state) { + + case INIT: + if (mode == URLENCODED) { + state = URLENCODED_BODY; + return Status::DATA_AVAIL; + } + // MULTIPART: the very first boundary line has no leading CRLF + // (RFC 2046) - handled here directly rather than via IMFBoundary, + // which always expects the CRLF that terminates a PRECEDING part. + // Split into two states (find+consume the marker, then separately + // disambiguate -- vs CRLF) so that if the marker is consumed but + // there isn't yet enough data to tell -- from CRLF, retrying does + // not re-search for (and skip past) a marker that is already + // behind the cursor - the exact bug class already fixed once in + // IMFBoundary's PREFIX/TAIL split. + { + MString marker; + marker << "--"; + marker << boundary; + int pos = buffer.find(marker); + if (pos < 0) + return Status::INCOMPLETE; + if (!buffer.ifNext(marker)) + throw coreutils::Exception("CGIFormData: opening boundary vanished between find and ifNext."); + state = OPENING_TAIL; + return Status::DATA_AVAIL; + } + + case OPENING_TAIL: + if (buffer.getLength() < 2) + return Status::INCOMPLETE; + if (buffer.ifNext(ZString("--"))) { + state = DONE; + return Status::COMPLETE; + } + if (!buffer.ifNext(ZString("\r\n"))) + throw coreutils::Exception("CGIFormData: malformed opening boundary line."); + state = PART_HEADERS; + return Status::DATA_AVAIL; + + case URLENCODED_BODY: + if ((int)buffer.getLength() < contentLength) + return Status::INCOMPLETE; + { + ZString body(buffer.getCursor(), contentLength); + buffer.setCursor(buffer.getCursor() + contentLength); + CGIFormattedData parsed(body); + for (auto &pair : parsed.data) + storeValue(pair.first, pair.second); + } + state = DONE; + return Status::COMPLETE; + + case PART_HEADERS: + if (!headerParser) + headerParser = new IMFHeader(reader); + if (!checkParse(*headerParser)) + return Status::INCOMPLETE; + extractPartName(*headerParser); + delete headerParser; + headerParser = NULL; + state = PART_BODY; + return Status::DATA_AVAIL; + + case PART_BODY: { + MString marker; + marker << "\r\n--"; + marker << boundary; + char *bodyStart = buffer.getCursor(); + int pos = buffer.find(marker); + if (pos < 0) + return Status::INCOMPLETE; + char *bodyEnd = buffer.getCursor(); + if (!currentSkip && currentName.getLength() > 0) + storeValue(currentName, MString(ZString(bodyStart, bodyEnd - bodyStart))); + currentName = MString(); + currentSkip = false; + state = PART_BOUNDARY; + return Status::DATA_AVAIL; + } + + case PART_BOUNDARY: + if (!boundaryParser) + boundaryParser = new IMFBoundary(reader, boundary); + if (!checkParse(*boundaryParser)) + return Status::INCOMPLETE; + { + bool last = boundaryParser->isLast(); + delete boundaryParser; + boundaryParser = NULL; + if (last) { + state = DONE; + return Status::COMPLETE; + } + state = PART_HEADERS; + return Status::DATA_AVAIL; + } + + case DONE: + return Status::COMPLETE; + + } + throw coreutils::Exception("CGIFormData: unreachable state."); + } + + void CGIFormData::storeValue(MString name, MString value) { + int index = nameCounts[name]++; + MString key = name; + key << ":"; + key << MString(std::to_string(index)); + data[key] = value; + } + MString CGIFormData::operator[](ZString name) { - if(formdata == NULL) - return MString(); - return (*formdata).data[name]; + MString key(name); + if (key.find(ZString(":")) == -1) { + key << ":0"; + } + return data[key]; } } diff --git a/CGIFormData.h b/CGIFormData.h index 860799e..c3e022d 100644 --- a/CGIFormData.h +++ b/CGIFormData.h @@ -1,27 +1,75 @@ #ifndef __CGIFormData_h__ # define __CGIFormData_h__ -#include "MString.h" -#include "ZString.h" -#include "CGIFormattedData.h" +#include "Parser.h" +#include "IMFHeader.h" +#include "IMFBoundary.h" +#include namespace coreutils { - class CGIFormData : public MString { + /// + /// CGIFormData parses a CGI request body (application/x-www-form-urlencoded + /// or multipart/form-data) from the shared StreamReader and exposes the + /// resulting name/value pairs via operator[] - the object a caller (e.g. + /// JetCore's Tag::getVariable, for its $[:name] CGI variable syntax) holds + /// onto and indexes by variable name once doParse() reports COMPLETE. + /// + /// Resumable like every other Parser here: for multipart/form-data, each + /// part's boundary line and header block are parsed via IMFBoundary/ + /// IMFHeader (chained through checkParse(), sharing this same + /// StreamReader), so a part - or even its boundary marker - can straddle + /// any number of feed() calls without losing state. File-upload parts + /// (Content-Disposition with a filename= parameter) are recognized and + /// skipped - matching this codebase's existing convention that uploads + /// get a separate mechanism, not buffered through here. + /// + + class CGIFormData : public Parser { public: - CGIFormData(int fd, ZString contentType, int contentLength, ZString boundary); - + + CGIFormData(StreamReader &reader, ZString contentType, int contentLength, ZString boundary); + virtual ~CGIFormData(); + MString operator[](ZString name); - + + protected: + + Status parse() override; + private: - CGIFormattedData *formdata = NULL; - MString contentType; - + + enum Mode { URLENCODED, MULTIPART, UNKNOWN } mode; + enum State { INIT, OPENING_TAIL, PART_HEADERS, PART_BODY, PART_BOUNDARY, URLENCODED_BODY, DONE } state = INIT; + + ZString contentType; + int contentLength; + ZString boundary; + std::map data; + + IMFHeader *headerParser = NULL; + IMFBoundary *boundaryParser = NULL; + MString currentName; + bool currentSkip = false; + std::map nameCounts; + + void extractPartName(IMFHeader &header); + + /// + /// Stores value under "name:N", where N is how many values have + /// already been stored under this same base name (0 for the first, + /// 1 for the second, etc.) - matches the indexed key format + /// JetCore's Tag::getVariable's $[:name] syntax looks up by + /// (defaulting to :0 when the caller doesn't specify an index), + /// supporting repeated field names (checkboxes, multi-select) the + /// same way. + /// + + void storeValue(MString name, MString value); + }; } #endif - - diff --git a/IMFHeader.cpp b/IMFHeader.cpp index 1635b65..bfe68a9 100644 --- a/IMFHeader.cpp +++ b/IMFHeader.cpp @@ -40,7 +40,11 @@ namespace coreutils { case FIELD: field << buffer.getTokenExclude("\r\n"); - if (buffer.eod()) + // Need 2 bytes buffered to reliably test for "\r\n" - eod() (a + // 1-byte check) would wrongly treat a lone trailing \r as + // malformed instead of "wait for the \n". Same bug class already + // fixed once in IMFBoundary's TAIL state. + if (buffer.getLength() < 2) return Status::INCOMPLETE; if (!buffer.ifNext(ZString("\r\n"))) throw coreutils::Exception("IMFHeader: expected CRLF at end of header field.");