Skip to content

[GH-259] Fix: Nonce-based CSRF vulnerability #275

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
+13 −2
Open

[GH-259] Fix: Nonce-based CSRF vulnerability #275

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15103f7
refactor: update google client nonce
Pathan-Amaankhan Aug 27, 2025
cad2337
refactor: initialize state data
Pathan-Amaankhan Aug 28, 2025
a4f9990
refactor: update transient key
Pathan-Amaankhan Sep 3, 2025
4ffb514
feat: add rtcamp.google_login_oauth_state_expiration filter
Pathan-Amaankhan Sep 3, 2025
58f9dc0
refactor: remove trailing commas
Pathan-Amaankhan Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Modules/Login.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,12 @@ public function authenticate( $user = null ) {
return $user;
}

if ( empty( $decoded_state['nonce'] ) || ! wp_verify_nonce( $decoded_state['nonce'], 'login_with_google' ) ) {
if ( empty( $decoded_state['nonce'] ) || ! get_transient( 'google_oauth_state_' . $decoded_state['nonce'] ) ) {
return $user;
}

delete_transient( 'google_oauth_state_' . $decoded_state['nonce'] ); // One-time use only.
Copy link
Member

@mi5t4n mi5t4n Sep 2, 2025

Choose a reason for hiding this comment

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

Could we prefix the transient just as we do with hooks?

Copy link
Member Author

@Pathan-Amaankhan Pathan-Amaankhan Sep 3, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Pathan-Amaankhan Pathan-Amaankhan Sep 3, 2025

Choose a reason for hiding this comment

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

Updated in a4f9990


try {
$this->gh_client->set_access_token( $code );
$user = $this->gh_client->user();
Expand Down
7 changes: 6 additions & 1 deletion src/Utils/GoogleClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,15 @@ public function user(): \stdClass {
* @return string
*/
public function state(): string {
$state_data['nonce'] = wp_create_nonce( 'login_with_google' );
$state_data = [];

$state_data['nonce'] = wp_generate_password( 32, false ); // Strong random token.
$state_data = apply_filters( 'rtcamp.google_login_state', $state_data );
$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 );
Copy link
Member

@mi5t4n mi5t4n Sep 2, 2025

Choose a reason for hiding this comment

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

Can we create a filter for the expiration, allowing developers to modify it as needed?

Copy link
Member Author

@Pathan-Amaankhan Pathan-Amaankhan Sep 3, 2025

Choose a reason for hiding this comment

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

Added filter in 4ffb514


return base64_encode( wp_json_encode( $state_data ) );
}
}