Use HTTP 303 for redirects, instead HTTP 307

Bruno Oliveira da Silva · Bruno Oliveira da Silva · commit 33c23da83436 · 2020-06-04T08:47:40.000-03:00
Based on the security best practices we should not use HTTP 307 for redirects, instead HTTP 303 should be used. The option `InvalidAuthRedirectsWith303` was removed, as does not make sense to have it anymore. More details: https://tools.ietf.org/id/draft-ietf-oauth-security-topics-08.html#rfc.section.3.9 Resolves #627
diff --git a/doc.go b/doc.go
@@ -306,8 +306,6 @@ type Config struct {
 	// EncryptionKey is the encryption key used to encrypt the refresh token
 	EncryptionKey string `json:"encryption-key" yaml:"encryption-key" usage:"encryption key used to encryption the session state" env:"ENCRYPTION_KEY"`
 
-	// InvalidAuthRedirectsWith303 will make requests with invalid auth headers redirect using HTTP 303 instead of HTTP 307.  See github.com/keycloak/keycloak-gatekeeper/issues/292 for context.
-	InvalidAuthRedirectsWith303 bool `json:"invalid-auth-redirects-with-303" yaml:"invalid-auth-redirects-with-303" usage:"use HTTP 303 redirects instead of 307 for invalid auth tokens"`
 	// NoRedirects informs we should hand back a 401 not a redirect
 	NoRedirects bool `json:"no-redirects" yaml:"no-redirects" usage:"do not have back redirects when no authentication is present, 401 them"`
 	// SkipTokenVerification tells the service to skipp verifying the access token - for testing purposes
diff --git a/handlers.go b/handlers.go
@@ -101,7 +101,7 @@ func (r *oauthProxy) oauthAuthorizationHandler(w http.ResponseWriter, req *http.
 		return
 	}
 
-	r.redirectToURL(authURL, w, req, http.StatusTemporaryRedirect)
+	r.redirectToURL(authURL, w, req, http.StatusSeeOther)
 }
 
 // getClientAuthMethod maps the config value CLIENT_AUTH_METHOD to valid OAuth2 auth method keys
@@ -229,7 +229,7 @@ func (r *oauthProxy) oauthCallbackHandler(w http.ResponseWriter, req *http.Reque
 		}
 	}
 
-	r.redirectToURL(redirectURI, w, req, http.StatusTemporaryRedirect)
+	r.redirectToURL(redirectURI, w, req, http.StatusSeeOther)
 }
 
 // loginHandler provide's a generic endpoint for clients to perform a user_credentials login to the provider
@@ -359,7 +359,7 @@ func (r *oauthProxy) logoutHandler(w http.ResponseWriter, req *http.Request) {
 			}
 		}
 
-		r.redirectToURL(fmt.Sprintf("%s?redirect_uri=%s", sendTo, url.QueryEscape(redirectURL)), w, req, http.StatusTemporaryRedirect)
+		r.redirectToURL(fmt.Sprintf("%s?redirect_uri=%s", sendTo, url.QueryEscape(redirectURL)), w, req, http.StatusSeeOther)
 
 		return
 	}
@@ -411,7 +411,7 @@ func (r *oauthProxy) logoutHandler(w http.ResponseWriter, req *http.Request) {
 	}
 	// step: should we redirect the user
 	if redirectURL != "" {
-		r.redirectToURL(redirectURL, w, req, http.StatusTemporaryRedirect)
+		r.redirectToURL(redirectURL, w, req, http.StatusSeeOther)
 	}
 }
 
diff --git a/handlers_test.go b/handlers_test.go
@@ -179,7 +179,7 @@ func TestLogoutHandlerGood(t *testing.T) {
 		{
 			URI:              c.WithOAuthURI(logoutURL) + "?redirect=http://example.com",
 			HasToken:         true,
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 			ExpectedLocation: "http://example.com",
 		},
 	}
