Skip to content

fix(gateway): reassemble WebChat frames before JSON parse - #144

Open
cursor[bot] wants to merge 1 commit into
mainfrom
fix/webchat-rx-reassembly
Open

cursor[bot] wants to merge 1 commit into
mainfrom
fix/webchat-rx-reassembly

Conversation

@cursor

@cursor cursor Bot commented Nov 11, 2026

Copy link
Copy Markdown

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_RECEIVE called cJSON_Parse on 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_rx passed, including a WS_TEXT_MAX string of backslashes split on WS_RX_SPILL boundaries.
  • The same tests passed under gcc AddressSanitizer and UBSan.
  • src/gateway/http.c, http_lws.c, ws.c, and ws_rx.c compiled with -Werror.
Open in Web View Automation 

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>
@cursor
cursor Bot requested a review from adriannoes as a code owner November 11, 2026 11:17

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. tests/test_ws_rx.c:118-122 does 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 following hi message. Also cross the cap after a short prefix so ws_rx_too_big runs with acc->len > 0. A throwaway driver linked to ws_rx.o on this commit dropped the poison text and returned hi. The test should keep that result.

  2. src/gateway/http_lws.c:471 cannot close an oversize frame. ws_conn_rx_feed returns 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 (skip frees the buffer). The one lws_service thread still drains the rest of the frame, and dashboard HTTP shares that thread. Return a distinct oversize status and return -1 from LWS_CALLBACK_RECEIVE. Leave -1 from ws_rx_acc_feed as the OOM result: skip already covers the rest of that message, and a failed realloc should leave the socket up. The default bind is 127.0.0.1 and 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:29 ties "one byte past a WS_TEXT_MAX string of backslashes" to WS_RX_JSON_MAX. That extra byte is rejected by the decoded-length check at ws_rx.c:80. Backslashes expand 2x (wire size 65564). The 6x bound is \u00XX: 26 + 32768 * 6 + 2 = 196636, and WS_RX_JSON_MAX is 196672.
  • cJSON_Parse accepts trailing bytes. {"type":"message","text":"hi"}TRAIL returns hi. 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_feed holds g_mutex across realloc and cJSON_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:109 prints 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 == 0 returns 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[].rx because lws_set_wsi_user already stores the conn id in the only per-wsi slot. Register, unregister, and ws_cleanup all 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_fragment is true, and the decoded text still has to fit in WS_TEXT_MAX. http.c static-asserts the protocol spill against WS_RX_SPILL.
  • test_escaped_max_rejoins_across_spills round-trips a WS_TEXT_MAX string of backslashes split on spill boundaries, and the CHANGELOG Unreleased note matches that. test_ws and test_ws_rx passed locally. This environment has no libwebsockets headers, so test_gateway_http was not run. CI did not reach its test steps.
Open in Web View Automation 

Sent by Cursor Automation: Code review

Comment thread src/gateway/ws_rx.c
{
size_t need;
char *grown;
size_t ncap;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread tests/test_ws_rx.c
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);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/gateway/http_lws.c
* 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);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant