Closed muntaxir4 closed 1 week ago
Here are some key observations to aid the review process:
**๐ซ Ticket compliance analysis โ ** **[519](https://github.com/keyshade-xyz/keyshade/issues/519) - Fully compliant** Compliant requirements: - Created integration folder with schema and types files - Added zod schemas for integration base type, requests and responses - Added type exports in integration/index.types.ts - Updated api-client to use types from schema package |
โฑ๏ธ Estimated effort to review: 2 ๐ต๐ตโชโชโช |
๐งช PR contains tests |
๐ No security concerns identified |
โก Recommended focus areas for review Schema Validation Verify that the metadata field validation in IntegrationSchema is sufficient for all integration types, as it currently accepts any string key-value pairs Type Safety The UpdateIntegrationRequestSchema makes all fields optional which could lead to empty update requests. Consider adding validation for minimum required fields |
Explore these optional code suggestions:
Category | Suggestion | Score |
Security |
Move sensitive data from URL query parameters to request body___ **Pass the OTP value in the request body instead of as a query parameter to avoidexposing sensitive data in URLs and server logs.** [packages/api-client/src/controllers/user.ts [48]](https://github.com/keyshade-xyz/keyshade/pull/547/files#diff-b9f547168be56e5f084cf20416e152e4848786e0f045e613fb7aad029679a5f9R48-R48) ```diff const response = await this.apiClient.post( - `/api/user/validate-email-change-otp?otp=${request.otp}`, - request, + '/api/user/validate-email-change-otp', + { ...request, otp: request.otp }, headers ) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Moving OTP from URL query parameters to request body is a critical security improvement, as sensitive data in URLs can be exposed in logs and browser history. | 9 |
General |
Add validation to prevent empty string values in metadata records___ **Add validation to ensure metadata values are not empty strings, as empty metadatavalues could cause issues with integrations.** [packages/schema/src/integration/index.ts [12]](https://github.com/keyshade-xyz/keyshade/pull/547/files#diff-e0fe8ad7da9f8755287c52f6456e99ec592811149268339e0ae16fdb426beaaeR12-R12) ```diff export const IntegrationSchema = z.object({ - metadata: z.record(z.string()), + metadata: z.record(z.string().min(1)), }) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Adding validation to prevent empty metadata values is important for data integrity and preventing potential integration issues. Empty strings could cause problems with integration functionality. | 7 |
๐ก Need additional feedback ? start a PR chat
User description
Description
@keyshade/schema
integration
.integration
schemas.index
and its types fromindex.types
.@keyshade/api-client
integration.types.d.ts
.@keyshade/schema
.Related to #519
Developer's checklist
If changes are made in the code:
Documentation Update
PR Type
enhancement, tests
Description
@keyshade/schema
package.api-client
to import integration types from@keyshade/schema
.integration.types.d.ts
file fromapi-client
.schema
package.ProjectSchema
to use a base schema for consistency.Changes walkthrough ๐
integration.ts
Update integration type imports to use schema package
packages/api-client/src/controllers/integration.ts
@keyshade/schema
.integration.types.d.ts
Remove deprecated integration types file
packages/api-client/src/types/integration.types.d.ts
@keyshade/schema
.index.types.ts
Export integration types from new module
packages/schema/src/index.types.ts - Exported integration types from the new integration module.
index.ts
Add integration schemas for requests and responses
packages/schema/src/integration/index.ts
index.types.ts
Add type definitions for integration schemas
packages/schema/src/integration/index.types.ts - Added type definitions inferred from integration schemas.
index.ts
Refactor ProjectSchema to use BaseProjectSchema
packages/schema/src/project/index.ts - Refactored `ProjectSchema` to use `BaseProjectSchema`.
user.ts
Correct URL path in validateEmailChangeOTP method
packages/api-client/src/controllers/user.ts - Fixed URL path in `validateEmailChangeOTP` method.
integration.spec.ts
Add tests for integration schemas and types
packages/schema/tests/integration.spec.ts