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 e5c2b655d0d5646a1728dd25f223ecff8467c06b..1766d1a0c718830ff4a8f3293088aaa9cb95594a 100644 (file)
--- 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 89f5cfad8a4f72f8835f188de4c6dba2ba74e16d..c936987577cc3ef94200061b65f0645ea2dc0be6 100644 (file)
--- 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', <<EOF);
         r.return(200, 'echo:' + r.requestText);
     }
 
+    // js_access must fail closed when the async body-read continuation
+    // throws: an invalid body denies, a valid body allows.
+    function access_throw(r) {
+        return r.readRequestText().then(body => {
+            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) = @_;