MapBot: Bugfix : Fix issue caused by converting SQL insert queries to f-strings without proper variable conversion

Description Changes made to the queries in databaseconnect.py where insert queries’ strings were converted to f-strings are causing SQL failures in certain cases.

Example

INSERT INTO question_table(subject,root_word,verb,sentence) VALUES ('['distance']','what','[]','what is the distance between Nagpur and MUmbai?')

The variables are lists of strings and are thus appearing as lists in the query which is then truncated as a string at '['. This needs to be fixed in such a way that queries run in all cases.

Additionally, since the variables are a list, the cases where the length is more than one need to be handled in a better way. However, this wasn’t present in the first version itself and is more of an enhancement than a bugfix. @vishakha-lall I think that should be taken up in another issue.

Pre-requisite Intermediate knowledge of Python

About this issue

  • Original URL
  • State: closed
  • Created 4 years ago
  • Comments: 28 (24 by maintainers)

Commits related to this issue

Most upvoted comments

long_data_used

Seeing the above word in the error I get an instinct that you may not be converting each argument to string explicitly. I think that needs to be done. Maybe, that’ll solve it.

Not totally a wild guess though. If you browse the history of the file databseconnect.py you’ll see typecasting existed before a PR was merged. Look here @Pihu1998.

The discussion is here and the doubts existed here.

+2
chttrjeankr on Mar 21, 2020

@Pihu1998 Thanks for these references, this makes it quite clear why any sort of string formatting would be vulnerable to attacks. Please go ahead and make the changes using the sql module’s functions.

+2
janakrajchadha on Mar 18, 2020

Hi @vishakha-lall , I am not able to resolve this issue using f-strings as this issue is due to SQL-injection and f-strings are highly vulnerable to it. Some of the references among many which suggest this fact are:

https://stackoverflow.com/questions/53144535/pythons-string-formatting-throws-error-when-applied-within-mysql https://www.reddit.com/r/learnpython/comments/9kjw47/f_string_and_sql/ https://www.reddit.com/r/learnpython/comments/fjsypr/converting_sql_insert_queries_to_fstrings_gives/

Can I use prepared statements as that would only be the solution to this issue or would you suggest anything else? Would love to know your thoughts on it. Thanks 😃

+2
Pihu1998 on Mar 18, 2020

@janakrajchadha logging is a nice option, please create a new issue for the same.

+2
vishakha-lall on Mar 15, 2020

@Pihu1998 Yes, the solution does make sense. While this may be solved using either the executemany function or f-strings, I think we should go ahead with the latter to maintain consistency. Additionally, I think we should log any queries that are run to insert data. @vishakha-lall thoughts?

+2
janakrajchadha on Mar 15, 2020

Hi @vishakha-lall , I am waiting for @janakrajchadha to give his views on my solution. Moroever, I tried generating the error and was successful in generating a database error. I have messaged @janakrajchadha on the slack with a screenshot of the error to confirm for the same. Along with this, I am working on that error as well to fix it. I will try to raise a PR by tomorrow. Thanks 😃

+2
Pihu1998 on Mar 14, 2020

Thanks for the above suggestion @chttrjeankr . So, are you suggesting to typecast the arguments to string as discussed? I am currently doing this way: cur.execute("INSERT INTO chat_table(root_word,verb,sentence) VALUES (%s, %s, %s)", (root, verb, H))

Thanks for helping 😃

+1
Pihu1998 on Mar 22, 2020
Read more comments on GitHub
← react-google-maps: useMap() is always returning null for me.
ElixirSublime: Code completion not working →