HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.

YAGA - Fatal Error in PHP.gdn_ErrorHandler();

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 :)

Comments

  • 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_JR_J Ex-Fanboy Munich Admin

    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();
      }
    


  • 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 :)

  • neptronixneptronix Utah New
    edited January 2019

    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..

  • 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

  • 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.

  • 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.

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

    Nice link.

  • 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.

Sign In or Register to comment.