Skip to content

Commit a71e0a0

Browse files
const-cloudinaryconst-cloudinary
authored
Fix API parameters signature
1 parent 692e9ea commit a71e0a0

File tree

4 files changed

+123
-12
lines changed

4 files changed

+123
-12
lines changed

‎src/Api/Utils/ApiUtils.php‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -242,8 +242,16 @@ public static function serializeResponsiveBreakpoints(?array $breakpoints): bool
242242
*
243243
* @internal
244244
*/
245-
public static function serializeQueryParams(array $parameters = []): string
245+
public static function serializeQueryParams(array $parameters = [], int $signatureVersion = 2): string
246246
{
247+
// Version 2: URL encode & characters in values to prevent parameter smuggling
248+
if ($signatureVersion >= 2) {
249+
$parameters = ArrayUtils::mapAssoc(
250+
static fn($key, $value) => str_replace('&', '%26', $value),
251+
$parameters
252+
);
253+
}
254+
247255
return ArrayUtils::implodeAssoc(
248256
$parameters,
249257
self::QUERY_STRING_OUTER_DELIMITER,
@@ -257,6 +265,7 @@ public static function serializeQueryParams(array $parameters = []): string
257265
* @param array $parameters Parameters to sign.
258266
* @param string $secret The API secret of the cloud.
259267
* @param string $signatureAlgorithm Signature algorithm
268+
* @param int $signatureVersion Signature version (1 or 2)
260269
*
261270
* @return string The signature.
262271
*
@@ -265,13 +274,14 @@ public static function serializeQueryParams(array $parameters = []): string
265274
public static function signParameters(
266275
array $parameters,
267276
string $secret,
268-
string $signatureAlgorithm = Utils::ALGO_SHA1
277+
string $signatureAlgorithm = Utils::ALGO_SHA1,
278+
int $signatureVersion = 2
269279
): string {
270280
$parameters = array_map(self::class . '::serializeSimpleApiParam', $parameters);
271281

272282
ksort($parameters);
273283

274-
$signatureContent = self::serializeQueryParams($parameters);
284+
$signatureContent = self::serializeQueryParams($parameters, $signatureVersion);
275285

276286
return Utils::sign($signatureContent, $secret, false, $signatureAlgorithm);
277287
}
@@ -287,7 +297,8 @@ public static function signRequest(?array &$parameters, CloudConfig $cloudConfig
287297
$parameters['signature'] = self::signParameters(
288298
$parameters,
289299
$cloudConfig->apiSecret,
290-
$cloudConfig->signatureAlgorithm
300+
$cloudConfig->signatureAlgorithm,
301+
$cloudConfig->signatureVersion
291302
);
292303
$parameters['api_key'] = $cloudConfig->apiKey;
293304
}

‎src/Configuration/CloudConfig.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
* target="_blank">Get account details from the Cloudinary Console.</a>
2020
*
2121
* @property ?string $signatureAlgorithm By default, set to self::DEFAULT_SIGNATURE_ALGORITHM.
22+
* @property ?int $signatureVersion By default, set to self::DEFAULT_SIGNATURE_VERSION.
2223
*
2324
* @api
2425
*/
@@ -29,13 +30,15 @@ class CloudConfig extends BaseConfigSection
2930
public const CONFIG_NAME = 'cloud';
3031

3132
public const DEFAULT_SIGNATURE_ALGORITHM = Utils::ALGO_SHA1;
33+
public const DEFAULT_SIGNATURE_VERSION = 2;
3234

3335
// Supported parameters
3436
public const CLOUD_NAME = 'cloud_name';
3537
public const API_KEY = 'api_key';
3638
public const API_SECRET = 'api_secret';
3739
public const OAUTH_TOKEN = 'oauth_token';
3840
public const SIGNATURE_ALGORITHM = 'signature_algorithm';
41+
public const SIGNATURE_VERSION = 'signature_version';
3942

4043
/**
4144
* @var array of configuration keys that contain sensitive data that should not be exported (for example api key)
@@ -69,6 +72,11 @@ class CloudConfig extends BaseConfigSection
6972
*/
7073
protected ?string $signatureAlgorithm = null;
7174

75+
/**
76+
* Sets the signature version (2 by default).
77+
*/
78+
protected ?int $signatureVersion = null;
79+
7280
/**
7381
* Serialises configuration section to a string representation.
7482
*

‎src/Configuration/CloudConfigTrait.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,20 @@ public function signatureAlgorithm(string $signatureAlgorithm): static
5959
return $this->setCloudConfig(CloudConfig::SIGNATURE_ALGORITHM, $signatureAlgorithm);
6060
}
6161

62+
/**
63+
* Sets the signature version.
64+
*
65+
* @param int $signatureVersion The signature version to use. (Can be 1 or 2).
66+
*
67+
* @return $this
68+
*
69+
* @api
70+
*/
71+
public function signatureVersion(int $signatureVersion): static
72+
{
73+
return $this->setCloudConfig(CloudConfig::SIGNATURE_VERSION, $signatureVersion);
74+
}
75+
6276
/**
6377
* Sets the Cloud configuration key with the specified value.
6478
*

‎tests/Unit/Utils/ApiUtilsTest.php‎

Lines changed: 86 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@
2222
*/
2323
final class ApiUtilsTest extends UnitTestCase
2424
{
25-
public function tearDown()
25+
public const API_SIGN_REQUEST_TEST_SECRET = 'hdcixPpR2iKERPwqvH6sHdK9cyac';
26+
public const API_SIGN_REQUEST_CLOUD_NAME = 'dn6ot3ged';
27+
28+
public function tearDown(): void
2629
{
2730
parent::tearDown();
2831

@@ -230,7 +233,7 @@ public function dataProviderSignParameters()
230233
],
231234
[
232235
'value' => ['p1' => 'v1=v2*|}{ & !@#$%^&*()_;/.,?><\\/|_+a'],
233-
'result' => 'bbdc631f4b490c0ba65722d8dbf9300d1fd98e86',
236+
'result' => 'ced1e363d8db0a8d7ebcfb9e67fadbf5ee78a0f1',
234237
],
235238
];
236239
}
@@ -276,12 +279,12 @@ public function dataProviderSignWithAlgorithmParameters()
276279
],
277280
[
278281
'value' => ['p1' => 'v1=v2*|}{ & !@#$%^&*()_;/.,?><\\/|_+a'],
279-
'result' => 'bbdc631f4b490c0ba65722d8dbf9300d1fd98e86',
282+
'result' => 'ced1e363d8db0a8d7ebcfb9e67fadbf5ee78a0f1',
280283
'signatureAlgorithm' => 'sha1',
281284
],
282285
[
283286
'value' => ['p1' => 'v1=v2*|}{ & !@#$%^&*()_;/.,?><\\/|_+a'],
284-
'result' => '9cdbdd04f587b41db72d66437f6dac2a379cd899c0cf3c3430925b1beca6052d',
287+
'result' => '0c06416c30bfc727eb2cbc9f93245be70bd6567c788b5bd93a3772e8253312bf',
285288
'signatureAlgorithm' => 'sha256',
286289
],
287290
];
@@ -310,13 +313,13 @@ public function testSignParametersWithExplicitSignatureAlgorithm($value, $result
310313
public function testApiSignRequestWithGlobalConfig()
311314
{
312315
$initialParams = [
313-
'cloud_name' => 'dn6ot3ged',
316+
'cloud_name' => self::API_SIGN_REQUEST_CLOUD_NAME,
314317
'timestamp' => 1568810420,
315318
'username' => '[email protected]'
316319
];
317320

318321
$params = $initialParams;
319-
Configuration::instance()->cloud->apiSecret = 'hdcixPpR2iKERPwqvH6sHdK9cyac';
322+
Configuration::instance()->cloud->apiSecret = self::API_SIGN_REQUEST_TEST_SECRET;
320323
Configuration::instance()->cloud->signatureAlgorithm = Utils::ALGO_SHA256;
321324
ApiUtils::signRequest($params, Configuration::instance()->cloud);
322325
$expected = '45ddaa4fa01f0c2826f32f669d2e4514faf275fe6df053f1a150e7beae58a3bd';
@@ -335,15 +338,90 @@ public function testApiSignRequestWithGlobalConfig()
335338
public function testApiSignRequestWithExplicitConfig()
336339
{
337340
$params = [
338-
'cloud_name' => 'dn6ot3ged',
341+
'cloud_name' => self::API_SIGN_REQUEST_CLOUD_NAME,
339342
'timestamp' => 1568810420,
340343
'username' => '[email protected]'
341344
];
342345

343-
$config = new Configuration('cloudinary://key:hdcixPpR2iKERPwqvH6sHdK9cyac@test123');
346+
$config = new Configuration('cloudinary://key:' . self::API_SIGN_REQUEST_TEST_SECRET . '@test123');
344347
$config->cloud->signatureAlgorithm = Utils::ALGO_SHA256;
345348
ApiUtils::signRequest($params, $config->cloud);
346349
$expected = '45ddaa4fa01f0c2826f32f669d2e4514faf275fe6df053f1a150e7beae58a3bd';
347350
self::assertEquals($expected, $params['signature']);
348351
}
352+
353+
/**
354+
* Should prevent parameter smuggling via & characters in parameter values.
355+
*/
356+
public function testApiSignRequestPreventsParameterSmuggling()
357+
{
358+
// Test with notification_url containing & characters
359+
$paramsWithAmpersand = [
360+
'cloud_name' => self::API_SIGN_REQUEST_CLOUD_NAME,
361+
'timestamp' => 1568810420,
362+
'notification_url' => 'https://fake.com/callback?a=1&tags=hello,world'
363+
];
364+
365+
$config = new Configuration('cloudinary://key:' . self::API_SIGN_REQUEST_TEST_SECRET . '@test123');
366+
ApiUtils::signRequest($paramsWithAmpersand, $config->cloud);
367+
$signatureWithAmpersand = $paramsWithAmpersand['signature'];
368+
369+
// Test that attempting to smuggle parameters by splitting the notification_url fails
370+
$paramsSmugggled = [
371+
'cloud_name' => self::API_SIGN_REQUEST_CLOUD_NAME,
372+
'timestamp' => 1568810420,
373+
'notification_url' => 'https://fake.com/callback?a=1',
374+
'tags' => 'hello,world' // This would be smuggled if & encoding didn't work
375+
];
376+
377+
ApiUtils::signRequest($paramsSmugggled, $config->cloud);
378+
$signatureSmugggled = $paramsSmugggled['signature'];
379+
380+
// The signatures should be different, proving that parameter smuggling is prevented
381+
self::assertNotEquals($signatureWithAmpersand, $signatureSmugggled,
382+
'Signatures should be different to prevent parameter smuggling');
383+
384+
// Verify the expected signature for the properly encoded case
385+
$expectedSignature = '4fdf465dd89451cc1ed8ec5b3e314e8a51695704';
386+
self::assertEquals($expectedSignature, $signatureWithAmpersand);
387+
388+
// Verify the expected signature for the smuggled parameters case
389+
$expectedSmuggledSignature = '7b4e3a539ff1fa6e6700c41b3a2ee77586a025f9';
390+
self::assertEquals($expectedSmuggledSignature, $signatureSmugggled);
391+
}
392+
393+
/**
394+
* Should apply the configured signature version from CloudConfig.
395+
*/
396+
public function testConfiguredSignatureVersionIsApplied()
397+
{
398+
$params = [
399+
'cloud_name' => self::API_SIGN_REQUEST_CLOUD_NAME,
400+
'timestamp' => 1568810420,
401+
'notification_url' => 'https://fake.com/callback?a=1&tags=hello,world'
402+
];
403+
404+
$config = new Configuration('cloudinary://key:' . self::API_SIGN_REQUEST_TEST_SECRET . '@test123');
405+
406+
// Test with signature version 1 (legacy behavior - no URL encoding)
407+
$config->cloud->signatureVersion = 1;
408+
$paramsV1 = $params;
409+
ApiUtils::signRequest($paramsV1, $config->cloud);
410+
$signatureV1 = $paramsV1['signature'];
411+
412+
// Test with signature version 2 (current behavior - with URL encoding)
413+
$config->cloud->signatureVersion = 2;
414+
$paramsV2 = $params;
415+
ApiUtils::signRequest($paramsV2, $config->cloud);
416+
$signatureV2 = $paramsV2['signature'];
417+
418+
// Signatures should be different, proving the version setting is applied
419+
self::assertNotEquals($signatureV1, $signatureV2,
420+
'Signature versions should produce different results');
421+
422+
// Version 2 should match the expected encoded signature
423+
$expectedV2Signature = '4fdf465dd89451cc1ed8ec5b3e314e8a51695704';
424+
self::assertEquals($expectedV2Signature, $signatureV2,
425+
'Version 2 should match expected encoded signature');
426+
}
349427
}

0 commit comments

Comments
 (0)