HTTP: fixed r.return() with body in js_access.

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.

diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c
index 29333fc11c7025fbab53fee8d9ea4dc46f9e30cb..b91c700a9944b3529467e2b08a95f0872403b0da 100644 (file)
--- a/nginx/ngx_http_js_module.c
+++ b/nginx/ngx_http_js_module.c
@@ -174,6 +174,8 @@ typedef struct {
 
 
 static ngx_int_t ngx_http_js_access_handler(ngx_http_request_t *r);
+static ngx_int_t ngx_http_js_access_finalize(ngx_http_request_t *r,
+    ngx_http_js_ctx_t *ctx);
 static void ngx_http_js_access_write_event_handler(ngx_http_request_t *r);
 static ngx_int_t ngx_http_js_content_handler(ngx_http_request_t *r);
 static void ngx_http_js_content_event_handler(ngx_http_request_t *r);
@@ -1540,7 +1542,7 @@ ngx_http_js_access_handler(ngx_http_request_t *r)
             return NGX_HTTP_INTERNAL_SERVER_ERROR;
         }
 
-        return ctx->status;
+        return ngx_http_js_access_finalize(r, ctx);
     }
 
     ctx->status = NGX_OK;
@@ -1594,6 +1596,18 @@ done:
         return NGX_AGAIN;
     }
 
+    return ngx_http_js_access_finalize(r, ctx);
+}
+
+
+static ngx_int_t
+ngx_http_js_access_finalize(ngx_http_request_t *r, ngx_http_js_ctx_t *ctx)
+{
+    if (r->header_sent) {
+        ngx_http_finalize_request(r, NGX_OK);
+        return NGX_DONE;
+    }
+
     return ctx->status;
 }
 
@@ -1601,6 +1615,7 @@ done:
 static void
 ngx_http_js_access_write_event_handler(ngx_http_request_t *r)
 {
+    ngx_int_t          rc;
     ngx_http_js_ctx_t  *ctx;
 
     ctx = ngx_http_get_module_ctx(r, ngx_http_js_module);
@@ -1608,10 +1623,17 @@ ngx_http_js_access_write_event_handler(ngx_http_request_t *r)
     ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
                    "http js access write event handler");
 
-    if (!ngx_js_ctx_pending(ctx)) {
-        ngx_http_core_run_phases(r);
+    if (ngx_js_ctx_pending(ctx)) {
         return;
     }
+
+    rc = ngx_http_js_access_finalize(r, ctx);
+
+    if (rc == NGX_DONE) {
+        return;
+    }
+
+    ngx_http_core_run_phases(r);
 }
 
 
@@ -3697,6 +3719,10 @@ ngx_http_js_access_body_finalize(ngx_http_request_t *r, ngx_http_js_ctx_t *ctx,
             return;
         }
 
+        if (ngx_http_js_access_finalize(r, ctx) == NGX_DONE) {
+            return;
+        }
+
         r->write_event_handler = ngx_http_core_run_phases;
         ngx_http_core_run_phases(r);
         break;

diff --git a/nginx/t/js_access.t b/nginx/t/js_access.t
index 29ac026eb8808f4a8769c16e5fe84c2f5964b2b9..bf328f0d030e8405248d53ff3cdb35b77e898a68 100644 (file)
--- a/nginx/t/js_access.t
+++ b/nginx/t/js_access.t
@@ -89,6 +89,31 @@ http {
             js_content test.content;
         }
 
+        location /deny_body_proxy {
+            js_access test.deny_body;
+            proxy_pass http://127.0.0.1:8083;
+        }
+
+        location /async_deny_body_proxy {
+            js_access test.async_deny_body;
+            proxy_pass http://127.0.0.1:8083;
+        }
+
+        location /access_return_200 {
+            js_access test.access_return_200;
+            js_content test.content;
+        }
+
+        location /deny_error_page {
+            error_page 403 /custom_403;
+            js_access test.deny;
+            js_content test.content;
+        }
+
+        location /custom_403 {
+            return 200 "custom403";
+        }
+
         location /async_exception {
             js_access test.async_exception;
             js_content test.content;
@@ -161,6 +186,15 @@ http {
             return 200 "backend2";
         }
     }
