Re: is_literal() is back

> On Jun 26, 2021, at 10:03 AM, Craig Francis <craig@craigfrancis.co.uk> wrote: > > Just a quick reply at the moment, but what happens if a db library had a method to set what was > in the LIMIT part of a query, you did the MySQL thing of “30, 10” to get paginated results, and > then one day the library changed to require the argument to be an integer? You didn't provide enough details on your hypothetical for a definitive answer, so let's fill in some details so I can provide an answer (but if theses details do not fit the use-case you envision, you'll need to be more specific.) This db library you speak of is a SQL query builder. Using it would look like this (code examples derived from [1]): $builder = new MySqlBuilder(); $query = $builder->select()->setTable('user')->limit('30,10'); The version where the above code works is version 2 and there are no type hint on method parameters. Then they release version 3 is which they add an int type hint on the limit() method, and they introduce an offset() method. This is the scenario I will use to answer your question. ----- I upgrade to version 3 and my code breaks. I inspect the error, figure out the problem and then I change my code to look like this: $query = $builder->select()->setTable('user')->offset(30)->limit(10); Problem solved. That said, please note the difference between your LIMIT scenario and the is_literal() scenario are three (3) fold: 1. If added to PHP is_literal() will be promoted as the panacea to solve all injection vulnerabilities in (almost?) all the "What's new in PHP" blog posts and then many of the PHP library developers who read these articles will rush to add is_literal() checks to their libraries without fully understanding and addressing the ramifications. And that over-zealousness will take time to recognize and thus recover from. 2. Your LIMIT scenario does not have a coincidental concurrent PR blitz that is_literal() will get meaning that even if some db libraries do constrain LIMIT it will only be a few at a time at most. So no more a problem than any other backward compatibility break in a library. 3. Finally and most importantly, there is an easy fix in userland for your LIMIT scenario as I illustrated. For many use-cases where is_literal() may be added as a gatekeeper there would be no easy fix, except to use a technique that your RFC describes as "clearly the developer doing something wrong." -Mike [1] https://github.com/nilportugues/php-sql-query-builder > On Sat, 26 Jun 2021 at 2:51 pm, Mike Schinkel <mike@newclarity.net > <mailto:mike@newclarity.net>> wrote: > The idea behind is_literal() is of good intention, but as they say the road to hell is paved > with good intentions. > > The RFC proposes to add an internal "literal" flag to a string, the is_literal() > function, and nothing else. > > Further the RFC states a vision to get "libraries to start using is_literal() to check > their inputs." Again, that comes from a great intention. > > The problem lies with the fact that library developer who choose to disallow non-literal > strings will offer solutions when a use-case literally (pun-intended) cannot produce a literal > string. > > Sure, most leading library developers will offer a solution, but many of the long-tail library > developers will not either because it will add scope and/or because those library developers > don't have to skill and/or experience to do so. > > So what will those users of those libraries do when faced with a required to only use literal > strings? They will find a workaround so they can get their jobs done. And that workaround is > really simple: > > function make_literal(string $non_literal):string { > $literal = ''; > for( $i = 0; $i< strlen($non_literal); $i++ ){ > $literal .= chr(ord($non_literal[$i])); > } > return $literal; > } > > You can see it in action 3v4l.org <http://3v4l.org/> > <http://3v4l.org/ > <http://3v4l.org/>> here[1] and for posterity on > gist.github.com <http://gist.github.com/> > <http://gist.github.com/ > <http://gist.github.com/>> here[2]. > > Once developers start bypassing the is_literal() check then all those good intentions will be > moot, and many who think they are secure from injection attacks will be vulnerable: > > $sql = 'SELECT * FROM foo WHERE id=' . make_literal( $_GET['id']); > $result = mysqli_query($conn, $sql); > > So what am I suggesting we do? > > 1. We postpone passing this is_literal() RFC until we have collectively addressed how userland > developers will be able to handle non-literals in SQL, HTML, etc. when their use-cases require > non-literals. > > 2. We could also go ahead and add the internal "literal" flag to a string so that if > someone wants to use it to write an extension to add an is_literal() function in the mean time it > would be available, but not yet standardized into PHP. > > Thank you in advance for your consideration. > > -Mike > > [1] https://3v4l.org/oCBp7#focus=rfc.literals > <https://3v4l.org/oCBp7#focus=rfc.literals> > <https://3v4l.org/oCBp7#focus=rfc.literals > <https://3v4l.org/oCBp7#focus=rfc.literals>> > [2] > https://gist.github.com/mikeschinkel/b9abd4178db461568b813269bc936c18 > <https://gist.github.com/mikeschinkel/b9abd4178db461568b813269bc936c18><https://gist.github.com/mikeschinkel/b9abd4178db461568b813269bc936c18 > <https://gist.github.com/mikeschinkel/b9abd4178db461568b813269bc936c18>> >