Multisite + short-url + secure_uploads + s3

Bug
1

Ok, I seem to be on a bug-hunting spree this week.

We have a forum with secure_uploads enabled, all uploads are on (the official) AWS S3.

Problem: short-url links do not seem to function correctly, the uploads/{database_id} is missing from the URL.

It looks like the offending code is in UploadsController::show_short

if upload = Upload.find_by(sha1: sha1)
  return handle_secure_upload_request(upload, Discourse.store.get_path_for_upload(upload)) 
     if upload.secure? && SiteSetting.secure_media?

  if Discourse.store.internal?
    send_file_local_upload(upload)
  else
    redirect_to Discourse.store.url_for(upload, force_download: params[:dl] == "1")
  end
else
  render_404
end

So if upload.secure? && SiteSetting.secure_media? is true then the request is processed by
handle_secure_upload_request(upload, Discourse.store.get_path_for_upload(upload))

Now Discourse.store.get_path_for_upload(upload) returns an URL without the uploads/{database_id} part:

original/3X/f/d/fd0b5775899541b9d42e67f8e0dd6bf587a179d3.png"

and consequently handle_secure_upload_request returns a signed URL but with a missing part in the URL since it starts with /original

https://redacted.s3.us-east-2.amazonaws.com/original/3X/f/d/fd0b5775899541b9d42e67f8e0dd6bf587a179d3.png?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKI....

If upload.secure? && SiteSetting.secure_media? would be false (which it is not) then the request would be processed by Discourse.store.url_for(upload, force_download: params[:dl] == "1")

and that actually returns a correct URL:

https://redacted.s3.us-east-2.amazonaws.com/uploads/db3999/original/3X/f/d/fd0b5775899541b9d42e67f8e0dd6bf587a179d3.png?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKI...

So apparently there is a url_for and a get_path_for_upload which behave differently, and the wrong one seems to be used?

7 Likes
3

Thank you for the report, this should be quite easy to fix; handle_secure_upload_request is just not taking into account multisite connections. I will work on a fix for this today and report back when done.

8 Likes
4

Fixed here:

https://github.com/discourse/discourse/pull/9212

9 Likes