Marco Bonardo [:mak] Reporter
	Description • 9 years ago

We don't have good support for escaped LIKE parameters in Sqlite.jsm
Usually like parameters should be passed to stmt.escapeStringForLike, but what that method does is very simple (add the escape char before %, _ or the escape char itself).

I think we could just rely on the consumer doing the right thing, but we need to verify he's doing right.
We must verify that any LIKE is followed by ?, : or @ and that it also specifies ESCAPE '/' (or any other char)
For example

value LIKE :token ESCAPE '/'
value LIKE @token ESCAPE '\'
value LIKE ?token ESCAPE '!'

If it's not in this shape we should throw.

A regex should be able to do that.

	Kalpesh Krishna [:martianwars] Assignee
	Comment 1 • 9 years ago

Hey Marco, I'm new to Bugzilla. I came across this bug as a "good-first-bug". Could I be assigned this bug? I do have a fair knowledge of JS / regex.

	Kalpesh Krishna [:martianwars] Assignee
	Comment 2 • 9 years ago

(In reply to Marco Bonardo [::mak] from comment #0)
> We don't have good support for escaped LIKE parameters in Sqlite.jsm
> Usually like parameters should be passed to stmt.escapeStringForLike, but
> what that method does is very simple (add the escape char before %, _ or the
> escape char itself).
> 
> I think we could just rely on the consumer doing the right thing, but we
> need to verify he's doing right.
> We must verify that any LIKE is followed by ?, : or @ and that it also
> specifies ESCAPE '/' (or any other char)
> For example
> 
> value LIKE :token ESCAPE '/'
> value LIKE @token ESCAPE '\'
> value LIKE ?token ESCAPE '!'
> 
> If it's not in this shape we should throw.
> 
> A regex should be able to do that.

Hey Marco, I hope you are back. I've understood the error, but not sure about where to start. I couldn't find any escapeStringForLike in Sqlite.jsm . Where is the existing support for LIKE parameters?

	Marco Bonardo [:mak] Reporter
	Comment 3 • 9 years ago

there is no specific support for LIKE parameters in Sqlite.jsm and we won't add one, here we should just check the passed-in query and throw if the consumer is trying to hardcode a LIKE parameter instead of using binding.
basically execute and executeCached should check the query before creating a statement, a regular expression can be used to check LIKE is followed by one of the binding characters, as explained in comment 0.

	Kalpesh Krishna [:martianwars] Assignee
	Comment 4 • 9 years ago

I didn't fully understand the regex you desired. Do let me know the changes needed.

	Marco Bonardo [:mak] Reporter
	Comment 5 • 9 years ago

Comment on attachment 8655460 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8655460 [details] [diff] [review]:
-----------------------------------------------------------------

Some general hints:
1. it would be nice to move the code to a global helper function, since it's reused in 2 places.
2. similarly, that function could cache the regex in a global var on first use, so there's no need to parse it every time
3. we need to add a test for this to toolkit/modules/tests/xpcshell/test_sqlite.js, should be easy to just pass in invalid SQL to both methods and check the thrown error is as expected. you can run the test using "./mach test toolkit/modules/tests/xpcshell/test_sqlite.js"
4. please use let instead of var, per coding style

::: toolkit/modules/Sqlite.jsm
@@ +1274,5 @@
>     */
>    executeCached: function (sql, params=null, onRow=null) {
> +    var likeExists = /LIKE/i;
> +    if (sql.search(likeExists)!=-1) {
> +      var likePattern = /LIKE.+[@:?].+ESCAPE\s+'\S'/i;

I think a regex like /\bLIKE\b\s(?![@:?])/i could be used once to figure if the query contains a LIKE that is not properly bound. You may test it.

	Kalpesh Krishna [:martianwars] Assignee
	Comment 6 • 9 years ago

I couldn't understand the format of the test javascript file. Did the rest as you mentioned to the best of my ability. Do let me know the further changes

	Marco Bonardo [:mak] Reporter
	Comment 7 • 9 years ago

Comment on attachment 8657228 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8657228 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +1134,5 @@
>    TRANSACTION_EXCLUSIVE: "EXCLUSIVE",
>  
>    TRANSACTION_TYPES: ["DEFERRED", "IMMEDIATE", "EXCLUSIVE"],
>  
> +  validQueryCache: true,

this is not what I meant, what you should cache is the regular expression.

You should basically add a global variable, for example

let likeSqlRegex = /\bLIKE\b\s(?![@:?])/i;

then later just reuse this instead of creating a new regular expression object every time.

@@ +1219,5 @@
>    /**
> +   * Helper function to check whether LIKE is implemented using proper bindings.
> +   *
> +   * @param sql
> +   *        (string) The name of the registered statement to execute.

it's not the name of the statement, it's "The SQL query to be verified."
This also needs a @return

@@ +1221,5 @@
> +   *
> +   * @param sql
> +   *        (string) The name of the registered statement to execute.
> +  */
> +  checkLikeInQuery: function(sql) {

please move this as a simple function in the global scope, it's not an API we should expose. I'd also name it checkBoundLikeQuery

@@ +1223,5 @@
> +   *        (string) The name of the registered statement to execute.
> +  */
> +  checkLikeInQuery: function(sql) {
> +    let badLikePattern = /\bLIKE\b\s(?![@:?])/i;
> +    if (badLikePattern!=-1) {

you should just use .test() on the regular expression and return the negation of its result, cause the regular expression is checking "LIKE not followed by a bound variable" that is what we don't want. the function should return false if the query is invalid, true otherwise.

@@ +1293,5 @@
> +      throw new Error("Please enter a LIKE clause with bindings");
> +    } 
> +    else if (this.checkLikeInQuery(sql)) {
> +      throw new Error("Please enter a LIKE clause with bindings");
> +    }

you just need one check

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +277,5 @@
>  
> +add_task(function* test_incorrect_like_bindings() {
> +  let c = yield getDummyDatabase("incorrect_like_bindings");
> +
> +});

well, you got a database, now you should execute and executeCached an invalid query (containing an unbound LIKE) and verify that those 2 calls will throw an exception.

then remember to close the connection.

	Kalpesh Krishna [:martianwars] Assignee
	Comment 8 • 9 years ago

Did pretty much as you told me to. I hope it is correct this time

	Marco Bonardo [:mak] Reporter
	Comment 9 • 9 years ago

Comment on attachment 8658830 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8658830 [details] [diff] [review]:
-----------------------------------------------------------------

yes, it looks much better, a couple things yet, but nothing critical

::: toolkit/modules/Sqlite.jsm
@@ +70,5 @@
> + *        (string) The SQL query to be verified.
> + * @return boolean value telling us whether query was correct or not
> +*/
> +function checkBoundLikeQuery(sql) {
> +  return !likeSqlRegex.test(sql);

so, after looking at how this is uses, I think it's worth to rename to isInvalidBoundLikeQuery, remove the negation, so later checks will look like 

if (isInvalid...)
  throw...

Sorry for the bouncing, I didn't notice the double negation before...

@@ +1227,5 @@
>    executeBeforeShutdown: function(name, task) {
>      return this._connectionData.executeBeforeShutdown(this, name, task);
>    },
>  
> +

please remove the newline added here since it's not needed

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +277,5 @@
>  
> +add_task(function* test_incorrect_like_bindings() {
> +  let c = yield getDummyDatabase("incorrect_like_bindings");
> +
> +  let sql = "select * from dirs where path like non%";

this would be an invalid query cause the like argument should be wrapped by apices, so please use LIKE 'non%'

@@ +286,5 @@
> +  }
> +  catch (ex if ex.message.startsWith("Please enter a LIKE clause with bindings")) {
> +    success = true;
> +  }
> +  do_check_true(success);

I think you can use  Assert.throws() here to simplify the test, it will automatically try/catch and check the exception, you can find a lot of examples in the codebase
http://mxr.mozilla.org/mozilla-central/search?string=Assert.throws&case=on

(note you won't be able to use yield with it, but doesn't matter in this case cause we expect the exception to fire synchronously)

@@ +296,5 @@
> +  }
> +  catch (ex if ex.message.startsWith("Please enter a LIKE clause with bindings")) {
> +    successCached = true;
> +  }
> +  do_check_true(successCached);

ditto

	Marco Bonardo [:mak] Reporter
	Comment 10 • 9 years ago

or even isUnboundLikeQuery could be a valid name (shorter too). whatever you prefer.

	Kalpesh Krishna [:martianwars] Assignee
	Comment 11 • 9 years ago

Added the things you told me to.

	Marco Bonardo [:mak] Reporter
	Comment 12 • 9 years ago

Comment on attachment 8660299 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8660299 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +37,5 @@
>                                    "resource://gre/modules/PromiseUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "console",
>                                    "resource://gre/modules/devtools/Console.jsm");
>  
> +// Regular expression used by checkBoundLikeQuery

comment needs updated function name

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +277,5 @@
>  
> +add_task(function* test_incorrect_like_bindings() {
> +  let c = yield getDummyDatabase("incorrect_like_bindings");
> +
> +  let sql = "select * from dirs where path LIKE non%";

please wrap non% in single apices

@@ +280,5 @@
> +
> +  let sql = "select * from dirs where path LIKE non%";
> +  Assert.throws(() => c.execute(sql),
> +                /Please enter a LIKE clause/);
> +  

trailing spaces

@@ +286,5 @@
> +                /Please enter a LIKE clause/);
> +
> +  yield c.close();
> +
> +});

please remove the newline before });

	Kalpesh Krishna [:martianwars] Assignee
	Comment 13 • 9 years ago

I hope it can be pushed now.

	Marco Bonardo [:mak] Reporter
	Comment 14 • 9 years ago

Comment on attachment 8661462 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8661462 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I pushed your patch to Try to very tests. Once done if tests did pass, a tree sheriff will merge your patch to the main repository.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=258e7e808e29

	Kalpesh Krishna [:martianwars] Assignee
	Comment 15 • 9 years ago

Finally :) @mak - I really enjoyed the process. What should be my next step? Should I fix some more bugs? Would it be possible to start contributing actively somewhere?

	Marco Bonardo [:mak] Reporter
	Comment 16 • 9 years ago

The previous push didn't work for some infrastructure reason, here's another one
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ef31646aa5f

(In reply to Kalpesh Krishna from comment #15)
> Finally :) @mak - I really enjoyed the process. What should be my next step?
> Should I fix some more bugs? Would it be possible to start contributing
> actively somewhere?

I'd suggest to look into the list of mentored bugs for something more involving, when you feel like you're good enough for bigger tasks, you may try to look for "diamond" bugs or select an area of the product you're interested into and ask the owner for things to do in that area.
See https://developer.mozilla.org/en-US/docs/Introduction#Find_a_bug_we%27ve_identified_as_a_good_fit_for_new_contributors.

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago

Hi, this didn't apply cleanly to fx-team:

applying sqlite_fix.patch
patching file toolkit/modules/Sqlite.jsm
Hunk #1 FAILED at 32
Hunk #2 succeeded at 57 with fuzz 2 (offset 0 lines).
1 out of 4 hunks FAILED -- saving rejects to file toolkit/modules/Sqlite.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh sqlite_fix.patch

could you take a look? Thanks!

	Marco Bonardo [:mak] Reporter
	Comment 18 • 9 years ago

looks like due to bug 1202902, and looks like from now on we should use var in the global object... I wonder if this change was announced somewhere and which kind of protection we have to avoid reintroducing the problem.

Btw, you likely just need to merge the patch on top of the current tree, and use let when you define something (like the regex) in the global object.

	Marco Bonardo [:mak] Reporter
	Comment 19 • 9 years ago

(In reply to Marco Bonardo [::mak] from comment #18)
>  and use let when you define something (like the regex) in the global object.

ehr, I meant "var", use var when defining in the global scope.

	Kalpesh Krishna [:martianwars] Assignee
	Comment 20 • 9 years ago

So I just change the regex type to "var"?

	Kalpesh Krishna [:martianwars] Assignee
	Comment 21 • 9 years ago

changing let to var

	Marco Bonardo [:mak] Reporter
	Comment 22 • 9 years ago

Comment on attachment 8662346 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8662346 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +44,3 @@
>  // Counts the number of created connections per database basename(). This is
>  // used for logging to distinguish connection instances.
>  let connectionCounters = new Map();

did you merge the patch on top of the current tree? doesn't look like...

	Kalpesh Krishna [:martianwars] Assignee
	Comment 23 • 9 years ago

I hope this will do.

	Marco Bonardo [:mak] Reporter
	Comment 24 • 9 years ago

Comment on attachment 8662421 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8662421 [details] [diff] [review]:
-----------------------------------------------------------------

There's still a small bitrot, but it's fur to my delay, so I'll take care of that.

	Marco Bonardo [:mak] Reporter
	Comment 25 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a279a50e4bd

	Marco Bonardo [:mak] Reporter
	Comment 26 • 9 years ago

thank you for your contribution, feel free to look around for more bugs to fix.

	Pulsebot
	Comment 27 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/5aeeb7979d4f

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 28 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/5aeeb7979d4f