tests: add HTTP server unit tests and fix several identified bugs #1471

HMarcien · 2026-06-29T01:03:17Z

HMarcien commented

2026-06-29 01:03:17 +00:00

Add new unit tests to improve HTTP server coverage.
Fix a security bug where URL-encoded paths allowed directory traversal.
Add proper support for HTTP HEAD requests with Content-Length headers.
Refactored the handler structure to route requests based on HTTP methods.
Automatically reject unsupported methods with a 405 status.
Fix static mux parsing to allow chaining multiple rules.

- Add new unit tests to improve HTTP server coverage. - Fix a security bug where URL-encoded paths allowed directory traversal. - Add proper support for HTTP HEAD requests with Content-Length headers. - Refactored the handler structure to route requests based on HTTP methods. - Automatically reject unsupported methods with a 405 status. - Fix static mux parsing to allow chaining multiple rules.

HMarcien added 3 commits

2026-06-29 01:03:17 +00:00

fix(os/socket): pass exact sockaddr size to fix BSD(FreeBSD) crash 2379cf6a57

FreeBSD throw an EINVAL error if the socket address size is too large (128 bytes from the union instead of 16 for IPv4). This commit updates the ffi to calculate and send the exact size based on the address family, preventing the crash.

Fix TCP deadlock in HTTP client for large data transfers 12f35e7d37

The "large data blob" test was crashing due to a TCP deadlock.

The HTTP client was trying to write the entire request body before reading the server's response. With a full-duplex server (using `io-copy!`), this fills up the OS TCP buffers and blocks both sides.

Fix: Use `spawn` to write the request in a background thread. The main thread can now read the response immediately, avoiding the deadlock.

tests: add HTTP server unit tests and fix several identified bugs 4ddb36e700

- Add new unit tests to improve HTTP server coverage.
- Fix a security bug where URL-encoded paths allowed directory traversal.
- Add proper support for HTTP HEAD requests with Content-Length headers.
- Refactored the handler structure to route requests based on HTTP methods.
- Automatically reject unsupported methods with a 405 status.
- Fix static mux parsing to allow chaining multiple rules.

HMarcien requested review from vyzo

2026-06-29 01:05:19 +00:00

fare reviewed

2026-06-30 09:13:40 +00:00

