From 7b093a6bba505810d00f7937754e26e626f829b3 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Wed, 3 Mar 2021 11:53:50 +0100 Subject: [PATCH 01/10] =?UTF-8?q?N=C2=B03671=20-=20app=5Froot=5Furl:=20han?= =?UTF-8?q?dle=20reverse=20proxies=20during=20the=20setup=20and=20preserve?= =?UTF-8?q?=20existing=20configuration=20during=20an=20upgrade.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/utils.inc.php | 134 +++++++++++++++++++++++------ core/config.class.inc.php | 8 ++ setup/setuppage.class.inc.php | 2 +- setup/wizardsteps.class.inc.php | 24 +++--- test/application/UtilsTest.php | 147 ++++++++++++++++++++++++++++++++ 5 files changed, 274 insertions(+), 41 deletions(-) diff --git a/application/utils.inc.php b/application/utils.inc.php index 92915bfc2..443bf3781 100644 --- a/application/utils.inc.php +++ b/application/utils.inc.php @@ -782,14 +782,29 @@ class utils } } + public static function IsProxyTrusted() + { + if (empty($_SERVER['REMOTE_ADDR'])) { + return false; + } + + $bTrustProxies = (bool) self::GetConfig()->Get('trust_proxies'); + + return $bTrustProxies; + } + /** * Returns the absolute URL to the application root path * + * @param bool $bForceTrustProxy + * * @return string The absolute URL to the application root, without the first slash * * @throws \Exception + * + * @since 2.7.4 $bForceTrustProxy param added */ - public static function GetAbsoluteUrlAppRoot() + public static function GetAbsoluteUrlAppRoot($bForceTrustProxy = false) { static $sUrl = null; if ($sUrl === null) @@ -797,7 +812,7 @@ class utils $sUrl = self::GetConfig()->Get('app_root_url'); if ($sUrl == '') { - $sUrl = self::GetDefaultUrlAppRoot(); + $sUrl = self::GetDefaultUrlAppRoot($bForceTrustProxy); } elseif (strpos($sUrl, SERVER_NAME_PLACEHOLDER) > -1) { @@ -821,40 +836,31 @@ class utils * For most usages, when an root url is needed, use utils::GetAbsoluteUrlAppRoot() instead as uses this only as a fallback when the * app_root_url conf parameter is not defined. * + * @param bool $bForceTrustProxy + * * @return string * * @throws \Exception - */ - public static function GetDefaultUrlAppRoot() + * + * @since 2.7.4 $bForceTrustProxy param added + */ + public static function GetDefaultUrlAppRoot($bForceTrustProxy = false) { // Build an absolute URL to this page on this server/port - $sServerName = isset($_SERVER['SERVER_NAME']) ? $_SERVER['SERVER_NAME'] : ''; - $sProtocol = self::IsConnectionSecure() ? 'https' : 'http'; - $iPort = isset($_SERVER['SERVER_PORT']) ? $_SERVER['SERVER_PORT'] : 80; - if ($sProtocol == 'http') - { - $sPort = ($iPort == 80) ? '' : ':'.$iPort; - } - else + $sServerName = self::GetServerName($bForceTrustProxy); + $bIsSecure = self::IsConnectionSecure($bForceTrustProxy) ; + $sProtocol = $bIsSecure ? 'https' : 'http'; + $iPort = self::GetServerPort($bForceTrustProxy); + if ($bIsSecure) { $sPort = ($iPort == 443) ? '' : ':'.$iPort; } - // $_SERVER['REQUEST_URI'] is empty when running on IIS - // Let's use Ivan Tcholakov's fix (found on www.dokeos.com) - if (!empty($_SERVER['REQUEST_URI'])) - { - $sPath = $_SERVER['REQUEST_URI']; - } else { - $sPath = $_SERVER['SCRIPT_NAME']; - if (!empty($_SERVER['QUERY_STRING'])) - { - $sPath .= '?'.$_SERVER['QUERY_STRING']; - } - $_SERVER['REQUEST_URI'] = $sPath; + $sPort = ($iPort == 80) ? '' : ':'.$iPort; } - $sPath = $_SERVER['REQUEST_URI']; + + $sPath = self::GetRequestUri($bForceTrustProxy); // remove all the parameters from the query string $iQuestionMarkPos = strpos($sPath, '?'); @@ -870,6 +876,61 @@ class utils return self::GetAppRootUrl($sCurrentScript, $sAppRoot, $sAbsoluteUrl); } + /** + * @param false $bForceTrustProxy + * @since 2.7.4 + */ + public static function GetServerName($bForceTrustProxy = false) + { + $bTrustProxy = $bForceTrustProxy || self::IsProxyTrusted(); + + $sServerName = isset($_SERVER['SERVER_NAME']) ? $_SERVER['SERVER_NAME'] : ''; + + if ($bTrustProxy) { + $sServerName = isset($_SERVER['X_FORWARDED_HOST']) ? $_SERVER['X_FORWARDED_HOST'] : $sServerName; + } + + return $sServerName; + } + + /** + * @param false $bForceTrustProxy + * @since 2.7.4 + */ + public static function GetServerPort($bForceTrustProxy = false) + { + $bTrustProxy = $bForceTrustProxy || self::IsProxyTrusted(); + + $sServerPort = isset($_SERVER['SERVER_PORT']) ? $_SERVER['SERVER_PORT'] : 80; + + if ($bTrustProxy) { + $sServerPort = isset($_SERVER['X_FORWARDED_PORT']) ? $_SERVER['X_FORWARDED_PORT'] : $sServerPort; + } + + return $sServerPort; + } + + /** + * @since 2.7.4 + */ + public static function GetRequestUri() + { + // $_SERVER['REQUEST_URI'] is empty when running on IIS + // Let's use Ivan Tcholakov's fix (found on www.dokeos.com) + if (empty($_SERVER['REQUEST_URI'])) + { + $sPath = $_SERVER['SCRIPT_NAME']; + if (!empty($_SERVER['QUERY_STRING'])) + { + $sPath .= '?'.$_SERVER['QUERY_STRING']; + } + $_SERVER['REQUEST_URI'] = $sPath; + } + $sPath = $_SERVER['REQUEST_URI']; + + return $sPath; + } + /** * @param $sCurrentScript * @param $sAppRoot @@ -914,16 +975,33 @@ class utils * * Though the official specs says 'a non empty string', some servers like IIS do set it to 'off' ! * nginx set it to an empty string - * Others might leave it unset (no array entry) + * Others might leave it unset (no array entry) + * + * @param bool $bForceTrustProxy + * + * @return bool + * + * @since 2.7.4 reverse proxies handling */ - public static function IsConnectionSecure() + public static function IsConnectionSecure($bForceTrustProxy = false) { $bSecured = false; - if (!empty($_SERVER['HTTPS']) && (strtolower($_SERVER['HTTPS']) != 'off')) + $bTrustProxy = $bForceTrustProxy || self::IsProxyTrusted(); + + if ($bTrustProxy && !empty($_SERVER['HTTP_X_FORWARDED_PROTO'])) + { + $bSecured = ($_SERVER['HTTP_X_FORWARDED_PROTO'] === 'https'); + } + elseif ($bTrustProxy && !empty($_SERVER['HTTP_X_FORWARDED_PROTOCOL'])) + { + $bSecured = ($_SERVER['HTTP_X_FORWARDED_PROTOCOL'] === 'https'); + } + elseif ((!empty($_SERVER['HTTPS'])) && (strtolower($_SERVER['HTTPS']) != 'off')) { $bSecured = true; } + return $bSecured; } diff --git a/core/config.class.inc.php b/core/config.class.inc.php index 5d6e63938..313bb1b94 100644 --- a/core/config.class.inc.php +++ b/core/config.class.inc.php @@ -1265,6 +1265,14 @@ class Config 'source_of_value' => '', 'show_in_conf_sample' => false, ], + 'trust_proxies' => [ + 'type' => 'bool', + 'description' => 'If true, then proxies custom header (X-Forwarded-*) are taken into account. Use only if the webserver is not publicly accessible (reachable only by the reverse proxy)', + 'default' => false, + 'value' => false, + 'source_of_value' => '', + 'show_in_conf_sample' => false, + ], ); diff --git a/setup/setuppage.class.inc.php b/setup/setuppage.class.inc.php index a25ad3d23..2c4bbef22 100644 --- a/setup/setuppage.class.inc.php +++ b/setup/setuppage.class.inc.php @@ -144,7 +144,7 @@ class SetupPage extends NiceWebPage public function output() { - $sLogo = utils::GetAbsoluteUrlAppRoot().'/images/itop-logo.png'; + $sLogo = utils::GetAbsoluteUrlAppRoot(true).'/images/itop-logo.png'; $this->s_content = "

\" ".htmlentities($this->s_title, ENT_QUOTES, self::PAGES_CHARSET)."

\n
{$this->s_content}\n
\n"; diff --git a/setup/wizardsteps.class.inc.php b/setup/wizardsteps.class.inc.php index 40d407055..7edbaeb40 100644 --- a/setup/wizardsteps.class.inc.php +++ b/setup/wizardsteps.class.inc.php @@ -978,7 +978,7 @@ class WizStepMiscParams extends WizardStep public function Display(WebPage $oPage) { $sDefaultLanguage = $this->oWizard->GetParameter('default_language', $this->oWizard->GetParameter('admin_language')); - $sApplicationURL = $this->oWizard->GetParameter('application_url', utils::GetDefaultUrlAppRoot()); + $sApplicationURL = $this->oWizard->GetParameter('application_url', utils::GetDefaultUrlAppRoot(true)); $sDefaultGraphvizPath = (strtolower(substr(PHP_OS, 0, 3)) === 'win') ? 'C:\\Program Files\\Graphviz\\bin\\dot.exe' : '/usr/bin/dot'; $sGraphvizPath = $this->oWizard->GetParameter('graphviz_path', $sDefaultGraphvizPath); $sSampleData = $this->oWizard->GetParameter('sample_data', 'yes'); @@ -996,14 +996,14 @@ class WizStepMiscParams extends WizardStep $oPage->add('
'); $oPage->add('Application URL'); $oPage->add(''); - $oPage->add(''); - $oPage->add(''); + $oPage->add(''); + $oPage->add(''); $oPage->add('
URL:
Change the value above if the end-users will be accessing the application by another path due to a specific configuration of the web server.
URL:
Change the value above if the end-users will be accessing the application by another path due to a specific configuration of the web server.
'); $oPage->add('
'); $oPage->add('
'); $oPage->add('Path to Graphviz\' dot application'); $oPage->add(''); - $oPage->add(''); + $oPage->add(''); $oPage->add(''); $oPage->add(''); $oPage->add('
Path:
Path:
Graphviz is required to display the impact analysis graph (i.e. impacts / depends on).
'); @@ -1125,21 +1125,21 @@ class WizStepUpgradeMiscParams extends WizardStep public function Display(WebPage $oPage) { - $sApplicationURL = $this->oWizard->GetParameter('application_url', utils::GetDefaultUrlAppRoot()); + $sApplicationURL = $this->oWizard->GetParameter('application_url', utils::GetAbsoluteUrlAppRoot(true)); //Preserve existing configuration. $sDefaultGraphvizPath = (strtolower(substr(PHP_OS, 0, 3)) === 'win') ? 'C:\\Program Files\\Graphviz\\bin\\dot.exe' : '/usr/bin/dot'; $sGraphvizPath = $this->oWizard->GetParameter('graphviz_path', $sDefaultGraphvizPath); $oPage->add('

Additional parameters

'); $oPage->add('
'); $oPage->add('Application URL'); $oPage->add(''); - $oPage->add(''); - $oPage->add(''); + $oPage->add(''); + $oPage->add(''); $oPage->add('
URL:
Change the value above if the end-users will be accessing the application by another path due to a specific configuration of the web server.
URL:
Change the value above if the end-users will be accessing the application by another path due to a specific configuration of the web server.
'); $oPage->add('
'); $oPage->add('
'); $oPage->add('Path to Graphviz\' dot application'); $oPage->add(''); - $oPage->add(''); + $oPage->add(''); $oPage->add(''); $oPage->add(''); $oPage->add('
Path:
Path:
Graphviz is required to display the impact analysis graph (i.e. impacts / depends on).
'); @@ -1376,14 +1376,14 @@ class WizStepModulesChoice extends WizardStep if (substr($sBannerPath, 0, 1) == '/') { // absolute path, means relative to APPROOT - $sBannerUrl = utils::GetDefaultUrlAppRoot().$sBannerPath; + $sBannerUrl = utils::GetDefaultUrlAppRoot(true).$sBannerPath; } else { // relative path: i.e. relative to the directory containing the XML file $sFullPath = dirname($this->GetSourceFilePath()).'/'.$sBannerPath; $sRealPath = realpath($sFullPath); - $sBannerUrl = utils::GetDefaultUrlAppRoot().str_replace(realpath(APPROOT), '', $sRealPath); + $sBannerUrl = utils::GetDefaultUrlAppRoot(true).str_replace(realpath(APPROOT), '', $sRealPath); } $oPage->add(''); } @@ -2515,7 +2515,7 @@ class WizStepDone extends WizardStep $aManualSteps = array(); $aAvailableModules = SetupUtils::AnalyzeInstallation($this->oWizard); - $sRootUrl = utils::GetAbsoluteUrlAppRoot(); + $sRootUrl = utils::GetAbsoluteUrlAppRoot(true); $aSelectedModules = json_decode($this->oWizard->GetParameter('selected_modules'), true); foreach($aSelectedModules as $sModuleId) { @@ -2547,7 +2547,7 @@ class WizStepDone extends WizardStep { // To mitigate security risks: pass only the filename without the extension, the download will add the extension itself $oPage->p('Your backup is ready'); - $oPage->p(' Download '.basename($sBackupDestination).''); + $oPage->p(' Download '.basename($sBackupDestination).''); } else { diff --git a/test/application/UtilsTest.php b/test/application/UtilsTest.php index ee3d02559..93e2ab241 100644 --- a/test/application/UtilsTest.php +++ b/test/application/UtilsTest.php @@ -168,4 +168,151 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase ); } + /** + * @dataProvider GetDefaultUrlAppRootProvider + */ + public function testGetDefaultUrlAppRoot($bForceTrustProxy, $bConfTrustProxy, $aServerVars, $sExpectedAppRootUrl) + { + $_SERVER = $aServerVars; + utils::GetConfig()->Set('trust_proxies', $bConfTrustProxy); + $sAppRootUrl = utils::GetDefaultUrlAppRoot($bForceTrustProxy); + $this->assertEquals($sExpectedAppRootUrl, $sAppRootUrl); + } + + public function GetDefaultUrlAppRootProvider() + { + $this->setUp(); + + $baseServerVar = [ + 'REMOTE_ADDR' => '127.0.0.1', //is not set, disable IsProxyTrusted + 'SERVER_NAME' => 'example.com', + 'X_FORWARDED_HOST' => null, + 'SERVER_PORT' => '80', + 'X_FORWARDED_PORT' => null, + 'REQUEST_URI' => '/index.php?baz=1', + 'SCRIPT_NAME' => '/index.php', + 'SCRIPT_FILENAME' => APPROOT.'index.php', + 'QUERY_STRING' => 'baz=1', + 'HTTP_X_FORWARDED_PROTO' => null, + 'HTTP_X_FORWARDED_PROTOCOL' => null, + 'HTTPS' => null, + ]; + + return [ + 'no proxy, http' => [ + 'bForceTrustProxy' => false, + 'bConfTrustProxy' => false, + 'aServerVars' => array_merge($baseServerVar, []), + 'sExpectedAppRootUrl' => 'http://example.com/', + ], + 'no proxy, subPath, http' => [ + 'bForceTrustProxy' => false, + 'bConfTrustProxy' => false, + 'aServerVars' => array_merge($baseServerVar, [ + 'REQUEST_URI' => '/foo/index.php?baz=1', + ]), + 'sExpectedAppRootUrl' => 'http://example.com/foo/', + ], + 'IIS lack REQUEST_URI' => [ + 'bForceTrustProxy' => false, + 'bConfTrustProxy' => false, + 'aServerVars' => array_merge($baseServerVar, [ + 'REQUEST_URI' => null, + 'SCRIPT_NAME' => '/foo/index.php', + ]), + 'sExpectedAppRootUrl' => 'http://example.com/foo/', + ], + 'no proxy, https' => [ + 'bForceTrustProxy' => false, + 'bConfTrustProxy' => false, + 'aServerVars' => array_merge($baseServerVar, [ + 'SERVER_PORT' => '443', + 'HTTPS' => 'on', + ]), + 'sExpectedAppRootUrl' => 'https://example.com/', + ], + 'no proxy, https on 4443' => [ + 'bForceTrustProxy' => false, + 'bConfTrustProxy' => false, + 'aServerVars' => array_merge($baseServerVar, [ + 'SERVER_PORT' => '4443', + 'HTTPS' => 'on', + ]), + 'sExpectedAppRootUrl' => 'https://example.com:4443/', + ], + 'with proxy, not enabled' => [ + 'bForceTrustProxy' => false, + 'bConfTrustProxy' => false, + 'aServerVars' => array_merge($baseServerVar, [ + 'X_FORWARDED_HOST' => 'proxy.com', + 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_PROTO' => 'https', + ]), + 'sExpectedAppRootUrl' => 'http://example.com/', + ], + 'with proxy, enabled' => [ + 'bForceTrustProxy' => false, + 'bConfTrustProxy' => true, + 'aServerVars' => array_merge($baseServerVar, [ + 'X_FORWARDED_HOST' => 'proxy.com', + 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_PROTO' => 'https', + ]), + 'sExpectedAppRootUrl' => 'https://proxy.com:4443/', + ], + 'with proxy, enabled - alt' => [ + 'bForceTrustProxy' => false, + 'bConfTrustProxy' => true, + 'aServerVars' => array_merge($baseServerVar, [ + 'X_FORWARDED_HOST' => 'proxy.com', + 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_PROTOCOL' => 'https', + ]), + 'sExpectedAppRootUrl' => 'https://proxy.com:4443/', + ], + 'with proxy, disabled, forced' => [ + 'bForceTrustProxy' => true, + 'bConfTrustProxy' => false, + 'aServerVars' => array_merge($baseServerVar, [ + 'X_FORWARDED_HOST' => 'proxy.com', + 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_PROTO' => 'https', + ]), + 'sExpectedAppRootUrl' => 'https://proxy.com:4443/', + ], + 'with proxy, enabled, forced' => [ + 'bForceTrustProxy' => true, + 'bConfTrustProxy' => true, + 'aServerVars' => array_merge($baseServerVar, [ + 'X_FORWARDED_HOST' => 'proxy.com', + 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_PROTO' => 'https', + ]), + 'sExpectedAppRootUrl' => 'https://proxy.com:4443/', + ], + + 'with proxy, disabled, forced, no remote addr' => [ + 'bForceTrustProxy' => true, + 'bConfTrustProxy' => false, + 'aServerVars' => array_merge($baseServerVar, [ + 'REMOTE_ADDR' => null, + 'X_FORWARDED_HOST' => 'proxy.com', + 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_PROTO' => 'https', + ]), + 'sExpectedAppRootUrl' => 'https://proxy.com:4443/', + ], + 'with proxy, enabled, no remote addr' => [ + 'bForceTrustProxy' => false, + 'bConfTrustProxy' => true, + 'aServerVars' => array_merge($baseServerVar, [ + 'REMOTE_ADDR' => null, + 'X_FORWARDED_HOST' => 'proxy.com', + 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_PROTO' => 'https', + ]), + 'sExpectedAppRootUrl' => 'http://example.com/', + ], + ]; + } } From 1f26b59d902967d14101e665c4ba8e3cbac08041 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Thu, 4 Mar 2021 09:32:13 +0100 Subject: [PATCH 02/10] =?UTF-8?q?N=C2=B03671=20-=20add=20an=20API=20endpoi?= =?UTF-8?q?nt=20(it=20will=20be=20used=20by=20N=C2=B03668=20and=20N=C2=B03?= =?UTF-8?q?760)=20+=20some=20code=20cleanup=20asked=20by=20@molkobain?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/utils.inc.php | 76 +++++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 27 deletions(-) diff --git a/application/utils.inc.php b/application/utils.inc.php index 443bf3781..ae2dde710 100644 --- a/application/utils.inc.php +++ b/application/utils.inc.php @@ -782,6 +782,9 @@ class utils } } + /** + * @since 2.7.4 + */ public static function IsProxyTrusted() { if (empty($_SERVER['REMOTE_ADDR'])) { @@ -846,29 +849,7 @@ class utils */ public static function GetDefaultUrlAppRoot($bForceTrustProxy = false) { - // Build an absolute URL to this page on this server/port - $sServerName = self::GetServerName($bForceTrustProxy); - $bIsSecure = self::IsConnectionSecure($bForceTrustProxy) ; - $sProtocol = $bIsSecure ? 'https' : 'http'; - $iPort = self::GetServerPort($bForceTrustProxy); - if ($bIsSecure) - { - $sPort = ($iPort == 443) ? '' : ':'.$iPort; - } - else - { - $sPort = ($iPort == 80) ? '' : ':'.$iPort; - } - - $sPath = self::GetRequestUri($bForceTrustProxy); - - // remove all the parameters from the query string - $iQuestionMarkPos = strpos($sPath, '?'); - if ($iQuestionMarkPos !== false) - { - $sPath = substr($sPath, 0, $iQuestionMarkPos); - } - $sAbsoluteUrl = "$sProtocol://{$sServerName}{$sPort}{$sPath}"; + $sAbsoluteUrl = self::GetCurrentAbsoluteUrl($bForceTrustProxy, true); $sCurrentScript = realpath($_SERVER['SCRIPT_FILENAME']); $sAppRoot = realpath(APPROOT); @@ -876,8 +857,49 @@ class utils return self::GetAppRootUrl($sCurrentScript, $sAppRoot, $sAbsoluteUrl); } + /** - * @param false $bForceTrustProxy + * Build the current absolute URL from the server's variables. + * + * For almost every usage, you should use the more secure utils::GetAbsoluteUrlAppRoot() : instead of reading the current uri, it provide you the configured application's root URL (this is done during the setup and chn be changed in the configuration file) + * + * @see utils::GetAbsoluteUrlAppRoot + * + * @return string + * + * @since 2.7.4 + */ + public static function GetCurrentAbsoluteUrl($bForceTrustProxy = false, $bTrimQueryString = false) + { + // Build an absolute URL to this page on this server/port + $sServerName = self::GetServerName($bForceTrustProxy); + $bIsSecure = self::IsConnectionSecure($bForceTrustProxy); + $sProtocol = $bIsSecure ? 'https' : 'http'; + $iPort = self::GetServerPort($bForceTrustProxy); + if ($bIsSecure) { + $sPort = ($iPort == 443) ? '' : ':'.$iPort; + } else { + $sPort = ($iPort == 80) ? '' : ':'.$iPort; + } + + $sPath = self::GetRequestUri($bForceTrustProxy); + + if ($bTrimQueryString) { + // remove all the parameters from the query string + $iQuestionMarkPos = strpos($sPath, '?'); + if ($iQuestionMarkPos !== false) { + $sPath = substr($sPath, 0, $iQuestionMarkPos); + } + } + + $sAbsoluteUrl = "$sProtocol://{$sServerName}{$sPort}{$sPath}"; + + return $sAbsoluteUrl; + } + + /** + * @return string + * * @since 2.7.4 */ public static function GetServerName($bForceTrustProxy = false) @@ -894,7 +916,6 @@ class utils } /** - * @param false $bForceTrustProxy * @since 2.7.4 */ public static function GetServerPort($bForceTrustProxy = false) @@ -911,6 +932,8 @@ class utils } /** + * @return string + * * @since 2.7.4 */ public static function GetRequestUri() @@ -977,8 +1000,6 @@ class utils * nginx set it to an empty string * Others might leave it unset (no array entry) * - * @param bool $bForceTrustProxy - * * @return bool * * @since 2.7.4 reverse proxies handling @@ -2423,4 +2444,5 @@ class utils public static function IsWindowsEnvironment(){ return (substr(PHP_OS,0,3) === 'WIN'); } + } From 142979269003e49bac5a74283a9e2ec535b1f8ff Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Thu, 4 Mar 2021 09:39:48 +0100 Subject: [PATCH 03/10] =?UTF-8?q?N=C2=B03668=20-=20fix=20an=20improper=20r?= =?UTF-8?q?edirection=20to=20the=20homepage=20when=20iTop=20is=20behind=20?= =?UTF-8?q?a=20reverse=20proxy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/loginwebpage.class.inc.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/application/loginwebpage.class.inc.php b/application/loginwebpage.class.inc.php index db06af42b..47cfe6563 100644 --- a/application/loginwebpage.class.inc.php +++ b/application/loginwebpage.class.inc.php @@ -685,7 +685,7 @@ class LoginWebPage extends NiceWebPage public static function HTTPReload() { - $sOriginURL = $_SERVER['REQUEST_SCHEME'] . '://' . $_SERVER['HTTP_HOST'].$_SERVER['REQUEST_URI']; + $sOriginURL = utils::GetCurrentAbsoluteUrl(); if (!utils::StartsWith($sOriginURL, utils::GetAbsoluteUrlAppRoot())) { // If the found URL does not start with the configured AppRoot URL From bb8d4a92cbddef15cfe5da4d7ebe8d33734f9597 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Thu, 4 Mar 2021 09:56:05 +0100 Subject: [PATCH 04/10] fix an indentation problem (thanks @Hipska) --- application/utils.inc.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/application/utils.inc.php b/application/utils.inc.php index ae2dde710..fb19e0b07 100644 --- a/application/utils.inc.php +++ b/application/utils.inc.php @@ -847,7 +847,7 @@ class utils * * @since 2.7.4 $bForceTrustProxy param added */ - public static function GetDefaultUrlAppRoot($bForceTrustProxy = false) + public static function GetDefaultUrlAppRoot($bForceTrustProxy = false) { $sAbsoluteUrl = self::GetCurrentAbsoluteUrl($bForceTrustProxy, true); From a06bf6ea7c616656b65495d5f92cbc2106ea0ebd Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Fri, 5 Mar 2021 09:20:04 +0100 Subject: [PATCH 05/10] coding convention (thanks @molkobain) --- application/utils.inc.php | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/application/utils.inc.php b/application/utils.inc.php index fb19e0b07..afa04a5b0 100644 --- a/application/utils.inc.php +++ b/application/utils.inc.php @@ -783,6 +783,8 @@ class utils } /** + * @return bool The boolean value of the conf. "trust_proxies" (except if there is no REMOTE_ADDR int his case, it return false) + * * @since 2.7.4 */ public static function IsProxyTrusted() @@ -865,6 +867,9 @@ class utils * * @see utils::GetAbsoluteUrlAppRoot * + * @param bool $bForceTrustProxy + * @param bool $bTrimQueryString + * * @return string * * @since 2.7.4 @@ -898,6 +903,8 @@ class utils } /** + * @param bool $bForceTrustProxy + * * @return string * * @since 2.7.4 @@ -916,6 +923,9 @@ class utils } /** + * @param bool $bForceTrustProxy + * + * @return int|mixed * @since 2.7.4 */ public static function GetServerPort($bForceTrustProxy = false) @@ -995,15 +1005,17 @@ class utils /** * Helper to handle the variety of HTTP servers * See N°286 (fixed in [896]), and N°634 (this fix) - * + * * Though the official specs says 'a non empty string', some servers like IIS do set it to 'off' ! * nginx set it to an empty string * Others might leave it unset (no array entry) * + * @param bool $bForceTrustProxy + * * @return bool * * @since 2.7.4 reverse proxies handling - */ + */ public static function IsConnectionSecure($bForceTrustProxy = false) { $bSecured = false; From ae6a264d6d7dfa393639fdea39757a828caafa2b Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Fri, 5 Mar 2021 16:57:03 +0100 Subject: [PATCH 06/10] =?UTF-8?q?N=C2=B03671=20-=20fix=20typo=20in=20HTTP?= =?UTF-8?q?=20header=20name?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/utils.inc.php | 4 ++-- test/application/UtilsTest.php | 32 ++++++++++++++++---------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/application/utils.inc.php b/application/utils.inc.php index afa04a5b0..721fda1d7 100644 --- a/application/utils.inc.php +++ b/application/utils.inc.php @@ -916,7 +916,7 @@ class utils $sServerName = isset($_SERVER['SERVER_NAME']) ? $_SERVER['SERVER_NAME'] : ''; if ($bTrustProxy) { - $sServerName = isset($_SERVER['X_FORWARDED_HOST']) ? $_SERVER['X_FORWARDED_HOST'] : $sServerName; + $sServerName = isset($_SERVER['HTTP_X_FORWARDED_HOST']) ? $_SERVER['HTTP_X_FORWARDED_HOST'] : $sServerName; } return $sServerName; @@ -935,7 +935,7 @@ class utils $sServerPort = isset($_SERVER['SERVER_PORT']) ? $_SERVER['SERVER_PORT'] : 80; if ($bTrustProxy) { - $sServerPort = isset($_SERVER['X_FORWARDED_PORT']) ? $_SERVER['X_FORWARDED_PORT'] : $sServerPort; + $sServerPort = isset($_SERVER['HTTP_X_FORWARDED_PORT']) ? $_SERVER['HTTP_X_FORWARDED_PORT'] : $sServerPort; } return $sServerPort; diff --git a/test/application/UtilsTest.php b/test/application/UtilsTest.php index 93e2ab241..779ea8b38 100644 --- a/test/application/UtilsTest.php +++ b/test/application/UtilsTest.php @@ -186,9 +186,9 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase $baseServerVar = [ 'REMOTE_ADDR' => '127.0.0.1', //is not set, disable IsProxyTrusted 'SERVER_NAME' => 'example.com', - 'X_FORWARDED_HOST' => null, + 'HTTP_X_FORWARDED_HOST' => null, 'SERVER_PORT' => '80', - 'X_FORWARDED_PORT' => null, + 'HTTP_X_FORWARDED_PORT' => null, 'REQUEST_URI' => '/index.php?baz=1', 'SCRIPT_NAME' => '/index.php', 'SCRIPT_FILENAME' => APPROOT.'index.php', @@ -244,8 +244,8 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase 'bForceTrustProxy' => false, 'bConfTrustProxy' => false, 'aServerVars' => array_merge($baseServerVar, [ - 'X_FORWARDED_HOST' => 'proxy.com', - 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_HOST' => 'proxy.com', + 'HTTP_X_FORWARDED_PORT' => '4443', 'HTTP_X_FORWARDED_PROTO' => 'https', ]), 'sExpectedAppRootUrl' => 'http://example.com/', @@ -254,8 +254,8 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase 'bForceTrustProxy' => false, 'bConfTrustProxy' => true, 'aServerVars' => array_merge($baseServerVar, [ - 'X_FORWARDED_HOST' => 'proxy.com', - 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_HOST' => 'proxy.com', + 'HTTP_X_FORWARDED_PORT' => '4443', 'HTTP_X_FORWARDED_PROTO' => 'https', ]), 'sExpectedAppRootUrl' => 'https://proxy.com:4443/', @@ -264,8 +264,8 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase 'bForceTrustProxy' => false, 'bConfTrustProxy' => true, 'aServerVars' => array_merge($baseServerVar, [ - 'X_FORWARDED_HOST' => 'proxy.com', - 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_HOST' => 'proxy.com', + 'HTTP_X_FORWARDED_PORT' => '4443', 'HTTP_X_FORWARDED_PROTOCOL' => 'https', ]), 'sExpectedAppRootUrl' => 'https://proxy.com:4443/', @@ -274,8 +274,8 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase 'bForceTrustProxy' => true, 'bConfTrustProxy' => false, 'aServerVars' => array_merge($baseServerVar, [ - 'X_FORWARDED_HOST' => 'proxy.com', - 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_HOST' => 'proxy.com', + 'HTTP_X_FORWARDED_PORT' => '4443', 'HTTP_X_FORWARDED_PROTO' => 'https', ]), 'sExpectedAppRootUrl' => 'https://proxy.com:4443/', @@ -284,8 +284,8 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase 'bForceTrustProxy' => true, 'bConfTrustProxy' => true, 'aServerVars' => array_merge($baseServerVar, [ - 'X_FORWARDED_HOST' => 'proxy.com', - 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_HOST' => 'proxy.com', + 'HTTP_X_FORWARDED_PORT' => '4443', 'HTTP_X_FORWARDED_PROTO' => 'https', ]), 'sExpectedAppRootUrl' => 'https://proxy.com:4443/', @@ -296,8 +296,8 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase 'bConfTrustProxy' => false, 'aServerVars' => array_merge($baseServerVar, [ 'REMOTE_ADDR' => null, - 'X_FORWARDED_HOST' => 'proxy.com', - 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_HOST' => 'proxy.com', + 'HTTP_X_FORWARDED_PORT' => '4443', 'HTTP_X_FORWARDED_PROTO' => 'https', ]), 'sExpectedAppRootUrl' => 'https://proxy.com:4443/', @@ -307,8 +307,8 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase 'bConfTrustProxy' => true, 'aServerVars' => array_merge($baseServerVar, [ 'REMOTE_ADDR' => null, - 'X_FORWARDED_HOST' => 'proxy.com', - 'X_FORWARDED_PORT' => '4443', + 'HTTP_X_FORWARDED_HOST' => 'proxy.com', + 'HTTP_X_FORWARDED_PORT' => '4443', 'HTTP_X_FORWARDED_PROTO' => 'https', ]), 'sExpectedAppRootUrl' => 'http://example.com/', From 83f99642e06916bd4fac886fbfe161766eefd4d2 Mon Sep 17 00:00:00 2001 From: odain Date: Mon, 8 Mar 2021 11:38:20 +0100 Subject: [PATCH 07/10] =?UTF-8?q?N=C2=B03793=20-=20Cleanup=20of=20orphan?= =?UTF-8?q?=20CMDBChange=20can=20hang=20the=20setup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/applicationinstaller.class.inc.php | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/setup/applicationinstaller.class.inc.php b/setup/applicationinstaller.class.inc.php index 004094b1e..29cfd992c 100644 --- a/setup/applicationinstaller.class.inc.php +++ b/setup/applicationinstaller.class.inc.php @@ -727,10 +727,15 @@ class ApplicationInstaller SetupPage::log_info("There are $iOrphanCount useless records in {$sDBPrefix}priv_change (".sprintf('%.2f', ((100.0*$iOrphanCount)/$iTotalCount))."%)"); if ($iOrphanCount > 0) { - SetupPage::log_info("Removing the orphan records..."); - $sCleanup = "DELETE FROM `{$sDBPrefix}priv_change` USING `{$sDBPrefix}priv_change` LEFT JOIN `{$sDBPrefix}priv_changeop` ON `{$sDBPrefix}priv_change`.id = `{$sDBPrefix}priv_changeop`.changeid WHERE `{$sDBPrefix}priv_changeop`.id IS NULL;"; - CMDBSource::Query($sCleanup); - SetupPage::log_info("Cleanup completed successfully."); + if ($iOrphanCount > 100000) + { + SetupPage::log_warning("There are too much useless records ($iOrphanCount) in {$sDBPrefix}priv_change. Cleanup cannot be done during setup."); + } else { + SetupPage::log_info("Removing the orphan records..."); + $sCleanup = "DELETE FROM `{$sDBPrefix}priv_change` USING `{$sDBPrefix}priv_change` LEFT JOIN `{$sDBPrefix}priv_changeop` ON `{$sDBPrefix}priv_change`.id = `{$sDBPrefix}priv_changeop`.changeid WHERE `{$sDBPrefix}priv_changeop`.id IS NULL;"; + CMDBSource::Query($sCleanup); + SetupPage::log_info("Cleanup completed successfully."); + } } else { From c842162fe20faa9e9f46101d18bfe2c37ab28fa3 Mon Sep 17 00:00:00 2001 From: odain Date: Tue, 9 Mar 2021 08:13:57 +0100 Subject: [PATCH 08/10] =?UTF-8?q?N=C2=B03788=20-=20timeout/excessive=20dur?= =?UTF-8?q?ation=20during=20MTP?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../module.itop-attachments.php | 63 +++++++++++++++++-- 1 file changed, 58 insertions(+), 5 deletions(-) diff --git a/datamodels/2.x/itop-attachments/module.itop-attachments.php b/datamodels/2.x/itop-attachments/module.itop-attachments.php index 86e13fb5c..db56390d2 100644 --- a/datamodels/2.x/itop-attachments/module.itop-attachments.php +++ b/datamodels/2.x/itop-attachments/module.itop-attachments.php @@ -80,6 +80,30 @@ if (!class_exists('AttachmentInstaller')) return $oConfiguration; } + /** + * @param string $sTableName + * @param int $iBulkSize + * + * @return array + * @throws \CoreException + * @throws \MySQLException + * @throws \MySQLHasGoneAwayException + */ + public static function GetOrphanAttachmentIds($sTableName, $iBulkSize){ + $sSqlQuery = <<fetch_array()){ + $aIds[] = $aRow['attachment_id']; + } + + return $aIds; + } + /** * Handler called before creating or upgrading the database schema * @param $oConfiguration Config The new configuration of the application @@ -97,10 +121,32 @@ if (!class_exists('AttachmentInstaller')) $iCount = CMDBSource::QueryToScalar($sCountQuery); if ($iCount > 0) { - SetupPage::log_info("Cleanup of orphan attachments that cannot be migrated to the new ObjKey model: $iCount record(s) must be deleted."); - $sRepairQuery = "DELETE FROM `$sTableName` WHERE (`item_id`='' OR `item_id` IS NULL)"; - $iRet = CMDBSource::Query($sRepairQuery); // Throws an exception in case of error - SetupPage::log_info("Cleanup of orphan attachments successfully completed."); + SetupPage::log_info("Cleanup of orphan attachments that cannot be migrated to the new ObjKey model: $iCount record(s) must be deleted."); + + $iBulkSize = 100; + $iMaxDuration = 5; + $iDeletedCount = 0; + $fStartTime = microtime(true); + $aIds = self::GetOrphanAttachmentIds($sTableName, $iBulkSize); + + while (count($aIds) !== 0) { + $sCleanupQuery = sprintf("DELETE FROM `$sTableName` WHERE `id` IN (%s)", implode(",", $aIds)); + CMDBSource::Query($sCleanupQuery); // Throws an exception in case of error + + $iDeletedCount += count($aIds); + $fElapsed = microtime(true) - $fStartTime; + + if ($fElapsed > $iMaxDuration){ + SetupPage::log_info(sprintf("Cleanup of orphan attachments interrupted after %.3f s. $iDeletedCount records were deleted among $iCount.", $fElapsed)); + break; + } + + $aIds = self::GetOrphanAttachmentIds($sTableName, $iBulkSize); + } + + if (count($aIds) === 0){ + SetupPage::log_info("Cleanup of orphan attachments successfully completed."); + } } else { @@ -108,7 +154,7 @@ if (!class_exists('AttachmentInstaller')) } } } - + /** * Handler called after the creation/update of the database schema * @param $oConfiguration Config The new configuration of the application @@ -123,6 +169,7 @@ if (!class_exists('AttachmentInstaller')) // Prerequisite: change null into 0 (workaround to the fact that we cannot use IS NULL in OQL) SetupPage::log_info("Initializing attachment/item_org_id - null to zero"); $sTableName = MetaModel::DBGetTable('Attachment'); + $sRepair = "UPDATE `$sTableName` SET `item_org_id` = 0 WHERE `item_org_id` IS NULL"; CMDBSource::Query($sRepair); @@ -132,7 +179,13 @@ if (!class_exists('AttachmentInstaller')) $iUpdated = 0; while ($oAttachment = $oSet->Fetch()) { + if ($oAttachment->Get('item_id') == 0) { + //do not treat orphan attachment + continue; + } + $oContainer = MetaModel::GetObject($oAttachment->Get('item_class'), $oAttachment->Get('item_id'), false /* must be found */, true /* allow all data */); + if ($oContainer) { $oAttachment->SetItem($oContainer, true /*updateonchange*/); From 995619af9bc93875e88b284a6e883fcea76c6677 Mon Sep 17 00:00:00 2001 From: odain Date: Tue, 9 Mar 2021 08:27:33 +0100 Subject: [PATCH 09/10] =?UTF-8?q?N=C2=B03788=20-=20timeout/excessive=20dur?= =?UTF-8?q?ation=20during=20MTP=20-=20increase=20timeout=20from=205=20to?= =?UTF-8?q?=2030s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- datamodels/2.x/itop-attachments/module.itop-attachments.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datamodels/2.x/itop-attachments/module.itop-attachments.php b/datamodels/2.x/itop-attachments/module.itop-attachments.php index db56390d2..cebd8da03 100644 --- a/datamodels/2.x/itop-attachments/module.itop-attachments.php +++ b/datamodels/2.x/itop-attachments/module.itop-attachments.php @@ -124,7 +124,7 @@ SQL; SetupPage::log_info("Cleanup of orphan attachments that cannot be migrated to the new ObjKey model: $iCount record(s) must be deleted."); $iBulkSize = 100; - $iMaxDuration = 5; + $iMaxDuration = 30; $iDeletedCount = 0; $fStartTime = microtime(true); $aIds = self::GetOrphanAttachmentIds($sTableName, $iBulkSize); From 52cd4f7c5e8525414319a1ea095a70cc7f28bf2d Mon Sep 17 00:00:00 2001 From: odain Date: Tue, 9 Mar 2021 11:44:25 +0100 Subject: [PATCH 10/10] =?UTF-8?q?N=C2=B03788=20-=20timeout/excessive=20dur?= =?UTF-8?q?ation=20during=20MTP=20-=20fix=20PostDbCreation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- datamodels/2.x/itop-attachments/module.itop-attachments.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datamodels/2.x/itop-attachments/module.itop-attachments.php b/datamodels/2.x/itop-attachments/module.itop-attachments.php index cebd8da03..f8ae08ba1 100644 --- a/datamodels/2.x/itop-attachments/module.itop-attachments.php +++ b/datamodels/2.x/itop-attachments/module.itop-attachments.php @@ -179,7 +179,7 @@ SQL; $iUpdated = 0; while ($oAttachment = $oSet->Fetch()) { - if ($oAttachment->Get('item_id') == 0) { + if (empty($oAttachment->Get('item_class'))) { //do not treat orphan attachment continue; }