Preserve query when building redirect (fix for #695)

[avoltz]

Description

Use URL.RequestURI() in GetRedirect() to preserve any query string

Note: If you cherry-pick just the unit test from this branch the UT will express the same behavior seen in #695

Motivation and Context

Fixes: #695

How Has This Been Tested?

Added unit test for a request with querystring present.
Also built a new oauth2-proxy with these changes and ran it in my env. Confirmed that the redirect works as expected behind the reverse proxy (as described in the linked bug).

Checklist:

• My change requires a change to the documentation or CHANGELOG.
• I have updated the documentation/CHANGELOG accordingly.
• I have created a feature (non-master) branch for my PR.

I'm not sure if these checkbox items apply, but let me know.

[NickMeves]

Can you add a fragment URI test to the table as well?

[JoelSpeed]

What's the expected output here? Fragments aren't sent by a browser as part of a request so I don't think a redirect URL could ever contain one could it?

[avoltz]

At first I actually used a fragment, but I've found in my testing that the fragment is reapplied by the browser (chrome) after redirect. This doc seems to indicate Fragment is stripped, as @JoelSpeed mentioned: https://golang.org/pkg/net/url/#ParseRequestURI

At first I had a fragment in this test case, but then I needed this change to add fragment because URL.RequestURI() stripped it:

diff --git a/oauthproxy.go b/oauthproxy.go
index 4b310b9..5fa2572 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -580,7 +580,12 @@ func (p *OAuthProxy) GetRedirect(req *http.Request) (redirect string, err error)
                redirect = req.Form.Get("rd")
        }
        if !p.IsValidRedirect(redirect) {
-               redirect = req.URL.Path
+               // Use RequestURI to preserve ?query
+               redirect = req.URL.RequestURI()
+               // RequestURI() will not provide a fragment if present, so grab that here
+               if req.URL.Fragment != "" {
+                       redirect += "#" + req.URL.Fragment
+               }
                if strings.HasPrefix(redirect, p.ProxyPrefix) {
                        redirect = "/"
                }

I think this code would only be needed to satisfy tests, but not really used at runtime. Let me know what you think.

[NickMeves]

Intestering. I didn't realize that's how it worked under the hood. TIL 😄

[NickMeves]

Good find. Thanks for the PR!

Overall looks good. Can you add a /fragment#blah test case and go ahead and add an entry to the CHANGELOG with this PR's link & description?

[JoelSpeed]

LGTM, will wait for @NickMeves to approve before merge though, to allow for continued discussion of fragments

[NickMeves]

Move this line down to the ## Changes since v6.0.0 section (starting on line 13)

[avoltz]

Oops, yeah that makes more sense. Fixed in latest update. Thanks!

[JoelSpeed]

LGTM, thanks @avoltz