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

[SDK-1418] Advanced option to clear transaction cookies on redirect #436

Closed
wants to merge 12 commits into from

Conversation

stevehobbsdev
Copy link
Contributor

Description

This PR adds an advanced option to clear transaction cookies on authorization redirect. The default value is false.

Usage:

await createAuth0Client({
  advancedOptions: {
    clearTransactionCookies: true
  }
})

References

Fixes #319

Testing

  • This change adds integration tests
  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@stevehobbsdev stevehobbsdev added feature request A feature has been asked for or suggested by the community small CH: Added PR is adding feature or functionality labels Apr 24, 2020
@stevehobbsdev stevehobbsdev added this to the vNext milestone Apr 24, 2020
@stevehobbsdev
Copy link
Contributor Author

This PR is currently based on feature/advanced-default-scopes to get access to the AdvancedOptions type. Once #435 is merged, I will rebase.

@stevehobbsdev stevehobbsdev removed this from the vNext milestone Apr 29, 2020
@adamjmcgrath
Copy link
Contributor

I have a couple of concerns

  1. I don't think this will fix anyone's underlying issue. The only 2 examples of this I've seen are:

#387 Redirecting users from within rules which messed with the state param and caused a login loop (and the transaction can't be cleared if you have the wrong state param)

#319 This is a login loop as well (from the screenshot I estimate a new transaction every 300ms) and they have a custom property in their state "returnPath": "/scans?dateTimeRange=preset-lastDay&freeText=liron&origin=VOID&verdict=VOID" so perhaps they're trying to do something with redirecting in rules too?

For both these customers, if they had added the clearTransactionCookies option, the login loops would go on indefinitely rather than be stopped by the 494 - which I suppose might be preferable, as they don't have to be told to clear their cookies?

  1. Enabling the option will cause concurrency issues with multiple tabs logging in at the same time - if you call loginWithRedirect from 2 tabs at the same time, the second one will invalidate the first one. This would definitely become an issue if you had a watchInactivity type process like in Cookie Pollution - still happening to us with latest version #319 (comment) that automatically called loginWithRedirect

That said, I don't have a better solution than finding and fixing the customers underlying issue and telling them to tell their users to clear their cookies - which I know is not very satisfactory...

@stevehobbsdev
Copy link
Contributor Author

Closing this in favor of a different approach.

@stevehobbsdev stevehobbsdev deleted the feature/clear-transaction branch May 21, 2020 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Added PR is adding feature or functionality feature request A feature has been asked for or suggested by the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookie Pollution - still happening to us with latest version
3 participants