From 45b838009c2cb66f206028b018a7736dca45cbb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Fri, 6 Sep 2013 19:18:42 +0200 Subject: [PATCH] proper content-disposition header when downloading attachments --- .../discourse/views/composer_view.js | 3 +- app/controllers/topics_controller.rb | 2 - app/controllers/uploads_controller.rb | 17 ++++- config/nginx.sample.conf | 64 +++++++++++-------- config/routes.rb | 3 +- .../20130906171631_add_index_to_uploads.rb | 5 ++ spec/controllers/uploads_controller_spec.rb | 39 ++++++++--- spec/fabricators/upload_fabricator.rb | 3 +- 8 files changed, 94 insertions(+), 42 deletions(-) create mode 100644 db/migrate/20130906171631_add_index_to_uploads.rb diff --git a/app/assets/javascripts/discourse/views/composer_view.js b/app/assets/javascripts/discourse/views/composer_view.js index 707c5df78..416254196 100644 --- a/app/assets/javascripts/discourse/views/composer_view.js +++ b/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 diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 2005e42f1..125b24ece 100644 --- a/app/controllers/topics_controller.rb +++ b/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) diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 11710d71a..d5a09c084 100644 --- a/app/controllers/uploads_controller.rb +++ b/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 diff --git a/config/nginx.sample.conf b/config/nginx.sample.conf index efb23d939..41771cc2e 100644 --- a/config/nginx.sample.conf +++ b/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; } } diff --git a/config/routes.rb b/config/routes.rb index 40283267c..441d65282 100644 --- a/config/routes.rb +++ b/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' diff --git a/db/migrate/20130906171631_add_index_to_uploads.rb b/db/migrate/20130906171631_add_index_to_uploads.rb new file mode 100644 index 000000000..38e91b75b --- /dev/null +++ b/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 diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 4f6f27151..d45961804 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/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 diff --git a/spec/fabricators/upload_fabricator.rb b/spec/fabricators/upload_fabricator.rb index 60bf415a1..c8788a972 100644 --- a/spec/fabricators/upload_fabricator.rb +++ b/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