Skip to content

Escape all sql statements to avoid sql injections#16

Merged
roschaefer merged 1 commit intomasterfrom
14-patch-sql-injections
Jan 28, 2019
Merged

Escape all sql statements to avoid sql injections#16
roschaefer merged 1 commit intomasterfrom
14-patch-sql-injections

Conversation

@matboehm
Copy link
Copy Markdown
Contributor

Escape all sql statements to avoid sql injections as this is not done… by default from node mysql package.

Contrary to the statement here the user input and thus most field are NOT automatically sanitized and escaped by node mysql. At least not in the way it was done until now.

See: https://www.npmjs.com/package/mysql#escaping-query-values and https://stackoverflow.com/questions/49015570/node-mysql-escaping-mysql-escape-mysql-escapeid (last answer) just to name a few.

This is why I had to change every sql statement accessing user input, so that is escaped and we don't suffer from sql inection attacks.

@matboehm matboehm added the bug Something isn't working label Jan 26, 2019
Copy link
Copy Markdown
Contributor

@roschaefer roschaefer left a comment

Choose a reason for hiding this comment

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

Relieved

@matthib thank you so much!

  • As a developer you should get anxious, if you write string interpolation and feed this to a SQL connection
  • How can we get this deployed?

I'm going to CC @mimicc83 can you help us to deploy this security fix?

@roschaefer roschaefer merged commit 9bad2bb into master Jan 28, 2019
@roschaefer roschaefer deleted the 14-patch-sql-injections branch January 28, 2019 14:29
@ghost
Copy link
Copy Markdown

ghost commented Jan 28, 2019

:o not sure where I've read this, but I was in the thought that this package does automatic escaping.
Good that you've found this issue.

However: the initial thought was to use mysql only as temporary so we can keep the old db structure as much as possible and switch to mongodb at some point.

@roschaefer what exactly do you need help with? I am a bit short of time at the moment.

@roschaefer
Copy link
Copy Markdown
Contributor

@mimicc83 I would like to get this deployed as soon as possible to avoid the SQL vulnerability - I have no idea how to do that. Neither do I have a clue how to backup the database to avoid data loss.

@ghost
Copy link
Copy Markdown

ghost commented Jan 28, 2019

There is no deployment pipeline setup on this so you would have to either setup docker or do a drag and drop deployment (should be only this single file that needs changes, the client would not be affected)

To backup the database mysqldump should be sufficient.

@roschaefer
Copy link
Copy Markdown
Contributor

@mimicc83 and @matthib are you available for today 4:30pm? I would like to backup or deploy today.

cc @AndreiBu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working PR: merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants