fix(server): dispatch response writes through the connection's loop - #30
Merged
Merged
Conversation
added 9 commits
July 28, 2026 00:29
Closes the path-traversal bypass reported in Zane-JS#16. The isPathSafe() guard added in 6402e4b was only applied to sync fs functions and a small subset of async ones. The remaining async, promise, and stream entry points called raw filesystem syscalls with no validation, letting any Zane script read or write outside the working directory (e.g. readFile('../../../etc/passwd'), rm({recursive: true}), createReadStream('/etc/shadow'), open('C:\\Windows\\...')). Functions patched: rmdir/rmdirPromise, rename/renamePromise, copyFile/copyFilePromise/cpSync, open/openPromise, rm/rmPromise, cp/cpPromise, mkdtemp/mkdtempPromise, lutimes/lutimesPromise, opendir/opendirPromise, createReadStream, createWriteStream. Each fix mirrors the existing pattern: throw SecurityError for sync and async-callback variants, p_resolver->Reject for the promise variants. Read/write/close/readv/writev are untouched — they operate on already-opened file descriptors, not paths. Includes test/test_fs_path_safety.js which exercises every patched function with traversal sequences, absolute paths, drive letters, and Windows reserved device names, asserting that each one throws SecurityError. A negative control verifies that relative paths without traversal still work as expected.
Closes Zane-JS#17. The zlib, brotli, and zstd APIs had no default cap on decompression output and accepted an unbounded chunkSize. Two distinct attack vectors: 1. Decompression bomb: a 1 MB compressed payload could expand to GBs because maxOutputLength=0 silently meant 'no limit'. The check was so the gate never fired. 2. Pre-allocation OOM: {chunkSize: 2_000_000_000} caused an immediate ~2 GB allocation before any work began. Fixes applied uniformly in parseZlibOptions / parseBrotliOptions / parseZstdOptions: - chunkSize is clamped to [kMinChunkSize=64, kMaxChunkSize=1<<20] - maxOutputLength defaults to kDefaultMaxOutput=512 MB when the user doesn't set it The 512 MB default is generous enough for any legitimate in-process use; raise it only if you actually need more (would warrant a louder log entry). The existing 'maxOutputLength exceeded' error path is unchanged — only the trigger is now reachable by default.
Bump libs/http to commit 6895c08 which rejects requests with Transfer-Encoding headers. The previous commit (32824ea) silently dropped the body of any chunked request and marked it COMPLETE, giving attackers a request-smuggling gate when Zane is behind a reverse proxy that decodes chunked for it. The submodule bump is the parent-side mirror of Zane-HTTPParser PR Zane-JS#2 and closes Zane-JS#10 from the consumer end.
Bumps libs/http to commit 2d33964 which adds the HTTP method whitelist. The submodule-side PR is Zane-JS#2 at Zane-JS/Zane-HTTPParser (combined with the Transfer-Encoding fix from Zane-JS#10). Closes Zane-JS#12 from the consumer end.
Closes Zane-JS#8 and Zane-JS#13. - Issue Zane-JS#13: Zane.serve() defaulted to hostname='0.0.0.0', which silently bound the server on every interface. A dev who forgot to set hostname in a container or dev VM would expose an unauthenticated HTTP handler on their LAN/internet. Changed default to '127.0.0.1'. Apps that intentionally want public interfaces still pass an explicit hostname. - Issue Zane-JS#8 (slowloris): no idle timeout meant an attacker could open a connection, send one byte at a time, and hold the socket open forever — exhausting FDs and memory across thousands of half-open connections. Trantor's kickoffIdleConnections(30) walks the live connection set every 30s and closes anything that has been idle. 30s is generous enough for normal HTTP including chunked uploads on slow links, and tight enough that a slowloris attacker can't keep many sockets alive at once.
Closes Zane-JS#18 (correctness fix; Zane-JS#16's missing-check side has its own PR). The existing isPathSafe() scanned the whole path string for any pair of consecutive dots. That refused legitimate filenames like 'file..txt', 'v1..0-release', 'a...b', or '..test' — even though none of those are directory escapes. The fix splits the path on '/' and '\\' and rejects only a component that is *exactly* '..'. That's the only sequence that genuinely escapes the directory. Examples after the fix: isPathSafe('file..txt') -> true (was false) isPathSafe('a...b') -> true (was false) isPathSafe('v1..0-release') -> true (was false) isPathSafe('..') -> false (still) isPathSafe('foo/../bar') -> false (still) The remaining problem Zane-JS#18 flags (no symlink resolution, no absolute path support within a sandbox) is a larger design change — root-jail via fs::canonical() — that needs its own scope. Filed under note for followup; this commit just fixes the false-positive rejection which is the high-impact, low-risk half. Verified with /tmp/pi-cpp-test/test_isPathSafe.cpp — 17 cases all pass with the new logic; 6 of them wrongly rejected by the OLD logic (verified by re-running with the old implementation).
Closes Zane-JS#19 bug 1. `path.posix.basename("")` invokes std::string::back() on an empty string, which is UB per C++ §24.4.5.2. In practice it reads out-of-bounds heap memory — a one-call info leak or crash. The fix is a one-line guard: skip the trailing-slash check when p is empty. Same defensive treatment applied to extnamePosix for symmetry (the existing check there was already correct, but the empty-string case was implicit). The host variants (basename, extname) use std::filesystem::path which handles empty paths correctly, so only the Posix string-manipulation ones were affected. Other Posix functions (dirname, join, normalize, resolve) already guard against empty input.
…parsing Closes Zane-JS#19 bug 3 (and incidentally the duplicate-parsing half of bug 3). parseZlibOptions / parseBrotliOptions / parseZstdOptions parsed the "dictionary" key twice. The first block (zlib.cpp line 130) only handled Uint8Array; the second block (line 148) handled both Uint8Array and ArrayBuffer. The second block overwrote the first, making the first dead code. The brotli and zstd variants had only the second block, so the duplication was a zlib-only indirection. Each retained block now also calls `view->Buffer()->IsDetached()` / `ab->IsDetached()` before reading the backing store pointer. If a caller passes a buffer that has been detached between the V8 call and the start of compression/decompression, the BackingStore data pointer is stale and the read is a use-after-free. With the guard we throw a TypeError instead. The copy into `std::vector<uint8_t>` happens immediately after the guard, so the async thread receives a private copy and the detached state is no longer raced.
Closes Zane-JS#14. The response callback captured the raw TcpConnection (which is actually a shared_ptr underneath) and called p_conn->send() directly from the V8 thread that runs the JS fetch handler. Trantor's TcpConnection requires its owning event loop thread to be the only writer; calling send() from the V8 thread races the write buffer and lifecycle. The fix captures the connection as a shared_ptr (which it already is per Trantor's callback signature) and dispatches each send through p_conn->getLoop()->runInLoop(). The data is moved into the lambda so the response bytes stay alive until the loop thread picks them up. The connected() check before send() prevents a race where the connection closed between the runInLoop enqueue and the actual schedule point.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #14. See commit message for details.