search

friends-of-reactphp / mysql

Async MySQL database client for ReactPHP.
MIT License
334 stars 66 forks source link

Support for LOAD DATA LOCAL INFILE #105

Closed psafarov closed 5 years ago

psafarov commented 5 years ago

I thought that this feature is available as the client passes corresponding flag to a server, but when a query LOAD DATA LOCAL INFILE ... is sent, nothing happens until connection closes.

psafarov commented 5 years ago

I would like to contribute with this one, doesn't seem as a very difficult task, but I use amphp more, just wanted to make you aware that this feature is absent.

clue commented 5 years ago

@psafarov Thanks for reporting, this is indeed not supported by this library at the moment and I'm not sure we should support it in the future.

The LOAD DATA LOCAL INFILE is broken beyond repair (https://blog.tarq.io/insecure-defaults-exploiting-load-data-local-infile/, https://medium.com/bugbountywriteup/adminer-script-results-to-pwning-server-private-bug-bounty-program-fe6d8a43fe6f, https://dev.mysql.com/doc/mysql-security-excerpt/5.7/en/load-data-local.html and others) with plenty of security incidents in the past and it's usually best to keep it disabled on the server and/or client side. Common clients (including PDO https://www.php.net/manual/en/ref.pdo-mysql.php) disable this by default and may or may not provide ways to explicitly enable it if it is really needed.

If anybody feels like working on a PR, the changeset looks reasonable and provided security is taken care of, I'm not opposed to getting this in. It's my understanding that it's probably not worth the effort and I don't plan to work on this any time soon, but I won't stand in the way of getting this in :+1:

psafarov commented 5 years ago

@clue despite its cons this feature is very useful in some cases, so I would not take choice away from users. Also it is not clear how to fix security problems on client level. I think if somebody comes up with a basic implementation, it should be accepted. The potential risks could be documented.

clue commented 5 years ago

I still think that this feature makes perfect sense :+1: However, there are currently no immediate plans to build this from my end (no demand at the moment and more important outstanding issues currently), but I suppose we would be really happy to accept PRs :+1:

I believe this has been answered, so I'm closing this for now. Please come back with more details if this problem persists and we can reopen this :+1: