Skip to content

Conversation

webnard
Copy link
Contributor

@webnard webnard commented Aug 17, 2019

Docs showed that the X-Auth-Request-Redirect header can specify a redirect URI, but only the rd POST parameter was being honored

This fixes that.

  • 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.

Docs showed that the X-Auth-Request-Redirect header can specify a redirect URI, but only the rd POST parameter was being honored
This fixes that.
@webnard webnard requested a review from a team August 17, 2019 20:54
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think previously the header was honoured when going via the login page but not when going directly to the start page as used in the Nginx Auth Request

Happy with the code, could you please add a note to the changelog though and could you expand a little on how you've tested this? Just trying to understand what your use case and behaviour observations have been, thanks

@webnard
Copy link
Contributor Author

webnard commented Sep 19, 2019

Hi @JoelSpeed -- sorry for the delay there.

We've got a domain that redirects all subdomain requests to internal IP addresses after a successful auth. This directive wasn't working before my changes:

resolver 172.31.0.2; # internal dns lookup
proxy_set_header X-Auth-Request-Redirect $scheme://$host$request_uri;

If the auth request initially came with a POST request with the body rd=http://somesite.somedomain.com, the rd value was respected and redirected accordingly. This new code still gives precedence to the rd form value.

I don't remember exactly how I tested this (probably a couple of curl requests), but I can flesh something out if important.

@webnard webnard requested a review from JoelSpeed September 26, 2019 15:20
@iain-buclaw-sociomantic
Copy link
Contributor

Note, I suggested this nearly 2 years ago in bitly/oauth2_proxy#521, we've been running with the patch in production since that time as well.

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, LGTM

Copy link
Contributor

@loshz loshz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you and congrats on your first contribution! 🎉

@JoelSpeed JoelSpeed merged commit 721d28b into oauth2-proxy:master Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants