Closed unamdev0 closed 2 weeks ago
β±οΈ Estimated effort to review: 2 π΅π΅βͺβͺβͺ |
π§ͺ No relevant tests |
π Security concerns Sensitive information exposure: The `prodSchema` includes validation for sensitive information such as `DATABASE_URL`, `REDIS_PASSWORD`, `GITHUB_CLIENT_SECRET`, `GOOGLE_CLIENT_SECRET`, `GITLAB_CLIENT_SECRET`, `SMTP_PASSWORD`, and `JWT_SECRET`. While the schema itself doesn't expose these values, it's crucial to ensure that these environment variables are properly protected and not logged or exposed in any part of the application. |
β‘ Key issues to review Inconsistent Validation The `prodSchema` uses `.min(1)` for most string fields, but some fields like `JWT_SECRET` use `.min(3)`. This inconsistency should be reviewed to ensure all minimum lengths are appropriate. Potential Oversight The `devSchema` (formerly `generalSchema`) lacks many of the fields present in `prodSchema`. It should be reviewed to ensure all necessary fields for the development environment are included. |
Category | Suggestion | Score |
Best practice |
Use appropriate data types for numeric environment variables___ **Consider usingz.coerce.number() for numeric environment variables like API_PORT , SMTP_PORT , MINIO_PORT , etc., to automatically convert string inputs to numbers.**
[apps/api/src/common/env/env.schema.ts [96-115]](https://github.com/keyshade-xyz/keyshade/pull/436/files#diff-3ac6831dfb43e9b21b1351465f5eebdf99c9e929c9abacb059cbf16f3d0b8c1dR96-R115)
```diff
-API_PORT: z.string().min(1),
+API_PORT: z.coerce.number().int().positive(),
GOOGLE_CLIENT_ID: z.string().min(1),
GOOGLE_CLIENT_SECRET: z.string().min(1),
GOOGLE_CALLBACK_URL: z.string().min(1),
GITLAB_CLIENT_ID: z.string().min(1),
GITLAB_CLIENT_SECRET: z.string().min(1),
GITLAB_CALLBACK_URL: z.string().min(1),
SENTRY_DSN: z.string().min(1),
SENTRY_ORG: z.string().min(1),
SENTRY_PROJECT: z.string().min(1),
-SENTRY_TRACES_SAMPLE_RATE: z.string().min(1),
-SENTRY_PROFILES_SAMPLE_RATE: z.string().min(1),
+SENTRY_TRACES_SAMPLE_RATE: z.coerce.number().min(0).max(1),
+SENTRY_PROFILES_SAMPLE_RATE: z.coerce.number().min(0).max(1),
SENTRY_ENV: z.string().min(1),
SMTP_HOST: z.string().min(1),
-SMTP_PORT: z.string().min(1),
+SMTP_PORT: z.coerce.number().int().positive(),
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 9Why: Using `z.coerce.number()` for numeric environment variables ensures that inputs are correctly parsed as numbers, improving data integrity and reducing potential runtime errors. | 9 |
Use appropriate data type for boolean environment variables___ **Consider usingz.boolean() for the MINIO_USE_SSL environment variable to ensure it's properly parsed as a boolean value.** [apps/api/src/common/env/env.schema.ts [135]](https://github.com/keyshade-xyz/keyshade/pull/436/files#diff-3ac6831dfb43e9b21b1351465f5eebdf99c9e929c9abacb059cbf16f3d0b8c1dR135-R135) ```diff -MINIO_USE_SSL: z.string().min(1), +MINIO_USE_SSL: z.coerce.boolean(), ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Changing the type to `z.coerce.boolean()` for `MINIO_USE_SSL` ensures that the environment variable is correctly interpreted as a boolean, which is crucial for its intended use. | 9 | |
Enhancement |
Improve the regex pattern for email address validation with display names___ **Consider using a more specific regex pattern forFROM_EMAIL validation to ensure it matches the exact format required for email addresses with display names.** [apps/api/src/common/env/env.schema.ts [117-121]](https://github.com/keyshade-xyz/keyshade/pull/436/files#diff-3ac6831dfb43e9b21b1351465f5eebdf99c9e929c9abacb059cbf16f3d0b8c1dR117-R121) ```diff FROM_EMAIL: z .string() .regex( - /^[a-zA-Z0-9._%+-]+(?: [a-zA-Z0-9._%+-]+)* <[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}>$/ + /^"?([^"<]+)"?\s*<([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,})>$/ ), ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: The improved regex pattern provides a more precise validation for email addresses with display names, enhancing the robustness of the validation logic. | 8 |
Improve validation for URL-related environment variables___ **Consider adding more specific validation for URL-related environment variables usingz.string().url() instead of just z.string().min(1) .**
[apps/api/src/common/env/env.schema.ts [94-104]](https://github.com/keyshade-xyz/keyshade/pull/436/files#diff-3ac6831dfb43e9b21b1351465f5eebdf99c9e929c9abacb059cbf16f3d0b8c1dR94-R104)
```diff
-GITHUB_CALLBACK_URL: z.string().min(1),
+GITHUB_CALLBACK_URL: z.string().url(),
-API_PORT: z.string().min(1),
+API_PORT: z.coerce.number().int().positive(),
GOOGLE_CLIENT_ID: z.string().min(1),
GOOGLE_CLIENT_SECRET: z.string().min(1),
-GOOGLE_CALLBACK_URL: z.string().min(1),
+GOOGLE_CALLBACK_URL: z.string().url(),
GITLAB_CLIENT_ID: z.string().min(1),
GITLAB_CLIENT_SECRET: z.string().min(1),
-GITLAB_CALLBACK_URL: z.string().min(1),
+GITLAB_CALLBACK_URL: z.string().url(),
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 8Why: The suggestion to use `z.string().url()` for URL-related variables enhances validation accuracy, ensuring that only valid URLs are accepted, which is important for application reliability. | 8 |
User description
Description
Added prod env schema in env file
Fixes #357
PR Type
enhancement
Description
prodSchema
to define environment variables for the production environment, ensuring all necessary variables are validated.generalSchema
todevSchema
to better reflect its purpose.EnvSchemaType
to infer from the newly addedprodSchema
.prodSchema
in theEnvSchema
discriminated union to support multiple environment configurations.Changes walkthrough π
env.schema.ts
Add production environment schema and update type inference
apps/api/src/common/env/env.schema.ts
generalSchema
todevSchema
.prodSchema
for production environment variables.EnvSchemaType
to infer fromprodSchema
.prodSchema
in theEnvSchema
discriminated union.