Harden OneSignal notification config#91
Open
MAJINSI wants to merge 2 commits into
Open
Conversation
68 tasks
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis pull request secures OneSignal notification delivery by migrating hardcoded credentials to environment variables, adding strict parameter validation and error handling in the notification model, enabling TLS peer verification, and enforcing the configuration pattern through documentation and automated tests. ChangesOneSignal Credential Security & TLS Verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/models/MobileNotification.php (1)
87-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit cURL timeouts to prevent request-thread hangs on network stalls.
The OneSignal API call at lines 87-96 has no connect or request timeout configured. If the OneSignal service becomes unresponsive or the network stalls, the request will block indefinitely.
curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE); curl_setopt($ch, CURLOPT_HEADER, FALSE); curl_setopt($ch, CURLOPT_POST, TRUE); curl_setopt($ch, CURLOPT_POSTFIELDS, $fields); + curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 5); + curl_setopt($ch, CURLOPT_TIMEOUT, 15); curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, TRUE);🤖 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/models/MobileNotification.php` around lines 87 - 96, The cURL call in MobileNotification (where $ch is initialized and configured for the OneSignal API) lacks timeouts and can hang; add explicit CURLOPT_CONNECTTIMEOUT (e.g., 5 seconds) and CURLOPT_TIMEOUT (e.g., 30 seconds) via curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, ...) and curl_setopt($ch, CURLOPT_TIMEOUT, ...) alongside the existing curl_setopt calls so the request will fail fast on network stalls and not block the thread indefinitely.
🤖 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.
Inline comments:
In `@common/models/MobileNotification.php`:
- Around line 29-32: The early return in notifyCandidate() prevents test-mode
bypass in sendNotification() when OneSignal config is missing; change the check
so it only returns false when OneSignal params are missing AND test-mode is NOT
active. Update the condition around Yii::$app->params['oneSignalCandidateAPPID']
/ ['oneSignalCandidateAPIKey'] to allow execution to continue when
Yii::$app->params['inCodeception'] (or equivalent test flag) is truthy so
sendNotification() can perform its Codeception bypass; ensure you reference
notifyCandidate() and sendNotification() when making the change.
- Around line 98-109: The code in MobileNotification that currently treats any
non-false curl_exec result as success should validate the HTTP response code:
after $response = curl_exec($ch) (and after the existing transport error check),
call curl_getinfo($ch, CURLINFO_HTTP_CODE) to get $status; if $status is not in
the 2xx range, log a warning with both $status and the $response (e.g. via
Yii::warning) and return false; ensure curl_close($ch) is called before
returning and keep the existing successful return true path when $status is
200–299.
In `@tests/check-onesignal-env-config.sh`:
- Line 12: Update the grep regex used in the test that currently searches for
the literal "CURLOPT_SSL_VERIFYPEER,\s*FALSE" so it matches any case variant of
the PHP boolean (e.g. false, False). Replace the pattern with a case-insensitive
match (for example add the (?i) inline flag or change "FALSE" to
"[Ff][Aa][Ll][Ss][Ee]") in the rg invocation that contains
"CURLOPT_SSL_VERIFYPEER,\\s*FALSE" so the test will catch all casings in
MobileNotification.php.
- Around line 9-10: The test currently checks globally for
getenv('ONESIGNAL_CANDIDATE_APP_ID') and getenv('ONESIGNAL_CANDIDATE_API_KEY')
and can miss missing occurrences in individual params-local.php files; update
tests/check-onesignal-env-config.sh to assert that every params-local.php under
environments/ contains those getenv calls (e.g., loop over each file found by
find environments -name params-local.php or use rg --glob
'environments/**/params-local.php' to search only those files) and fail if any
params-local.php is missing either getenv('ONESIGNAL_CANDIDATE_APP_ID') or
getenv('ONESIGNAL_CANDIDATE_API_KEY').
---
Outside diff comments:
In `@common/models/MobileNotification.php`:
- Around line 87-96: The cURL call in MobileNotification (where $ch is
initialized and configured for the OneSignal API) lacks timeouts and can hang;
add explicit CURLOPT_CONNECTTIMEOUT (e.g., 5 seconds) and CURLOPT_TIMEOUT (e.g.,
30 seconds) via curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, ...) and
curl_setopt($ch, CURLOPT_TIMEOUT, ...) alongside the existing curl_setopt calls
so the request will fail fast on network stalls and not block the thread
indefinitely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 003be1e6-8ebb-4687-b300-cf34b71a5ae8
⛔ Files ignored due to path filters (1)
docs/demo/studenthub-55-onesignal-hardening-demo.mp4is excluded by!**/*.mp4
📒 Files selected for processing (15)
common/models/MobileNotification.phpdocs/onesignal-env.mdenvironments/circle-ci/common/config/params-local.phpenvironments/dev-server-nginx-debug/common/config/params-local.phpenvironments/dev-server-nginx/common/config/params-local.phpenvironments/dev-server-railway/common/config/params-local.phpenvironments/dev-server/common/config/params-local.phpenvironments/dev/common/config/params-local.phpenvironments/docker/common/config/params-local.phpenvironments/krushn-nginx/common/config/params-local.phpenvironments/krushn/common/config/params-local.phpenvironments/prod-nginx/common/config/params-local.phpenvironments/prod-railway/common/config/params-local.phpenvironments/prod/common/config/params-local.phptests/check-onesignal-env-config.sh
Author
|
Addressed CodeRabbit feedback in 1d3f4b0:
Verification rerun:
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Summary
/claim #55
Demo
Verification
tests/check-onesignal-env-config.shgit diff --checkphp -l common/models/MobileNotification.phpcould not be run locally because this environment does not havephpinstalledScope
This is a narrow OneSignal notification credential/transport hardening slice. It does not touch AWS/IAM/S3 keys, bucket policies, Civil ID backend/UI flows, Cloudinary, SQS/MediaConvert/SES/Xero/service-token/Jira/Algolia/Google/Slack slices, live OneSignal access, candidate data, or real private secret values.
Transparency: AI-assisted with OpenAI Codex; I reviewed the diff and verification before submitting.
Summary by CodeRabbit
Security & Configuration Improvements
Documentation
Tests