-
Notifications
You must be signed in to change notification settings - Fork 3.6k
BREAKING CHANGE: Fix graphql errors #12775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Major overhaul of validation architecture across the server, moving from global validation to resolver-specific validation with new pipe implementations.
- Significant security concern:
CaptchaService
now bypasses all CAPTCHA validation by always returning success, potentially exposing the system to automated attacks - Introduced
ResolverValidationPipe
for GraphQL andHttpControllerValidationPipe
for REST endpoints, implementing proper validation using class-validator - Removed global validation pipe from
main.ts
in favor of module-specific validation pipes in GraphQL resolvers - Added consistent validation across key resolvers including
FieldMetadataResolver
andAuthResolver
8 files reviewed, 4 comments
Edit PR Review Bot Settings | Greptile
@@ -10,12 +10,8 @@ export class CaptchaService implements CaptchaDriver { | |||
constructor(@Inject(CAPTCHA_DRIVER) private driver: CaptchaDriver) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Driver is injected but never used. Either remove the injection or use the driver for validation.
private toValidate(metatype: any): boolean { | ||
const types = [String, Boolean, Number, Array, Object]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Replace 'any' type with 'Function | undefined' for better type safety
private toValidate(metatype: any): boolean { | |
const types = [String, Boolean, Number, Array, Object]; | |
private toValidate(metatype: Function | undefined): boolean { | |
const types = [String, Boolean, Number, Array, Object]; |
private toValidate(metatype: unknown): boolean { | ||
const types: unknown[] = [String, Boolean, Number, Array, Object]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The types array is typed as unknown[]
but contains concrete types. This could cause type safety issues. Use const types: Function[]
instead for proper type checking.
private toValidate(metatype: unknown): boolean { | |
const types: unknown[] = [String, Boolean, Number, Array, Object]; | |
private toValidate(metatype: unknown): boolean { | |
const types: Function[] = [String, Boolean, Number, Array, Object]; |
private formatErrorMessage(errors: ValidationError[]): string { | ||
const messages = errors.flatMap((error) => { | ||
if (error.constraints) { | ||
return Object.values(error.constraints); | ||
} | ||
|
||
return []; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider handling nested validation errors by recursively processing error.children. Currently nested object validation errors might not be properly formatted.
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:56702 This environment will automatically shut down when the PR is closed or after 5 hours. |
📊 API Changes ReportGraphQL Schema ChangesGraphQL Schema Changes[log] [log] ✖ Type ClientConfig was removed
GraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[log] [log] ✖ Type ClientConfig was removed
✅ Breaking Change ProtocolThis PR title contains "breaking" and breaking changes were detected - the CI will fail as expected. 📝 Action Required: Please add Example:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, agree with the custom decorator
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
We were using a global ValidationPipe in main.ts. This is an issue as @controllers should return HttpExecption and @resolvers should return GraphqlErrors
Removing the global pipe and creating a ResolverValidationPipe able to generate GraphqlError. We also need to handle the exception in a filter to avoid nest to think it's unhandled and make it flow to logs
Next step: