Skip to content

Commit b32353d

Browse files
Copilotovertrue
andauthored
Fix OAuth token validation vulnerabilities and add comprehensive test coverage (#292)
* Initial plan * Fix OAuth token validation vulnerabilities in multiple providers Co-authored-by: overtrue <[email protected]> * Fix additional WeWork vulnerabilities and add comprehensive tests Co-authored-by: overtrue <[email protected]> * Add comprehensive test coverage for all OAuth providers - Expanded QQTest with redirect URL testing, token handling, user mapping, and error scenarios - Enhanced DingTalkTest with configuration testing, signature validation, and complete flow coverage - Improved DouYinTest with URL validation, token processing, user data handling, and edge cases - Extended WeWorkTest with OAuth URL generation, QR code mode, detailed user flow, and configuration testing - Broadened OpenWeWorkTest with redirect handling, user type configuration, and comprehensive user mapping - Augmented AlipayTest with sandbox mode, signature generation, field validation, and complete token flow All tests now cover normal functionality, configuration scenarios, method behaviors, error cases, and edge cases as requested in code review feedback. Co-authored-by: overtrue <[email protected]> * Fix DingTalk open_id validation and test namespace issues - Add specific validation for open_id field in DingTalk provider user_info response - Fix test namespaces to use 'Providers' instead of 'Tests\Providers' for consistency - Correct test data structures to use proper field names (open_id vs openid) Co-authored-by: overtrue <[email protected]> * Fix test namespaces, assertions and HTTP client mocking in QQ and DingTalk tests Co-authored-by: overtrue <[email protected]> * Fix additional test files - DouYin tests now passing, major reduction in test failures Co-authored-by: overtrue <[email protected]> * Fix all remaining test failures - HTTP client mocking and variable naming issues resolved Co-authored-by: overtrue <[email protected]> * Add composer fix command using Laravel Pint for code formatting Co-authored-by: overtrue <[email protected]> * Fix PHPStan error: remove redundant null coalescing operator in QQ provider Co-authored-by: overtrue <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: overtrue <[email protected]>
1 parent efb0619 commit b32353d

19 files changed

+1727
-30
lines changed

composer.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
"scripts": {
4545
"check-style": "@php vendor/bin/pint --test",
4646
"fix-style": "@php vendor/bin/pint",
47+
"fix": "@php vendor/bin/pint",
4748
"test": "@php vendor/bin/phpunit --colors=always",
4849
"phpstan": "@php vendor/bin/phpstan analyse src"
4950
}

src/Providers/Alipay.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,10 @@ public function tokenFromCode(string $code): array
110110
throw new Exceptions\BadRequestException((string) $responseInstance->getBody());
111111
}
112112

113+
if (empty($response['alipay_system_oauth_token_response'])) {
114+
throw new Exceptions\AuthorizeFailedException('Authorization failed: missing alipay_system_oauth_token_response in response', $response);
115+
}
116+
113117
return $this->normalizeAccessTokenResponse($response['alipay_system_oauth_token_response']);
114118
}
115119

src/Providers/DingTalk.php

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,9 +114,17 @@ public function userFromCode(string $code): Contracts\UserInterface
114114
throw new Exceptions\BadRequestException((string) $responseInstance->getBody());
115115
}
116116

117+
if (empty($response['user_info'])) {
118+
throw new Exceptions\AuthorizeFailedException('Authorization failed: missing user_info in response', $response);
119+
}
120+
121+
if (empty($response['user_info'][Contracts\ABNF_OPEN_ID])) {
122+
throw new Exceptions\AuthorizeFailedException('Authorization failed: missing open_id in user_info response', $response);
123+
}
124+
117125
return new User([
118-
Contracts\ABNF_NAME => $response['user_info']['nick'],
119-
Contracts\ABNF_NICKNAME => $response['user_info']['nick'],
126+
Contracts\ABNF_NAME => $response['user_info']['nick'] ?? null,
127+
Contracts\ABNF_NICKNAME => $response['user_info']['nick'] ?? null,
120128
Contracts\ABNF_ID => $response['user_info'][Contracts\ABNF_OPEN_ID],
121129
]);
122130
}

src/Providers/DouYin.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,10 @@ public function tokenFromCode(string $code): array
6666
throw new Exceptions\AuthorizeFailedException('Invalid token response', $body);
6767
}
6868

69+
if (empty($body['data'][Contracts\ABNF_OPEN_ID] ?? null)) {
70+
throw new Exceptions\AuthorizeFailedException('Authorization failed: missing open_id in token response', $body);
71+
}
72+
6973
$this->withOpenId($body['data'][Contracts\ABNF_OPEN_ID]);
7074

7175
return $this->normalizeAccessTokenResponse($body['data']);

src/Providers/Douban.php

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,6 @@ protected function getTokenUrl(): string
2424
return 'https://www.douban.com/service/auth2/token';
2525
}
2626

