fix(convex): add type safety and security improvements - #6
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile OverviewGreptile SummaryImproved type safety across Convex functions by replacing
Security concern: Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant ConvexAPI
participant InternalMutations
participant Database
Note over Client,Database: Type-Safe Query Flow
Client->>ConvexAPI: query with typed args
ConvexAPI->>ConvexAPI: Validate args with v.object()
ConvexAPI->>Database: Query data
Database->>ConvexAPI: Return raw data
ConvexAPI->>ConvexAPI: Validate return with validator
ConvexAPI->>Client: Return typed response
Note over Client,Database: Secure Destructive Operations
Client->>ConvexAPI: ❌ Cannot call deleteAll directly
Note over ConvexAPI: Internal mutations only
Client->>ConvexAPI: mutation api.markets.upsert
ConvexAPI->>ConvexAPI: Validate typed evidence.metrics
ConvexAPI->>Database: Insert with typed payload
Database->>ConvexAPI: Success
ConvexAPI->>Client: Return typed ID
Note over Client,Database: Admin Operations via Internal
ConvexAPI->>InternalMutations: internal.accounts.deleteAll
InternalMutations->>Database: Delete all records
Database->>InternalMutations: Return { deleted: number }
InternalMutations->>ConvexAPI: Typed return value
|
| // Delete all markets (for cleanup) | ||
| // Delete all markets (admin operation) | ||
| // TODO: Add authentication check when auth is implemented | ||
| export const deleteAll = mutation({ |
There was a problem hiding this comment.
deleteAll still exposed as regular mutation allowing any client to delete all markets. The API route at src/app/api/markets/sync/route.ts:140 is already public, so this doesn't add auth protection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/markets.ts
Line: 52:52
Comment:
`deleteAll` still exposed as regular `mutation` allowing any client to delete all markets. The API route at `src/app/api/markets/sync/route.ts:140` is already public, so this doesn't add auth protection.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
This PR tightens Convex API contracts by adding return validators, replacing v.any() with structured validators in the schema, and restricting destructive operations to internal-only mutations (with one exception).
Changes:
- Converted multiple destructive mutations to
internalMutationand added explicitreturnsvalidators across Convex functions. - Replaced
v.any()with typed validators foralerts.evidence.metrics,agentRuns.toolCalls.input/output, andactivityFeed.payload. - Updated cron scheduling to use
crons.cron()and added{}args to all scheduled calls for consistency.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| convex/seed.ts | Makes seeding internal-only and adds a typed return contract. |
| convex/schema.ts | Replaces v.any() fields with typed schema validators for stronger runtime guarantees. |
| convex/markets.ts | Adds typed return validators; keeps deleteAll public with a TODO for auth. |
| convex/crons.ts | Migrates from deprecated crons.daily() to crons.cron() and standardizes cron args. |
| convex/alerts.ts | Adds typed validators and converts destructive ops to internalMutation. |
| convex/agentRuns.ts | Adds typed validators/returns and internalizes scheduled trigger mutation. |
| convex/activityFeed.ts | Adds typed validators and explicit null return for cleanup mutation. |
| convex/accounts.ts | Adds typed return validators and converts destructive ops to internalMutation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const triggerScheduledRun = internalMutation({ | ||
| args: {}, | ||
| returns: v.null(), | ||
| handler: async (ctx) => { |
There was a problem hiding this comment.
triggerScheduledRun declares returns: v.null(), but inside the handler there is a return; (undefined) path when there are no active markets. This will violate the return validator at runtime; return null consistently in that branch.
| args: {}, | ||
| returns: v.object({ deleted: v.number() }), | ||
| handler: async (ctx) => { |
There was a problem hiding this comment.
markets.deleteAll is still a public mutation with no authorization check, which means any client with access to the Convex deployment can delete all markets. If this must remain callable from a Next.js API route, add an explicit auth/secret check in the mutation (or gate it behind an admin-only identity) rather than leaving it as an open operation.
| args: {}, | |
| returns: v.object({ deleted: v.number() }), | |
| handler: async (ctx) => { | |
| args: { adminSecret: v.string() }, | |
| returns: v.object({ deleted: v.number() }), | |
| handler: async (ctx, { adminSecret }) => { | |
| const expectedSecret = process.env.ADMIN_SECRET; | |
| if (!expectedSecret || adminSecret !== expectedSecret) { | |
| throw new Error("Unauthorized: invalid admin secret"); | |
| } |
fix(convex): add type safety and security improvements
Summary
deleteAll,removeDuplicates,deleteByAddress,seedInsiderCases) to internal mutations (exceptmarkets.deleteAllwhich is used by API routes)returnsvalidators to all Convex functions for better API contractsv.any()with typed validators in schema and functions:evidence.metricsnow has typed fieldstoolCalls.input/outputnow have typed fieldsactivityFeed.payloadnow has typed fieldscrons.daily()helper → usecrons.cron()with cron expression{}to allcrons.interval()calls for consistencySecurity Improvements
markets.deleteAllfor future auth implementationTest plan