Skip to content
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

Wrong queries causes unhandled exceptions #161

Closed
regevbr opened this issue Nov 19, 2019 · 8 comments · Fixed by #178
Closed

Wrong queries causes unhandled exceptions #161

regevbr opened this issue Nov 19, 2019 · 8 comments · Fixed by #178

Comments

@regevbr
Copy link

regevbr commented Nov 19, 2019

Steps to reproduce

Run a select query (all()) with order by on a bogus field that includes ? char in it
https://github.com/strongloop/loopback-connector/blob/deb461ea62e6226f053774fb10c1f023d6061f8c/lib/sql.js#L1455-L1459

Current Behavior

an unhandled exception caused by the buildSelect() method due to ParameterizedSQL class assertion error:

https://github.com/strongloop/loopback-connector/blob/deb461ea62e6226f053774fb10c1f023d6061f8c/lib/parameterized-sql.js#L38-L42

Which in turn means that the callback is never called, causing connections to hang

Expected Behavior

cb() should be called with the proper error

@regevbr regevbr added the bug label Nov 19, 2019
regevbr added a commit to PruvoNet/loopback-connector that referenced this issue Nov 19, 2019
@regevbr
Copy link
Author

regevbr commented Nov 19, 2019

quick and dirty solution is to tyr/catch the call to buildSelect
A more correct solution will be to sanitize the fields used for order by, like done in fields selection

@dhmlau
Copy link
Member

dhmlau commented Nov 19, 2019

@regevbr , would you like to submit a PR? Thanks.

@regevbr
Copy link
Author

regevbr commented Nov 19, 2019

@dhmlau sure, which solution would you prefer?

@jannyHou
Copy link
Contributor

@regevbr Thank you for willing to submit a patch, I think you can start with try catch the error for buildSelect to at least reports a proper error instead of the app hangs.
Then you can add a new test reproduce the failure and add the sanitization for buildSelect, maybe in a separate PR.
Let us know if you have any questions.

@regevbr
Copy link
Author

regevbr commented Nov 19, 2019

Sounds good!

regevbr added a commit to regevbr/loopback-connector that referenced this issue Nov 20, 2019
regevbr added a commit to regevbr/loopback-connector that referenced this issue Nov 20, 2019
regevbr added a commit to regevbr/loopback-connector that referenced this issue Nov 20, 2019
regevbr added a commit to regevbr/loopback-connector that referenced this issue Nov 26, 2019
regevbr added a commit to regevbr/loopback-connector that referenced this issue Nov 26, 2019
regevbr added a commit to regevbr/loopback-connector that referenced this issue Nov 29, 2019
regevbr added a commit to regevbr/loopback-connector that referenced this issue Nov 29, 2019
regevbr added a commit to regevbr/loopback-connector that referenced this issue Nov 29, 2019
regevbr added a commit to regevbr/loopback-connector that referenced this issue Dec 23, 2019
@stale
Copy link

stale bot commented Jan 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 19, 2020
@stale
Copy link

stale bot commented Feb 2, 2020

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this as completed Feb 2, 2020
@ewrayjohnson
Copy link
Contributor

ewrayjohnson commented Jul 13, 2020

I have encountered this error and have a unique perspective on the cause. In my case one of the parameters is a very large Buffer. The assert statement always appends the parameters even if the assertion is true and the parameters are never displayed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants