Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch samesite for implicit #758

Merged
merged 6 commits into from
Jan 17, 2020
Merged

Conversation

joshcanhelp
Copy link
Contributor

@joshcanhelp joshcanhelp commented Jan 13, 2020

Changes

  • Add SameSite=None and Secure attribute to nonce and state cookies when implicit mode is turned on
  • Add notice for implicit mode deprecation and HTTPS check if this setting is turned on

Important note for sites using the implicit flow: The upcoming changes to SameSite handling in multiple browsers will require sites using the Implicit Login Flow setting to also be served on a secure channel (callback URL using "https"). This setting will be removed in the upcoming major version (please comment below if you have an important use case that this will affect).

References

Web.dev: SameSite cookies explained

Testing

  • This change adds unit test coverage
  • This change has been tested on PHP 7.2 and WP 5.3.2

Checklist

  • All existing and new tests complete without errors
  • All code quality tools/guidelines in the Contribution guide have been run/followed
  • All active GitHub CI checks have passed

@joshcanhelp joshcanhelp added this to the 3.11.2 milestone Jan 13, 2020
@@ -166,13 +179,94 @@ public function generate_unique( $bytes = 32 ) {
* @return bool
*/
protected function handle_cookie( $cookie_name, $cookie_value, $cookie_exp ) {
$illegal_chars = ",; \t\r\n\013\014";
if ( strpbrk( $cookie_name, $illegal_chars ) != null ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check just clues the administrator into an issue via the plugin error log if, for some reason, they're doing something wrong with customer state/nonce values. With one of these characters in place, the validation would be silently failing so this is not a breaking change.

@@ -39,6 +39,14 @@ class WP_Auth0_Nonce_Handler {
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pattern for the additions to this file was pulled from the PHP SDK:

https://github.com/auth0/auth0-PHP/pull/395/files#diff-edb92fb2369f39801bab7466d95cbc1c

@joshcanhelp joshcanhelp marked this pull request as ready for review January 14, 2020 04:59
@jimmyjames jimmyjames self-requested a review January 17, 2020 20:29
@joshcanhelp joshcanhelp merged commit 1a679fa into 3.11.2-dev Jan 17, 2020
@joshcanhelp joshcanhelp deleted the patch-samesite-for-implicit branch January 17, 2020 21:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants