Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115161
Message-ID: <903A2661-67DA-458F-A26A-2A4949A9C2D9@newclarity.net>
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_FD2A804A-AC93-4739-95A3-B7A574BF8C8A"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Date: Sat, 26 Jun 2021 12:33:49 -0400
In-Reply-To: <CAFv4g+GSrbErpT-hCUVbtTagphz79kmtzvMQcuRbarKTsQd+4w@mail.gmail.com>
Cc: PHP internals <internals@lists.php.net>
To: Craig Francis <craig@craigfrancis.co.uk>
References: <CAFv4g+GCbRataTtCKSrej5yU259=hbjNxQVzZX+oXm_vh0z-TQ@mail.gmail.com>
 <A0784946-A9F5-4EB0-B04C-EAFDD1359FF5@newclarity.net>
 <CAFv4g+GSrbErpT-hCUVbtTagphz79kmtzvMQcuRbarKTsQd+4w@mail.gmail.com>
Subject: Re: [PHP-DEV] is_literal() is back
From: mike@newclarity.net (Mike Schinkel)

--Apple-Mail=_FD2A804A-AC93-4739-95A3-B7A574BF8C8A
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

> On Jun 26, 2021, at 10:03 AM, Craig Francis <craig@craigfrancis.co.uk> =
wrote:
>=20
> 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 =E2=80=9C30, 10=E2=80=9D 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 =3D new MySqlBuilder();=20
$query =3D $builder->select()->setTable('user')->limit('30,10');=20

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 =3D $builder->select()->setTable('user')->offset(30)->limit(10); =20=


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. =20

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.
>=20
> The RFC proposes to add an internal "literal" flag to a string, the =
is_literal() function, and nothing else.=20
>=20
>  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. =20
>=20
> 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.=20
>=20
> 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.
>=20
> 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:
>=20
> function make_literal(string $non_literal):string {
>     $literal =3D '';
>     for( $i =3D 0; $i< strlen($non_literal); $i++ ){
>         $literal .=3D chr(ord($non_literal[$i]));
>     }
>     return $literal;
> }
>=20
> 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].=20
>=20
> 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:
>=20
> $sql =3D 'SELECT * FROM foo WHERE id=3D' . make_literal( $_GET['id']);
> $result =3D mysqli_query($conn, $sql);
>=20
> So what am I suggesting we do? =20
>=20
> 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.
>=20
> 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.
>=20
> Thank you in advance for your consideration.
>=20
> -Mike
>=20
> [1] https://3v4l.org/oCBp7#focus=3Drfc.literals =
<https://3v4l.org/oCBp7#focus=3Drfc.literals> =
<https://3v4l.org/oCBp7#focus=3Drfc.literals =
<https://3v4l.org/oCBp7#focus=3Drfc.literals>>
> [2] =
https://gist.github.com/mikeschinkel/b9abd4178db461568b813269bc936c18 =
<https://gist.github.com/mikeschinkel/b9abd4178db461568b813269bc936c18><ht=
tps://gist.github.com/mikeschinkel/b9abd4178db461568b813269bc936c18 =
<https://gist.github.com/mikeschinkel/b9abd4178db461568b813269bc936c18>>=20=



--Apple-Mail=_FD2A804A-AC93-4739-95A3-B7A574BF8C8A--