Closed (fixed)
Project:
Salesforce Suite
Version:
8.x-3.x-dev
Component:
Code
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
23 Jan 2019 at 16:58 UTC
Updated:
14 Feb 2019 at 17:39 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
acrosmanPatch switches to skipping types that do not need wrapping in ' and therefore will trigger errors if one is included. New version checks for ISO dates, all numbers, and boolean values, and escapes and wraps other values.
I do have a custom use case where user entered values end up in a where clause, so without the escape there is an injection possibility. Nothing in the suite appears to do this by default.
Comment #3
aaronbaumanah, right, i forgot about dates too.
SOQL is the worst, though, seriously.
What about something like this, which exposes a flag to enable/disable escaping the value?
That regex looks gnarly.
Comment #4
acrosmanI thought about a similar approach when I wrote the initial patch, and normally I hate complex regular expressions, but since SOQL only has the three data types that do not get wrapped in single quotes (numbers, boolean, and date), and all three are detectable it is probably more reliable to do it automatically.
Granted the other reason I didn't use this approach initially was I wasn't sure where all the inbound calls were in the suite which you have a better sense of. If it's just one or two that would need tweaking the parameter isn't as scary.
Comment #5
aaronbaumanyeah, there's only the 2 calls within the module, which will take advantage of the new parameter.
for contrib using this method, the new parameter defaults to using the existing behavior.
if the behavior was recently broken, then the new parameter gives them the opportunity to fix their implementations.
for my own implementations, i almost never use this method.
Would love to tear this all out and start fresh, per #2879844: Update SelectQuery interface to closely resemble core database API.
Comment #6
chrisolofI'm finding that with the new escaping mechanism I can no longer add string where conditions. If I include the single-quotes around my string value, as SelectQuery::addCondition() still says is needed ("...Note that the caller must enclose the value in quotes as needed by the SF API."), the single quotes get escaped and I get no results back. If I remove the single-quotes then I get a "MALFORMED_QUERY" back because my string is not enclosed in single-quotes.
I also found that this new escaping code, as written, breaks IN conditions (see #3029608: MALFORMED_QUERY when using IN condition).
This is pretty major for those of us building atop the SelectQuery API.
Stepping back a bit, it seems that the use of SelectQueryBase::escapeSoqlValue() in SelectQuery::__toString() would only ever be helpful if you willfully violated the API doc and didn't single-quote your condition's string, and - by really an off-chance here - added one or more single-quotes into the middle of your string somewhere. In this one, odd situation the correct single-quotes would get escaped and your string would get wrapped up in the necessary single-quotes for you (which you would not expect to happen if you read the doc on SelectQuery::addCondition()). Maybe I'm missing something but if that's the only use-case here then I'd suggest we take this out.
The attached patch removes SelectQueryBase::escapeSoqlValue() and instead updates the API doc to inform users that it is their responsibility to escape single-quotes in the values they supply to SelectQuery::addCondition().
Comment #7
chrisolofComment #9
aaronbaumanYeah, i think it probably makes sense to back it out at this point.
Let's get something more robust into 4.x
If we want to continue pursuing this for 3.x, let's create a new class - e.g. SelectQuerySanitized or SelectQuerySafe - so that we can preserve full BC without the headache.