Deprecate standalone endpoints to match PrestaShop 9.0 new security policy #983

jolelievre · 2024-02-13T12:29:48Z

Questions	Answers
Description?	Allow executing only the specific PHP files as external endpoints, required since PrestaShop 9.0 because additional security was added in this PR PrestaShop/PrestaShop#34184 The idea in this PR is to allow access to very specific files to prevent unwanted accesses on malicious files
Type?	bug fix
BC breaks?	no
Deprecations?	no
Fixed ticket?	~
How to test?	Install a shop based on develop branch, go to the module administration of faceted search and try to build indexes or clear the cache. An error message should appear and when you check the network in browser developer tools you should see that the ajax request was forbidden (403 error). Now use the branch from this PR and do the same, the ajax calls should now be successful.

…quired since PrestaShop 9.0

kpodemski · 2024-02-13T14:19:44Z

My understanding is that we should create ModuleFrontControllers as a replacement for .PHP files, not to show how to omit the rules for developers in the ecosystem :D

kpodemski · 2024-02-13T14:21:35Z

I'll also repeat what @matks already wrote:

I hope the people who participated in this discussion to advocate for this change will be helping this task to be complete 😝 . I plan to submit a few PRs to fix some of the modules listed but I will not do all of them.

maybe we should ping @jf-viguier @clotairer @touchweb-vincent

Hlavtox · 2024-02-13T14:35:12Z

@kpodemski With French efficiency, we will be in grave by then :D

Hlavtox · 2024-02-13T14:45:35Z

@jolelievre Inspiration here - https://github.com/PrestaShop/gsitemap/pull/175/files

Just throw some deprecation messages and create three simple files like:

class Ps_facetedsearchPriceindexerModuleFrontController extends ModuleFrontController
{
    public function postProcess()
    {
      if (substr(Tools::hash('ps_facetedsearch/index'), 0, 10) != Tools::getValue('token')) {
          exit('Bad token');
      }

      Shop::setContext(Shop::CONTEXT_ALL);
      $this->module->indexAttributes();
      $this->module->indexFeatures();
      $this->module->indexAttributeGroup();
    }
}

matks

Message deleted by @matks, see #983 (comment) for context.

clotairer · 2024-02-13T18:58:30Z

Not sure comments opposing dev secops and opensource dev or a "national cliche" serve the cause... I'm all days on Slack if you want request me about something bet not at the same frequency on Github. Neverminds.
Could you please define me a dead line ? I'll try to find a developper for this issue.

jolelievre · 2024-02-14T09:24:06Z

@kpodemski @Hlavtox hmm, you're right introducing dedicated controllers would be cleaner, I'm gonna do that to show the example

matks · 2024-02-14T09:37:53Z

Not sure comments opposing dev secops and opensource dev or a "national cliche" serve the cause... I'm all days on Slack if you want request me about something bet not at the same frequency on Github. Neverminds. Could you please define me a dead line ? I'll try to find a developper for this issue.

Hello here,

Thank you @clotairer for reaching out to me, both in this GitHub thread and in private messages on Slack.

As I explained to Clotaire, yesterday I wrote the message above because I felt kindof like I had been let down. During discussion PrestaShop/PrestaShop#34184 there was a lot of people arguing and a lot of time was spent discussing the change. Initially I was not thrilled with the idea of the change. However I took a step forward and I approved the PR, knowing that it would request some time and energy to reverberate that change into all of the project. Yesterday when I found out the only people who worked on reverberating this change were me and Hlavtox, I felt let down. This is why I wrote I was disappointed. I felt like I was compelled to carry out the changes even though initially I was not pushing for them. So yesterday I expressed my feelings.

Today I regret writing this message. There's a saying "never write an email when you're irritated" I should have listened to this piece of wisdom.

@clotairer has already reached out to me and offered help so things will probably turn out a lot better than I thought. Thank you again for that.

My apologies for the above message, it was snarky and I regret that. Venting off on the internet is never a good thing, the right thing to do is to wait 24 hours and write a better and more constructive message later. I should have done that.

clotairer · 2024-02-14T11:04:40Z

@jolelievre Why not create Command ?
For website in PS 9, already compliant. For previous PS release, the old script could be stay in place and deprecated.

jolelievre · 2024-02-14T14:50:24Z

@clotairer creating command to be run in CLI would be nice, but it's not the same behaviour as the current one which is based on URLs that can be called (I guess it's mainly for merchants who do not have access to cron editing on their server and use external third party services that call the URL as a webhook), so in this PR I only focused on making a similar behaviour

