From 7f3417b6ab9c5b9bc034cd9b6df91e4703c295ba Mon Sep 17 00:00:00 2001 From: Constantine Nathanson Date: Tue, 30 Jun 2026 01:54:10 +0300 Subject: [PATCH 1/3] Fix: raise clear error when signing without api_secret Add guards to cloudinary_url() and api_sign_request() to raise ValueError("Must supply api_secret") instead of opaque TypeError when attempting to sign without a configured secret. This matches the behavior of other signing paths (sign_request, Search.to_url, etc.) and provides clear guidance to OAuth-only configurations. Changes: - cloudinary_url(): Guard signing block to validate api_secret before use - api_sign_request(): Guard function entry to validate api_secret parameter - Add 7 comprehensive test cases covering all scenarios Fixes spec: cloudinary-url-sign-without-secret.md Co-Authored-By: Claude Haiku 4.5 --- cloudinary/utils.py | 4 ++++ test/test_utils.py | 49 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/cloudinary/utils.py b/cloudinary/utils.py index 87914ee..be21fb7 100644 --- a/cloudinary/utils.py +++ b/cloudinary/utils.py @@ -642,6 +642,8 @@ def api_sign_request(params_to_sign, api_secret, algorithm=SIGNATURE_SHA1, signa - Version 2+: Includes parameter encoding to prevent parameter smuggling :return: Computed signature """ + if not api_secret: + raise ValueError("Must supply api_secret") to_sign = api_string_to_sign(params_to_sign, signature_version) return compute_hex_hash(to_sign + api_secret, algorithm) @@ -875,6 +877,8 @@ def cloudinary_url(source, **options): signature = None if sign_url and (not auth_token or auth_token.pop('set_url_signature', False)): + if not api_secret: + raise ValueError("Must supply api_secret") to_sign = "/".join(__compact([transformation, source_to_sign])) if long_url_signature: # Long signature forces SHA256 diff --git a/test/test_utils.py b/test/test_utils.py index 57f19e1..c9a6997 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -21,6 +21,7 @@ encode_unicode_url, base64url_encode, patch_fetch_format, + cloudinary_url, cloudinary_scaled_url, chain_transformations, generate_transformation_string, @@ -1591,6 +1592,54 @@ def test_sign_request_with_signature_version(self): self.assertEqual(signed_params_v1['signature'], expected_sig_v1) self.assertEqual(signed_params_v2['signature'], expected_sig_v2) + def test_cloudinary_url_sign_without_secret_raises(self): + """Signing without a secret should raise ValueError""" + cloudinary.config(cloud_name="test123", api_secret=None) + with self.assertRaises(ValueError) as context: + cloudinary_url("sample", sign_url=True) + self.assertEqual(str(context.exception), "Must supply api_secret") + + def test_cloudinary_url_unsigned_without_secret_works(self): + """Unsigned URL should work without a secret""" + cloudinary.config(cloud_name="test123", api_secret=None) + url, _ = cloudinary_url("sample") + self.assertIn("test123", url) + self.assertNotIn("s--", url) + + def test_cloudinary_url_sign_with_secret_works(self): + """Signing with a secret should work and include signature""" + cloudinary.config(cloud_name="test123", api_key="key", api_secret="secret") + url, _ = cloudinary_url("sample", sign_url=True) + self.assertIn("s--", url) + self.assertIn("test123", url) + + def test_cloudinary_url_per_call_secret_override(self): + """Per-call api_secret override should sign successfully""" + cloudinary.config(cloud_name="test123", api_secret=None) + url, _ = cloudinary_url("sample", sign_url=True, api_secret="override_secret") + self.assertIn("s--", url) + self.assertIn("test123", url) + + def test_api_sign_request_without_secret_raises(self): + """api_sign_request with None secret should raise ValueError""" + params = {"a": "b"} + with self.assertRaises(ValueError) as context: + api_sign_request(params, None) + self.assertEqual(str(context.exception), "Must supply api_secret") + + def test_api_sign_request_with_empty_string_raises(self): + """api_sign_request with empty string secret should raise ValueError""" + params = {"a": "b"} + with self.assertRaises(ValueError) as context: + api_sign_request(params, "") + self.assertEqual(str(context.exception), "Must supply api_secret") + + def test_api_sign_request_with_secret_works(self): + """api_sign_request with a real secret should work""" + params = dict(cloud_name=API_SIGN_REQUEST_CLOUD_NAME, timestamp=1568810420, username="user@cloudinary.com") + signature = api_sign_request(params, API_SIGN_REQUEST_TEST_SECRET) + self.assertEqual(signature, "14c00ba6d0dfdedbc86b316847d95b9e6cd46d94") + if __name__ == '__main__': unittest.main() From 1e82061c890290e55f4fa35f7a0582e41d06cb11 Mon Sep 17 00:00:00 2001 From: Constantine Nathanson Date: Tue, 30 Jun 2026 01:58:47 +0300 Subject: [PATCH 2/3] CI: skip duplicate test runs on PR creation Prevent workflow from running twice (once on push, once on PR) by adding a condition that skips push events when they have an associated PR. Co-Authored-By: Claude Haiku 4.5 --- .github/workflows/test.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 214cbef..3c43a1d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -4,6 +4,8 @@ on: [push, pull_request] jobs: build: + # Skip redundant runs: don't run on push if there's an associated PR + if: github.event_name == 'pull_request' || github.event.pull_request == null runs-on: ubuntu-24.04 # noble equivalent strategy: From d9fc7792051593e8af46f14d503993ccd71aef1a Mon Sep 17 00:00:00 2001 From: Constantine Nathanson Date: Tue, 30 Jun 2026 02:09:49 +0300 Subject: [PATCH 3/3] CI: run tests on all branch pushes, once per same-repo PR The previous condition (github.event.pull_request == null) did not work: github.event.pull_request is always null on push events regardless of whether a PR exists, so the redundant run was never skipped. Correct condition keeps on: [push, pull_request] and runs tests on every branch push while avoiding the duplicate when a same-repo PR exists: - push event: always runs (push to any branch, PR or not). On a push event github.event.pull_request.head.repo.fork dereferences to an empty string (falsy, no error), so the negation is true. - same-repo PR: skipped, since that push already triggered a run. - fork PR: runs via pull_request, since forks can't trigger a push here. Verified against GitHub docs: dereferencing a missing context property yields an empty string rather than an error. Co-Authored-By: Claude Haiku 4.5 --- .github/workflows/test.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3c43a1d..ef1572a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -4,8 +4,12 @@ on: [push, pull_request] jobs: build: - # Skip redundant runs: don't run on push if there's an associated PR - if: github.event_name == 'pull_request' || github.event.pull_request == null + # Run on every branch push, but avoid duplicate runs when a same-repo PR + # exists: same-repo changes run via the push event, while fork PRs (which + # can't trigger a push in this repo) run via the pull_request event. + if: >- + (github.event_name == 'push' && !github.event.pull_request.head.repo.fork) || + (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork) runs-on: ubuntu-24.04 # noble equivalent strategy: