React-starter-kit: This starter kit has SQL injection vulnerabilities up the wazoo

[robbyoconnor]

See bobby tables...use an ORM such as bookshelf.js -- just don't do what you're doing. It's insecure!

[richb-hanover]

Would you care to point out one or two places that you discovered? Thanks.

[robbyoconnor]

He's not using an ORM -- he's just constructing queries and depending on postgres to sanitize the inputs, this falls apart when people choose to not use postgres...

[richb-hanover]

Not intending to be sarcastic, but it would help me if you could point out modules/line numbers where you see this bad practice? (I'm not doubting, but it would help me to see things through your eyes.) Thanks.

[ncrawlins]

Everyone knows, as long as you use an ORM, SQL injection is impossible and you never have to think about it again. But if you don't use an ORM, your whole database will be stolen within minutes! Yep, you should totally add in knex and bookshelf on top of pg just to feel safer about a handful of parameterized queries on 4 simple tables that are already protected from SQL injection (here). (I did intend to be sarcastic.)

[richb-hanover]

@ncrawlins This is to confirm I understand your sarcasm :-)

1. There's no reason to fear SQL injection attacks if your code sanitizes all the inputs before creating the SQL queries to be sent to the database.
2. It would be foolish (and indeed, a serious SQL injection vulnerability) to allow the web client to specify a full SQL command and forward it blindly to the database. That's the "Bobby Tables" scenario.
3. OP hasn't pointed out any specific modules/lines, so react-starter-kit likely handles SQL commands properly.

Right?

[robbyoconnor]

@richb-hanover what's up with the condescending tone? https://github.com/kriasoft/react-starter-kit/blob/67593f112b93df3471c4f07ac0727703f16e1367/src/core/passport.js

[mlomeli]

According to the link @ncrawlins shared, node-postgres already offers SQL injection protection, so there isn't a security vulnerability.

I'm not sure if the starter kit provides support for other databases so probably not an issue. I don't think introducing a ORM for a login should be part of the starterkit.

Documenting with an inline comment that the SQL validation is performed by the package could be helpful, but I'm too new to Node.js to know if this is standard practice.

[koistya]

What @mlomeli said ^^ Closing for now, unless there is a proof of SQL injection vulnerability.