Skip to content

core/oauth2: don't set state in responses if not supplied#9735

Open
alxndrsn wants to merge 1 commit intogoauthentik:mainfrom
alxndrsn:minimal
Open

core/oauth2: don't set state in responses if not supplied#9735
alxndrsn wants to merge 1 commit intogoauthentik:mainfrom
alxndrsn:minimal

Conversation

@alxndrsn
Copy link
Copy Markdown

@alxndrsn alxndrsn commented May 15, 2024

I think this fixes a bug in OAuth2 / OpenID Connect (OIDC) implementation, as reported at getodk/central-backend#1135 (comment)

I guess this PR would ideally include tests which would fail without the code changes.

@alxndrsn alxndrsn requested a review from a team as a code owner May 15, 2024 08:47
@netlify
Copy link
Copy Markdown

netlify Bot commented May 15, 2024

Deploy Preview for authentik-docs ready!

Name Link
🔨 Latest commit be46401
🔍 Latest deploy log https://app.netlify.com/sites/authentik-docs/deploys/664476a1c032170008ffa32d
😎 Deploy Preview https://deploy-preview-9735--authentik-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link
Copy Markdown

netlify Bot commented May 15, 2024

Deploy Preview for authentik-storybook ready!

Name Link
🔨 Latest commit be46401
🔍 Latest deploy log https://app.netlify.com/sites/authentik-storybook/deploys/664476a15427750008d241f1
😎 Deploy Preview https://deploy-preview-9735--authentik-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@BeryJu
Copy link
Copy Markdown
Member

BeryJu commented May 15, 2024

Thanks for the PR! I'll have to do some reading but iirc as part of OIDC the state parameter is required? In that case we'd have to enforce it being set when the openid scope is set

@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.31%. Comparing base (49cf10e) to head (be46401).
⚠️ Report is 7134 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9735      +/-   ##
==========================================
- Coverage   92.46%   92.31%   -0.15%     
==========================================
  Files         669      704      +35     
  Lines       32751    34404    +1653     
==========================================
+ Hits        30282    31760    +1478     
- Misses       2469     2644     +175     
Flag Coverage Δ
e2e 49.19% <60.00%> (-1.44%) ⬇️
integration 25.36% <0.00%> (-0.63%) ⬇️
unit 89.82% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alxndrsn
Copy link
Copy Markdown
Author

iirc as part of OIDC the state parameter is required?

I may be reading the wrong place, but https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest reads:

state
RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie.

@lognaturel
Copy link
Copy Markdown

Hi! Any chance this would be a straightforward review for you @BeryJu or someone else given the spec clarification above? Thanks!

alxndrsn added a commit to getodk/central-backend that referenced this pull request Dec 5, 2024
* pass `next` value to IdP as `state`, with additional random prefix
* this makes the "next" value more trusted
* fix support for Authentik and other IdPs which do not support auth URLs without `state` (goauthentik/authentik#9735)
* may also improve support for IdPs which lack PKCE support (see: https://github.com/panva/openid-client/blob/1486c3a020af8d12449d1d6a4bdf4f2bf4d32b77/README.md#authorization-code-flow)

Closes #1134
Closes #1135
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.

3 participants