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

mappedResultsQuery interprets "@" symbol in string literal as parameter substitution #57

Closed
tejainece opened this issue Sep 12, 2018 · 16 comments · Fixed by #113
Closed

Comments

@tejainece
Copy link

When I execute the following query:

"SELECT * FROM users WHERE email == '[email protected]'"

mappedResultsQuery excepts with the following exception:

FormatException: Format string specified identifier with name some, but key was not present in values.

@tejainece
Copy link
Author

A solution would be to ignore @[a-zA-Z0-9_]+ occurrences if corresponding parameter is not supplied.

@itsjoeconway
Copy link
Contributor

Is it not possible to parameterize '[email protected]'? What is the use case to providing non-parameterized values in the query string?

I'm not sure I understand the proposed solution; it would just turn off validation that the parameters in the query match the parameters in the substation map. The solution would be to ignore @ inside a string, but then we have to start parsing SQL and validating locally.

@tejainece
Copy link
Author

Is it not possible to parameterize '[email protected]'?

I dont want to parametrize it. I want to deparameterize it. It is not a parameter, if it is in a string.

The solution would be to ignore @ inside a string, but then we have to start parsing SQL and validating locally.

Yes, That would be the ideal option.

Another option is to provide a method to query without prepared parameters. For example: mappedResultsQueryWithoutParams or whatever.

@isoos
Copy link
Collaborator

isoos commented Sep 13, 2018

I dont want to parametrize it. I want to deparameterize it. It is not a parameter, if it is in a string.

I'd rather use a library that enforces parameters, then one that tries to be smart about the strings inside the SQL while parsing it (e.g. you can have different quotes, embedded quotes, embedded escaped inline JSON). The overhead of using parameters is very small, and it keeps security at a much higher standard, while keeping the library small and simple.

Another option is to provide a method to query without prepared parameters. For example: mappedResultsQueryWithoutParams or whatever.

I'd rather keep the interface simple and minimal. It is much easier to create a wrapping proxy for it (e.g. I do that for tracking long-running queries).

@tejainece
Copy link
Author

tejainece commented Sep 13, 2018

I'd rather use ..

I'd rather keep ...

Good for you! But is this what all users of the library like to have?

So, this library will never support self built queries? Isn't this very restricted?

Atleast there should be a way to escape @ that is not intrusive to writing queries.

@isoos
Copy link
Collaborator

isoos commented Sep 13, 2018

there should be a way to escape @ that is not intrusive to writing queries.

+1

@itsjoeconway
Copy link
Contributor

itsjoeconway commented Sep 13, 2018

Escaping sounds like a good solution. \@ sounds reasonable. Can a user still use this string if they need to by double escaping the slash, and then how does this play with Dart raw strings?

@tejainece
Copy link
Author

@joeconwaystk "Escaping" solution has the problems you mentioned. I don't have the most non-intrusive solution. But I will be very grateful if postgres supported it.

@isoos
Copy link
Collaborator

isoos commented Sep 16, 2018

Escaping sounds like a good solution. @ sounds reasonable. Can a user still use this string if they need to by double escaping the slash, and then how does this play with Dart raw strings?

If extrapolation/raw strings are a concern, @@ could work too, but I don't see much issues with \@.

@tejainece
Copy link
Author

\@ is tricky. If you detect @@ and remove one @ before storing it to the DB it would be awesome.

@itsjoeconway
Copy link
Contributor

I don't have the cycles to look at it fully. One thing that elevates this priority of this is request is that JSON operators @> and <@ use this symbol. Is there any concern with using @@ in the format string only to escape substitution variables? Is anyone willing to contribute this as well?

@vlidholt
Copy link

I think there are definitely use cases where you want to build your own queries, without the substitution. Being able to use email addresses without escaping in that case is paramount. I suggest the following fix:
newsvoice@61d7045

Not passing in substitution values will treat the query as a raw query and bypass the substitution.

@TheBosZ
Copy link

TheBosZ commented Oct 17, 2019

In case it helps, I have a use case for this.

I am writing an Aqueduct API. I need to load the database. I see that the best way is to use the migration files's seed method. I run a quick postgres command to list out the rows in my local db as insert statements. I then copy and paste the data into seed as a call to store.execute.

Since my initial data includes email addresses, it blows up because it fails to substitute at @gmail in the first address.

Instead of making me jump through hoops, I would prefer it to not try to substitute if there are no substitutions available.

@PlugFox
Copy link

PlugFox commented Nov 29, 2019

I think there are definitely use cases where you want to build your own queries, without the substitution. Being able to use email addresses without escaping in that case is paramount. I suggest the following fix:
newsvoice@61d7045

Not passing in substitution values will treat the query as a raw query and bypass the substitution.

bump

@isoos
Copy link
Collaborator

isoos commented Nov 30, 2019

I can take ownership on this one. To reiterate, the consensus seems to be the following breaking change:

Previously, when sending a query with @identifierLike string, but not providing substitutionValue for it, we were throwing an exception:
https://github.com/stablekernel/postgresql-dart/blob/master/test/query_test.dart#L363-L382

The change would ignore substituting such strings in the query, and we will NOT throw exception on such occasions, instead, it will pass the query to the server. It is possible that this will not catch errors when building and sending queries, but it will unblock a few classes of legitimate queries. If somebody wants to have extra syntax verification of their query, then they need to do it outside of the postgres library.

The change is breaking in behaviour, but not breaking in API, so there will be only a minor version increase (2.0 -> 2.1).

@isoos
Copy link
Collaborator

isoos commented Nov 30, 2019

#113 should fix this. I'd appreciate if somebody could verify it on their use-case. If nothing is against this change, I'll merge and release this in the next few days.

@isoos isoos closed this as completed in #113 Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants