fix: prevent parameter injection via ampersand in parameter values (#709)

* fix: prevent parameter injection via ampersand in parameter values

* fix: correct escaping

* fix: longer timeout between restore calls

* test: integration test for parameter value escaping

* chore: formatting

* chore: cleaner code

* chore: running all tests

* chore: grammar

* test: increased wait on restore tests

* test: aligned with other cld sdks

* feat: signature fix to uploader

* feat: verify_api_response_signature

* chore: verify_api_response_signature used from utils

* chore: whitespaces

Author: cloudinary-pkoniu

lib/preloaded_file.js

@@ -24,12 +24,7 @@ class PreloadedFile {
   }
 
   is_valid() {
-    let expected_signature;
-    expected_signature = utils.api_sign_request({
-      public_id: this.public_id,
-      version: this.version
-    }, config().api_secret);
-    return this.signature === expected_signature;
+    return utils.verify_api_response_signature(this.public_id, this.version, this.signature);
   }
 
   static split_format(identifier) {

lib/utils/index.js

@@ -544,9 +544,7 @@ function generate_transformation_string(options) {
   let base_transformations = toArray(consumeOption(options, "transformation", []));
   let named_transformation = [];
   if (base_transformations.some(isObject)) {
-    base_transformations = base_transformations.map(tr => utils.generate_transformation_string(
-      isObject(tr) ? clone(tr) : {transformation: tr}
-    ));
+    base_transformations = base_transformations.map(tr => utils.generate_transformation_string(isObject(tr) ? clone(tr) : {transformation: tr}));
   } else {
     named_transformation = base_transformations.join(".");
     base_transformations = [];
@@ -555,9 +553,7 @@ function generate_transformation_string(options) {
   if (isArray(effect)) {
     effect = effect.join(":");
   } else if (isObject(effect)) {
-    effect = entries(effect).map(
-      ([key, value]) => `${key}:${value}`
-    );
+    effect = entries(effect).map(([key, value]) => `${key}:${value}`);
   }
   let border = consumeOption(options, "border");
   if (isObject(border)) {
@@ -634,9 +630,7 @@ function generate_transformation_string(options) {
     .map(([key, value]) => {
       delete options[key];
       return `${key}_${normalize_expression(value)}`;
-    }).sort().concat(
-      variablesParam.map(([name, value]) => `${name}_${normalize_expression(value)}`)
-    ).join(',');
+    }).sort().concat(variablesParam.map(([name, value]) => `${name}_${normalize_expression(value)}`)).join(',');
 
   let transformations = entries(params)
     .filter(([key, value]) => utils.present(value))
@@ -649,8 +643,7 @@ function generate_transformation_string(options) {
   base_transformations.push(transformations);
   transformations = base_transformations;
   if (responsive_width) {
-    let responsive_width_transformation = config().responsive_width_transformation
-      || DEFAULT_RESPONSIVE_WIDTH_TRANSFORMATION;
+    let responsive_width_transformation = config().responsive_width_transformation || DEFAULT_RESPONSIVE_WIDTH_TRANSFORMATION;
 
     transformations.push(utils.generate_transformation_string(clone(responsive_width_transformation)));
   }
@@ -745,27 +738,7 @@ function updateable_resource_params(options, params = {}) {
  * A list of keys used by the url() function.
  * @private
  */
-const URL_KEYS = [
-  'api_secret',
-  'auth_token',
-  'cdn_subdomain',
-  'cloud_name',
-  'cname',
-  'format',
-  'long_url_signature',
-  'private_cdn',
-  'resource_type',
-  'secure',
-  'secure_cdn_subdomain',
-  'secure_distribution',
-  'shorten',
-  'sign_url',
-  'ssl_detected',
-  'type',
-  'url_suffix',
-  'use_root_path',
-  'version'
-];
+const URL_KEYS = ['api_secret', 'auth_token', 'cdn_subdomain', 'cloud_name', 'cname', 'format', 'long_url_signature', 'private_cdn', 'resource_type', 'secure', 'secure_cdn_subdomain', 'secure_distribution', 'shorten', 'sign_url', 'ssl_detected', 'type', 'url_suffix', 'use_root_path', 'version'];
 
 /**
  * Create a new object with only URL parameters
@@ -930,9 +903,7 @@ function url(public_id, options = {}) {
       urlAnalytics
     };
 
-    let analyticsOptions = getAnalyticsOptions(
-      Object.assign({}, options, sdkVersions)
-    );
+    let analyticsOptions = getAnalyticsOptions(Object.assign({}, options, sdkVersions));
 
     let sdkAnalyticsSignature = getSDKAnalyticsSignature(analyticsOptions);
 
@@ -1033,16 +1004,7 @@ function finalize_resource_type(resource_type, type, url_suffix, use_root_path,
 //    if cdn_domain is true uses a[1-5].cname for http.
 //    For https, uses the same naming scheme as 1 for shared distribution and as 2 for private distribution.
 
-function unsigned_url_prefix(
-  source,
-  cloud_name,
-  private_cdn,
-  cdn_subdomain,
-  secure_cdn_subdomain,
-  cname,
-  secure,
-  secure_distribution
-) {
+function unsigned_url_prefix(source, cloud_name, private_cdn, cdn_subdomain, secure_cdn_subdomain, cname, secure, secure_distribution) {
   let prefix;
   if (cloud_name.indexOf("/") === 0) {
     return '/res' + cloud_name;
@@ -1112,13 +1074,42 @@ function signed_preloaded_image(result) {
   return `${result.resource_type}/upload/v${result.version}/${filter([result.public_id, result.format], utils.present).join(".")}#${result.signature}`;
 }
 
-function api_sign_request(params_to_sign, api_secret) {
-  let to_sign = entries(params_to_sign).filter(
-    ([k, v]) => utils.present(v)
-  ).map(
-    ([k, v]) => `${k}=${toArray(v).join(",")}`
-  ).sort().join("&");
-  return compute_hash(to_sign + api_secret, config().signature_algorithm || DEFAULT_SIGNATURE_ALGORITHM, 'hex');
+// Encodes a parameter for safe inclusion in URL query strings (only replaces & with %26)
+function encode_param(value) {
+  return String(value).replace(/&/g, '%26');
+}
+
+// Generates a string to be signed for API requests
+function api_string_to_sign(params_to_sign, signature_version = 2) {
+  let params = entries(params_to_sign)
+    .map(([k, v]) => [String(k), Array.isArray(v) ? v.join(",") : v])
+    .filter(([k, v]) => v !== null && v !== undefined && v !== "");
+  params.sort((a, b) => a[0].localeCompare(b[0]));
+  let paramStrings = params.map(([k, v]) => {
+    const paramString = `${k}=${v}`;
+    return signature_version >= 2 ? encode_param(paramString) : paramString;
+  });
+  return paramStrings.join("&");
+}
+
+/**
+ * Signs API request parameters
+ * @param {Object} params_to_sign Parameters to sign
+ * @param {string} api_secret API secret
+ * @param {string|undefined|null} signature_algorithm Hash algorithm to use ('sha1' or 'sha256')
+ * @param {number|undefined|null} signature_version Version of signature algorithm to use:
+ *   - Version 1: Original behavior without parameter encoding
+ *   - Version 2+ (default): Includes parameter encoding to prevent parameter smuggling
+ * @return {string} Hexadecimal signature
+ * @private
+ */
+function api_sign_request(params_to_sign, api_secret, signature_algorithm = null, signature_version = null) {
+  if (signature_version == null) {
+    signature_version = config().signature_version || 2;
+  }
+  const to_sign = api_string_to_sign(params_to_sign, signature_version);
+  const algo = signature_algorithm || config().signature_algorithm || DEFAULT_SIGNATURE_ALGORITHM;
+  return compute_hash(to_sign + api_secret, algo, 'hex');
 }
 
 /**
@@ -1139,13 +1130,9 @@ function compute_hash(input, signature_algorithm, encoding) {
 
 function clear_blank(hash) {
   let filtered_hash = {};
-  entries(hash).filter(
-    ([k, v]) => utils.present(v)
-  ).forEach(
-    ([k, v]) => {
-      filtered_hash[k] = v.filter ? v.filter(x => x) : v;
-    }
-  );
+  entries(hash).filter(([k, v]) => utils.present(v)).forEach(([k, v]) => {
+    filtered_hash[k] = v.filter ? v.filter(x => x) : v;
+  });
   return filtered_hash;
 }
 
@@ -1163,8 +1150,10 @@ function merge(hash1, hash2) {
 function sign_request(params, options = {}) {
   let apiKey = ensureOption(options, 'api_key');
   let apiSecret = ensureOption(options, 'api_secret');
+  let signature_algorithm = options.signature_algorithm;
+  let signature_version = options.signature_version;
   params = exports.clear_blank(params);
-  params.signature = exports.api_sign_request(params, apiSecret);
+  params.signature = exports.api_sign_request(params, apiSecret, signature_algorithm, signature_version);
   params.api_key = apiKey;
   return params;
 }
@@ -1556,9 +1545,7 @@ function generate_responsive_breakpoints_string(breakpoints) {
     let breakpoint_settings = breakpoints[j];
     if (breakpoint_settings != null) {
       if (breakpoint_settings.transformation) {
-        breakpoint_settings.transformation = utils.generate_transformation_string(
-          clone(breakpoint_settings.transformation)
-        );
+        breakpoint_settings.transformation = utils.generate_transformation_string(clone(breakpoint_settings.transformation));
       }
     }
   }
@@ -1568,11 +1555,9 @@ function generate_responsive_breakpoints_string(breakpoints) {
 function build_streaming_profiles_param(options = {}) {
   let params = pickOnlyExistingValues(options, "display_name", "representations");
   if (isArray(params.representations)) {
-    params.representations = JSON.stringify(params.representations.map(
-      r => ({
-        transformation: utils.generate_transformation_string(r.transformation)
-      })
-    ));
+    params.representations = JSON.stringify(params.representations.map(r => ({
+      transformation: utils.generate_transformation_string(r.transformation)
+    })));
   }
   return params;
 }
@@ -1597,9 +1582,7 @@ function hashToParameters(hash) {
  * @return {string} A URI query string.
  */
 function hashToQuery(hash) {
-  return hashToParameters(hash).map(
-    ([key, value]) => `${querystring.escape(key)}=${querystring.escape(value)}`
-  ).join('&');
+  return hashToParameters(hash).map(([key, value]) => `${querystring.escape(key)}=${querystring.escape(value)}`).join('&');
 }
 
 /**
@@ -1742,3 +1725,27 @@ Object.assign(module.exports, {
   keys: source => Object.keys(source),
   ensurePresenceOf
 });
+
+/**
+ * Verifies an API response signature for a given public_id and version.
+ * Always uses signature version 1 for backward compatibility, matching the Ruby SDK.
+ * @param {string} public_id
+ * @param {string|number} version
+ * @param {string} signature
+ * @returns {boolean}
+ */
+function verify_api_response_signature(public_id, version, signature) {
+  const api_secret = config().api_secret;
+  const expected = exports.api_sign_request(
+    {
+      public_id,
+      version
+    },
+    api_secret,
+    null,
+    1
+  );
+  return signature === expected;
+}
+
+exports.verify_api_response_signature = verify_api_response_signature;

test/integration/api/admin/api_spec.js

@@ -1374,12 +1374,12 @@ describe("api", function () {
       const secondAssetVersion = getVersionsResp.versions[1].version_id;
 
       // Restore first version, ensure it's equal to the upload size
-      await wait(1000)();
+      await wait(2000)();
       const firstVerRestore = await API_V2.restore([PUBLIC_ID_BACKUP_1], {versions: [firstAssetVersion]});
       expect(firstVerRestore[PUBLIC_ID_BACKUP_1].bytes).to.eql(firstUpload.bytes);
 
       // Restore second version, ensure it's equal to the upload size
-      await wait(1000)();
+      await wait(2000)();
       const secondVerRestore = await API_V2.restore([PUBLIC_ID_BACKUP_1], {versions: [secondAssetVersion]});
       expect(secondVerRestore[PUBLIC_ID_BACKUP_1].bytes).to.eql(secondUpload.bytes);