fix: skip provider button auth only redirect#3309
Conversation
|
@StefanMarkmann as we are now a CNCF project we are required to have all commits signed as "Developer Certificate of Origin": The rest looks great! |
When SkipProviderButton is enabled and a user needs to login, the AuthOnly endpoint now returns a 302 redirect directly to the OAuth provider instead of returning 401. This fixes an issue with nginx auth_request architecture where 401 triggers error_page handling, which can break redirect flows because nginx overrides the status code (e.g., to 403), and browsers don't follow Location headers for non-3xx responses. Fixes: oauth2-proxy#334 Signed-off-by: Stefan Markmann <stefan@markmann.net>
Signed-off-by: Stefan Markmann <stefan@markmann.net>
Improve TestAuthOnlyEndpointRedirectWithSkipProviderButton to verify that the Location header actually redirects to the OAuth provider's authorize endpoint with required parameters (client_id, redirect_uri, state), not just that a Location header exists. Signed-off-by: Stefan Markmann <stefan@markmann.net>
Move the SkipProviderButton check outside of the nested err != nil block using an if-else structure. This makes the special case more visible and reduces nesting depth without changing behavior. Signed-off-by: Stefan Markmann <stefan@markmann.net>
73dce1e to
e9f0629
Compare
Signed-off-by: Jan Larwig <jan@larwig.com>
|
@StefanMarkmann amazing this bug has been in the open for way to long! |
|
Hi, nginx
My nginx started returning 500's when I got this update with this error message: I've rolled back to 7.14.0 for now but just a heads up. |
|
Thanks everyone for the reports and for the review feedback. After a deeper investigation, I realized that my earlier analysis (and proposed fix) was incorrect. In the documentation example, nginx rewrites the 401 from The correct pattern is to keep I will update the documentation to clarify this nginx configuration. I can't revert or close this PR, so please keep it excluded. |
|
@StefanMarkmann unfortunately I just released it |
…)" This reverts commit 9c61c49.
…)" This reverts commit 9c61c49. ## Why This Revert is Necessary The original fix broke nginx deployments using `auth_request`. When `/oauth2/auth` returns 302, nginx's `auth_request` module treats this as an internal error: [error] auth request unexpected status: 302 while sending to client nginx then returns **500 Internal Server Error** to the browser. ## Root Cause The nginx `auth_request` module has strict semantics (non-negotiable): | Subrequest status | nginx behavior | |---|---| | 2xx | Allow request | | 401 / 403 | Deny → trigger `error_page` | | **Any other status** | **Internal error → 500** | The `/oauth2/auth` endpoint is used as a **policy oracle** (yes/no decision), not as a browser-facing endpoint. It cannot return redirects. ## User Impact Any nginx deployment with: - `skip-provider-button=true` - Using `auth_request` directive Will receive 500 errors instead of the expected authentication flow. ## The Correct Solution The correct fix for oauth2-proxy#334 is a **documentation update**, not a code change: ```nginx error_page 401 = @oauth2_signin; location @oauth2_signin { return 302 /oauth2/sign_in?rd=$scheme://$host$request_uri; } ``` This keeps `/oauth2/auth` as a pure 401/2xx oracle and lets nginx perform the proper 302 redirect to the browser. ## Related - Original Issue: oauth2-proxy#334 - Broken PR: oauth2-proxy#3309 Signed-off-by: Stefan Markmann <stefan@markmann.net>
)" This reverts commit 9c61c49. The original fix broke nginx deployments using `auth_request`. When `/oauth2/auth` returns 302, nginx's `auth_request` module treats this as an internal error: [error] auth request unexpected status: 302 while sending to client nginx then returns **500 Internal Server Error** to the browser. > If the subrequest returns a 2xx response code, the access is allowed. If it returns 401 or 403, > the access is denied with the corresponding error code. Any other response code returned by the > subrequest is considered an error. https://nginx.org/en/docs/http/ngx_http_auth_request_module.html The nginx `auth_request` module has strict semantics (non-negotiable): | Subrequest status | nginx behavior | |---|---| | 2xx | Allow request | | 401 / 403 | Deny → trigger `error_page` | | **Any other status** | **Internal error → 500** | The `/oauth2/auth` endpoint is used as a **policy oracle** (yes/no decision), not as a browser-facing endpoint. It cannot return redirects. Any nginx deployment with: - `skip-provider-button=true` - Using `auth_request` directive Will receive 500 errors instead of the expected authentication flow. The correct fix for oauth2-proxy#334 is a **documentation update**, not a code change: ```nginx error_page 401 = @oauth2_signin; location @oauth2_signin { return 302 /oauth2/sign_in?rd=$scheme://$host$request_uri; } ``` This keeps `/oauth2/auth` as a pure 401/2xx oracle and lets nginx perform the proper 302 redirect to the browser. - Original Issue: oauth2-proxy#334 - Regression introduced in PR: oauth2-proxy#3309 Signed-off-by: Stefan Markmann <stefan@markmann.net> Signed-off-by: Jan Larwig <jan@larwig.com>
This reverts commit 9c61c49. The original fix broke nginx deployments using `auth_request`. When `/oauth2/auth` returns 302, nginx's `auth_request` module treats this as an internal error: [error] auth request unexpected status: 302 while sending to client nginx then returns **500 Internal Server Error** to the browser. > If the subrequest returns a 2xx response code, the access is allowed. If it returns 401 or 403, > the access is denied with the corresponding error code. Any other response code returned by the > subrequest is considered an error. https://nginx.org/en/docs/http/ngx_http_auth_request_module.html The nginx `auth_request` module has strict semantics (non-negotiable): | Subrequest status | nginx behavior | |---|---| | 2xx | Allow request | | 401 / 403 | Deny → trigger `error_page` | | **Any other status** | **Internal error → 500** | The `/oauth2/auth` endpoint is used as a **policy oracle** (yes/no decision), not as a browser-facing endpoint. It cannot return redirects. Any nginx deployment with: - `skip-provider-button=true` - Using `auth_request` directive Will receive 500 errors instead of the expected authentication flow. The correct fix for #334 is a **documentation update**, not a code change: ```nginx error_page 401 = @oauth2_signin; location @oauth2_signin { return 302 /oauth2/sign_in?rd=$scheme://$host$request_uri; } ``` This keeps `/oauth2/auth` as a pure 401/2xx oracle and lets nginx perform the proper 302 redirect to the browser. - Original Issue: #334 - Regression introduced in PR: #3309 Signed-off-by: Stefan Markmann <stefan@markmann.net> Signed-off-by: Jan Larwig <jan@larwig.com> Co-authored-by: Jan Larwig <jan@larwig.com>
|
@StefanMarkmann thank you for the quick intervention. I now released v7.14.2. E2E testing would have caught this earlier... My bad |
Description
This PR fixes the
/oauth2/auth(AuthOnly) endpoint to return a 302 redirect directly whenskip-provider-button=trueand no valid session exists, instead of returning 401 Unauthorized.Changes:
AuthOnlyfunction inoauthproxy.goto return 302 redirect whenSkipProviderButtonis enabledskip-provider-buttondocumentation indocs/configuration/overview.mdTestAuthOnlyEndpointRedirectWithSkipProviderButtonwith assertions for OAuth redirect parametersMotivation and Context
Fixes #334
When using oauth2-proxy with nginx's
auth_requestdirective, the standard configuration uses:This pattern intercepts 401 responses and redirects users to the sign-in page. However, when
skip-provider-button=trueis enabled, the/oauth2/sign_inendpoint returns a 302 redirect with body<a href="...">Found</a>.The problem: nginx's
error_pagedirective converts this 302 into a 403 (due to=403), and browsers don't follow redirects from 403 error responses. Users see the "Found." link text instead of being automatically redirected to the OAuth provider.Solution: Make the
/oauth2/authendpoint return the 302 redirect directly whenskip-provider-button=true, bypassing theerror_pagemechanism entirely. Nginx passes 302 redirects through unchanged, allowing automatic redirect to the OAuth provider.How Has This Been Tested?
TestAuthOnlyEndpointRedirectWithSkipProviderButtonthat verifies:/oauth/authorizeendpointclient_id,redirect_uri,state)go test -v ./...)auth_requestconfiguration in production environmentChecklist: