[BUG] - Check for private path breaks deployer builds

[dingo-d]

Not sure if this happened to you, but for some reason, the path of the component inside the Components helper on line 147 will return something like

/www/my_project/public/releases/2023-11-20-153149//src/Blocks/components/head/head.php

for the header components (I am rendering some components inside the header.php template).

Because this doesn't contain themes or plugins in it, it will trigger this part:

eightshift-libs/src/Helpers/Components.php

Lines 151 to 153 in aebc4ae

	if (!\preg_match('(themes|plugins)', $componentPath)) {
	throw ComponentException::throwPrivatePath();
	}

When I comment out this part of the code, the site works fine, and loads all the components just fine.

Not sure if you experienced a similar situation on your projects (since you're using deployer), but I'm not sure this is a good way to figure out if something is loaded from a theme or a plugin.

[iruzevic]

this was implemented so we prevent any miss use of this helpers as you can theoretically get project secrets using this helper from the wp-admin, and we don't want that. I will see what I can do here

[dingo-d]

Would it make more sense that the path should always have Blocks/components part in it?
Because we're always rendering components, no?

All the components are located in that folder. I don't think other combinations will work unless you modify the JS/SCSS scripts to load the correct paths, and that would be really non-standard way of doing things. That could then be argued that it's not really the correct way of handling components because of security reasons.

The only thing that comes to mind is that you'd want to render a plugin component inside a theme, but I'm not sure I've ever done that, or seen it being done.

@mbmjertan @goranalkovic-infinum @iruzevic what do you think?

[iruzevic]

well you can render also theme files like single.php but in general you should not do that. I will remove this restriction and see in the future how to fix this

[dingo-d]

Just blanket removing would really introduce a security issue, so for now, try to test if the projects will work if you replace the current regex with Blocks\/components and see if that will work (you can test on one of the staging sites to see if the site will get broken)

[iruzevic]

This was like this for 4 years so it is ok to have it removed.

https://github.com/infinum/eightshift-libs/releases/tag/7.1.0

We will invest more time in the future for this

[mbmjertan]

Thanks for raising this concern @dingo-d, and I believe that we should really dig into this.

Currently, render and renderPartial are unsafe methods: you could call Components::render('../../../../secrets.json'); or even Components::render('../../../../../../../etc/passwd'). The themes/plugin check that was removed in the latest release isn't even really helpful here, given that you could trick it by doing Components::render('../../themes/../../../../secrets.json');, so we haven't lost much by removing it.

This is obviously a bad thing to happen, but unless we have arbitrary code execution somewhere else (in which case we're already screwed as you call do anything at that point), the only risk here is if someone passes user input (attribute value, request data etc.) to the method. Documentation and docblocks should be updated to warn users against that, maybe even coding standards?

Tl;dr: While I'd like to see this being a safe method, I believe the risk is minimal. The removed code is not a solution to this issue however; refactoring the render methods entirely (and breaking all projects) or resolving the path and checking whether it's allowed are. The issue of defining allowed paths remains.

[dingo-d]

Maybe a check could be made to see if the component name is located in the components path, independently of the $parentPath variable? That way you could never fetch a non component file.

In addition to that we could limit the render to only fetch .php files (this way you cannot render .json or things like etc/passwd).