feat(proxy-wasm) consistent context checks for all header setters

This commit creates a helper function `hfuncs_set_header_check` which is used by all Proxy-Wasm host functions setting headers (set, add, replace, remove), ensuring a consistent behavior across all of them. In particular, some of the header setters did not produce `BAD_ARGUMENT` when `map_type` is invalid. This also changes the behavior of the functions to produce traps when they are called in invalid contexts, instead of logging an error and returning `OK`. This matches the behavior of the body setter functions. Also add context checks suites for all header setters. Co-Authored-By: Thibault Charbonnier <[email protected]>
Kong · Apr 24, 2024 · 4ed3817 · 4ed3817
1 parent ee5890d
commit 4ed3817
Show file tree

Hide file tree

Showing 16 changed files with 1,670 additions and 304 deletions.
diff --git a/src/common/proxy_wasm/ngx_proxy_wasm_host.c b/src/common/proxy_wasm/ngx_proxy_wasm_host.c
@@ -622,6 +622,79 @@ ngx_proxy_wasm_hfuncs_get_header_map_value(ngx_wavm_instance_t *instance,
 }
 
 
+static ngx_int_t
+ngx_proxy_wasm_hfuncs_set_header_check(ngx_wavm_instance_t *instance,
+    ngx_proxy_wasm_map_type_e map_type, wasm_val_t rets[])
+{
+#ifdef NGX_WASM_HTTP
+    ngx_proxy_wasm_exec_t    *pwexec;
+    ngx_proxy_wasm_ctx_t     *pwctx;
+    ngx_http_wasm_req_ctx_t  *rctx;
+
+    rctx = ngx_http_proxy_wasm_get_rctx(instance);
+    pwexec = ngx_proxy_wasm_instance2pwexec(instance);
+    pwctx = pwexec->parent;
+#endif
+
+    switch (map_type) {
+#ifdef NGX_WASM_HTTP
+    case NGX_PROXY_WASM_MAP_HTTP_REQUEST_HEADERS:
+
+        /* check context */
+
+        switch (pwctx->step) {
+        case NGX_PROXY_WASM_STEP_REQ_HEADERS:
+        case NGX_PROXY_WASM_STEP_REQ_BODY:
+        case NGX_PROXY_WASM_STEP_REQ_TRAILERS:
+            break;
+        case NGX_PROXY_WASM_STEP_DISPATCH_RESPONSE:
+            if (!rctx->entered_header_filter) {
+                return NGX_OK;
+            }
+            /* fallthrough */
+        default:
+            return ngx_proxy_wasm_result_trap(pwexec,
+                                              "can only set request headers "
+                                              "before response is produced",
+                                              rets, NGX_WAVM_BAD_USAGE);
+        }
+
+        break;
+
+    case NGX_PROXY_WASM_MAP_HTTP_RESPONSE_HEADERS:
+
+        /* check context */
+
+        switch (pwctx->step) {
+        case NGX_PROXY_WASM_STEP_REQ_HEADERS:
+        case NGX_PROXY_WASM_STEP_REQ_BODY:
+        case NGX_PROXY_WASM_STEP_REQ_TRAILERS:
+        case NGX_PROXY_WASM_STEP_RESP_HEADERS:
+            break;
+        case NGX_PROXY_WASM_STEP_DISPATCH_RESPONSE:
+            if (!rctx->r->header_sent) {
+                return NGX_OK;
+            }
+            /* fallthrough */
+        default:
+            return ngx_proxy_wasm_result_trap(pwexec,
+                                              "can only set response headers "
+                                              "before \"on_response_body\"",
+                                              rets, NGX_WAVM_BAD_USAGE);
+        }
+
+        break;
+#endif
+    default:
+        ngx_wasm_log_error(NGX_LOG_WASM_NYI, instance->log, 0,
+                           "NYI - set_map bad map_type: %d", map_type);
+        return ngx_proxy_wasm_result_badarg(rets);
+    }
+
+    return NGX_OK;
+}
+
+
 static ngx_int_t
 ngx_proxy_wasm_hfuncs_set_header_map_pairs(ngx_wavm_instance_t *instance,
     wasm_val_t args[], wasm_val_t rets[])
