Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:109209
Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
In-Reply-To: <CAFv4g+HgE1E0ecNDe9jX9RZyq5-+4FvjxXySL6v4OrgpjUXfzg@mail.gmail.com>
Date: Sun, 22 Mar 2020 15:10:58 -0400
Cc: PHP internals <internals@lists.php.net>
Content-Transfer-Encoding: quoted-printable
Message-ID: <4A0450AD-9F41-4762-B7E7-AF12A50C5CF9@newclarity.net>
References: <A1C95241-282A-4BF7-AEAC-93CAB56811E4@newclarity.net>
 <CAFv4g+HgE1E0ecNDe9jX9RZyq5-+4FvjxXySL6v4OrgpjUXfzg@mail.gmail.com>
To: Craig Francis <craig@craigfrancis.co.uk>
Subject: Re: [PHP-DEV] Are PECL modules preferable?
From: mike@newclarity.net (Mike Schinkel)

> On Mar 21, 2020, at 7:15 PM, Craig Francis <craig@craigfrancis.co.uk> =
wrote:
>=20
> On Sat, 21 Mar 2020 at 22:53, Mike Schinkel <mike@newclarity.net> =
wrote:
> A large number of PHP users have no control over the platform they run =
on, so the option to use PECL modules is a non-starter for them.
>=20
>=20
> Thanks Mike,
>=20
> Personally I agree, I would say PECL modules are not preferable for =
"useful features"; simply because I try to keep my systems only using =
core PHP features where possible (makes server admin easier).

+1

> As you mention working with WordPress, I've seen a couple of =
developers who have taken examples like:
>=20
>     $posts =3D $wpdb->get_results("SELECT ... WHERE =
post_type=3D'post'");
>=20
> Then edited it to something dangerous like:
>=20
>     $posts =3D $wpdb->get_results("SELECT ... WHERE post_type=3D'" . =
$_GET['type'] . "'");

Yes.  That is all too common.

> To guard against this, do you think that WordPress could update their =
get_results() function to do something like:
>=20
>     public function get_results( $query =3D null, $output =3D OBJECT ) =
{
>       if (!is_literal($sql)) {
>         trigger_error('This is an unsafe $query, please use =
$wpdb->prepare()', E_USER_NOTICE);
>       }
>=20
> Perhaps with a better message; then, over the years, increase the =
warning level?
>=20
> I think that would be a very useful way of getting developers aware of =
these dangers.

Well...

IMO getting that in WordPress core is highly unlikely for two (2) =
reasons:

------------
1. Getting the WordPress team to adopt new features is about as =
difficult as getting the PHP community to agree on and add a new =
feature. IOW, it is a huge uphill battle.

I have many, many unfulfilled feature requests in WordPress' trac that =
are up to 8 years old or more even though on many of the tickets there =
appears to be universal interest.  What I have found is that the =
WordPress team does not address anything unless they need it to =
implement the features that Matt has dictated should be in the next =
version of WordPress. Because of that, many very useful ticketsl sit =
there for years and years and do not get addressed. =20

Of course if a core team member with commit access has a pet feature =
they want to add, it typically gets slipped it with no fanfare and then =
we are stuck with it (*cough* Customizer *cough*)

I did recently get one ticket addressed about 11 months ago that was a =
subset of a ticket that is 9 years old (and sadly the OP on the older =
ticket has since died of cancer.)

But I think I invested probably 80 hours over that duration constantly =
bringing it up on Slack publicly and via DMs and on the ticket. My guess =
is they were just tired of hearing from me.  And ALL my ticket was =
asking for was a damn hook that many plugins had already added because =
WordPress core had not:

https://core.trac.wordpress.org/ticket/47056

------------
2. More importantly =E2=80=94 and a much higher bar =E2=80=94 WordPress =
core supports PHP 5.6 and so core cannot contain any features that are =
in a later version of PHP.=20

=
https://wptavern.com/wordpress-ends-support-for-php-5-2-5-5-bumps-minimum-=
required-php-version-to-5-6

And don't expect that to change any time soon. WordPress' minimum PHP =
requirements was notoriously 5.2.4 until March 2019 so it is unlikely =
they will bump the PHP requirement before March 2029. =20

(just kidding, but only a bit.)

Of course WordPress currently *recommends* PHP 7.3 and most (all?) =
managed hosts support that so userland developers are free to use newer =
version of WordPress on our own projects and in our plugins and themes.=20=


https://wordpress.org/about/requirements/

But again, WordPress core cannot use any features anytime soon that are =
not in PHP 5.6.

------------
BTW, I did not comment on your is_literal() proposal because I try not =
to comment on things where others are addressing concerns and where I do =
not feel very strongly about adding or avoiding the feature.  I created =
this thread as a new thread to expliclty separate discussions because =
they are orthogonal and I did not want to imply that I supported =
is_literal() as currently proposed.

But since you brought it up let me comment on is_literal().  I recognize =
the problem I think you are trying to solve =E2=80=94 to be able to =
guard against SQL injection attacks =E2=80=94 but I don't think =
is_literal() is a viable solution for that problem.

1. Looking at my code most of my dynamic values use to compose SQL that =
I pass to $wpdb->get_results() is in variables, not literals. In fact I =
rarely use literals.  So I don't see that it would help me (or many =
others?) much at all.

2. Focusing on it being literal or not seems to me to be focusing on =
wrong thing. Something could be non-literal but still be safe. And a =
code hack could introduce a tainted literal (but with a code hack all =
bets are off.) is_taint() makes more sense to me, but even then I am not =
sure it directly addresses the use-case.

3. I feel like we might be better to introduce functionality that =
addresses the exact problem of SQL injection and not one that dances =
around its edges. Maybe we need a MySQL class as an alternative to the =
mysqli functions that has a "safe" property and when that "safe" =
property is true you can't run DDL, TRUNCATE, or DELETE?  Not sure how =
to protect against injection for UPDATE but maybe someone else has an =
idea?

4. Lastly, I think it would be better to specify the problem(s) you are =
trying to solve and then hash out potential solutions on the list rather =
than propose a specific one in advance, maybe even creating an ad-hoc =
working group to come up with a solution that is likely to be accepted?

Anyway, #jmtcw on is_literal().

-Mike


>=20
> Craig
>=20
>=20
>=20
> On Sat, 21 Mar 2020 at 22:53, Mike Schinkel <mike@newclarity.net> =
wrote:
> > On Mar 21, 2020, at 5:59 PM, tyson andre <tysonandre775@hotmail.com> =
wrote:
> > FROM: Re: [PHP-DEV] [RFC] is_literal()
> >=20
> > And if it can be implemented as a PECL module, that would be more =
preferable to me than a core module of php.
> >  If it was in core, having to support that feature may limit =
optimizations or implementation changes that could be done in the =
future.
>=20
> Just wanted to address this comment which was made on another thread =
(I did not want to hijack that thread.)
>=20
> A large number of PHP users have no control over the platform they run =
on, so the option to use PECL modules is a non-starter for them.
>=20
> Here are several of those managed hosting platforms I speak of. =
Collectively they host a large number of WordPress sites, and Pantheon =
also host Drupal sites:
>=20
> https://pagely.com/
> https://wpvip.com/
> https://wpengine.com/
> https://kinsta.com/
> https://pantheon.io/
>=20
> Given that, if there is an option between a useful feature being added =
to core or left in PECL, I would vote 100% of the time for core, since =
working with WordPress on a corporate site I can rarely ever use PECL =
extensions.
>=20
> #fwiw
>=20
> -Mike
> --=20
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>=20