-
Notifications
You must be signed in to change notification settings - Fork 510
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
Use of cache for XSS checks #514
Comments
You could pass a custom CacheService that is backed by other thing than the Play cache. |
Aren't there other aspects of secure social which are using the cache as an optimisation? This would mean the CacheService needs to look at the key and determine if it's something that can safely be cached or not. Doesn't sound like an elegant solution to me. Note that this isn't a blocker for me. |
The CacheService is used by the OAuth providers for XSS checks and by the default implementation of the AuthenticatorStore. The Play cache is used because it's easily available, not because it's an optimization. The name of the trait could be changed I guess to something like StateService maybe to better reflect the intended use. Not sure I follow about the need to look at each key to determine if the value can be safely cached. Isn't that something you'd know before trying to put things in the cache? As of the 5 minute timeout I think we could add a property so you can customize that. |
I think that the use of the play cache or
CacheService
for XSS checks is incorrect. A cache should be an optimisation. Invalidating a cache should only affect performance, not functionality. This is not true for the XSS checks performed. If the cache is invalidated, the user will fail to login.Also, I feel that the 5 minute timeout for login is too short. If the user forgets their password, it may take them longer than 5 minutes to complete the password reset path with the oauth provider. The website I'm responsible for has many older users, and failed logins due to the short timeout affect us.
Happy to consider other opinions on this.
The text was updated successfully, but these errors were encountered: