To do it properly you'd want to use prepared statements[1] which requires a non-trivial, though not particularly complicated, syntax change. So your global actor would have to parse the PHP in a rather more intelligent way that just a string replace.

You could just use mysql_real_escape_string [2] but that's less secure than prepared statements, and may break some things (eg if the code is relying on certain things not being escaped, etc). Also that extension is deprecated in favour of prepared statements.

Also, some of the dodgy code is using $_GET vars for things other than values, like field and table names (!!!!) which would need a more significant refactoring in order to fix.

However it would be a great little problem to work on, and you could probably write a bot to fix 80% of the code pretty easily.

The other issue is how many people will understand and accept the pull requests...

[1] http://php.net/manual/en/pdo.prepared-statements.php

[2] http://php.net/manual/en/function.mysql-real-escape-string.p...

I know the PHP/PDO way to do it, is prepared statements.

I'm not able to see why the parent's suggestion to just escape the strings is not a valid solution from a security perspective.

because you also have integer based SQL injection. Escaping strings isn't a complete fix.

Yep. In fact the typical SQL injection example is " 0 OR 1=1 ".

>that's less secure than prepared statements, and may break some things (eg if the code is relying on certain things not being escaped, etc)

The pull request could be done "stupidly", which may fix most problems (your 80% value), and for cases where it wouldn't "just work" the author could just reject the pull request but still be made aware of the problem.

I think what I'd do is write a bot that issues pull requests along with a commit message explaining what was fixed, why it needed fixing, how the automatic fix isn't perfect and that they should really consider rewriting it to use prepared statements.

Include a check to make sure multiple bugs in a single repo are handled by just one pull request too.

Because there are plenty of times when that would break things. Consider:

    $table_name = $_GET['from'];
    if (!in_array($table_name, $VALID_TABLES)) {
      die('Invalid table name');
    }
    $set_to = (int)$_GET['set_to'];
    mysql_query("INSERT INTO $table_name (value) " .
      "VALUES ($set_to);

Quoting/escaping the table name in this case would break the code. (Backticks will work, but that's another story.) The variable has already been sanitized immediately before, so there's no injection possibility.

The case with $set_to is still pretty bad form, but it has been sanitized above by casting to an int.

Not saying this is great code, just pointing out the fact that there's no possible way to do a mass-commit fix.