Change error type for redirect parsing errors (oauth2-proxy#1649)

* Change error type for redirect parsing errors This changes the error type returned when the proxy fails to parse the redirect target to be a 400 error instead of a 500 error. As far as I can tell, the only way that this can fail is a failure to parse the properties of the request to identity the redirect target. This indicates that the user has sent a malformed request, and so should result in a 400 rather than a 500. I've added a test to exercise this, based on a real work example. * Update changelog Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
atiasn · May 20, 2022 · 743c344 · 743c344
1 parent 086b869
commit 743c344
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 1 deletion.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -39,6 +39,7 @@ If you are using an architecture specific tag (ex: v7.2.1-arm64) you should move
 - [#1286](https://github.com/oauth2-proxy/oauth2-proxy/pull/1286) Add the `allowed_email_domains` and the `allowed_groups` on the `auth_request` + support standard wildcard char for validation with sub-domain and email-domain. (@w3st3ry @armandpicard)
 - [#1361](https://github.com/oauth2-proxy/oauth2-proxy/pull/1541) PKCE Code Challenge Support - RFC-7636 (@braunsonm)
 - [#1594](https://github.com/oauth2-proxy/oauth2-proxy/pull/1594) Release ARMv8 docker images (@braunsonm)
+- [#1649](https://github.com/oauth2-proxy/oauth2-proxy/pull/1649) Return a 400 instead of a 500 when a request contains an invalid redirect target (@niksko)
 - [#1638](https://github.com/oauth2-proxy/oauth2-proxy/pull/1638) Implement configurable upstream timeout (@jacksgt)
 
 # V7.2.1

diff --git a/oauthproxy.go b/oauthproxy.go
@@ -714,7 +714,7 @@ func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, ove
 	appRedirect, err := p.appDirector.GetRedirect(req)
 	if err != nil {
 		logger.Errorf("Error obtaining application redirect: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
+		p.ErrorPage(rw, req, http.StatusBadRequest, err.Error())
 		return
 	}
 

diff --git a/oauthproxy_test.go b/oauthproxy_test.go
@@ -678,6 +678,17 @@ func TestSignInPageIncludesTargetRedirect(t *testing.T) {
 	}
 }
 
+func TestSignInPageInvalidQueryStringReturnsBadRequest(t *testing.T) {
+	sipTest, err := NewSignInPageTest(true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	const endpoint = "/?q=%va"
+
+	code, _ := sipTest.GetEndpoint(endpoint)
+	assert.Equal(t, 400, code)
+}
+
 func TestSignInPageDirectAccessRedirectsToRoot(t *testing.T) {
 	sipTest, err := NewSignInPageTest(false)
 	if err != nil {