Skip to content

Commit

Permalink
fix(proxy-wasm) consistent context checks for header setters
Browse files Browse the repository at this point in the history 
This commit creates a helper function `ngx_proxy_wasm_hfuncs_set_header_check`
which is used by all proxy-wasm host functions which set 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.
  • Loading branch information
@hishamhm
hishamhm committed Apr 19, 2024
1 parent 087f287 commit ba18f58
Show file tree
Hide file tree
Showing 7 changed files with 452 additions and 157 deletions.
164 changes: 89 additions & 75 deletions src/common/proxy_wasm/ngx_proxy_wasm_host.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -623,45 +623,47 @@ ngx_proxy_wasm_hfuncs_get_header_map_value(ngx_wavm_instance_t *instance,


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[])
ngx_proxy_wasm_hfuncs_set_header_check(ngx_wavm_instance_t *instance,
ngx_proxy_wasm_map_type_e map_type, wasm_val_t rets[])
{
ngx_int_t rc = NGX_ERROR;
ngx_proxy_wasm_map_type_e map_type;
ngx_array_t headers;
ngx_proxy_wasm_marshalled_map_t map;
ngx_proxy_wasm_exec_t *pwexec;
ngx_proxy_wasm_ctx_t *pwctx;
ngx_proxy_wasm_exec_t *pwexec;
ngx_proxy_wasm_ctx_t *pwctx;
#ifdef NGX_WASM_HTTP
ngx_http_wasm_req_ctx_t *rctx;
ngx_http_wasm_req_ctx_t *rctx;

rctx = ngx_http_proxy_wasm_get_rctx(instance);
#endif

pwexec = ngx_proxy_wasm_instance2pwexec(instance);
pwctx = pwexec->parent;

map_type = args[0].of.i32;
map.len = args[2].of.i32;
map.data = NGX_WAVM_HOST_LIFT_SLICE(instance, args[1].of.i32, map.len);

if (ngx_proxy_wasm_pairs_unmarshal(pwexec, &headers, &map) != NGX_OK) {
return ngx_proxy_wasm_result_err(rets);
}

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);
/* 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);
}

rc = ngx_http_wasm_clear_req_headers(
ngx_http_proxy_wasm_get_req(instance));
ngx_wa_assert(!rctx->entered_header_filter);
break;

case NGX_PROXY_WASM_MAP_HTTP_RESPONSE_HEADERS:
Expand All @@ -674,6 +676,13 @@ ngx_proxy_wasm_hfuncs_set_header_map_pairs(ngx_wavm_instance_t *instance,
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 "
Expand All @@ -682,14 +691,54 @@ ngx_proxy_wasm_hfuncs_set_header_map_pairs(ngx_wavm_instance_t *instance,
rets, NGX_WAVM_BAD_USAGE);
}

if (rctx->r->header_sent) {
ngx_wavm_log_error(NGX_LOG_ERR, instance->log, NULL,
"cannot set response headers: "
"headers already sent");
ngx_wa_assert(!rctx->r->header_sent);
break;
#endif

return ngx_proxy_wasm_result_ok(rets);
}
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[])
{
ngx_int_t rc = NGX_ERROR;
ngx_proxy_wasm_map_type_e map_type;
ngx_array_t headers;
ngx_proxy_wasm_marshalled_map_t map;
ngx_proxy_wasm_exec_t *pwexec;

pwexec = ngx_proxy_wasm_instance2pwexec(instance);

map_type = args[0].of.i32;
map.len = args[2].of.i32;
map.data = NGX_WAVM_HOST_LIFT_SLICE(instance, args[1].of.i32, map.len);

if (ngx_proxy_wasm_pairs_unmarshal(pwexec, &headers, &map) != NGX_OK) {
return ngx_proxy_wasm_result_err(rets);
}

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:
rc = ngx_http_wasm_clear_req_headers(
ngx_http_proxy_wasm_get_req(instance));
break;

case NGX_PROXY_WASM_MAP_HTTP_RESPONSE_HEADERS:
rc = ngx_http_wasm_clear_resp_headers(
ngx_http_proxy_wasm_get_req(instance));
break;
Expand Down Expand Up @@ -727,37 +776,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);
Expand All @@ -781,37 +810,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);
Expand Down Expand Up @@ -842,6 +851,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);

Expand Down
94 changes: 82 additions & 12 deletions t/03-proxy_wasm/hfuncs/107-proxy_add_http_request_header.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -547,35 +547,105 @@ qr/.*? on_request_headers, 3 headers.*



=== TEST 27: proxy_wasm - add_http_request_header() x on_phases
should log an error (but no trap) when response is produced
=== TEST 27: proxy_wasm - add_http_request_header() x on_request_headers
--- 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';
}
--- more_headers
Hello: world
pwm-add-req-header: Hello=world
--- response_body_like
Hello: world
Hello: world
--- grep_error_log eval: qr/\[(error|hostcalls)\] [^,]*/
--- grep_error_log_out
[hostcalls] on_request_headers
[hostcalls] testing in "RequestHeaders"
[hostcalls] on_request_headers
[hostcalls] testing in "RequestHeaders"
[hostcalls] on_response_headers
[hostcalls] on_response_headers
[hostcalls] on_response_body
[hostcalls] on_response_body
[hostcalls] on_done while logging request
[hostcalls] on_log while logging request
[hostcalls] on_done while logging request
[hostcalls] on_log while logging request
--- no_error_log
[warn]
[crit]
[alert]



=== TEST 28: proxy_wasm - set_http_request_headers() x on_response_headers
--- wasm_modules: hostcalls
--- config
location /t {
proxy_wasm hostcalls 'on=response_headers \
test=/t/add_request_header';
proxy_wasm hostcalls 'on=log \
test=/t/add_request_header';

return 200;
}
--- more_headers
Hello: world
pwm-add-req-header: Hello=world
--- response_body_like
--- grep_error_log eval: qr/.*?host trap.*/
--- grep_error_log_out eval
qr~(\[error\]|Uncaught RuntimeError|\s+).*?host trap \(bad usage\): can only set request headers before response is produced.*~
--- no_error_log
[warn]
[crit]
[alert]
[emerg]



=== TEST 29: proxy_wasm - set_http_request_headers() x on_response_body
--- wasm_modules: hostcalls
--- config
location /t {
proxy_wasm hostcalls 'on=response_body \
test=/t/add_request_header';

return 200;
}
--- more_headers
Hello: world
pwm-add-req-header: Hello=world
--- grep_error_log eval: qr/.*?host trap.*/
--- grep_error_log_out eval
qr~(\[error\]|Uncaught RuntimeError|\s+).*?host trap \(bad usage\): can only set request headers before response is produced.*~
--- no_error_log
[warn]
[crit]
[alert]
[emerg]



=== TEST 30: proxy_wasm - set_http_request_headers() x on_log
--- wasm_modules: hostcalls
--- config
location /t {
proxy_wasm hostcalls 'on=log \
test=/t/add_request_header';

return 200;
}
--- more_headers
Hello: world
--- grep_error_log eval: qr/\[(info|error)\] .*? (testing in|cannot add).*/
pwm-add-req-header: Hello=world
--- grep_error_log eval: qr/.*?host trap.*/
--- 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.*/
qr~(\[error\]|Uncaught RuntimeError|\s+).*?host trap \(bad usage\): can only set request headers before response is produced.*~
--- no_error_log
[warn]
[crit]
[alert]
[emerg]
Loading

0 comments on commit ba18f58

Please sign in to comment.