src/std/net/http/client/request.ss

					
				@ -65,1 +68,4 @@

				         (catch (e)

				           (void)))))

				     ;; read request head (executes concurrently with the writer thread)

				     (let* (((values status status-line)

Don't we want to thread-join! or otherwise catch any subthread error in the main thread (after maybe using ##thread-end-with-uncaught-exception! in the internal thread)?

My initial intuition, was that we could just let the Gambit runtime silently swallow the EPIPE without needing to join.

However, if we do a synchronous thread-join!, I think we will recreate the TCP deadlock: if the server stops reading (e.g. for a 413) but keeps the socket open, the writer thread will block on I/O and the main thread will deadlock on the join.

Because of this, I will go with your second suggestion and use ##thread-end-with-uncaught-exception! to surface real bugs without blocking the main thread.

My initial intuition, was that we could just let the Gambit runtime silently swallow the EPIPE without needing to join. However, if we do a synchronous thread-join!, I think we will recreate the TCP deadlock: if the server stops reading (e.g. for a 413) but keeps the socket open, the writer thread will block on I/O and the main thread will deadlock on the join. Because of this, I will go with your second suggestion and use ##thread-end-with-uncaught-exception! to surface real bugs without blocking the main thread.

is this really necessary? We do want to join this thread somewhere indeed but i am not convinced the write should happen asynchronously at all.

Why do we need it? We have to write the request anyway before we read the response.

The dangling thread can become problematic too when we upgrade in things like websockets as well.

is this really necessary? We do want to join this thread somewhere indeed but i am not convinced the write should happen asynchronously at all. Why do we need it? We have to write the request anyway before we read the response. The dangling thread can become problematic too when we upgrade in things like websockets as well.

src/std/net/http/http-test.ss

					
				@ -12,2 +15,4 @@

				  (make-temporary-file-name "log."))

				(def tmp-file

				  (string-append (make-temporary-file-name "file") ".js"))

make-temporary-file-name now supports a suffix: argument.

👍 1

HMarcien marked this conversation as resolved

src/std/net/http/server/handlers/directory.ss

					
				@ -50,8 +51,13 @@

				          (serve-dir self.path)

I'm not sure what serve-dir does, but shouldn't that only be attempted after the path has been found safe?

I'm not sure what serve-dir does, but shouldn't that only be attempted *after* the path has been found safe?

serve-dir simply looks for an index.html inside the base directory. It is only called when the requested URL exactly matches the root path, meaning there is no user-provided subpath to sanitize here. The directory traversal check (safe-target?) is applied in the other branch, where a subpath is actually requested.

src/std/net/http/server/handlers/directory.ss

					
				@ -55,0 +57,4 @@

				                       (let ((root (path-normalize self.path)))

				                         (let ((root/ (if (string-suffix? "/" root) root (string-append root "/"))))

				                           (string-prefix? root/ target))))))

				            (if (and safe-target? (file-exists? target))

Wait, is the root a path in the URL or in the filesystem?
Is target absolute or relative? If relative, to what?

Wait, is the root a path in the URL or in the filesystem? Is target absolute or relative? If relative, to what?

The root (from self.path) is a path in the filesystem. target is an absolute filesystem path because path-normalize resolves the URL subpath relative to the physical root. Then, safe-target? simply checks that this final absolute path still starts with the root directory to prevent any ../ escapes.

src/std/net/http/server/handlers/file.ss

					
				@ -35,64 +36,75 @@

				  => :pair

				  (cons "Content-Type" (mime-file-type path)))

				(defsyntax-case with-requested-file-info ()

You might be better off using the new with-identifiers or maybe even the new with-id (in a syntax-rules)

👍 1

HMarcien marked this conversation as resolved

HMarcien added 1 commit

2026-06-30 17:57:07 +00:00

Fix CR c953247771

HMarcien requested review from fare

2026-06-30 18:02:16 +00:00

HMarcien added 1 commit

2026-06-30 18:07:07 +00:00

Fix typo and missing import ea8e01c913

vyzo requested changes

2026-07-02 23:20:18 +00:00

vyzo left a comment

left some comments to address.

src/std/net/http/client/request.ss

					
				@ -65,0 +66,4 @@

				         (write-newline! writer)

				         (__request-write-body! writer body)

				         (writer.flush)

				         (catch (OSError? e)

i think the pipe errors should propagate and not cause a timeout.

src/std/net/http/server/handlers/directory.ss

					
				@ -12,3 +13,3 @@

				        new-directory-listing-handler)

				(defstruct directory-handler

				(defstruct (directory-handler HttpMethodHandler)

we have a funky case of kebab eating camels here.

src/std/net/http/server/handlers/directory.ss

					
				@ -55,0 +55,4 @@

				                 (safe-target?

				                  (and target

				                       (let ((root (path-normalize self.path)))

				                         (let ((root/ (if (string-suffix? "/" root) root (string-append root "/"))))

lets precompute this and store it on the handler, no need to do it for every request.

src/std/net/http/server/handlers/file.ss

					
				@ -15,3 +16,3 @@

				(def cache-threshold 32768)

				(defstruct file-handler

				(defstruct (file-handler HttpMethodHandler)

camel kebab :)

src/std/net/http/server/handlers/http-method.ss

					
				@ -0,0 +7,4 @@

				(export #t)

				(defstruct HttpMethodHandler ())

i think we need to kebab case this.

src/std/net/http/server/handlers/http-method.ss

					
				@ -0,0 +34,4 @@

				(implement RequestHandler HttpMethodHandler

				  (handle-request!

				   (lambda (self (req : Request) (res : ResponseHandler))

				     (using (self : MethodHandler)

this will cast and allocate a new instance for every request.

it is preferable to have a constructor that precasts self to this and stores it in the object.

this will cast and allocate a new instance for every request. it is preferable to have a constructor that precasts self to this and stores it in the object.

src/std/net/http/server/interface.ss

					
				@ -74,0 +73,4 @@

				;; MethodHandler extends RequestHandler to provide HTTP verb-specific routing.

				;; Concrete handlers should inherit from HttpMethodHandler (in http-method.ss)

				;; to automatically dispatch requests and fallback to 405 Method Not Allowed.

				(interface (MethodHandler RequestHandler)

nice

src/std/os/socket.ss

					
				@ -198,2 +198,3 @@

				                       (addr :- sockaddr))

				  "bind(___arg1, ___arg2, (socklen_t)___U8VECTORSIZE(___ARG2))")

				  "bind(___arg1, ___arg2,

				        (___arg2->sa_family == AF_INET)  ? sizeof(struct sockaddr_in) :

is this actually necessary? caller preallocates. no?

also i don't think bind cares at all if you give it bigger.

is this actually necessary? caller preallocates. no? also i don't think bind cares at all if you give it bigger.

src/std/os/socket.ss

					
				@ -202,2 +206,3 @@

				                          (addr :- sockaddr))

				  "connect(___arg1, ___arg2, (socklen_t)___U8VECTORSIZE(___ARG2))")

				  "connect(___arg1, ___arg2,

				           (___arg2->sa_family == AF_INET)  ? sizeof(struct sockaddr_in) :

same here

src/std/os/socket.ss

					
				@ -239,2 +247,3 @@

				                         (dest    :- sockaddr))

				  "sendto(___arg1, (void*)(___arg2 + ___arg3), ___arg4, ___arg5, ___arg6, (socklen_t)___U8VECTORSIZE(___ARG6))")

				  "sendto(___arg1, (void*)(___arg2 + ___arg3), ___arg4, ___arg5, ___arg6,

				          (___arg6->sa_family == AF_INET)  ? sizeof(struct sockaddr_in) :

and here.

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin +refs/pull/1471/head:hmarcien/net-http-tests

git switch hmarcien/net-http-tests

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch v0.19-staging

git merge --no-ff hmarcien/net-http-tests

git switch hmarcien/net-http-tests

git rebase v0.19-staging

git switch v0.19-staging

git merge --ff-only hmarcien/net-http-tests

git switch hmarcien/net-http-tests

git rebase v0.19-staging

git switch v0.19-staging

git merge --no-ff hmarcien/net-http-tests

git switch v0.19-staging

git merge --squash hmarcien/net-http-tests