27-
/**
28-
* @param ?array $query
29-
*/
3027
protected function getUserByToken(string $token, ?array $query = []): array
3128
{
3229
$response = $this->getHttpClient()->get('https://api.douban.com/v2/user/~me', [

src/Providers/OpenWeWork.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,9 @@ public function userFromCode(string $code): Contracts\UserInterface
8585
$user = $this->getUser($this->getSuiteAccessToken(), $code);
8686

8787
if ($this->detailed) {
88+
if (empty($user['user_ticket'])) {
89+
throw new Exceptions\AuthorizeFailedException('Authorization failed: missing user_ticket in response', $user);
90+
}
8891
$user = \array_merge($user, $this->getUserByTicket($user['user_ticket']));
8992
}
9093

src/Providers/PayPal.php

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ class PayPal extends Base
4242
public function __construct(array $config)
4343
{
4444
parent::__construct($config);
45-
$this->sandbox = (bool)$this->config->get('sandbox', false);
45+
$this->sandbox = (bool) $this->config->get('sandbox', false);
4646
if ($this->sandbox) {
4747
$this->authUrl = 'https://www.sandbox.paypal.com/signin/authorize';
4848
$this->tokenURL = 'https://api-m.sandbox.paypal.com/v1/oauth2/token';
@@ -97,7 +97,6 @@ protected function getTokenUrl(): string
9797
* @throws \Overtrue\Socialite\Exceptions\AuthorizeFailedException
9898
*
9999
* @see https://developer.paypal.com/docs/log-in-with-paypal/integrate/#link-getaccesstoken
100-
*
101100
*/
102101
public function tokenFromCode(string $code): array
103102
{
@@ -110,11 +109,12 @@ public function tokenFromCode(string $code): array
110109
],
111110
'headers' => [
112111
'Accept' => 'application/json',
113-
'Authorization' => 'Basic ' . \base64_encode(\sprintf('%s:%s', $this->getClientId(), $this->getClientSecret())),
112+
'Authorization' => 'Basic '.\base64_encode(\sprintf('%s:%s', $this->getClientId(), $this->getClientSecret())),
114113
],
115114
]
116115
);
117-
return $this->normalizeAccessTokenResponse((string)$response->getBody());
116+
117+
return $this->normalizeAccessTokenResponse((string) $response->getBody());
118118
}
119119

120120
/**
@@ -129,10 +129,11 @@ protected function getUserByToken(string $token): array
129129
[
130130
'headers' => [
131131
'Content-Type' => 'application/x-www-form-urlencoded',
132-
'Authorization' => 'Bearer ' . $token,
132+
'Authorization' => 'Bearer '.$token,
133133
],
134134
]
135135
);
136+
136137
return $this->fromJsonBody($response);
137138
}
138139

src/Providers/QQ.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,10 @@ protected function getUserByToken(string $token): array
7777

7878
$me = $this->fromJsonBody($response);
7979

80+
if (empty($me['openid'])) {
81+
throw new AuthorizeFailedException('Authorization failed: missing openid in token response', $me);
82+
}
83+
8084
$response = $this->getHttpClient()->get($this->baseUrl.'/user/get_user_info', [
8185
'query' => [
8286
Contracts\RFC6749_ABNF_ACCESS_TOKEN => $token,
@@ -94,7 +98,7 @@ protected function getUserByToken(string $token): array
9498

9599
return $user + [
96100
'unionid' => $me['unionid'] ?? null,
97-
'openid' => $me['openid'] ?? null,
101+
'openid' => $me['openid'],
98102
];
99103
}
100104

src/Providers/WeWork.php

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ public function userFromCode(string $code): Contracts\UserInterface
4848
$user = $this->getUser($token, $code);
4949

5050
if ($this->detailed) {
51+
if (empty($user['UserId'])) {
52+
throw new Exceptions\AuthorizeFailedException('Authorization failed: missing UserId in user response', $user);
53+
}
5154
$user = $this->getUserById($user['UserId']);
5255
}
5356

@@ -203,6 +206,10 @@ protected function requestApiAccessToken(): string
203206
throw new Exceptions\AuthorizeFailedException((string) $responseInstance->getBody(), $response);
204207
}
205208

209+
if (empty($response[Contracts\RFC6749_ABNF_ACCESS_TOKEN])) {
210+
throw new Exceptions\AuthorizeFailedException('Authorization failed: missing access_token in response', $response);
211+
}
212+
206213
return $response[Contracts\RFC6749_ABNF_ACCESS_TOKEN];
207214
}
208215

tests/OAuthTest.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
class OAuthTest extends TestCase
99
{
10-
public function tearDown(): void
10+
protected function tearDown(): void
1111
{
1212
m::close();
1313
}
@@ -130,7 +130,7 @@ public function test_it_can_get_user_by_code()
130130

131131
$response = m::mock(\Psr\Http\Message\ResponseInterface::class);
132132
$stream = m::mock(\Psr\Http\Message\StreamInterface::class);
133-
133+
134134
$stream->shouldReceive('__toString')->andReturn(\json_encode([
135135
'access_token' => 'fake_access_token',
136136
'refresh_token' => 'fake_refresh_token',

0 commit comments

Comments
 (0)