mysql

mysql copied to clipboard

I noticed that Prepared Statements seem to be emulated client-side by escaping certain characters.

Any plans to fully support service-side Prepared Statements? This can be done via the binary protocol, but there's a slower SQL-based approach available for non-binary clients: http://dev.mysql.com/doc/refman/5.5/en/sql-syntax-prepared-statements.html

Yes, prepared statements are on my todo list. I don't need them myself, so unfortunately they kind of linger at the bottom of the list unless somebody wants to sponsor some of my time to work on the feature.

That being said, the SQL based approach looks interesting as a stop-gap solution for the short term.

The only downside with the SQL-based approach is that you probably still end up needing to do client-side escaping. Still it does offer a little bit more structure, so it might still buy some protection. Depending on how you do it, it might also simplify the escaping part.

Unless I'm mistaken, you are already implementing the actual protocol at the lower levels of your driver. I wonder how much more you need at that level to finish? http://dev.mysql.com/doc/internals/en/command-packet-details.html

Prepared statements use a range of additional packets that are currently not implemented by my driver:

• http://dev.mysql.com/doc/internals/en/prepared-statement-initialization-packet.html
• http://dev.mysql.com/doc/internals/en/parameter-packet.html
• http://dev.mysql.com/doc/internals/en/long-data-packet.html
• http://dev.mysql.com/doc/internals/en/execute-packet.html

I have not yet analyzed how much work it would be to implement them, but my gut feeling is ~5 days of work.

How does https://github.com/sidorares/nodejs-mysql-native handle this? Any reason not to just borrow parts of the way it's done over there?

I'm still somewhat struggling with the number of different MySQL drivers for Node.JS. I think Node makes it way too fun to write network protocol code. :P Maybe in a year or so the community will have coalesced around one or two really solid libraries.

How does https://github.com/sidorares/nodejs-mysql-native handle this?

It seems to implement the parts of the protocol that are required for prepared statements.

Any reason not to just borrow parts of the way it's done over there?

Yes, I didn't have the time to work on this yet. I'm also not in the business of copying code unless it's up to my personal coding standards. So even with good inspiration like this, it will still take me some time.

Maybe in a year or so the community will have coalesced around one or two really solid libraries.

This library is solid. It just does not implement all features.

Couldn't we just prepare and execute statements using SQL instead of raw packets?

@dresende the SQL method still winds up tampering with values to make them safe (escaping quotes, etc), whereas the protocol method explicitly separates query from values so tampering is not necessary. To be fair, as long as its impossible to smuggle a query in as a value, the driver is plenty secure enough. I suppose I'm just being a nitpicky ex-PHP developer who wants everything to be conceptually elegant. :P

I just asked because I don't know the protocol deeply, but besides that it seems easy to implement based on the current module structure. I just don't have the knowledge about the protocol and documentation is scarce..

