search

nhoening / gritttt-rss

Implements 'cancelled' sharing-features of Google Reader for the excellent open-source RSS-Reader TinyTiny-RSS: share any page on the web via a bookmarklet and show your shared items in a widget on any website you want. Also allow to import shared&starred articles from Google Reader.
gritttt-rss.nicolashoening.de
Other
48 stars 4 forks source link

fix sql injection #44

Closed dr4Ke closed 11 years ago

dr4Ke commented 11 years ago

Hi, This commit fixes possible sql injections through driveby-sharing form. Without it, there are errors with every title or comment containing the ' character. Disclaimer : I'm no security expert, so this fix can be wrong.

EDIT: my comment was misleading.

nhoening commented 11 years ago

Thanks for the commit! I hope I find time soonish to see what the ' character issue is. In the meantime, sql injections are not as big a worry for me as they usually are because you have to authenticate (to tt-rss) anyway before you can enter data.

dr4Ke commented 11 years ago

2013/2/1 Nicolas Honing notifications@github.com

Thanks for the commit! I hope I find time soonish to see what the ' character issue is. In the meantime, sql injections are not as big a worry for me as they usually are because you have to authenticate (to tt-rss) anyway before you can enter data.

Yes, the security concern does not worry me too much either.

The ' character issue is just that this line : INSERT into table (col1, col2) VALUES ('val1', 'val2');

will fail if val1 = "someone's thing" as : INSERT into table (col1, col2) VALUES ('someone's thing', 'val2');

The use of mysql_real_escape_string() solves this nicely.

Christophe.

nhoening commented 11 years ago

The use of mysql_real_escape_string() solves this nicely.

Ah, so you fix this problem? From your first comment, I thought your patch still has this problem. So this is probably good news :)

dr4Ke commented 11 years ago

Yes, sorry I didn't read again before posting.

nhoening commented 11 years ago

Just one thing: mysql_real_escape_string is deprecated and will be removed. I think we should use mysqli_real_escape_string(). If you could change that, I'll gladly merge :)

dr4Ke commented 11 years ago

Done. I've seen you used other functions from this module, though.

nhoening commented 11 years ago

I've seen you used other functions from this module, though.

Are you sure? Honest question, I am not a PHP guru in my main time. I searched around and I seem to use mysql_query and db_connect from the db module in tt-rss, and those use deprecated stuff, true. Also init_connection is from tt-rss (functions.php). Let me know if I have written deprecated things myself, would be appreciated.

Anyway, thanks for the contribution!

dr4Ke commented 11 years ago

2013/2/1 Nicolas Honing notifications@github.com

Let me know if I have written deprecated things myself, would be appreciated.

Well, maybe mysql_insert_id in share.php, line 44. But I'm not a PHP dev myself, so…

Anyway. Thanks for merging my change.

Regards, Christophe.