fix: sonar security hotspots (https, --ignore-scripts, api_key, math.random) #5538
Conversation
|
@vijayraghav-io is attempting to deploy a commit to the formbricks Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
Warning Rate limit exceeded@mattinannt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughThis set of changes introduces several updates across multiple files. In the Dockerfile for the web application, all global npm install commands and the pnpm install command now include the Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/Dockerfile(4 hunks)apps/web/lib/constants.ts(1 hunks)apps/web/lib/env.ts(2 hunks)apps/web/lib/survey/tests/__mock__/survey.mock.ts(1 hunks)apps/web/lib/telemetry.ts(1 hunks)apps/web/modules/survey/editor/components/open-question-form.tsx(1 hunks)packages/js-core/src/lib/common/tests/utils.test.ts(1 hunks)packages/js-co 8000 re/src/lib/common/utils.ts(1 hunks)packages/surveys/src/lib/utils.ts(1 hunks)turbo.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/js-core/src/lib/common/tests/utils.test.ts (1)
packages/js-core/src/lib/common/utils.ts (1)
shouldDisplayBasedOnPercentage(186-192)
apps/web/lib/constants.ts (1)
apps/web/lib/env.ts (1)
env(4-230)
🔇 Additional comments (10)
turbo.json (1)
197-197: Secure handling of telemetry API keyAdding the
TELEMETRY_API_KEYto the environment variables list in the build task is a good security practice as it avoids hardcoding sensitive credentials.apps/web/lib/env.ts (2)
104-104: Good security practice for API key handlingAdding the telemetry API key to the server environment schema as an optional string type is appropriate and consistent with how other API keys are handled in this file.
217-217: Consistent implementation of environment variableThe runtime environment mapping is correctly implemented to match the schema definition, ensuring the API key is properly loaded from process.env.
packages/js-core/src/lib/common/utils.ts (1)
187-190: Security enhancement: Replace Math.random with cryptographically secure randomGreat security improvement! Replacing
Math.random()withcrypto.getRandomValues()addresses a security hotspot by using a cryptographically secure random number generator, which is significantly more secure for any feature that depends on unpredictability.You've implemented this correctly by:
- Creating a Uint32Array
- Using crypto.getRandomValues to fill it
- Normalizing to a [0,1) range
- Properly scaling to match the previous behavior
packages/surveys/src/lib/utils.ts (1)
19-27: Security enhancement: Improve shuffle with cryptographically secure randomnessExcellent update to the shuffle function! This change addresses two important aspects:
- Security: Replacing
Math.random()withcrypto.getRandomValues()for cryptographically secure randomness- Algorithm quality: Implementing the Fisher-Yates shuffle algorithm correctly by iterating backward
This makes the shuffle both more secure and more efficient.
packages/js-core/src/lib/common/tests/utils.test.ts (1)
402-419: Great security improvement by using cryptographically secure random number generation.You've correctly updated the test to mock
crypto.getRandomValuesinstead ofMath.random, which aligns with the corresponding implementation change in the utility function. Using cryptographically secure random number generation is a security best practice, especially for any code that might influence decision-making or sampling.apps/web/modules/survey/editor/components/open-question-form.tsx (1)
235-235: Good security update by using HTTPS in the URL placeholder.Switching from HTTP to HTTPS in the placeholder example promotes secure practices. This change aligns with modern web security standards where HTTPS should be preferred over HTTP.
apps/web/lib/survey/tests/__mock__/survey.mock.ts (1)
205-205: Security improvement - updated to HTTPS URL.Good job updating the mock URL to use HTTPS instead of HTTP. This change is consistent with security best practices and the other changes in this PR addressing Sonar security hotspots.
apps/web/lib/constants.ts (1)
84-84: Secure handling for SMTP port 465This change improves security by automatically enabling secure mode when SMTP port 465 is used, which is the standard port for SMTP over SSL. Previously, secure mode would only be enabled with an explicit environment variable setting, potentially allowing insecure communication on a port that should always be secured.
This fix directly addresses the SonarQube security hotspot mentioned in the PR objectives.
apps/web/Dockerfile (1)
21-21: Addition of--ignore-scriptsflag improves securityAdding the
--ignore-scriptsflag to all npm and pnpm install commands is an excellent security improvement. This prevents potentially malicious code in pre/post-install scripts from executing during package installation, reducing the attack surface of the application during build time.This systematically addresses the SonarQube security hotspot related to npm script execution mentioned in the PR objectives.
Also applies to: 62-62, 78-78, 144-144
There was a problem hiding this comment.
Pull Request Overview
This PR addresses several sonar security hotspots by replacing insecure methods and hardcoded values with secure counterparts. Key changes include:
- Refactoring random number generation in utility functions to use cryptographically secure methods.
- Updating URLs from "http" to "https" and removing hardcoded telemetry API keys.
- Adjusting SMTP secure enabling logic based on the SMTP port value.
Reviewed Changes
Copilot reviewed 8 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/surveys/src/lib/utils.ts | Updated the shuffle algorithm to use crypto-based secure randomness with a Fisher-Yates approach. |
| packages/js-core/src/lib/common/utils.ts | Replaced Math.random with crypto-secure random generation in percentage-based display logic. |
| packages/js-core/src/lib/common/tests/utils.test.ts | Modified tests to mock crypto.getRandomValues, aligning them with the updated randomness approach. |
| apps/web/modules/survey/editor/components/open-question-form.tsx | Changed placeholder URL to use "https". |
| apps/web/lib/telemetry.ts | Replaced hardcoded telemetry API key with an environment variable. |
| apps/web/lib/survey/tests/mock/survey.mock.ts | Updated redirect URL to use "https". |
| apps/web/lib/env.ts | Added TELEMETRY_API_KEY configuration in the environment schema. |
| apps/web/lib/constants.ts | Enhanced SMTP secure flag logic to enable secure mode when SMTP_PORT is "465". |
Files not reviewed (4)
- .env.example: Language not supported
- apps/web/Dockerfile: Language not supported
- docs/self-hosting/configuration/environment-variables.mdx: Language not supported
- turbo.json: Language not supported
|
@vijayraghav-io thanks a lot for the PR 😊💪 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.env.example(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
.env.example
193-193: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run Linters / Linters
- GitHub Check: Run E2E Tests / Run E2E Tests
- GitHub Check: Run Unit Tests / Unit Tests
- GitHub Check: SonarQube
- GitHub Check: Validate Docker Build
Sure, Thank you 🙏 , updated telemetry api key (sonar is not flagging for .env files) |
What does this PR do?
Fixes #(issue)
Fixes following sonar security hotspots
1.
The below hotspot is addressed by adding a condition that SMTP_SECURE_ENABLED will be true if SMTP_PORT = '465'
How should this be tested?
Tests updated
Run Sonar scanner
Checklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores