YAGA - Fatal Error in PHP.gdn_ErrorHandler();

losttheplot

Hi, After installing the latest copy of Yaga from GitHub (including the date fixes), we get the following error when trying to access the admin profile page:

Fatal Error in PHP.gdn_ErrorHandler();

In aggregated query without GROUP BY, expression #1 of SELECT list contains nonaggregated column '*****.Reaction.ReactionID'; this is incompatible with sql_mode=only_full_group_by

Application: Vanilla
Application Version: 2.6.4
PHP Version: 7.1.26
Operating System: Linux
Server Software: Apache/2.4.6 (CentOS)
Request Uri: /profile
Controller: PHP
Method: gdn_ErrorHandler

We are running mysql as follows: 5.7.23-23-57-log - Percona XtraDB Cluster (GPL), Release rel23, Revision f5578f0, WSREP version 31.31, wsrep_31.31

The issue is probably due to Percona's strictness with regards to SQL statements, but is nonetheless a sign that the query is incorrect. We are not familiar enough with vanilla or Yaga to be able to fix this, but if the author can do so it would be very much appreciated.

This StackOverflow page explains the problem in detail and the steps needed to fix it > https://stackoverflow.com/questions/43481869/aggregated-query-without-group-by

Thanks in anticipation

pioc34

https://github.com/hgtonight/Application-Yaga/issues/144

losttheplot

Thanks, but if I ask my sysadmin to reconfigure our Percona clusters to suit a (now) invalid sql query, I already know what he'll say: "Fix the code!"

So this particular query is put together by the GetUserCount() method at line 119 of /html/applications/yaga/models/class.reactionmodel.php as follows:

public function GetUserCount($UserID, $ActionID) {
  return $this->SQL
      ->Select()
      ->From('Reaction')
      ->Where('ActionID', $ActionID)
      ->Where('ParentAuthorID', $UserID)
      ->GetCount();
  }

Rightly or wrongly, I don't use query abstraction tools, favouring a more direct approach. So at first glance this seems to work, and doesn't trigger any errors:

 public function GetUserCount($UserID, $ActionID) {
  $Sql = 'SELECT count(ActionID) AS `RowCount` 
      FROM `GDN_Reaction` `Reaction` 
      WHERE `ActionID` = :ActionID 
      AND `ParentAuthorID` = :ParentAuthorID 
      GROUP BY ActionID';
  return $this->Database->Query($Sql, array(':ActionID' => $ActionID, ':ParentAuthorID' => $UserID))->Result();
 }

Clearly I will need to apply a similar solution to other methods using GetCount(), and look for other possible side-effects, but it's a start, and when I get time to do this properly, I'll post back with my more considered findings.

R_J

Instead of rewriting the complete SQL, you might want to shorten that process.

This should be an adequate replacement, too:

public function GetUserCount($UserID, $ActionID) {
  return $this->SQL
      ->Select('ActionID')
      ->From('Reaction')
      ->Where('ActionID', $ActionID)
      ->Where('ParentAuthorID', $UserID)
      ->GetCount();
  }

losttheplot

Like I said, I'm not familiar with the syntax of whatever ORM is in use here, so thanks for that; I'll give it a go and see what exact query comes out the other end :)

neptronix

You should use the ORM if you can. It's critical for preventing SQL injection. YAGA probably uses this as the primary means of preventing SQL injections, thus you'd be removing that and creating a hole on your site unless other countermeasures are used. Just so you know..

losttheplot

Thank you for your well-intended advice, neptronix, and I have no wish to start a flame-war, but an ORM is not in any way critical for preventing SQL injection as long as PDO is used correctly, as per my suggested fix. I don't use an ORM because I understand coal-face SQL and how to use indexes, and I prefer to avoid additional layers of abstraction if at all possible. Speed and simplicity are my watchwords. This article (the first one I found on the subject) explains the pros and cons of ORMs, and note that SQL injection is not an issue in either list > https://medium.com/@mithunsasidharan/should-i-or-should-i-not-use-orm-4c3742a639ce

neptronix

I write PHP software for a living and agree with you. We have some tricky code for handling SQL injection issues using raw queries, but they have some quirks and i would not advise anyone else to follow my way. Yes, ORM imposes a performance penalty, code testability penalty, has a weird syntax, etc..

When something is designed to work with ORM, i just wouldn't break it's critical security feature without substituting another. That's all.

losttheplot

You shouldn't need any tricky code for handling SQL injection issues; PDO is totally safe out of the box - provided you use prepared statements - and it's really very simple to understand and use > https://phpdelusions.net/pdo#prepared

As for the YAGA Class that I made my fix in, if you look in the GetList() method, you'll find PDO being used in exactly the same way as my fix suggestion.

neptronix

My apologies; i was unaware that you were using PDO in that example. I have never seen it used that way.

Nice link.

losttheplot

No worries; I appreciate that you were only trying to help :)

Basically, any time you see a word beginning with a colon (e.g. :ActionID) as part of what otherwise looks like a normal SQL query, you know that prepared statements are being used with PDO. What my fix above is doing, is removing the ORM from the loop (because I have no idea what exact query will result), although the ORM is still technically being used here to pass the statement to the PDO handler, because that's the way the application has been structured to work.

Going back to your comment about having "some tricky code for handling SQL injection issues using raw queries", if you choose not to use an ORM (like me) then I really would suggest that you take the time to learn about PDO as it will solve all your potential injection issues.