@@ -631,11 +704,6 @@ ngx_proxy_wasm_hfuncs_set_header_map_pairs(ngx_wavm_instance_t *instance,
     ngx_array_t                       headers;
     ngx_proxy_wasm_marshalled_map_t   map;
     ngx_proxy_wasm_exec_t            *pwexec;
-#ifdef NGX_WASM_HTTP
-    ngx_http_wasm_req_ctx_t          *rctx;
-
-    rctx = ngx_http_proxy_wasm_get_rctx(instance);
-#endif
 
     pwexec = ngx_proxy_wasm_instance2pwexec(instance);
 
@@ -647,35 +715,23 @@ ngx_proxy_wasm_hfuncs_set_header_map_pairs(ngx_wavm_instance_t *instance,
         return ngx_proxy_wasm_result_err(rets);
     }
 
-    switch (map_type) {
+    rc = ngx_proxy_wasm_hfuncs_set_header_check(instance, map_type, rets);
+    if (rc != NGX_OK) {
+        return rc;
+    }
 
+    switch (map_type) {
 #ifdef NGX_WASM_HTTP
     case NGX_PROXY_WASM_MAP_HTTP_REQUEST_HEADERS:
-        if (rctx->entered_header_filter) {
-            ngx_wavm_log_error(NGX_LOG_ERR, instance->log, NULL,
-                               "cannot set request headers: response produced");
-
-            return ngx_proxy_wasm_result_ok(rets);
-        }
-
         rc = ngx_http_wasm_clear_req_headers(
                  ngx_http_proxy_wasm_get_req(instance));
         break;
 
     case NGX_PROXY_WASM_MAP_HTTP_RESPONSE_HEADERS:
-        if (rctx->r->header_sent) {
-            ngx_wavm_log_error(NGX_LOG_ERR, instance->log, NULL,
-                               "cannot set response headers: "
-                               "headers already sent");
-
-            return ngx_proxy_wasm_result_ok(rets);
-        }
-
         rc = ngx_http_wasm_clear_resp_headers(
                  ngx_http_proxy_wasm_get_req(instance));
         break;
 #endif
-
     default:
         ngx_wasm_log_error(NGX_LOG_WASM_NYI, instance->log, 0,
                            "NYI - set_map bad map_type: %d", map_type);
@@ -708,37 +764,17 @@ ngx_proxy_wasm_hfuncs_add_header_map_value(ngx_wavm_instance_t *instance,
     ngx_int_t                   rc;
     ngx_str_t                   key, value;
     ngx_proxy_wasm_map_type_e   map_type;
-#ifdef NGX_WASM_HTTP
-    ngx_http_wasm_req_ctx_t    *rctx;
-
-    rctx = ngx_http_proxy_wasm_get_rctx(instance);
-#endif
 
     map_type = args[0].of.i32;
     key.len = args[2].of.i32;
     key.data = NGX_WAVM_HOST_LIFT_SLICE(instance, args[1].of.i32, key.len);
     value.len = args[4].of.i32;
     value.data = NGX_WAVM_HOST_LIFT_SLICE(instance, args[3].of.i32, value.len);
 
-#ifdef NGX_WASM_HTTP
-    if (map_type == NGX_PROXY_WASM_MAP_HTTP_REQUEST_HEADERS
-        && rctx->entered_header_filter)
-    {
-        ngx_wavm_log_error(NGX_LOG_ERR, instance->log, NULL,
-                           "cannot add request header: response produced");
-
-        return ngx_proxy_wasm_result_ok(rets);
-
-    } else if (map_type == NGX_PROXY_WASM_MAP_HTTP_RESPONSE_HEADERS
-               && rctx->r->header_sent)
-    {
-        ngx_wavm_log_error(NGX_LOG_ERR, instance->log, NULL,
-                           "cannot add response header: "
-                           "headers already sent");
-
-        return ngx_proxy_wasm_result_ok(rets);
+    rc = ngx_proxy_wasm_hfuncs_set_header_check(instance, map_type, rets);
+    if (rc != NGX_OK) {
+        return rc;
     }
-#endif
 
     dd("adding '%.*s: %.*s' to map of type '%d'",
        (int) key.len, key.data, (int) value.len, value.data, map_type);
@@ -762,37 +798,17 @@ ngx_proxy_wasm_hfuncs_replace_header_map_value(ngx_wavm_instance_t *instance,
     ngx_int_t                   rc;
     ngx_str_t                   key, value;
     ngx_proxy_wasm_map_type_e   map_type;
-#ifdef NGX_WASM_HTTP
-    ngx_http_wasm_req_ctx_t    *rctx;
-
-    rctx = ngx_http_proxy_wasm_get_rctx(instance);
-#endif
 
     map_type = args[0].of.i32;
     key.len = args[2].of.i32;
     key.data = NGX_WAVM_HOST_LIFT_SLICE(instance, args[1].of.i32, key.len);
     value.len = args[4].of.i32;
     value.data = NGX_WAVM_HOST_LIFT_SLICE(instance, args[3].of.i32, value.len);
 
-#ifdef NGX_WASM_HTTP
-    if (map_type == NGX_PROXY_WASM_MAP_HTTP_REQUEST_HEADERS
-        && rctx->entered_header_filter)
-    {
-        ngx_wavm_log_error(NGX_LOG_ERR, instance->log, NULL,
-                           "cannot set request header: response produced");
-
-        return ngx_proxy_wasm_result_ok(rets);
-
-    } else if (map_type == NGX_PROXY_WASM_MAP_HTTP_RESPONSE_HEADERS
-               && rctx->r->header_sent)
-    {
-        ngx_wavm_log_error(NGX_LOG_ERR, instance->log, NULL,
-                           "cannot set response header: "
-                           "headers already sent");
-
-        return ngx_proxy_wasm_result_ok(rets);
+    rc = ngx_proxy_wasm_hfuncs_set_header_check(instance, map_type, rets);
+    if (rc != NGX_OK) {
+        return rc;
     }
-#endif
 
     dd("setting '%.*s: %.*s' into map of type '%d'",
        (int) key.len, key.data, (int) value.len, value.data, map_type);
@@ -823,6 +839,11 @@ ngx_proxy_wasm_hfuncs_remove_header_map_value(ngx_wavm_instance_t *instance,
     klen = args[2].of.i32;
     key = NGX_WAVM_HOST_LIFT_SLICE(instance, args[1].of.i32, klen);
 
+    rc = ngx_proxy_wasm_hfuncs_set_header_check(instance, map_type, rets);
+    if (rc != NGX_OK) {
+        return rc;
+    }
+
     dd("removing '%.*s' from map of type '%d'",
        (int) klen, (u_char *) key, map_type);
 

diff --git a/t/03-proxy_wasm/hfuncs/107-proxy_add_http_request_header.t b/t/03-proxy_wasm/hfuncs/107-proxy_add_http_request_header.t
@@ -544,38 +544,3 @@ User-Agent: Konqueror
 --- grep_error_log_out eval
 qr/.*? on_request_headers, 3 headers.*
 .*? on_request_headers, 4 headers.*/
-
-
-
-=== TEST 27: proxy_wasm - add_http_request_header() x on_phases
-should log an error (but no trap) when response is produced
---- wasm_modules: hostcalls
---- config
-    location /t {
-        proxy_wasm hostcalls 'on=request_headers \
-                              test=/t/add_request_header';
-        proxy_wasm hostcalls 'on=request_headers \
-                              test=/t/echo/headers';
-        proxy_wasm hostcalls 'on=response_headers \
-                              test=/t/add_request_header';
-        proxy_wasm hostcalls 'on=log \
-                              test=/t/add_request_header';
-    }
---- more_headers
-Hello: world
-pwm-add-req-header: Hello=world
---- response_body_like
-Hello: world
-Hello: world
---- grep_error_log eval: qr/\[(info|error)\] .*? (testing in|cannot add).*/
---- grep_error_log_out eval
-qr/.*? testing in "RequestHeaders".*
-.*? testing in "RequestHeaders".*
-.*? testing in "ResponseHeaders".*
-.*? cannot add request header: response produced.*
-.*? testing in "Log".*
-.*? cannot add request header: response produced.*/
---- no_error_log
-[warn]
-[crit]
-[alert]
diff --git a/t/03-proxy_wasm/hfuncs/108-proxy_add_http_response_header.t b/t/03-proxy_wasm/hfuncs/108-proxy_add_http_response_header.t
@@ -224,7 +224,6 @@ qr/\[error\] .*? \[wasm\] cannot add new "Content-Length" builtin response heade
 
 === TEST 9: proxy_wasm - add_http_response_header() cannot add empty Content-Length
 should log an error but not produce a trap
---- load_nginx_modules: ngx_http_headers_more_filter_module
 --- wasm_modules: hostcalls
 --- config
     location /t {
@@ -372,42 +371,3 @@ qr/.*? on_response_headers, 6 headers.*
 resp Cache-Control: no-cache.*/
 --- no_error_log
 [error]
-
-
-
-=== TEST 15: proxy_wasm - add_http_response_header() x on_phases
-should log an error (but no trap) when headers are sent
---- wasm_modules: hostcalls
---- config
-    location /t {
-        proxy_wasm hostcalls 'on=request_headers \
-                              test=/t/add_response_header \
-                              value=From:request_headers';
-
-        proxy_wasm hostcalls 'on=response_headers \
-                              test=/t/add_response_header \
-                              value=From:response_headers';
-
-        proxy_wasm hostcalls 'on=response_body \
-                              test=/t/add_response_header \
-                              value=From:response_body';
-
-        proxy_wasm hostcalls 'on=log \
-                              test=/t/add_response_header \
-                              value=From:log';
-        return 200;
-    }
---- response_headers
-From: request_headers, response_headers
---- ignore_response_body
---- grep_error_log eval: qr/\[(error|hostcalls)\] [^on_].*/
---- grep_error_log_out eval
-qr/.*? testing in "RequestHeaders".*
-.*? testing in "ResponseHeaders".*
-.*? testing in "ResponseBody".*
-\[error\] .*? \[wasm\] cannot add response header: headers already sent.*
-.*? testing in "Log".*
-\[error\] .*? \[wasm\] cannot add response header: headers already sent.*/
---- no_error_log
-[crit]
-[emerg]
diff --git a/t/03-proxy_wasm/hfuncs/109-proxy_set_http_request_headers.t b/t/03-proxy_wasm/hfuncs/109-proxy_set_http_request_headers.t
@@ -99,36 +99,3 @@ Welcome: wasm
 [error]
 [crit]
 [alert]
-
-
-
-=== TEST 5: proxy_wasm - set_http_request_headers() x on_phases
-should log an error (but no trap) when response is produced
---- wasm_modules: hostcalls
---- config
-    location /t {
-        proxy_wasm hostcalls 'on=request_headers \
-                              test=/t/set_request_headers';
-        proxy_wasm hostcalls 'on=request_headers \
-                              test=/t/echo/headers';
-        proxy_wasm hostcalls 'on=response_headers \
-                              test=/t/set_request_headers';
-        proxy_wasm hostcalls 'on=log \
-                              test=/t/set_request_headers';
-    }
---- more_headers eval
-CORE::join "\n", map { "Header$_: value-$_" } 1..20
---- response_body
-Hello: world
-Welcome: wasm
---- grep_error_log eval: qr/\[(error|hostcalls)\] [^on_].*/
---- grep_error_log_out eval
-qr/.*? testing in "RequestHeaders".*
-.*? testing in "RequestHeaders".*
-.*? testing in "ResponseHeaders".*
-\[error\] .*? \[wasm\] cannot set request headers: response produced.*
-.*? testing in "Log".*
-\[error\] .*? \[wasm\] cannot set request headers: response produced.*/
---- no_error_log
-[warn]
-[crit]