On March 26, 2019 we launched new MODX Forums. Please join us at the new MODX Community Forums.
Subscribe: RSS
  • Hello,

    I'm very new to xpdo and I need to create a multi input custom search for a site that I'm building.

    I've written a basic snippet as a proof of principle which works fine but I would really appreciate feedback as to whether it is safe and sane.

    Any comments welcome, don't hold back.

    Here it is
    <?php
    $surname = mysql_real_escape_string(strip_tags($_POST['surname']));
    $speciality = mysql_real_escape_string(strip_tags($_POST['speciality']));
    $keyword = mysql_real_escape_string(strip_tags($_POST['keyword']));
    $output = '';
    $sql = "SELECT * FROM modx_site_content WHERE parent=2 AND pagetitle LIKE '%$surname%' AND content LIKE '%$speciality%' AND content LIKE '%$keyword%'";
    foreach ($modx->query($sql) as $row) {
        $output .= $row['pagetitle'] .'<br/>';
    }
    return $output;
    
    • Two things come to mind:

      - if you are only going to be using pagetitle then you could "SELECT pagetitle FROM..." (a very minor thing)
      - if each of the search criteria is a manageable list (ie not hundreds of possible values) it may be worthwhile to build your search criteria as drop-downs and populate them with the list of values from the database. That way you give the users a better experience and you can replace the LIKEs with =.

      Ok. Three things. Put your output in a chunk and use $modx->getchunk().
      • This function is called on every request, so it may not be necessary to sanitize the post values (though it can't hurt):

        public function sanitizeRequest() {
                $modxtags = array_values($this->modx->sanitizePatterns);
                modX :: sanitize($_GET, $modxtags);
                if ($this->modx->getOption('allow_tags_in_post',null,true)) {
                    modX :: sanitize($_POST);
                } else {
                    modX :: sanitize($_POST, $modxtags);
                }
                modX :: sanitize($_COOKIE, $modxtags);
                modX :: sanitize($_REQUEST, $modxtags);
                $rAlias = $this->modx->getOption('request_param_alias', null, 'q');
                if (isset ($_GET[$rAlias])) {
                    $_GET[$rAlias] = preg_replace("/[^A-Za-z0-9_\-\.\/]/", "", $_GET[$rAlias]);
                }
            }


        If you want to do it yourself, it's easier (and maybe safer) to just do this at the top:

        foreach ($_POST as $k => $v) {
            $_POST[$k] = $modx->sanitizeString($v);
        }
          Did I help you? Buy me a beer
          Get my Book: MODX:The Official Guide
          MODX info for everyone: http://bobsguides.com/modx.html
          My MODX Extras
          Bob's Guides is now hosted at A2 MODX Hosting
        • Quote from: gissirob at Oct 22, 2015, 05:35 PM
          Two things come to mind:

          - if you are only going to be using pagetitle then you could "SELECT pagetitle FROM..." (a very minor thing)
          - if each of the search criteria is a manageable list (ie not hundreds of possible values) it may be worthwhile to build your search criteria as drop-downs and populate them with the list of values from the database. That way you give the users a better experience and you can replace the LIKEs with =.

          Ok. Three things. Put your output in a chunk and use $modx->getchunk().

          Thank you Gissirob,

          Yes - this is exactly what I intended to do. I was going to place the output in a chunk but, the current code was just for testing. I will check out getchunk!
          • Quote from: BobRay at Oct 22, 2015, 08:25 PM
            This function is called on every request, so it may not be necessary to sanitize the post values (though it can't hurt):

            public function sanitizeRequest() {
                    $modxtags = array_values($this->modx->sanitizePatterns);
                    modX :: sanitize($_GET, $modxtags);
                    if ($this->modx->getOption('allow_tags_in_post',null,true)) {
                        modX :: sanitize($_POST);
                    } else {
                        modX :: sanitize($_POST, $modxtags);
                    }
                    modX :: sanitize($_COOKIE, $modxtags);
                    modX :: sanitize($_REQUEST, $modxtags);
                    $rAlias = $this->modx->getOption('request_param_alias', null, 'q');
                    if (isset ($_GET[$rAlias])) {
                        $_GET[$rAlias] = preg_replace("/[^A-Za-z0-9_\-\.\/]/", "", $_GET[$rAlias]);
                    }
                }


            If you want to do it yourself, it's easier (and maybe safer) to just do this at the top:

            foreach ($_POST as $k => $v) {
                $_POST[$k] = $modx->sanitizeString($v);
            }

            Thanks BobRay,

            This is what I was hoping would happen. Other posts suggested that query would sanitize the data automatically but they were a bit vague.
            Thanks for the sanitizeString loop. I will use this as I have had problems with using mysql_real_escape_string on the production machine.

            I've done some reading and there is a lot of talk about prepared statements. Do you think that prepared statements should be used as well to prevent my sql string being hyjacked?.

            If so does anyone know how to do this using xpdo.query?
            • Prepared queries are always a good idea, though they're a little extra trouble. You can always call $query->prepare(), but if you want to do a true parameterized query, I think you'd have to use PDO directly. I could be wrong.

              This might help: http://bobsguides.com/blog.html/2014/12/17/using-parameterized-prepared-statements-to-retrieve-data/.
                Did I help you? Buy me a beer
                Get my Book: MODX:The Official Guide
                MODX info for everyone: http://bobsguides.com/modx.html
                My MODX Extras
                Bob's Guides is now hosted at A2 MODX Hosting