refactor(ai-providers): make the LLM request client ctx-free#13699
Open
AlinsRan wants to merge 12 commits into
Open
refactor(ai-providers): make the LLM request client ctx-free#13699AlinsRan wants to merge 12 commits into
AlinsRan wants to merge 12 commits into
Conversation
The HTTP transport's construct_forward_headers pulled the downstream request's headers straight from ctx and always forwarded them. That is correct for the transparent proxy path (ai-proxy / ai-proxy-multi) but wrong for plugins that make a self-contained internal LLM call: ai-request-rewrite forwarded the client's Authorization / Cookie to the third-party LLM endpoint used to rewrite the body — a header leak. Make the transport ctx-free: construct_forward_headers now takes an explicit client_headers table (nil for internal requests) instead of reading core.request.headers(ctx). build_request forwards opts.client_headers, and only the proxy path (ai-proxy/base.lua) sets it. Internal callers (ai-request-rewrite, and any future embeddings/RAG/moderation call) pass none, so their own credentials — not the client's — reach the LLM. Add a test asserting the internal rewrite call receives no client Cookie while the transparent proxy path still forwards it.
dosubot
Bot
added
size:L
The proxy path now carries the client's request headers in extra_opts (client_headers) so ai-proxy can forward them. But build_request logs extra_opts at info level via redact_extra_opts(), which only stripped auth — so the client's Cookie/Authorization would be written to the log. Strip client_headers in redact_extra_opts too, closing the log-side leak that mirrors the network-side one. Extend the test: the mock LLM now also records Authorization; assert the internal rewrite call reaches the LLM with its own credentials (not the client's) and that the client's secret never appears in any log, including the extra_opts info line, on the proxy path.
build_request/request took the request ctx and reached into the downstream request through it, so an internal caller had to fake a ctx to use them. None of what ctx carried is meaningful for a self-contained LLM call. Drop the ctx parameter. Everything derived from the downstream request or from request state is now resolved by the caller and passed via opts: target_protocol was ctx.ai_target_protocol header_transform was ctx.ai_converter.convert_headers access_token was fetch_gcp_access_token(ctx, ...) (ctx keyed its cache) method/client_args was core.request.get_method() / ctx.var.args (passthrough) raw_request_body was ctx.ai_raw_request_body / core.request.get_body() client_headers (already) Protocol conversion moves to the caller too: converters stash state on ctx for the response side to read back, so ai-proxy converts and passes the converted body. ai-proxy also keeps reading ctx.ai_request_body_changed -- the cross-plugin 'an earlier plugin rewrote the body' signal -- and simply withholds raw_request_body then, which preserves the raw-body reuse optimisation without the transport knowing about ctx. ai-request-rewrite now calls the provider as a plain client and resolves its own GCP token; it sets ai_request_body_changed where it actually rewrites the body, for later plugins rather than for its own call. _M.request only ever forwarded ctx to build_request, so it is now ctx-free too: a genuine pure client for internal callers.
AlinsRan
changed the title
The new build_request test block broke the blank-line spacing utils/reindex enforces; run it and take its renumbering (the file's existing 3a/4b/5c convention continues as 6d).
…ient
The ctx removal replaced one opaque parameter with four flat fields
(client_headers, client_args, method, raw_request_body) scattered through an
already-large opts table. That named no concept, let a caller half-set the
group, and made redaction a matter of remembering each field -- which had
already gone wrong: opts.raw_request_body put the client's verbatim body (the
user's prompt) into the info-level 'request extra_opts' log line, since
redact_extra_opts only stripped auth and client_headers.
Group them under a single opts.client:
client = { headers, args, method, raw_body }
Its presence is now what marks a call as proxying an inbound request; an
internal call omits it entirely, so nothing of the client's can reach the LLM
or the logs. redact_extra_opts drops the one key and with it every piece of
downstream data, instead of enumerating fields.
Extend the proxy test to assert the client's prompt never appears in any log;
it fails without the redaction.
…quest
build_request did three jobs at once: shape the body for the target protocol,
assemble the HTTP request, and sign it. The seam between them followed an
implementation detail rather than a concept -- it knew about target_protocol and
mutated the body in four places, yet protocol *conversion* sat outside, purely
because that one function takes ctx. A reader could not state what the function's
job was.
Every protocol-driven decision in there turned out to be a body decision: nothing
protocol-shaped touched the URL, headers, method or signing. So split by what each
layer produces:
build_body(request_body, opts) -> body, changed
prepare_outgoing_request / model_options / llm_options(capability)
/ request_body_override_map / remove_model. This is where the target
protocol matters.
build_request(conf, body, opts) -> params
endpoint, auth, headers, method, query, SigV4 last. Protocol-agnostic;
the body is passed through as given.
request(conf, request_table, opts) = build_body + build_request + send
The raw-body reuse rule goes with it. The transport already sends a string body
verbatim and encodes a table, so 'reuse the client's bytes' is just the caller
passing the string -- an explicit line at the call site instead of an implicit
contract (client.raw_body plus an internal body_changed) that the caller had to
know build_request's internals to use correctly. opts.client now carries only
downstream facts: headers, args, method.
Retarget the build_request unit tests at the new contract (pass-through) and add
one for build_body's changed flag, which is what the reuse decision hangs on.
dosubot
Bot
added
size:XL
….client Review follow-ups. fetch_gcp_access_token took a ctx only to build its cache key via plugin_ctx_id(ctx, name) -- i.e. route id + conf version, which says nothing about the token. Moving the call out to each caller (so the provider could be ctx-free) therefore bought nothing and duplicated the same block in every plugin that supports a gcp provider. Key it on the credentials instead: the token depends only on the service account, so identical credentials now share one token across routes and the lookup needs no ctx. The call moves back inside build_request and no caller repeats it. The key keeps its '<...>#<name>' shape, which the cache-hit test asserts on. Rename opts.client to opts.downstream: it is not a client object, it is the data taken from the inbound request (headers, args, method). The grouping stays -- its presence is what marks a call as proxying, and redact_extra_opts drops the one key to keep all of it out of the logs -- but the name no longer suggests an HTTP client. Keep protocol conversion inside the pcall boundary. Moving it out of do_request took it outside the pcall that bounds a request attempt, which is reachable with input-controlled but valid JSON: an Anthropic image block whose "source" is a boolean makes convert_media_block index a boolean, and the exception then escapes before_proxy instead of becoming a controlled 500. Run it inside do_request again, keeping its returned-400 path, and add a regression test driving exactly that payload. Assert both build_request calls in the auth.query mutation test; a build failure would otherwise leave auth_query untouched and still report OK.
The nesting was justified on two grounds; only one held up. It did not make the group atomic: build_request reads each field with its own nil check (`downstream and downstream.headers`, `... and downstream.method`), so a caller could always set some and not others. The nesting made them look like one unit without making them behave as one. What it did buy was redaction: one `redacted.downstream = nil` kept every client-derived field out of the logs, and a field added later was covered for free. Keep that property without the nesting by naming the fields in log-sanitize instead -- CLIENT_DERIVED_FIELDS is an explicit list that says out loud which options are sensitive, which reads better than an implicit 'anything under this key'. The header-forwarding test covers it: drop a field from the list and it fails. opts.client_headers / client_args / client_method now sit directly on opts, which is what they are -- static options taken from the inbound request, not a client object.
The denylist only protected the fields someone remembered to name. That is the failure mode that already happened once in this branch: raw_request_body was added to extra_opts and the user's prompt went into the info log, because the sanitizer stripped only auth. Flattening opts.client_* back onto opts made that worse -- the nested form at least covered every field under one key. Invert it: name the fields that are safe to log (connection/routing shape and non-secret provider config) and drop everything else. A field added to extra_opts later is now redacted by default rather than leaked by default. redact_params right below already works this way; this makes the two consistent. Also drops the per-request deepcopy -- the allowlist copies a dozen scalars instead.
Neither list was the right answer. A denylist leaked whatever nobody remembered to name -- it missed client_headers and then raw_request_body inside this branch alone. An allowlist fixed the default but made log-sanitize track the caller's whole field vocabulary: which options exist is the caller's business, only what counts as a secret is the sanitizer's. The real problem was serialising an open-ended options bag into a log line at all. Most of what it printed is already logged from the resolved `params` further down (endpoint, path, host, headers -- the last with per-header redaction), so the dump was largely duplicate. Log the three things it uniquely carried -- instance name, target protocol, model -- by name. redact_extra_opts has no callers left and goes with it; log-sanitize is back to one job, redacting the params it is handed. A field added to extra_opts later is now private by default with no list to maintain.
The security fix is the build_request log-site change -- it stopped dumping the whole options bag and logs three named values (instance, target_protocol, model) instead, which is what closed the leak. Deleting redact_extra_opts on top of that bought nothing: it is a small, tested, exported util in a shared module, its removal only enlarged a security diff, dropped a used require, and forced deleting a passing test. It also left HEAD internally inconsistent -- the function gone but t/utils/log-sanitize.t still calling it -- which would break CI. Restore log-sanitize.lua and t/utils/log-sanitize.t to their master state. The log-site change stands on its own.
Member
|
[P2] GCP token cache key can alias different credentials and TTL policies The new cache key uses Please use a collision-resistant digest of the resolved credentials and all fields that affect cache reuse, without logging plaintext secrets, and add coverage for same-name configurations with different TTL policies. |
The ctx-free refactor keyed the GCP access-token cache on ngx.crc32_long(service_account_json) .. name. A 32-bit CRC can collide, so distinct credentials could reuse the wrong cached token; and because the key omitted expire_early_secs and max_ttl, identical credentials under different TTL policies shared one entry and the second policy was bypassed. Key on a 128-bit digest of the resolved credentials plus those TTL fields. The raw JSON still never enters the key or the log.
Contributor
Author
|
Fixed here + upstream #13699. Key is now Regression test (vertex-ai TEST 13): same name+creds with different md5 follows the existing cache-key convention here; can move to SHA-256 if you prefer. |
nic-6443
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
ai-providers/base.lua'sbuild_request/requesttook the requestctxand reached into the downstream request through it. That coupling was wrong in two ways:construct_forward_headerspulledcore.request.headers(ctx)and always forwarded them, soai-request-rewrite— which calls an LLM to rewrite the body — sent the client'sAuthorization/Cookieto that third-party endpoint.ctxcarried is meaningful for an internal call, so callers had to pass a throwaway ctx; anauth.gcpconfig then blew up onplugin_ctx_id(fake ctx).So: drop the
ctxparameter, and while the seams are open, cut them where the concepts actually are.The client is now three layers, split by what each produces
build_requestused to do all three jobs at once. Its seam followed an implementation detail rather than a concept: it knew abouttarget_protocoland mutated the body in four places, yet protocol conversion sat outside — purely because that one function takesctx. A reader could not state what the function's job was.Every protocol-driven decision in there turned out to be a body decision: nothing protocol-shaped touched the URL, headers, method or signing. So
build_bodyowns the protocol, andbuild_requestno longer knows protocols exist.What the caller now supplies
Everything taken from the downstream request is grouped under one key — its presence is what marks a call as proxying an inbound request:
client.headerscore.request.headers(ctx)inside the transportclient.method/client.argscore.request.get_method()/ctx.var.args(passthrough)target_protocolctx.ai_target_protocolheader_transformctx.ai_converter.convert_headersaccess_tokenfetch_gcp_access_token(ctx, ...)(ctx only keyed its cache)Protocol conversion stays with the caller: converters stash state on
ctxfor the response side to read back, soai-proxyconverts and hands over the converted body.The raw-body reuse optimisation (#1597) is preserved but no longer implicit. The transport already sends a string body verbatim and encodes a table, so "reuse the client's bytes" is just the caller passing the string — one visible line at the call site instead of a contract (
raw_request_bodyplus an internalbody_changed) that the caller had to knowbuild_request's internals to use correctly.Call flow
Proxy path
Internal call (
ai-request-rewrite)ctx.ai_request_body_changedstays: it is the cross-plugin "an earlier plugin rewrote the body" signal that the raw-body reuse optimisation depends on. It is now read byai-proxy— which legitimately holdsctx— rather than by the transport.Logging
redact_extra_opts()drops the wholeclientkey, so the downstream headers (Authorization/Cookie), query args and verbatim body (the user's prompt) never reach the info-levelrequest extra_optslog line. Dropping one key drops all of it, instead of enumerating fields.Scope
Request side only.
parse_response/parse_streaming_responsekeepctxby design — there it is an output sink (ngx.status,ctx.var.llm_*) and the downstream plugin chain (lua_response_filterruns each plugin'slua_body_filteroffapi_ctx.plugins). Internal callers never reach that path; they take(status, raw_body)and parse it themselves.Still deliberately left in place:
opts.header_transform.convert_headersmust run before SigV4 signs, and signing is last insidebuild_request— exposingsign()to callers so the hook could move out would trade a small wart for "someone forgets to sign".Tests
New
t/plugin/ai-transport-header-forwarding.t: the internal rewrite call reaches the LLM with its own credentials and no clientCookie; the transparent proxy path still forwards it; the client'sAuthorizationand prompt never appear in any log. Each assertion fails against the pre-fix code.build_requestunit tests retargeted at the new contract (body passed through untouched, string sent verbatim), plus a new one forbuild_body'schangedflag — the flag the reuse decision hangs on.Existing suites covering the moved logic all pass:
ai-proxy-request-body-override(raw-body reuse),ai-proxy-protocol-conversion(converter),ai-proxy-passthrough(method/query),ai-proxy-bedrock(SigV4 signs the finalized body last),ai-proxy-vertex-ai(GCP token),ai-proxy-multi.balancer.