fix: sql injection vuln #2871
fix: sql injection vuln #2871
Conversation
ogzhanolguncu
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes update two functions that construct query parameters by dynamically generating unique parameter names based on the index of each provided path. In both Changes
Sequence Diagram(s)Sequence Diagram for
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/clickhouse/src/logs.ts (3)
47-49: Use a more descriptive name or inline comment forparamSchemaExtension.
Currently, it’s clear that this object is meant to store additional zod parameters for dynamic paths. However, a more explicit naming convention or an inline comment could improve readability for future maintainers.
60-66: Check wildcard usage inlikeclause.
When using thecontainsoperator withlike(path, CONCAT('%', {paramName: String}, '%')), ensure that if a user includes wildcard symbols (%,_), the query matches the intended scope. You might need to escape special characters if you want a literal “contains”.
300-306: Similar concern with wildcard usage for timeseries paths.
As ingetLogs, consider whethercontainsrequires escaping special characters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/clickhouse/src/logs.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
internal/clickhouse/src/logs.ts (13)
50-50: Good practice adding a dedicated comment.
Documenting the reason for dynamic path parameterization makes the code more maintainable and self-explanatory.
53-57: Prevent potential misuse of theoperatorproperty.
This logic effectively enumerates known operators. Ensure no unrecognized operator can slip through if validation changes in the future. Consider making the operator enumeration more robust or adding a default-case handling that throws an error if it’s invalid.
74-74: Excellent approach extending the existing Zod schema.
This helps ensure that any newly introduced parameters are strictly validated, preventing potential injection vectors.
167-167: Ensure the correct schema is applied.
UsingextendedParamsSchemahere is crucial to validate the dynamic path parameters properly. Good job referencing the extended schema.
171-171: Handle consistent error responses.
This function now merges original arguments and extended parameters before passing them into the query. Consider how errors or type mismatches are propagated if the extended schema fails validation.
253-253: Return type clarification.
Returning{ whereClause: string; paramSchema: z.ZodType<any> }is explicit and helps with usage clarity. Just ensure the rest of the codebase references this updated return signature properly.
289-290: Keep an eye on potential naming collisions.
Because we’re extending the schema with new parameter names, verify that future parameters won’t inadvertently overwrite existing keys inlogsTimeseriesParams.
292-292: Clear commentary enhances maintainability.
Documenting the path filter logic provides valuable context for other developers.
294-297: Consistently define param schemas and assigned values.
Similar togetLogs, eachparamNameis accurately mapped to its pathvalue. This keeps the code consistent and safe from injection.
314-316: Good pattern for returning bothwhereClauseandparamSchema.
This separation of concerns makes the function more testable and easier to maintain.
321-321: LeverageadditionalConditionsfor more flexibility.
By passing extra conditions togetLogsTimeseriesWhereClause, you maintain a cleaner separation between standard filters and context-specific ones. Nicely done.
326-337: Consolidate parameter merging logic.
Merging path parameters intoargsis consistent with the approach ingetLogs. This helps keep function usage uniform. Make sure any future expansions (e.g., new param types) follow the same pattern to avoid confusion.
342-344: Verify Zod schema usage in final query.
Specifyingparams: paramSchemaand then calling with(parameters)is a robust pattern for preventing malformed inputs. Just ensure that any future expansions to the schema are also accounted for inparameters.
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit