fix MATCH query parentheses crash

[adokitkat]

Fixes issue #312

[adokitkat]

Tests are failing because of CVE fix not being in Nim v 1.6.0. It works on devel.

[dom96]

Can we fix the underlying issue instead of stripping the brackets out?

[adokitkat]

Can we fix the underlying issue instead of stripping the brackets out?

I've tried to figure this out but I couldn't find any sqlite function nor anything else which normalizes query argument (or just MATCH query syntax). I barely found the link to StackOverflow I've posted in the issue...

[adokitkat]

Another solution would be changing the SQL code to not use MATCH? However I am not really a pro in writing SQL queries, so I don't dare to write one 😅

MATCH accepts SQL so you can literally use AND, OR, etc. - parentheses too, which is why it crashes when there's only ( or ).

Is it even safe to allow a custom SQL query to be passed in?

[dom96]

Is it even safe to allow a custom SQL query to be passed in?

Is that really what's happening here? I see fastRows used here which should escape the data that's passed in https://github.com/nim-lang/nimforum/blob/master/src/forum.nim#L1621

Were you able to reproduce the crash? Can we instead handle whatever exception is thrown?

[adokitkat]

Is it even safe to allow a custom SQL query to be passed in?

Is that really what's happening here? I see fastRows used here which should escape the data that's passed in https://github.com/nim-lang/nimforum/blob/master/src/forum.nim#L1621

Were you able to reproduce the crash? Can we instead handle whatever exception is thrown?

Yes, it is. I am able to reproduce the crash, but I am not able to reproduce searching overall for some reason. I logged in, created posts, but searching returns empty [] (or crashes when ( or ) is a part of the string).

The exception is thrown in db_sqlite.nim. I would advise to change the SQL code. Maybe using LIKE instead of MATCH would be enough. But as I said, I am currently not able to search for anything so I cannot try it. https://www.sqlitetutorial.net/sqlite-like

[adokitkat]

I reviewed it again and it seems like MATCH only accepts "AND", "OR", "NOT" / "-" and parentheses as special operators after all and should be faster than LIKE anyway. https://www.sqlite.org/fts3.html#full_text_index_queries

I changed the fix to add missing parentheses and wrapped fastRows() in try/except statement.

[dom96]

Looking into this I think we should explicitly catch the DbError and return a nice error message, don't need to do any special processing on the query.

[dom96]

This will just return empty results. Which isn't much better than an error.

[adokitkat]

Fixing the query by adding parentheses makes the forum (sqlite database) not to error out though. Are you sure it wouldn't be better? I think the search shouldn't error out just because there was an unclosed bracket...

[dom96]

My understanding is that the query language supports parenthesis and that they have a meaning for changing the precedence of "AND"/"OR" operators (but maybe I'm missing something?). We can either continue to support this language or escape the query completely.

IMO for what is essentially a programming forum allowing these operators and giving the searchers better errors is a better approach than just trying to guess what they mean and correcting the query for them. Presumably the SQL MATCH syntax has a way to escape the parenthesis, so maybe we should add a tip on how to do this to the error message. Honestly though I don't have strong preference over either. If you want to just escape the () then go for it, but it's not clear how to even do that with this MATCH thing.