[GH-259] Fix: Nonce-based CSRF vulnerability#275
Conversation
rtBot
left a comment
rtBot
left a comment
There was a problem hiding this comment.
Code analysis identified issues
action-phpcs-code-review has identified potential problems in this pull request during automated scanning. We recommend reviewing the issues noted and that they are resolved.
phpcs scanning turned up:
Powered by rtCamp's GitHub Actions Library
Pathan-Amaankhan
changed the title
mi5t4n
left a comment
mi5t4n
left a comment
There was a problem hiding this comment.
Left some comments, otherwise all looks good to me.
| return $user; | ||
| } | ||
|
|
||
| delete_transient( 'google_oauth_state_' . $decoded_state['nonce'] ); // One-time use only. |
There was a problem hiding this comment.
Could we prefix the transient just as we do with hooks?
There was a problem hiding this comment.
Hi @mi5t4n,
I went through the codebase and found that we are adding rtcamp. as a prefix, but if memcache/redis are not available, transients are stored in the DB. So, I will prefer not to add a special character, i.e. . in our transient key. Instead will be adding rtcamp_ as a prefix.
Please do let me know if there are any concerns about the above approach.
| $state_data['provider'] = 'google'; | ||
|
|
||
| // Store it in a transient keyed by the visitor. | ||
| set_transient( 'google_oauth_state_' . $state_data['nonce'], 1, 15 * MINUTE_IN_SECONDS ); |
There was a problem hiding this comment.
Can we create a filter for the expiration, allowing developers to modify it as needed?
rtBot
left a comment
rtBot
left a comment
There was a problem hiding this comment.
Code analysis identified issues
action-phpcs-code-review has identified potential problems in this pull request during automated scanning. We recommend reviewing the issues noted and that they are resolved.
phpcs scanning turned up:
🚫 1 error
Powered by rtCamp's GitHub Actions Library
mi5t4n
left a comment
mi5t4n
left a comment
There was a problem hiding this comment.
Left just 1 comment, rest looks good.
| set_transient( | ||
| 'rtcamp_google_oauth_state_' . $state_data['nonce'], | ||
| 1, | ||
| apply_filters( 'rtcamp.google_login_oauth_state_expiration', 15 * MINUTE_IN_SECONDS ) |
There was a problem hiding this comment.
Let's document this filter similar to this one.
3 participants
Description
Fixes/Covers