From 1c2ba095969d460f34ba454c4880d6d17b28a47c Mon Sep 17 00:00:00 2001 From: Constantine Nathanson Date: Sun, 15 Jun 2025 16:31:18 +0300 Subject: [PATCH 1/2] Fix API parameters signature --- src/Api/Utils/ApiUtils.php | 8 ++++- tests/Unit/Utils/ApiUtilsTest.php | 59 ++++++++++++++++++++++++++----- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/src/Api/Utils/ApiUtils.php b/src/Api/Utils/ApiUtils.php index b8ebeb3f..6503ff75 100644 --- a/src/Api/Utils/ApiUtils.php +++ b/src/Api/Utils/ApiUtils.php @@ -244,8 +244,14 @@ public static function serializeResponsiveBreakpoints(?array $breakpoints): bool */ public static function serializeQueryParams(array $parameters = []): string { + // URL encode & characters in values to prevent parameter smuggling + $encodedParameters = ArrayUtils::mapAssoc( + static fn($key, $value) => str_replace('&', '%26', $value), + $parameters + ); + return ArrayUtils::implodeAssoc( - $parameters, + $encodedParameters, self::QUERY_STRING_OUTER_DELIMITER, self::QUERY_STRING_INNER_DELIMITER ); diff --git a/tests/Unit/Utils/ApiUtilsTest.php b/tests/Unit/Utils/ApiUtilsTest.php index d06c9b65..eb211c6c 100644 --- a/tests/Unit/Utils/ApiUtilsTest.php +++ b/tests/Unit/Utils/ApiUtilsTest.php @@ -22,7 +22,10 @@ */ final class ApiUtilsTest extends UnitTestCase { - public function tearDown() + public const API_SIGN_REQUEST_TEST_SECRET = 'hdcixPpR2iKERPwqvH6sHdK9cyac'; + public const API_SIGN_REQUEST_CLOUD_NAME = 'dn6ot3ged'; + + public function tearDown(): void { parent::tearDown(); @@ -230,7 +233,7 @@ public function dataProviderSignParameters() ], [ 'value' => ['p1' => 'v1=v2*|}{ & !@#$%^&*()_;/.,?><\\/|_+a'], - 'result' => 'bbdc631f4b490c0ba65722d8dbf9300d1fd98e86', + 'result' => 'ced1e363d8db0a8d7ebcfb9e67fadbf5ee78a0f1', ], ]; } @@ -276,12 +279,12 @@ public function dataProviderSignWithAlgorithmParameters() ], [ 'value' => ['p1' => 'v1=v2*|}{ & !@#$%^&*()_;/.,?><\\/|_+a'], - 'result' => 'bbdc631f4b490c0ba65722d8dbf9300d1fd98e86', + 'result' => 'ced1e363d8db0a8d7ebcfb9e67fadbf5ee78a0f1', 'signatureAlgorithm' => 'sha1', ], [ 'value' => ['p1' => 'v1=v2*|}{ & !@#$%^&*()_;/.,?><\\/|_+a'], - 'result' => '9cdbdd04f587b41db72d66437f6dac2a379cd899c0cf3c3430925b1beca6052d', + 'result' => '0c06416c30bfc727eb2cbc9f93245be70bd6567c788b5bd93a3772e8253312bf', 'signatureAlgorithm' => 'sha256', ], ]; @@ -310,13 +313,13 @@ public function testSignParametersWithExplicitSignatureAlgorithm($value, $result public function testApiSignRequestWithGlobalConfig() { $initialParams = [ - 'cloud_name' => 'dn6ot3ged', + 'cloud_name' => self::API_SIGN_REQUEST_CLOUD_NAME, 'timestamp' => 1568810420, 'username' => 'user@cloudinary.com' ]; $params = $initialParams; - Configuration::instance()->cloud->apiSecret = 'hdcixPpR2iKERPwqvH6sHdK9cyac'; + Configuration::instance()->cloud->apiSecret = self::API_SIGN_REQUEST_TEST_SECRET; Configuration::instance()->cloud->signatureAlgorithm = Utils::ALGO_SHA256; ApiUtils::signRequest($params, Configuration::instance()->cloud); $expected = '45ddaa4fa01f0c2826f32f669d2e4514faf275fe6df053f1a150e7beae58a3bd'; @@ -335,15 +338,55 @@ public function testApiSignRequestWithGlobalConfig() public function testApiSignRequestWithExplicitConfig() { $params = [ - 'cloud_name' => 'dn6ot3ged', + 'cloud_name' => self::API_SIGN_REQUEST_CLOUD_NAME, 'timestamp' => 1568810420, 'username' => 'user@cloudinary.com' ]; - $config = new Configuration('cloudinary://key:hdcixPpR2iKERPwqvH6sHdK9cyac@test123'); + $config = new Configuration('cloudinary://key:' . self::API_SIGN_REQUEST_TEST_SECRET . '@test123'); $config->cloud->signatureAlgorithm = Utils::ALGO_SHA256; ApiUtils::signRequest($params, $config->cloud); $expected = '45ddaa4fa01f0c2826f32f669d2e4514faf275fe6df053f1a150e7beae58a3bd'; self::assertEquals($expected, $params['signature']); } + + /** + * Should prevent parameter smuggling via & characters in parameter values. + */ + public function testApiSignRequestPreventsParameterSmuggling() + { + // Test with notification_url containing & characters + $paramsWithAmpersand = [ + 'cloud_name' => self::API_SIGN_REQUEST_CLOUD_NAME, + 'timestamp' => 1568810420, + 'notification_url' => 'https://fake.com/callback?a=1&tags=hello,world' + ]; + + $config = new Configuration('cloudinary://key:' . self::API_SIGN_REQUEST_TEST_SECRET . '@test123'); + ApiUtils::signRequest($paramsWithAmpersand, $config->cloud); + $signatureWithAmpersand = $paramsWithAmpersand['signature']; + + // Test that attempting to smuggle parameters by splitting the notification_url fails + $paramsSmugggled = [ + 'cloud_name' => self::API_SIGN_REQUEST_CLOUD_NAME, + 'timestamp' => 1568810420, + 'notification_url' => 'https://fake.com/callback?a=1', + 'tags' => 'hello,world' // This would be smuggled if & encoding didn't work + ]; + + ApiUtils::signRequest($paramsSmugggled, $config->cloud); + $signatureSmugggled = $paramsSmugggled['signature']; + + // The signatures should be different, proving that parameter smuggling is prevented + self::assertNotEquals($signatureWithAmpersand, $signatureSmugggled, + 'Signatures should be different to prevent parameter smuggling'); + + // Verify the expected signature for the properly encoded case + $expectedSignature = '4fdf465dd89451cc1ed8ec5b3e314e8a51695704'; + self::assertEquals($expectedSignature, $signatureWithAmpersand); + + // Verify the expected signature for the smuggled parameters case + $expectedSmuggledSignature = '7b4e3a539ff1fa6e6700c41b3a2ee77586a025f9'; + self::assertEquals($expectedSmuggledSignature, $signatureSmugggled); + } } From 2c96380f07e6144335efaec662b4fe29f54a87c6 Mon Sep 17 00:00:00 2001 From: Constantine Nathanson Date: Tue, 17 Jun 2025 20:08:06 +0300 Subject: [PATCH 2/2] Add `signatureVersion` config --- src/Api/Utils/ApiUtils.php | 27 ++++++++++++-------- src/Configuration/CloudConfig.php | 8 ++++++ src/Configuration/CloudConfigTrait.php | 14 +++++++++++ tests/Unit/Utils/ApiUtilsTest.php | 35 ++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 11 deletions(-) diff --git a/src/Api/Utils/ApiUtils.php b/src/Api/Utils/ApiUtils.php index 6503ff75..58d22850 100644 --- a/src/Api/Utils/ApiUtils.php +++ b/src/Api/Utils/ApiUtils.php @@ -242,16 +242,18 @@ public static function serializeResponsiveBreakpoints(?array $breakpoints): bool * * @internal */ - public static function serializeQueryParams(array $parameters = []): string + public static function serializeQueryParams(array $parameters = [], int $signatureVersion = 2): string { - // URL encode & characters in values to prevent parameter smuggling - $encodedParameters = ArrayUtils::mapAssoc( - static fn($key, $value) => str_replace('&', '%26', $value), - $parameters - ); - + // Version 2: URL encode & characters in values to prevent parameter smuggling + if ($signatureVersion >= 2) { + $parameters = ArrayUtils::mapAssoc( + static fn($key, $value) => str_replace('&', '%26', $value), + $parameters + ); + } + return ArrayUtils::implodeAssoc( - $encodedParameters, + $parameters, self::QUERY_STRING_OUTER_DELIMITER, self::QUERY_STRING_INNER_DELIMITER ); @@ -263,6 +265,7 @@ public static function serializeQueryParams(array $parameters = []): string * @param array $parameters Parameters to sign. * @param string $secret The API secret of the cloud. * @param string $signatureAlgorithm Signature algorithm + * @param int $signatureVersion Signature version (1 or 2) * * @return string The signature. * @@ -271,13 +274,14 @@ public static function serializeQueryParams(array $parameters = []): string public static function signParameters( array $parameters, string $secret, - string $signatureAlgorithm = Utils::ALGO_SHA1 + string $signatureAlgorithm = Utils::ALGO_SHA1, + int $signatureVersion = 2 ): string { $parameters = array_map(self::class . '::serializeSimpleApiParam', $parameters); ksort($parameters); - $signatureContent = self::serializeQueryParams($parameters); + $signatureContent = self::serializeQueryParams($parameters, $signatureVersion); return Utils::sign($signatureContent, $secret, false, $signatureAlgorithm); } @@ -293,7 +297,8 @@ public static function signRequest(?array &$parameters, CloudConfig $cloudConfig $parameters['signature'] = self::signParameters( $parameters, $cloudConfig->apiSecret, - $cloudConfig->signatureAlgorithm + $cloudConfig->signatureAlgorithm, + $cloudConfig->signatureVersion ); $parameters['api_key'] = $cloudConfig->apiKey; } diff --git a/src/Configuration/CloudConfig.php b/src/Configuration/CloudConfig.php index 19b80698..5ecc1dda 100644 --- a/src/Configuration/CloudConfig.php +++ b/src/Configuration/CloudConfig.php @@ -19,6 +19,7 @@ * target="_blank">Get account details from the Cloudinary Console. * * @property ?string $signatureAlgorithm By default, set to self::DEFAULT_SIGNATURE_ALGORITHM. + * @property ?int $signatureVersion By default, set to self::DEFAULT_SIGNATURE_VERSION. * * @api */ @@ -29,6 +30,7 @@ class CloudConfig extends BaseConfigSection public const CONFIG_NAME = 'cloud'; public const DEFAULT_SIGNATURE_ALGORITHM = Utils::ALGO_SHA1; + public const DEFAULT_SIGNATURE_VERSION = 2; // Supported parameters public const CLOUD_NAME = 'cloud_name'; @@ -36,6 +38,7 @@ class CloudConfig extends BaseConfigSection public const API_SECRET = 'api_secret'; public const OAUTH_TOKEN = 'oauth_token'; public const SIGNATURE_ALGORITHM = 'signature_algorithm'; + public const SIGNATURE_VERSION = 'signature_version'; /** * @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 */ protected ?string $signatureAlgorithm = null; + /** + * Sets the signature version (2 by default). + */ + protected ?int $signatureVersion = null; + /** * Serialises configuration section to a string representation. * diff --git a/src/Configuration/CloudConfigTrait.php b/src/Configuration/CloudConfigTrait.php index b24d0e8f..6f57ecb0 100644 --- a/src/Configuration/CloudConfigTrait.php +++ b/src/Configuration/CloudConfigTrait.php @@ -59,6 +59,20 @@ public function signatureAlgorithm(string $signatureAlgorithm): static return $this->setCloudConfig(CloudConfig::SIGNATURE_ALGORITHM, $signatureAlgorithm); } + /** + * Sets the signature version. + * + * @param int $signatureVersion The signature version to use. (Can be 1 or 2). + * + * @return $this + * + * @api + */ + public function signatureVersion(int $signatureVersion): static + { + return $this->setCloudConfig(CloudConfig::SIGNATURE_VERSION, $signatureVersion); + } + /** * Sets the Cloud configuration key with the specified value. * diff --git a/tests/Unit/Utils/ApiUtilsTest.php b/tests/Unit/Utils/ApiUtilsTest.php index eb211c6c..8cc6c9bc 100644 --- a/tests/Unit/Utils/ApiUtilsTest.php +++ b/tests/Unit/Utils/ApiUtilsTest.php @@ -389,4 +389,39 @@ public function testApiSignRequestPreventsParameterSmuggling() $expectedSmuggledSignature = '7b4e3a539ff1fa6e6700c41b3a2ee77586a025f9'; self::assertEquals($expectedSmuggledSignature, $signatureSmugggled); } + + /** + * Should apply the configured signature version from CloudConfig. + */ + public function testConfiguredSignatureVersionIsApplied() + { + $params = [ + 'cloud_name' => self::API_SIGN_REQUEST_CLOUD_NAME, + 'timestamp' => 1568810420, + 'notification_url' => 'https://fake.com/callback?a=1&tags=hello,world' + ]; + + $config = new Configuration('cloudinary://key:' . self::API_SIGN_REQUEST_TEST_SECRET . '@test123'); + + // Test with signature version 1 (legacy behavior - no URL encoding) + $config->cloud->signatureVersion = 1; + $paramsV1 = $params; + ApiUtils::signRequest($paramsV1, $config->cloud); + $signatureV1 = $paramsV1['signature']; + + // Test with signature version 2 (current behavior - with URL encoding) + $config->cloud->signatureVersion = 2; + $paramsV2 = $params; + ApiUtils::signRequest($paramsV2, $config->cloud); + $signatureV2 = $paramsV2['signature']; + + // Signatures should be different, proving the version setting is applied + self::assertNotEquals($signatureV1, $signatureV2, + 'Signature versions should produce different results'); + + // Version 2 should match the expected encoded signature + $expectedV2Signature = '4fdf465dd89451cc1ed8ec5b3e314e8a51695704'; + self::assertEquals($expectedV2Signature, $signatureV2, + 'Version 2 should match expected encoded signature'); + } }