From 67d48a344f01c37dddc12de398202a9893be5e73 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Wed, 20 Nov 2024 17:44:23 -0300 Subject: [PATCH 1/4] tests(proxy-wasm) check that querystring works in dispatch_http_call --- .../hfuncs/130-proxy_dispatch_http.t | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/t/03-proxy_wasm/hfuncs/130-proxy_dispatch_http.t b/t/03-proxy_wasm/hfuncs/130-proxy_dispatch_http.t index 7356cd8a7..6d742d1cf 100644 --- a/t/03-proxy_wasm/hfuncs/130-proxy_dispatch_http.t +++ b/t/03-proxy_wasm/hfuncs/130-proxy_dispatch_http.t @@ -1329,3 +1329,31 @@ cannot override the "Host" header, skipping cannot override the "Connection" header, skipping --- no_error_log [error] + + + +=== TEST 51: proxy_wasm - dispatch_http_call() can use ':path' with querystring +--- load_nginx_modules: ngx_http_echo_module +--- wasm_modules: hostcalls +--- config + location /dispatched { + return 200 "Hello back $request_uri $uri $is_args $args"; + } + + location /t { + proxy_wasm hostcalls 'on=request_body \ + test=/t/dispatch_http_call \ + host=127.0.0.1:$TEST_NGINX_SERVER_PORT \ + path=/dispatched?foo=bar \ + on_http_call_response=echo_response_body'; + echo failed; + } +--- request +GET /t + +Hello world +--- response_body +Hello back /dispatched?foo=bar /dispatched ? foo=bar +--- no_error_log +[error] +[crit] From 0bacf513bc9c6ea0766d6813bbdcaa3decd88392 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Wed, 20 Nov 2024 17:45:07 -0300 Subject: [PATCH 2/4] feat(proxy-wasm) set querystring when setting ":path" --- src/common/proxy_wasm/ngx_proxy_wasm_maps.c | 14 +- .../111-proxy_set_http_request_header.t | 133 ++++++++++++++++-- 2 files changed, 129 insertions(+), 18 deletions(-) diff --git a/src/common/proxy_wasm/ngx_proxy_wasm_maps.c b/src/common/proxy_wasm/ngx_proxy_wasm_maps.c index 20c175bea..45502dd83 100644 --- a/src/common/proxy_wasm/ngx_proxy_wasm_maps.c +++ b/src/common/proxy_wasm/ngx_proxy_wasm_maps.c @@ -511,15 +511,17 @@ ngx_proxy_wasm_maps_set_path(ngx_wavm_instance_t *instance, ngx_str_t *value, ngx_proxy_wasm_ctx_t *pwctx = pwexec->parent; ngx_http_wasm_req_ctx_t *rctx = ngx_http_proxy_wasm_get_rctx(instance); ngx_http_request_t *r = rctx->r; + u_char *query; + size_t len = value->len; - if (ngx_strchr(value->data, '?')) { - ngx_wavm_instance_trap_printf(instance, - "NYI - cannot set request path " - "with querystring"); - return NGX_ERROR; + query = (u_char *) ngx_strchr(value->data, '?'); + if (query) { + len = query - value->data; + r->args.len = value->len - len - 1; + r->args.data = query + 1; } - r->uri.len = value->len; + r->uri.len = len; r->uri.data = value->data; if (pwctx->path.len) { diff --git a/t/03-proxy_wasm/hfuncs/111-proxy_set_http_request_header.t b/t/03-proxy_wasm/hfuncs/111-proxy_set_http_request_header.t index a9cc579f6..24c2c4957 100644 --- a/t/03-proxy_wasm/hfuncs/111-proxy_set_http_request_header.t +++ b/t/03-proxy_wasm/hfuncs/111-proxy_set_http_request_header.t @@ -295,26 +295,135 @@ path: /test -=== TEST 12: proxy_wasm - set_http_request_headers() cannot set ':path' with querystring (NYI) +=== TEST 12: proxy_wasm - set_http_request_headers() can set ':path' with querystring --- wasm_modules: hostcalls +--- http_config eval +qq{ + upstream test_upstream { + server unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + } + + server { + listen unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + + location / { + return 200 '\$request_uri \$uri \$is_args \$args\n'; + } + } +} --- config location /t { proxy_wasm hostcalls 'test=/t/set_request_header name=:path value=/test?foo=bar'; - return 200; + proxy_wasm hostcalls 'test=/t/log/request_path'; + proxy_pass http://test_upstream$uri$is_args$args; } ---- error_code: 500 ---- response_body_like: 500 Internal Server Error ---- grep_error_log eval: qr/(NYI|\[.*?failed resuming).*/ ---- grep_error_log_out eval -qr/.*?NYI - cannot set request path with querystring.* -\[info\] .*? filter chain failed resuming: previous error \(instance trapped\)/ +--- response_body +/test?foo=bar /test ? foo=bar +--- error_log +path: /test?foo=bar --- no_error_log -[alert] -[stderr] +[error] +[crit] + + + +=== TEST 13: proxy_wasm - set_http_request_headers() setting ':path' with empty querystring drops the querystring +--- wasm_modules: hostcalls +--- http_config eval +qq{ + upstream test_upstream { + server unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + } + + server { + listen unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + + location / { + return 200 '(\$request_uri) (\$uri) (\$is_args) (\$args)\n'; + } + } +} +--- config + location /t { + proxy_wasm hostcalls 'test=/t/set_request_header name=:path value=/test?'; + proxy_wasm hostcalls 'test=/t/log/request_path'; + proxy_pass http://test_upstream$uri$is_args$args; + } +--- response_body +(/test) (/test) () () +--- error_log +path: /test +--- no_error_log +[error] +[crit] + + + +=== TEST 14: proxy_wasm - set_http_request_headers() can set ':path' with empty path +--- wasm_modules: hostcalls +--- http_config eval +qq{ + upstream test_upstream { + server unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + } + + server { + listen unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + + location / { + return 200 '(\$request_uri) (\$uri) (\$is_args) (\$args)\n'; + } + } +} +--- config + location /t { + proxy_wasm hostcalls 'test=/t/set_request_header name=:path value='; + proxy_wasm hostcalls 'test=/t/log/request_path'; + proxy_pass http://test_upstream$uri$is_args$args; + } +--- response_body +(/t) (/t) () () +--- error_log +path: +--- no_error_log +[error] +[crit] + + + +=== TEST 15: proxy_wasm - set_http_request_headers() setting ':path' with single '?' has same behavior as empty path +--- wasm_modules: hostcalls +--- http_config eval +qq{ + upstream test_upstream { + server unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + } + + server { + listen unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + + location / { + return 200 '(\$request_uri) (\$uri) (\$is_args) (\$args)\n'; + } + } +} +--- config + location /t { + proxy_wasm hostcalls 'test=/t/set_request_header name=:path value=?'; + proxy_wasm hostcalls 'test=/t/log/request_path'; + proxy_pass http://test_upstream$uri$is_args$args; + } +--- response_body +(/t) (/t) () () +--- error_log +path: +--- no_error_log +[error] +[crit] -=== TEST 13: proxy_wasm - set_http_request_header() sets ':method' +=== TEST 16: proxy_wasm - set_http_request_header() sets ':method' --- wasm_modules: hostcalls --- http_config eval qq{ @@ -344,7 +453,7 @@ POST -=== TEST 14: proxy_wasm - set_http_request_header() cannot set ':scheme' +=== TEST 17: proxy_wasm - set_http_request_header() cannot set ':scheme' --- wasm_modules: hostcalls --- http_config eval qq{ From fd0fdcca407ae8a3f0dbaafe17483070b9559c75 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Wed, 20 Nov 2024 17:46:13 -0300 Subject: [PATCH 3/4] refactor(proxy-wasm) rename call->uri to call->path ...to clarify that it matches `":path"` (that is, path including querystring), making the field name match the pseudo-header, as is the case with the other entries in `ngx_http_proxy_wasm_dispatch_s`. --- src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c | 10 +++++----- src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c b/src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c index bfe15af25..f047819dc 100644 --- a/src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c +++ b/src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c @@ -258,8 +258,8 @@ ngx_http_proxy_wasm_dispatch(ngx_proxy_wasm_exec_t *pwexec, call->method.data = elt->value.data; } else if (ngx_str_eq(elt->key.data, elt->key.len, ":path", -1)) { - call->uri.len = elt->value.len; - call->uri.data = elt->value.data; + call->path.len = elt->value.len; + call->path.data = elt->value.data; } else if (ngx_str_eq(elt->key.data, elt->key.len, ":authority", -1)) @@ -330,7 +330,7 @@ ngx_http_proxy_wasm_dispatch(ngx_proxy_wasm_exec_t *pwexec, call->error = NGX_HTTP_PROXY_WASM_DISPATCH_ERR_BAD_METHOD; goto error; - } else if (!call->uri.len) { + } else if (!call->path.len) { call->error = NGX_HTTP_PROXY_WASM_DISPATCH_ERR_BAD_PATH; goto error; } @@ -577,7 +577,7 @@ ngx_http_proxy_wasm_dispatch_request(ngx_http_proxy_wasm_dispatch_t *call) * Connection: * Content-Length: */ - len += call->method.len + 1 + call->uri.len + 1 + len += call->method.len + 1 + call->path.len + 1 + sizeof(ngx_http_header_version11) - 1; len += sizeof(ngx_http_host_header) - 1 + sizeof(": ") - 1 @@ -638,7 +638,7 @@ ngx_http_proxy_wasm_dispatch_request(ngx_http_proxy_wasm_dispatch_t *call) b->last = ngx_cpymem(b->last, call->method.data, call->method.len); *b->last++ = ' '; - b->last = ngx_cpymem(b->last, call->uri.data, call->uri.len); + b->last = ngx_cpymem(b->last, call->path.data, call->path.len); *b->last++ = ' '; b->last = ngx_cpymem(b->last, ngx_http_header_version11, diff --git a/src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.h b/src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.h index a23e9a22e..97f5b99dd 100644 --- a/src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.h +++ b/src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.h @@ -40,7 +40,7 @@ struct ngx_http_proxy_wasm_dispatch_s { ngx_str_t host; ngx_str_t method; - ngx_str_t uri; + ngx_str_t path; /* ":path" (including query) */ ngx_str_t authority; ngx_array_t headers; ngx_array_t trailers; From f22115b9a6ea334f83ef182c186854cdc3b17271 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Fri, 22 Nov 2024 13:51:17 -0300 Subject: [PATCH 4/4] tests(proxy-wasm) showcase that invalid characters in querystring are passed through. This matches the plain Nginx behavior, and showcased in one of the tests. --- .../111-proxy_set_http_request_header.t | 75 ++++++++++++++++++- .../hfuncs/130-proxy_dispatch_http.t | 28 +++++++ 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/t/03-proxy_wasm/hfuncs/111-proxy_set_http_request_header.t b/t/03-proxy_wasm/hfuncs/111-proxy_set_http_request_header.t index 24c2c4957..348c16d6c 100644 --- a/t/03-proxy_wasm/hfuncs/111-proxy_set_http_request_header.t +++ b/t/03-proxy_wasm/hfuncs/111-proxy_set_http_request_header.t @@ -423,7 +423,78 @@ path: -=== TEST 16: proxy_wasm - set_http_request_header() sets ':method' +=== TEST 16: proxy_wasm - set_http_request_headers() setting ':path' with invalid querystring matches nginx behavior +See following test for a showcase of the nginx behavior. + +--- wasm_modules: hostcalls +--- http_config eval +qq{ + upstream test_upstream { + server unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + } + + server { + listen unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + + location / { + return 200 '(\$request_uri) (\$uri) (\$is_args) (\$args)\n'; + } + } +} +--- config + location /set_by_proxy_wasm { + proxy_wasm hostcalls 'test=/t/set_request_header name=:path value=/test?foo=bár%20bla'; + proxy_wasm hostcalls 'test=/t/log/request_path'; + proxy_pass http://test_upstream$uri$is_args$args; + } +--- request +GET /set_by_proxy_wasm +--- response_body +(/test?foo=bár%20bla) (/test) (?) (foo=bár%20bla) +--- error_log +path: /test +--- no_error_log +[error] +[crit] + + + +=== TEST 17: proxy_wasm - showcase that that path with invalid querystring passes through +This test is here just as documentation, but it showcases +the same behavior as set_http_request_headers(':path'); +see previous test for the proxy-wasm behavior. + +--- wasm_modules: hostcalls +--- http_config eval +qq{ + upstream test_upstream { + server unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + } + + server { + listen unix:$ENV{TEST_NGINX_UNIX_SOCKET}; + + location / { + return 200 '(\$request_uri) (\$uri) (\$is_args) (\$args)\n'; + } + } +} +--- config + location /raw_nginx { + proxy_pass http://test_upstream$uri$is_args$args; + } +--- request +GET /raw_nginx?foo=bár%20bla +--- response_body +(/raw_nginx?foo=bár%20bla) (/raw_nginx) (?) (foo=bár%20bla) +--- no_error_log +[error] +[crit] +[alert] + + + +=== TEST 18: proxy_wasm - set_http_request_header() sets ':method' --- wasm_modules: hostcalls --- http_config eval qq{ @@ -453,7 +524,7 @@ POST -=== TEST 17: proxy_wasm - set_http_request_header() cannot set ':scheme' +=== TEST 19: proxy_wasm - set_http_request_header() cannot set ':scheme' --- wasm_modules: hostcalls --- http_config eval qq{ diff --git a/t/03-proxy_wasm/hfuncs/130-proxy_dispatch_http.t b/t/03-proxy_wasm/hfuncs/130-proxy_dispatch_http.t index 6d742d1cf..247d6a250 100644 --- a/t/03-proxy_wasm/hfuncs/130-proxy_dispatch_http.t +++ b/t/03-proxy_wasm/hfuncs/130-proxy_dispatch_http.t @@ -1357,3 +1357,31 @@ Hello back /dispatched?foo=bar /dispatched ? foo=bar --- no_error_log [error] [crit] + + + +=== TEST 52: proxy_wasm - dispatch_http_call() can use ':path' with querystring, passes through invalid characters +--- load_nginx_modules: ngx_http_echo_module +--- wasm_modules: hostcalls +--- config + location /dispatched { + return 200 "Hello back $request_uri $uri $is_args $args"; + } + + location /t { + proxy_wasm hostcalls 'on=request_body \ + test=/t/dispatch_http_call \ + host=127.0.0.1:$TEST_NGINX_SERVER_PORT \ + path=/dispatched?foo=bár%20bla \ + on_http_call_response=echo_response_body'; + echo failed; + } +--- request +GET /t + +Hello world +--- response_body +Hello back /dispatched?foo=bár%20bla /dispatched ? foo=bár%20bla +--- no_error_log +[error] +[crit]