Jelajahi Sumber

#970 and #650 Corrupted attachements. Reworked the cleanup of undesired output, to protect it against the case when the output buffer is unfortunately closed. On the other hand, I found out that several output buffer can be stacked. Thus the protection could be further improved (difficulty: that can be web server dependent).

git-svn-id: http://svn.code.sf.net/p/itop/code/trunk@3376 a333f486-631f-4898-b8df-5754b55c2be0
romainq 10 tahun lalu
induk
melakukan
8037809342

+ 3 - 5
application/ajaxwebpage.class.inc.php

@@ -1,5 +1,5 @@
 <?php
-// Copyright (C) 2010-2012 Combodo SARL
+// Copyright (C) 2010-2014 Combodo SARL
 //
 //   This file is part of iTop.
 //
@@ -225,9 +225,8 @@ EOF
 EOF
 			);
 		}	
-        $s_captured_output = ob_get_contents();
-        ob_end_clean();
-        if (($this->sContentType == 'text/html') &&  ($this->sContentDisposition == 'inline'))
+        $s_captured_output = $this->ob_get_clean_safe();
+		  if (($this->sContentType == 'text/html') &&  ($this->sContentDisposition == 'inline'))
         {
         	// inline content != attachment && html => filter all scripts for malicious XSS scripts
         	echo self::FilterXSS($this->s_content);
@@ -393,4 +392,3 @@ EOF
 	}
 }
 
-?>

+ 6 - 3
application/csvpage.class.inc.php

@@ -1,5 +1,5 @@
 <?php
-// Copyright (C) 2010-2012 Combodo SARL
+// Copyright (C) 2010-2014 Combodo SARL
 //
 //   This file is part of iTop.
 //
@@ -39,7 +39,11 @@ class CSVPage extends WebPage
 
     public function output()
     {
-		$this->add_header("Content-Length: ".strlen(trim($this->s_content)));
+			$this->add_header("Content-Length: ".strlen(trim($this->s_content)));
+
+			// Get the unexpected output but do nothing with it
+			$sTrash = $this->ob_get_clean_safe();
+
         foreach($this->a_headers as $s_header)
         {
             header($s_header);
@@ -105,4 +109,3 @@ class CSVPage extends WebPage
 	}
 }
 
-?>

+ 1 - 3
application/itopwebpage.class.inc.php

@@ -587,8 +587,7 @@ EOF
 				header($s_header);
 			}
 		}
-		$s_captured_output = ob_get_contents();
-		ob_end_clean();
+		$s_captured_output = $this->ob_get_clean_safe();
 		$sHtml = "<!DOCTYPE html PUBLIC \"-//W3C//DTD XHTML 1.0 Strict//EN\" \"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd\">\n";
 		$sHtml .= "<html>\n";
 		$sHtml .= "<head>\n";
@@ -1039,4 +1038,3 @@ EOF
 			$this->m_sMessage = $sMessage;
 	}
 }
-?>

+ 46 - 11
application/webpage.class.inc.php

@@ -1,5 +1,5 @@
 <?php
-// Copyright (C) 2010-2013 Combodo SARL
+// Copyright (C) 2010-2014 Combodo SARL
 //
 //   This file is part of iTop.
 //
@@ -68,6 +68,7 @@ class WebPage implements Page
 	protected $sContentType;
 	protected $sContentDisposition;
 	protected $sContentFileName;
+	protected $bTrashUnexpectedOutput;
 	protected $s_sOutputFormat;
 	protected $a_OutputOptions;
 	    
@@ -88,6 +89,7 @@ class WebPage implements Page
         $this->sContentType = '';
         $this->sContentDisposition = '';
         $this->sContentFileName = '';
+		$this->bTrashUnexpectedOutput = false;
 		$this->s_OutputFormat = utils::ReadParam('output_format', 'html');
 		$this->a_OutputOptions = array();
         ob_start(); // Start capturing the output
