diff --git a/CHANGELOG.md b/CHANGELOG.md index 2aa2ef46..9245bf12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ## Changes since v7.13.0 - [#3197](https://github.com/oauth2-proxy/oauth2-proxy/pull/3197) fix: NewRemoteKeySet is not using DefaultHTTPClient (@rsrdesarrollo / @tuunit) +- [#3219](https://github.com/oauth2-proxy/oauth2-proxy/pull/3219) fix: escape OAuth state redirect URLs to preserve query parameters (@CallumWayve) # V7.13.0 diff --git a/oauthproxy.go b/oauthproxy.go index c6db18a7..60524935 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -1250,6 +1250,10 @@ func checkAllowedEmails(req *http.Request, s *sessionsapi.SessionState) bool { // encodeState builds the OAuth state param out of our nonce and // original application redirect func encodeState(nonce string, redirect string, encode bool) string { + if !encode { + redirect = url.QueryEscape(redirect) + } + rawString := fmt.Sprintf("%v:%v", nonce, redirect) if encode { return base64.RawURLEncoding.EncodeToString([]byte(rawString)) @@ -1270,7 +1274,17 @@ func decodeState(state string, encode bool) (string, string, error) { if len(parsedState) != 2 { return "", "", errors.New("invalid length") } - return parsedState[0], parsedState[1], nil + nonce := parsedState[0] + redirect := parsedState[1] + + if encode { + redirect, err := url.QueryUnescape(redirect) + if err != nil { + return "", "", fmt.Errorf("couldn't parse redirect url: %w", err) + } + } + + return nonce, redirect, nil } // addHeadersForProxying adds the appropriate headers the request / response for proxying diff --git a/oauthproxy_test.go b/oauthproxy_test.go index ccabdbbd..171a9a7d 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -3356,26 +3356,45 @@ func TestAuthOnlyAllowedEmailDomains(t *testing.T) { } func TestStateEncodesCorrectly(t *testing.T) { - state := "some_state_to_test" + state := "https://example.com/callback?foo=bar&baz=qux" nonce := "some_nonce_to_test" encodedResult := encodeState(nonce, state, true) - assert.Equal(t, "c29tZV9ub25jZV90b190ZXN0OnNvbWVfc3RhdGVfdG9fdGVzdA", encodedResult) + expectedEncoded := base64.RawURLEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", nonce, state))) + assert.Equal(t, expectedEncoded, encodedResult) notEncodedResult := encodeState(nonce, state, false) - assert.Equal(t, "some_nonce_to_test:some_state_to_test", notEncodedResult) + expectedUnencoded := fmt.Sprintf("%s:%s", nonce, url.QueryEscape(state)) + assert.Equal(t, expectedUnencoded, notEncodedResult) + assert.NotContains(t, notEncodedResult, "&") } func TestStateDecodesCorrectly(t *testing.T) { - nonce, redirect, _ := decodeState("c29tZV9ub25jZV90b190ZXN0OnNvbWVfc3RhdGVfdG9fdGVzdA", true) + state := "https://example.com/callback?foo=bar&baz=qux" + nonce := "some_nonce_to_test" - assert.Equal(t, "some_nonce_to_test", nonce) - assert.Equal(t, "some_state_to_test", redirect) + encodedState := base64.RawURLEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", nonce, state))) + decodedNonce, decodedRedirect, err := decodeState(encodedState, true) + assert.NoError(t, err) + assert.Equal(t, nonce, decodedNonce) + assert.Equal(t, state, decodedRedirect) - nonce2, redirect2, _ := decodeState("some_nonce_to_test:some_state_to_test", false) + rawState := fmt.Sprintf("%s:%s", nonce, url.QueryEscape(state)) + decodedNonce2, decodedRedirect2, err := decodeState(rawState, false) + assert.NoError(t, err) + assert.Equal(t, nonce, decodedNonce2) + assert.Equal(t, state, decodedRedirect2) +} - assert.Equal(t, "some_nonce_to_test", nonce2) - assert.Equal(t, "some_state_to_test", redirect2) +func TestStateRoundTripWithMultipleQueryParameters(t *testing.T) { + state := "https://example.com/callback?foo=bar&baz=qux&zap=zazzle" + nonce := "another_nonce" + + encoded := encodeState(nonce, state, false) + decodedNonce, decodedRedirect, err := decodeState(encoded, false) + assert.NoError(t, err) + assert.Equal(t, nonce, decodedNonce) + assert.Equal(t, state, decodedRedirect) } func TestAuthOnlyAllowedEmails(t *testing.T) {