FIX: Copying image markdown for secure media loading full image (PR #9488)

  • When copying the markdown for an image between posts, we were not adding the srcset and data-small-image attributes which are done by calling optimize_image! in cooked post processor
  • Refactored the code which was confusing in its current state (the consider_for_reuse method was super confusing) and fixed the issue

GitHub

So… this code will copy secure images from one post to another, simply using the URL?

Could someone use this to steal images which they’re not supposed to have access to?

@davidtaylorhq I will check this out, this is a good point. Definitely want to redownload if uploaded_before_secure_media_enabled? but yeah maybe not in the other case. Thanks for pointing this out!

Do we really want to pull this in if we are just going to be removing this anyway? Are we going to keep any of this code after we add the stricter rules?

@SamSaffron still worth pulling in, I think its good to have the server-side checks there. At any rate it may be a little while until I get stricter rules merged so would be good to have it fixed until then. Oh also it has a legit fix for pull_hotlinked_images when the hotlinked image is secure so that needs to be merged anyway.

I don’t think this is strong enough. What if an admin creates a wiki post, and then a TL1 edits it? Now the TL1 can steal any uploads which the admin can access.

I don’t think we can solve this cleanly. I think we should never try and “pull hotlinked” on secure uploads.

Why is this function needed? Don’t we backfill the uploads table when secure media is enabled?

This is a nice change. I agree there is no need to prevent thumbnail generation for copied secure uploads :+1:

@davidtaylorhq you don’t have to run the rake task that goes and marks old uploads as secure, and it is not automatic. Depending on the forum it may not even be wise if there are a lot of uploads reused all over the place, because the first time it is used in a post that post will be used as the access control post, which may break subsequent uses of the upload.

You can just turn on secure uploads + have it apply from that point in time. From an issue we had internally this check on pull hotlinked is used so if someone copies an old non-secure image into a secure context it is always created as a new upload so it does not get an access_control_post attached to the old upload.