HTTP: fixed internalRedirect() 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.

diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c
index b91c700a9944b3529467e2b08a95f0872403b0da..2ac3963798d6fcded6f186c4fc4cc8c53f628584 100644 (file)
--- a/nginx/ngx_http_js_module.c
+++ b/nginx/ngx_http_js_module.c
@@ -182,6 +182,8 @@ static void ngx_http_js_content_event_handler(ngx_http_request_t *r);
 static void ngx_http_js_content_write_event_handler(ngx_http_request_t *r);
 static void ngx_http_js_content_finalize(ngx_http_request_t *r,
     ngx_http_js_ctx_t *ctx);
+static ngx_int_t ngx_http_js_internal_redirect(ngx_http_request_t *r,
+    ngx_http_js_ctx_t *ctx);
 static ngx_int_t ngx_http_js_header_filter(ngx_http_request_t *r);
 static ngx_int_t ngx_http_js_variable_set(ngx_http_request_t *r,
     ngx_http_variable_value_t *v, uintptr_t data);
@@ -1603,6 +1605,14 @@ done:
 static ngx_int_t
 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);
+        return NGX_DONE;
+    }
+
     if (r->header_sent) {
         ngx_http_finalize_request(r, NGX_OK);
         return NGX_DONE;
@@ -1761,33 +1771,48 @@ ngx_http_js_content_write_event_handler(ngx_http_request_t *r)
 static void
 ngx_http_js_content_finalize(ngx_http_request_t *r, ngx_http_js_ctx_t *ctx)
 {
-    ngx_str_t   args;
-    ngx_int_t   rc;
-    ngx_uint_t  flags;
+    ngx_int_t  rc;
 
     ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
                    "http js content rc: %i", ctx->status);
 
     if (ctx->redirect_uri.len) {
-        if (ctx->redirect_uri.data[0] == '@') {
-            ngx_http_named_location(r, &ctx->redirect_uri);
+        rc = ngx_http_js_internal_redirect(r, ctx);
+        ngx_http_finalize_request(r, rc);
+        return;
+    }
 
-        } else {
-            ngx_str_null(&args);
-            flags = NGX_HTTP_LOG_UNSAFE;
+    ngx_http_finalize_request(r, ctx->status);
+}
 
-            rc = ngx_http_parse_unsafe_uri(r, &ctx->redirect_uri, &args,
-                                           &flags);
-            if (rc != NGX_OK) {
-                ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
-                return;
-            }
 
-            ngx_http_internal_redirect(r, &ctx->redirect_uri, &args);
-        }
+static ngx_int_t
+ngx_http_js_internal_redirect(ngx_http_request_t *r, ngx_http_js_ctx_t *ctx)
+{
+    ngx_str_t   args;
+    ngx_int_t   rc;
+    ngx_uint_t  flags;
+
+    if (r->header_sent) {
+        ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
+                      "ignored internalRedirect() because headers were "
+                      "already sent");
+        return NGX_OK;
     }
 
-    ngx_http_finalize_request(r, ctx->status);
+    if (ctx->redirect_uri.data[0] == '@') {
+        return ngx_http_named_location(r, &ctx->redirect_uri);
+    }
+
+    ngx_str_null(&args);
+    flags = NGX_HTTP_LOG_UNSAFE;
+
+    rc = ngx_http_parse_unsafe_uri(r, &ctx->redirect_uri, &args, &flags);
+    if (rc != NGX_OK) {
+        return NGX_HTTP_INTERNAL_SERVER_ERROR;
+    }
+
+    return ngx_http_internal_redirect(r, &ctx->redirect_uri, &args);
 }
 
 

diff --git a/nginx/t/js_access.t b/nginx/t/js_access.t
index bf328f0d030e8405248d53ff3cdb35b77e898a68..4744436fcbb83314b101bacdb800e2e8ba4ee8c3 100644 (file)
--- a/nginx/t/js_access.t
+++ b/nginx/t/js_access.t
@@ -157,6 +157,29 @@ http {
             js_content test.content;
         }
 
+        location /access_internal_redirect {
+            js_access test.access_internal_redirect;
+            js_content test.content;
+        }
+
+        location /access_internal_redirect_async {
+            js_access test.access_internal_redirect_async;
+            js_content test.content;
+        }
+
+        location /access_internal_redirect_named {
+            js_access test.access_internal_redirect_named;
+            js_content test.content;
+        }
+
+        location /internal_redirect_dest {
+            return 200 "internal_redirect";
+        }
+
+        location @internal_redirect_named {
+            return 200 "internal_redirect_named";
+        }
+
         location /callback {
             js_content test.content;
         }
@@ -313,11 +336,27 @@ $t->write_file('test.js', <<EOF);
         r.return(302, 'http://127.0.0.1:$p0/callback');
     }
 
+    function access_internal_redirect(r) {
+        r.internalRedirect('/internal_redirect_dest');
+    }
+
+    async function access_internal_redirect_async(r) {
+        await new Promise(resolve => setTimeout(resolve, 5));
+        r.internalRedirect('/internal_redirect_dest');
+    }
+
+    function access_internal_redirect_named(r) {
+        r.internalRedirect('\@internal_redirect_named');
+    }
+
     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 };
+                     auth_check, redirect, redirect_async,
+                     access_internal_redirect,
+                     access_internal_redirect_async,
+                     access_internal_redirect_named };
 
 EOF
 
@@ -330,7 +369,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(33);
+$t->try_run('no js_access')->plan(36);
 
 ###############################################################################
 
@@ -392,6 +431,12 @@ like(http_get('/redirect_async'), qr/302 Moved/,
        'js_access async redirect');
 like(http_get('/redirect_async'), qr!Location: http://127.0.0.1:$p0/callback!,
        'js_access async redirect Location header');
+like(http_get('/access_internal_redirect'), qr/internal_redirect/,
+       'js_access internalRedirect');
+like(http_get('/access_internal_redirect_async'), qr/internal_redirect/,
+       'async js_access internalRedirect');
+like(http_get('/access_internal_redirect_named'), qr/internal_redirect_named/,
+       'js_access internalRedirect to named location');
 
 my ($rc, $out) = nginx_test_conf($t, 'duplicate.conf');