Skip to content

Fix: Strip query string from pctx.Path in ext_proc and ext_authz #882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
huang195 merged 2 commits into from
Sep 8, 2026
+239 −20
Merged

Fix: Strip query string from pctx.Path in ext_proc and ext_authz #882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e1e66b
Fix: Strip query string from pctx.Path in ext_proc and ext_authz
JoshSag Sep 6, 2026
29d8be3
Fix: Harden pctx.Path parity — shared helper, hedged docs, fallback t…
JoshSag Sep 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions authbridge/authlib/listener/extauthz/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
rpcstatus "google.golang.org/genproto/googleapis/rpc/status"

"github.com/rossoctl/cortex/authbridge/authlib/auth"
"github.com/rossoctl/cortex/authbridge/authlib/listener/httpx"
"github.com/rossoctl/cortex/authbridge/authlib/pipeline"
)

Expand Down Expand Up @@ -44,7 +45,7 @@ func (s *Server) Check(ctx context.Context, req *authv3.CheckRequest) (*authv3.C
if host == "" {
host = headers["host"]
}
path := httpReq.GetPath()
path := httpx.PathOnly(httpReq.GetPath())
scheme := httpReq.GetScheme()

// Inbound validation via pipeline
Expand Down Expand Up @@ -121,7 +122,6 @@ func mapToHTTPHeader(m map[string]string) http.Header {
return h
}


// deniedFromAction renders a pipeline Reject as an ext_authz CheckResponse
// preserving the plugin's status, headers, and body. The flat
// {"error":reason} body of the old denied() is gone — plugins can now
Expand Down
86 changes: 86 additions & 0 deletions authbridge/authlib/listener/extauthz/server_path_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package extauthz

import (
"context"
"testing"

authv3 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3"

"github.com/rossoctl/cortex/authbridge/authlib/pipeline"
"github.com/rossoctl/cortex/authbridge/authlib/plugins/plugintesting"
)

// pathCapture records the pctx.Path each pipeline run sees, so tests can
// assert on what the listener actually constructed rather than on side
// effects of a real plugin.
type pathCapture struct {
paths []string
}

func (p *pathCapture) Name() string { return "path-capture" }
func (p *pathCapture) Capabilities() pipeline.PluginCapabilities {
return pipeline.PluginCapabilities{}
}
func (p *pathCapture) OnRequest(_ context.Context, pctx *pipeline.Context) pipeline.Action {
p.paths = append(p.paths, pctx.Path)
return pipeline.Action{Type: pipeline.Continue}
}
func (p *pathCapture) OnResponse(_ context.Context, _ *pipeline.Context) pipeline.Action {
return pipeline.Action{Type: pipeline.Continue}
}

// Envoy's AttributeContext.HttpRequest.path is "the request target, as it
// appears in the first line of the HTTP request" — query string included.
// pctx.Path must contain only the URL path, exactly as the proxy listeners
// produce it from r.URL.Path (query dropped, percent-decoding applied), so
// plugin behavior keyed on Path cannot differ by listener mode. An
// unparseable target keeps the plain query-strip fallback.
func TestCheck_PathMatchesProxyListeners(t *testing.T) {
cases := []struct {
name string
target string
want string
}{
{"query stripped", "/api/x?secret=1", "/api/x"},
{"percent-decoded", "/api/hello%20world?secret=1&b=2", "/api/hello world"},
{"unparseable falls back to query strip", "/a%zz?secret=1", "/a%zz"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
inCap, outCap := &pathCapture{}, &pathCapture{}
inbound, err := plugintesting.BuildPipeline([]pipeline.Plugin{inCap})
if err != nil {
t.Fatalf("building inbound pipeline: %v", err)
}
outbound, err := plugintesting.BuildPipeline([]pipeline.Plugin{outCap})
if err != nil {
t.Fatalf("building outbound pipeline: %v", err)
}
srv := &Server{
InboundPipeline: pipeline.NewHolder(inbound),
OutboundPipeline: pipeline.NewHolder(outbound),
}

req := &authv3.CheckRequest{
Attributes: &authv3.AttributeContext{
Request: &authv3.AttributeContext_Request{
Http: &authv3.AttributeContext_HttpRequest{
Headers: map[string]string{":authority": "target-svc"},
Path: tc.target,
},
},
},
}
if _, err := srv.Check(context.Background(), req); err != nil {
t.Fatalf("Check: %v", err)
}

if len(inCap.paths) != 1 || inCap.paths[0] != tc.want {
t.Errorf("inbound pctx.Path = %q, want [%q]", inCap.paths, tc.want)
}
if len(outCap.paths) != 1 || outCap.paths[0] != tc.want {
t.Errorf("outbound pctx.Path = %q, want [%q]", outCap.paths, tc.want)
}
})
}
}
8 changes: 4 additions & 4 deletions authbridge/authlib/listener/extproc/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func (s *Server) handleInbound(stream extprocv3.ExternalProcessor_ProcessServer,
Direction: pipeline.Inbound,
Method: getHeader(headers, ":method"),
Scheme: getHeader(headers, ":scheme"),
Path: getHeader(headers, ":path"),
Path: httpx.PathOnly(getHeader(headers, ":path")),
Headers: headerMapToHTTP(headers),
Body: body,
Shared: s.Shared,
Expand All @@ -186,7 +186,7 @@ func (s *Server) handleInboundBody(stream extprocv3.ExternalProcessor_ProcessSer
Direction: pipeline.Inbound,
Method: getHeader(headers, ":method"),
Scheme: getHeader(headers, ":scheme"),
Path: getHeader(headers, ":path"),
Path: httpx.PathOnly(getHeader(headers, ":path")),
Headers: headerMapToHTTP(headers),
Body: body,
Shared: s.Shared,
Expand Down Expand Up @@ -471,7 +471,7 @@ func (s *Server) handleOutbound(stream extprocv3.ExternalProcessor_ProcessServer
Method: getHeader(headers, ":method"),
Scheme: getHeader(headers, ":scheme"),
Host: authorityOf(headers),
Path: getHeader(headers, ":path"),
Path: httpx.PathOnly(getHeader(headers, ":path")),
Headers: headerMapToHTTP(headers),
Body: body,
Shared: s.Shared,
Expand Down Expand Up @@ -513,7 +513,7 @@ func (s *Server) handleOutboundBody(stream extprocv3.ExternalProcessor_ProcessSe
Method: getHeader(headers, ":method"),
Scheme: getHeader(headers, ":scheme"),
Host: authorityOf(headers),
Path: getHeader(headers, ":path"),
Path: httpx.PathOnly(getHeader(headers, ":path")),
Headers: headerMapToHTTP(headers),
Body: body,
Shared: s.Shared,
Expand Down
100 changes: 100 additions & 0 deletions authbridge/authlib/listener/extproc/server_path_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
package extproc

import (
"context"
"testing"

extprocv3 "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3"

"github.com/rossoctl/cortex/authbridge/authlib/pipeline"
"github.com/rossoctl/cortex/authbridge/authlib/plugins/plugintesting"
)

// pathCapture records the pctx.Path each pipeline run sees, so tests can
// assert on what the listener actually constructed rather than on side
// effects of a real plugin.
type pathCapture struct {
paths []string
}

func (p *pathCapture) Name() string { return "path-capture" }
func (p *pathCapture) Capabilities() pipeline.PluginCapabilities {
return pipeline.PluginCapabilities{}
}
func (p *pathCapture) OnRequest(_ context.Context, pctx *pipeline.Context) pipeline.Action {
p.paths = append(p.paths, pctx.Path)
return pipeline.Action{Type: pipeline.Continue}
}
func (p *pathCapture) OnResponse(_ context.Context, _ *pipeline.Context) pipeline.Action {
return pipeline.Action{Type: pipeline.Continue}
}

func captureServer(t *testing.T) (*Server, *pathCapture, *pathCapture) {
t.Helper()
inCap, outCap := &pathCapture{}, &pathCapture{}
inbound, err := plugintesting.BuildPipeline([]pipeline.Plugin{inCap})
if err != nil {
t.Fatalf("building inbound pipeline: %v", err)
}
outbound, err := plugintesting.BuildPipeline([]pipeline.Plugin{outCap})
if err != nil {
t.Fatalf("building outbound pipeline: %v", err)
}
return &Server{
InboundPipeline: pipeline.NewHolder(inbound),
OutboundPipeline: pipeline.NewHolder(outbound),
}, inCap, outCap
}

// The :path pseudo-header carries the full request target, query string
// included. pctx.Path must contain only the URL path, exactly as the
// forward and reverse proxy listeners produce it from r.URL.Path (query
// dropped, percent-decoding applied) — so plugin behavior keyed on Path
// cannot differ by listener mode. An unparseable target (which net/http
// would reject with 400 before any pipeline runs) keeps the plain
// query-strip fallback.
func TestExtProc_PathMatchesProxyListeners(t *testing.T) {
cases := []struct {
name string
target string
want string
}{
{"query stripped", "/api/x?secret=1", "/api/x"},
{"percent-decoded", "/api/hello%20world?secret=1&b=2", "/api/hello world"},
{"unparseable falls back to query strip", "/a%zz?secret=1", "/a%zz"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
// One mock stream per request — each ext_proc stream carries a
// single request in production.
srv, inCap, outCap := captureServer(t)
inStream := &mockStream{
ctx: context.Background(),
requests: []*extprocv3.ProcessingRequest{
inboundRequest(makeHeaders(
"x-authbridge-direction", "inbound",
":path", tc.target,
)),
},
}
_ = srv.Process(inStream)
outStream := &mockStream{
ctx: context.Background(),
requests: []*extprocv3.ProcessingRequest{
outboundRequest(makeHeaders(
":authority", "target-svc",
":path", tc.target,
)),
},
}
_ = srv.Process(outStream)

if len(inCap.paths) != 1 || inCap.paths[0] != tc.want {
t.Errorf("inbound pctx.Path = %q, want [%q]", inCap.paths, tc.want)
}
if len(outCap.paths) != 1 || outCap.paths[0] != tc.want {
t.Errorf("outbound pctx.Path = %q, want [%q]", outCap.paths, tc.want)
}
})
}
}
26 changes: 26 additions & 0 deletions authbridge/authlib/listener/httpx/path.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package httpx

import (
"net/url"
"strings"
)

// PathOnly extracts the URL path from a raw request target. The Envoy-fed
// listeners (ext_proc's :path pseudo-header, ext_authz's
// AttributeContext.HttpRequest.path) receive the full request target, query
// string included, but pctx.Path must hold only the path — see
// pipeline.Context.Path. It runs the same parser net/http runs for the
// proxy listeners, so pctx.Path is identical across listener modes
// (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 {

@huang195 huang195 Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

u, err := url.ParseRequestURI(target)
if err != nil {
if i := strings.IndexByte(target, '?'); i >= 0 {
return target[:i]
}
return target
}
return u.Path
}
17 changes: 14 additions & 3 deletions authbridge/authlib/pipeline/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,20 @@ type Context struct {
// fixtures, an unrecognized transport, etc.). Plugins that need a
// concrete scheme should pick a default explicitly — treating ""
// as "assume http" would silently mask missing listener plumbing.
Scheme string
Host string
Path string
Scheme string
Host string

// Path is the URL path of the request, 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 (httpx.PathOnly → url.ParseRequestURI), so the value
// 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

@huang195 huang195 Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// stripping a query themselves.
Path string

Headers http.Header
Body []byte // nil unless at least one plugin declares BodyAccess: true

Expand Down
18 changes: 7 additions & 11 deletions authbridge/authlib/plugins/inferenceparser/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,13 @@ func (p *InferenceParser) Capabilities() pipeline.PluginCapabilities {

// endpointPath returns pctx.Path with any query string removed.
//
// The listeners disagree on what Path holds, and dialect dispatch below is
// exact-match, so this has to be normalised in one place. The HTTP listeners
// set Path from r.URL.Path, which already excludes the query; extproc sets it
// from the HTTP/2 :path pseudo-header, which per RFC 9113 §8.3.1 includes it.
//
// Claude Code posts to /v1/messages?beta=true, so without this the request
// falls to the default arm on the envoy-sidecar path and the parser records no
// inference telemetry at all — and once OnRequest did match, the four
// dialect-selection sites below would send an Anthropic stream to the OpenAI
// parser. Both failure modes are silent, which is why every site normalises
// rather than only the dispatch switch.
// Every listener now guarantees Path is query-free (see pipeline.Context.Path),
// so this is defense in depth for contexts constructed outside a listener
// (tests, future transports). It stays because the failure mode it guards is
// silent: Claude Code posts to /v1/messages?beta=true, and with a query
// attached the exact-match dialect dispatch below falls to the default arm and
// records no inference telemetry at all — or worse, sends an Anthropic stream
// to the OpenAI parser.
func endpointPath(pctx *pipeline.Context) string {
path, _, _ := strings.Cut(pctx.Path, "?")
return path
Expand Down
Loading