Store the crawl logs in a unique subfolder per installation #1865

bohnmedia · 2020-06-24T11:29:11Z

I moved a contao instance to a sharehoster (strato) and got the following error when i tried to generate a crawl index.

The stream or file "/var/tmp//contao-crawl/3ce19d12-15a6-45df-89f3-b717adb35f51_log.csv" could not be opened: failed to open stream: Permission denied

It turned out that the folder "/var/tmp/contao-crawl/" already was created by another contao instance on the same sharehoster. I was able to read the folder with "readdir", load the files in it and see all crawled pages.

After changing the two lines in the Crawler.php, the crawler worked fine. Furthermore its not possible to use "readdir" on "/var/tmp/" so it isnt possible to scan for other contao instances by scanning "/var/tmp/contao-crawl/" anymore.

m-vo · 2020-06-24T11:36:26Z

core-bundle/src/Resources/contao/classes/Crawl.php

@@ -99,7 +99,7 @@ public function run()

 		$jobId = Input::get('jobId');
 		$queue = $factory->createLazyQueue();
-		$crawLogsDir = sys_get_temp_dir() . '/contao-crawl';
+		$crawLogsDir = sys_get_temp_dir() . '/contao-crawl-' . $jobId;


Should imo rather be something like /contao-crawl/md5(%kernel.projectDir%).

Note: sys_get_temp_dir() can contain a trailing slash, as visible in this example on Strato.

Mabe something like this, then 🙂

Suggested change

$crawLogsDir = sys_get_temp_dir() . '/contao-crawl-' . $jobId;

