Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Show a more specific error message if config file is not writable #11663

Closed
wants to merge 6 commits into from
Closed

Show a more specific error message if config file is not writable #11663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e44ff03
Check if the config file is writable and throw an exception if it's not
burned42 Oct 7, 2018
7b7d9fe
If there are exceptions thrown, display an error page
burned42 Oct 7, 2018
ac1d3e6
It's also ok if config dir is not writable but config file itself is
burned42 Oct 7, 2018
845cce5
Make sure creating a new config file works if the config dir is writable
burned42 Oct 8, 2018
60c1fd1
Check if file modes got applied before proceeding with the test
burned42 Oct 8, 2018
59fac23
Merge branch 'master' into show_error_if_config_not_writable
burned42 Oct 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion lib/base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]]);
MorrisJobke marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the intention of this PR, trying to print a page when config has an error is a bad idea. This will try to load SCSS stuff which might also fail to be written, etc, and then you end up with a blank page instead of message which at least gives you a hint what is wrong.


exit;
}

try {
// Allow session apps to create a custom session object
Expand Down
15 changes: 14 additions & 1 deletion lib/private/Config.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,20 @@ private function writeData() {
$content .= var_export($this->cache, true);
$content .= ";\n";

touch ($this->configFilePath);
// Check if config directory/file is writable and/or create the config file
if (
(!is_file($this->configFilePath) && !is_writable(dirname($this->configFilePath)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would !is_file($this->configFilePath) && !touch($this->configFilePath) work? I guess when either directory or file is not writable touch will fail anyway?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so 👍

Copy link
Contributor Author

@burned42 burned42 Oct 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically you are correct, touch will fail, but when it fails it fails with a PHP Notice generated. I tried to prevent this by first making sure the touch wouldn't fail due to missing write permissions. Another problem is that touch can succeed if the file exists and the current user is the owner of the file but has no write permissions, check https://stackoverflow.com/a/990893 for more information about this.

On further testing I also noticed that we should first touch and then check for is_writable because for a non existing file is_writable also returns false, see 5b2cf3c

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just found this. Just wondering is these function broken? I guess when config not writable there should be an error.

server/lib/base.php

Lines 233 to 268 in 40d185e

public static function checkConfig() {
$l = \OC::$server->getL10N('lib');
// Create config if it does not already exist
$configFilePath = self::$configDir .'/config.php';
if(!file_exists($configFilePath)) {
@touch($configFilePath);
}
// Check if config is writable
$configFileWritable = is_writable($configFilePath);
if (!$configFileWritable && !OC_Helper::isReadOnlyConfigEnabled()
|| !$configFileWritable && \OCP\Util::needUpgrade()) {
$urlGenerator = \OC::$server->getURLGenerator();
if (self::$CLI) {
echo $l->t('Cannot write into "config" directory!')."\n";
echo $l->t('This can usually be fixed by giving the webserver write access to the config directory')."\n";
echo $l->t('See %s', [ $urlGenerator->linkToDocs('admin-dir_permissions') ])."\n";
echo "\n";
echo $l->t('Or, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it.')."\n";
echo $l->t('See %s', [ $urlGenerator->linkToDocs('admin-config') ])."\n";
exit;
} else {
OC_Template::printErrorPage(
$l->t('Cannot write into "config" directory!'),
$l->t('This can usually be fixed by giving the webserver write access to the config directory. See %s',
[ $urlGenerator->linkToDocs('admin-dir_permissions') ]) . '. '
. $l->t('Or, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it. See %s',
[ $urlGenerator->linkToDocs('admin-config') ] ),
503
);
}
}
}

PhpStorm tells me that !$configFileWritable && !OC_Helper::isReadOnlyConfigEnabled() || !$configFileWritable && \OCP\Util::needUpgrade() may not work as expected.

Copy link
Contributor Author

@burned42 burned42 Oct 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a config parameter config_is_read_only which can be set to true if the config file is intentionally not writable, so I guess there shouldn't always be an error if the config file is not writable.

I guess PhpStorm is reporting this because there are no parentheses to clarify the intended order/grouping of those statements, but it seems to do the right thing. At least it makes sense to me, it goes into the if (displaying an error) if either the config file is not writable and not configured to be readonly or if the config file is not writable but an upgrade should be done (which would need to alter the config file).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it makes sense to add a check "is directory writable" to the function above? What do you think is the current check to late? I guess your check is executed much earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I understand, where exactly would you suggest adding a check 'is directory writable' and how would that help?

I'm not sure if checkConfig is called too late, 'my check' is called a bit earlier in base.php via self::initSession() which might try to create a config.php to set an instance ID. But I don't know if changing the order would be better or not. I guess it wouldn't really matter, we just would have to change checkConfig a bit.
checkConfig is using OC_Template::printErrorPage which as I already found out in #11663 (comment) tries to do some DB queries which of course can fail if there is no config.php with some DB settings.
But as 'my check' is running before that it shouldn't happen because otherwise it wouldn't even get that far to call checkConfig.

|| !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
Expand Down
59 changes: 59 additions & 0 deletions tests/lib/ConfigTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,4 +160,63 @@ public function testConfigMerge() {
unlink($additionalConfigPath);
}

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);

$config->setValue('foobar', 'baz');
}

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);

$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);
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');

$this->assertTrue(true, 'No exception when writing to config file');

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');
}
}