Skip to content

Commit 603316c

Browse files
PothercaPotherca
authored
Merge pull request #202 from pdsinterop/fix/CLN-006-open-redirect
Fix CLN-006 open redirect
2 parents c1d8675 + 71e1fdc commit 603316c

File tree

5 files changed

+822
-12
lines changed

5 files changed

+822
-12
lines changed

‎solid/lib/BaseServerConfig.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ public function setUserSubDomainsEnabled($enabled) {
202202

203203
////////////////////////////// UTILITY METHODS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\
204204

205-
private function castToBool(string $mixedValue): bool
205+
private function castToBool(?string $mixedValue): bool
206206
{
207207
$type = gettype($mixedValue);
208208

‎solid/lib/Controller/ServerController.php‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ class ServerController extends Controller
2323
{
2424
use DpopFactoryTrait;
2525

26+
public const ERROR_UNREGISTERED_URI = 'Provided redirect URI "%s" does not match any registered URIs';
2627
private $userId;
2728

2829
/* @var IUserManager */
@@ -220,10 +221,28 @@ public function authorize() {
220221
return $result; // ->addHeader('Access-Control-Allow-Origin', '*');
221222
}
222223

223-
$parsedOrigin = parse_url($clientRegistration['redirect_uris'][0]);
224+
if (isset($getVars['redirect_uri'])) {
225+
$redirectUri = $getVars['redirect_uri'];
226+
if (! isset($clientRegistration['redirect_uris']) || ! is_array($clientRegistration['redirect_uris'])) {
227+
return new JSONResponse('Invalid client registration, no redirect URIs found', Http::STATUS_BAD_REQUEST);
228+
}
229+
230+
$redirectUris = $clientRegistration['redirect_uris'];
231+
232+
$validRedirectUris = array_filter($redirectUris, function ($uri) use ($redirectUri) {
233+
return $uri === $redirectUri;
234+
});
235+
236+
if (count($validRedirectUris) === 0) {
237+
$message = vsprintf(self::ERROR_UNREGISTERED_URI, [$redirectUri]);
238+
return new JSONResponse($message, Http::STATUS_BAD_REQUEST);
239+
}
240+
}
241+
242+
$parsedOrigin = parse_url($redirectUri);
224243
if (
225-
$parsedOrigin['scheme'] != "https" &&
226-
$parsedOrigin['scheme'] != "http" &&
244+
$parsedOrigin['scheme'] !== "https" &&
245+
$parsedOrigin['scheme'] !== "http" &&
227246
!isset($_GET['customscheme'])
228247
) {
229248
$result = new JSONResponse('Custom schema');

‎solid/tests/Unit/BaseServerConfigTest.php‎

Lines changed: 278 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,31 @@
11
<?php
22

3-
namespace Unit;
3+
namespace OCA\Solid;
44

55
use OCA\Solid\AppInfo\Application;
6-
use OCA\Solid\BaseServerConfig;
76
use OCP\IConfig;
87
use PHPUnit\Framework\TestCase;
98
use TypeError;
109

10+
function random_bytes()
11+
{
12+
return BaseServerConfigTest::MOCK_RANDOM_BYTES;
13+
}
14+
1115
/**
1216
* @coversDefaultClass \OCA\Solid\BaseServerConfig
17+
* @uses \OCA\Solid\BaseServerConfig
1318
* @covers ::__construct
1419
*/
1520
class BaseServerConfigTest extends TestCase
1621
{
22+
////////////////////////////////// FIXTURES \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
23+
24+
public const MOCK_RANDOM_BYTES = 'mock random bytes';
25+
private const MOCK_REDIRECT_URI = 'mock redirect uri';
26+
private const MOCK_CLIENT_ID = 'mock-client-id';
27+
private const MOCK_ORIGIN = 'mock origin';
28+
1729
/////////////////////////////////// TESTS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
1830

1931
/**
@@ -44,12 +56,13 @@ public function testConstructorWithValidConfig()
4456
/**
4557
* @testdox BaseServerConfig should return a boolean when asked whether UserSubDomains are Enabled
4658
* @covers ::getUserSubDomainsEnabled
59+
* @covers ::castToBool
4760
* @dataProvider provideBooleans
4861
*/
49-
public function testGetUserSubDomainsEnabled($expected)
62+
public function testGetUserSubDomainsEnabled($value, $expected)
5063
{
5164
$configMock = $this->createMock(IConfig::class);
52-
$configMock->method('getAppValue')->willReturn($expected);
65+
$configMock->method('getAppValue')->willReturn($value);
5366

5467
$baseServerConfig = new BaseServerConfig($configMock);
5568
$actual = $baseServerConfig->getUserSubDomainsEnabled();
@@ -78,10 +91,11 @@ public function testGetUserSubDomainsEnabledFromAppConfig()
7891
/**
7992
* @testdox BaseServerConfig should set value in AppConfig when asked to set UserSubDomainsEnabled
8093
* @covers ::setUserSubDomainsEnabled
94+
* @covers ::castToBool
8195
*
8296
* @dataProvider provideBooleans
8397
*/
84-
public function testSetUserSubDomainsEnabled($expected)
98+
public function testSetUserSubDomainsEnabled($value, $expected)
8599
{
86100
$configMock = $this->createMock(IConfig::class);
87101
$configMock->expects($this->atLeast(1))
@@ -90,16 +104,272 @@ public function testSetUserSubDomainsEnabled($expected)
90104
;
91105

92106
$baseServerConfig = new BaseServerConfig($configMock);
93-
$baseServerConfig->setUserSubDomainsEnabled($expected);
107+
$baseServerConfig->setUserSubDomainsEnabled($value);
108+
}
109+
110+
/**
111+
* @testdox BaseServerConfig should retrieve client ID AppValue when asked to GetClientRegistration for existing client
112+
* @covers ::getClientRegistration
113+
*/
114+
public function testGetClientRegistrationForExistingClient()
115+
{
116+
$configMock = $this->createMock(IConfig::class);
117+
$baseServerConfig = new BaseServerConfig($configMock);
118+
119+
$expected = ['mock' => 'client'];
120+
121+
$configMock->expects($this->once())
122+
->method('getAppValue')
123+
->with(Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID)
124+
->willReturn(json_encode($expected));
125+
126+
$actual = $baseServerConfig->getClientRegistration(self::MOCK_CLIENT_ID);
127+
128+
$this->assertEquals($expected, $actual);
129+
}
130+
131+
/**
132+
* @testdox BaseServerConfig should return empty array when asked to GetClientRegistration for non-existing client
133+
* @covers ::getClientRegistration
134+
*/
135+
public function testGetClientRegistrationForNonExistingClient()
136+
{
137+
$configMock = $this->createMock(IConfig::class);
138+
$baseServerConfig = new BaseServerConfig($configMock);
139+
140+
$expected = [];
141+
142+
$configMock->expects($this->once())
143+
->method('getAppValue')
144+
->with(Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID)
145+
->willReturnArgument(2);
146+
147+
$actual = $baseServerConfig->getClientRegistration(self::MOCK_CLIENT_ID);
148+
149+
$this->assertEquals($expected, $actual);
150+
}
151+
152+
/**
153+
* @testdox BaseServerConfig should complain when asked to save ClientRegistration without origin
154+
* @covers ::saveClientRegistration
155+
*/
156+
public function testSaveClientRegistrationWithoutOrigin()
157+
{
158+
$this->expectException(TypeError::class);
159+
$this->expectExceptionMessage('Too few arguments to function');
160+
161+
$configMock = $this->createMock(IConfig::class);
162+
$baseServerConfig = new BaseServerConfig($configMock);
163+
164+
$baseServerConfig->saveClientRegistration();
165+
}
166+
167+
/**
168+
* @testdox BaseServerConfig should complain when asked to save ClientRegistration without client data
169+
* @covers ::saveClientRegistration
170+
*/
171+
public function testSaveClientRegistrationWithoutClientData()
172+
{
173+
$this->expectException(TypeError::class);
174+
$this->expectExceptionMessage('Too few arguments to function');
175+
176+
$configMock = $this->createMock(IConfig::class);
177+
$baseServerConfig = new BaseServerConfig($configMock);
178+
179+
$baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN);
180+
}
181+
182+
/**
183+
* @testdox BaseServerConfig should save ClientRegistration when asked to save ClientRegistration for new client
184+
* @covers ::saveClientRegistration
185+
*/
186+
public function testSaveClientRegistrationForNewClient()
187+
{
188+
$configMock = $this->createMock(IConfig::class);
189+
190+
$configMock->expects($this->once())
191+
->method('getAppValue')
192+
->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN))
193+
->willReturnArgument(2);
194+
195+
$expected = [
196+
'client_id' => md5(self::MOCK_ORIGIN),
197+
'client_name' => self::MOCK_ORIGIN,
198+
'client_secret' => md5(self::MOCK_RANDOM_BYTES),
199+
];
200+
201+
$configMock->expects($this->exactly(2))
202+
->method('setAppValue')
203+
->willReturnMap([
204+
// Using willReturnMap as withConsecutive is removed since PHPUnit 10
205+
[Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)],
206+
[Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)]
207+
]);
208+
209+
$baseServerConfig = new BaseServerConfig($configMock);
210+
211+
$actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, []);
212+
213+
$this->assertEquals($expected, $actual);
214+
}
215+
216+
/**
217+
* @testdox BaseServerConfig should save ClientRegistration when asked to save ClientRegistration for existing client
218+
* @covers ::saveClientRegistration
219+
*/
220+
public function testSaveClientRegistrationForExistingClient()
221+
{
222+
$configMock = $this->createMock(IConfig::class);
223+
224+
$expected = [
225+
'client_id' => md5(self::MOCK_ORIGIN),
226+
'client_name' => self::MOCK_ORIGIN,
227+
'client_secret' => md5(self::MOCK_RANDOM_BYTES),
228+
'redirect_uris' => [self::MOCK_REDIRECT_URI],
229+
];
230+
231+
$configMock->expects($this->once())
232+
->method('getAppValue')
233+
->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN))
234+
->willReturn(json_encode($expected));
235+
236+
$configMock->expects($this->exactly(2))
237+
->method('setAppValue')
238+
->willReturnMap([
239+
// Using willReturnMap as withConsecutive is deprecated since PHPUnit 10
240+
[Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)],
241+
[Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)]
242+
]);
243+
244+
$baseServerConfig = new BaseServerConfig($configMock);
245+
246+
$actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, []);
247+
248+
$this->assertEquals($expected, $actual);
249+
}
250+
251+
/**
252+
* @testdox BaseServerConfig should save ClientRegistration when asked to save ClientRegistration for blocked client
253+
* @covers ::saveClientRegistration
254+
*/
255+
public function testSaveClientRegistrationForBlockedClient()
256+
{
257+
$configMock = $this->createMock(IConfig::class);
258+
259+
$expected = [
260+
'client_id' => md5(self::MOCK_ORIGIN),
261+
'client_name' => self::MOCK_ORIGIN,
262+
'client_secret' => md5(self::MOCK_RANDOM_BYTES),
263+
'redirect_uris' => [self::MOCK_REDIRECT_URI],
264+
'blocked' => true,
265+
];
266+
267+
$configMock->expects($this->once())
268+
->method('getAppValue')
269+
->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN))
270+
->willReturn(json_encode($expected));
271+
272+
$configMock->expects($this->exactly(2))
273+
->method('setAppValue')
274+
->willReturnMap([
275+
// Using willReturnMap as withConsecutive is deprecated since PHPUnit 10
276+
[Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)],
277+
[Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)]
278+
]);
279+
280+
$baseServerConfig = new BaseServerConfig($configMock);
281+
282+
$actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, $expected);
283+
284+
$this->assertEquals($expected, $actual);
285+
}
286+
287+
/**
288+
* @testdox BaseServerConfig should always "blocked" to existing value when asked to save ClientRegistration for blocked client
289+
* @covers ::saveClientRegistration
290+
*/
291+
public function testSaveClientRegistrationSetsBlocked()
292+
{
293+
$configMock = $this->createMock(IConfig::class);
294+
295+
$expected = [
296+
'client_id' => md5(self::MOCK_ORIGIN),
297+
'client_name' => self::MOCK_ORIGIN,
298+
'client_secret' => md5(self::MOCK_RANDOM_BYTES),
299+
'redirect_uris' => [self::MOCK_REDIRECT_URI],
300+
'blocked' => true,
301+
];
302+
303+
$configMock->expects($this->once())
304+
->method('getAppValue')
305+
->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN))
306+
->willReturn(json_encode($expected));
307+
308+
$clientData = $expected;
309+
$clientData['blocked'] = false;
310+
311+
$configMock->expects($this->exactly(2))
312+
->method('setAppValue')
313+
->willReturnMap([
314+
// Using willReturnMap as withConsecutive is deprecated since PHPUnit 10
315+
[Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)],
316+
[Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)]
317+
]);
318+
319+
$baseServerConfig = new BaseServerConfig($configMock);
320+
321+
$actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, $clientData);
322+
323+
$this->assertEquals($expected, $actual);
324+
}
325+
326+
/**
327+
* @testdox BaseServerConfig should remove ClientRegistration when asked to remove ClientRegistration
328+
* @covers ::removeClientRegistration
329+
*/
330+
public function testRemoveClientRegistration()
331+
{
332+
$configMock = $this->createMock(IConfig::class);
333+
$baseServerConfig = new BaseServerConfig($configMock);
334+
335+
$configMock->expects($this->once())
336+
->method('deleteAppValue')
337+
->with(Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID);
338+
339+
$baseServerConfig->removeClientRegistration(self::MOCK_CLIENT_ID);
94340
}
95341

