Repository navigation
fix(gateway): reassemble WebChat frames before JSON parse - #144
cursor[bot] wants to merge 1 commit into
Conversation
libwebsockets spills a frame into WS_RX_SPILL chunks and reports the final fragment only when the packet is complete. Parsing each chunk dropped a WS_TEXT_MAX chat message once quotes or backslashes expanded the JSON past that spill. Co-authored-by: esadrianno <esadrianno@gmail.com>
There was a problem hiding this comment.
Summary
ShellClaw is a C99 edge agent. WebChat rides a libwebsockets gateway, and this PR reassembles each socket message before cJSON_Parse, so a chat body that fits in the 32 KiB text cap still reaches the agent when JSON escaping pushes the wire form past the 32 KiB + 64 byte receive spill. The accumulation, the per-connection buffer, and the decoded-length cap do that. The required CI job is red, so this should not merge yet. GitHub rejects a changes request from the PR author, so this is filed as a comment. The cppcheck failure is still a merge blocker.
Must Fix
src/gateway/ws_rx.c:39 fails make static. cppcheck 2.13 reports variableScope on ncap. That step is first in the branch-protection job static, test, release, and the rest of the job never started on 3cc4841: https://github.com/asap-protocol/shellclaw/actions/runs/43010772605/job/130489517406
Declare ncap inside if (acc->cap < need). The same cppcheck command is clean after that move. grown on the line above is not reported.
Should Fix
-
tests/test_ws_rx.c:118-122does not lock the skip path."rest"fails JSON parse even when the final spill is parsed on its own. Feed a final spill of{"type":"message","text":"poison"}and require it to be dropped, then require the followinghimessage. Also cross the cap after a short prefix sows_rx_too_bigruns withacc->len > 0. A throwaway driver linked tows_rx.oon this commit dropped the poison text and returnedhi. The test should keep that result. -
src/gateway/http_lws.c:471cannot close an oversize frame.ws_conn_rx_feedreturns 0 for a truncated message, a rejected payload, an unknown conn id, and "need more data". This callback then returns 0, so libwebsockets keeps reading. Memory is bounded (skipfrees the buffer). The onelws_servicethread still drains the rest of the frame, and dashboard HTTP shares that thread. Return a distinct oversize status andreturn -1fromLWS_CALLBACK_RECEIVE. Leave-1fromws_rx_acc_feedas the OOM result:skipalready covers the rest of that message, and a failedreallocshould leave the socket up. The default bind is127.0.0.1and the upgrade checks the bearer token, so this is a local authenticated client. Discord gateway RX skips the same way and leaves the socket open. WebChat is this process's only service thread, which is the reason to close here.
Once make static is green, those two can land in this PR or a follow-up.
Nice to Have
ws.h:29ties "one byte past aWS_TEXT_MAXstring of backslashes" toWS_RX_JSON_MAX. That extra byte is rejected by the decoded-length check atws_rx.c:80. Backslashes expand 2x (wire size 65564). The 6x bound is\u00XX: 26 + 32768 * 6 + 2 = 196636, andWS_RX_JSON_MAXis 196672.cJSON_Parseaccepts trailing bytes.{"type":"message","text":"hi"}TRAILreturnshi.cJSON_ParseWithOpts(acc->buf, NULL, 1)rejects that. The previous per-chunk parser accepted trailing bytes too.- A binary frame whose payload is chat JSON is parsed as text.
lws_frame_is_binary(wsi)can reject it. The previous parser did the same. ws_conn_rx_feedholdsg_mutexacrossreallocandcJSON_Parse(ws.c:462-469). The buffer is at most about 192 KiB. Copy it out and parse after unlock if send or pop latency shows up.- The oversize log at
ws_rx.c:109prints the cap and omits the byte count that tripped it. - Two cases that already behave on this commit and are untested: a complete JSON object with
is_final == 0returns nothing, and a partial frame on conn 1 does not mix with a finished message on conn 2.
Positive Highlights
- The partial frame lives on
g_conns[].rxbecauselws_set_wsi_useralready stores the conn id in the only per-wsi slot. Register, unregister, andws_cleanupall free it. - The size check runs before the add, and the allocation includes the trailing NUL. That is the off-by-one Discord RX already hit.
- Nothing is parsed until
lws_is_final_fragmentis true, and the decoded text still has to fit inWS_TEXT_MAX.http.cstatic-asserts the protocol spill againstWS_RX_SPILL. test_escaped_max_rejoins_across_spillsround-trips aWS_TEXT_MAXstring of backslashes split on spill boundaries, and the CHANGELOG Unreleased note matches that.test_wsandtest_ws_rxpassed locally. This environment has no libwebsockets headers, sotest_gateway_httpwas not run. CI did not reach its test steps.
Sent by Cursor Automation: Code review
| { | ||
| size_t need; | ||
| char *grown; | ||
| size_t ncap; |
There was a problem hiding this comment.
Must fix: make static fails on this declaration, and that step is first in the required static, test, release job, so tests, ASan, and the release build never ran on 3cc4841.
cppcheck 2.13 reports [variableScope]: ncap is only used inside if (acc->cap < need). Declare it in that block. The same cppcheck command is clean after the move. grown on the line above is not reported.
https://github.com/asap-protocol/shellclaw/actions/runs/43010772605/job/130489517406
| memset(huge, '{', WS_RX_JSON_MAX + 1); | ||
| ASSERT(ws_rx_acc_feed(&acc, huge, WS_RX_JSON_MAX + 1, 0, &got) == 0); | ||
| ASSERT(got == NULL); | ||
| ASSERT(ws_rx_acc_feed(&acc, "rest", 4, 1, &got) == 0); |
There was a problem hiding this comment.
Should fix: "rest" is not JSON, so this stays green if the final spill is parsed on its own. The skip path needs a final spill that is itself a chat message, {"type":"message","text":"poison"}, and that call must return 0. The next {"type":"message","text":"hi"} must still return hi.
Also cross the cap after a short prefix, so ws_rx_too_big runs with acc->len > 0. This test's first chunk is already larger than WS_RX_JSON_MAX, and the buffer is never allocated.
A throwaway driver linked to ws_rx.o on this commit dropped the poison text and then returned hi, including the prefix-then-cross case. The implementation matches the test name. This assertion does not.
| * partial frame lives on the connection table instead. */ | ||
| if (conn_id <= 0) | ||
| break; | ||
| fed = ws_conn_rx_feed(conn_id, in, len, lws_is_final_fragment(wsi), &text); |
There was a problem hiding this comment.
Should fix: ws_conn_rx_feed returns 0 when it needs another spill, when the frame crosses WS_RX_JSON_MAX, when the JSON is rejected, and when conn_id is unknown. This callback then returns 0, so libwebsockets keeps the socket.
skip frees the heap buffer and drops the rest of that message, so memory stays bounded. http_thread_fn is the only lws_service loop, and it still unmasks every later spill. One authenticated client can send a multi-gigabyte frame and stall dashboard HTTP plus every other WebChat socket for the duration of that read.
Return a distinct oversize status and return -1 here so libwebsockets closes the wsi. Leave -1 from ws_rx_acc_feed as OOM: skip already covers the remainder, and a failed realloc should leave the chat socket up.
The default bind is 127.0.0.1 and the upgrade already checks the bearer token. Discord gateway RX uses the same skip-until-final pattern and leaves its socket open. This callback is the process's only service thread, which is the reason to close.


Summary
WebChat parsed each libwebsockets receive chunk as its own JSON message. A chat text that still fits in the 32 KiB cap can exceed the 32 KiB + 64 byte spill once quotes or backslashes are escaped. The agent never saw that message. The page had already echoed it as sent.
Root cause
LWS_CALLBACK_RECEIVEcalledcJSON_Parseon the current spill. In libwebsockets 4.3,lws_is_final_fragment()stays false until the packet length is zero. Discord already reassembles gateway frames that way. WebChat did not.lws_set_wsi_user()stores the connection id in the only per-wsi slot and frees protocol session data, so the partial frame is kept on the connection table.Fix
Accumulate chunks until the final fragment, then parse. Drop a body over the JSON expansion cap (
WS_TEXT_MAX * 6 + 64) and ignore the rest of that message so the next one is not glued on.Validation
CI=true make test_ws test_ws_rxpassed, including aWS_TEXT_MAXstring of backslashes split onWS_RX_SPILLboundaries.src/gateway/http.c,http_lws.c,ws.c, andws_rx.ccompiled with-Werror.