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

Protect modules vendor folder on install/upgrade/enable #17036

Merged
merged 3 commits into from Jan 10, 2020

Conversation

@jolelievre
Copy link
Contributor

jolelievre commented Jan 6, 2020

Questions Answers
Branch? 1.7.6.x
Description? Create HtaccessFolderProtector to protect folder, it is bound to module install, enable and upgrade
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? ~
How to test? Install a module that contains a vendor folder (welcome for example) Check that the vendor folder contains a .htaccess file after installation (should also work with upgrade, enable and on fresh install)

This change is Reviewable

…le install and upgrade
@jolelievre jolelievre requested a review from PrestaShop/prestashop-core-developers as a code owner Jan 6, 2020
@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Jan 6, 2020

Needs to target 1.7.6.x

@jolelievre jolelievre changed the base branch from develop to 1.7.6.x Jan 6, 2020
@jolelievre jolelievre added this to In progress in PrestaShop 1.7.6 via automation Jan 6, 2020
@jolelievre jolelievre added this to the 1.7.6.3 milestone Jan 6, 2020

$htaccessPath = $folderPath . DIRECTORY_SEPARATOR . '.htaccess';
if (!file_exists($htaccessPath)) {
if (!@file_put_contents($htaccessPath, $this->htaccessContent)) {

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jan 6, 2020

Contributor

use is_writable($folderPath) instead of hiding error with @

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jan 6, 2020

Author Contributor

Sadly I can't do that is_writable returns false on non existing files (which is always the case for this use case)
So I can only check if file_put_contents was successful

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jan 6, 2020

Contributor

This is why I talked about the $folderPath :)

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jan 6, 2020

Author Contributor

Ooooohh! My bad 😅
Done!

@jolelievre jolelievre force-pushed the jolelievre:htaccess-protection branch from ce7b1b0 to e6b4b6b Jan 6, 2020
@@ -0,0 +1,59 @@
<?php
/**
* 2007-2019 PrestaShop SA and Contributors

This comment has been minimized.

Copy link
@Progi1984

Progi1984 Jan 7, 2020

Contributor
Suggested change
* 2007-2019 PrestaShop SA and Contributors
* 2007-2020 PrestaShop SA and Contributors
* needs please refer to https://www.prestashop.com for more information.
*
* @author PrestaShop SA <contact@prestashop.com>
* @copyright 2007-2019 PrestaShop SA and Contributors

This comment has been minimized.

Copy link
@Progi1984

Progi1984 Jan 7, 2020

Contributor
Suggested change
* @copyright 2007-2019 PrestaShop SA and Contributors
* @copyright 2007-2020 PrestaShop SA and Contributors

and so on...

use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
* Listens to install/upgrade events from module manager, and protects the module vendor

This comment has been minimized.

Copy link
@Progi1984

Progi1984 Jan 7, 2020

Contributor
Suggested change
* Listens to install/upgrade events from module manager, and protects the module vendor
* Listen install/upgrade events from module manager, and protect the module vendor
* Class HtaccessFolderProtector protects a designated folder by inserting an htaccess file in it
* which prevents access from an external call.
*/
class HtaccessFolderProtector implements FolderProtectorInterface

This comment has been minimized.

Copy link
@Progi1984

Progi1984 Jan 7, 2020

Contributor
Suggested change
class HtaccessFolderProtector implements FolderProtectorInterface
class HtaccessFolderGuard implements FolderProtectorInterface

Sounds more 🇬🇧

Copy link
Contributor

Progi1984 left a comment

@jolelievre Some feedback

@Progi1984 Progi1984 moved this from In progress to To be tested in PrestaShop 1.7.6 Jan 7, 2020
@Robin-Fischer-PS Robin-Fischer-PS self-assigned this Jan 10, 2020
@Robin-Fischer-PS Robin-Fischer-PS moved this from To be tested to To be merged in PrestaShop 1.7.6 Jan 10, 2020
@Progi1984 Progi1984 merged commit a60c143 into PrestaShop:1.7.6.x Jan 10, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
PrestaShop 1.7.6 automation moved this from To be merged to Done Jan 10, 2020
@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Jan 10, 2020

Thanks @jolelievre

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 13, 2020

Sorry guys, I'm quite late for the party 😅I just saw this PR.

In order to ease unit testing, do you think it would make sense, in a new PR, to use vFsStream to handle I/O simulations ?
And maybe also use a FileSystem implementation to handle file contents inside the source code, instead of raw file_put_contents() ?

The reason I say this is that we have issues in the migration with classes which perform files/folders operations, the issues mainly arise when writing integration tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.