restconf: wait for complete HTTP header before parsing by hoxnox · Pull Request #676 · clicon/clixon

hoxnox · 2026-05-25T08:19:02Z

Large HTTP headers (e.g. JWT Bearer tokens >2KB) may be fragmented across multiple TCP segments. The current code calls clixon_http1_parse_string() on every read and, if parsing fails, uses SSL_pending() / clixon_event_poll() to decide whether more data is coming. This check is racy: when the next TCP segment hasn't yet arrived in the kernel buffer (common over WAN links with

1ms inter-packet delay), poll returns 0 and a valid request is rejected
with 400 Bad Request / malformed-message.

Fix this by checking for the HTTP header terminator (\r\n\r\n) before attempting to parse. If the terminator is not present, return to the event loop; the next readable event will append the remaining bytes to sd_inbuf and parsing will succeed on the complete header.

Fixes #667

olofhagsand

Logic looks correct, had a minor style comment, a proposal for a timer.
Also, can one make a test for this case, ie that exercises the code?
It would mean sending a partial message, waiting, and then sending the rest, which may be non-trivial?

olofhagsand · 2026-05-25T11:01:47Z

+         * to reject with 400. Need >= 8 bytes (longest is "OPTIONS ") to
+         * classify; until then, assume incomplete and wait. */
+        {
+            const char *inbuf = cbuf_get(sd->sd_inbuf);


Clixon convention requires all local variables declared at the top of the function, not in an inner block mid-function. The
inner {} block was added specifically to work around this. These should be hoisted to the function's variable declarations.
See CONTRIBUTING.md

olofhagsand · 2026-05-25T11:04:16Z

+                strncmp(inbuf, "PATCH ",   6) == 0 ||
+                strncmp(inbuf, "DELETE ",  7) == 0;
+            if (looks_http && strstr(inbuf, "\r\n\r\n") == NULL){
+                goto ok;


There should probably be a timer to avoid unbound waits on this session?

Large HTTP headers (e.g. JWT Bearer tokens >2KB) may be fragmented across multiple TCP segments. The current code calls clixon_http1_parse_string() on every read and, if parsing fails, uses SSL_pending() / clixon_event_poll() to decide whether more data is coming. This check is racy: when the next TCP segment hasn't yet arrived in the kernel buffer (common over WAN links with >1ms inter-packet delay), poll returns 0 and a valid request is rejected with 400 Bad Request / malformed-message. Fix this by checking for the HTTP header terminator (\r\n\r\n) before attempting to parse. If the terminator is not present, return to the event loop; the next readable event will append the remaining bytes to sd_inbuf and parsing will succeed on the complete header. To preserve immediate rejection of non-HTTP input (e.g. TLS handshake bytes sent to an HTTP port — test_restconf.sh test 15), only delay parsing when the request line begins with one of the RESTCONF-accepted methods: OPTIONS, HEAD, GET, POST, PUT, PATCH, DELETE. Anything else falls through to the parser for a proper 400. Fixes clicon#667

The partial-header wait introduced for clicon#667 lets a peer hold the socket open by sending the start of a request and then stalling. Add a one-shot timer (RESTCONF_HEADER_TIMEOUT_S = 10s) registered when we first defer parsing, cancelled when the header parses successfully or the connection is closed. On expiry, the connection is closed via restconf_close_ssl_socket(). The timer state is tracked by rc_header_timer on restconf_conn; the unreg helper is idempotent so close paths can call it unconditionally.

Exercises the HTTP/1 partial-header path that the previous commits introduced handling for: 1. Split-write of a >2KB request (mimicking a JWT in an Authorization header) across two writes with a sleep in between - must return 200, not 400 malformed-message. 2. TLS handshake bytes sent to an HTTP port - must still be rejected with 400 immediately, not stall waiting for "\r\n\r\n". 3. A peer that sends a plausible request line but never completes the header - must be closed by the header timeout (between 2s and 15s given RESTCONF_HEADER_TIMEOUT_S = 10s). Uses nc(1) and shell-level write+sleep to deterministically produce the fragmented delivery; curl writes the whole request in one go and cannot trigger the race directly.

hoxnox · 2026-05-27T07:45:56Z

I've added test and resolved all remarks.

olofhagsand · 2026-05-27T09:20:13Z

There is a problem with nc vs netcat on linux vs musl/alpine vs freebsd, I had those problems before, see test/lib.sh

Per maintainer feedback: nc / netcat / ncat / busybox-nc differ across distros and BSDs on the -q / -N / -w flags needed for "write then close" semantics (lib.sh already documents this with an unused netcat wrapper). Switch the partial-header test to bash /dev/tcp redirections, which is already a dependency of clixon's test framework (wait_grpc in lib.sh).

hoxnox · 2026-05-27T10:26:36Z

corrected

There is a problem with nc vs netcat on linux vs musl/alpine vs freebsd, I had those problems before, see test/lib.sh

bash /dev/tcp gave empty reads on Alpine CI even though the same port worked under curl. Move to expect/Tcl, which is already a clixon test dependency (see test_pagination_expect.exp) and is installed in docker/test/Dockerfile.native. Tcl's built-in 'socket' command gives us deterministic split-write semantics that no nc/netcat variant offers portably. Verified locally against a Python HTTP server that the split-write + read response loop captures the response correctly.

hoxnox · 2026-05-28T05:38:28Z

corrected tests

On Alpine CI Test 10 reported elapsed=s (empty), even though the total script wallclock matched the 10s server header-timeout. The Tcl puts that emits the elapsed seconds runs after close $sock; on a non- blocking socket whose peer just closed, Tcl's close can raise, which terminates the script before puts reaches stdout. The captured $(...) output is then empty and the test reports failure. Time the wait in bash with date +%s and let the Tcl side just block on eof. catch around socket / read / close keeps any Tcl-level error from leaking through the heredoc exit status.

hoxnox · 2026-05-28T10:12:11Z

test timing adjusted

olofhagsand · 2026-05-28T12:47:00Z

Hey, I understand the test might be difficult to do, we could pass it if you do some ad-hoc testing for this specific thing.

hoxnox · 2026-05-28T12:49:37Z

Hey, I understand the test might be difficult to do, we could pass it if you do some ad-hoc testing for this specific thing.

All tests is green. Everything is ready to merge.

olofhagsand · 2026-05-28T12:52:58Z

My bad, I was looking at an earlier run

hoxnox · 2026-06-03T06:42:00Z

Is there anything else I need to do for that PR?

olofhagsand · 2026-06-03T18:11:52Z

I will look at it soon

hoxnox force-pushed the fix-restconf-partial-header-667 branch from 190d708 to eb57d10 Compare May 25, 2026 09:42

olofhagsand reviewed May 25, 2026

View reviewed changes

hoxnox added 3 commits May 25, 2026 19:18

hoxnox force-pushed the fix-restconf-partial-header-667 branch from eb57d10 to c133150 Compare May 25, 2026 16:27

Uh oh!

Conversation

hoxnox commented May 25, 2026

Uh oh!

olofhagsand left a comment

Choose a reason for hiding this comment

Uh oh!

olofhagsand May 25, 2026

Choose a reason for hiding this comment

Uh oh!

olofhagsand May 25, 2026

Choose a reason for hiding this comment

Uh oh!

hoxnox commented May 27, 2026

Uh oh!

olofhagsand commented May 27, 2026

Uh oh!

hoxnox commented May 27, 2026

Uh oh!

hoxnox commented May 28, 2026

Uh oh!

hoxnox commented May 28, 2026

Uh oh!

olofhagsand commented May 28, 2026

Uh oh!

hoxnox commented May 28, 2026

Uh oh!

olofhagsand commented May 28, 2026

Uh oh!

hoxnox commented Jun 3, 2026

Uh oh!

olofhagsand commented Jun 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants