Update from upstream#3
Conversation
Removed the incorrect "alg":"RS256" field from RSA JWK test data files. The field is optional per RFC 7517, and the value RS256 (RSASSA-PKCS1-v1_5) was wrong for tests that use RSA-OAEP and RSA-PSS algorithms. Node.js correctly rejects JWK imports when the "alg" field does not match the requested algorithm. The hash mismatch negative test was updated to use an inline JWK with an explicit "alg" field instead of the shared file. This ensures: `test/test262 --binary=node test/webcrypto` pass.
Supports 128, 192, and 256-bit key sizes with generateKey, importKey, and exportKey operations in raw and JWK formats. Also fixed deriveKey to accept 192-bit AES key lengths.
Implemented Ed25519 sign/verify/generateKey/importKey/exportKey Supports raw, PKCS8, SPKI, and JWK (OKP) key formats. Implemented X25519 deriveBits/deriveKey/generateKey/importKey/ exportKey.
Added JWK format support to unwrapKey: decrypted data is parsed as JSON and imported through the shared import path.
Previously, a notice-level log message "js vm init %s: %p" was emitted for each JS engine created during configuration parsing. This message leaked to the compiled-in default error log even when the user explicitly configured logging elsewhere, because config-time logging uses the initial cycle log whose file descriptor is opened before the user's error_log directive takes effect. The log message was used for VM deduplication tests. The tests are updated to verify unique engine identifiers via HTTP and stream responses instead of grepping for log messages. This fixes #1042 issue on Github.
Since 3185ce8 (0.9.7), njs_crypto_module has been registered conditionally in auto/modules under NJS_HAVE_OPENSSL. libnjs.a is built for the nginx module via nginx/config.make with "./configure --no-openssl ...", so libnjs.a's njs_modules[] no longer contains the crypto module. The nginx addon lists (njs_http_js_addon_modules[] and njs_stream_js_addon_modules[], plus the qjs variants) were not updated accordingly, making "import cr from 'crypto'" fail at nginx -t with ENOENT. Added &njs_crypto_module and &qjs_crypto_module to the shared addon list under the existing NJS_HAVE_OPENSSL guard, next to the webcrypto entries. While here, the near-identical addon arrays were factored into shared macros in a new header nginx/ngx_js_modules.h, so adding a future conditional addon needs a single edit instead of four. This closes #1049 issue on Github.
The directive registers a JavaScript handler in the access phase, running after built-in access checkers (allow/deny, auth_basic, auth_request). r.subrequest(), ngx.fetch() and other async operations are supported. The handler defaults to NGX_OK (access granted) on normal completion, matching the behavior of other access phase modules. The r.decline() method allows the handler to return NGX_DECLINED (no opinion), deferring the decision to other access checkers under "satisfy any". The r.return() method can send any HTTP response from the access phase, including 3xx redirects for authentication flows.
Added async methods
- r.readRequestText() as string
- r.readRequestArrayBuffer() as ArrayBuffer
- r.readRequestJSON() as object.
that return Promises resolving with the request body wrapped
as a corresponding type.
The async method parses the client request body as an HTML form and returns a Promise resolving to a form object with get(), getAll(), has(), forEach(), hasFiles() accessors. Supports "application/x-www-form-urlencoded" and "multipart/form-data" content types. File parts are detected but their contents are not exposed. An optional maxKeys option caps the number of fields. File parts are detected but their contents are not exposed. A proper File API with streaming Blob semantics is a significant amount of work and is out of scope.
The destination buffers for the decoded user and password in ngx_js_parse_proxy_url() were a fixed 128 bytes, while the encoded input length was bounded only by the URL length. Since ngx_unescape_uri() writes at most one byte per input byte, raw credentials longer than 128 bytes overflowed the buffer; the length check ran only after the decode. The fix is to size the destination buffer based on the encoded input length. The bug appeared in dea8318 (0.9.4).
This makes the ownership of the request pointer clearer for readers and avoids unnecessary confusion for static analyzers.
Found by Coverity (CID 1692699).
Native frames always have an associated function. Keep the function lookup in the native-frame branch to make the invariant explicit and to avoid confusing static analysis. Found by Coverity (CID 1681309).
Previously, when a js_access handler called r.return(status, body), ngx_http_send_response() sent the response but returned NGX_OK. The access handler then returned NGX_OK to the phase engine, which treated the access check as allowed and continued to the content phase. As a result, a denied request could still reach proxy_pass or another content handler after the response had already been sent. Now the access handler finalizes the request if a response was already sent during js_access execution. This keeps r.return() behavior in js_content unchanged, while making r.return(status, body) terminal in js_access.
Previously, r.internalRedirect() only stored the redirect URI and set ctx->status to NGX_DONE. The stored URI was handled only by the js_content finalization path. As a result, when internalRedirect() was called from js_access, the access handler returned NGX_DONE to the phase engine without starting the redirect, leaving the request unfinished. Now the internal redirect handling is shared by js_access and js_content, so internalRedirect() works consistently in both handlers.
njs_chb_destroy() frees chain nodes through chain->free() and guards against a NULL free callback. njs_chb_drain() and the tail-freeing path of njs_chb_drop() called njs_mp_free() directly, which is wrong for chains initialized with NJS_CHB_CTX_INIT() where chain->free is js_free(), and unsafe for NGX_CHB_CTX_INIT() chains where chain->free is NULL. Both paths now route through chain->free() with the same NULL guard as njs_chb_destroy().
Since fd5e523 (0.9.7), njs has evaluated all call argument expressions before emitting the frame and PUT_ARG instructions. This fixed await expressions in call arguments and preserved argument side effects before callee validation, but left PUT_ARG reading argument values only after later argument expressions had already run. As a result, an earlier argument backed by a mutable variable slot could observe a later mutation of the same slot. For example, f(a, a = 2) passed 2 as both arguments instead of preserving the first value as 1. The same issue applied to later argument effects such as getters. The fix is to preserve affected argument values in temporary registers where appropriate. This fixes #1059 issue on Github.
The method returns names of variables declared with js_var. An optional
string argument filters returned names by prefix.
This lets JavaScript code discover a configuration-defined variable schema
without duplicating every variable name in the script. For example:
js_var $extract_name;
js_var $extract_arguments_workspace;
function handler(r) {
r.jsVarNames("extract_").forEach(function(name) {
r.variables[name] = extract(name.slice(8));
});
}
The same API is available as s.jsVarNames() in stream. Variables created by
js_set or by the core modules are not returned.
The new top-level AGENTS.md (with CLAUDE.md symlinked to it) is the
canonical index for agent and contributor instructions. Detailed
guides live under docs/agent/:
docs/agent/engine-dev.md building, testing, debugging the engine
and the nginx modules.
docs/agent/js-dev.md writing JavaScript for either engine,
with the common nginx API surface and
engine differences.
docs/agent/js-dev-njs.md specifics of the deprecated njs engine
and migration to QuickJS.
Freed the typed array constructor object after reading constructor.name while detecting Float32Array in Buffer.from(). This keeps the exception path from leaking the constructor object. Handled JS_ToCString() failure for constructor.name before comparing the name, avoiding a NULL dereference when the property converts to an exception, such as a Symbol value. Divided the source offset by the element size in qjs_buffer_from() typed-array path so the offset addresses the right element for 2-, 4-, and 8-byte element types (previously the offset was left in byte units while size was already in element units). Added a NULL check on JS_ToCStringLen() in qjs_buffer_encoding(), and moved the JS_FreeCString() call after the JS_ThrowTypeError() so the encoding name remains valid while the error message is formatted. Routed array source errors in qjs_buffer_from_object() through a single fail label so the destination buffer is freed once on every failure path (previously leaked on three of them).
The property definition consumes ownership, so freeing the result object is enough on later error paths. Returned JS_EXCEPTION on property-definition failures instead of the unrelated typed-array lookup result.
Returned the JS_ThrowOutOfMemory() result from ngx_qjs_fetch_response_ctor() on ngx_list_init() failure: previously the call result was discarded and execution fell through. Replaced a stale "ret" check with "rc" after ngx_qjs_headers_fill(), so a non-NGX_OK return is no longer ignored. Added a NULL check after JS_ToCStringLen() on the header key in ngx_qjs_headers_ext_keys(), and replaced a stale "value" check with "item" when testing JS_NewStringLen() for failure.
On 32-bit platforms, where size_t is 32 bits, callers passing
int64_t lengths >= 2^32 to njs_buffer_alloc() had the value
silently truncated before reaching the UINT32_MAX check in
njs_array_buffer_alloc(), so Buffer.from({length: 0x100000000})
returned a zero-sized buffer instead of raising a RangeError.
Widened the size parameter to uint64_t so the value reaches
njs_typed_array_alloc() intact and the "invalid index" range
error fires consistently on 32-bit and 64-bit builds.
Buffer.concat() is protected by the same change: it likewise
reads an int64_t length from JS and forwards it to
njs_buffer_alloc() without an upper bound check of its own.
Code Review — PR #3: Update from upstream+12067/-2322, 65 files. Upstream sync: 🔴 bug
🟡 risk
🔵 nit
❓ q
Confirmed bugfixes ✅
|
Follow-up findings (deeper C analysis)
|
Make CONTRIBUTING.md the canonical source for commit message style and point agent development notes to that guide instead of duplicating the rules.
HTTPS connections established with certificate verification disabled are now excluded from the Fetch keepalive cache. This prevents a later verified request to the same destination from reusing a TLS connection that was created with verification disabled.
Reject control characters and DEL in Fetch Headers values while preserving OWS trimming and obs-text.
Reject empty methods, C0 control bytes, space, and DEL before a fetch request can be serialized. Preserve existing forbidden-method checks, common-method normalization, and valid server-side extension methods.
Reject raw C0 control bytes, space, and DEL in the parsed request target before fetch request serialization. Leave percent-encoded bytes unchanged.
Wrong receiver errors are caused by JS API misuse, not by internal host failures. Report TypeError for TextEncoder, TextDecoder, console, and fs Stats receiver checks.
Preserve exceptions raised by QuickJS conversions and synthesize an out of memory exception only for silent pool allocation failures in ngx_qjs_string() callers. Use JS_GetOpaque() where a class mismatch is a normal miss, avoiding a stale pending TypeError while continuing with non-Headers and non-Request input. Also release the headers init value before throwing for a non-object Headers option.
Report API misuse in Fetch, Request, Response, and Headers as TypeError, and report status bounds violations as RangeError. Keep internal host failures as InternalError and preserve conversion helper exceptions where they already provide the real cause.
Set pending exceptions before returning NJS_ERROR from njs response output paths. Previously sendHeader(), send(), and finish() could fail without an exception being available to the VM.
Report HTTP request API misuse as TypeError and status bounds violations as RangeError. Keep nginx output and request body collection failures as InternalError, since they are host/runtime failures rather than invalid JavaScript arguments.
Reset the variable value validity flag when allocating storage for a stream variable fails. This matches the HTTP variable path and prevents a failed assignment from leaving a half-initialized value marked as valid.
Report stream session API misuse as TypeError, including wrong receiver, unknown or duplicate event handlers, wrong handler phase, and invalid variable access. Report status bounds violations as RangeError and keep stream output pipeline failures as InternalError. Also fix the async send error message spacing.
Report XML API misuse as TypeError and XML parse failures as SyntaxError, aligning njs and QuickJS behavior. The QuickJS message "'this' is not XMLNode or XMLDoc" is changed to "value is not XMLNode or XMLDoc" because qjs_xml_node() is not always validating this.
Align common helper failures with the exception policy used by both engines: invalid numeric conversion is TypeError, and missing external receiver/context for common log helpers is TypeError rather than an internal host failure. The numeric conversion message changes from "is not a number" to "invalid number" to match the QuickJS helper text.
QuickJS property setter and define APIs consume the JSValue argument on both success and failure. This applies to JS_SetProperty(), JS_SetPropertyStr(), JS_SetPropertyUint32(), JS_DefinePropertyValue(), JS_DefinePropertyValueStr(), and JS_DefinePropertyValueUint32(). Do not free values after passing them to these APIs, including failure paths in shared cleanup labels. Target objects and containers are not consumed by these calls and still need normal cleanup unless ownership has been transferred by inserting them into another object. Also fix adjacent cleanup paths that leaked target objects after a setter consumed the value being attached. Reported by R4mbb of KRsecurity.
2 participants
Proposed changes
Update from upstream
Checklist
Before creating a PR, run through this checklist and mark each as complete:
CONTRIBUTINGdocument