+
+    server {
+        listen       127.0.0.1:8083;
+        access_log  %%TESTDIR%%/backend_access.log combined;
+
+        location / {
+            return 200 "backend_access";
+        }
+    }
 }
 
 EOF
@@ -178,6 +212,10 @@ $t->write_file('test.js', <<EOF);
         r.return(403);
     }
 
+    function deny_body(r) {
+        r.return(403, 'denied');
+    }
+
     function exception(r) {
         throw new Error("access_error");
     }
@@ -208,6 +246,11 @@ $t->write_file('test.js', <<EOF);
         r.return(403);
     }
 
+    async function async_deny_body(r) {
+        await new Promise(resolve => setTimeout(resolve, 5));
+        r.return(403, 'denied_async');
+    }
+
     async function async_exception(r) {
         await new Promise(resolve => setTimeout(resolve, 5));
         throw new Error("async_access_error");
@@ -257,6 +300,10 @@ $t->write_file('test.js', <<EOF);
         }
     }
 
+    function access_return_200(r) {
+        r.return(200, 'access_ok');
+    }
+
     function redirect(r) {
         r.return(302, 'http://127.0.0.1:$p0/callback');
     }
@@ -266,13 +313,16 @@ $t->write_file('test.js', <<EOF);
         r.return(302, 'http://127.0.0.1:$p0/callback');
     }
 
-    export default { content, deny, exception, noop, override,
+    export default { content, deny, deny_body, exception, noop, override,
                      decline, content_only, async_timeout, async_deny,
+                     async_deny_body, access_return_200,
                      async_exception, sr_skip, sr, fetch, route,
                      auth_check, redirect, redirect_async };
 
 EOF
 
+$t->write_file('backend_access.log', '');
+
 $t->write_file_expand('duplicate.conf', bad_conf(
        http     => 'js_import test.js;',
        location => 'js_access test.noop; js_access test.noop;'));
@@ -280,7 +330,7 @@ $t->write_file_expand('duplicate.conf', bad_conf(
 $t->write_file_expand('no_import.conf', bad_conf(
        location => 'js_access test.noop;'));
 
-$t->try_run('no js_access')->plan(26);
+$t->try_run('no js_access')->plan(33);
 
 ###############################################################################
 
@@ -306,6 +356,18 @@ like(http_get('/async_timeout'), qr/var:timeout_ok/,
        'async js_access with setTimeout');
 like(http_get('/async_deny'), qr/403 Forbidden/,
        'async js_access r.return(403) rejects');
+like(http_post('/deny_body_proxy'), qr/403 Forbidden.*denied/s,
+       'js_access r.return(403, body) rejects');
+is($t->read_file('backend_access.log'), '',
+       'js_access r.return(403, body) skips proxy_pass');
+like(http_post('/async_deny_body_proxy'), qr/403 Forbidden.*denied_async/s,
+       'async js_access r.return(403, body) rejects');
+is($t->read_file('backend_access.log'), '',
+       'async js_access r.return(403, body) skips proxy_pass');
+like(http_get('/access_return_200'), qr/200 OK.*access_ok/s,
+       'js_access r.return(200, body) finalizes request');
+like(http_get('/deny_error_page'), qr/custom403/,
+       'js_access r.return(403) preserves error_page');
 like(http_get('/async_exception'), qr/500 Internal Server Error/,
        'async js_access exception returns 500');
 like(http_get('/sr_skip'), qr/var:/,
@@ -343,6 +405,9 @@ isnt($rc, 0, 'js_access without js_import fails');
 like($out, qr/no imports defined for "js_access" "test\.noop"/,
        'js_access without js_import error');
 
+unlike($t->read_file('error.log'), qr/header already sent/,
+       'js_access body return does not double-send headers');
+
 ###############################################################################
 
 sub http_post {