From 63ebef3740d1b7a0354b8a87917ef38194737c4e Mon Sep 17 00:00:00 2001 From: Scott Claiborne Date: Thu, 11 Jun 2026 19:09:38 -0700 Subject: [PATCH 1/3] Add regression tests for HTTP response status codes Covers the LLHttpResponse status input end-to-end through the server send path, plus LLHttpBuildResponse unit coverage for non-200 codes, the status 0 default, and content-length on empty payloads. Co-Authored-By: Claude Fable 5 --- test/tests.cpp | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/test/tests.cpp b/test/tests.cpp index 4379734..b0e844c 100644 --- a/test/tests.cpp +++ b/test/tests.cpp @@ -24,6 +24,7 @@ extern "C" { #include #include #include +#include using namespace std; @@ -218,6 +219,42 @@ TEST_CASE( "Test HTTP Build Response", "[LLHttp]" ) { CHECK_THAT(buffer, Catch::Matchers::ContainsSubstring(contentLength)); CHECK_THAT(buffer, Catch::Matchers::ContainsSubstring(date)); } + + SECTION("Build Http Response with non-200 status") { + strcpy(response.uri, "/"); + response.pPayload = (UDINT)&"conflict"; + response.payloadLength = strlen((char*)response.pPayload); + response.status = 409; + LLHttpBuildResponse((UDINT)&buffer, (UDINT)&response, sizeof(buffer), &bufferLen); + CHECK_THAT(buffer, Catch::Matchers::StartsWith("HTTP/1.1 409 Conflict\r\n")); + } + + SECTION("Build Http Response defaults status 0 to 200 OK") { + strcpy(response.uri, "/"); + response.pPayload = (UDINT)&"simple"; + response.payloadLength = strlen((char*)response.pPayload); + response.status = 0; // Application never set the status input + LLHttpBuildResponse((UDINT)&buffer, (UDINT)&response, sizeof(buffer), &bufferLen); + CHECK_THAT(buffer, Catch::Matchers::StartsWith("HTTP/1.1 200 OK\r\n")); + } + + SECTION("Build Http Response with empty payload includes content-length: 0") { + strcpy(response.uri, "/"); + response.status = 202; + LLHttpBuildResponse((UDINT)&buffer, (UDINT)&response, sizeof(buffer), &bufferLen); + CHECK_THAT(buffer, Catch::Matchers::StartsWith("HTTP/1.1 202 Accepted\r\n")); + const std::string contentLengthZero{ "\r\ncontent-length: 0\r\n" }; + CHECK_THAT(buffer, Catch::Matchers::ContainsSubstring(contentLengthZero)); + CHECK_THAT(buffer, Catch::Matchers::EndsWith("\r\n\r\n")); + } + + SECTION("Build Http Response with 204 status omits content-length") { + strcpy(response.uri, "/"); + response.status = 204; // RFC 9110: no content-length on 204 + LLHttpBuildResponse((UDINT)&buffer, (UDINT)&response, sizeof(buffer), &bufferLen); + CHECK_THAT(buffer, Catch::Matchers::StartsWith("HTTP/1.1 204 No Content\r\n")); + CHECK_THAT(buffer, !Catch::Matchers::ContainsSubstring(std::string{"content-length"})); + } } TEST_CASE( "Test HTTP Header line utility", "[LLHttp]") { @@ -372,6 +409,81 @@ static void serverNewMessageCallback(UDINT context, LLHttpServiceLink_typ* api, strncpy(serverCallbackBody, (char*)data, sizeof(serverCallbackBody)-1); } +TEST_CASE( "Test HTTP Server response honors the LLHttpResponse status input", "[LLHttp]") { + + struct StatusCase { UDINT status; const char* statusLine; }; + StatusCase c = GENERATE( + StatusCase{200, "HTTP/1.1 200 OK\r\n"}, + StatusCase{202, "HTTP/1.1 202 Accepted\r\n"}, + StatusCase{204, "HTTP/1.1 204 No Content\r\n"}, + StatusCase{400, "HTTP/1.1 400 Bad Request\r\n"}, + StatusCase{404, "HTTP/1.1 404 Not Found\r\n"}, + StatusCase{409, "HTTP/1.1 409 Conflict\r\n"}, + StatusCase{500, "HTTP/1.1 500 Internal Server Error\r\n"}, + StatusCase{0, "HTTP/1.1 200 OK\r\n"}); // Status left at 0 defaults to 200 + + CAPTURE(c.status); + + LLHttpServer_typ server = {}; + server.enable = true; + server.numClients = LLHTTP_MAX_NUM_CLIENTS; + server.bufferSize = 2000; + + // First cycle initializes internals and publishes server.ident + LLHttpServer(&server); + + LLHttpServerInternalClient_typ* client = &server.internal.pClients[0]; + + // Service FB listening for the request, like a PLC task would set it up + LLHttpResponse_typ fbResp = {}; + fbResp.ident = server.ident; + fbResp.method = LLHTTP_METHOD_POST; + strcpy(fbResp.uri, "/api/brew"); + fbResp.enable = true; + LLHttpResponse(&fbResp); // Registers the handler + + const char* request = + "POST /api/brew HTTP/1.1\r\n"\ + "Host: plc.local\r\n"\ + "content-type: application/json\r\n"\ + "content-length: 13\r\n"\ + "\r\n"\ + "{\"brew\":true}"; + + // Preload the request so it is in place when the stubbed connection manager accepts + strcpy((char*)client->pReceiveData, request); + client->tcpStream.Internal.FUB.Receive.recvlen = strlen(request); + + // Cycle until the request is dispatched to the service FB + int cycles = 0; + while(!fbResp.newRequest && ++cycles < 10) { + LLHttpServer(&server); + LLHttpResponse(&fbResp); + } + REQUIRE(fbResp.newRequest == 1); + + // Respond like an application would: status and send set in the same cycle, + // with the FB called at the bottom of the cyclic + const char* body = "{\"queued\":true}"; + fbResp.status = c.status; + fbResp.pContent = (UDINT)body; + fbResp.contentLength = strlen(body); + fbResp.send = true; + LLHttpResponse(&fbResp); // Queues the response + + // Next server cycle dequeues the response, builds the packet, and sends it + LLHttpServer(&server); + + CHECK_THAT((char*)client->pSendData, Catch::Matchers::StartsWith(c.statusLine)); + CHECK_THAT((char*)client->pSendData, Catch::Matchers::EndsWith(body)); + + // FB reports completion on its next call + LLHttpResponse(&fbResp); + CHECK(fbResp.done == 1); + CHECK(fbResp.error == 0); + +} + TEST_CASE( "Test HTTP Server POST body split across TCP segments", "[LLHttp]") { LLHttpServer_typ server = {}; From 757745ff66086d8c37bd2ad2f235aad46009f4b3 Mon Sep 17 00:00:00 2001 From: Scott Claiborne Date: Thu, 11 Jun 2026 19:29:05 -0700 Subject: [PATCH 2/3] Fix URI match so query strings cannot match a longer handler pattern LLHttpUriMatch returned a match as soon as the request URI reached its query string, without requiring the handler pattern to be consumed too. A request like 'POST /api/brew?durationS=180' therefore dispatched to a handler listening on '/api/brew/abort' as well as the '/api/brew' one. Every matching LLHttpResponse FB then answered the same request, and whichever response was queued first reached the client - typically a sibling route's 200 that masked the real route's custom status (202, 409, ...). Requests without a query string were unaffected, which made the bug look like the status input was ignored. Co-Authored-By: Claude Fable 5 --- src/Ar/LLHttp/HttpMisc.c | 5 ++- test/tests.cpp | 87 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 89 insertions(+), 3 deletions(-) diff --git a/src/Ar/LLHttp/HttpMisc.c b/src/Ar/LLHttp/HttpMisc.c index 925e46f..ab27056 100644 --- a/src/Ar/LLHttp/HttpMisc.c +++ b/src/Ar/LLHttp/HttpMisc.c @@ -53,7 +53,10 @@ plcbit LLHttpUriMatch(unsigned long _a, unsigned long _b) { } if(*b == '?') { // We do not support matching parameters - return 1; // We have matched everything but the parameters + // The request path ends at the query string. This is only a match if the + // handler pattern is exhausted too, otherwise '/api/brew?x=1' would match + // a handler listening on '/api/brew/abort' + return (*a == '\0' || *a == '?'); } if(*a == '*') { // Match next element diff --git a/test/tests.cpp b/test/tests.cpp index b0e844c..6acedce 100644 --- a/test/tests.cpp +++ b/test/tests.cpp @@ -75,6 +75,18 @@ TEST_CASE( "Test HTTP URI Match", "[LLHttp]" ) { testHttpUriMatch("/files/test.cpp", "/other", false); testHttpUriMatch("/files/test.cpp", "/", false); + } + SECTION( "Should only ignore query parameters when the full path matched" ) { + + testHttpUriMatch("/api/brew", "/api/brew?durationS=180", true); + testHttpUriMatch("/api/brew/abort", "/api/brew?durationS=180", false); + testHttpUriMatch("/api/brew", "/api/brew/abort?x=1", false); + testHttpUriMatch("/api/brew/abort", "/api/brew/abort?x=1", true); + testHttpUriMatch("/api/brewery", "/api/brew?x=1", false); + testHttpUriMatch("/api/brew", "/api/brewery?x=1", false); + testHttpUriMatch("/files/*", "/files/test?x=1", true); + testHttpUriMatch("/files/**", "/files/deeper/test?x=1", true); + } #undef testHttpUriMatch @@ -454,12 +466,13 @@ TEST_CASE( "Test HTTP Server response honors the LLHttpResponse status input", " strcpy((char*)client->pReceiveData, request); client->tcpStream.Internal.FUB.Receive.recvlen = strlen(request); - // Cycle until the request is dispatched to the service FB + // Cycle until the connection is accepted and the request dispatched to the service FB int cycles = 0; - while(!fbResp.newRequest && ++cycles < 10) { + while(!(client->connected && fbResp.newRequest) && ++cycles < 10) { LLHttpServer(&server); LLHttpResponse(&fbResp); } + REQUIRE(client->connected == 1); REQUIRE(fbResp.newRequest == 1); // Respond like an application would: status and send set in the same cycle, @@ -484,6 +497,76 @@ TEST_CASE( "Test HTTP Server response honors the LLHttpResponse status input", " } +TEST_CASE( "Test HTTP Server query string request dispatches only to the matching route", "[LLHttp]") { + + // Regression for custom status codes arriving as 200 at the client: a request + // with a query string ('POST /api/brew?durationS=180') used to match every + // handler whose URI is a prefix of the request path ('/api/brew/abort' too). + // Both routes then answered the same request and whichever response was queued + // first won the wire - typically a 200 that masked the real route's 202/409 + + LLHttpServer_typ server = {}; + server.enable = true; + server.numClients = LLHTTP_MAX_NUM_CLIENTS; + server.bufferSize = 2000; + + // First cycle initializes internals and publishes server.ident + LLHttpServer(&server); + + LLHttpServerInternalClient_typ* client = &server.internal.pClients[0]; + + LLHttpResponse_typ fbBrew = {}; + fbBrew.ident = server.ident; + fbBrew.method = LLHTTP_METHOD_POST; + strcpy(fbBrew.uri, "/api/brew"); + fbBrew.enable = true; + LLHttpResponse(&fbBrew); + + LLHttpResponse_typ fbAbort = {}; + fbAbort.ident = server.ident; + fbAbort.method = LLHTTP_METHOD_POST; + strcpy(fbAbort.uri, "/api/brew/abort"); + fbAbort.enable = true; + LLHttpResponse(&fbAbort); + + const char* request = + "POST /api/brew?durationS=180&batchSize=50 HTTP/1.1\r\n"\ + "Host: plc.local\r\n"\ + "\r\n"; + + strcpy((char*)client->pReceiveData, request); + client->tcpStream.Internal.FUB.Receive.recvlen = strlen(request); + + int cycles = 0; + while(!(client->connected && fbBrew.newRequest) && ++cycles < 10) { + LLHttpServer(&server); + LLHttpResponse(&fbBrew); + LLHttpResponse(&fbAbort); + } + REQUIRE(client->connected == 1); + REQUIRE(fbBrew.newRequest == 1); + + // The sibling route must not see the request + CHECK(fbAbort.newRequest == 0); + + // The abort route answers 200 immediately, the way a one-cycle route state + // machine would. If it was wrongly dispatched, this 200 is queued ahead of + // the brew route's 202 and is what the client reads + fbAbort.status = 200; + fbAbort.send = true; + LLHttpResponse(&fbAbort); + + // The brew route accepts the command with a 202 + fbBrew.status = 202; + fbBrew.send = true; + LLHttpResponse(&fbBrew); + + LLHttpServer(&server); + + CHECK_THAT((char*)client->pSendData, Catch::Matchers::StartsWith("HTTP/1.1 202 Accepted\r\n")); + +} + TEST_CASE( "Test HTTP Server POST body split across TCP segments", "[LLHttp]") { LLHttpServer_typ server = {}; From b00e0a39d1390a0ca5fe1e8ade37307263776c66 Mon Sep 17 00:00:00 2001 From: Scott Claiborne Date: Thu, 11 Jun 2026 19:29:05 -0700 Subject: [PATCH 3/3] Default response status to 200 and send content-length: 0 when empty A response queued with status 0 (the FB input's initial value) went out as the malformed status line 'HTTP/1.1 0 ', which strict clients such as .NET HttpClient reject outright. Default to 200 OK so applications that never set the status input keep working. Responses with an empty payload now carry an explicit content-length: 0 (except 1xx/204/304, which must not have one) so keep-alive clients do not wait for a body that never comes. Co-Authored-By: Claude Fable 5 --- src/Ar/LLHttp/HttpServer.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Ar/LLHttp/HttpServer.c b/src/Ar/LLHttp/HttpServer.c index f08b3a4..f3aaa01 100644 --- a/src/Ar/LLHttp/HttpServer.c +++ b/src/Ar/LLHttp/HttpServer.c @@ -361,17 +361,18 @@ signed long LLHttpBuildResponse(unsigned long data, unsigned long _response, uns LLHttpServiceResponse_typ* response = _response; unsigned long destLen = 0; UtcDTGetTime_typ utcGetTime = {}; - + unsigned long status = response->status ? response->status : 200; // Default to 200 OK when the application leaves status at 0 + // Clear dest dest[0] = '\0'; - + // Status strcat(dest, "HTTP/"); strcat(dest, "1.1 "); // TODO: User version here - brsitoa(response->status, (UDINT)&temp); + brsitoa(status, (UDINT)&temp); strcat(dest, temp); strcat(dest, " "); - strcat(dest, HttpStatusPhrase(response->status)); + strcat(dest, HttpStatusPhrase(status)); appendNewLine(dest); // Date @@ -406,13 +407,18 @@ signed long LLHttpBuildResponse(unsigned long data, unsigned long _response, uns // Content-Length - // Optional if(response->payloadLength) { strcat(dest, "content-length: "); brsitoa(response->payloadLength, (UDINT)&temp); strcat(dest, temp); appendNewLine(dest); } + else if(status >= 200 && status != 204 && status != 304) { + // An explicit zero keeps keep-alive clients from waiting for a body that + // never comes. 1xx, 204 and 304 must not carry a content-length at all + strcat(dest, "content-length: 0"); + appendNewLine(dest); + } // End Header