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.

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

tempDir setting is idioatic #701

Closed
worikgh opened this issue Mar 27, 2018 · 10 comments
Closed

tempDir setting is idioatic #701

worikgh opened this issue Mar 27, 2018 · 10 comments

Comments

@worikgh
Copy link

worikgh commented Mar 27, 2018

In ConfigVariables.php
'tempDir' => __DIR__ . '/../../tmp',
Why should this directory exist? The default should be to use sys_get_temp_dir
Passing ['tempDir' => sys_get_temp_dir()] as a argument to the sonstructor gets around it

@Klap-in
Copy link
Contributor

Klap-in commented Mar 27, 2018

This definition is in the ConfigVariables.php file, which lives in: https://github.com/mpdf/mpdf/blob/development/src/Config/ConfigVariables.php

So two levels up, and you are in the root directory.
In this root directory lives the tmp/ folder.
See: https://github.com/mpdf/mpdf/tree/development/tmp

This folder exists to ensure it exists somewhere.

Of course it is better to define it at a more suitable location.
Please see the mpdf manual for details:
https://mpdf.github.io/installation-setup/folders-for-temporary-files.html

@worikgh
Copy link
Author

worikgh commented Mar 27, 2018

mPDF is pre-configured to use <path to mpdf>/tmp as a directory to write temporary files (mainly for images). Write permissions must be set for read/write access for the tmp directory.
Why? What is wrong with sys_get_temp_dir?.

@finwe finwe closed this as completed Mar 28, 2018
@finwe
Copy link
Member

finwe commented Mar 28, 2018

System wide temporary directory presents potential collisions of multiple mPDF instances used by different system users, especially on shared hosts. It also makes cleanup much harder. Custom temporary directory for each installation is safer.

@madorin
Copy link

madorin commented Jun 5, 2018

Using /tmp inside mPdf lib, where sources are located, is not the best idea from security point of view.
Most people will assign full rights to entire source folder to apache/nginx user, normally it should be RO.
Also you have to play with the folder rights each time on site update.
Collisions are solved somehow. This is proved by 99.5% of apps, that uses system temporary directory.

@Klap-in
Copy link
Contributor

Klap-in commented Jun 5, 2018

@finwe
Copy link
Member

finwe commented Jun 5, 2018

The main argument against sys_get_temp_dir() (actually a no-go) in a PHP application is open_basedir restrictions applied on many shared hosts. PHP is not able to write and read in /tmp.

The default tempDir setting is for sake of backwards compatibility.

Its value can be changed on mPDF instantiation to any suitable value by user.

It is possible tempDir will not have a default value and will be required to be set by user in a future major version.

@KazuAlex
Copy link

As madorin says, normally the mPdf installation dir will only have RO rights (that is my situation for the #1047 ). And sys_get_temp_dir() php function is an interface to not create 5000 tmp folders. (THAT makes temporary's files cleanup much harder)
Normally, the /tmp (or other dependly of your configuration) is cleaned regularly by one or another method. (cron, manually or any other methods)
If the problem is the collisions, i can replace the sys_get_temp_dir() by tempnam(sys_get_temp_dir(), 'mPdf') in my pull request.
But actually, using mPdf with phpoffice/phpword bundle do not allow me to change the mpdf's tempDir configuration.
I just want a solution to have a powerful solution. And in my opinion, using sys_get_temp_dir() is the better solution. And people that have "shared hosts" problems is only a shared hosting configuration issue and not a library's configuration issue

@finwe
Copy link
Member

finwe commented Jun 16, 2019

By the same token, people that have "3rd party libraries" problems is only the 3rd party library configuration issue and not the library's configuration issue.

Changing a behaviour of code is much easier than changing behaviour of shared hosting provider. In another point of view, on a shared host with open_basedir in effect, your change would not help users of phpoffice/phpword in any way, as they would't be able to change the default in non-writable default temp dir anyway.

I suggest proposing a change to phpoffice/phpword to allow changing mpdf settings or to accept a custom \Mpdf\Mpdf instance.

@finwe
Copy link
Member

finwe commented Jun 16, 2019

To quote the previous comment,

It is possible tempDir will not have a default value and will be required to be set by user in a future major version.

I consider this discussion closed.

@KazuAlex
Copy link

That's not because one method is "mush easier" that is the best way to do

finwe commented on 5 Jun 2018
It is possible tempDir will not have a default value and will be required to be set by user in a future major version.

One year ago, and nothing ?

KazuAlex pushed a commit to KazuAlex/PHPWord that referenced this issue Jun 17, 2019
KazuAlex pushed a commit to KazuAlex/PHPWord that referenced this issue Jun 17, 2019
KazuAlex pushed a commit to KazuAlex/PHPWord that referenced this issue Jun 17, 2019
…mpdf)

Issue on mpdf library : mpdf/mpdf#701 (comment)

Rejected PR on mpdf library : mpdf/mpdf#1047
KazuAlex pushed a commit to KazuAlex/PHPWord that referenced this issue Jun 17, 2019
…mpdf)

Issue on mpdf library : mpdf/mpdf#701 (comment)

Rejected PR on mpdf library : mpdf/mpdf#1047
KazuAlex pushed a commit to KazuAlex/PHPWord that referenced this issue Jun 19, 2019
…mpdf)

Issue on mpdf library : mpdf/mpdf#701 (comment)

Rejected PR on mpdf library : mpdf/mpdf#1047
KazuAlex pushed a commit to KazuAlex/PHPWord that referenced this issue Jun 19, 2019
…mpdf)

Issue on mpdf library : mpdf/mpdf#701 (comment)

Rejected PR on mpdf library : mpdf/mpdf#1047
KazuAlex pushed a commit to KazuAlex/PHPWord that referenced this issue Jun 19, 2019
…mpdf)

Issue on mpdf library : mpdf/mpdf#701 (comment)

Rejected PR on mpdf library : mpdf/mpdf#1047
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants