AlinsRan commented on PR #13803: URL: https://github.com/apache/apisix/pull/13803#issuecomment-5264765069
You are right, and it was not a deliberate trade-off — I only took the copy in the branch that destroys the value and did not think about the branch that does not. Fixed in f343e0c0. Measured against `master`, request carrying `X-Forwarded-For: 9.9.9.9`, no `trusted_addresses`: | | `master` | before the fix | after | |---|---|---|---| | `ctx.var.original_x_forwarded_for` | `9.9.9.9` | `""` | `9.9.9.9` | | `ctx.var.http_x_forwarded_for` | `9.9.9.9` | `9.9.9.9` | `9.9.9.9` | The copy now happens before the trust check, so it covers every path. TEST 19 pins the default one. Two notes on why it had to be Lua rather than another `set`, in case it comes up again: `$http_x_forwarded_for` is a dedicated entry in `ngx_http_core_variables[]`, not a prefix variable like `$http_x_forwarded_proto` and friends. Naming it anywhere in the configuration — `set`, `map`, `more_set_input_headers` — indexes it, and the rewrite-phase evaluation then pins the client's value in `r->variables[]` for the rest of the request. The untrusted-peer clear could not dislodge it, so route `vars`, `key_type: var` rate-limit keys and every other `ctx.var` reader would keep seeing the chain the trust boundary is supposed to remove. That is why the other four originals are `set` in the template and this one is not. Cost on the default path is one `ctx.var` read plus, when the client sent the header, one write. The other four are still taken by the configuration. Also worth recording for reviewers: on the default path nothing is actually lost even without this fix, because X-Forwarded-For is not cleared there and `ctx.var.http_x_forwarded_for` still holds the chain. The reason to fix it is source compatibility for plugins written against the old field, which is reason enough. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
