Re: [PHP-DEV] Are PECL modules preferable?

This is only part of a thread. view whole thread
  109195
March 21, 2020 23:15 craig@craigfrancis.co.uk (Craig Francis)
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.
Thanks Mike, 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). --- As you mention working with WordPress, I've seen a couple of developers who have taken examples like: $posts = $wpdb->get_results("SELECT ... WHERE post_type='post'"); Then edited it to something dangerous like: $posts = $wpdb->get_results("SELECT ... WHERE post_type='" . $_GET['type'] . "'"); To guard against this, do you think that WordPress could update their get_results() function to do something like: public function get_results( $query = null, $output = OBJECT ) { if (!is_literal($sql)) { trigger_error('This is an unsafe $query, please use $wpdb->prepare()', E_USER_NOTICE); } Perhaps with a better message; then, over the years, increase the warning level? I think that would be a very useful way of getting developers aware of these dangers. Craig 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() > > > > 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. > > Just wanted to address this comment which was made on another thread (I > did not want to hijack that thread.) > > 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. > > 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: > > https://pagely.com/ > https://wpvip.com/ > https://wpengine.com/ > https://kinsta.com/ > https://pantheon.io/ > > 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. > > #fwiw > > -Mike > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
  109209
March 22, 2020 19:10 mike@newclarity.net (Mike Schinkel)
> On Mar 21, 2020, at 7:15 PM, Craig Francis <craig@craigfrancis.co.uk> wrote: > > 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. > > > Thanks Mike, > > 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: > > $posts = $wpdb->get_results("SELECT ... WHERE post_type='post'"); > > Then edited it to something dangerous like: > > $posts = $wpdb->get_results("SELECT ... WHERE post_type='" . $_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: > > public function get_results( $query = null, $output = OBJECT ) { > if (!is_literal($sql)) { > trigger_error('This is an unsafe $query, please use $wpdb->prepare()', E_USER_NOTICE); > } > > Perhaps with a better message; then, over the years, increase the warning level? > > 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. 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 — and a much higher bar — WordPress core supports PHP 5.6 and so core cannot contain any features that are in a later version of PHP. 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. (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. 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 — to be able to guard against SQL injection attacks — 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
> > Craig > > > > 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() > > > > 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. > > Just wanted to address this comment which was made on another thread (I did not want to hijack that thread.) > > 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. > > 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: > > https://pagely.com/ > https://wpvip.com/ > https://wpengine.com/ > https://kinsta.com/ > https://pantheon.io/ > > 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. > > #fwiw > > -Mike > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php >