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

Fix filesystem permission issues #5372

Merged
merged 2 commits into from
Apr 22, 2017
Merged

Fix filesystem permission issues #5372

merged 2 commits into from
Apr 22, 2017

Conversation

BlackIkeEagle
Copy link
Member

@BlackIkeEagle BlackIkeEagle commented Jun 30, 2016

When you have configured some strict filesystem permissions to only allow the minimum required write access to the magento application there are some issues

for example:

we have an existing robots.txt that is writable, but the parent directory is writable.

When you create a sitemap and have the option checked to automatically add it to the robots.txt file, everything goes south. Because the assertWritable checks if the parent directory is writable which is totally not nescessary because the file is writable.

Additionally there were some inconsistencies in the filesystem directory stuff.

And as an extra there is an extra check to avoid concatinating the absolute basepath with a path that is already absolute. (this could for example cause basepath='/var/www/website', path='/var/www/website/robots.txt' if you pass in both to get the aboslute path the result would be '/var/www/website/var/www/website/robots.txt' which was not the goal, now it wil properly return '/var/www/website/robots.txt')

It would be nice to have these fixes backported to 2.0 and 2.1 because we have this sort of issues in our production environments at the moment.

@BlackIkeEagle
Copy link
Member Author

Do I have to close this ticket because there has been no activity for over two weeks?

@okorshenko
Copy link
Contributor

@BlackIkeEagle sorry for the delay over 2 weeks.
Could you please resolve conflicts so that we can proceed with PR acceptance. Thank you

BlackIkeEagle and others added 2 commits April 18, 2017 15:35
in the driver classes in Magento\Framework\Filesystem\Driver the
getAbsolutePath could be called with a basepath and a path that is
already absolute, so when the path is already absolute just return that
with the scheme instead of concatinating basepath and path resulting in
a faulty absolute path.

Signed-off-by: Ike Devolder <[email protected]>
@BlackIkeEagle
Copy link
Member Author

@okorshenko I noticed some of the changes I had in there already landed in the magento code base in a slightly different format but same behaviour. The question that remains is the check if we already have a absolute path is desired or not?

@okorshenko
Copy link
Contributor

okorshenko commented Apr 19, 2017

@BlackIkeEagle could you please provide a reference to the changes that you mentioned?

The question that remains is the check if we already have a absolute path is desired or not?

If there is a use case when lack of this validation will cause an issue - yes, we need it.

@BlackIkeEagle
Copy link
Member Author

@okorshenko I don't remember what my original fix was anymore

the other part of the fix that still is in here was because we were trying to write robots.txt and there was already a absolute path in so it concatenated to something like /var/www/magento/var/www/magento/robots.txt

@magento-team magento-team merged commit abbb03e into magento:develop Apr 22, 2017
@magento-team
Copy link
Contributor

@BlackIkeEagle thank you for your contribution to Magento 2 project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants