From e44ff03f6a13813170082ccbdaf530ea47a25d3b Mon Sep 17 00:00:00 2001 From: Bernd Stellwag Date: Sun, 7 Oct 2018 13:58:27 +0200 Subject: [PATCH 1/5] Check if the config file is writable and throw an exception if it's not Signed-off-by: Bernd Stellwag --- lib/private/Config.php | 15 ++++++++++++++- tests/lib/ConfigTest.php | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/lib/private/Config.php b/lib/private/Config.php index c881e485006cb..280951bd2bdce 100644 --- a/lib/private/Config.php +++ b/lib/private/Config.php @@ -237,7 +237,20 @@ private function writeData() { $content .= var_export($this->cache, true); $content .= ";\n"; - touch ($this->configFilePath); + // Check if config directory is writable and create the config file + if ( + !is_writable(dirname($this->configFilePath)) + || !touch($this->configFilePath) + || !is_writable($this->configFilePath) + ) { + // TODO fix this via DI once it is very clear that this doesn't cause side effects due to initialization order + // currently this breaks app routes but also could have other side effects especially during setup and exception handling + $url = \OC::$server->getURLGenerator()->linkToDocs('admin-dir_permissions'); + throw new HintException( + "Can't write into config directory!", + 'This can usually be fixed by giving the webserver write access to the config directory. See ' . $url); + } + $filePointer = fopen($this->configFilePath, 'r+'); // Prevent others not to read the config diff --git a/tests/lib/ConfigTest.php b/tests/lib/ConfigTest.php index 2a4c962034079..ee962359c2860 100644 --- a/tests/lib/ConfigTest.php +++ b/tests/lib/ConfigTest.php @@ -160,4 +160,24 @@ public function testConfigMerge() { unlink($additionalConfigPath); } + public function testExceptionOnConfigDirNotWritable() { + $notWritableDir = $this->randomTmpDir.'/not_writable/'; + mkdir($notWritableDir, 0500); + $config = new \OC\Config($notWritableDir, 'testconfig.php'); + + $this->expectException(\OC\HintException::class); + + $config->setValue('foobar', 'baz'); + } + + public function testExceptionOnConfigFileNotWritable() { + $configFile = $this->randomTmpDir.'not_writable_config.php'; + touch($configFile); + chmod($configFile, 0400); + $config = new \OC\Config($this->randomTmpDir, 'not_writable_config.php'); + + $this->expectException(\OC\HintException::class); + + $config->setValue('foobar', 'baz'); + } } From 7b7d9feb6da986b16ff1c44bc244930e9039411e Mon Sep 17 00:00:00 2001 From: Bernd Stellwag Date: Sun, 7 Oct 2018 14:01:47 +0200 Subject: [PATCH 2/5] If there are exceptions thrown, display an error page OC_Util::getInstanceId() does try to set a new value if currently there is no instance id set. This could lead to an exception if the config file is not writable. Signed-off-by: Bernd Stellwag --- lib/base.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/base.php b/lib/base.php index 762646fa4c1a1..0824224645fba 100644 --- a/lib/base.php +++ b/lib/base.php @@ -412,7 +412,20 @@ public static function initSession() { ini_set('session.cookie_path', $cookie_path); // Let the session name be changed in the initSession Hook - $sessionName = OC_Util::getInstanceId(); + try { + $sessionName = OC_Util::getInstanceId(); + } catch (\OC\HintException $e) { + $error = [ + 'error' => $e->getMessage(), + 'hint' => $e->getHint(), + ]; + + http_response_code(503); + OC_Util::addStyle('guest'); + OC_Template::printGuestPage('', 'error', ['errors' => [$error]]); + + exit; + } try { // Allow session apps to create a custom session object From ac1d3e6b5f3c904d6a9809b9ed7971854a760784 Mon Sep 17 00:00:00 2001 From: Bernd Stellwag Date: Sun, 7 Oct 2018 15:08:49 +0200 Subject: [PATCH 3/5] It's also ok if config dir is not writable but config file itself is Signed-off-by: Bernd Stellwag --- lib/private/Config.php | 4 ++-- tests/lib/ConfigTest.php | 20 +++++++++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/lib/private/Config.php b/lib/private/Config.php index 280951bd2bdce..18e12824ed22f 100644 --- a/lib/private/Config.php +++ b/lib/private/Config.php @@ -237,9 +237,9 @@ private function writeData() { $content .= var_export($this->cache, true); $content .= ";\n"; - // Check if config directory is writable and create the config file + // Check if config directory/file is writable and/or create the config file if ( - !is_writable(dirname($this->configFilePath)) + (!is_file($this->configFilePath) && !is_writable(dirname($this->configFilePath))) || !touch($this->configFilePath) || !is_writable($this->configFilePath) ) { diff --git a/tests/lib/ConfigTest.php b/tests/lib/ConfigTest.php index ee962359c2860..6082998df97b6 100644 --- a/tests/lib/ConfigTest.php +++ b/tests/lib/ConfigTest.php @@ -161,7 +161,7 @@ public function testConfigMerge() { } public function testExceptionOnConfigDirNotWritable() { - $notWritableDir = $this->randomTmpDir.'/not_writable/'; + $notWritableDir = $this->randomTmpDir.'not_writable/'; mkdir($notWritableDir, 0500); $config = new \OC\Config($notWritableDir, 'testconfig.php'); @@ -180,4 +180,22 @@ public function testExceptionOnConfigFileNotWritable() { $config->setValue('foobar', 'baz'); } + + public function testNoExceptionOnConfigDirNotWritableButConfigFileWritable() { + $notWritableDir = $this->randomTmpDir.'not_writable/'; + mkdir($notWritableDir, 0700); + + $configFile = $notWritableDir.'writable_config.php'; + touch($configFile); + chmod($configFile, 0600); + + chmod($notWritableDir, 0500); + + $config = new \OC\Config($notWritableDir, 'writable_config.php'); + $config->setValue('foobar', 'baz'); + + $this->assertTrue(true, 'No exception when writing to config file'); + + chmod($notWritableDir, 0700); + } } From 845cce5c49bec82cacf3773ecc82d3a5ca3c415b Mon Sep 17 00:00:00 2001 From: Bernd Stellwag Date: Mon, 8 Oct 2018 20:46:38 +0200 Subject: [PATCH 4/5] Make sure creating a new config file works if the config dir is writable Signed-off-by: Bernd Stellwag --- tests/lib/ConfigTest.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/lib/ConfigTest.php b/tests/lib/ConfigTest.php index 6082998df97b6..b8d2fe276fc7a 100644 --- a/tests/lib/ConfigTest.php +++ b/tests/lib/ConfigTest.php @@ -198,4 +198,11 @@ public function testNoExceptionOnConfigDirNotWritableButConfigFileWritable() { chmod($notWritableDir, 0700); } + + public function testNoExceptionOnCreatingConfigFileInWritableConfigDir() { + $config = new \OC\Config($this->randomTmpDir, 'this_file_does_not_exist.config.php'); + $config->setValue('foobar', 'baz'); + + $this->assertTrue(true, 'No exception when creating config file'); + } } From 60c1fd141ef0eecda8129f31e1a027c87b60f20b Mon Sep 17 00:00:00 2001 From: Bernd Stellwag Date: Mon, 8 Oct 2018 21:50:44 +0200 Subject: [PATCH 5/5] Check if file modes got applied before proceeding with the test Signed-off-by: Bernd Stellwag --- tests/lib/ConfigTest.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/lib/ConfigTest.php b/tests/lib/ConfigTest.php index b8d2fe276fc7a..92753b7f2836e 100644 --- a/tests/lib/ConfigTest.php +++ b/tests/lib/ConfigTest.php @@ -163,6 +163,10 @@ public function testConfigMerge() { public function testExceptionOnConfigDirNotWritable() { $notWritableDir = $this->randomTmpDir.'not_writable/'; mkdir($notWritableDir, 0500); + if (is_writable($notWritableDir)) { + $this->markTestSkipped("Couldn't ensure that the test directory is not writable"); + } + $config = new \OC\Config($notWritableDir, 'testconfig.php'); $this->expectException(\OC\HintException::class); @@ -174,6 +178,10 @@ public function testExceptionOnConfigFileNotWritable() { $configFile = $this->randomTmpDir.'not_writable_config.php'; touch($configFile); chmod($configFile, 0400); + if (is_writable($configFile)) { + $this->markTestSkipped("Couldn't ensure that the test file is not writable"); + } + $config = new \OC\Config($this->randomTmpDir, 'not_writable_config.php'); $this->expectException(\OC\HintException::class); @@ -188,8 +196,14 @@ public function testNoExceptionOnConfigDirNotWritableButConfigFileWritable() { $configFile = $notWritableDir.'writable_config.php'; touch($configFile); chmod($configFile, 0600); + if (!is_writable($configFile)) { + $this->markTestSkipped("Couldn't ensure that the test file is writable"); + } chmod($notWritableDir, 0500); + if (is_writable($notWritableDir)) { + $this->markTestSkipped("Couldn't ensure that the test directory is not writable"); + } $config = new \OC\Config($notWritableDir, 'writable_config.php'); $config->setValue('foobar', 'baz');