refactor(ai-providers): make the LLM request client ctx-free#13699
Open
AlinsRan wants to merge 6 commits into
Open
refactor(ai-providers): make the LLM request client ctx-free#13699AlinsRan wants to merge 6 commits into
AlinsRan wants to merge 6 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
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
1 participant
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).Drop the
ctxparameter. Everything derived from the downstream request or from request state is now resolved by the caller and passed viaopts:target_protocolctx.ai_target_protocolheader_transformctx.ai_converter.convert_headersaccess_tokenfetch_gcp_access_token(ctx, ...)(ctx keyed its cache)method/client_argscore.request.get_method()/ctx.var.args(passthrough)raw_request_bodyctx.ai_raw_request_body/core.request.get_body()client_headerscore.request.headers(ctx)inside the transportProtocol conversion moves to the caller as well: converters stash state on
ctxfor the response side to read back, soai-proxyconverts and passes the converted body.ai-proxyalso still readsctx.ai_request_body_changed— the cross-plugin "an earlier plugin rewrote the body" signal — and simply withholdsraw_request_bodythen, which preserves the raw-body reuse optimisation (#1597) without the transport knowing aboutctx._M.requestonly ever forwardedctxtobuild_request, so it is ctx-free too — a genuine pure client for internal callers.Also:
redact_extra_opts()now stripsclient_headers, so the forwarded headers don't reach the info-levelrequest extra_optslog line.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)from_M.requestand parse it themselves.Tests
New
t/plugin/ai-transport-header-forwarding.t: the internal rewrite call reaches the LLM with its own credentials and no clientCookie, while the transparent proxy path still forwards it; the client's secret never appears in any log. Both assertions fail against the pre-fix code.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).build_requestunit tests updated to the new signature.