From a895cdcdf8c25800670dfeb7188c2c21a703f685 Mon Sep 17 00:00:00 2001
From: Laurent Destailleur <eldy@destailleur.fr>
Date: Thu, 17 Sep 2020 21:09:16 +0200
Subject: [PATCH] Fix dol_string_nohtmltag: the decode of entity must be done
 before split

---
 htdocs/admin/mails_templates.php  |  2 +-
 htdocs/core/lib/functions.lib.php | 11 ++++++-----
 htdocs/document.php               |  3 ++-
 test/phpunit/SecurityTest.php     |  9 +++++++--
 4 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/htdocs/admin/mails_templates.php b/htdocs/admin/mails_templates.php
index ea36b25e3ce..b33741cd571 100644
--- a/htdocs/admin/mails_templates.php
+++ b/htdocs/admin/mails_templates.php
@@ -284,7 +284,6 @@ if (empty($reshook))
 				} else {
 					$sql .= "'".$db->escape(GETPOST($keycode, 'nohtml'))."'";
 				}
-
 				$i++;
 			}
 			$sql .= ", 1)";
@@ -341,6 +340,7 @@ if (empty($reshook))
 
 				$i++;
 			}
+
 			$sql .= " WHERE ".$rowidcol." = '".$rowid."'";
 			//print $sql;exit;
 			dol_syslog("actionmodify", LOG_DEBUG);
diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php
index 2604253d01b..248beb0959d 100644
--- a/htdocs/core/lib/functions.lib.php
+++ b/htdocs/core/lib/functions.lib.php
@@ -5486,23 +5486,24 @@ function dol_string_nohtmltag($stringtoclean, $removelinefeed = 1, $pagecodeto =
 	if ($removelinefeed == 2) $stringtoclean = preg_replace('/<br[^>]*>(\n|\r)+/ims', '<br>', $stringtoclean);
 	$temp = preg_replace('/<br[^>]*>/i', "\n", $stringtoclean);
 
+	// We remove entities BEFORE stripping (in case of a separator char is encoded and not the other, the strip will fails)
+	$temp = dol_html_entity_decode($temp, ENT_COMPAT, $pagecodeto);
+
 	if ($strip_tags) {
 		$temp = strip_tags($temp);
 	} else {
 		$pattern = "/<[^<>]+>/";
-		// Exemple of $temp: <a href="/myurl" title="<u>A title</u>">0000-021</a>
+		// Example of $temp: <a href="/myurl" title="<u>A title</u>">0000-021</a>
 		$temp = preg_replace($pattern, "", $temp); // pass 1
 		// $temp after pass 1: <a href="/myurl" title="A title">0000-021
 		$temp = preg_replace($pattern, "", $temp); // pass 2
 		// $temp after pass 2: 0000-021
 	}
 
-	$temp = dol_html_entity_decode($temp, ENT_COMPAT, $pagecodeto);
-
-	// Supprime aussi les retours
+	// Remove also CR LF
 	if ($removelinefeed == 1) $temp = str_replace(array("\r\n", "\r", "\n"), " ", $temp);
 
-	// et les espaces doubles
+	// and double spaces
 	while (strpos($temp, "  "))
 	{
 		$temp = str_replace("  ", " ", $temp);
diff --git a/htdocs/document.php b/htdocs/document.php
index d4d2b5fb909..ab3ad20a8f8 100644
--- a/htdocs/document.php
+++ b/htdocs/document.php
@@ -158,7 +158,8 @@ if (GETPOST('type', 'alpha')) $type = GETPOST('type', 'alpha');
 else $type = dol_mimetype($original_file);
 // Security: Force to octet-stream if file is a dangerous file. For example when it is a .noexe file
 // We do not force if file is a javascript to be able to get js from website module with <script src="
-if (! in_array($type, array('text/x-javascript')) && !dolIsAllowedForPreview($original_file)) {
+// Note: Force whatever is $modulepart seems ok.
+if (!in_array($type, array('text/x-javascript')) && !dolIsAllowedForPreview($original_file)) {
 	$type = 'application/octet-stream';
 }
 
diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php
index 50ae74e5031..303fd272601 100644
--- a/test/phpunit/SecurityTest.php
+++ b/test/phpunit/SecurityTest.php
@@ -176,6 +176,7 @@ class SecurityTest extends PHPUnit\Framework\TestCase
         $_GET["param3"]='"a/b#e(pr)qq-rr\cc';    // Same than param2 + "
         $_GET["param4"]='../dir';
         $_GET["param5"]="a_1-b";
+        $_POST["param6"]="&quot;&gt;<svg o&#110;load='console.log(&quot;Stored XSS &quot;)'&gt;";
 
         // Test int
         $result=GETPOST('id', 'int');              // Must return nothing
@@ -218,11 +219,15 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 
         $result=GETPOST("param4", 'aZ09');  // Must return '' as string contains car not in aZ09 definition
         print __METHOD__." result=".$result."\n";
-        $this->assertEquals($result, '');
+        $this->assertEquals('', $result);
 
         $result=GETPOST("param5", 'aZ09');
         print __METHOD__." result=".$result."\n";
-        $this->assertEquals($result, $_GET["param5"]);
+        $this->assertEquals($_GET["param5"], $result);
+
+        $result=GETPOST("param6", 'nohtml');
+        print __METHOD__." result=".$result."\n";
+        $this->assertEquals('">', $result);
 
         return $result;
     }