-
Notifications
You must be signed in to change notification settings - Fork 150
More validation for query parameters #354
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
f493c15
to
278248a
Compare
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.
Changes look good to me. Only added a couple of comments.
src/v1/internal/util.js
Outdated
} | ||
|
||
function assertQueryParameters(obj) { | ||
if (!isObject(obj) && Boolean(obj)) { |
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.
isObject(obj)
already includes the condition Boolean(obj)
, maybe just remove && Boolean(obj)
here?
src/v1/internal/util.js
Outdated
@@ -39,8 +39,29 @@ function isEmptyObjectOrNull(obj) { | |||
} | |||
|
|||
function isObject(obj) { | |||
const type = typeof obj; | |||
return type === 'function' || type === 'object' && Boolean(obj); | |||
return typeof obj === 'object' && !Array.isArray(obj) && Boolean(obj); |
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 don't know an immediate replacement but not sure if Boolean(obj)
is the correct thing to check here. Here are a couple of console tests;
isObject(new Date())
returns true
isObject(new Number())
returns true
Do you think we can have a better way of checking for actual object types?
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 will replace Boolean(obj)
with obj !== null
which is much less cryptic.
Technically, Date
and Number
are objects so maybe it's okay to accept them. We should probably support parameters given like this:
class PersonParams {
constructor(name, age) {
this.name = name;
this.age = age;
}
}
session.run('CREATE (:Peson {name: $name, age: $age})', new PersonParams('Bob', 42))
and that is why I'm not sure how to allow these but disallow Date
and Number
.
278248a
to
b0b418b
Compare
Driver expects query parameters to either be undefined/null or an object. This was previously not enforced and illegal parameters, like strings or arrays, were sent to the database. This resulted in a protocol violation and database closed the connection. Users were only able to see the actual error/stacktrace in the database logs. Driver received a `ServiceUnavailable` or `SessionExpired` error. This commit adds validation of query parameters. It also prohibits nodes, relationships and paths from being used as query parameters in the driver.
b0b418b
to
40f5258
Compare
Driver expects query parameters to either be undefined/null or an object. This was previously not enforced and illegal parameters, like strings or arrays, were sent to the database. This resulted in a protocol violation and database closed the connection. Users were only able to see the actual error/stacktrace in the database logs. Driver received a
ServiceUnavailable
orSessionExpired
error.This PR adds validation of query parameters. It also prohibits nodes, relationships, and paths from being used as query parameters in the driver.
Resolves #340