Skip to content

Commit 93d0d36

Browse files
committed
change(core): keep the original X-Forwarded-* on the ctx, not on ctx.var
`api_ctx.var` proxies NGINX variables: `__index` resolves a name against `ngx.var`, `__newindex` writes through to the real variable for the whitelisted names. `original_x_forwarded_*` is none of those -- it is a plain Lua value that #12551 parked in that namespace. Three consequences, none of them intended: - It looks like a variable it is not. `$original_x_forwarded_proto` is empty in `log_format` and in `proxy_set_header`, which is where recording what a client claimed would actually be useful, yet `ctx.var.original_x_forwarded_proto` reads back fine from Lua. - It shares a namespace with real variable names, so a future NGINX variable of the same name would be silently shadowed by the cached entry. - Reading one that was never written walks the whole `__index` chain -- the `uri_param_` / `http_` / `graphql_` / `post_arg.` prefix tests, the dotted-path branch, the `apisix_var_names` lookup -- and ends in an `ngx.var` lookup for a name NGINX has never heard of. Measured at 53 ns against 0 for a plain table read. So they become `api_ctx.original_x_forwarded_*`. Nothing in this repo reads them under either name -- they have been write-only since they were added -- so this changes no behaviour that anything observes. A plugin outside the tree reading the old name now gets nil rather than a value, which is the same thing it already got on every trusted request. TEST 22 gives the field its first coverage: a forged `X-Forwarded-Proto: https` is rewritten to `http` while the plugin can still see the original.
1 parent 9a22aae commit 93d0d36

2 files changed

Lines changed: 46 additions & 6 deletions

File tree

‎apisix/init.lua‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -740,12 +740,18 @@ local function handle_x_forwarded_headers(api_ctx)
740740
return
741741
end
742742

743-
-- store the original x-forwarded-* headers
744-
-- to allow future use by other plugins or processes
745-
api_ctx.var.original_x_forwarded_proto = xf_proto
746-
api_ctx.var.original_x_forwarded_host = xf_host
747-
api_ctx.var.original_x_forwarded_port = xf_port
748-
api_ctx.var.original_x_forwarded_for = xf_for
743+
-- keep what the client claimed, for plugins that want to see it after the
744+
-- rewrite. These are plain `api_ctx` fields, not `api_ctx.var` entries:
745+
-- `var` proxies NGINX variables, and these are not NGINX variables -- they
746+
-- cannot appear in `log_format` or `proxy_set_header`, so putting them there
747+
-- only makes them look like something they are not, shares a namespace with
748+
-- real variable names, and makes reading an unset one walk the whole
749+
-- `__index` chain down to an `ngx.var` lookup for a name NGINX has never
750+
-- heard of (53 ns, against a plain table read).
751+
api_ctx.original_x_forwarded_proto = xf_proto
752+
api_ctx.original_x_forwarded_host = xf_host
753+
api_ctx.original_x_forwarded_port = xf_port
754+
api_ctx.original_x_forwarded_for = xf_for
749755

750756
-- the replacement values
751757
-- ref: ngx_tpl.lua#L831-L840

‎t/core/trusted-addresses.t‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -802,3 +802,37 @@ routes:
802802
GET /old_uri
803803
--- response_body_like eval
804804
qr/\nx-forwarded-host: localhost\nx-forwarded-port: 1984\nx-forwarded-proto: http\n/
805+
806+
807+
808+
=== TEST 22: the original client-supplied values are kept on the ctx for plugins
809+
--- yaml_config
810+
apisix:
811+
node_listen: 1984
812+
enable_admin: false
813+
deployment:
814+
role: data_plane
815+
role_data_plane:
816+
config_provider: yaml
817+
--- apisix_yaml
818+
routes:
819+
-
820+
id: 1
821+
uri: /old_uri
822+
plugins:
823+
serverless-pre-function:
824+
phase: rewrite
825+
functions:
826+
- return function(conf, ctx) ngx.log(ngx.WARN, "original xfp=", tostring(ctx.original_x_forwarded_proto), " xff=", tostring(ctx.original_x_forwarded_for), " current xfp=", tostring(ctx.var.http_x_forwarded_proto)) end
827+
upstream:
828+
nodes:
829+
"127.0.0.1:1980": 1
830+
type: roundrobin
831+
#END
832+
--- request
833+
GET /old_uri
834+
--- more_headers
835+
X-Forwarded-Proto: https
836+
X-Forwarded-For: 1.2.3.4
837+
--- error_log
838+
original xfp=https xff=1.2.3.4 current xfp=http

0 commit comments

Comments
 (0)