Hacker News new | ask | show | jobs
by ecopoesis 4629 days ago
Absolutely, emphatically no.

Building SQL strings from user data is a terrible idea. You will have SQL injections. You will compromise your database. I'm sure someone else will add the obligatory link to the Johnny Droptables xkcd.

Just learn SQL. It is just not the hard. And please, please use bind variables.

2 comments

The advice given to programmers using an ORM or a database access library just doesn't work for programmers building one. He can't just "use" bind variables. He has to incorporate the concept into his code.

It is unfortunate that the Python DB-API evolved in the way that it did. It is only an API doc, and therefore every db library has to reimplement much of the same functionality, like the string escaping that prevents SQL injection attacks.

It would do a lot of good for a simple thin mapper like this to be more widely used so that more people can see what is involved in making things secure, as opposed to just piggybacking on a library that does everything for you.

People are actually starring it! Is this some sort of subtle joke?

Just to emphatically underline ecopoesis' point, this code is completely insecure, open to the most basic SQL injection attacks.

Op you need to delete the repo asap.

Of course I'm not saying this code is suitable for production. It's just a toy "ORM". How you could possibly think I'm trying to replace something like sqlalchemy with 50 lines of code? Please guys, I'm not gonna delete the repo. It's just for fun. I'll add a warning about the sql injection problems and explicitly tell that this code is NOT for production. Event when I think that is obvious.
I don't even think it's ready to be used in a prototype. No joins, no unions, as mentionned before SQL injections. I mean, besides teaching yourself what a basic Mapper is, I don't see any use to this.
Not just teaching myself. This could teach anyone else that didn't know about object mappers. Even if this is the most insecure code in the world, it can be improved. That's the reason it is on github. You could contribute and add joins, unions, sanitize the user inputs, etc.
Hey, it's a github repo. If you have discovered an issue with it, then clone the repo, fix the issue, and send in a pull request.

Yes, it does the simple form of variable substitution that opens the door to SQL injection attacks, but using a .execute method with ? in the SQL for variable substition is not the real solution. The real solution is what happens inside the .execute methods. There is no reason why that same code could not be incorporated in this mapper, and still maintain the goal of a minimal data access layer with no magic.

You can get some ideas in how to improve the code here https://github.com/PyMySQL/PyMySQL/blob/master/pymysql/curso... starting around line 91.