-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix: Add validation for invalid date query parameter values #3330
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
var calledDone = false | ||
|
||
client.query(new pg.Query({ text: 'SELECT $1::timestamp', values: [new Date(undefined)] }), function (err, res) { | ||
if (!calledDone) { | ||
calledDone = true | ||
assert.equal(err.message, 'Query parameter value cannot be an invalid date.') | ||
client.end(done) | ||
} | ||
}) |
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.
The query
callback can only be called once (and if it weren’t, we wouldn’t want to silently ignore that):
var calledDone = false | |
client.query(new pg.Query({ text: 'SELECT $1::timestamp', values: [new Date(undefined)] }), function (err, res) { | |
if (!calledDone) { | |
calledDone = true | |
assert.equal(err.message, 'Query parameter value cannot be an invalid date.') | |
client.end(done) | |
} | |
}) | |
client.query(new pg.Query({ text: 'SELECT $1::timestamp', values: [new Date(undefined)] }), function (err, res) { | |
assert.equal(err.message, 'Query parameter value cannot be an invalid date.') | |
client.end(done) | |
}) |
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.
It will cause this error, as the connection also gets terminated, and the callback will be executed again.
Message: 'Connection terminated' == 'Query parameter value cannot be an invalid date.'
AssertionError [ERR_ASSERTION]: 'Connection terminated' == 'Query parameter value cannot be an invalid date.
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.
That’s a serious bug, then.
@@ -257,3 +257,18 @@ suite.test('cannot pass non-string values to query as text', (done) => { | |||
}) | |||
}) | |||
}) | |||
|
|||
if (!helper.args.native) { |
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.
What happens in the 'native' case?
hmmm...I'm not sure we should be in the business of validating the date passed to the library isn't invalid. The backend already does this w/ an informative error saying the date is bad. If this resulted in insert garbage or doing something unexpected I'd be more concerned, but this already results in an error. |
@brianc The way it fails is with this cryptic date string. If you're intent on sending it, you can serialise it as "InvalidDate" or something like that, so that the cause would be clearer. |
PR for Issue #3318
Issue Description:
Currently, new Date(undefined) (an invalid date) is serialized as "0NaN-NaN-NaNTNaN:NaN:NaN.NaN+NaN:NaN". It should fail early with a JS error instead of being passed to the database.