mirror of
https://github.com/zap-proto/zip.git
synced 2026-10-19 15:38:57 +00:00
an argument addresses a resource, and may not address a route
Remote.Invoke percent-encodes every path argument (urlEscape) precisely so that
what a caller passes as a value cannot become part of the path. fasthttp then
undid it: its default serialisation is appendQuotedPath(u.Path()), and Path()
has already decoded %2F and resolved "..".
Invoke(GET /v1/platform/apps/:app, {"app": "../../../v1/iam/users"})
→ GET /v1/iam/users
One operation's ARGUMENT addressing a different operation entirely, on whatever
credential the caller attached. Every decision made above this line — which
command was named, whether that command may run, whether it mutates — is made
about a request that is not the one sent.
Two settings, because two things have to be told and only one of them is read
per transport:
Remote.do sets it on the REQUEST, so RequestURI() serialises PathOriginal() —
the bytes Invoke built. This is what carries the property over ZAP (the
default transport) and anything RegisterTransport adds.
transport.go sets it on the http and https HostClients, because
HostClient.doNonNilReqResp ASSIGNS req.URI().DisablePathNormalizing from its
own field (fasthttp client.go:1631). On those two transports the request's own
setting is overwritten, and the client's is the only one that survives. The
request-level line alone measurably does not hold — that is how this was found.
Pinned on a live server across the cases that matter: a plain id arrives
unchanged, "../../.." arrives escaped as one segment, a bare "/" arrives as
%2F, a pre-encoded %2F arrives as %252F rather than collapsing a level per hop,
and a bodyless op's flags still ride the query string untouched.
Co-authored-by: Hanzo Dev <[email protected]>
This commit is contained in:
@@ -502,6 +502,26 @@ func (r Remote) do(ctx context.Context, method, path string, body []byte) ([]byt
|
||||
req.SetRequestURI(path)
|
||||
req.SetHost(host)
|
||||
req.Header.SetMethod(method)
|
||||
// THE ESCAPING MUST SURVIVE THE TRANSPORT. Invoke percent-encodes every path
|
||||
// argument (urlEscape) precisely so an argument cannot become part of the
|
||||
// path, and fasthttp then undid it: its default serialisation is
|
||||
// appendQuotedPath(u.Path()), and Path() has already decoded %2F and resolved
|
||||
// "..". So `Invoke(cmd, {"app": "../../../v1/iam/users"})` on
|
||||
// GET /v1/platform/apps/:app left as GET /v1/iam/users — one op's argument
|
||||
// addressing another op entirely, on the caller's own credential.
|
||||
//
|
||||
// Set here so RequestURI() serialises PathOriginal(), the bytes Invoke built.
|
||||
// It is stated AGAIN on the http/https HostClients (transport.go), and that is
|
||||
// not a belt on a brace: HostClient.doNonNilReqResp ASSIGNS this field from
|
||||
// its own, so for those two transports the request's own wish is overwritten
|
||||
// and only the client's setting is read. This line is what carries the
|
||||
// property over every OTHER transport — ZAP, the default, and anything
|
||||
// RegisterTransport adds — which serialise the request as it was built.
|
||||
//
|
||||
// A command addresses ONE operation. That is the whole basis on which any
|
||||
// caller decides whether it may run, and a transport that re-reads the address
|
||||
// makes that decision about a different request than the one it sends.
|
||||
req.URI().DisablePathNormalizing = true
|
||||
if scheme == "https" {
|
||||
req.URI().SetScheme("https")
|
||||
}
|
||||
|
||||
@@ -0,0 +1,74 @@
|
||||
package zip_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
|
||||
"github.com/zap-proto/zip"
|
||||
)
|
||||
|
||||
// A COMMAND ADDRESSES ONE OPERATION, and its arguments may not change which.
|
||||
//
|
||||
// Invoke percent-encodes every path argument for exactly this reason, but the
|
||||
// encoding is only worth as much as the transport that carries it: fasthttp's
|
||||
// default serialisation re-reads the path through Path(), which decodes %2F and
|
||||
// resolves "..", so an argument could retarget the request at any route the
|
||||
// method reached. do() disables that normalising; this is the pin.
|
||||
func TestRemoteInvoke_ArgumentsCannotRetargetThePath(t *testing.T) {
|
||||
var got string
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
got = r.URL.EscapedPath()
|
||||
_, _ = w.Write([]byte(`{}`))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
cmd := zip.Command{
|
||||
Service: "platform", Name: "apps-get",
|
||||
Method: http.MethodGet, Path: "/v1/platform/apps/:app",
|
||||
Args: []zip.Arg{{Name: "app"}},
|
||||
}
|
||||
remote := zip.Remote{Base: srv.URL}
|
||||
|
||||
cases := []struct {
|
||||
arg string
|
||||
want string
|
||||
}{
|
||||
{"web", "/v1/platform/apps/web"},
|
||||
{"../../../v1/iam/users", "/v1/platform/apps/..%2F..%2F..%2Fv1%2Fiam%2Fusers"},
|
||||
{"a/b", "/v1/platform/apps/a%2Fb"},
|
||||
{"a%2Fb", "/v1/platform/apps/a%252Fb"},
|
||||
{"..", "/v1/platform/apps/.."},
|
||||
}
|
||||
for _, c := range cases {
|
||||
if _, err := remote.Invoke(context.Background(), cmd, map[string]string{"app": c.arg}, nil); err != nil {
|
||||
t.Fatalf("arg %q: %v", c.arg, err)
|
||||
}
|
||||
if got != c.want {
|
||||
t.Errorf("arg %q reached %q, want %q", c.arg, got, c.want)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The query half is unaffected: a bodyless op's flags still ride the URL.
|
||||
func TestRemoteInvoke_QuerySurvives(t *testing.T) {
|
||||
var got string
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
got = r.URL.EscapedPath() + "?" + r.URL.RawQuery
|
||||
_, _ = w.Write([]byte(`{}`))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
cmd := zip.Command{
|
||||
Service: "o11y", Name: "logs-list",
|
||||
Method: http.MethodGet, Path: "/v1/o11y/logs",
|
||||
}
|
||||
remote := zip.Remote{Base: srv.URL}
|
||||
if _, err := remote.Invoke(context.Background(), cmd, nil, []byte(`{"limit":5}`)); err != nil {
|
||||
t.Fatalf("invoke: %v", err)
|
||||
}
|
||||
if got != "/v1/o11y/logs?limit=5" {
|
||||
t.Fatalf("reached %q", got)
|
||||
}
|
||||
}
|
||||
+12
-2
@@ -72,8 +72,18 @@ var (
|
||||
Serve: func(addr string, h fasthttp.RequestHandler) Server {
|
||||
return &httpServer{addr: addr, srv: &fasthttp.Server{Handler: h}}
|
||||
},
|
||||
Dial: func(addr string) Client { return &fasthttp.HostClient{Addr: withPort(addr, "80")} },
|
||||
Dial: func(addr string) Client {
|
||||
return &fasthttp.HostClient{Addr: withPort(addr, "80"), DisablePathNormalizing: true}
|
||||
},
|
||||
},
|
||||
// DisablePathNormalizing on both dialers: the address a caller BUILT is the
|
||||
// address that goes out. fasthttp's default is to re-read the path —
|
||||
// decoding %2F and resolving ".." — which turns a percent-encoded argument
|
||||
// back into path structure and lets one operation's input address another
|
||||
// (see Remote.do). HostClient.doNonNilReqResp assigns the request's flag
|
||||
// FROM this field, so on these two transports this is the only setting that
|
||||
// is read; the request-level one covers the rest.
|
||||
//
|
||||
// Dial-only: terminating TLS is the ingress's job, so nothing in the
|
||||
// fleet serves https directly — but reaching something that does (an
|
||||
// api.* host, a third party) is an ordinary Proxy, and a CLI talking to
|
||||
@@ -81,7 +91,7 @@ var (
|
||||
// covered by the half that exists.
|
||||
"https": {
|
||||
Dial: func(addr string) Client {
|
||||
return &fasthttp.HostClient{Addr: withPort(addr, "443"), IsTLS: true}
|
||||
return &fasthttp.HostClient{Addr: withPort(addr, "443"), IsTLS: true, DisablePathNormalizing: true}
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user