$crawLogsDir = Path::join(sys_get_temp_dir(), 'contao/crawl', md5(System::getContainer()->getParameter('kernel.projectDir'));

I like the use of Path::join. But its important to use a unique folder in tmp. Otherwise the next user that tries to write into the folder "contao" would also get a "permission denied".

Maybe something like this.

$crawLogsDir = Path::join(sys_get_temp_dir(), 'contao-crawl-' . $jobId);

Otherwise the next user that tries to write into the folder "contao" would also get a "permission denied".

The main problem is, that your hoster is using a wrong tmp folder configuration. Every user needs to have its own tmp folder.

I contactet strato to clarify the problem.

@bohnmedia see #267 (comment)

Every user needs to have its own tmp folder.

This is not generally true. Shared folders such as /tmp or /var/lib/php/sessions often have the sticky bit set to protect the files.

fritzmg · 2020-06-24T11:36:40Z

Related: #1813

fritzmg · 2020-06-24T11:37:38Z

Note: sys_get_temp_dir() can contain a trailing slash, as visible in this example on Strato.

fritzmg · 2020-06-24T11:44:36Z

Also related: #267 - using a folder based on the kernel.projectDir might still fail in special circumstances apparently.

m-vo · 2020-06-24T13:34:48Z

Thinking about it again, this

It turned out that the folder "/var/tmp/contao-crawl/" already was created by another contao instance

should never be a problem if multiple Contao applications are running in the same hosting. So imo no fix is needed.

fritzmg · 2020-06-24T13:48:21Z

@m-vo he probably means, by another Contao installation from a completely different user.

m-vo · 2020-06-24T13:50:14Z

he probably means, by another Contao installation from a completely different user.

Yeah, well this is clearly a misconfiguration by the hoster. In which case we shouldn't do anything as well, don't we?

bohnmedia · 2020-06-24T13:50:28Z

Its a completely different customer. Sounds like strato has to fix this problem. I am waiting for their answer and will keep you up to date.

fritzmg · 2020-06-24T13:56:22Z

Yeah, well this is clearly a misconfiguration by the hoster. In which case we shouldn't do anything as well, don't we?

Yes, however, similar adjustments were made to the old AbstractLockedCommand for that reason, before it was eventually dropped.

asaage · 2020-06-24T15:57:11Z

Reading this i checked my own sys_temp_dir wich was not set in php.ini /tmp was utilised via TMPDIR by default.
File-ownership was correct however - do you realy think having a serverwide tmp-dir is a misconfiguration?
I then set it to /home/myown/tmp
And was greeted by the manager:

which was because i did not create the directory 🙈.

I doubt that it's good if getenv('TMPDIR') and sys_get_temp_dir() return different values.

m-vo · 2020-06-24T16:01:33Z

do you realy think having a serverwide tmp-dir is a misconfiguration?

As already outlined, it's a potential security issue. 🤷

bohnmedia · 2020-06-25T10:04:24Z

It seems, that other webhosters also share the tmp-folder by default.

https://help.dreamhost.com/hc/en-us/articles/216735938--tmp-directory-overview

I was wondering for what operations the folder "system/tmp" is used for and if it could be an alternative?

$crawLogsDir = System::getContainer()->getParameter('kernel.project_dir') . '/system/tmp/contao-crawl';
...
return System::getContainer()->getParameter('kernel.project_dir') . '/system/tmp/contao-crawl/' . $jobId . '_' . $subscriberName . '_log.csv';

fritzmg · 2020-06-25T10:07:56Z

I was wondering for what operations the folder "system/tmp" is used for and if it could be an alternative?

The whole system/ folder is there for legacy reasons. (var/tmp/ would be better suited.)

leofeyer · 2020-06-25T10:25:53Z

If we must continue to use sys_get_temp_dir(), even though it is error-prone and even the Symfony devs have already discussed adding a var/tmp folder in symfony/symfony#36515, we should generate installation specific subfolders as we do here:

contao/core-bundle/src/Command/AbstractLockedCommand.php

Lines 59 to 69 in 8e364e7

    
           private function getTempDir(): string 
        
           { 
        
               $container = $this->getContainer(); 
        
               $tmpDir = sys_get_temp_dir().'/'.md5($container->getParameter('kernel.project_dir')); 
        
               if (!is_dir($tmpDir)) { 
        
                   $container->get('filesystem')->mkdir($tmpDir); 
        
               } 
        
               return $tmpDir; 
        
           }

bohnmedia · 2020-06-26T14:27:16Z

If we must continue to use sys_get_temp_dir(), even though it is error-prone and even the Symfony devs have already discussed adding a var/tmp folder in symfony/symfony#36515, we should generate installation specific subfolders as we do here:

contao/core-bundle/src/Command/AbstractLockedCommand.php

Lines 59 to 69 in 8e364e7

private function getTempDir(): string

{

$container = $this->getContainer();

$tmpDir = sys_get_temp_dir().'/'.md5($container->getParameter('kernel.project_dir'));

if (!is_dir($tmpDir)) {

$container->get('filesystem')->mkdir($tmpDir);

}

return $tmpDir;

}

The Crawl.php ist generating a md5 hashed folder name based on kernel.project_dir now. Furthermore pathnames are generated with Path::join now to avoid double slashes.

bohnmedia · 2020-06-26T14:50:51Z

Sorry, didnt realize that Webmozart is no requirement of the contao-core. Removed Path::join.

m-vo · 2020-06-26T15:00:56Z

Sorry, didnt realize that Webmozart is no requirement of the contao-core. Removed Path::join.

It's implicitly there anyway (for example via contao/image).

fritzmg · 2020-06-26T15:15:58Z

It's implicitly there anyway (for example via contao/image).

Hm, if the core-bundle uses it, it should be required. Same for #1868

fritzmg · 2020-06-26T15:18:12Z

core-bundle/src/Resources/contao/classes/Crawl.php

@@ -99,7 +99,7 @@ public function run()

 		$jobId = Input::get('jobId');
 		$queue = $factory->createLazyQueue();
-		$crawLogsDir = sys_get_temp_dir() . '/contao-crawl';
+		$crawLogsDir = sys_get_temp_dir() . md5(System::getContainer()->getParameter('kernel.project_dir')) . '/contao-crawl';


This won't work, if sys_get_temp_dir() contains no trailing slash.

m-vo · 2020-06-26T15:20:25Z

Yes, of course. I just wanted to say that using it won't stretch our requirements budget. 🙂

bohnmedia · 2020-06-26T15:23:01Z

It's implicitly there anyway (for example via contao/image).

Hm, if the core-bundle uses it, it should be required. Same for #1868

But where does it come from? Cant find it in the composer.json of contao.

https://github.com/contao/contao/blob/master/core-bundle/composer.json

fritzmg · 2020-06-26T15:29:23Z

But where does it come from?

contao/image, as @m-vo already said.

Cant find it in the composer.json of contao.

https://github.com/contao/contao/blob/master/core-bundle/composer.json

Yes, you need to add it in this PR.

m-vo · 2020-06-26T15:30:44Z

Like I said, It's from a dependency. You can use composer why webmozart/path-util --tree to find out.

It has to be put into the core's composer.json if you're using it in the core's source, but it won't - to this date - install any new packages.

bohnmedia · 2020-06-26T15:39:37Z

Ok, I got it. Files are changed again.

leofeyer · 2020-06-30T07:34:41Z

@bohnmedia Since this is a bugfix, can you please rebase your changes onto the 4.9 branch?

bohnmedia · 2020-06-30T09:43:52Z

I am not sure how to do a rebase. I am getting many conflicts when I try to do "git rebase origin/4.9". I could create a completetly new pull request.

leofeyer · 2020-06-30T10:06:28Z

That's ok as well. But you should probably wait up the discussion regarding Path::join() here, because IMHO it is an unnecessary dependency that could be avoided.

bohnmedia · 2020-06-30T10:16:26Z

Alternatively, we can use rtrim to avoid the double slash.

rtrim(sys_get_temp_dir(), '/') . '/' . md5(System::getContainer()->getParameter('kernel.project_dir')) . '/contao-crawl/' . $jobId . '_' . $subscriberName . '_log.csv'

leofeyer · 2020-06-30T12:33:35Z

Alternatively, we can use rtrim to avoid the double slash.

Yes, that is what I prefer as well. But let's see.

leofeyer · 2020-07-01T12:34:02Z

I have rebased the PR and adjusted the implementation. @bohnmedia Can you please test if it works for you?

core-bundle/src/Resources/contao/classes/Crawl.php

Toflar

When you merge, please make sure the commit message does not contain "crawling problems" because that's not correct.

leofeyer · 2020-07-03T08:19:31Z

Thank you @bohnmedia.

m-vo reviewed Jun 24, 2020

View reviewed changes

fritzmg requested changes Jun 26, 2020

View reviewed changes

bohnmedia requested a review from fritzmg June 26, 2020 15:45

fritzmg previously approved these changes Jun 26, 2020

View reviewed changes

leofeyer assigned bohnmedia Jun 30, 2020

leofeyer added the bug label Jun 30, 2020

leofeyer added this to the 4.9 milestone Jun 30, 2020

Fixed crawling problems on sharehoster

5987d7a

leofeyer dismissed fritzmg’s stale review via 5987d7a July 1, 2020 12:18

leofeyer changed the base branch from master to 4.9 July 1, 2020 12:18

Adjust the implementation

88da5c4

bohnmedia commented Jul 1, 2020

View reviewed changes

core-bundle/src/Resources/contao/classes/Crawl.php Show resolved Hide resolved

leofeyer requested a review from a team July 3, 2020 07:14

fritzmg approved these changes Jul 3, 2020

View reviewed changes

Toflar approved these changes Jul 3, 2020

View reviewed changes

leofeyer changed the title ~~Fixed crawling problems on sharehoster (strato)~~ Store the crawl logs in a unique subfolder per installation Jul 3, 2020

leofeyer merged commit 9969120 into contao:4.9 Jul 3, 2020

bohnmedia removed their assignment Jul 8, 2021

leofeyer assigned bohnmedia Jan 23, 2023

	$crawLogsDir = sys_get_temp_dir() . '/contao-crawl-' . $jobId;
	$crawLogsDir = Path::join(sys_get_temp_dir(), 'contao/crawl', md5(System::getContainer()->getParameter('kernel.projectDir'));

Store the crawl logs in a unique subfolder per installation #1865

Store the crawl logs in a unique subfolder per installation #1865

Conversation

bohnmedia commented Jun 24, 2020

m-vo Jun 24, 2020

Choose a reason for hiding this comment

m-vo Jun 24, 2020

Choose a reason for hiding this comment

bohnmedia Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

fritzmg Jun 24, 2020

Choose a reason for hiding this comment

bohnmedia Jun 24, 2020

Choose a reason for hiding this comment

m-vo Jun 24, 2020

Choose a reason for hiding this comment

leofeyer Jun 25, 2020

Choose a reason for hiding this comment

fritzmg commented Jun 24, 2020

fritzmg commented Jun 24, 2020

fritzmg commented Jun 24, 2020

m-vo commented Jun 24, 2020

fritzmg commented Jun 24, 2020

m-vo commented Jun 24, 2020

bohnmedia commented Jun 24, 2020 • edited Loading

fritzmg commented Jun 24, 2020

asaage commented Jun 24, 2020

m-vo commented Jun 24, 2020

bohnmedia commented Jun 25, 2020 • edited Loading

fritzmg commented Jun 25, 2020

leofeyer commented Jun 25, 2020 • edited Loading

bohnmedia commented Jun 26, 2020

bohnmedia commented Jun 26, 2020

m-vo commented Jun 26, 2020

fritzmg commented Jun 26, 2020

fritzmg Jun 26, 2020

Choose a reason for hiding this comment

m-vo commented Jun 26, 2020

bohnmedia commented Jun 26, 2020

fritzmg commented Jun 26, 2020

m-vo commented Jun 26, 2020 • edited Loading

bohnmedia commented Jun 26, 2020

leofeyer commented Jun 30, 2020

bohnmedia commented Jun 30, 2020

leofeyer commented Jun 30, 2020

bohnmedia commented Jun 30, 2020 • edited Loading

leofeyer commented Jun 30, 2020

leofeyer commented Jul 1, 2020

Toflar left a comment

Choose a reason for hiding this comment

leofeyer commented Jul 3, 2020

bohnmedia Jun 24, 2020 •

edited

Loading

bohnmedia commented Jun 24, 2020 •

edited

Loading

bohnmedia commented Jun 25, 2020 •

edited

Loading

leofeyer commented Jun 25, 2020 •

edited

Loading

m-vo commented Jun 26, 2020 •

edited

Loading

bohnmedia commented Jun 30, 2020 •

edited

Loading