Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 65e2bfb

Browse filesBrowse files
aslilacaslilac
authored
fix(coderd): harden oauth2 redirect validation (#27274) (#27464)
Backport of #27274 Original PR: #27274 — fix(coderd): harden oauth2 redirect validation Merge commit: 2f87991 Requested by: @aslilac
1 parent 17cbc26 commit 65e2bfb
Copy full SHA for 65e2bfb

5 files changed

+83-24Lines changed: 83 additions & 24 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎coderd/externalauth.go‎

Copy file name to clipboardExpand all lines: coderd/externalauth.go
+1-11Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"errors"
66
"fmt"
77
"net/http"
8-
"net/url"
98

109
"github.com/sqlc-dev/pqtype"
1110
"golang.org/x/sync/errgroup"
@@ -331,7 +330,7 @@ func (api *API) externalAuthCallback(externalAuthConfig *externalauth.Config) ht
331330
// FE know not to enter the authentication loop again, and instead display an error.
332331
redirect = fmt.Sprintf("/external-auth/%s?redirected=true", externalAuthConfig.ID)
333332
}
334-
redirect = uriFromURL(redirect)
333+
redirect = httpapi.SafeRedirectPath(redirect)
335334
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
336335
}
337336
}
@@ -429,12 +428,3 @@ func ExternalAuthConfig(cfg *externalauth.Config) codersdk.ExternalAuthLinkProvi
429428
CodeChallengeMethodsSupported: slice.ToStrings(cfg.CodeChallengeMethodsSupported),
430429
}
431430
}
432-
433-
func uriFromURL(u string) string {
434-
uri, err := url.Parse(u)
435-
if err != nil {
436-
return "/"
437-
}
438-
439-
return uri.RequestURI()
440-
}
Collapse file

‎coderd/httpapi/redirect.go‎

Copy file name to clipboard
+31Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
package httpapi
2+
3+
import (
4+
"net/url"
5+
"strings"
6+
)
7+
8+
// SafeRedirectPath reduces a redirect URL down to a safe, relative path. The
9+
// scheme and host are dropped to prevent redirecting to another origin. Opaque
10+
// URLs (e.g. `javascript:`, `data:`) are rejected outright and default to /.
11+
func SafeRedirectPath(u string) string {
12+
uri, err := url.Parse(u)
13+
if err != nil || uri.Opaque != "" {
14+
return "/"
15+
}
16+
17+
// A path with 2 or more leading slashes (e.g. "//evil.com") is interpreted as
18+
// protocol-relative, so make sure there is exactly one.
19+
path := "/" + strings.TrimLeft(uri.EscapedPath(), "/")
20+
if uri.RawQuery != "" {
21+
path += "?" + uri.RawQuery
22+
}
23+
// We're specifically checking Fragment instead of RawFragment here because
24+
// RawFragment is only populated when the parser needs to preserve a
25+
// non-default escaping, so it is empty for plain-alphanumeric fragments like
26+
// "#wooble". EscapedFragment handles escaping correctly in either case.
27+
if uri.Fragment != "" {
28+
path += "#" + uri.EscapedFragment()
29+
}
30+
return path
31+
}
Collapse file

‎coderd/httpapi/redirect_test.go‎

Copy file name to clipboard
+48Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
package httpapi_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/coder/coder/v2/coderd/httpapi"
7+
)
8+
9+
func TestSafeRedirectPath(t *testing.T) {
10+
t.Parallel()
11+
12+
tests := []struct {
13+
name string
14+
in string
15+
want string
16+
}{
17+
{"empty", "", "/"},
18+
{"simple path", "/foo/bar", "/foo/bar"},
19+
{"path with query", "/foo/bar?baz=qux", "/foo/bar?baz=qux"},
20+
{"path with fragment", "/foo/bar#wooble", "/foo/bar#wooble"},
21+
{"path with query+fragment", "/foo/bar?wibble=wobble#wooble", "/foo/bar?wibble=wobble#wooble"},
22+
{"no leading slash", "foo/bar", "/foo/bar"},
23+
{"malformed", "http://[::1]:namedport", "/"},
24+
// Ensure backslashes aren't a blindspot.
25+
{"backslash after slash", `/\evil.example.com`, "/%5Cevil.example.com"},
26+
{"leading double backslash", `\\evil.example.com`, "/%5C%5Cevil.example.com"},
27+
{"backslash then slash", `\/evil.example.com`, "/%5C/evil.example.com"},
28+
{"mixed slash backslash", `/\/evil.example.com`, "/%5C/evil.example.com"},
29+
{"scheme with backslash", `https:/\evil.example.com`, "/%5Cevil.example.com"},
30+
// Cure53 CDM-02-009: triple-slash open redirect.
31+
{"protocol relative triple slash", "///evil.example.com", "/evil.example.com"},
32+
{"protocol relative double slash", "//evil.example.com", "/"},
33+
{"absolute url with host", "http://evil.example.com/path", "/path"},
34+
{"absolute url with host and query", "https://evil.example.com/path?a=b", "/path?a=b"},
35+
// Cure53 CDM-02-009: javascript: scheme bypassing CSP.
36+
{"javascript scheme", "javascript:alert(origin)", "/"},
37+
{"nested javascript scheme", "javascript:javascript:javascript:alert(origin)", "/"},
38+
{"data scheme", "data:text/html,<script>alert(origin)</script>", "/"},
39+
}
40+
for _, tt := range tests {
41+
t.Run(tt.name, func(t *testing.T) {
42+
t.Parallel()
43+
if got := httpapi.SafeRedirectPath(tt.in); got != tt.want {
44+
t.Errorf("SafeRedirectPath(%q) = %q, want %q", tt.in, got, tt.want)
45+
}
46+
})
47+
}
48+
}
Collapse file

‎coderd/httpmw/oauth2.go‎

Copy file name to clipboardExpand all lines: coderd/httpmw/oauth2.go
+1-11Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"fmt"
66
"net/http"
7-
"net/url"
87
"reflect"
98
"slices"
109

@@ -100,7 +99,7 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg
10099
// the host of the AccessURL but ultimately as long as our redirect
101100
// url omits a host we're ensuring that we're routing to a path
102101
// local to the application.
103-
redirect = uriFromURL(redirect)
102+
redirect = httpapi.SafeRedirectPath(redirect)
104103
}
105104

106105
if code == "" {
@@ -415,12 +414,3 @@ func ExtractOAuth2ProviderAppSecret(db database.Store) func(http.Handler) http.H
415414
})
416415
}
417416
}
418-
419-
func uriFromURL(u string) string {
420-
uri, err := url.Parse(u)
421-
if err != nil {
422-
return "/"
423-
}
424-
425-
return uri.RequestURI()
426-
}
Collapse file

‎coderd/userauth.go‎

Copy file name to clipboardExpand all lines: coderd/userauth.go
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1139,7 +1139,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
11391139
http.SetCookie(rw, cookie)
11401140
}
11411141

1142-
redirect = uriFromURL(redirect)
1142+
redirect = httpapi.SafeRedirectPath(redirect)
11431143
if api.GithubOAuth2Config.DeviceFlowEnabled {
11441144
// In the device flow, the redirect is handled client-side.
11451145
httpapi.Write(ctx, rw, http.StatusOK, codersdk.OAuth2DeviceFlowCallbackResponse{
@@ -1560,7 +1560,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
15601560
redirect := state.Redirect
15611561
// Strip the host if it exists on the URL to prevent
15621562
// any nefarious redirects.
1563-
redirect = uriFromURL(redirect)
1563+
redirect = httpapi.SafeRedirectPath(redirect)
15641564
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
15651565
}
15661566

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.