Harden SMS provider configuration#96
Open
jamilahmadzai wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughSMSComponent configuration is hardened by converting hard-coded credentials to environment-driven public properties with validation. The main config wires environment variables, a static validation script prevents credential leaks, and documentation describes the required setup. ChangesSMS Provider Configuration Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 2 documents with suggested updates are ready for review. BAWES Universe DB, API endpoint, S3, Slack, and payment keysView Suggested Changes@@ -42,6 +42,14 @@
MYFATOORAH_KUWAIT_TEST_API_KEY=your_kuwait_test_api_key
MYFATOORAH_SAUDI_LIVE_API_KEY=your_saudi_live_api_key
+# --- SMS Provider Configuration ---
+# The SMS component requires all four credentials to function.
+# The provider endpoint MUST use HTTPS; HTTP endpoints are rejected.
+SMS_PROVIDER_ENDPOINT=https://your.sms.provider/api
+SMS_PROVIDER_USERNAME=your_sms_username
+SMS_PROVIDER_PASSWORD=your_sms_password
+SMS_PROVIDER_SENDER=YourSenderName
+
# --- Optional: Other Configuration Keys ---
# The following keys are present in main.php but are not required for most local setups.
# Uncomment and set as needed.
@@ -59,7 +67,6 @@
# Additional optional components:
# ARMADA_DELIVERY_KEY=your_armada_delivery_key
-# SMS_COMPONENT_KEY=your_sms_component_key
# FILE_GENERATOR_KEY=your_file_generator_key
# MASHKOR_DELIVERY_KEY=your_mashkor_delivery_key
# AUTH0_CLIENT_ID=your_auth0_client_id
@@ -68,6 +75,7 @@
**Notes:**
- Replace all `your_*` values with your actual credentials or secrets.
-- The S3, Slack, and payment keys are directly mapped from the structure in [`common/config/main.php`](https://github.com/BAWES-Universe/plugn/blob/bc485b0a1da61d516955c5dc4fc29e95afccea92/common/config/main.php#L3-L172).
+- The S3, Slack, payment, and SMS provider keys are directly mapped from the structure in [`common/config/main.php`](https://github.com/BAWES-Universe/plugn/blob/bc485b0a1da61d516955c5dc4fc29e95afccea92/common/config/main.php#L3-L172).
+- **SMS Provider:** All four SMS_PROVIDER_* variables are required. The component will fail to initialize if any are missing or empty. The endpoint must use HTTPS; non-HTTPS endpoints are rejected.
- Database and API endpoint keys are placeholders, as they are not defined in `main.php` and may be set elsewhere in your project.
- All other configuration keys in `main.php` are optional and can be set if your local environment requires them.StudentHub – Complete Services & Infrastructure Map (Org-wide)”View Suggested Changes@@ -12,6 +12,7 @@
| Mixpanel | Analytics | Tracks user events and analytics | backend, admin portal, frontend portals | [studenthub/docs/analytics.md](https://github.com/BAWES-Universe/studenthub/blob/ca90a5502040ba8191fe9f80d29d0b6a4d2a6152/docs/analytics.md#L3-L41) | MIXPANEL_KEY (configurable in admin) | N/A |
| Segment | Analytics | Aggregates analytics events and forwards to destinations | backend, admin portal, frontend portals | [studenthub/docs/analytics.md](https://github.com/BAWES-Universe/studenthub/blob/ca90a5502040ba8191fe9f80d29d0b6a4d2a6152/docs/analytics.md#L3-L41) | SEGMENT_KEY (configurable in admin) | N/A |
| Puppeteer | Other | Headless browser automation for backend tasks | backend | [studenthub/docs/setup.md](https://github.com/BAWES-Universe/studenthub/blob/ca90a5502040ba8191fe9f80d29d0b6a4d2a6152/docs/setup.md#L5-L59) | N/A | N/A |
+| SMS Provider | Messaging | Sends password-reset and notification SMS messages | backend | [studenthub/common/components/SMSComponent.php](https://github.com/BAWES-Universe/studenthub/blob/main/common/components/SMSComponent.php), [studenthub/common/config/main.php](https://github.com/BAWES-Universe/studenthub/blob/main/common/config/main.php) | SMS_PROVIDER_ENDPOINT, SMS_PROVIDER_USERNAME, SMS_PROVIDER_PASSWORD, SMS_PROVIDER_SENDER | HTTPS endpoint (provider-specific) |
## Hosting & Environments
@@ -27,5 +28,5 @@
- No evidence was found for DNS/CDN/security services such as Cloudflare, Route53, or WAF. These may be configured outside of code, for example in AWS or Netlify dashboards. Check "AWS Route53 Hosted Zones" or "Netlify Site Settings → Domain Management" for DNS/CDN configuration.
- No explicit deployment or hosting configuration was found for the candidate or company portals. Check "Netlify Site Settings", "Vercel Project Settings", or "AWS S3 Buckets" for these portals.
-- No evidence of authentication, email, SMS, payments, monitoring, maps, queue, media, or support services in code or configs. These may be configured in third-party dashboards or as environment variables not present in the repositories. Check "Environment Variable dashboards" in Netlify, Railway, or AWS, and review admin portal configuration screens for integrations.
+- No evidence of authentication, email, payments, monitoring, maps, queue, media, or support services in code or configs. These may be configured in third-party dashboards or as environment variables not present in the repositories. Check "Environment Variable dashboards" in Netlify, Railway, or AWS, and review admin portal configuration screens for integrations.
- Some environment variables and secrets are referenced only by name (e.g., in CircleCI contexts or Netlify build configs) and their actual values or usage may be managed in CI/CD or hosting dashboards. Check "CircleCI Contexts → org-global", "Netlify Site Settings → Environment Variables", and "Railway Variables" for full lists and values.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 (4)
scripts/check-sms-provider-hardening.py (1)
19-20: 💤 Low valueConsider adding file existence checks for clearer error messages.
If the target files are missing,
read_text()will raise aFileNotFoundErrorwith a traceback. Adding explicit existence checks would provide clearer error messages when the repository structure is unexpected.♻️ Proposed improvement
+if not SMS_COMPONENT.exists(): + fail(f"SMSComponent not found at {SMS_COMPONENT}") +if not MAIN_CONFIG.exists(): + fail(f"Configuration file not found at {MAIN_CONFIG}") + component = SMS_COMPONENT.read_text() config = MAIN_CONFIG.read_text()🤖 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-sms-provider-hardening.py` around lines 19 - 20, The current calls to SMS_COMPONENT.read_text() and MAIN_CONFIG.read_text() can raise FileNotFoundError with a traceback; add explicit existence checks using SMS_COMPONENT.exists() and MAIN_CONFIG.exists() before calling read_text(), and if either is missing raise or log a clear, contextual error (e.g., raise FileNotFoundError or call sys.exit with a message like "Missing required file: <symbol> — expected at repository root") so the script fails with a concise, actionable message referencing the missing SMS_COMPONENT or MAIN_CONFIG.docs/setup.md (1)
65-65: 💤 Low valueConsider more general phrasing for component usage.
The documentation refers to "password-reset SMS component," but the component is named
SMSComponentand could potentially be used for other SMS purposes beyond password resets. Unless the component is exclusively used for password resets, consider more general phrasing like "SMS component."📝 Suggested rewording
-The password-reset SMS component reads provider settings from environment variables. Do not commit provider usernames, passwords, sender accounts, or private provider URLs. +The SMS component reads provider settings from environment variables. Do not commit provider usernames, passwords, sender accounts, or private provider URLs.🤖 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 `@docs/setup.md` at line 65, The docs currently call this the "password-reset SMS component" but the implementation is named SMSComponent and can be used for other SMS purposes; update the wording to a more general term (e.g., "SMS component" or "SMSComponent") throughout the sentence/section to avoid implying exclusivity, and ensure any examples or warnings about environment variables still refer to SMSComponent so readers can map docs to the code.common/components/SMSComponent.php (1)
45-52: ⚡ Quick winConsider trimming validated properties to avoid confusing error messages.
The validation checks that
trim($this->$attribute) === '', but doesn't actually trim the value. If an environment variable contains leading or trailing whitespace (e.g.,SMS_PROVIDER_ENDPOINT=" https://example.com "), it will pass the empty check but fail the HTTPS check at line 54 with "must use HTTPS", even though the real issue is whitespace, not the lack of HTTPS.♻️ Proposed fix to trim validated values
foreach (['apiEndpoint', 'username', 'password', 'sender'] as $attribute) { if (!is_string($this->$attribute) || trim($this->$attribute) === '') { throw new InvalidConfigException(strtr('"{class}::{attribute}" cannot be empty.', [ '{class}' => static::class, '{attribute}' => '$' . $attribute ])); } + $this->$attribute = trim($this->$attribute); }🤖 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 `@common/components/SMSComponent.php` around lines 45 - 52, Validation currently checks trim($this->$attribute) but doesn't modify the property, causing later checks (e.g. HTTPS check) to see untrimmed values; inside the foreach in SMSComponent that iterates ['apiEndpoint','username','password','sender'], trim and reassign each string property (e.g. $this->$attribute = trim((string)$this->$attribute)) before the empty check and then proceed to throw InvalidConfigException as before so subsequent validations operate on the trimmed values.common/config/main.php (1)
11-12: Consider extending credential hardening to other components.While this PR correctly hardens the SMS provider configuration, several other components in this file contain hardcoded credentials and API keys:
temporaryBucketResourceManager: AWS key and secret (lines 11-12)reCaptcha: secret key (line 31)jira: API token (line 47)algolia: API key (line 52)ipstack: access key (line 58)cloudinaryManager: API key and secret (lines 63-64)slack: webhook URL (line 77)These credentials should follow the same environment-variable pattern used for the SMS provider to prevent credential leaks in version control.
Also applies to: 31-31, 47-47, 52-52, 58-58, 63-64, 77-77
🤖 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 `@common/config/main.php` around lines 11 - 12, Replace all hardcoded credentials in this config with environment-driven values like the SMS provider does: for temporaryBucketResourceManager (replace 'key' and 'secret' values), reCaptcha (replace 'secretKey'), jira (replace 'apiToken'), algolia (replace 'apiKey'), ipstack (replace 'accessKey'), cloudinaryManager (replace API 'api_key' and 'api_secret'), and slack (replace 'webhookUrl') — use the same env lookup pattern/function used for the SMS config to read each secret from process environment (or your config helper) and provide sensible default/failure behavior where appropriate.
🤖 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 `@common/components/SMSComponent.php`:
- Around line 45-52: Validation currently checks trim($this->$attribute) but
doesn't modify the property, causing later checks (e.g. HTTPS check) to see
untrimmed values; inside the foreach in SMSComponent that iterates
['apiEndpoint','username','password','sender'], trim and reassign each string
property (e.g. $this->$attribute = trim((string)$this->$attribute)) before the
empty check and then proceed to throw InvalidConfigException as before so
subsequent validations operate on the trimmed values.
In `@common/config/main.php`:
- Around line 11-12: Replace all hardcoded credentials in this config with
environment-driven values like the SMS provider does: for
temporaryBucketResourceManager (replace 'key' and 'secret' values), reCaptcha
(replace 'secretKey'), jira (replace 'apiToken'), algolia (replace 'apiKey'),
ipstack (replace 'accessKey'), cloudinaryManager (replace API 'api_key' and
'api_secret'), and slack (replace 'webhookUrl') — use the same env lookup
pattern/function used for the SMS config to read each secret from process
environment (or your config helper) and provide sensible default/failure
behavior where appropriate.
In `@docs/setup.md`:
- Line 65: The docs currently call this the "password-reset SMS component" but
the implementation is named SMSComponent and can be used for other SMS purposes;
update the wording to a more general term (e.g., "SMS component" or
"SMSComponent") throughout the sentence/section to avoid implying exclusivity,
and ensure any examples or warnings about environment variables still refer to
SMSComponent so readers can map docs to the code.
In `@scripts/check-sms-provider-hardening.py`:
- Around line 19-20: The current calls to SMS_COMPONENT.read_text() and
MAIN_CONFIG.read_text() can raise FileNotFoundError with a traceback; add
explicit existence checks using SMS_COMPONENT.exists() and MAIN_CONFIG.exists()
before calling read_text(), and if either is missing raise or log a clear,
contextual error (e.g., raise FileNotFoundError or call sys.exit with a message
like "Missing required file: <symbol> — expected at repository root") so the
script fails with a concise, actionable message referencing the missing
SMS_COMPONENT or MAIN_CONFIG.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88c9cbf8-552b-435a-9618-d97ba688a598
📒 Files selected for processing (4)
common/components/SMSComponent.phpcommon/config/main.phpdocs/setup.mdscripts/check-sms-provider-hardening.py
BAWES
approved these changes
Jun 9, 2026
BAWES
left a comment
BAWES
left a comment
Collaborator
There was a problem hiding this comment.
✅ Approved — SMS provider credential hardening
Review: Clean refactoring of SMSComponent. All checks green.
- Removes hardcoded provider URL and credentials (
usrbawes/bawes1452) - Makes all provider settings configurable via public properties
- Enforces HTTPS on endpoint in
init() - Fails closed (throws
InvalidConfigException) on missing config - Sets 10s timeout on HTTP client
- Wires
getenv()calls in config - Adds docs and validation script
Note: language defaults to L which matches the previous hardcoded value — good backward compatibility.
GitGuardian ✅ | CodeRabbit ✅
Collaborator
|
🔴 Merge blocked — source branch deleted The source branch Suggested next steps:
@cto — please advise on how to proceed with this bounty. |
jamilahmadzai
force-pushed
the
security/sms-provider-hardening-55
branch
from
01474d6 to
a3a5b00
Compare
Author
|
Restored the source branch and rebased it onto current The conflict from #98 is resolved by keeping the Cloudinary runtime configuration docs from Validation rerun: python3 scripts/check-sms-provider-hardening.py
python3 -m py_compile scripts/check-sms-provider-hardening.py
git diff --check upstream/master...HEAD |
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
2 participants
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 SMS provider credential and transport hardening slice. It removes the checked-in SMS provider username, password, sender, and hardcoded provider endpoint from
SMSComponent, wires the component through runtime environment variables, fails closed when required provider settings are missing, and rejects non-HTTPS provider endpoints so credentials are not posted over plaintext HTTP.It also documents the required variables and adds a static regression guard to prevent the SMS provider literals from being reintroduced.
Safety Boundary
No live SMS provider account, live AWS/IAM/S3 access, candidate data, or private production data was accessed. This PR does not include secret values.
Demo
Privacy-safe validation demo: https://github.com/jamilahmadzai/studenthub/releases/download/studenthub-55-sms-provider-demo-20260516/studenthub-55-sms-provider-demo.mp4
Verification
Full SMS delivery smoke testing was not run because real provider credentials and endpoint values are intentionally not present in the local environment.
Summary by CodeRabbit
Bug Fixes
Documentation
Chores
Payout
Payment method: Algora bounty-platform payout to GitHub user @jamilahmadzai for issue #55.