From 260c6deca3fab250faaeefc8923d2bf3de6e443a Mon Sep 17 00:00:00 2001 From: Jan Larwig Date: Thu, 1 Oct 2026 09:30:09 +0200 Subject: [PATCH] Merge commit from fork Signed-off-by: Jan Larwig --- CHANGELOG.md | 23 ++- docs/docs/behaviour.md | 1 + docs/docs/configuration/overview.md | 32 +++- oauthproxy.go | 14 +- oauthproxy_test.go | 275 ++++++++++++++++++++++++++++ pkg/requests/util/util.go | 51 +++--- pkg/requests/util/util_test.go | 105 ++++++++++- 7 files changed, 464 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7e9978b..61e0bae8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,9 +19,30 @@ Additionally refer to OAuth client configuration for Bitbucket provider in the [ ## Breaking Changes + +### (Critical) [GHSA-63jm-59jj-478j](https://github.com/oauth2-proxy/oauth2-proxy/security/advisories/GHSA-63jm-59jj-478j) Authentication bypass via inconsistent skip-auth path interpretation +**Skip-auth path matching is now stricter.** `--skip-auth-route` and +`--skip-auth-regex` no longer grant exemptions for invalid or ambiguous paths, +even when a positive or negated rule would otherwise match. This includes dot +segments, repeated slashes (including leading `//`), semicolons, backslashes, +fragment-like content, decoded question marks, and control characters, including +encoded forms. For example, previously public `/public/file;version=1` and +`/public//file` now follow normal authentication and authorization. `#` and `%23` +are no longer silently stripped before matching. + +Trusted `X-Forwarded-Uri` metadata must use origin form (`/path?query`), not a full +URL. Before upgrading, review public-route exemptions and external-auth header +configuration. If unusual paths must remain public, expose them separately from +the protected routing boundary rather than broadening skip-auth rules. + +Flag names and configuration syntax are unchanged. Ordinary query strings, +trailing-slash distinctions, and unambiguous escaped characters retain their +matching behavior. The fix does not rewrite upstream request targets or +unconditionally reject authenticated requests; existing router behavior and +separately configured exemptions remain unchanged. + ## Changes since v7.15.4 -<<<<<<< HEAD - [#3546](https://github.com/oauth2-proxy/oauth2-proxy/pull/3546) fix: strip the port from the request host when matching cookie domains @kirilju - [#3547](https://github.com/oauth2-proxy/oauth2-proxy/pull/3547) fix: refresh additional claims for OIDC and MS Entra ID providers and properly populate additional claims during login (@Apollo3zehn) - [#3477](https://github.com/oauth2-proxy/oauth2-proxy/pull/3477) fix(bitbucket): auth failure due to Bitbucket OAuth 2.0 [changes on May 4th 2026](https://developer.atlassian.com/cloud/bitbucket/changelog/#CHANGE-3052) @mfouad diff --git a/docs/docs/behaviour.md b/docs/docs/behaviour.md index d0be452e..605ec013 100644 --- a/docs/docs/behaviour.md +++ b/docs/docs/behaviour.md @@ -5,6 +5,7 @@ title: Behaviour 1. Authentication Requirement: All requests passing through the proxy to upstream applications require authentication, excluding default proxy endpoints. - Exception: If the request matches a skipped route (configured via `--skip-auth-route`): + - Invalid or ambiguous paths cannot grant this exemption, even with a negated rule. See [Skip-auth path matching](configuration/overview.md#skip-auth-path-matching). - Authentication is not enforced, but the proxy will opportunistically attempt to validate a session cookie (`--cookie-name`) or JWT (`--skip-jwt-bearer-tokens`) if present in the request. - Configured user info and authentication headers (e.g., `--pass-access-token`) are injected to upstream routes when validation succeeds. diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 013cd7b4..7e6a036d 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -218,8 +218,8 @@ When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or | flag: `--trusted-proxy-ip`
toml: `trusted_proxy_ips` | string \| list | list of IPs or CIDR ranges allowed to supply `X-Forwarded-*` headers when `--reverse-proxy` is enabled. If not set, OAuth2 Proxy preserves backwards compatibility by trusting all source IPs (`0.0.0.0/0`, `::/0`) and logs a warning at startup. Configure this to your reverse proxy addresses to prevent forwarded header spoofing. | `"0.0.0.0/0", "::/0"` | | flag: `--signature-key`
toml: `signature_key` | string | GAP-Signature request signature key (algorithm:secretkey) | | | flag: `--skip-auth-preflight`
toml: `skip_auth_preflight` | bool | will skip authentication for OPTIONS requests | false | -| flag: `--skip-auth-regex`
toml: `skip_auth_regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests paths that match (may be given multiple times). Path matching is performed against the normalized path only; fragment identifiers (`#`) and their URL-encoded form (`%23`) are stripped before evaluation. | | -| flag: `--skip-auth-route`
toml: `skip_auth_routes` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_regex. Path matching is performed against the normalized path only; fragment identifiers (`#`) and their URL-encoded form (`%23`) are stripped before evaluation. | | +| flag: `--skip-auth-regex`
toml: `skip_auth_regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests whose paths match (may be given multiple times). Matching uses the decoded path without query parameters; invalid or ambiguous paths cannot grant a path exemption. See [Skip-auth path matching](#skip-auth-path-matching). | | +| flag: `--skip-auth-route`
toml: `skip_auth_routes` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_regex. Matching uses the decoded path without query parameters; invalid or ambiguous paths cannot grant a path exemption, including with negated rules. See [Skip-auth path matching](#skip-auth-path-matching). | | | flag: `--skip-jwt-bearer-tokens`
toml: `skip_jwt_bearer_tokens` | bool | will skip requests that have verified JWT bearer tokens (the token must have [`aud`](https://en.wikipedia.org/wiki/JSON_Web_Token#Standard_fields) that matches this client id or one of the extras from `extra-jwt-issuers`) | false | | flag: `--skip-provider-button`
toml: `skip_provider_button` | bool | will skip sign-in-page to directly reach the next step: oauth/start | false | | flag: `--ssl-insecure-skip-verify`
toml: `ssl_insecure_skip_verify` | bool | skip validation of certificates presented when using HTTPS providers | false | @@ -273,6 +273,34 @@ When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or | flag: `--upstream-timeout`
toml: `upstream_timeout` | duration | maximum amount of time the server will wait for a response from the upstream | 30s | | flag: `--upstream`
toml: `upstreams` | string \| list | the http url(s) of the upstream endpoint, file:// paths for static files or `static://` for static response. Routing is based on the path | | +### Skip-auth path matching + +Both `--skip-auth-route` and the deprecated `--skip-auth-regex` match the +percent-decoded path, excluding query parameters. In reverse-proxy mode, +`X-Forwarded-Uri` is used only when the existing forwarded-header trust checks +permit it. Original-URI metadata must use origin form (`/path?query`), not an +absolute URL, authority, or relative path. + +Invalid or ambiguous targets do not grant a path-based authentication exemption. +This check happens before any regular expression or its negation is evaluated. +In particular, exemptions are declined for malformed path escapes, `.` or `..` +path segments, repeated slashes (including leading `//`), semicolons, backslashes, +fragment-like `#` content, decoded `?` characters, and control characters. +Encoded forms are checked after a single decoding pass; paths are not recursively +decoded. + +This intentionally also requires normal authentication for valid semicolon paths +such as `/public/file;version=1` and repeated-slash paths such as `/public//file`. +Fragments are no longer silently stripped. Ordinary query strings do not affect +matching. Unambiguous escaped characters remain supported, and trailing slashes +remain significant: `/public` and `/public/` are distinct matching paths. + +These checks do not normalize or rewrite the upstream request target. A declined +path exemption follows normal authentication and authorization; it is not an +unconditional rejection of authenticated requests using unusual paths. Existing +router redirects and separately configured exemptions, such as trusted client +IPs or skipped preflight requests, are unchanged. + ## Configuration Validation The `--config-test` flag validates your configuration file without starting the proxy server. This is useful for: diff --git a/oauthproxy.go b/oauthproxy.go index c1f56412..d376d5bf 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -595,8 +595,8 @@ func isAllowedMethod(req *http.Request, route allowedRoute) bool { return route.method == "" || req.Method == route.method } -func isAllowedPath(req *http.Request, route allowedRoute) bool { - matches := route.pathRegex.MatchString(requestutil.GetRequestPath(req)) +func isAllowedPath(requestPath string, route allowedRoute) bool { + matches := route.pathRegex.MatchString(requestPath) if route.negate { return !matches @@ -607,8 +607,16 @@ func isAllowedPath(req *http.Request, route allowedRoute) bool { // IsAllowedRoute is used to check if the request method & path is allowed without auth func (p *OAuthProxy) isAllowedRoute(req *http.Request) bool { + if len(p.allowedRoutes) == 0 { + return false + } + requestPath, err := requestutil.GetRequestPath(req) + if err != nil { + logger.Errorf("Skipping path-based authentication exemption: %v", err) + return false + } for _, route := range p.allowedRoutes { - if isAllowedMethod(req, route) && isAllowedPath(req, route) { + if isAllowedMethod(req, route) && isAllowedPath(requestPath, route) { return true } } diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 2eb5aa23..90eab493 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -9,9 +9,12 @@ import ( "io" "net/http" "net/http/httptest" + "net/http/httputil" "net/url" + "path" "regexp" "strings" + "sync/atomic" "testing" "time" @@ -2790,6 +2793,278 @@ func TestApiRoutes(t *testing.T) { } } +func TestSkipAuthPathRouting(t *testing.T) { + for _, mode := range []string{"default proxy", "raw proxy", "external auth"} { + t.Run(mode, func(t *testing.T) { + var hits, mutations atomic.Int32 + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + // Model the reported servlet ordering: strip literal matrix parameters, + // then decode and resolve dot segments. + segments := strings.Split(r.URL.EscapedPath(), "/") + for i, segment := range segments { + segments[i], _, _ = strings.Cut(segment, ";") + } + decodedPath, err := url.PathUnescape(strings.Join(segments, "/")) + if err != nil { + http.Error(w, "invalid fixture path", http.StatusBadRequest) + return + } + routedPath := path.Clean(decodedPath) + if strings.HasPrefix(routedPath, "/protected/") && r.Method == http.MethodPost { + mutations.Add(1) + } + _, _ = io.WriteString(w, r.RequestURI) + })) + t.Cleanup(backend.Close) + + opts := baseTestOptions() + opts.ForceJSONErrors = true + if mode == "external auth" { + opts.ReverseProxy = true + opts.TrustedProxyIPs = []string{"127.0.0.1/32"} + } + opts.SkipAuthRoutes = []string{"POST=^/public/"} + opts.UpstreamServers = options.UpstreamConfig{ + ProxyRawPath: ptr.To(mode == "raw proxy"), + Upstreams: []options.Upstream{{ + ID: "backend", Path: "/", URI: backend.URL, + }}, + } + require.NoError(t, validation.Validate(opts)) + proxy, err := NewOAuthProxy(opts, func(string) bool { return true }) + require.NoError(t, err) + + newFrontend := func(t *testing.T, proxy *OAuthProxy) *httptest.Server { + t.Helper() + var handler http.Handler = proxy + if mode == "external auth" { + backendURL, err := url.Parse(backend.URL) + require.NoError(t, err) + forward := httputil.NewSingleHostReverseProxy(backendURL) + handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + authReq := httptest.NewRequest(r.Method, "/oauth2/auth", nil) + authReq.RemoteAddr = "127.0.0.1:4180" + authReq.Header = r.Header.Clone() + authReq.Header.Set("X-Forwarded-Uri", r.RequestURI) + authResponse := httptest.NewRecorder() + proxy.ServeHTTP(authResponse, authReq) + if authResponse.Code != http.StatusAccepted { + w.WriteHeader(authResponse.Code) + return + } + forward.ServeHTTP(w, r) + }) + } + return httptest.NewServer(handler) + } + client := &http.Client{ + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + } + + send := func(t *testing.T, server, target string, cookies []*http.Cookie) (int, string) { + t.Helper() + req, err := http.NewRequest(http.MethodPost, server+target, nil) + require.NoError(t, err) + for _, cookie := range cookies { + req.AddCookie(cookie) + } + resp, err := client.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + return resp.StatusCode, string(body) + } + + created, expires := time.Now(), time.Now().Add(time.Hour) + cookieResponse := httptest.NewRecorder() + require.NoError(t, proxy.SaveSession(cookieResponse, httptest.NewRequest(http.MethodGet, "/", nil), &sessions.SessionState{ + User: "user", Email: "user@example.com", CreatedAt: &created, ExpiresOn: &expires, + })) + for _, tc := range []struct { + name string + target string + pattern string + public bool + protected bool + routerRedirect bool + decodedRedirect bool + }{ + {name: "encoded parent", target: "/public/%2e%2e/protected/change", protected: true, decodedRedirect: true}, + {name: "mixed encoded parent", target: "/public/.%2E/protected/change", protected: true, decodedRedirect: true}, + {name: "encoded separators", target: "/public%2f..%2fprotected/change", protected: true, decodedRedirect: true}, + {name: "literal parent", target: "/public/../protected/change", protected: true, routerRedirect: true}, + {name: "current segment", target: "/public/./file", routerRedirect: true}, + {name: "matrix parent", target: "/public/..;/protected/change", protected: true}, + {name: "encoded matrix parent", target: "/public/%2e%2e;/protected/change", protected: true}, + {name: "encoded matrix semicolon control", target: "/public/%2e%2e%3b/protected/change"}, + {name: "matrix value", target: "/public/..;version=1/protected/change", protected: true}, + {name: "matrix current and parent", target: "/public/.;/..;/protected/change", protected: true}, + {name: "nested matrix parents", target: "/public/a/..;/..;/protected/change", protected: true}, + {name: "leading double slash", target: "//protected/public", pattern: "^/public$", protected: true, routerRedirect: true}, + {name: "encoded leading slash", target: "/%2fprotected/public", pattern: "^/public$", protected: true, decodedRedirect: true}, + {name: "repeated slash", target: "/public//file", routerRedirect: true}, + {name: "encoded repeated slash", target: "/public/%2ffile", decodedRedirect: true}, + {name: "valid semicolon", target: "/public/file;version=1"}, + {name: "encoded semicolon", target: "/public/file%3bversion=1"}, + {name: "fragment suffix", target: "/public/file%23suffix"}, + {name: "ordinary public", target: "/public/file", public: true}, + {name: "public query", target: "/public/file?next=/../protected;v=1&tag=%23", public: true}, + {name: "public trailing slash", target: "/public/", pattern: "^/public/$", public: true}, + {name: "trailing slash is significant", target: "/public/file/", pattern: "^/public/file$"}, + {name: "escaped space", target: "/public/a%20b", pattern: "^/public/a b$", public: true}, + {name: "escaped letter", target: "/public/%66ile", pattern: "^/public/file$", public: true}, + {name: "ordinary protected", target: "/protected/change", protected: true}, + } { + t.Run(tc.name, func(t *testing.T) { + // Prove each original target reaches the intended downstream fixture. + status, body := send(t, backend.URL, tc.target, nil) + require.Equal(t, http.StatusOK, status) + require.Equal(t, tc.target, body) + require.EqualValues(t, 1, hits.Swap(0)) + var wantMutations int32 + if tc.protected { + wantMutations = 1 + } + require.Equal(t, wantMutations, mutations.Swap(0)) + + pattern := tc.pattern + if pattern == "" { + pattern = "^/public/" + } + configs := []string{"regex", "route"} + if tc.pattern == "" || tc.protected { + configs = append(configs, "negated route") + } + for _, config := range configs { + t.Run(config, func(t *testing.T) { + opts.SkipAuthRegex, opts.SkipAuthRoutes = nil, nil + if config == "regex" { + opts.SkipAuthRegex = []string{pattern} + } else if config == "negated route" { + opts.SkipAuthRoutes = []string{"POST!=^/protected"} + } else { + opts.SkipAuthRoutes = []string{"POST=" + pattern} + } + testProxy, err := NewOAuthProxy(opts, func(string) bool { return true }) + require.NoError(t, err) + frontend := newFrontend(t, testProxy) + t.Cleanup(frontend.Close) + + wantStatus := http.StatusUnauthorized + if tc.public { + wantStatus = http.StatusOK + } else if tc.routerRedirect && mode != "external auth" { + wantStatus = http.StatusMovedPermanently + } + status, _ := send(t, frontend.URL, tc.target, nil) + assert.Equal(t, wantStatus, status) + var wantHits int32 + if tc.public { + wantHits = 1 + } + assert.Equal(t, wantHits, hits.Swap(0)) + assert.Zero(t, mutations.Swap(0)) + + wantStatus = http.StatusOK + wantHits = 1 + redirects := (tc.routerRedirect && mode != "external auth") || + (tc.decodedRedirect && mode == "default proxy") + if redirects { + wantStatus = http.StatusMovedPermanently + wantHits = 0 + } + status, body := send(t, frontend.URL, tc.target, cookieResponse.Result().Cookies()) + assert.Equal(t, wantStatus, status) + if !redirects { + assert.Equal(t, tc.target, body) + } + assert.Equal(t, wantHits, hits.Swap(0)) + assert.Equal(t, wantMutations*wantHits, mutations.Swap(0)) + }) + } + }) + } + }) + } +} + +func TestSkipAuthForwardedTargets(t *testing.T) { + for _, config := range []string{"regex", "route", "negated route", "empty path"} { + t.Run(config, func(t *testing.T) { + opts := baseTestOptions() + opts.ReverseProxy = true + opts.TrustedProxyIPs = []string{"127.0.0.1/32"} + switch config { + case "regex": + opts.SkipAuthRegex = []string{".*"} + case "route": + opts.SkipAuthRoutes = []string{"GET=.*"} + case "negated route": + opts.SkipAuthRoutes = []string{"GET!=^/protected"} + case "empty path": + opts.SkipAuthRoutes = []string{"GET=^$"} + } + require.NoError(t, validation.Validate(opts)) + proxy, err := NewOAuthProxy(opts, func(string) bool { return true }) + require.NoError(t, err) + created, expires := time.Now(), time.Now().Add(time.Hour) + cookieResponse := httptest.NewRecorder() + require.NoError(t, proxy.SaveSession(cookieResponse, httptest.NewRequest(http.MethodGet, "/", nil), &sessions.SessionState{ + User: "user", Email: "user@example.com", CreatedAt: &created, ExpiresOn: &expires, + })) + + for _, target := range []string{ + "/public/../protected", "/public/%2e%2e/protected", + "//protected/public", "/public//file", + "/public/..;/protected", "/public/%2e%2e%3b/protected", + "/public/file;version=1", "/public/%zz", "/public/%", + "public/file", "https://example.com/public", "example.com:443", "*", + "/public#fragment", "/public%23fragment", "/public%3fquery", + "/public\\file", "/public/%00", + } { + t.Run(target, func(t *testing.T) { + for _, authenticated := range []bool{false, true} { + req := httptest.NewRequest(http.MethodGet, "/oauth2/auth", nil) + req.RemoteAddr = "127.0.0.1:4180" + req.Header.Set("X-Forwarded-Uri", target) + if authenticated { + for _, cookie := range cookieResponse.Result().Cookies() { + req.AddCookie(cookie) + } + } + rw := httptest.NewRecorder() + proxy.ServeHTTP(rw, req) + wantStatus := http.StatusUnauthorized + if authenticated { + wantStatus = http.StatusAccepted + } + assert.Equal(t, wantStatus, rw.Code) + assert.Equal(t, target, req.Header.Get("X-Forwarded-Uri")) + assert.Equal(t, "/oauth2/auth", req.RequestURI) + } + }) + } + + for _, method := range []string{http.MethodGet, http.MethodPost} { + t.Run("ordinary public "+method, func(t *testing.T) { + req := httptest.NewRequest(method, "/oauth2/auth", nil) + req.RemoteAddr = "127.0.0.1:4180" + req.Header.Set("X-Forwarded-Uri", "/public/file?query=value") + rw := httptest.NewRecorder() + proxy.ServeHTTP(rw, req) + wantStatus := http.StatusUnauthorized + if config != "empty path" && (config == "regex" || method == http.MethodGet) { + wantStatus = http.StatusAccepted + } + assert.Equal(t, wantStatus, rw.Code) + }) + } + }) + } +} + func TestAllowedRequest(t *testing.T) { upstreamServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) diff --git a/pkg/requests/util/util.go b/pkg/requests/util/util.go index 568ebcc6..ebfed274 100644 --- a/pkg/requests/util/util.go +++ b/pkg/requests/util/util.go @@ -1,6 +1,7 @@ package util import ( + "errors" "net/http" "net/url" "strings" @@ -45,35 +46,35 @@ func GetRequestURI(req *http.Request) string { return uri } -// GetRequestPath returns the request URI or X-Forwarded-Uri if present and the -// request came from a trusted reverse proxy but always strips the query -// parameters and fragment suffixes and only returns the pure path. -func GetRequestPath(req *http.Request) string { - uri := stripRequestFragment(GetRequestURI(req)) - - // Parse URI and return only the path component - if parsedURL, err := url.Parse(uri); err == nil { - return stripRequestFragment(parsedURL.Path) +// GetRequestPath returns a decoded path suitable for skip-auth matching, using +// X-Forwarded-Uri only for a trusted reverse proxy. An error means the path must +// not grant an authentication exemption. It does not modify the request URL. +func GetRequestPath(req *http.Request) (string, error) { + uri := GetRequestURI(req) + if !strings.HasPrefix(uri, "/") || strings.ContainsAny(uri, "# \t\r\n") { + return "", errors.New("request target is not an unambiguous origin-form URI") } - // Fallback: strip query parameters manually - return stripRequestQuery(uri) -} - -func stripRequestFragment(uri string) string { - if idx := strings.Index(uri, "#"); idx != -1 { - return uri[:idx] + // Unlike url.Parse, ParseRequestURI keeps a leading // in the path. + parsedURL, err := url.ParseRequestURI(uri) + if err != nil { + return "", errors.New("invalid request target") } - - return uri -} - -func stripRequestQuery(uri string) string { - if idx := strings.Index(uri, "?"); idx != -1 { - return uri[:idx] + requestPath := parsedURL.Path + if strings.ContainsAny(requestPath, ";\\#?") || strings.Contains(requestPath, "//") { + return "", errors.New("request path contains ambiguous separators") } - - return uri + for _, char := range requestPath { + if char < 0x20 || char == 0x7f { + return "", errors.New("request path contains a control character") + } + } + for _, segment := range strings.Split(requestPath, "/") { + if segment == "." || segment == ".." { + return "", errors.New("request path contains a dot segment") + } + } + return requestPath, nil } // CanTrustForwardedHeaders determines if forwarded headers should be processed diff --git a/pkg/requests/util/util_test.go b/pkg/requests/util/util_test.go index c4185b35..84e9101a 100644 --- a/pkg/requests/util/util_test.go +++ b/pkg/requests/util/util_test.go @@ -143,6 +143,93 @@ var _ = Describe("Util Suite", func() { }) Context("GetRequestPath", func() { + DescribeTable("preserves unambiguous paths without changing the request", + func(target, expected string) { + for _, forwarded := range []bool{false, true} { + req = httptest.NewRequest(http.MethodGet, target, nil) + if forwarded { + req = httptest.NewRequest(http.MethodGet, "/oauth2/auth", nil) + req.RemoteAddr = "127.0.0.1:4180" + req = middleware.AddRequestScope(req, &middleware.RequestScope{ + ReverseProxy: true, TrustedProxies: trustedProxies, + }) + req.Header.Set("X-Forwarded-Uri", target) + } + originalURL, originalTarget := *req.URL, req.RequestURI + requestPath, err := util.GetRequestPath(req) + Expect(err).NotTo(HaveOccurred()) + Expect(requestPath).To(Equal(expected)) + Expect(*req.URL).To(Equal(originalURL)) + Expect(req.RequestURI).To(Equal(originalTarget)) + } + }, + Entry("root", "/", "/"), + Entry("public path", "/public/file", "/public/file"), + Entry("protected path", "/protected/file", "/protected/file"), + Entry("trailing slash", "/public/", "/public/"), + Entry("query ignored", "/public/file?next=/../protected;v=1&tag=%23", "/public/file"), + Entry("invalid query escape does not affect the path", "/public/file?q=%zz", "/public/file"), + Entry("escaped space", "/public/a%20b", "/public/a b"), + Entry("escaped letter", "/public/%66ile", "/public/file"), + Entry("escaped Unicode", "/public/caf%C3%A9", "/public/caf\u00e9"), + Entry("escaped slash", "/public%2ffile", "/public/file"), + Entry("literal plus", "/public/a+b", "/public/a+b"), + Entry("escaped plus", "/public/a%2bb", "/public/a+b"), + Entry("escaped percent", "/public/100%25", "/public/100%"), + Entry("dot within a segment", "/public/file.txt", "/public/file.txt"), + Entry("multiple dots are not a parent segment", "/public/...", "/public/..."), + ) + + DescribeTable("declines ambiguous or invalid original request targets", + func(target string) { + req.RemoteAddr = "127.0.0.1:4180" + req = middleware.AddRequestScope(req, &middleware.RequestScope{ + ReverseProxy: true, TrustedProxies: trustedProxies, + }) + req.Header.Set("X-Forwarded-Uri", target) + requestPath, err := util.GetRequestPath(req) + Expect(err).To(HaveOccurred()) + Expect(requestPath).To(BeEmpty()) + Expect(err.Error()).NotTo(ContainSubstring("sensitive-value")) + }, + Entry("literal parent segment", "/public/../protected?token=sensitive-value"), + Entry("encoded parent segment", "/public/%2e%2E/protected"), + Entry("encoded slash exposes parent segment", "/public%2f..%2fprotected"), + Entry("current segment", "/public/./file"), + Entry("terminal parent segment", "/public/.."), + Entry("terminal current segment", "/public/."), + Entry("leading double slash", "//protected/public"), + Entry("repeated slash", "/public//file"), + Entry("encoded repeated slash", "/public/%2ffile"), + Entry("matrix parent segment", "/public/..;/protected"), + Entry("encoded matrix parent segment", "/public/%2e%2e%3b/protected"), + Entry("valid matrix parameter", "/public/file;version=1"), + Entry("matrix parameter containing a slash", "/protected;/public"), + Entry("invalid escape", "/public/%zz?token=sensitive-value"), + Entry("incomplete escape", "/public/%"), + Entry("relative target", "public/file"), + Entry("absolute target", "https://example.com/public/file"), + Entry("authority target", "example.com:443"), + Entry("asterisk target", "*"), + Entry("literal fragment", "/public#sensitive-value"), + Entry("encoded fragment", "/public%23sensitive-value"), + Entry("encoded question mark", "/public%3fsensitive-value"), + Entry("backslash", "/public\\..\\protected"), + Entry("encoded backslash", "/public%5c..%5cprotected"), + Entry("encoded control character", "/public/%00"), + Entry("encoded delete character", "/public/%7f"), + Entry("unescaped space", "/public/a b"), + ) + + It("ignores a forged forwarded URI from an untrusted peer", func() { + req.RemoteAddr = "192.0.2.10:4180" + req = middleware.AddRequestScope(req, &middleware.RequestScope{ + ReverseProxy: true, TrustedProxies: trustedProxies, + }) + req.Header.Set("X-Forwarded-Uri", "/public") + Expect(util.GetRequestPath(req)).To(Equal(uriNoQueryParams)) + }) + Context("trusted forwarded headers are disabled", func() { BeforeEach(func() { req = middleware.AddRequestScope(req, &middleware.RequestScope{}) @@ -152,21 +239,25 @@ var _ = Describe("Util Suite", func() { Expect(util.GetRequestPath(req)).To(Equal(uriNoQueryParams)) }) - It("drops fragment content from a parsed request path", func() { + It("declines fragment content in a parsed request path", func() { // Simulate net/http ParseRequestURI preserving '#' in URL.Path. req.URL.Path = "/foo/secret#/bar" req.URL.RawPath = "/foo/secret%23/bar" - Expect(util.GetRequestPath(req)).To(Equal("/foo/secret")) + requestPath, err := util.GetRequestPath(req) + Expect(err).To(HaveOccurred()) + Expect(requestPath).To(BeEmpty()) }) - It("drops fragment-like suffixes from encoded number signs", func() { + It("declines encoded number signs", func() { req = httptest.NewRequest( http.MethodGet, fmt.Sprintf("%s://%s/foo/secret%%23/bar?query=param", proto, host), nil, ) req = middleware.AddRequestScope(req, &middleware.RequestScope{}) - Expect(util.GetRequestPath(req)).To(Equal("/foo/secret")) + requestPath, err := util.GetRequestPath(req) + Expect(err).To(HaveOccurred()) + Expect(requestPath).To(BeEmpty()) }) It("ignores X-Forwarded-Uri and returns the URI (without query params)", func() { @@ -193,9 +284,11 @@ var _ = Describe("Util Suite", func() { Expect(util.GetRequestPath(req)).To(Equal("/some/other/path")) }) - It("drops fragment-like suffixes from the X-Forwarded-Uri", func() { + It("declines fragment-like suffixes from the X-Forwarded-Uri", func() { req.Header.Add("X-Forwarded-Uri", "/foo/secret%23/bar?query=param") - Expect(util.GetRequestPath(req)).To(Equal("/foo/secret")) + requestPath, err := util.GetRequestPath(req) + Expect(err).To(HaveOccurred()) + Expect(requestPath).To(BeEmpty()) }) }) })