Opened 16 years ago

Closed 16 years ago

#1906 closed Bug (fixed)

PHP connector in filemanager should have better error checking

Reported by: Kyle Owned by: Wiktor Walc
Priority: Normal Milestone: FCKeditor 2.6
Component: Server : PHP Version: FCKeditor 2.5
Keywords: Confirmed Review+ Cc:

Description (last modified by Wojciech Olchawa)

The PHP connector DetectHTML function does no error checking to make sure that the file was opened or read correctly. This causes a cascade of errors on systems with the PHP open_basedir set to disallow opening of files in the temporary file-upload directory. See the forums post

In the file 'editor/filemanager/connectors/php/util.php' starting on line 87 is the DetectHTML function.


function DetectHtml( $filePath )
	$fp = fopen( $filePath, 'rb' ) ;
	$chunk = fread( $fp, 1024 ) ;
	fclose( $fp ) ;

With improved error checking, it should be something like this...

function DetectHtml( $filePath )
	$fp = fopen( $filePath, 'rb' ) ;
        if ( $fp !== false )
         	$chunk = fread( $fp, 1024 ) ;
             if ( $chunk === false )
                     $chunk = '';
	       fclose( $fp ) ;
             $chunk = '';

I'm not sure whether it would be better to return TRUE or FALSE in the case of being unable to open and/or read the file. I leave it to the security experts to debate that.

Attachments (1)

1906.patch (2.3 KB) - added by Wiktor Walc 16 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 16 years ago by Wojciech Olchawa

Description: modified (diff)
Keywords: HasPatch added
Version: SVN

comment:2 Changed 16 years ago by Wiktor Walc

Milestone: FCKeditor 2.6
Owner: set to Wiktor Walc
Status: newassigned
Version: SVNFCKeditor 2.5

Thanks Kyle for posting this here. I'm targeting it to 2.6 because it should be fixed as soon as possible.

If DetectHtml() fail to open the uploaded file, it will be checked once again after moving it to the final directory. If the file is invalid, FCKeditor will make an attempt do delete it.

Changed 16 years ago by Wiktor Walc

Attachment: 1906.patch added

comment:3 Changed 16 years ago by Wiktor Walc

Keywords: Confirmed Review? added; HasPatch removed

comment:4 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

I haven't tested it throughly, but the implementation seems ok.

I would just change line 240: "if" => "else if".

Then, if you have tested it well Wiktor, go ahead committing it. Please include a changelog entry for it.

comment:5 Changed 16 years ago by Wiktor Walc

Fixed with [1615].

comment:6 Changed 16 years ago by Wiktor Walc

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy