Hacker News Reader

Comment by torgard | Hacker News Reader

Just a heads-up, you might want to take down the example website. Now that it's been posted to HN you might see malicious actors.

I also think it's prone to SQL injection at the moment? At least, it's raising a syntax error when inputting an apostrophe.

View on HN

Yes, don't do this:

https://github.com/rehacktive/caffeine/blob/master/database/...

"INSERT INTO %v (id, data) VALUES('%v','%v') ON CONFLICT (id) DO UPDATE SET data = '%v'"

Use prepared statements and parameters passed to the db driver, not building strings with strings or you are vulnerable to sqli.

I'd also avoid using %v anyway when building strings - safer to use a specific type like %d for int.

You're right of course, but I think the idea of this software is that the user (and, by extension, their input) are trusted.

Good to start with the right habits. People will put this on the internet, the author did for example.

> You're right of course, but I think the idea of this software is that the user (and, by extension, their input) are trusted.

No, you should enforce the most basic security practices even if the users are "trusted". Somebody might put that on the internet for a demo for client and get hacked in no time, because the code is opened to SQL injection. This isn't acceptable. There are minimum security standards every project should follow.

And since none of the minimum security standards are implemented in that project, I would not recommend using it until they are.

I'm not sure what you are "no"-ing. I agreed with the parent comment.

the reason I went "quick'n dirty" is for the prototyping nature of the project. But I'll fix this anyway, thanks!

Using the db Exec / Query / Query row is the same amount of code and effort the fmt.Sprintf statements. Even in quick-and-dirty mock-ups, it's a good idea to not cut corners there.

There's nothing as permanent as a temporary solution. There's been countless SQL injection vulnerabilities exploited over the past decades with the "I'll fix it later" mindset.

Start with prepared statements by default, they are not more work than formatting strings.

Are you saying that %v should be avoided in general, or just in the context of queries? (Go noob who doesn't understand %v)

In queries, you should use the database/sql.DB interface if possible with your database https://pkg.go.dev/database/sql#DB.Exec

It should sanitize / quote arguments for you and protect against SQL injection. Note that this doesn't mean all data sanitization is performed, just the basic '; do my stuff here; -- type of things.

First, don't build strings like this for sql.

But for other strings built with sprintf, yes I am saying don't use %v unless for debugging, it's just one more avenue where you might be surprised by input, even if you're not building strings for sql. For example, someone might do this with user supplied data:

myoutput := fmt.Sprintf("user:%v",userID)

and if userID is a string like "foo" it'll end up in their string and they won't get what they expect. So better to just put another guardrail on there and insist that the param is an integer or whatever you expect by using %d which will only accept an int - this means you have to convert it to an integer first.

Many vulnerabilities are caused by data not being in the format people expect (not just sqli).

Thanks for the explanation! That is a good point about being data-aware.

>>Now that it's been posted to HN you might see malicious actors.

This needs to be an HN fN Masterclass by @Dang....

We should have regular visibility into /how/ HN is used by malicious actors.... HN has enough pedigree to get some insight from some of the top security folks... and it would be REALLY good to know how HN is being harvested in this space.

Reply on news.ycombinator.com