FIX: Allow themes to upload and serve js files (#8188)

FIX: Allow themes to upload and serve js files (#8188)

If you set config.public_file_server.enabled = false when you try to get uploaded js file you will get an error: Security warning: an embedded <script> tag on another site requested protected JavaScript. If you know what you're doing, go ahead and disable forgery protection on this action to permit cross-origin JavaScript embedding.

The reason is that content type is application/javascript and in Rails 5 guard looked like that: rails/request_forgery_protection.rb at 5-2-stable · rails/rails · GitHub However, in Rails 6 application was added to regex: rails/request_forgery_protection.rb at master · rails/rails · GitHub

This pull request is related to Uploaded .js file for theme causes a rejection? - support - Discourse Meta

diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index 78c1623704..78fd13c8eb 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -6,6 +6,7 @@ class UploadsController < ApplicationController
   requires_login except: [:show, :show_short]
 
   skip_before_action :preload_json, :check_xhr, :redirect_to_login_if_required, only: [:show, :show_short]
+  protect_from_forgery except: :show
 
   def create
     # capture current user for block later on
diff --git a/spec/fixtures/themes/test.js b/spec/fixtures/themes/test.js
new file mode 100644
index 0000000000..14c198cadb
--- /dev/null
+++ b/spec/fixtures/themes/test.js
@@ -0,0 +1 @@
+console.log("test");
diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb
index d779898e5f..cb738c73ea 100644
--- a/spec/requests/uploads_controller_spec.rb
+++ b/spec/requests/uploads_controller_spec.rb
@@ -265,6 +265,13 @@ describe UploadsController do
         .to eq(%Q|attachment; filename="logo.png"; filename*=UTF-8''logo.png|)
     end
 
+    it 'returns 200 when js file' do
+      ActionDispatch::FileHandler.any_instance.stubs(:match?).returns(false)
+      upload = upload_file("test.js", "themes")
+      get upload.url
+      expect(response.status).to eq(200)
+    end
+
     it "handles image without extension" do
       SiteSetting.authorized_extensions = "*"
       upload = upload_file("image_no_extension")

GitHub sha: 99086edf

rogue commit ;p

Can we do this thing without that stub?

1 Like

Writing a test for that stuff was harder than I expected. Let me walk you through what is happening:

In environments/test.rb we got config.public_file_server.enabled = true

When that is set to true, ActionDispatch::Static middleware is added:


if config.public_file_server.enabled
  headers = config.public_file_server.headers || {}
  middleware.use ::ActionDispatch::Static, paths["public"].first, index: 
  config.public_file_server.index_name, headers: headers
end

This middleware https://api.rubyonrails.org/classes/ActionDispatch/Static.html#method-i-call is a shortcut to serve the file directly from disk and it skips protect from forgery, so the test is passing with and without fix.

So to test the fix, I thought about 2 options:

  1. Change the flag in environments/test.rb to be false. The benefit is that false is the default for production so it makes some sense. I checked that on my local machine tests are equally performant, however, I wasn’t sure if I want to change that flag for the whole test suite.

  2. I decided to stub the method used in ActionDispatch::Static middleware, to skip shortcut and move forward through rake stack.

What is your recommendation here?

1 Like

I kind of like this option, having better parity with production makes me happy. If perf is identical for the test suite go ahead and change that.

2 Likes

Sorry, I needed to revert that change Revert "FIX: public_file_server.enabled is false in test" by lis2 · Pull Request #8196 · discourse/discourse · GitHub

My bad that I checked only rspec, didn’t run qunit and beside failing, that change was making it terrible slow

2 Likes