Skip to content

Commit 328e696

Browse files
sreyasreya
authored
fix: limit OAuth redirects to local paths (coder#14585)
- This prevents a malicious user from crafting a redirect URL to a nefarious site under their control.
1 parent 2a9234e commit 328e696

File tree

8 files changed

+183
-20
lines changed

8 files changed

+183
-20
lines changed

‎coderd/coderdtest/coderdtest.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1143,7 +1143,7 @@ func MustWorkspace(t testing.TB, client *codersdk.Client, workspaceID uuid.UUID)
11431143

11441144
// RequestExternalAuthCallback makes a request with the proper OAuth2 state cookie
11451145
// to the external auth callback endpoint.
1146-
func RequestExternalAuthCallback(t testing.TB, providerID string, client *codersdk.Client) *http.Response {
1146+
func RequestExternalAuthCallback(t testing.TB, providerID string, client *codersdk.Client, opts ...func(*http.Request)) *http.Response {
11471147
client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
11481148
return http.ErrUseLastResponse
11491149
}
@@ -1160,6 +1160,9 @@ func RequestExternalAuthCallback(t testing.TB, providerID string, client *coders
11601160
Name: codersdk.SessionTokenCookie,
11611161
Value: client.SessionToken(),
11621162
})
1163+
for _, opt := range opts {
1164+
opt(req)
1165+
}
11631166
res, err := client.HTTPClient.Do(req)
11641167
require.NoError(t, err)
11651168
t.Cleanup(func() {

‎coderd/coderdtest/oidctest/idp.go

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,6 @@ func (f *FakeIDP) AttemptLogin(t testing.TB, client *codersdk.Client, idTokenCla
479479
// This is a niche case, but it is needed for testing ConvertLoginType.
480480
func (f *FakeIDP) LoginWithClient(t testing.TB, client *codersdk.Client, idTokenClaims jwt.MapClaims, opts ...func(r *http.Request)) (*codersdk.Client, *http.Response) {
481481
t.Helper()
482-
483482
path := "/api/v2/users/oidc/callback"
484483
if f.callbackPath != "" {
485484
path = f.callbackPath
@@ -489,13 +488,23 @@ func (f *FakeIDP) LoginWithClient(t testing.TB, client *codersdk.Client, idToken
489488
f.SetRedirect(t, coderOauthURL.String())
490489

491490
cli := f.HTTPClient(client.HTTPClient)
492-
cli.CheckRedirect = func(req *http.Request, via []*http.Request) error {
491+
redirectFn := cli.CheckRedirect
492+
checkRedirect := func(req *http.Request, via []*http.Request) error {
493493
// Store the idTokenClaims to the specific state request. This ties
494494
// the claims 1:1 with a given authentication flow.
495-
state := req.URL.Query().Get("state")
496-
f.stateToIDTokenClaims.Store(state, idTokenClaims)
495+
if state := req.URL.Query().Get("state"); state != "" {
496+
f.stateToIDTokenClaims.Store(state, idTokenClaims)
497+
return nil
498+
}
499+
// This is mainly intended to prevent the _last_ redirect
500+
// The one involving the state param is a core part of the
501+
// OIDC flow and shouldn't be redirected.
502+
if redirectFn != nil {
503+
return redirectFn(req, via)
504+
}
497505
return nil
498506
}
507+
cli.CheckRedirect = checkRedirect
499508

500509
req, err := http.NewRequestWithContext(context.Background(), "GET", coderOauthURL.String(), nil)
501510
require.NoError(t, err)

‎coderd/externalauth.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"errors"
66
"fmt"
77
"net/http"
8+
"net/url"
89

910
"github.com/sqlc-dev/pqtype"
1011
"golang.org/x/sync/errgroup"
@@ -306,6 +307,7 @@ func (api *API) externalAuthCallback(externalAuthConfig *externalauth.Config) ht
306307
// FE know not to enter the authentication loop again, and instead display an error.
307308
redirect = fmt.Sprintf("/external-auth/%s?redirected=true", externalAuthConfig.ID)
308309
}
310+
redirect = uriFromURL(redirect)
309311
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
310312
}
311313
}
@@ -401,3 +403,12 @@ func ExternalAuthConfig(cfg *externalauth.Config) codersdk.ExternalAuthLinkProvi
401403
AllowValidate: cfg.ValidateURL != "",
402404
}
403405
}
406+
407+
func uriFromURL(u string) string {
408+
uri, err := url.Parse(u)
409+
if err != nil {
410+
return "/"
411+
}
412+
413+
return uri.RequestURI()
414+
}

‎coderd/externalauth_test.go

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,12 +207,12 @@ func TestExternalAuthManagement(t *testing.T) {
207207
const gitlabID = "fake-gitlab"
208208

209209
githubCalled := false
210-
githubApp := oidctest.NewFakeIDP(t, oidctest.WithServing(), oidctest.WithRefresh(func(email string) error {
210+
githubApp := oidctest.NewFakeIDP(t, oidctest.WithServing(), oidctest.WithRefresh(func(_ string) error {
211211
githubCalled = true
212212
return nil
213213
}))
214214
gitlabCalled := false
215-
gitlab := oidctest.NewFakeIDP(t, oidctest.WithServing(), oidctest.WithRefresh(func(email string) error {
215+
gitlab := oidctest.NewFakeIDP(t, oidctest.WithServing(), oidctest.WithRefresh(func(_ string) error {
216216
gitlabCalled = true
217217
return nil
218218
}))
@@ -508,6 +508,35 @@ func TestExternalAuthCallback(t *testing.T) {
508508
resp = coderdtest.RequestExternalAuthCallback(t, "github", client)
509509
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
510510
})
511+
512+
t.Run("CustomRedirect", func(t *testing.T) {
513+
t.Parallel()
514+
client := coderdtest.New(t, &coderdtest.Options{
515+
IncludeProvisionerDaemon: true,
516+
ExternalAuthConfigs: []*externalauth.Config{{
517+
InstrumentedOAuth2Config: &testutil.OAuth2Config{},
518+
ID: "github",
519+
Regex: regexp.MustCompile(`github\.com`),
520+
Type: codersdk.EnhancedExternalAuthProviderGitHub.String(),
521+
}},
522+
})
523+
maliciousHost := "https://malicious.com"
524+
expectedURI := "/some/path?param=1"
525+
_ = coderdtest.CreateFirstUser(t, client)
526+
resp := coderdtest.RequestExternalAuthCallback(t, "github", client, func(req *http.Request) {
527+
req.AddCookie(&http.Cookie{
528+
Name: codersdk.OAuth2RedirectCookie,
529+
Value: maliciousHost + expectedURI,
530+
})
531+
})
532+
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
533+
location, err := resp.Location()
534+
require.NoError(t, err)
535+
require.Equal(t, expectedURI, location.RequestURI())
536+
require.Equal(t, client.URL.Host, location.Host)
537+
require.NotContains(t, location.String(), maliciousHost)
538+
})
539+
511540
t.Run("ValidateURL", func(t *testing.T) {
512541
t.Parallel()
513542
ctx := testutil.Context(t, testutil.WaitLong)

‎coderd/httpmw/oauth2.go

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"net/http"
7+
"net/url"
78
"reflect"
89

910
"github.com/go-chi/chi/v5"
@@ -85,6 +86,15 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOp
8586

8687
code := r.URL.Query().Get("code")
8788
state := r.URL.Query().Get("state")
89+
redirect := r.URL.Query().Get("redirect")
90+
if redirect != "" {
91+
// We want to ensure that we're only ever redirecting to the application.
92+
// We could be more strict here and check to see if the host matches
93+
// the host of the AccessURL but ultimately as long as our redirect
94+
// url omits a host we're ensuring that we're routing to a path
95+
// local to the application.
96+
redirect = uriFromURL(redirect)
97+
}
8898

8999
if code == "" {
90100
// If the code isn't provided, we'll redirect!
@@ -119,7 +129,7 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOp
119129
// an old redirect could apply!
120130
http.SetCookie(rw, &http.Cookie{
121131
Name: codersdk.OAuth2RedirectCookie,
122-
Value: r.URL.Query().Get("redirect"),
132+
Value: redirect,
123133
Path: "/",
124134
HttpOnly: true,
125135
SameSite: http.SameSiteLaxMode,
@@ -150,7 +160,6 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOp
150160
return
151161
}
152162

153-
var redirect string
154163
stateRedirect, err := r.Cookie(codersdk.OAuth2RedirectCookie)
155164
if err == nil {
156165
redirect = stateRedirect.Value
@@ -302,3 +311,12 @@ func ExtractOAuth2ProviderAppSecret(db database.Store) func(http.Handler) http.H
302311
})
303312
}
304313
}
314+
315+
func uriFromURL(u string) string {
316+
uri, err := url.Parse(u)
317+
if err != nil {
318+
return "/"
319+
}
320+
321+
return uri.RequestURI()
322+
}

‎coderd/httpmw/oauth2_test.go

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,31 @@ func TestOAuth2(t *testing.T) {
6767
cookie := res.Result().Cookies()[1]
6868
require.Equal(t, "/dashboard", cookie.Value)
6969
})
70+
t.Run("OnlyPathBaseRedirect", func(t *testing.T) {
71+
t.Parallel()
72+
// Construct a URI to a potentially malicious
73+
// site and assert that we omit the host
74+
// when redirecting the request.
75+
uri := &url.URL{
76+
Scheme: "https",
77+
Host: "some.bad.domain.com",
78+
Path: "/sadf/asdfasdf",
79+
RawQuery: "foo=hello&bar=world",
80+
}
81+
expectedValue := uri.Path + "?" + uri.RawQuery
82+
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape(uri.String()), nil)
83+
res := httptest.NewRecorder()
84+
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
85+
httpmw.ExtractOAuth2(tp, nil, nil)(nil).ServeHTTP(res, req)
86+
location := res.Header().Get("Location")
87+
if !assert.NotEmpty(t, location) {
88+
return
89+
}
90+
require.Len(t, res.Result().Cookies(), 2)
91+
cookie := res.Result().Cookies()[1]
92+
require.Equal(t, expectedValue, cookie.Value)
93+
})
94+
7095
t.Run("NoState", func(t *testing.T) {
7196
t.Parallel()
7297
req := httptest.NewRequest("GET", "/?code=something", nil)
@@ -108,7 +133,7 @@ func TestOAuth2(t *testing.T) {
108133
})
109134
res := httptest.NewRecorder()
110135
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
111-
httpmw.ExtractOAuth2(tp, nil, nil)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
136+
httpmw.ExtractOAuth2(tp, nil, nil)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
112137
state := httpmw.OAuth2(r)
113138
require.Equal(t, "/dashboard", state.Redirect)
114139
})).ServeHTTP(res, req)

‎coderd/userauth.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -707,9 +707,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
707707
http.SetCookie(rw, cookie)
708708
}
709709

710-
if redirect == "" {
711-
redirect = "/"
712-
}
710+
redirect = uriFromURL(redirect)
713711
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
714712
}
715713

@@ -1085,9 +1083,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
10851083
}
10861084

10871085
redirect := state.Redirect
1088-
if redirect == "" {
1089-
redirect = "/"
1090-
}
1086+
// Strip the host if it exists on the URL to prevent
1087+
// any nefarious redirects.
1088+
redirect = uriFromURL(redirect)
10911089
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
10921090
}
10931091

@@ -1687,7 +1685,7 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data
16871685
}
16881686
}
16891687
var claims OAuthConvertStateClaims
1690-
token, err := jwt.ParseWithClaims(jwtCookie.Value, &claims, func(token *jwt.Token) (interface{}, error) {
1688+
token, err := jwt.ParseWithClaims(jwtCookie.Value, &claims, func(_ *jwt.Token) (interface{}, error) {
16911689
return api.OAuthSigningKey[:], nil
16921690
})
16931691
if xerrors.Is(err, jwt.ErrSignatureInvalid) || !token.Valid {

‎coderd/userauth_test.go

Lines changed: 73 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -354,11 +354,25 @@ func TestUserOAuth2Github(t *testing.T) {
354354
})
355355
numLogs := len(auditor.AuditLogs())
356356

357-
resp := oauth2Callback(t, client)
357+
// Validate that attempting to redirect away from the
358+
// site does not work.
359+
maliciousHost := "https://malicious.com"
360+
expectedPath := "/my/path"
361+
resp := oauth2Callback(t, client, func(req *http.Request) {
362+
// Add the cookie to bypass the parsing in httpmw/oauth2.go
363+
req.AddCookie(&http.Cookie{
364+
Name: codersdk.OAuth2RedirectCookie,
365+
Value: maliciousHost + expectedPath,
366+
})
367+
})
358368
numLogs++ // add an audit log for login
359369

360370
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
361-
371+
redirect, err := resp.Location()
372+
require.NoError(t, err)
373+
require.Equal(t, expectedPath, redirect.Path)
374+
require.Equal(t, client.URL.Host, redirect.Host)
375+
require.NotContains(t, redirect.String(), maliciousHost)
362376
client.SetSessionToken(authCookieValue(resp.Cookies()))
363377
user, err := client.User(context.Background(), "me")
364378
require.NoError(t, err)
@@ -1436,6 +1450,59 @@ func TestUserOIDC(t *testing.T) {
14361450
_, resp := fake.AttemptLogin(t, client, jwt.MapClaims{})
14371451
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
14381452
})
1453+
1454+
t.Run("StripRedirectHost", func(t *testing.T) {
1455+
t.Parallel()
1456+
ctx := testutil.Context(t, testutil.WaitShort)
1457+
1458+
expectedRedirect := "/foo/bar?hello=world&bar=baz"
1459+
redirectURL := "https://malicious" + expectedRedirect
1460+
1461+
callbackPath := fmt.Sprintf("/api/v2/users/oidc/callback?redirect=%s", url.QueryEscape(redirectURL))
1462+
fake := oidctest.NewFakeIDP(t,
1463+
oidctest.WithRefresh(func(_ string) error {
1464+
return xerrors.New("refreshing token should never occur")
1465+
}),
1466+
oidctest.WithServing(),
1467+
oidctest.WithCallbackPath(callbackPath),
1468+
)
1469+
cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) {
1470+
cfg.AllowSignups = true
1471+
})
1472+
1473+
client := coderdtest.New(t, &coderdtest.Options{
1474+
OIDCConfig: cfg,
1475+
})
1476+
1477+
client.HTTPClient.Transport = http.DefaultTransport
1478+
1479+
client.HTTPClient.CheckRedirect = func(*http.Request, []*http.Request) error {
1480+
return http.ErrUseLastResponse
1481+
}
1482+
1483+
claims := jwt.MapClaims{
1484+
"email": "user@example.com",
1485+
"email_verified": true,
1486+
}
1487+
1488+
// Perform the login
1489+
loginClient, resp := fake.LoginWithClient(t, client, claims)
1490+
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
1491+
1492+
// Get the location from the response
1493+
location, err := resp.Location()
1494+
require.NoError(t, err)
1495+
1496+
// Check that the redirect URL has been stripped of its malicious host
1497+
require.Equal(t, expectedRedirect, location.RequestURI())
1498+
require.Equal(t, client.URL.Host, location.Host)
1499+
require.NotContains(t, location.String(), "malicious")
1500+
1501+
// Verify the user was created
1502+
user, err := loginClient.User(ctx, "me")
1503+
require.NoError(t, err)
1504+
require.Equal(t, "user@example.com", user.Email)
1505+
})
14391506
}
14401507

14411508
func TestUserLogout(t *testing.T) {
@@ -1587,7 +1654,7 @@ func TestOIDCSkipIssuer(t *testing.T) {
15871654
require.Equal(t, found.LoginType, codersdk.LoginTypeOIDC)
15881655
}
15891656

1590-
func oauth2Callback(t *testing.T, client *codersdk.Client) *http.Response {
1657+
func oauth2Callback(t *testing.T, client *codersdk.Client, opts ...func(*http.Request)) *http.Response {
15911658
client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
15921659
return http.ErrUseLastResponse
15931660
}
@@ -1597,6 +1664,9 @@ func oauth2Callback(t *testing.T, client *codersdk.Client) *http.Response {
15971664
require.NoError(t, err)
15981665
req, err := http.NewRequestWithContext(context.Background(), "GET", oauthURL.String(), nil)
15991666
require.NoError(t, err)
1667+
for _, opt := range opts {
1668+
opt(req)
1669+
}
16001670
req.AddCookie(&http.Cookie{
16011671
Name: codersdk.OAuth2StateCookie,
16021672
Value: state,

0 commit comments

Comments
 (0)