@@ -220,7 +220,7 @@ func TestServiceRedirect(t *testing.T) {
 		{
 			URI:              "/admin",
 			Redirects:        true,
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 			ExpectedLocation: "/oauth/authorize?state",
 		},
 		{
@@ -248,25 +248,25 @@ func TestAuthorizationURL(t *testing.T) {
 			URI:              "/admin",
 			Redirects:        true,
 			ExpectedLocation: "/oauth/authorize?state",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:              "/admin/test",
 			Redirects:        true,
 			ExpectedLocation: "/oauth/authorize?state",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:              "/help/../admin",
 			Redirects:        true,
 			ExpectedLocation: "/oauth/authorize?state",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:              "/admin?test=yes&test1=test",
 			Redirects:        true,
 			ExpectedLocation: "/oauth/authorize?state",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:          "/oauth/test",
@@ -298,19 +298,19 @@ func TestCallbackURL(t *testing.T) {
 			URI:              cfg.WithOAuthURI(callbackURL) + "?code=fake",
 			ExpectedCookies:  map[string]string{cfg.CookieAccessName: ""},
 			ExpectedLocation: "/",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:              cfg.WithOAuthURI(callbackURL) + "?code=fake&state=/admin",
 			ExpectedCookies:  map[string]string{cfg.CookieAccessName: ""},
 			ExpectedLocation: "/",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:              cfg.WithOAuthURI(callbackURL) + "?code=fake&state=L2FkbWlu",
 			ExpectedCookies:  map[string]string{cfg.CookieAccessName: ""},
 			ExpectedLocation: "/",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 	}
 	newFakeProxy(cfg).RunTests(t, requests)
diff --git a/middleware_test.go b/middleware_test.go
@@ -356,7 +356,7 @@ func TestOauthRequests(t *testing.T) {
 		{
 			URI:          "/oauth/authorize",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{
 			URI:          "/oauth/callback",
@@ -379,7 +379,7 @@ func TestOauthRequestsWithBaseURI(t *testing.T) {
 		{
 			URI:          "/base-uri/oauth/authorize",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{
 			URI:          "/base-uri/oauth/callback",
@@ -614,22 +614,22 @@ func TestNoProxyingRequests(t *testing.T) {
 		{ // check for escaping
 			URI:          "/.%2e/.%2e/.%2e/.%2e/.%2e/.%2e/.%2e/etc/passwd",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for escaping
 			URI:          "/.%2e/.%2e/.%2e/.%2e/.%2e/.%2e/.%2e/",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for escaping
 			URI:          "/../%2e",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for escaping
 			URI:          "",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 	}
 	newFakeProxy(c).RunTests(t, requests)
@@ -650,27 +650,27 @@ func TestStrangeAdminRequests(t *testing.T) {
 		{ // check for escaping
 			URI:          "//admin%2Ftest",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for escaping
 			URI:          "///admin/../admin//%2Ftest",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for escaping
 			URI:          "/admin%2Ftest",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for prefix slashs
 			URI:          "/" + testAdminURI,
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for double slashs
 			URI:          testAdminURI,
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for double slashs no redirects
 			URI:          "/admin//test",
@@ -681,7 +681,7 @@ func TestStrangeAdminRequests(t *testing.T) {
 		{ // check for dodgy url
 			URI:          "//admin/.." + testAdminURI,
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for it works
 			URI:           "/" + testAdminURI,
@@ -942,7 +942,7 @@ func TestRolePermissionsMiddleware(t *testing.T) {
 		{ // check for redirect
 			URI:          "/",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check with a token but not test role
 			URI:          "/",
diff --git a/misc.go b/misc.go
@@ -107,11 +107,7 @@ func (r *oauthProxy) redirectToAuthorization(w http.ResponseWriter, req *http.Re
 		w.WriteHeader(http.StatusForbidden)
 		return r.revokeProxy(w, req)
 	}
-	if r.config.InvalidAuthRedirectsWith303 {
-		r.redirectToURL(r.config.WithOAuthURI(authorizationURL+authQuery), w, req, http.StatusSeeOther)
-	} else {
-		r.redirectToURL(r.config.WithOAuthURI(authorizationURL+authQuery), w, req, http.StatusTemporaryRedirect)
-	}
+	r.redirectToURL(r.config.WithOAuthURI(authorizationURL+authQuery), w, req, http.StatusSeeOther)
 
 	return r.revokeProxy(w, req)
 }
diff --git a/misc_test.go b/misc_test.go
@@ -36,15 +36,14 @@ func TestRedirectToAuthorization(t *testing.T) {
 			URI:              "/admin",
 			Redirects:        true,
 			ExpectedLocation: "/oauth/authorize?state",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 	}
 	newFakeProxy(nil).RunTests(t, requests)
 }
 
 func TestRedirectToAuthorizationWith303Enabled(t *testing.T) {
 	cfg := newFakeKeycloakConfig()
-	cfg.InvalidAuthRedirectsWith303 = true
 
 	requests := []fakeRequest{
 		{
diff --git a/oauth_test.go b/oauth_test.go
@@ -188,7 +188,7 @@ func (r *fakeAuthServer) authHandler(w http.ResponseWriter, req *http.Request) {
 	}
 	redirectionURL := fmt.Sprintf("%s?state=%s&code=%s", redirect, state, getRandomString(32))
 
-	http.Redirect(w, req, redirectionURL, http.StatusTemporaryRedirect)
+	http.Redirect(w, req, redirectionURL, http.StatusSeeOther)
 }
 
 func (r *fakeAuthServer) logoutHandler(w http.ResponseWriter, req *http.Request) {
diff --git a/server_test.go b/server_test.go
@@ -508,7 +508,7 @@ func makeTestCodeFlowLogin(location string) (*http.Response, error) {
 		if err != nil {
 			return nil, err
 		}
-		if resp.StatusCode != http.StatusTemporaryRedirect {
+		if resp.StatusCode != http.StatusSeeOther {
 			return nil, errors.New("no redirection found in resp")
 		}
 		location = resp.Header.Get("Location")

Original file line number	Diff line number	Diff line change
`@@ -101,7 +101,7 @@ func (r oauthProxy) oauthAuthorizationHandler(w http.ResponseWriter, req http.`
`101`	`101`	`return`
`102`	`102`	`}`
`103`	`103`
`104`		`- r.redirectToURL(authURL, w, req, http.StatusTemporaryRedirect)`
	`104`	`+ r.redirectToURL(authURL, w, req, http.StatusSeeOther)`
`105`	`105`	`}`
`106`	`106`
`107`	`107`	`// getClientAuthMethod maps the config value CLIENT_AUTH_METHOD to valid OAuth2 auth method keys`
`@@ -229,7 +229,7 @@ func (r oauthProxy) oauthCallbackHandler(w http.ResponseWriter, req http.Reque`
`229`	`229`	`}`
`230`	`230`	`}`
`231`	`231`
`232`		`- r.redirectToURL(redirectURI, w, req, http.StatusTemporaryRedirect)`
	`232`	`+ r.redirectToURL(redirectURI, w, req, http.StatusSeeOther)`
`233`	`233`	`}`
`234`	`234`
`235`	`235`	`// loginHandler provide's a generic endpoint for clients to perform a user_credentials login to the provider`
`@@ -359,7 +359,7 @@ func (r oauthProxy) logoutHandler(w http.ResponseWriter, req http.Request) {`
`359`	`359`	`}`
`360`	`360`	`}`
`361`	`361`
`362`		`- r.redirectToURL(fmt.Sprintf("%s?redirect_uri=%s", sendTo, url.QueryEscape(redirectURL)), w, req, http.StatusTemporaryRedirect)`
	`362`	`+ r.redirectToURL(fmt.Sprintf("%s?redirect_uri=%s", sendTo, url.QueryEscape(redirectURL)), w, req, http.StatusSeeOther)`
`363`	`363`
`364`	`364`	`return`
`365`	`365`	`}`
`@@ -411,7 +411,7 @@ func (r oauthProxy) logoutHandler(w http.ResponseWriter, req http.Request) {`
`411`	`411`	`}`
`412`	`412`	`// step: should we redirect the user`
`413`	`413`	`if redirectURL != "" {`
`414`		`- r.redirectToURL(redirectURL, w, req, http.StatusTemporaryRedirect)`
	`414`	`+ r.redirectToURL(redirectURL, w, req, http.StatusSeeOther)`
`415`	`415`	`}`
`416`	`416`	`}`
`417`	`417`
Original file line number	Diff line number	Diff line change
`@@ -107,11 +107,7 @@ func (r oauthProxy) redirectToAuthorization(w http.ResponseWriter, req http.Re`
`107`	`107`	`w.WriteHeader(http.StatusForbidden)`
`108`	`108`	`return r.revokeProxy(w, req)`
`109`	`109`	`}`
`110`		`- if r.config.InvalidAuthRedirectsWith303 {`
`111`		`- r.redirectToURL(r.config.WithOAuthURI(authorizationURL+authQuery), w, req, http.StatusSeeOther)`
`112`		`- } else {`
`113`		`- r.redirectToURL(r.config.WithOAuthURI(authorizationURL+authQuery), w, req, http.StatusTemporaryRedirect)`
`114`		`- }`
	`110`	`+ r.redirectToURL(r.config.WithOAuthURI(authorizationURL+authQuery), w, req, http.StatusSeeOther)`
`115`	`111`
`116`	`112`	`return r.revokeProxy(w, req)`
`117`	`113`	`}`
Original file line number	Diff line number	Diff line change
`@@ -36,15 +36,14 @@ func TestRedirectToAuthorization(t *testing.T) {`
`36`	`36`	`URI: "/admin",`
`37`	`37`	`Redirects: true,`
`38`	`38`	`ExpectedLocation: "/oauth/authorize?state",`
`39`		`- ExpectedCode: http.StatusTemporaryRedirect,`
	`39`	`+ ExpectedCode: http.StatusSeeOther,`
`40`	`40`	`},`
`41`	`41`	`}`
`42`	`42`	`newFakeProxy(nil).RunTests(t, requests)`
`43`	`43`	`}`
`44`	`44`
`45`	`45`	`func TestRedirectToAuthorizationWith303Enabled(t *testing.T) {`
`46`	`46`	`cfg := newFakeKeycloakConfig()`
`47`		`- cfg.InvalidAuthRedirectsWith303 = true`
`48`	`47`
`49`	`48`	`requests := []fakeRequest{`
`50`	`49`	`{`
Original file line number	Diff line number	Diff line change
`@@ -188,7 +188,7 @@ func (r fakeAuthServer) authHandler(w http.ResponseWriter, req http.Request) {`
`188`	`188`	`}`
`189`	`189`	`redirectionURL := fmt.Sprintf("%s?state=%s&code=%s", redirect, state, getRandomString(32))`
`190`	`190`
`191`		`- http.Redirect(w, req, redirectionURL, http.StatusTemporaryRedirect)`
	`191`	`+ http.Redirect(w, req, redirectionURL, http.StatusSeeOther)`
`192`	`192`	`}`
`193`	`193`
`194`	`194`	`func (r fakeAuthServer) logoutHandler(w http.ResponseWriter, req http.Request) {`
Original file line number	Diff line number	Diff line change
`@@ -508,7 +508,7 @@ func makeTestCodeFlowLogin(location string) (*http.Response, error) {`
`508`	`508`	`if err != nil {`
`509`	`509`	`return nil, err`
`510`	`510`	`}`
`511`		`- if resp.StatusCode != http.StatusTemporaryRedirect {`
	`511`	`+ if resp.StatusCode != http.StatusSeeOther {`
`512`	`512`	`return nil, errors.New("no redirection found in resp")`
`513`	`513`	`}`
`514`	`514`	`location = resp.Header.Get("Location")`