I also need this feature from a security point of view. Your implementation of parameter escaping in JS only is naive (no harm intended - I've written things like that myself in the past), and I'm pretty sure it's open to any semi sophisticated SQL injection bot (e.g. using UTF-8 escape strings). MYSQL Prepared statements uses MYSQL own escaping subsystem, which has been refined for years, and is maintained by security experts.

Support for prepared statements and parameter binding is a must if you want your library to be used in corporate world.

I also need this feature from a security point of view. Your implementation of parameter escaping in JS only is naive (no harm intended - I've written things like that myself in the past), and I'm pretty sure it's open to any semi sophisticated SQL injection bot (e.g. using UTF-8 escape strings).

If you care about security, please don't spread FUD.

Support for prepared statements and parameter binding is a must if you want your library to be used in corporate world

I see that you're a prolific open source contributor yourself, so you should know that this isn't exactly the most successful strategy for requesting features. Of course this is an important feature and needs implementing, but unless somebody steps up to the challenge, it won't happen anytime soon.

Again, no harm intended. I don't know your security background, and both your past answers in this issue and the current implementation of parameter escaping made me think that you may not care about this as much as it deserves to. If I was wrong, sorry to have offended you.

So I understand that you care about this issue but won't implement it yourself, is that correct? I am surprised you don't put this one on top of your list, but maybe you're a much better expert in security that me and I overestimate the risks.

and the current implementation of parameter escaping made me think that you may not care about this as much as it deserves to.

This is what I mean by FUD. Please point out an actual attack vector. If it exists, it will be fixed.

So I understand that you care about this issue but won't implement it yourself, is that correct? I am surprised you don't put this one on top of your list, but maybe you're a much better expert in security that me and I overestimate the risks.

As I said before, I don't have any time to hack on this module right now / don't need this feature myself. This says nothing about the importance of the feature, it's just my current situation. If somebody was to sponsor the development costs, I could make room in my schedule, otherwise this feature won't land until somebody else contributes a patch.

but maybe you're a much better expert in security that me and I overestimate the risks.

I'm not a security expert. But I implemented the same escaping approach outlined by the MySQL documentation, which is also used by libmysql as well as mysqlnd. Afaik, it is correct and secure.

Additionally, multiple statement execution is disabled by default in this module, this limits the impact of any possible exploit could have.

PHP has had tons of different vectors of SQL injection attacks before everyone started using prepared statements by default. Escaping is better left for MySQL's engine, rather than trying to reinvent the wheel on the client-side (in this case, the client is the app making the query).

For example, if I remember correctly, in PHP one attack vector involved the escaping mechanism trying to escape strings for a certain encoding, and the connection using a slightly different encoding, causing some characters to not be escaped properly for what MySQL was expecting.

Prepared statements are also important for queries that are re-used a lot. There are significant performance gains to he bad by using the same prepared statement multiple times rather than the fully query every time.

Some literature on the encoding thing: http://security.stackexchange.com/questions/9908/multibyte-character-exploits-php-mysql http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string/12118602#12118602

I'm not a accusing the library of being bad / insecure, but PHP battled with SQL injection problems for many years. The only "real" solution they came up with after all these years is using prepared statements.

So there is event more issues then security with this, which regardless should obviously be the most important reason for implementing this. I've done my own personal benchmarks vs mysql-native, which has prepared statements, and this library and it's over twice as performant, that's a pretty big jump. I'm not sure if this will convince you more that this might be a feature you need, honestly I think the whole security aspect should do it alone IMO.

I'm sympathetic to the idea of if you want this feature, submit a patch. But I have a few issues with the attitude taken here:

1. This isn't some minor feature. A lot of people, myself included, would consider prepared statements essential to any driver considering itself to be mature and stable. As of now, from what I can see this is the main driver the node community seems to be coalescing around. To not have prepared statements be a priority I think is a real problem because of that.

2. You haven't really abandoned this project, from what I can see. There is active development going on, there is a 2.0 alpha 7 version. From the commit logs it doesn't seem you're doing it yourself now, so I would be interested to see the developers who are actually actively working on this chime in. Stemming from the point above I see it as a real concern that a 2.0 version of mysql driver is being developed and prepared statements just aren't "that important" to be included.

3. If you care above the Node js community then you should care about this issue. If you care about security or radically increasing the performance of your code base then you should care about this issue. If you don't care about any of the above I'm a little concerned for the Node.js community in general, if this is the attitude essential libraries and their maintainers are going to take.

Yes, I'm free to not use this code, I'm free to not use Node.js in general. But if you care about actually improving the community and improving the quality of code out there on node js then the attitude shouldn't be either pay me or write it yourself. In the end this was your project and you decided to contribute back and still have developers actively working on it. And I think if you really want to keep contributing to the community you should reevaluate your attitude to this sort of issue.

I think everyone is convinced but none of us is payed to do it. We have other projects (some give as money, others give us joy..), so we can't be 100% on this. If anyone wants to step up and do something, just do it and make a pull request. It doesn't need to be perfect or even complete, we can improve it then. Feel free to be part of that community and actually improve the quality of this code.

Why are you closing this issue? You don't think not supporting prepared statements is an issue? People can still contribute to this issue and discuss it. If you close it, someone else will eventually open a duplicate of it.

Supporting prepared statements is not an issue. Having someone do it is an issue. Since when someone does something about it and does a pull request, a new issue is opened, this is not an issue anymore.

I don't think anyone here is demanding that you drop everything and implement prepared statements. But I do feel like the lack of prepared statements is a bigger issue than you guys are making it out to be, and closing the ticket on github seems like an attempt to sweep this under the proverbial rug instead leaving it open to encourage a transparent and constructive discussion, inducing collaboration / contributing with code. Just my 2 cents.

@dresende I'm re-opening this. The open github issues should list all reasonable suggestions for improvements, this is one of them.

@efuquen please go away. People arguing that me not implementing this feature in my spare time with virtually no benefits for myself is the same as not caring about the community is incredibly hurtful. I've poured my heart and soul into this library, and yes, I didn't get to implement every feature yet, and yes, this is unfortunately a very important one, but seriously, it's people like you that make me regret giving away so much of my work for free sometimes

go away? I've made what I feel are valid arguments. I'm sorry I hurt your feelings but I was only being direct and I certainly wasn't rude and I don't feel like that means I should "go away" or that I'm somehow a bad person like you've implied in your comment. You've made a library that right now is the defacto mysql library in the nodejs world, there are no other maintained alternatives. There is a "2.0" version being developed missing a critical feature that was brushed aside, I tried to highlight this with new arguments because previous ones haven't worked. Anyway there doesn't seem much more to be gained by commenting anymore so I'll leave it at that.

@efuquen : You seem not to understand the issue. Everyone agrees that prepared statements are important, no need to argue on that. Instead, if it's so important to you please step forward. I don't have the knowledge to do it. If I did, it would be already in an alpha version, and I don't have time to search for it. Do you? Can you please explain me how to do it? I'll happily implement it if you don't want to contribute and code some lines and just give me a technical explanation. I'll wait for a comment explaining or pointing to a clear explanation page and I'll do it.

This applies to anybody really wanting this feature. I want it to, it's just not my top priority and I don't have time for research.

Everybody who wants/needs prepared statements, here is your chance: @pyramation contacted me to make this happen, setup a crowd funding campaign, and made an incredibly generous initial donation: https://www.crowdtilt.com/campaigns/prepared-statements-for-nodemysql/description

If the funding goal is met, I've guaranteed to handle the implementation. However, if somebody from the community would like to lead this effort, I'd be happy to mentor / share the funds. I'm mostly thinking about @dresende who's been the driving force of goodness behind node-mysql for several months now and would really deserve to be paid for some open source hacking. So if anybody is interested, let me know - otherwise I'll write the code.

I'm in :) :+1:

I will try to help as much as I can. What's the game plan? :D

Great to see @dresende is in!

@cblage one easy thing people can do is share this link where people who are interested will find it: https://www.crowdtilt.com/campaigns/prepared-statements-for-nodemysql/description

@pyramation I'll share :) I'm also willing to help out with code. I'm not interested in getting any money for this, but I have a real interest in helping out. Prepared statements would make life easier for me at work :)

I don't have enough time to write it all / send a patch in, but I'm volunteering to help :)

I was wondering if this was being worked on. If not I would like to take the time to contribute. Please let me know. Thanks.

@smitt04 that would be awesome. I did some initial work here: https://github.com/felixge/node-mysql/tree/prepare but never completed it.

Ok cool, I will check out your other branch. I have some stuff to finish up first but hopefully i can get started on it with in the next couple of weeks.