-
Notifications
You must be signed in to change notification settings - Fork 22
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
Adds json schema parsing check during body validation #214
Conversation
9af94fa
to
2f6b181
Compare
2f6b181
to
ecab140
Compare
@kylef, could you please review it once more?
|
7342a7a
to
c2ede2d
Compare
lib/units/validateBody.js
Outdated
*/ | ||
const validateJsonSchema = (schema) => { | ||
try { | ||
JSON.parse(schema); |
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.
I think at other places in Gavel we use json-parse-helpfulerror
to provide even more helpful error messages than what JSON.parse()
provides
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.
I was looking for how the message body was done, didn't find it with JSON.parse
.
src/mixins/validatable-http-message.coffee:jph = require 'json-parse-helpfulerror'
src/mixins/validatable-http-message.coffee: jph.parse @body
src/mixins/validatable-http-message.coffee: jph.parse @body
src/mixins/validatable-http-message.coffee: parsed = jph.parse @expected.bodySchema
src/mixins/validatable-http-message.coffee: jph.parse @expected.body
src/mixins/validatable-http-message.coffee: jph.parse @expected.body
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.
While looking over the changes yesterday I've spotted that we already have a part of the body validation that attempts to parse a JSON Schema:
gavel.js/lib/units/validateBody.js
Lines 102 to 109 in bba8206
try { | |
jph.parse(bodySchema); | |
return [null, jsonSchemaType]; | |
} catch (exception) { | |
const error = `Can't validate: expected body JSON Schema is not a parseable JSON:\n${exception.message}`; | |
return [error, null]; | |
} |
Which means that mentioned issue scenario somehow doesn't trigger this logic. I would need to investigate. Hopefully, we don't need any new functions/checks, just need to fix the old ones.
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.
So, it looks like with the current founding we must treat malformed JSON Schemas like an exception. Shall we change this to an exception then?
const error = `Can't validate: expected body JSON Schema is not a parseable JSON:\n${exception.message}`;
return [error, null];
At the moment it's being tolerated as a validation error, not stopping the process, and ending up in the public results.
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.
I confirm that changing that soft handling to throwing an exception produces the same behavior:
- Stops Gavel validation to prevent operating on malformed data
- Propagates user-friendly error + JSON parsing error from
jph
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.
I think exception is all right. If Gavel is given schema, the user's intention is to validate according to it. If Gavel cannot parse it, then it shouldn't be silent or just warning. There's no way for Gavel to continue in that case, otherwise it could lead to false positives in validation.
🎉 This PR is included in version 6.1.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Performs given JSON Schema parsing during body validation to prevent processing of invalid or malformed JSON Schema. Note that this performs no actual validation either a JSON Schema is a valid JSON Schema.
GitHub
"
in JSON values are not escaped correctly drafter#719