Closed YuvalBubnovsky closed 1 month ago
Here are some key observations to aid the review process:
β±οΈ Estimated effort to review: 2 π΅π΅βͺβͺβͺ |
π§ͺ No relevant tests |
π No security concerns identified |
β‘ Key issues to review Validation Improvement The validation for SENTRY_TRACES_SAMPLE_RATE and SENTRY_PROFILES_SAMPLE_RATE could be simplified by using z.coerce.number() instead of parsing the string manually. Error Handling The error caught during Sentry initialization is logged and captured, but the application continues to run. Consider whether this is the desired behavior or if the application should exit on Sentry initialization failure. |
Explore these optional code suggestions:
Category | Suggestion | Score |
Best practice |
Validate parsed sample rates before using them in configuration___ **Consider adding a check to ensure that the parsed sample rates are within the validrange (0 to 1) before using them in the Sentry configuration.** [apps/api/src/main.ts [35-48]](https://github.com/keyshade-xyz/keyshade/pull/470/files#diff-32c5294dd7ca131397ef57a87c3e0cf38ea5c2e4ca0061c602e73f7f00514c1aR35-R48) ```diff -const tracesSampleRate = +const validateSampleRate = (rate: number): number => { + return rate >= 0 && rate <= 1 ? rate : 1.0 +} + +const tracesSampleRate = validateSampleRate( parseFloat(process.env.SENTRY_TRACES_SAMPLE_RATE) || 1.0 -const profilesSampleRate = +) +const profilesSampleRate = validateSampleRate( parseFloat(process.env.SENTRY_PROFILES_SAMPLE_RATE) || 1.0 +) Sentry.init({ dsn: process.env.SENTRY_DSN, enabled: sentryEnv !== 'test' && sentryEnv !== 'e2e', environment: sentryEnv, tracesSampleRate, profilesSampleRate, integrations: [nodeProfilingIntegration()], debug: sentryEnv.startsWith('dev') }) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Ensuring that parsed sample rates are within the valid range before using them in configuration is crucial for maintaining the integrity of the Sentry setup, preventing potential misconfigurations. | 9 |
Maintainability |
Extract common validation logic into a separate function___ **Consider extracting the common validation logic forSENTRY_TRACES_SAMPLE_RATE and SENTRY_PROFILES_SAMPLE_RATE into a separate function to reduce code duplication and improve maintainability.** [apps/api/src/common/env/env.schema.ts [53-80]](https://github.com/keyshade-xyz/keyshade/pull/470/files#diff-3ac6831dfb43e9b21b1351465f5eebdf99c9e929c9abacb059cbf16f3d0b8c1dR53-R80) ```diff +const validateSampleRate = (val: string | undefined) => { + if (val === undefined) return true + const num = parseFloat(val) + return !isNaN(num) && num >= 0 && num <= 1 +} + SENTRY_TRACES_SAMPLE_RATE: z .string() .optional() - .refine( - (val) => { - if (val === undefined) return true - const num = parseFloat(val) - return !isNaN(num) && num >= 0 && num <= 1 - }, - { - message: - 'SENTRY_TRACES_SAMPLE_RATE must be a string representing a number between 0 and 1' - } - ), + .refine(validateSampleRate, { + message: 'SENTRY_TRACES_SAMPLE_RATE must be a string representing a number between 0 and 1' + }), SENTRY_PROFILES_SAMPLE_RATE: z .string() .optional() - .refine( - (val) => { - if (val === undefined) return true - const num = parseFloat(val) - return !isNaN(num) && num >= 0 && num <= 1 - }, - { - message: - 'SENTRY_PROFILES_SAMPLE_RATE must be a string representing a number between 0 and 1' - } - ), + .refine(validateSampleRate, { + message: 'SENTRY_PROFILES_SAMPLE_RATE must be a string representing a number between 0 and 1' + }), ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Extracting common validation logic into a separate function reduces code duplication and improves maintainability, making the codebase easier to manage and extend in the future. | 8 |
Enhancement |
Add a custom error message for URL validation___ **Consider using a custom error message for the URL validation ofSENTRY_DSN . This will provide more specific feedback if an invalid URL is provided.** [apps/api/src/common/env/env.schema.ts [50]](https://github.com/keyshade-xyz/keyshade/pull/470/files#diff-3ac6831dfb43e9b21b1351465f5eebdf99c9e929c9abacb059cbf16f3d0b8c1dR50-R50) ```diff -SENTRY_DSN: z.string().url().optional(), +SENTRY_DSN: z.string().url({ message: "SENTRY_DSN must be a valid URL" }).optional(), ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Adding a custom error message for URL validation enhances user feedback by providing specific guidance when an invalid URL is provided, improving the usability of the validation schema. | 7 |
Error handling |
Add error handling for parsing sample rates___ **Consider using a try-catch block when parsing the sample rates to handle potentialparsing errors gracefully.** [apps/api/src/main.ts [35-38]](https://github.com/keyshade-xyz/keyshade/pull/470/files#diff-32c5294dd7ca131397ef57a87c3e0cf38ea5c2e4ca0061c602e73f7f00514c1aR35-R38) ```diff -const tracesSampleRate = - parseFloat(process.env.SENTRY_TRACES_SAMPLE_RATE) || 1.0 -const profilesSampleRate = - parseFloat(process.env.SENTRY_PROFILES_SAMPLE_RATE) || 1.0 +let tracesSampleRate = 1.0 +let profilesSampleRate = 1.0 +try { + tracesSampleRate = parseFloat(process.env.SENTRY_TRACES_SAMPLE_RATE) || 1.0 + profilesSampleRate = parseFloat(process.env.SENTRY_PROFILES_SAMPLE_RATE) || 1.0 +} catch (error) { + logger.warn(`Error parsing sample rates: ${error.message}. Using default values.`) +} ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6Why: Adding a try-catch block for parsing sample rates introduces error handling, which can prevent unexpected crashes due to parsing errors, although the likelihood of such errors is low given the context. | 6 |
π‘ Need additional feedback ? start a PR chat
PR Code Suggestions β¨
Explore these optional code suggestions: Category Suggestion Score Best practice
Validate parsed sample rates before using them in configuration9 Maintainability
Extract common validation logic into a separate function8 Enhancement
Add a custom error message for URL validation7 Error handling
Add error handling for parsing sample rates6
π‘ Need additional feedback ? start a PR chat
I fixed number 2 & 3 here, the other remarks the bot makes are irrelevant and are handles by us having the input validation inside the env.schema file and having the validation logic in a separate function.
:tada: This PR is included in version 2.6.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:
User description
Added input range validation for Sentry env vars
Description
Give a summary of the change that you have made
Fixes #145
Dependencies
No new deps introduced.
Future Improvements
None at the moment.
Mentions
@rajdip-b
Screenshots of relevant screens
N/A
Developer's checklist
If changes are made in the code:
Documentation Update
PR Type
enhancement, bug_fix
Description
SENTRY_DSN
and range validation forSENTRY_TRACES_SAMPLE_RATE
andSENTRY_PROFILES_SAMPLE_RATE
in environment schema.ProfilingIntegration
tonodeProfilingIntegration
.Changes walkthrough π
env.schema.ts
Add validation for Sentry environment variables
apps/api/src/common/env/env.schema.ts
SENTRY_DSN
.SENTRY_TRACES_SAMPLE_RATE
andSENTRY_PROFILES_SAMPLE_RATE
.main.ts
Enhance Sentry initialization with logging and error handling
apps/api/src/main.ts
ProfilingIntegration
.