From: Dmitry Volyntsev Date: Tue, 11 Aug 2026 01:14:38 +0000 (-0700) Subject: HTTP: fail closed on js_access body completion errors X-Git-Tag: 1.0.1~3 X-Git-Url: https://git.kaiwu.me/?a=commitdiff_plain;h=6898f7c9f844ca30af2aed9e8b1fe10f393644a5;p=njs.git HTTP: fail closed on js_access body completion errors Previously, an exception in an asynchronous readRequest*() continuation could leave the js_access decision unset, allowing nginx to continue processing the request as though the access check had succeeded. An unhandled rejection could also be missed by the body-read completion path or hidden by a pending internal redirect. Now body-read completion errors set the access status to 500, and access finalization checks execution errors before an uncommitted access decision or internal redirect. Responses already sent by the handler remain terminal. Reported by Ta Duc Thien. --- diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c index e5c2b655..1766d1a0 100644 --- a/nginx/ngx_http_js_module.c +++ b/nginx/ngx_http_js_module.c @@ -1560,12 +1560,6 @@ ngx_http_js_access_handler(ngx_http_request_t *r) ctx->in_progress = 0; - if (ctx->rejected_promises != NULL - && ctx->rejected_promises->items > 0) - { - return NGX_HTTP_INTERNAL_SERVER_ERROR; - } - return ngx_http_js_access_finalize(r, ctx); } @@ -1629,14 +1623,22 @@ ngx_http_js_access_finalize(ngx_http_request_t *r, ngx_http_js_ctx_t *ctx) { ngx_int_t rc; - if (ctx->redirect_uri.len) { - rc = ngx_http_js_internal_redirect(r, ctx); - ngx_http_finalize_request(r, rc); + if (r->header_sent) { + ngx_http_finalize_request(r, NGX_OK); return NGX_DONE; } - if (r->header_sent) { - ngx_http_finalize_request(r, NGX_OK); + if (ctx->status == NGX_HTTP_INTERNAL_SERVER_ERROR + || (ctx->rejected_promises != NULL + && ctx->rejected_promises->items > 0)) + { + ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR); + return NGX_DONE; + } + + if (ctx->redirect_uri.len) { + rc = ngx_http_js_internal_redirect(r, ctx); + ngx_http_finalize_request(r, rc); return NGX_DONE; } @@ -3781,6 +3783,10 @@ static void ngx_http_js_access_body_finalize(ngx_http_request_t *r, ngx_http_js_ctx_t *ctx, ngx_int_t rc) { + if (rc == NGX_ERROR) { + ctx->status = NGX_HTTP_INTERNAL_SERVER_ERROR; + } + switch (ngx_http_js_body_read_phase(ctx->body_read_state)) { case NGX_HTTP_JS_BODY_READ_IN_PROGRESS: ctx->body_read_state = NGX_HTTP_JS_BODY_READ_IDLE; diff --git a/nginx/t/js_access_body.t b/nginx/t/js_access_body.t index 89f5cfad..c9369875 100644 --- a/nginx/t/js_access_body.t +++ b/nginx/t/js_access_body.t @@ -150,6 +150,45 @@ http { js_content test.content; } + location /access_throw { + js_access test.access_throw; + js_content test.content; + } + + location /access_reject { + js_access test.access_reject; + js_content test.content; + } + + location /access_catch { + js_access test.access_catch; + js_content test.content; + } + + location /access_json_reject { + client_body_buffer_size 4k; + js_access test.access_json_reject; + js_content test.content; + } + + location /access_redirect_reject { + js_access test.access_redirect_reject; + js_content test.content; + } + + location /access_deny_throw { + js_access test.access_deny_throw; + js_content test.content; + } + + location /access_sent_deny_throw { + js_access test.access_sent_deny_throw; + js_content test.content; + } + + location /access_secret { + js_content test.secret; + } } @@ -253,15 +292,72 @@ $t->write_file('test.js', < { + let data = JSON.parse(body); + r.variables.foo = data.name; + }); + } + + // js_access must fail closed on a returned rejected promise + // (no body read involved). + function access_reject(r) { + return Promise.reject(new Error('boom')); + } + + // control: a caught rejection allows the request. + function access_catch(r) { + return r.readRequestText().then(body => { + JSON.parse(body); + }).catch(e => { + r.variables.foo = 'caught'; + }); + } + + function access_json_reject(r) { + return r.readRequestJSON().then(data => { + r.variables.foo = data.name; + }); + } + + async function access_redirect_reject(r) { + await new Promise(resolve => setTimeout(resolve, 5)); + r.internalRedirect('/access_secret'); + throw new Error('after redirect'); + } + + function access_deny_throw(r) { + return r.readRequestText().then(body => { + r.return(403); + JSON.parse(body); + }); + } + + function access_sent_deny_throw(r) { + return r.readRequestText().then(body => { + r.return(403, 'denied'); + JSON.parse(body); + }); + } + + function secret(r) { + r.return(200, 'SECRET'); + } + export default { content, content_async, content_text, read_text, read_text_timeout, read_buffer, read_text_twice, read_buffer_twice, read_concurrent_text_buffer, read_text_then_buffer, read_json, read_json_invalid, - read_text_length, echo_body }; + read_text_length, echo_body, access_throw, access_reject, + access_catch, access_json_reject, + access_redirect_reject, access_deny_throw, + access_sent_deny_throw, secret }; EOF -$t->try_run('no js_access')->plan(23); +$t->try_run('no js_access')->plan(31); ############################################################################### @@ -321,6 +417,28 @@ like(http_post_chunked_too_large('/too_large_chunked'), like(http_post('/text'), qr/var:REQ-BODY/, 'readRequestText works after chunked 413'); +like(http_post_json('/access_throw', '{"name":"ok"}'), qr/var:ok/, + 'js_access async body read allows on success'); +like(http_post_json('/access_throw', 'not-json'), + qr/500 Internal Server Error/, + 'js_access fails closed when async continuation throws'); +like(http_post('/access_reject'), qr/500 Internal Server Error/, + 'js_access fails closed on returned rejected promise'); +like(http_post_json('/access_catch', 'not-json'), qr/var:caught/, + 'js_access allows when rejection is caught'); +like(http_post_big_invalid('/access_json_reject'), + qr/500 Internal Server Error/, + 'js_access fails closed on async readRequestJSON rejection'); +like(http_get('/access_redirect_reject'), + qr/500 Internal Server Error/, + 'js_access timeout rejection overrides internal redirect'); +like(http_post_json('/access_deny_throw', 'not-json'), + qr/500 Internal Server Error/, + 'js_access rejection overrides uncommitted deny'); +like(http_post_json('/access_sent_deny_throw', 'not-json'), + qr/403 Forbidden.*denied/s, + 'js_access preserves committed deny response'); + ############################################################################### sub http_post { @@ -360,6 +478,19 @@ sub http_post_big { return http($p, %extra); } +sub http_post_big_invalid { + my ($url, %extra) = @_; + my $body = 'x' x 10240; + + my $p = "POST $url HTTP/1.0" . CRLF . + "Host: localhost" . CRLF . + "Content-Length: " . length($body) . CRLF . + CRLF . + $body; + + return http($p, %extra); +} + sub http_post_slow { my ($url, %extra) = @_;