96342
/////////////////////////////// DATAPROVIDERS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
97343

98344
public function provideBooleans()
99345
{
100346
return [
101-
'false' => [false],
102-
'true' => [true],
347+
// Only 'boolean', 'NULL', 'integer', 'string' are allowed
348+
// @TODO: Add test for type that trigger a TypeError:
349+
// - array
350+
// - callable
351+
// - float
352+
// - object
353+
// - resource
354+
// @TODO: Add test for values that trigger a TypeError
355+
// 'integer:-1' => ['value'=> -1],
356+
// 'integer:2' => ['value'=> 2],
357+
// 'string:-1' => ['value'=> '-1'],
358+
// 'string:2' => ['value'=> '2'],
359+
// 'string:foo' => ['value'=> 'foo'],
360+
// 'string:NULL' => ['value'=> 'NULL'],
361+
'boolean:false' => ['value'=> false, 'expected' => false],
362+
'boolean:true' => ['value'=> true, 'expected' => true],
363+
'integer:0' => ['value'=> 0, 'expected' => false],
364+
'integer:1' => ['value'=> 1, 'expected' => true],
365+
'NULL' => ['value'=> null, 'expected' => false],
366+
'string:0' => ['value'=> '0', 'expected' => false],
367+
'string:1' => ['value'=> '1', 'expected' => true],
368+
'string:empty' => ['value'=> '', 'expected' => false],
369+
'string:false' => ['value'=> 'false', 'expected' => false],
370+
'string:FALSE' => ['value'=> 'FALSE', 'expected' => false],
371+
'string:true' => ['value'=> 'true', 'expected' => true],
372+
'string:TRUE' => ['value'=> 'TRUE', 'expected' => true],
103373
];
104374
}
105375
}

0 commit comments

Comments
 (0)