@@ -392,22 +394,55 @@ class WebPage implements Page
 	}
 	
 	/**
-	 * Discard unexpected output data
+	 * Discard unexpected output data (such as PHP warnings)
 	 * This is a MUST when the Page output is DATA (download of a document, download CSV export, download ...)
 	 */	
 	public function TrashUnexpectedOutput()
 	{
-		// This protection is redundant with a protection implemented in MetaModel::IncludeModule
-		// which detects such issues while loading module files
-		// Here, the purpose is to detect and discard characters produced by the code execution (echo)
-		$sPreviousContent = ob_get_clean();
-		if (trim($sPreviousContent) != '')
+		$this->bTrashUnexpectedOutput = true;
+	}
+
+	/**
+	 * Read the output buffer and deal with its contents:
+	 * - trash unexpected output if the flag has been set
+	 * - report unexpected behaviors such as the output buffering being stopped
+	 * 
+	 * Possible improvement: I've noticed that several output buffers are stacked,
+	 * if they are not empty, the output will be corrupted. The solution would
+	 * consist in unstacking all of them (and concatenate the contents).	 	 
+	 */
+	protected function ob_get_clean_safe()
+	{
+		$sOutput = ob_get_contents();
+		if ($sOutput === false)
 		{
-			if (Utils::GetConfig() && Utils::GetConfig()->Get('debug_report_spurious_chars'))
+			$sMsg = "Design/integration issue: No output buffer. Some piece of code has called ob_get_clean() or ob_end_clean() without calling ob_start()";
+			if ($this->bTrashUnexpectedOutput)
 			{
-				IssueLog::Error("Output already started before downloading file:\nContent was:'$sPreviousContent'\n");
+				IssueLog::Error($sMsg);
+				$sOutput = '';
+			}
+			else
+			{
+				$sOutput = $sMsg;
 			}
 		}
+		else
+		{
+			ob_end_clean(); // on some versions of PHP doing so when the output buffering is stopped can cause a notice
+			if ($this->bTrashUnexpectedOutput)
+			{
+				if (trim($sOutput) != '')
+				{
+					if (Utils::GetConfig() && Utils::GetConfig()->Get('debug_report_spurious_chars'))
+					{
+						IssueLog::Error("Trashing unexpected output:'$s_captured_output'\n");
+					}
+				}
+				$sOutput = '';
+			}
+		}
+		return $sOutput;
 	}
 
 	/**
@@ -419,8 +454,8 @@ class WebPage implements Page
         {
             header($s_header);
         }
-        $s_captured_output = ob_get_contents();
-        ob_end_clean();
+
+        $s_captured_output = $this->ob_get_clean_safe();
         echo "<!DOCTYPE html PUBLIC \"-//W3C//DTD XHTML 1.0 Strict//EN\" \"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd\">\n";
         echo "<html>\n";
         echo "<head>\n";

+ 5 - 12
application/xmlpage.class.inc.php

@@ -1,5 +1,5 @@
 <?php
-// Copyright (C) 2010-2013 Combodo SARL
+// Copyright (C) 2010-2014 Combodo SARL
 //
 //   This file is part of iTop.
 //
@@ -51,6 +51,9 @@ class XMLPage extends WebPage
 	{
 		if (!$this->m_bPassThrough)
 		{
+			// Get the unexpected output but do nothing with it
+			$sTrash = $this->ob_get_clean_safe();
+					
 			$this->s_content = "<?xml version=\"1.0\" encoding=\"UTF-8\"?".">\n".trim($this->s_content);
 			$this->add_header("Content-Length: ".strlen($this->s_content));
 			foreach($this->a_headers as $s_header)
@@ -79,8 +82,7 @@ class XMLPage extends WebPage
 			}
 			else
 			{
-				$s_captured_output = ob_get_contents();
-				ob_end_clean();
+				$s_captured_output = $this->ob_get_clean_safe();
 				foreach($this->a_headers as $s_header)
 				{
 					header($s_header);
@@ -101,13 +103,4 @@ class XMLPage extends WebPage
 	public function table($aConfig, $aData, $aParams = array())
 	{
 	}
-
-	public function TrashUnexpectedOutput()
-	{
-		if (!$this->m_bPassThrough)
-		{
-			parent::TrashUnexpectedOutput();
-		}
-	}
 }
-?>