user-configured redirect URL clobbered in oauthproxy.go

[dt-rush]

see line 155 of oauthproxy.go (link)

We literally overwrite whatever the value is with fmt.Sprintf("%s/callback", opts.ProxyPrefix) even if the user provided a redirect URL they want to use.

I have a PR for this which I tested and confirmed to resolve the issue. Will link.

[JoelSpeed]

This is now fixed since #10 has been merged

[Ghazgkull]

Awesome, thanks. For planning/adoption purposes, might I ask how soon we could expect to see a new Release available with this fix?

[JoelSpeed]

There are a couple of PRs open for optimizations and bug fixes that I would like to get merged before we make a new release, hopefully this should be within the next couple of weeks

[Ghazgkull]

@JoelSpeed I went ahead and tested this change today and the behavior isn't what I expected.

I thought this change would update oauth2_proxy so that it would simply hand out whatever redirect-url was configured. I expected this would allow me to configure oauth2_proxy with a redirect url independent of the proxyPrefix used for internal routing.

For example, I am running oauth2_proxy with the default proxy path... receiving all traffic under the /oauth... But I wanted to give out a redirect URL with the path of an external ingress which sits out in front of the proxy (a path that oauth2_proxy never sees). Something like /myprotectedpath/oauth/callback. When the callback comes back to my ingress at /myprotectedpath/oauth/callback, I would rewrite the URL to /oauth/callback before sending it on to oauth2_proxy.

But in testing and then looking at the code more closely, this change affects both the path the oauth2_proxy gives out during the oauth dance and it also updates the internal path routing within the proxy itself... So the external path and internal routing are still forced to match. In my example, oauth2_proxy would hand out the path /myprotectedpath/oauth/callback and then also only handle the callback if it came to /myprotectedpath/oauth/callback... while all other routes it would expect under /oauth.

So it feels like we've ended up in a state where we have a proxy-path for only one route? Is this the expected behavior?

[Ghazgkull]

Looking at the PR (https://github.com/pusher/oauth2_proxy/pull/10/files), I feel like the change to oauthproxy.go lines 186-188 are good because they fix the proxy to hand out whatever redirect-url was configured. But the change to oauthproxy.go line 223 is incorrect; that line should have been kept the same to keep oauth2_proxy's internal routing consistent based on the proxy path.

[Ghazgkull]

@dt-rush Please see my comments above. What do you think?

[Ghazgkull]

@JoelSpeed Can we re-open this issue? I really think that the PR which was merged is badly flawed.

[ploxiln]

I suggest opening a PR with your suggested adjustment, and having further discussion there.
(I think you're right, btw.)

[JoelSpeed]

@Ghazgkull Please open a PR with your suggested change as @ploxiln has suggested and I will take a look

[Ghazgkull]

@ploxiln @JoelSpeed Oh. Derp. I just opened PR 99, but I see you guys already made the change. Cheers.

[ploxiln]

nope, sorry to cause confusion, that was related to my fork, which has diverged, the fix is not in this repo

[JoelSpeed]

@ploxiln @Ghazgkull could you take a look at #101 and verify that this is the change we need to make here?