Skip to content

Commit 6a4fe45

Browse files
f3ndotschemaxxx
authored andcommitted
Enforce/verify state parameter of callback
This fixes a security vulnerability where a malicious actor can bypass authentication via a clickjacking attack (CSRF vulnerability). Signed-off-by: schema <[email protected]>
1 parent b1431d4 commit 6a4fe45

File tree

2 files changed

+14
-4
lines changed

2 files changed

+14
-4
lines changed

SpecialOAuth2Client.php

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,17 +92,27 @@ private function _redirect() {
9292
}
9393

9494
private function _handleCallback(){
95+
global $wgRequest;
96+
9597
try {
98+
$storedState = $wgRequest->getSession()->get('oauth2state');
99+
// Enforce the `state` parameter to prevent clickjacking/CSRF
100+
if(isset($storedState) && $storedState != $_GET['state']) {
101+
if(isset($_GET['state'])) {
102+
throw new UnexpectedValueException("State parameter of callback does not match original state");
103+
} else {
104+
throw new UnexpectedValueException("Required state parameter missing");
105+
}
106+
}
96107

97108
// Try to get an access token using the authorization code grant.
98109
$accessToken = $this->_provider->getAccessToken('authorization_code', [
99110
'code' => $_GET['code']
100111
]);
101112
} catch (\League\OAuth2\Client\Provider\Exception\IdentityProviderException $e) {
102-
103-
// Failed to get the access token or user details.
113+
exit($e->getMessage()); // Failed to get the access token or user details.
114+
} catch (UnexpectedValueException $e) {
104115
exit($e->getMessage());
105-
106116
}
107117

108118
$resourceOwner = $this->_provider->getResourceOwner($accessToken);

extension.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "MW-OAuth2Client",
3-
"version": "0.3",
3+
"version": "0.4",
44
"author": [
55
"[http://dekeijzer.org Joost de Keijzer]",
66
"[https://www.mediawiki.org/wiki/User:Nischayn22 Nischay Nahata]",

0 commit comments

Comments
 (0)