Fix: Strip query string from pctx.Path in ext_proc and ext_authz - #882
Conversation
pipeline.Context.Path meant different things depending on the listener: the forward and reverse proxies populate it from r.URL.Path (query-free, percent-decoded by net/http's parser), while ext_proc used the raw :path pseudo-header and ext_authz used AttributeContext.HttpRequest.path — both of which carry the full request target, query string included. Any plugin behavior keyed on Path therefore differed by deployment mode. Three consumers had already grown defensive strips (bypass matcher, tool-prune's gate, inference-parser's dialect dispatch), while others were still exposed: context-guru's suffix gate misses /v1/messages?beta=true under Envoy modes, OPA policies exact-matching input.path break only there, and ibac's judge prompt includes query parameters only there. Run the raw request target through url.ParseRequestURI — the same parser net/http runs for the proxy listeners — at pctx construction in both Envoy-fed listeners, so Path is byte-identical across listener modes. The invariant is documented on Context.Path and pinned by new tests in both fixed listeners (red before this change). inference-parser's defensive strip stays as defense in depth for contexts constructed outside a listener; its comment now reflects the guaranteed invariant. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: YehoshuaSagron <ysagron@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughExt_authz and ext_proc now populate ChangesPath normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Ext_authz and ext_proc now provide query-free, percent-decoded paths consistently across listener modes, with malformed targets retaining the documented query-strip fallback. Current coverage exercises the changed behaviors and no merge-blocking risk remains. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EnvoyOrAuthRequest
participant Listener
participant httpxPathOnly
participant Pipeline
EnvoyOrAuthRequest->>Listener: provide request target
Listener->>httpxPathOnly: normalize target
httpxPathOnly-->>Listener: decoded path without query
Listener->>Pipeline: store Context.Path
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
abigailgold
added
the
ready-for-ai-review
…ests Review-hardening pass on the previous commit: - Hoist pathOnly to a single exported httpx.PathOnly used by both Envoy-fed listeners. The function now defines a cross-listener invariant documented on pipeline.Context.Path; two private copies could drift silently. - Hedge the Context.Path and PathOnly doc comments: values are identical across listener modes modulo unparseable targets, which net/http rejects with 400 before any pipeline runs while the Envoy-fed listeners keep them query-stripped but otherwise raw. - Table-drive both listener tests and extend them to pin all three behaviors per listener: query strip, percent-decoding, and the unparseable-target fallback (previously uncovered — a regression there would have passed green). - Run each ext_proc test request on its own mock stream, matching the one-request-per-stream production shape instead of relying on incidental cross-request state handling. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: YehoshuaSagron <ysagron@gmail.com>
|
@abigailgold Thanks — both points acted on.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com |
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Fixes the listener divergence in #881 by running the raw request target through the same parser net/http gives the proxy listeners. Small, correctly scoped, honestly documented. No blocking findings.
Verified by running it, not by reading it
| Check | Result |
|---|---|
PathOnly ↔ net/http parity |
I extracted PathOnly verbatim and compared it against http.ReadRequest(…).URL.Path on the same wire bytes across 15 targets. Exact match on all 13 that net/http accepts, including the ones no test covers: encoded slash (/foo%2Fbar → /foo/bar), absolute-form (http://host/path?q=1 → /path), asterisk-form (*), //evil.com/x, /x?, /x#frag, /a//b/./c. The only two divergences are exactly the pair the doc calls out — /a%zz?secret=1 and "" — both of which net/http answers with 400 before any proxy-mode pipeline runs. The parity claim holds as stated |
| Completeness | All five sites that populate pctx.Path from a raw target are wrapped: extproc ×4 (handleInbound, handleInboundBody, handleOutbound, handleOutboundBody) and extauthz ×1 (at the path := assignment, so it covers both the inbound and outbound pctx). The three proxy sites already used r.URL.Path, and forwardproxy:1006 deliberately sets "" for CONNECT. Nothing left unwrapped |
| No wire impact | pctx.Path is never written back to Envoy as a :path header mutation, and no listener rebuilds a request URL from it — its only other listener use is one skip-host debug log. So the request forwarded upstream keeps its query intact; the change is confined to the pipeline's view |
| The tests are real regression guards | Ran both at the PR head (pass), then neutered PathOnly to return target to simulate main. All six sub-assertions failed, reporting exactly the pre-PR values (/api/x?secret=1, /api/hello%20world?secret=1&b=2, /a%zz?secret=1) for both inbound and outbound. Restored, re-passed. Then the full authlib suite (GOWORK=off go test ./...): all green |
| No consumer breaks | Nothing in the repo reads the request query today — the only url.Values uses are token-exchange RFC 8693 POST form bodies, unrelated to the request target. So dropping the query costs no current behavior, which matches the body's own framing |
.claude / .vscode gate; secrets |
no matches |
| CI | 23/23 pass; 2 commits, both signed off, both conventional prefixes |
This PR under-sells its own security win
The body credits the query no longer reaching "the ibac judge LLM." The exposure was wider than that. Invocation.Path is serialized (pipeline/extensions.go:389, json:"path,omitempty") and auto-populated from pctx.Path for every invocation record (pipeline/context.go:344-345, pipeline/pipeline.go:278). Invocation records are surfaced on /v1/sessions/{id} and /v1/events — the session API whose own documentation says "no authentication. Bind only on in-cluster addresses, never behind ingress."
So before this change, in envoy-sidecar and waypoint modes, a request to /api/x?token=SECRET put that query string into the session store and served it over an unauthenticated endpoint, and into roughly eight slog call sites besides (ibac, mcp-parser, context-guru, cpex, inference-parser). That is worth recording on #881, because it reframes the issue from a consistency bug into a secret-exposure one and is the strongest argument for merging.
One finding I checked and then dropped
I independently derived that this change also introduces percent-decoding to the two Envoy-fed listeners, which widens bypass_paths — an allow-list gating jwt-validation. I confirmed it against the real matcher: with pattern /healthz, Match("/%68ealthz") was false before and the decoded /healthz is true now. Then I found the body already documents it, with the same example, correctly framed as an alignment with proxy-sidecar's longstanding semantics. Recording that I verified it rather than took it on trust: the direction can only widen an allow-list, but it is not exploitable for escalation — a decoded path matching the bypass means the backend, which decodes the same way, also serves that bypass path. Aligning on the default mode's behavior is the right call.
The inference-parser decision is also right. Keeping the one-line strip as defense in depth for contexts built outside a listener costs nothing, and the comment now explains the silent failure it guards rather than the listener divergence that no longer exists.
Summary
Two non-blocking comments below: the shared helper wants its own table test, and one word in a field doc. Neither affects correctness.
Author: JoshSag (CONTRIBUTOR — returning external, elevated scrutiny; all 7 files read in full)
Areas reviewed: Go (listeners, pipeline contract, one plugin), tests, security (path-matching and session-API exposure)
Agent/IDE config (.claude/.vscode): none
Commits: 2, all signed off
CI status: 23/23 pass
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
| // (percent-decoding included), modulo targets that parser rejects: net/http | ||
| // answers those with 400 before any pipeline runs, while the Envoy-fed | ||
| // listeners fall back to a plain query strip. | ||
| func PathOnly(target string) string { |
There was a problem hiding this comment.
suggestion — this is the shared helper both fixed listeners now depend on, and it is the one new thing here without a direct test. Its sibling in this same package has one (render.go → render_test.go), so the convention is already established.
The two listener tests are good, but they are integration tests that happen to exercise PathOnly through a pipeline, and they pin the same three cases in both files. That leaves behaviors this function actually implements untested anywhere:
| target | PathOnly |
why it matters |
|---|---|---|
http://host/path?q=1 |
/path |
absolute-form. A real behavior change: pre-PR the entire absolute URL landed in pctx.Path. Nothing pins the fix |
/foo%2Fbar |
/foo/bar |
encoded slash — the decoding case with security relevance, since it is what bypass_paths now matches on |
* |
* |
OPTIONS *. Worth pinning that it is passed through rather than mangled |
"" |
"" |
empty target takes the error branch, not the happy path — the only fallback case tested today is /a%zz |
/x? |
/x |
empty query, an off-by-one on the IndexByte fallback |
I verified every row against http.ReadRequest(…).URL.Path on the same wire bytes: all five match exactly, so the table is ready to assert as-is and doubles as executable documentation of the parity claim your doc comment makes. That claim is currently asserted only in prose, and prose does not fail CI when someone "simplifies" url.ParseRequestURI into a strings.Cut.
Second, smaller point: pathCapture is byte-identical in extproc/server_path_test.go and extauthz/server_path_test.go — same five methods, same comment. Both files already import plugins/plugintesting, so it could live there as one exported helper and serve the next listener test too. Not worth a round trip on its own, but if you are touching these files for the table above it is free.
| // is identical across listener modes — modulo unparseable targets, | ||
| // which net/http rejects with 400 before any pipeline runs and the | ||
| // Envoy-fed listeners keep query-stripped but otherwise raw. | ||
| // Plugins may match, log, or feed Path into policy without |
There was a problem hiding this comment.
nit — this doc is the right place for the invariant, and it is the one place that does not say the value is percent-decoded.
The mechanism is stated (httpx.PathOnly → url.ParseRequestURI), so a reader who knows what that parser does can infer it. But this sentence actively invites plugin authors to "match, log, or feed Path into policy," and decoding is precisely the property that changes what a matcher sees — it is what makes /%68ealthz match a /healthz pattern, the example your own PR body leads with. Someone writing a new path matcher reads this field doc, not httpx/path.go and not the PR description.
One clause carries it:
// Path is the URL path of the request, percent-decoded and never
// including a query string: the proxy listeners populate it from
// r.URL.Path, and ext_proc / ext_authz run the raw request target
// through the same URL parser …Worth a second clause on the consequence, since this is an auth-relevant surface: something like "so a pattern is matched against the decoded path — /%68ealthz matches /healthz." That is the sentence that stops a future bypass_paths or policy author from assuming they are matching raw wire bytes.
While here: the fallback description — "the Envoy-fed listeners keep query-stripped but otherwise raw" — is accurate and worth keeping, but it is the one branch where Path is not decoded. Naming that asymmetry explicitly ("and therefore not decoded") closes the loop, because a matcher author's mental model needs to hold both states.
|
Addendum to my approval — two things I verified after submitting. Neither changes the verdict; both are additive. 1. This PR fixes a second latent bug it does not claim
// :273 — gated
for _, s := range p.cfg.Paths {
if path == s || strings.HasSuffix(path, s) { return true }
}
// :284 — providerFor
if strings.HasSuffix(path, "/v1/messages") { return bschemas.Anthropic }
return bschemas.OpenAIPre-PR, under ext_proc,
That is the same failure pair Worth recording on #881: it moves this from "one plugin had a workaround" to "a second plugin was actively broken in envoy-sidecar mode," which is a stronger justification than the issue currently carries. It also sharpens the 2. A correction to the follow-up framing in the description
True for the forward/reverse proxy listeners. Not literally true for the Envoy-fed ones. Two consequences worth writing down:
Separately, I checked whether this hatch undercuts the secret-exposure fix: it does not. Assisted-By: Claude (Anthropic AI) noreply@anthropic.com |
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
4 participants
Fixes #881.
The problem
pipeline.Context.Pathdiverged by listener mode: the proxies parse the requesttarget (
r.URL.Path— query-free, percent-decoded), while ext_proc and ext_authzpassed the raw target through, query string included. Full table, timeline, and
impact in #881.
The change
Run the raw request target through
url.ParseRequestURI— the same parsernet/httpruns for the proxy listeners — at pctx construction in both Envoy-fed listeners
(shared helper
httpx.PathOnly).Pathis now identical across all four listenermodes, decoding included:
/api/hello%20world?x=1yields/api/hello worldeverywhere. Targets that parser rejects (which
net/httpanswers with 400 before anyproxy-mode pipeline runs) keep a plain query-strip fallback — no worse than today.
Two behavior notes for Envoy-mode deployments, both alignments with proxy-sidecar's
longstanding semantics:
Decoding affects matching:
/%68ealthznow matches a/healthzbypass/policypattern, as it always has under the proxies.
The raw query — which routinely carries tokens and secrets — no longer reaches the
ibac judge LLM; the flip side is that policy loses query visibility entirely until
the
Queryfield lands (follow-up below).ext_proc:
Path: httpx.PathOnly(getHeader(headers, ":path"))at all fourconstruction sites.
ext_authz:
path := httpx.PathOnly(httpReq.GetPath()).pipeline.Context.Pathdoc comment now states the invariant, so plugins maymatch/log/policy-feed
Pathwithout stripping a query themselves.inference-parser's one-line defensive strip is kept (defense in depth forcontexts constructed outside a listener; the failure mode it guards is silent),
with its comment updated to reflect the new invariant. The other existing strips
(bypass matcher, tool-prune) are untouched.
No behavior change for the forward/reverse proxy listeners.
Evidence
New table-driven tests
TestExtProc_PathMatchesProxyListenersandTestCheck_PathMatchesProxyListenersdrive each fixed listener through a captureplugin and pin all three behaviors per listener — query strip (
/api/x?secret=1),percent-decoding (
/api/hello%20world?secret=1&b=2), and the unparseable-targetfallback (
/a%zz?secret=1) — assertingpctx.Pathholds exactly what the proxylisteners produce for the same wire bytes.
On
main(before):On this branch: both pass, and the same capture-plugin probe run against the forward
and reverse proxy listeners confirms they already produced these values. Full
go vet ./... && go test -count=1 -race ./listener/... ./pipeline/... ./plugins/...green.
Proposed follow-up (not in this PR)
Plugins that legitimately need query parameters currently have no channel for them —
the proxies drop the query on the floor. A follow-up could add a
Query stringfieldto
pipeline.Context, populated by all four listeners (fromr.URL.RawQuery/ therequest target), so query-aware plugins opt in explicitly instead of parsing
Path.This PR deliberately only restores the invariant; adding the field is a separate,
additive decision.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Documentation