We have moved our forum to GitHub Discussions. For questions about Phalcon v3/v4/v5 you can visit here and for Phalcon v6 here.
  1. Home
  2. Beginners

Binding parameters when saving which are HTML strings

Hi,

With regards to SQL injection and XSS prevention techniques I was wondering if its possible to bind a html string as a parameter when saving to a model.

This is my code:

    public function myAction($pageId){
    $pageId = $this->filter->sanitize($pageId, 'int');

        $pageLookup = Page::findFirst(
            "id = " . $pageId
        );

        if ($this->request->isPost()) {

            $purifier = new \HTMLPurifier();
            $editorData = $this->request->getPost("editPageEditor" );
            $cleanedEditorData = $purifier->purify($editorData);

            $pageLookup->content = $cleanedEditorData;
            $pageLookup->save();
        }

        $this->view->page = $pageLookup;
    }

These are the steps I have so used far:

  1. First on lookup I sanitize the parameter to ensure the page id is an int.
  2. The view uses ckeditor to produce some html which is then passed back to the controller on submit.
  3. With the help of html purfifer I clean the html string.
  4. I then update the content property of the page model with the cleaned string.
  5. And commit any changes.

I know I can do something similar to this but this strips out all html at this point: $this->request->getPost("editPageEditor","string" );

Is there anything I can do to retain the cleaned html string when binding or this step more than is required?



77.7k
Accepted
answer 
$pageLookup = Page::findFirst("id = " . $pageId);

Never concatenate parameters to SQL queries, even if it was sanitized to your desired type beforehand. Use binding parameters here also:

// magic method provided by Phalcon ORM on every model
$pageLookup = Page::findFirstById($pageId);
// the above basically expands to this:
$pageLookup = Page::find([
    'conditions'=>'id = :id:',
    'bind'=>['id'=>$pageId]
]);
// no need to sanitize it as a string, your purifier will be sufficient
$editorData = $this->request->getPost("editPageEditor");
$purifier = new \HTMLPurifier();
$cleanedEditorData = $purifier->purify($editorData);
$pageLookup->content = $cleanedEditorData;
$pageLookup->save();

The above code protects you from SQL injection, since the content property will be bound as a parameter in the final SQL query.

Regarding XSS attacks, you should clean any script in the html purifier step, and you're all set.



145.0k

If id is primary key just do:

Page::findFirst($pageId)

Easy as that.



77.7k
in reply to Wojciech Ślawski

There is one important caveat to this: if the $pageId variable is NULL, it will return the first record it finds, not an empty result... i've had a sleepless night debugging it ;]

If id is primary key just do:

Page::findFirst($pageId)

Easy as that.



1.5k
in reply to Lajos Bencz

Thanks for your reply.

Does the findById method automatically bind parameters?

$pageLookup = Page::findFirst("id = " . $pageId);

Never concatenate parameters to SQL queries, even if it was sanitized to your desired type beforehand. Use binding parameters here also:

// magic method provided by Phalcon ORM on every model
$pageLookup = Page::findFirstById($pageId);
// the above basically expands to this:
$pageLookup = Page::find([
  'conditions'=>'id = :id:',
  'bind'=>['id'=>$pageId]
]);
// no need to sanitize it as a string, your purifier will be sufficient
$editorData = $this->request->getPost("editPageEditor");
$purifier = new \HTMLPurifier();
$cleanedEditorData = $purifier->purify($editorData);
$pageLookup->content = $cleanedEditorData;
$pageLookup->save();

The above code protects you from SQL injection, since the content property will be bound as a parameter in the final SQL query.

Regarding XSS attacks, you should clean any script in the html purifier step, and you're all set.



77.7k

Yes, it does



145.0k
edited Sep '17

But much easier is just do Page::findFirst($pageId), no need to write Page::findFirstById($pageId) it does the same job and it's faster.



77.7k

@Jurigag yes, that's true, but imho it is always better to be explicit when writing code ;]