@kpodemski @Hlavtox I created a module front controller to replace these endpoints, I tried to create an Admin one for this but couldn't make it work on develop branch since we now have an automatic CQRF token checking on each request and it requires having a logged session to be validated So external calls are not compatible with CSRF protection which makes sense, so the only solution I found was to put this endpoint on the FO side, it sill integrates its custom token validation system which is the same as the standalone endpoint

The standalone endpoints have been deprecated and should be cleaned in the next major version of this module along with the .htaccess file

Hlavtox · 2024-02-14T15:06:11Z

controllers/front/cron.php

+
+    public function postProcess()
+    {
+        if (substr(Tools::hash('ps_facetedsearch/index'), 0, 10) != Tools::getValue('token') || !Module::isInstalled('ps_facetedsearch')) {


I think that !Module::isInstalled('ps_facetedsearch') can be removed, the controller should not be callable if the module is not active or installed.

matks · 2024-02-15T09:15:01Z

ps_facetedsearch-attribute-indexer.php

@@ -17,6 +17,14 @@
 * @copyright Since 2007 PrestaShop SA and Contributors
 * @license   https://opensource.org/licenses/AFL-3.0 Academic Free License 3.0 (AFL-3.0)
 */
+
+/*
+ * This standalone endpoint is deprecated, it should not be sued anymore and should be removed along with the


Suggested change

* This standalone endpoint is deprecated, it should not be sued anymore and should be removed along with the

* This standalone endpoint is deprecated, it should not be used anymore and should be removed along with the

Yes please don't sue it the poor guy

matks · 2024-02-15T09:15:26Z

ps_facetedsearch-clear-cache.php

@@ -17,6 +17,14 @@
 * @copyright Since 2007 PrestaShop SA and Contributors
 * @license   https://opensource.org/licenses/AFL-3.0 Academic Free License 3.0 (AFL-3.0)
 */
+
+/*
+ * This standalone endpoint is deprecated, it should not be sued anymore and should be removed along with the


Suggested change

* This standalone endpoint is deprecated, it should not be sued anymore and should be removed along with the

* This standalone endpoint is deprecated, it should not be used anymore and should be removed along with the

matks · 2024-02-15T09:15:35Z

ps_facetedsearch-price-indexer.php

@@ -17,6 +17,14 @@
 * @copyright Since 2007 PrestaShop SA and Contributors
 * @license   https://opensource.org/licenses/AFL-3.0 Academic Free License 3.0 (AFL-3.0)
 */
+
+/*
+ * This standalone endpoint is deprecated, it should not be sued anymore and should be removed along with the


Suggested change

* This standalone endpoint is deprecated, it should not be sued anymore and should be removed along with the

* This standalone endpoint is deprecated, it should not be used anymore and should be removed along with the

matks · 2024-02-15T09:15:59Z

ps_facetedsearch.php

-            'full_price_indexer_url' => $moduleUrl . 'ps_facetedsearch-price-indexer.php' . '?token=' . substr(Tools::hash('ps_facetedsearch/index'), 0, 10) . '&full=1',
-            'attribute_indexer_url' => $moduleUrl . 'ps_facetedsearch-attribute-indexer.php' . '?token=' . substr(Tools::hash('ps_facetedsearch/index'), 0, 10),
-            'clear_cache_url' => $moduleUrl . 'ps_facetedsearch-clear-cache.php' . '?token=' . substr(Tools::hash('ps_facetedsearch/index'), 0, 10),
+            'price_indexer_url' => $this->context->link->getModuleLink('ps_facetedsearch', 'cron', ['ajax' => true, 'action' => 'indexPrices', 'token' => $cronToken]),


jolelievre · 2024-02-15T09:24:07Z

Thanks @matks and @Hlavtox all should be good now

AureRita · 2024-02-16T09:52:52Z

Hi @jolelievre

Thank you for your PR, I tested it and it seems to works on develop and also on 8.1.x, as we can see :

recording.104.webm

Because the PR seems to works as expected, It's QA ✔️

Thank you

Allow executing only the specific PHP files as external endpoints, re…

1677f3b

…quired since PrestaShop 9.0

jolelievre force-pushed the attribute-deletion-update branch from d591e95 to 1677f3b Compare February 13, 2024 12:31

jolelievre added this to the 3.15.0 milestone Feb 13, 2024

boherm previously approved these changes Feb 13, 2024

View reviewed changes

ps-jarvis added the Waiting for QA label Feb 13, 2024

boherm self-requested a review February 13, 2024 14:30

matks previously approved these changes Feb 13, 2024

View reviewed changes

jolelievre marked this pull request as draft February 14, 2024 09:24

florine2623 mentioned this pull request Feb 14, 2024

ps_facetedsearch - When an attribute value is deleted, it is still displayed even after rebuilding the index PrestaShop/PrestaShop#35331

Closed

2 tasks

jolelievre dismissed stale reviews from matks and boherm via 8551df8 February 14, 2024 14:42

jolelievre removed the Waiting for QA label Feb 14, 2024

jolelievre force-pushed the attribute-deletion-update branch from 8551df8 to 98164b6 Compare February 14, 2024 14:44

jolelievre marked this pull request as ready for review February 14, 2024 14:50

jolelievre changed the title ~~Allow execution of specific endpoints to match PrestaShop 9.0 new security policy~~ Deprecate standalone endpoints to match PrestaShop 9.0 new security policy Feb 14, 2024

Hlavtox reviewed Feb 14, 2024

View reviewed changes

matks previously approved these changes Feb 15, 2024

View reviewed changes

ps-jarvis added the Waiting for QA label Feb 15, 2024

jolelievre added 2 commits February 15, 2024 10:23

Create front controller to replace old endpoints

2057141

Deprecate all standalone endpoints

0286927

jolelievre dismissed matks’s stale review via 0286927 February 15, 2024 09:23

jolelievre force-pushed the attribute-deletion-update branch from 98164b6 to 0286927 Compare February 15, 2024 09:23

matks approved these changes Feb 15, 2024

View reviewed changes

jolelievre mentioned this pull request Feb 15, 2024

Enable attribute migrated page PrestaShop/PrestaShop#35270

Merged

MatShir mentioned this pull request Feb 15, 2024

Release 3.15.0 #981

Merged

boherm approved these changes Feb 15, 2024

View reviewed changes

kpodemski approved these changes Feb 16, 2024

View reviewed changes

AureRita self-assigned this Feb 16, 2024

AureRita approved these changes Feb 16, 2024

View reviewed changes

AureRita added QA ✔️ and removed Waiting for QA labels Feb 16, 2024

jolelievre merged commit 0423259 into PrestaShop:dev Feb 16, 2024
11 checks passed

Hlavtox mentioned this pull request Feb 16, 2024

Adapting software & doc for change #34184 "Forbidden sensitive files extensions on modules directory" PrestaShop/PrestaShop#34578

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate standalone endpoints to match PrestaShop 9.0 new security policy #983

Deprecate standalone endpoints to match PrestaShop 9.0 new security policy #983

jolelievre commented Feb 13, 2024

kpodemski commented Feb 13, 2024

kpodemski commented Feb 13, 2024

Hlavtox commented Feb 13, 2024

Hlavtox commented Feb 13, 2024

matks left a comment •

edited

clotairer commented Feb 13, 2024 •

edited

jolelievre commented Feb 14, 2024

matks commented Feb 14, 2024

clotairer commented Feb 14, 2024 •

edited

jolelievre commented Feb 14, 2024

Hlavtox Feb 14, 2024

matks Feb 15, 2024

matks Feb 15, 2024

matks Feb 15, 2024

matks Feb 15, 2024

jolelievre commented Feb 15, 2024

AureRita commented Feb 16, 2024

	* This standalone endpoint is deprecated, it should not be sued anymore and should be removed along with the
	* This standalone endpoint is deprecated, it should not be used anymore and should be removed along with the

Deprecate standalone endpoints to match PrestaShop 9.0 new security policy #983

Deprecate standalone endpoints to match PrestaShop 9.0 new security policy #983

Conversation

jolelievre commented Feb 13, 2024

kpodemski commented Feb 13, 2024

kpodemski commented Feb 13, 2024

Hlavtox commented Feb 13, 2024

Hlavtox commented Feb 13, 2024

matks left a comment • edited

Choose a reason for hiding this comment

clotairer commented Feb 13, 2024 • edited

jolelievre commented Feb 14, 2024

matks commented Feb 14, 2024

clotairer commented Feb 14, 2024 • edited

jolelievre commented Feb 14, 2024

Hlavtox Feb 14, 2024

Choose a reason for hiding this comment

matks Feb 15, 2024

Choose a reason for hiding this comment

matks Feb 15, 2024

Choose a reason for hiding this comment

matks Feb 15, 2024

Choose a reason for hiding this comment

matks Feb 15, 2024

Choose a reason for hiding this comment

jolelievre commented Feb 15, 2024

AureRita commented Feb 16, 2024

matks left a comment •

edited

clotairer commented Feb 13, 2024 •

edited

clotairer commented Feb 14, 2024 •

edited