OWASP / Go-SCP

Golang Secure Coding Practices guide
https://owasp.org/www-project-go-secure-coding-practices-guide/
Creative Commons Attribution Share Alike 4.0 International
4.83k stars 369 forks source link

Place older "?" is true for mysql, not for postgresql #17

Closed nicobouliane closed 7 years ago

nicobouliane commented 7 years ago

https://github.com/Checkmarx/Go-SCP/blob/master/src/output-encoding/sql-injection.md

It could be specified that postgresql use $1, $2, $2... and that mysql use ?, ?...

customerId := r.URL.Query().Get("id")
query := "SELECT number, expireDate, cvv FROM creditcards WHERE customerId = ?"

stmt, _ := db.Query(query, customerId)
Notice the placeholder1 ? and how your query is:
jparnaut commented 7 years ago

Well, we didn't cover postgresql because it isn't part of the standard library. I also did a quick search and noticed that go-mssqldb uses mssqldb's standard format which is also different: db.QueryContext(ctx,select * from t where ID = @ID;, sql.Named("ID", 6))

Maybe we can mention that developers should read the documentation of the driver they're using regarding this, and note the fact that some database drivers use a different syntax for placeholders?

/cc @PauloASilva @ErezYalon

nicobouliane commented 7 years ago

Sounds like a good idea ! thanks for your reply.

PauloASilva commented 7 years ago

There's already a footnote stating that

The placeholder syntax in prepared statements is database-specific.

jparnaut commented 7 years ago

Ok I'll close the issue then, although, in my opinion a little more emphasis on this does not seem like a bad idea given it's importance.

PauloASilva commented 7 years ago

@jparnaut

Ok I'll close the issue then, although, in my opinion a little more emphasis on this does not seem like a bad idea given it's importance.

I do agree, let's reopen the issue and work on it.

@nicobouliane would you like to rewrite the section to include your suggestion?

PauloASilva commented 7 years ago

Waiting for PR#20

PauloASilva commented 7 years ago

PR#30 closes this issue.