From 0e6ec299b397fdc246a0ff38fb3453709136e50d Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Thu, 22 Oct 2026 20:42:19 +0200 Subject: [PATCH 1/2] Refuse dot segments in package paths and upstream auth matching Routers clean literal dot segments, but encoded ones such as %2e%2e reach the handlers. Maven, conda, cran, go and gem append the decoded path to the upstream URL, so the upstream resolved it outside the configured base path. Package routes now answer 400 for a '.' or '..' segment, also when encoded twice or written with a backslash. Upstream credentials matched such a URL by its path prefix, so /scope/../other still got the credentials configured for /scope. URLs with dot segments now get no credentials. --- internal/config/config.go | 24 ++++++++ internal/config/config_test.go | 4 ++ internal/server/dot_segments_test.go | 91 ++++++++++++++++++++++++++++ internal/server/middleware.go | 17 ++++++ internal/server/server.go | 2 +- 5 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 internal/server/dot_segments_test.go diff --git a/internal/config/config.go b/internal/config/config.go index cbb4e6e1..96487d44 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1068,6 +1068,12 @@ func parseAuthURL(value string) (*url.URL, error) { } func authURLMatches(configured, target *url.URL) bool { + // An upstream resolves dot segments, so /scope/../other would leave the + // configured path while still matching it by prefix. Real package URLs + // never contain them, so they get no credentials at all. + if HasDotSegment(target.Path) { + return false + } if !strings.EqualFold(configured.Scheme, target.Scheme) || !strings.EqualFold(configured.Hostname(), target.Hostname()) || authURLPort(configured) != authURLPort(target) { @@ -1085,6 +1091,24 @@ func authURLMatches(configured, target *url.URL) bool { return targetPath == configuredPath || strings.HasPrefix(targetPath, configuredPath+"/") } +// HasDotSegment reports whether the URL path p, or p decoded once more, has a +// '.' or '..' segment. Backslashes count as separators, since some upstream +// servers treat them as such. +func HasDotSegment(p string) bool { + candidates := []string{p} + if decoded, err := url.PathUnescape(p); err == nil { + candidates = append(candidates, decoded) + } + for _, candidate := range candidates { + for segment := range strings.SplitSeq(strings.ReplaceAll(candidate, `\`, "/"), "/") { + if segment == "." || segment == ".." { + return true + } + } + } + return false +} + func authURLPort(value *url.URL) string { if port := value.Port(); port != "" { return port diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 224c6517..bf27f1e9 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1322,6 +1322,10 @@ func TestUpstreamAuthForURLMatchesURLComponents(t *testing.T) { {name: "lookalike host rejected", url: "https://registry.example.com.evil.test/package"}, {name: "different scheme rejected", url: "http://registry.example.com/package"}, {name: "different port rejected", url: "https://registry.example.com:8443/package"}, + {name: "dot dot segment rejected", url: "https://registry.example.com/private/../package"}, + {name: "encoded dot dot segment rejected", url: "https://registry.example.com/private/%2e%2e/package"}, + {name: "double encoded dot dot segment rejected", url: "https://registry.example.com/private/%252e%252e/package"}, + {name: "dot segment rejected", url: "https://registry.example.com/private/./package"}, } for _, tt := range tests { diff --git a/internal/server/dot_segments_test.go b/internal/server/dot_segments_test.go new file mode 100644 index 00000000..c0979c35 --- /dev/null +++ b/internal/server/dot_segments_test.go @@ -0,0 +1,91 @@ +package server + +import ( + "context" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + + "github.com/git-pkgs/proxy/internal/config" + "github.com/git-pkgs/proxy/internal/database" + "github.com/git-pkgs/proxy/internal/handler" + "github.com/git-pkgs/proxy/internal/storage" + "github.com/git-pkgs/registries/fetch" + "github.com/go-chi/chi/v5" +) + +// TestProtocolHandlersRejectDotSegments sends paths with encoded '.' and '..' +// segments to every mounted ecosystem. Several handlers append the decoded +// path to the upstream URL, where a '..' leaves the configured upstream path. +func TestProtocolHandlersRejectDotSegments(t *testing.T) { + db, err := database.Create(t.TempDir() + "/test.db") + if err != nil { + t.Fatalf("creating database: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + store, err := storage.OpenBucket(context.Background(), "file://"+t.TempDir()) + if err != nil { + t.Fatalf("opening storage: %v", err) + } + + transport := &countingTransport{} + client := &http.Client{Transport: transport} + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(client), fetch.WithMaxRetries(0)) + t.Cleanup(func() { _ = fetcher.Close() }) + proxy := handler.NewProxy(db, store, fetcher, fetch.NewResolver(), slog.New(slog.NewTextHandler(io.Discard, nil))) + proxy.HTTPClient = client + + cfg := config.Default() + cfg.BaseURL = "http://proxy.example" + s := &Server{cfg: cfg} + r := chi.NewRouter() + s.mountProtocolHandlers(r, proxy) + + for _, target := range []string{ + "/maven/%2e%2e/%2e%2e/secret", + "/maven/com/%2e%2e/%2e%2e/x/maven-metadata.xml", + "/maven/com/%2E%2E/x/1.0/x-1.0.jar", + "/maven/com/%2e/x/1.0/x-1.0.jar", + "/maven/com/%252e%252e/x/maven-metadata.xml", + "/conda/c/%2e%2e/x.json", + "/cran/src/contrib/%2e%2e%2fx.tar.gz", + "/cran/src/contrib/..%5cx.tar.gz", + "/go/a/%2e%2e/%2e%2e/x/@v/list", + "/gem/info/%2e%2e%2f%2e%2e%2fx", + "/gem/gems/%2e%2e%2fx-1.0.gem", + "/nuget/v3-flatcontainer/%2e%2e/x/index.json", + "/debian/pool/%2e%2e/x.deb", + "/rpm/%2e%2e/x.rpm", + } { + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil)) + if w.Code != http.StatusBadRequest { + t.Errorf("%s: status = %d, want 400", target, w.Code) + } + } + if n := transport.hits.Load(); n != 0 { + t.Errorf("upstream was contacted %d times", n) + } +} + +// TestDotSegmentGuardKeepsDottedNames keeps names that only contain dots, +// such as versions and file extensions, working. +func TestDotSegmentGuardKeepsDottedNames(t *testing.T) { + for _, target := range []string{ + "/maven/com/example/x/1.0/x-1.0.jar", + "/pypi/simple/zope.interface/", + "/npm/.bin", + "/generic/github/o/r/releases/download/v1/...tar.gz", + "/maven/com/example/x/1..0/x-1..0.jar", + } { + var reached bool + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached = true }) + w := httptest.NewRecorder() + rejectDotSegments(next).ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil)) + if !reached { + t.Errorf("%s: status = %d, want the request passed on", target, w.Code) + } + } +} diff --git a/internal/server/middleware.go b/internal/server/middleware.go index 0de0dba5..b4729736 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -8,6 +8,7 @@ import ( "time" "github.com/git-pkgs/proxy/internal/accesslog" + "github.com/git-pkgs/proxy/internal/config" "github.com/git-pkgs/proxy/internal/metrics" "github.com/go-chi/chi/v5/middleware" ) @@ -140,6 +141,22 @@ func rejectQueryInPath(next http.Handler) http.Handler { }) } +// rejectDotSegments refuses requests whose decoded path has a '.' or '..' +// segment. Routers clean literal dot segments, but encoded ones such as +// %2e%2e reach the handlers, and several of them append the path to the +// upstream URL, where the upstream resolves it outside the configured base +// path. Backslashes count as separators, and the path is decoded once more +// so double-encoded segments are refused too. +func rejectDotSegments(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if config.HasDotSegment(r.URL.Path) { + http.Error(w, "invalid path", http.StatusBadRequest) + return + } + next.ServeHTTP(w, r) + }) +} + // sandboxUpstreamContent keeps package-route responses from running as pages // on the proxy's origin, which also serves /ui. Their bodies, and usually // their Content-Type, come from upstreams or from whoever pushed a hosted diff --git a/internal/server/server.go b/internal/server/server.go index 8a0a683a..9a4c5062 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -493,7 +493,7 @@ func (s *Server) mountProtocolHandlers(r chi.Router, proxy *handler.Proxy) { rpmHandler := handler.NewRPMHandlerWithUpstream(proxy, s.cfg.BaseURL, s.cfg.Upstream.RPM) genericHandler := handler.NewGenericHandler(proxy, s.cfg.Upstream.Generic, s.cfg.Upstream.GenericArtifacts) - r = r.With(sandboxUpstreamContent, rejectQueryInPath) + r = r.With(sandboxUpstreamContent, rejectQueryInPath, rejectDotSegments) r.Mount("/npm", http.StripPrefix("/npm", npmHandler.Routes())) r.Mount("/cargo", http.StripPrefix("/cargo", cargoHandler.Routes())) r.Mount("/gem", http.StripPrefix("/gem", gemHandler.Routes())) From 1fbee102b141797ca16420d3645416cae08370e9 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Fri, 23 Oct 2026 09:47:07 +0200 Subject: [PATCH 2/2] Ignore path parameters when checking for dot segments Tomcat and Jetty drop ";params" from a path segment before resolving it, so "..;" passed the dot-segment check and still got the credentials configured for the upstream path. The check now ignores ";params", and package routes refuse ';' like '?' and '#', which also stops "1.0;x" from passing the denylist as a different version. Composer archives stay exempt, since git branch names may contain ';'. --- internal/config/config.go | 5 +++-- internal/config/config_test.go | 3 +++ internal/server/dot_segments_test.go | 10 ++++++++-- internal/server/middleware.go | 16 +++++++++------- internal/server/query_in_path_test.go | 22 ++++++++++++++-------- 5 files changed, 37 insertions(+), 19 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 96487d44..14c4c207 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1092,8 +1092,8 @@ func authURLMatches(configured, target *url.URL) bool { } // HasDotSegment reports whether the URL path p, or p decoded once more, has a -// '.' or '..' segment. Backslashes count as separators, since some upstream -// servers treat them as such. +// '.' or '..' segment. Backslashes count as separators, and ";params" are +// ignored, since some upstream servers (Tomcat, Jetty) treat them that way. func HasDotSegment(p string) bool { candidates := []string{p} if decoded, err := url.PathUnescape(p); err == nil { @@ -1101,6 +1101,7 @@ func HasDotSegment(p string) bool { } for _, candidate := range candidates { for segment := range strings.SplitSeq(strings.ReplaceAll(candidate, `\`, "/"), "/") { + segment, _, _ = strings.Cut(segment, ";") if segment == "." || segment == ".." { return true } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index bf27f1e9..7a689f85 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1326,6 +1326,9 @@ func TestUpstreamAuthForURLMatchesURLComponents(t *testing.T) { {name: "encoded dot dot segment rejected", url: "https://registry.example.com/private/%2e%2e/package"}, {name: "double encoded dot dot segment rejected", url: "https://registry.example.com/private/%252e%252e/package"}, {name: "dot segment rejected", url: "https://registry.example.com/private/./package"}, + {name: "dot dot segment with parameter rejected", url: "https://registry.example.com/private/..;/package"}, + {name: "encoded dot dot segment with parameter rejected", url: "https://registry.example.com/private/%2e%2e;x/package"}, + {name: "dot dot segment with encoded parameter rejected", url: "https://registry.example.com/private/..%3b/package"}, } for _, tt := range tests { diff --git a/internal/server/dot_segments_test.go b/internal/server/dot_segments_test.go index c0979c35..429ccc09 100644 --- a/internal/server/dot_segments_test.go +++ b/internal/server/dot_segments_test.go @@ -17,8 +17,9 @@ import ( ) // TestProtocolHandlersRejectDotSegments sends paths with encoded '.' and '..' -// segments to every mounted ecosystem. Several handlers append the decoded -// path to the upstream URL, where a '..' leaves the configured upstream path. +// segments, and with ';' path parameters, to every mounted ecosystem. Several +// handlers append the decoded path to the upstream URL, where a '..' leaves +// the configured upstream path, and Tomcat and Jetty drop ";params" first. func TestProtocolHandlersRejectDotSegments(t *testing.T) { db, err := database.Create(t.TempDir() + "/test.db") if err != nil { @@ -58,6 +59,11 @@ func TestProtocolHandlersRejectDotSegments(t *testing.T) { "/nuget/v3-flatcontainer/%2e%2e/x/index.json", "/debian/pool/%2e%2e/x.deb", "/rpm/%2e%2e/x.rpm", + "/maven/..;/..;/other-repo/com/x/x/1.0/x-1.0.jar", + "/maven/com/%2e%2e;/x/1.0/x-1.0.jar", + "/maven/com/..%3b/x/1.0/x-1.0.jar", + "/maven/com/evil/evil/1.0;x/evil-1.0.jar", + "/generic/github/o/r/releases/download/v1/tool.tar.gz;x", } { w := httptest.NewRecorder() r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil)) diff --git a/internal/server/middleware.go b/internal/server/middleware.go index b4729736..c179744f 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -125,15 +125,17 @@ func requestEcosystem(path string) string { } } -// rejectQueryInPath refuses requests whose decoded path contains '?' or '#'. -// Handlers build upstream URLs from the decoded path, where such a character -// would end the path: busybox.apk%3F/foo-9.9-r0.apk fetches busybox but is -// parsed and cached as foo, under the wrong PURL and past the denylist. -// Composer archives are exempt: their upstream URL comes from the package -// metadata, and git branch versions such as dev-fix/#42 may contain '#'. +// rejectQueryInPath refuses requests whose decoded path contains '?', '#' +// or ';'. Handlers build upstream URLs from the decoded path, where '?' or +// '#' would end the path: busybox.apk%3F/foo-9.9-r0.apk fetches busybox but +// is parsed and cached as foo, under the wrong PURL and past the denylist. +// Tomcat and Jetty drop ";params" from a path segment, so 1.0;x/evil-1.0.jar +// would be served as version 1.0 while the proxy checks 1.0;x. Composer +// archives are exempt: their upstream URL comes from the package metadata, +// and git branch versions such as dev-fix/#42 may contain '#' or ';'. func rejectQueryInPath(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if strings.ContainsAny(r.URL.Path, "?#") && !strings.HasPrefix(r.URL.Path, "/composer/files/") { + if strings.ContainsAny(r.URL.Path, "?#;") && !strings.HasPrefix(r.URL.Path, "/composer/files/") { http.Error(w, "invalid path", http.StatusBadRequest) return } diff --git a/internal/server/query_in_path_test.go b/internal/server/query_in_path_test.go index 7c70a13c..44ab58ed 100644 --- a/internal/server/query_in_path_test.go +++ b/internal/server/query_in_path_test.go @@ -93,15 +93,21 @@ func TestProtocolHandlersRejectQueryInPath(t *testing.T) { } // TestComposerFilesAllowHashInVersion keeps Composer archive downloads of git -// branches with '#' in their name working: the version is escaped in the dist -// URL and the archive is resolved from metadata, not from the request path. +// branches with '#' or ';' in their name working: the version is escaped in +// the dist URL and the archive is resolved from metadata, not from the +// request path. func TestComposerFilesAllowHashInVersion(t *testing.T) { - var reached atomic.Bool - next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached.Store(true) }) + for _, target := range []string{ + "/composer/files/acme/lib/dev-fix%2F%2342/abc123.zip", + "/composer/files/acme/lib/dev-fix%3B42/abc123.zip", + } { + var reached atomic.Bool + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached.Store(true) }) - w := httptest.NewRecorder() - rejectQueryInPath(next).ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/composer/files/acme/lib/dev-fix%2F%2342/abc123.zip", nil)) - if w.Code == http.StatusBadRequest || !reached.Load() { - t.Errorf("status = %d, reached = %v; want the request passed on", w.Code, reached.Load()) + w := httptest.NewRecorder() + rejectQueryInPath(next).ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil)) + if w.Code == http.StatusBadRequest || !reached.Load() { + t.Errorf("%s: status = %d, reached = %v; want the request passed on", target, w.Code, reached.Load()) + } } }