Harden Cloudinary credential config#98
Open
haradahinata wants to merge 1 commit into
Open
Conversation
68 tasks
📝 WalkthroughWalkthroughCloudinary credentials are hardened by moving them from hardcoded values to environment variables, gating all API operations behind a configuration assertion in CloudinaryManager, and enforcing the pattern via a CI validation script that checks code, config, and docs consistency. ChangesCloudinary Credential Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Related Knowledge 3 documents with suggested updates are ready for review. BAWES Universe plugn backend – local setupView Suggested Changes@@ -62,7 +62,7 @@
- TapPayments: `plugnLiveApiKey`, `plugnTestApiKey`, `destinationId`
- MyFatoorah: `kuwaitLiveApiKey`, `kuwaitTestApiKey`, `saudiLiveApiKey`
- **Other Keys**:
- - Cloudinary: `cloud_name`, `api_key`, `api_secret`
+ - Cloudinary: Configured via environment variables `CLOUDINARY_CLOUD_NAME`, `CLOUDINARY_API_KEY`, `CLOUDINARY_API_SECRET` (set these in your `.env` file or local environment; `main.php` reads from these variables)
- ApplePay, Ipstack, reCaptcha, Auth0, Zapier, JWT, Google Maps, Netlify, Github, OneSignal, currencylayer
See [`main.php`](https://github.com/BAWES-Universe/plugn/blob/bc485b0a1da61d516955c5dc4fc29e95afccea92/common/config/main.php#L3-L172) for full details.Plugn IntegrationsView Suggested Changes@@ -117,15 +117,15 @@
```php
'cloudinaryManager' => [
'class' => 'common\components\CloudinaryManager',
- 'cloud_name' => 'plugn',
- 'api_key' => '...',
- 'api_secret' => '...',
+ 'cloud_name' => getenv('CLOUDINARY_CLOUD_NAME') ?: null,
+ 'api_key' => getenv('CLOUDINARY_API_KEY') ?: null,
+ 'api_secret' => getenv('CLOUDINARY_API_SECRET') ?: null,
],
```
[Source](https://github.com/BAWES-Universe/plugn/blob/bc485b0a1da61d516955c5dc4fc29e95afccea92/common/config/main.php#L3-L172)
**Local Development:**
-OPTIONAL. Required for media management features, but can be stubbed or omitted for local development if not needed.
+REQUIRED. `CloudinaryManager` implements fail-closed behavior: when any of the three environment variables (`CLOUDINARY_CLOUD_NAME`, `CLOUDINARY_API_KEY`, `CLOUDINARY_API_SECRET`) is missing, upload/delete/asset lookup operations will throw an exception. For local development, either set valid test credentials or avoid features that depend on Cloudinary.
---
@@ -144,7 +144,9 @@
## Local-only Minimal Config
-To run the backend locally, only core dependencies are required (such as MySQL, Redis, PHP, etc.). All external integrations listed above (AWS S3, Slack webhooks, TapPayments, MyFatoorah, Cloudinary) can be stubbed or omitted for local development. You do not need to provide real API keys or connect to external services unless you are specifically developing or testing those features.
+To run the backend locally, only core dependencies are required (such as MySQL, Redis, PHP, etc.). Most external integrations listed above (AWS S3, Slack webhooks, TapPayments, MyFatoorah) can be stubbed or omitted for local development. You do not need to provide real API keys or connect to external services unless you are specifically developing or testing those features.
+
+**Exception: Cloudinary** cannot be stubbed with empty strings. `CloudinaryManager` implements fail-closed behavior and will throw an exception when environment variables are missing. Either set valid test credentials or avoid Cloudinary-dependent features during local development.
**Example minimal config for local development:**
@@ -152,9 +154,10 @@
// In plugn/common/config/main.php, you can stub integrations like this:
'cloudinaryManager' => [
'class' => 'common\components\CloudinaryManager',
- 'cloud_name' => '',
- 'api_key' => '',
- 'api_secret' => '',
+ 'cloud_name' => getenv('CLOUDINARY_CLOUD_NAME') ?: null,
+ 'api_key' => getenv('CLOUDINARY_API_KEY') ?: null,
+ 'api_secret' => getenv('CLOUDINARY_API_SECRET') ?: null,
+ // Do not use empty strings; operations will fail if credentials are missing
],
'temporaryBucketResourceManager' => [
'class' => 'common\components\S3ResourceManager',
@@ -185,4 +188,4 @@
// ...other stubs as needed
```
-No external integration is strictly required to boot and run the backend locally, unless you are actively developing or testing features that depend on them.
+Most external integrations are not strictly required to boot and run the backend locally, unless you are actively developing or testing features that depend on them.Production Operations & Deployments RunbookView Suggested Changes@@ -39,7 +39,12 @@
### Backend (Railway)
- **Build & Deploy**: Uses Railway CLI. Set `RAILWAY_DOCKERFILE_PATH` to `./Dockerfile-nginx-dev-railway` (dev) or `./Dockerfile-nginx-railway` (prod). Use `railway login`, `railway link`, and `railway up` to deploy. [Reference](https://github.com/BAWES-Universe/studenthub/blob/ca90a5502040ba8191fe9f80d29d0b6a4d2a6152/railway/railway.md#L1-L34)
-- **Required Env Vars**: Set via Railway dashboard or `.env` files (not in repo).
+- **Required Env Vars**: Set via Railway dashboard or `.env` files (not in repo). Must include:
+ - `CLOUDINARY_CLOUD_NAME`: Cloudinary cloud name for image uploads
+ - `CLOUDINARY_API_KEY`: Cloudinary API key
+ - `CLOUDINARY_API_SECRET`: Cloudinary API secret
+
+ CloudinaryManager will throw exceptions on upload/delete/asset lookup operations if any of these variables are missing.
- **Database Connectivity**:
- Dev: MySQL (host=mysql, db=studenthub, user=studenthubuser, pass=12345)
- Prod: AWS RDS (host=studenthub-prod.cluster-c8mekjvvbygf.eu-west-2.rds.amazonaws.com, db=studenthub, user=bawes, pass=bawes12student!hub)
@@ -131,6 +136,7 @@
## Day-1 Ops Checklist
- Verify Railway deployments for dev/prod: run `railway link`, confirm `RAILWAY_DOCKERFILE_PATH`, inspect AWS RDS/Redis connections in `environments/prod/common/config/main-local.php`.
+- Set Cloudinary environment variables via Railway dashboard: `CLOUDINARY_CLOUD_NAME`, `CLOUDINARY_API_KEY`, `CLOUDINARY_API_SECRET`. Cloudinary upload/delete/asset lookup operations will fail if any of these are missing.
- Confirm CircleCI contexts: ensure `org-global` context holds AWS credentials for admin/staff S3/CloudFront buckets as listed in `.circleci/config.yml`.
- Check Sentry project (`dsn ...@sentry.io/168200`) for alerting rules tied to `environment` tags and ensure Slack logger targets the correct workspace.
- Review cron job logs under `~/logs` and trigger key jobs manually (e.g., `php ~/www/yii cron/daily`) after verifying cache flush steps from README.How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/check-cloudinary-hardening.py (1)
60-69: ⚡ Quick winConsider making the method body regex more robust.
The regex pattern on line 62 requires a newline before the closing brace and uses non-greedy matching, which could fail with different code formatting (e.g., single-line methods or varied whitespace). Consider using a more flexible pattern.
♻️ Proposed fix for more robust regex
method_match = re.search( - rf"public function {method}\([^)]*\)\s*\{{(.*?)\n\s*\}}", + rf"public function {method}\([^)]*\)\s*\{{(.*?)\}}", manager, flags=re.S, )Or for even more robustness, match balanced braces:
method_match = re.search( - rf"public function {method}\([^)]*\)\s*\{{(.*?)\n\s*\}}", + rf"public function {method}\([^)]*\)\s*\{{([^}}]*\$this->assertConfigured\(\);[^}}]*)\}}", manager, flags=re.S, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-cloudinary-hardening.py` around lines 60 - 69, The current regex that searches for the method bodies of CloudinaryManager::upload, ::delete, and ::getUrl is fragile because it requires a newline before the closing brace and can fail for single-line or differently formatted methods; update the search so it does not require a newline and accepts any whitespace/newline between the body and the closing brace (or alternatively implement a small brace-aware extractor to find the method body), ensure to keep re.S so dot matches newlines, and then check the extracted body for the "$this->assertConfigured();" call for the methods named upload, delete, and getUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/check-cloudinary-hardening.py`:
- Around line 60-69: The current regex that searches for the method bodies of
CloudinaryManager::upload, ::delete, and ::getUrl is fragile because it requires
a newline before the closing brace and can fail for single-line or differently
formatted methods; update the search so it does not require a newline and
accepts any whitespace/newline between the body and the closing brace (or
alternatively implement a small brace-aware extractor to find the method body),
ensure to keep re.S so dot matches newlines, and then check the extracted body
for the "$this->assertConfigured();" call for the methods named upload, delete,
and getUrl.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f534344b-c811-42d7-9d0e-fb1941cd614b
⛔ Files ignored due to path filters (1)
docs/demo/studenthub-55-cloudinary-hardening-demo.gifis excluded by!**/*.gif
📒 Files selected for processing (4)
common/components/CloudinaryManager.phpcommon/config/main.phpdocs/setup.mdscripts/check-cloudinary-hardening.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
/claim #55
Contributes to #55.
Scope
This is a narrow Cloudinary credential hardening slice. It removes the checked-in Cloudinary cloud name, API key, and API secret from
common/config/main.php, wires the component through runtime environment variables, and makesCloudinaryManagerfail closed before upload, delete, or asset lookup calls when any required Cloudinary setting is missing.This intentionally does not touch AWS/IAM/S3, Civil ID backend/UI flows, SQS, MediaConvert, SES/mailers, Xero, OneSignal, Google/Jira/Algolia/IP/Slack slices already covered by other PRs, live Cloudinary access, candidate data, or real secret values.
Demo
Privacy-safe static demo GIF: https://github.com/haradahinata/studenthub/blob/codex/cloudinary-config-hardening-55/docs/demo/studenthub-55-cloudinary-hardening-demo.gif?raw=1
Verification
python scripts/check-cloudinary-hardening.py # Cloudinary hardening check passed. python -m py_compile scripts/check-cloudinary-hardening.py git diff --checkphp -land Docker-based PHP linting could not be run in this Windows environment because neitherphpnordockeris installed. The PHP change is limited to env-backed config wiring and a fail-closed guard inCloudinaryManager; the regression script checks the security-sensitive behavior directly.Runtime variables
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores