Merge pull request #1417 from ZogStriP/content-disposition

proper content-disposition header when downloading attachments

app/assets/javascripts/discourse/views/composer_view.js

@@ -252,8 +252,7 @@ Discourse.ComposerView = Discourse.View.extend({
 
     $uploadTarget.fileupload({
         url: Discourse.getURL('/uploads'),
-        dataType: 'json',
-        timeout: 20000
+        dataType: 'json'
     });
 
     // submit - this event is triggered for each upload

app/controllers/topics_controller.rb

@@ -24,7 +24,6 @@ class TopicsController < ApplicationController
   skip_before_filter :check_xhr, only: [:show, :feed]
 
   def show
-
     # We'd like to migrate the wordpress feed to another url. This keeps up backwards compatibility with
     # existing installs.
     return wordpress if params[:best].present?
@@ -33,7 +32,6 @@ class TopicsController < ApplicationController
     begin
       @topic_view = TopicView.new(params[:id] || params[:topic_id], current_user, opts)
     rescue Discourse::NotFound
-      Rails.logger.info ">>>> B"
       topic = Topic.where(slug: params[:id]).first if params[:id]
       raise Discourse::NotFound unless topic
       return redirect_to(topic.relative_url)

app/controllers/uploads_controller.rb

@@ -1,5 +1,6 @@
 class UploadsController < ApplicationController
-  before_filter :ensure_logged_in
+  before_filter :ensure_logged_in, except: [:show]
+  skip_before_filter :check_xhr, only: [:show]
 
   def create
     file = params[:file] || params[:files].first
@@ -28,4 +29,18 @@ class UploadsController < ApplicationController
     render status: 422, text: I18n.t("upload.images.size_not_found")
   end
 
+  def show
+    return render nothing: true, status: 404 unless Discourse.store.internal?
+
+    id = params[:id].to_i
+    url = request.fullpath
+
+    # the "url" parameter is here to prevent people from scanning the uploads using the id
+    upload = Upload.where(id: id, url: url).first
+
+    return render nothing: true, status: 404 unless upload
+
+    send_file(Discourse.store.path_for(upload), filename: upload.original_filename)
+  end
+
 end

config/nginx.sample.conf

@@ -22,50 +22,60 @@ server {
   sendfile on;
 
   keepalive_timeout 65;
+
+  # maximum file upload size (keep up to date when changing the corresponding site setting)
   client_max_body_size 2m;
 
+  # path to discourse's public directory
+  set $public /var/www/discourse/public;
+
   location / {
-    root /var/www/discourse/public;
+    root $public;
 
-    ## optional upload anti-hotlinking rules
-    #location ~ ^/uploads/ {
-    #  valid_referers none blocked mysite.com *.mysite.com;
-    #  if ($invalid_referer) {
-    #    return 403;
-    #  }
-    #}
-
-    ## [LEGACY] this is deprecated, leaving it there for a small transition period
-    location ~ ^/t\/[0-9]+\/[0-9]+\/avatar {
-      expires 1d;
-      add_header Cache-Control public;
-      add_header ETag "";
-    }
-
-    location ~ ^/(assets|uploads)/ {
+    location ~ ^/assets/ {
       expires 1y;
       add_header Cache-Control public;
       add_header ETag "";
       break;
     }
 
-    proxy_set_header  X-Real-IP  $remote_addr;
-    proxy_set_header  X-Forwarded-For $proxy_add_x_forwarded_for;
-    proxy_set_header  X-Forwarded-Proto $scheme;
-    proxy_set_header  Host $http_host;
+    location ~ ^/uploads/ {
+      expires 1y;
+      add_header Cache-Control public;
+      add_header ETag "";
 
+      ## optional upload anti-hotlinking rules
+      #valid_referers none blocked mysite.com *.mysite.com;
+      #if ($invalid_referer) {
+      #  return 403;
+      #}
 
-    # If the file exists as a static file serve it directly without
-    # running all the other rewrite tests on it
-    if (-f $request_filename) {
-      break;
-    }
+      # let NGINX serve images
+      location ~* \.(gif|png|jpg|jpeg|bmp|tif|tiff)$ { try_files $uri =404; }
+      location ~ /_optimized/ { try_files $uri =404; }
 
-    if (!-f $request_filename) {
+      # attachments must go through the rails application to get the right content-disposition header
+      proxy_set_header X-Sendfile-Type X-Accel-Redirect;
+      proxy_set_header X-Accel-Mapping $public/=/downloads/;
       proxy_pass http://discourse;
       break;
     }
 
+    try_files $uri @discourse;
+  }
+
+  location /downloads/ {
+    internal;
+    alias $public/;
+  }
+
+  location @discourse {
+    proxy_set_header X-Real-IP $remote_addr;
+    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
+    proxy_set_header X-Forwarded-Proto $scheme;
+    proxy_set_header Host $http_host;
+
+    proxy_pass http://discourse;
   }
 
 }

config/routes.rb

@@ -146,7 +146,8 @@ Discourse::Application.routes.draw do
   get 'users/:username/activity' => 'users#show', constraints: {username: USERNAME_ROUTE_FORMAT}
   get 'users/:username/activity/:filter' => 'users#show', constraints: {username: USERNAME_ROUTE_FORMAT}
 
-  resources :uploads
+  get 'uploads/:site/:id/:sha.:extension' => 'uploads#show', constraints: {site: /\w+/, id: /\d+/, sha: /[a-z0-9]{15,16}/i, extension: /\w{2,}/}
+  post 'uploads' => 'uploads#create'
 
   get 'posts/by_number/:topic_id/:post_number' => 'posts#by_number'
   get 'posts/:id/reply-history' => 'posts#reply_history'

db/migrate/20130906171631_add_index_to_uploads.rb

@@ -0,0 +1,5 @@
+class AddIndexToUploads < ActiveRecord::Migration
+  def change
+    add_index :uploads, [:id, :url]
+  end
+end

spec/controllers/uploads_controller_spec.rb

@@ -2,17 +2,15 @@ require 'spec_helper'
 
 describe UploadsController do
 
-  it 'requires you to be logged in' do
-    -> { xhr :post, :create }.should raise_error(Discourse::NotLoggedIn)
-  end
+  context '.create' do
 
-  context 'logged in' do
-
-    before do
-      @user = log_in :user
+    it 'requires you to be logged in' do
+      -> { xhr :post, :create }.should raise_error(Discourse::NotLoggedIn)
     end
 
-    context '.create' do
+    context 'logged in' do
+
+      before { @user = log_in :user }
 
       let(:logo) do
         ActionDispatch::Http::UploadedFile.new({
@@ -97,4 +95,29 @@ describe UploadsController do
 
   end
 
+  context '.show' do
+
+    it "returns 404 when using external storage" do
+      store = stub(internal?: false)
+      Discourse.stubs(:store).returns(store)
+      Upload.expects(:where).never
+      get :show, site: "default", id: 1, sha: "1234567890abcdef", extension: "pdf"
+      response.response_code.should == 404
+    end
+
+    it "returns 404 when the upload doens't exist" do
+      Upload.expects(:where).with(id: 2, url: "/uploads/default/2/1234567890abcdef.pdf").returns [nil]
+      get :show, site: "default", id: 2, sha: "1234567890abcdef", extension: "pdf"
+      response.response_code.should == 404
+    end
+
+    it 'uses send_file' do
+      Fabricate(:attachment)
+      controller.stubs(:render)
+      controller.expects(:send_file)
+      get :show, site: "default", id: 42, sha: "66b3ed1503efc936", extension: "zip"
+    end
+
+  end
+
 end

spec/fabricators/upload_fabricator.rb

@@ -9,8 +9,9 @@ Fabricator(:upload) do
 end
 
 Fabricator(:attachment, from: :upload) do
+  id 42
   user
   original_filename "archive.zip"
   filesize 1234
-  url  "/uploads/default/186/66b3ed1503efc936.zip"
+  url  "/uploads/default/42/66b3ed1503efc936.zip"
 end