Skip to content

Conversation

@KyleKincer
Copy link

Summary

This PR adds support for optional state and nonce parameters in the OAuth2 authorization flow, enhancing security and enabling OpenID Connect compatibility.

Changes Made

  • ✅ Added optional state parameter support to OAuth2Provider constructor
  • ✅ Added optional nonce parameter support to OAuth2Provider constructor
  • ✅ Use custom state parameter if provided, otherwise generate UUID (maintains backward compatibility)
  • ✅ Include nonce parameter in authorization URL when provided
  • ✅ Both parameters are properly URL-encoded for security

Benefits

  • Enhanced CSRF Protection: Developers can pass custom state values for enhanced CSRF attack mitigation
  • OpenID Connect Support: Enables nonce parameter for ID token verification and replay attack prevention
  • Backward Compatible: Existing behavior is maintained when parameters are not provided
  • Security Best Practices: Proper URL encoding ensures safe parameter transmission

Usage Example

$provider:=New OAuth2 provider(New object(\
    "name"; "Microsoft";\
    "clientId"; "your-client-id";\
    "scope"; "openid profile email";\
    "state"; "your-custom-state-value";\
    "nonce"; "your-random-nonce-value"))

Security Note

⚠️ Important: The existing commented-out state verification code in _OpenBrowserForAuthorisation should be uncommented and updated to properly validate returned state parameters for complete CSRF protection.

Testing

  • Verified backward compatibility - existing code continues to work
  • Verified state parameter is included in authorization URL when provided
  • Verified nonce parameter is included in authorization URL when provided
  • Verified proper URL encoding of both parameters

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2025

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


Kyle Kincer seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

@KyleKincer KyleKincer changed the base branch from main to 20.x September 4, 2025 21:15
@KyleKincer KyleKincer force-pushed the feature/oauth-state-nonce-support branch from b1df1e0 to c871265 Compare September 4, 2025 21:18
…tion

- Add optional state parameter support to OAuth2Provider constructor
- Add optional nonce parameter support to OAuth2Provider constructor
- Use custom state parameter if provided, otherwise generate UUID (maintains backward compatibility)
- Include nonce parameter in authorization URL when provided
- Both parameters are properly URL-encoded for security

This enables developers to:
- Pass custom state values for enhanced CSRF protection
- Include nonce parameter for OpenID Connect ID token verification
- Maintain existing behavior when parameters are not provided

Note: The existing commented-out state verification code should be
uncommented and updated to properly validate returned state parameters.
@KyleKincer KyleKincer force-pushed the feature/oauth-state-nonce-support branch from c871265 to 35ab2a2 Compare September 4, 2025 21:24
@yannicktrinh
Copy link
Contributor

Hello,

Thank you for this code change proposal. Unfortunately, this PR was made from a fork of the 20.x branch (instead of the Main branch as indicated in our pull request guidelines).

Nevertheless, as the need seems real and useful (for OpenId authentication), we will work on it, into the Main branch (following the standard process: studies, tests, documentation, etc.).

If you have a need/problem in 20.x, I suggest you submit an incident report at https://taow.4d.com/ so that the incident can be tracked.

Thank you for your contribution.

@KyleKincer
Copy link
Author

Thanks @yannicktrinh , I'll look out for a PR in the main project. We will likely need this support in v20 until early next year, thus we will proceed with an internal fork until this can be resolved and potentially ported down to the 20.x branch. We will open an incident for that request. Thanks again!

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.

2 participants