October: Overriding partials in subfolders

[tobias-kuendig]

Given the following component structure:

components/mycomponent
├── default.htm
├── subfolder
│   └── default.htm

If I want to override the subfolder/default.htm partial in my theme, I would add it like so:

mytheme
├── partials
│   ├── mycomponent
│   │   └── subfolder
│   │       └── default.htm

Unfortunately, this currently does not work. The override is ignored.

I tracked this issue down to this line:

https://github.com/octobercms/october/blame/master/modules/cms/classes/Controller.php#L1003

It seems like subfolder overrides are explicitly excluded. This line has been there from the very early days. Removing the if check does not seem to break anything obvious, but subfolder overrides begin to work without any futher changes.

So I'm wondering, does any one have any idea why it's there? What's wrong with including partials from subfolders?

[bennothommo]

@tobias-kuendig Best guess from what I can see is that line is to prevent people from accessing files outside of a single folder - since $partialName is a user-contributed parameter, someone could quite easily access a system file as a partial and cause havoc by adding ../ or /usr/local/apache/... or something similar. Unfortunately, the upshot is that this prevent subfolder access too.

A suitable fix for this, in my mind, would be to allow for subfolders, but ensure that the directory tree always resides within a partial directory.

[tobias-kuendig]

Yes, this is obviously the reason for this check.

It looks like there is already code around to check for valid partial paths: https://github.com/octobercms/october/blob/fb2aa1730cc08e8563908b54b70e942bd2988f5e/modules/cms/helpers/File.php#L48

I'm looking further in to this.

[bennothommo]

@tobias-kuendig Have you had a chance to look further into this? :)

[tobias-kuendig]

Not yet! Had a few days off, I will tackle this in the next few days.

[tobias-kuendig]

I did some further checks, this is what I have found out:

:no_entry_sign: Including a partial outside the component's scope is currently blocked.

{% partial __SELF__ ~ '::sub/../../../malicious' %}

:ok: Including a partial from a subfolder is possible, but you cannot override this via the theme.

{% partial __SELF__ ~ '::sub/test' %}

Changing /modules/cms/classes/Controller.php:1003 from

if (strpos($partialName, '/') === false) {
    $partial = ComponentPartial::loadOverrideCached($this->theme, $componentObj, $partialName);
}

to

if (\Cms\Helpers\File::validatePath($partialName)) {
    $partial = ComponentPartial::loadOverrideCached($this->theme, $componentObj, $partialName);
}

yields the same results (.. is not allowed, subfolders work) but it is now possible to override partials in subfolders in the theme.

The validatePath method checks that the partial path contains no .., ./ or // and is at max two levels deep. This should prevent any directory traversal outside of the theme directory.

This is my proposed change to enable subfolder overrides. What do you guys think?

---

:exclamation: A thing I noticed: As soon as the component's alias is omitted, other rules apply. Without the :: part (loading from the theme directory), I am able to include any partial from anywhere (even outside the project).

<!-- This works, the path is relative to the theme directory -->
{% partial 'component/../../../../malicious' %}

[LukeTowers]

@tobias-kuendig the path in the last example currently isn't checked for validity here: https://github.com/octobercms/october/blob/master/modules/cms/classes/Controller.php#L1027-L1029. We could implement valid path checking at the lowest level of the CmsObject class' loadCached() method by replacing the .. check at the start of validatePath() with a call to https://github.com/octobercms/library/blob/8d96a7b239f8b30984ccde8dbfc525558d053197/src/Filesystem/Filesystem.php#L117 (including a check for cms.restrictBaseDir of course)

[faselunare]

Hi, any news on this issue?
i have to override some partials in a subfolder...

[tobias-kuendig]

I just tested this change:

    public static function validatePath($filePath, $maxNesting = 2)
    {
        if ( ! app(Filesystem::class)->isLocalPath($filePath)) {
            return false;
        }

        // Instead of ...
        // if (strpos($filePath, '..') !== false) {
        //     return false;
        // }

Was this what you had in mind, @LukeTowers? With this, partial paths like default are no longer accepted. We would somehow have to resolve the absolute path for the partial before validating the path.

[LukeTowers]

Roughly @tobias-kuendig, although you have to make sure it's checking the cms.restrictBaseDir directive like it's done elsewhere though. And yes, we would need to resolve the path before validating it.

[github-actions[bot]]

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days. If this issue is still relevant or you would like to see action on it, please respond and we will get the ball rolling.

[tobias-kuendig]

I will try to provide a PR in the next few days.

[bennothommo]

@tobias-kuendig Awesome. Let us know how you go.

[github-actions[bot]]

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days. If this issue is still relevant or you would like to see action on it, please respond and we will get the ball rolling.

[tobias-kuendig]

I'm closing this issue since #4652 contains a proposal for this.

[LukeTowers]

Re-opening until #4652 is actually merged.