silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 66 forks source link

protect assets still have hash in the file link, and the old file link doesn't redirect to the new file #304

Open normann opened 5 years ago

normann commented 5 years ago

Affected Version

Replicated in 4.4.1, might be replicable in 4.4.0

Description

Give the fact that version 4.4 is trying to solve the file link dynamically changing due to a hash (to refect file version) in the URL, once the file is replaced by another file where we can see the issue prior to version 4.3 (inclusive), but the fixes or the feature seemed not affecting protected assets

Steps to Reproduce

  1. [OK] Login as Admin and visit 'admin/assets'
  2. [OK] Create a Folder and set the folder to be protected, i.e. config those Who can view to Only These People and Viewer Group to an Administration group
  3. [OK] Upload a file to the folder created in step 2, edit the file and Save and Publish
  4. [OK] Insert a link pointing to the file created in step 3 to a page, *Save and Publish** the Page
  5. [Fail] Landing the page in another tab, copy the file's address and paste it to a text editor, observe that the link, expecting it is not containing the versioning hash in the URL, but it still containing a hash
  6. [OK] Paste the file's URL in a new tab of the same browser, the file is accessible.
  7. [OK] Go to the file's editing interface, make a replacement operation on the file, with a new file (either the same file with some changes or another file so we could easily compare later)
  8. [OK] Refresh the page with the file link, inspect the file link has changed with a new hash in the URL.
  9. [OK] Click on the file link or open it in a new tab, the content accessed shows it is the new file.
  10. [Fail] Revisit the old File link that is copied and put it in the text editor in Step 5, expecting it is either show the new's File's content or redirecting to the new file link. But, the link simply shows a "Not Found"
normann commented 5 years ago

If Step 5 is not considered as a failure, i.e. this is supposed behavior by SS4.4, at least Step 10 should be considered as a failure, right? An old URL bookmarked by site user should either redirect to the new URL or load with the new file content.

michalkleiner commented 5 years ago

The issue might be better logged on the https://github.com/silverstripe/silverstripe-asset-admin repo.

normann commented 5 years ago

Yes, noticed that after the creation of the issue :) @michalkleiner I think I will manage to migrate the issue to either SilverStripe-asset-admin or SilverStripe-assets(now they are separated), will ask SilverStripe though.

ScopeyNZ commented 5 years ago

I'd say this probably belongs in assets, not asset-admin. The administration screens are being used correctly but the URLs generated to serve the assets are wrong - which implies it's assets. I'll move it for you.

ScopeyNZ commented 5 years ago

Not quite sure how to rank the impact of this issue yet - I haven't had an opportunity to look into it.

normann commented 5 years ago

@ScopeyNZ thanks.

Could you migrate another issue to be under silverstripe-assets https://github.com/silverstripe/silverstripe-framework/issues/9141 which I think more serious

chillu commented 5 years ago

In terms of impact, I think this is limited to intranet-style SilverStripe usage where the intranet isn't IP whitelisted, but rather protected via CanView=LoggedInUsers on Page and File records. Since you could achieve this via silverstripe/securefiles in 3.x already, and that module is part of CWP, there will be existing 3.x users who are used to hotlinking to those files. When upgrading to 4.x, those hotlinks break. The short answer is that you should access protected files via a page that's listing them, but of course users will bookmark/share whatever URL they get in the browser for the file.

So I'd say this is a known limitation, but contributions are welcome. As as starting point, you'd need to look at FlysystemAssetStore->getResponseFor(), and modify either HashFileIDHelper or NaturalFileIDHelper to identify this file, and cause a redirect.

Zazama commented 2 years ago

I just experienced the same issue upgrading a SS3 instance to SS4, like @chillu said, it's a private website where the assets are all set to protected.

After upgrading, we had many broken image links in our Content fields, e.g. assets/2017/02/something.jpg which, after the upgrade, has to be called by assets/2017/02/f365a8dbcf/something.jpg.

What I noticed in https://github.com/silverstripe/silverstripe-assets/blob/1/src/Flysystem/FlysystemAssetStore.php#L1518

if ($parsedFileID = $publicStrategy->softResolveFileID($asset, $public))

only the public filesystem is searched for the filehash, I was able to mitigate the problem by expanding the condition to

if (
    ($parsedFileID = $publicStrategy->softResolveFileID($asset, $public)) ||
    ($parsedFileID = $protectedStrategy->softResolveFileID($asset, $protected)))

Now the filehash is correctly resolved for both public and private assets.

Do you think this would be a valid way to fix this issue?