FIX Grant access to draft files without permissions

[emteknetnz]

Issue https://github.com/silverstripe/silverstripe-sharedraftcontent/issues/132

[emteknetnz]

The downside of this approach is that it will grant you access to all draft file ... not just the one on the page.

That sounds like it's creating a permissions escalation issue?

Partially undo silverstripe/silverstripe-assets@8c7a719 and FIX Ensure session grant config is respected on uncached file shortcodes silverstripe-assets#403 so it session grants you access to the draft file if it otherwise would be available to anyone

I'm not exactly keen to undo work that was done to fix a CVE. Which bits are you proposing get undone?

[maxime-rainville]

The better solution

On those lines, I would do something like this:

// Retrieve the file URL (ensuring session grant config is respected)
if ($record instanceof File) {
    $grant = $allowSessionGrant || (
        class_exists(Versioned) &&
        Versioned::set_default_reading_mode() === 'Stage.Stage' &&
        !$record->hasRestrictedAccess()
    );
    $url = $record->getURL($grant);
} else {
    $url = $record->Link();
}

Essentially, the idea is that if you can get the site in the draft stage, you should get to see unrestricted draft files.

The alternative solution

The downside of this approach is that it will grant you access to all draft file ... not just the one on the page.

That sounds like it's creating a permissions escalation issue?

Sort of ... basically if I trust you enough to view unrestricted draft files on one page, it's not such a stretch to say I trust you to view all unrestricted draft file. But that is obviously a debatable point and different people might have different answers. That's why I prefer the other solution.

The main upside is that it would be easier to implement then my preferred solution.

[emteknetnz]

Essentially, the idea is that if you can get the site in the draft stage, you should get to see unrestricted draft files.

Would that work for that sharedraftcontent module though? This module allows CMS authors to share links with people who have no access to the CMS at all by creating a shareable link such as:

http://mysite.test/preview/0abea656f3e252cc/cef7ddf1ee3fd2c0

Which then will then generate use Director::handleRequest() to generate page HTML here

[maxime-rainville]

Calling $record->getURL(true); gives your session the right to view the file. It adds a session entry that says your session is allowed to see a specific version of a file. You don't need a user account for it to work.

That's why it was a security issue back in the day, because we would willy-nilly let any anonymous user see files irrespective of any restriction if it was reference on a page.

[emteknetnz]

$grant = $allowSessionGrant || (
        class_exists(Versioned) &&
        Versioned::set_default_reading_mode() === 'Stage.Stage' &&
        !$record->hasRestrictedAccess()
    );

OK so it'll grant if you're on the draft site and the file is unrestricted. That seems pretty safe.

I'd be inclined to add an extension hook to updateGrant since this code is in core and we want a satellite module to update the grant.

Shortcodes are super common so I'm concerned about any additional database queries, and hasRestrictedAccess() will create additional database queries to retrieve the parent record when CanViewType is Inherit - https://github.com/silverstripe/silverstripe-assets/blob/1/src/File.php#L514

To be fair, these will be the same queries that canView() checks will create, though I'm not sure if cached db query results would be shared between the two

I've tested that this alternate approach does work (without this sharedraftcontent PR) here https://github.com/silverstripe/silverstripe-assets/pull/464/files

There's an additional getGrant() check required in codeblock that deals with the cached shortedcode, which complicates things. This would certainly slow down getting cached shortcodes which is a big negative. We can't cache the grant status since it requires checking $record->hasRestrictedAccess(), which can changed by a CMS author in between the results being cached

Extension can make this more performant

FileShortCodeProvider.php

protected static function getGrant(): bool
{
    $grant = static::config()->allow_session_grant;
    $this->extend('updateGrant', $grant);
    return $grant;
}

ShareDraftContentExtension.php

public function upgradeGrant(&$grant)
{
   if (!$requestingShareDraftContentUrl || $grant) {
       return;
   }   
   $grant = class_exists(Versioned::class) &&
            Versioned::get_default_reading_mode() === 'Stage.' . Versioned::DRAFT &&
            !$record->hasRestrictedAccess();
}

[maxime-rainville]

Not there would have been a big cost in terms of implementing this directly into assets. Remember that draft content shouldn't be cached any way.

But using extensions does seem to make this more rigorous in the long run, so go for it.

Maybe double check what happens when you render images directly in templates or when your draft page renders a file/banner elemental blocks. You might need to have a hook there as well. If there's dozens of places where that permission grant check is needed, maybe we need to have central spot where it happens and provide a single extension point.

[maxime-rainville]

Parent issue has been closed. I'm going to assume this PR is no longer needed. Reopen if it still provides value.