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

mkdir() doesn't take care about the umask #189

Merged
merged 1 commit into from
May 29, 2013
Merged

mkdir() doesn't take care about the umask #189

merged 1 commit into from
May 29, 2013

Conversation

kingcrunch
Copy link
Contributor

I use the default setting

cache_mkdir_mode: 0777

The problem now is, that if a umask is set, the permissions for the created folders aren't 0777. This break the cache-cleanup. It should also take care, that every recursively created folder should have the same permissions.

Update: While I was looking for a solution I realized, that during storing the created files no permission mask is applied to mkdir() at all

https://github.com/liip/LiipImagineBundle/blob/master/Imagine/Cache/Resolver/AbstractFilesystemResolver.php#L75

This means, even if I set something different as cache_mkdir_mode it is never applied, is it? Is this intended?

havvg added a commit that referenced this pull request May 29, 2013
mkdir() doesn't take care about the umask
@havvg havvg merged commit 407c4f7 into liip:master May 29, 2013
@havvg
Copy link
Contributor

havvg commented May 29, 2013

@rvanlaak
Copy link

rvanlaak commented Jun 1, 2013

Aside of this, the cache resolver does not take any open_basedir restriction into account, since the resolvers use the request basePath (which is actually the URL). When the check on file_exists is performed, the check will be on /media/etc... which probably isn't in the include path if an open_basedir restriction is active.

@kingcrunch
Copy link
Contributor Author

@rvanlaak What do you recommend? I fear if somebody set open_basedir "wrong" this bundle cannot do very much against it.

@kingcrunch kingcrunch deleted the folder-permissions branch June 1, 2013 12:01
@rvanlaak
Copy link

rvanlaak commented Jun 1, 2013

Well, in my opinion the file_exists call should always should try to locate an absolute location. The open_basedir restriction only occurs when relative paths are used. Any idea how I can retrieve the absolute base-path for the images, then I can make a PR for it.

Edit: I've investigated why the resolving goes wrong, since it is possible to set the web_root in the config. It seems that both the CacheManager and the FileSystemLoader constructors set the rootPath, but first try to resolve it using the php realpath() function. The problem here is that the realpath function can return FALSE, resulting in unpredictable file resolving.

So for now it looks like the open_basedir restriction makes the realpath() function unusable. But, file_exists does resolve the files correct, so it looks like the realpath() call can be removed in all cache and data managers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Level: Bug 🐞 This item involves a legitimate regression (bug) to existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants