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

[IMP] Prevent SQL Injections #27

Merged
merged 1 commit into from
Mar 31, 2020
Merged

[IMP] Prevent SQL Injections #27

merged 1 commit into from
Mar 31, 2020

Conversation

arbaes
Copy link
Contributor

@arbaes arbaes commented Mar 31, 2020

Using python formatting to build SQL instructions is very insecure.
Also added .db files in .gitignore

@eibex
Copy link
Owner

eibex commented Mar 31, 2020

Thanks! Apologies for not being aware of this.

@eibex eibex merged commit 20c54ff into eibex:master Mar 31, 2020
@arbaes
Copy link
Contributor Author

arbaes commented Mar 31, 2020

My bad too, I should have told you sooner, but you're too fast for me :D

Also, small fix is coming

@eibex
Copy link
Owner

eibex commented Mar 31, 2020

Sorry, too much time on my hands due to COVID-19 :P

@@ -112,18 +112,18 @@ def end_creation(user, channel, message_id):


def exists(message_id):
db.execute(f"SELECT * FROM messages WHERE message_id = '{message_id}';")
db.execute("SELECT * FROM messages WHERE message_id = ?;", (message_id,))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the commas after, for example, message_id serve a purpose or can I delete them? I do not see examples in the sqlite3 library using trailing commas.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's for forcing tuples

@@ -34,7 +34,7 @@ def _generate_reactionrole_id(self):
while True:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, this is not the proper way to proceed, that why we use Primary Keys

@arbaes
Copy link
Contributor Author

arbaes commented Apr 1, 2020

I think I will not have time to do another PR soon, so here's a more few remarks if you have time.

  • I noticed you never consider that your transactions/connections could fail, you can use try-catch with sqlite3.Error
  • It's a better practice to close your connection,cursor when working with DB. I read in this case it's not a big problem as far as a single process is using the connection. But letting a cursor open could lead into memory leaks.
  • There should be also some sort of conventions while working with a db. In your code, database is actually a connection, while db is actually the database cursor.

I suggest you to read This guide, it will explain better than me :)

@eibex
Copy link
Owner

eibex commented Apr 1, 2020

Thanks, I will study it this week.

I barely ever worked with SQL queries beforehand, sorry!

@eibex eibex mentioned this pull request Apr 1, 2020
@arbaes
Copy link
Contributor Author

arbaes commented Apr 2, 2020

No need for excuses, I just wanted to give you some insights :)

eibex pushed a commit that referenced this pull request Aug 23, 2020
eibex pushed a commit that referenced this pull request Aug 23, 2020
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 this pull request may close these issues.

2 participants