Project

General

Profile

Bug #3055

Display::img() method: Remove Security::remove_XSS(), it is useless there.

Added by Ivan Tcholakov almost 8 years ago. Updated almost 8 years ago.

Status:
Feature implemented
Priority:
Normal
Category:
-
Target version:
Start date:
05/03/2011
Due date:
% Done:

100%

Estimated time:
Complexity:
Normal
SCRUM pts - complexity:
?

Description

Security::remove_XSS() is a heavy routine, we should be careful to use where it is really needed. See the method Dispaly::img():

    public static function img($image_path, $alt_text = '', $additional_attributes = array()) {
        $attribute_list = '';
        // alt text = the image name if there is none provided (for XHTML compliance)
        if ($alt_text == '') {
            $alt_text = basename($image_path);
        } 
        $image_path = Security::remove_XSS($image_path);

        $additional_attributes['src']   = $image_path;

        if (empty($additional_attributes['alt'])) {
            $additional_attributes['alt']   = $alt_text;
        }
        if (empty($additional_attributes['title'])) {
            $additional_attributes['title'] = $alt_text;
        }        
        //return '<img src="'.$image_path.'" alt="'.$alt_text.'"  title="'.$alt_text.'" '.$attribute_list.' />';
        return self::tag('img','',$additional_attributes);        
    }

Security::remove_XSS() is used here, which means that the HTMLPurifier is to be activated on every rendered icon. Imagine a page with a long table with many action links on the most right column.

Using Security::remove_XSS() in this low-level method is not needed here, because we as programmers control the what $image_path is passed as parameter. If (in theory) the $image_path depends on user's input, then we should filter that user's input.

I suggest the line

$image_path = Security::remove_XSS($image_path);
to be removed.

See also my experiment here: http://support.chamilo.org/issues/1297#note-61

History

#1

Updated by Ivan Tcholakov almost 8 years ago

  • Status changed from New to Assigned
  • Assignee set to Ivan Tcholakov
  • Target version set to 1.8.8 beta
#2

Updated by Ivan Tcholakov almost 8 years ago

  • Status changed from Assigned to Needs more info
  • Assignee changed from Ivan Tcholakov to Julio Montoya
  • % Done changed from 0 to 90

14107:045c5728309b Task #3055 - Additional check has been added, see the previous transaction.
http://code.google.com/p/chamilo/source/detail?r=045c5728309b53421a87d368141c47a2659a98d5&repo=classic

14106:7f86af6a837c Task #3055 - Implementing lighter sanitation for the input parameter $image_path, method Display::img().
http://code.google.com/p/chamilo/source/detail?r=7f86af6a837ce03b6ce172b76be7891cd52221e2&repo=classic

14105:a764d17c57aa Task #3055 - Cleaning whitespace in the file display.lib.php.
http://code.google.com/p/chamilo/source/detail?r=a764d17c57aabb694b6e97e4cbe3e4b6193b6e96&repo=classic

#3

Updated by Julio Montoya almost 8 years ago

  • Assignee changed from Julio Montoya to Ivan Tcholakov

Agree with those changes, thanks!

#4

Updated by Ivan Tcholakov almost 8 years ago

  • Status changed from Needs more info to Assigned

In this case I am going to check/re-thing carefully this protection again and to move it in the Security class, in a method for example Security::filter_img_path($img_path).

14109:933a771f02d9 Task #3055 - Whitespace cleaning in the file security.lib.php.
http://code.google.com/p/chamilo/source/detail?r=933a771f02d93255d515bbebc9fd4030928c5898&repo=classic

#5

Updated by Ivan Tcholakov almost 8 years ago

  • Status changed from Assigned to Feature implemented
  • % Done changed from 90 to 100

14110:d10de387cc00 Task #3055 - Implementing the method Security::filter_img_path().
http://code.google.com/p/chamilo/source/detail?r=d10de387cc00156938fd18ec9a4f76362661ca97&repo=classic

Also available in: Atom PDF