Skip to content

Commit

Permalink
FIX: Allow themes to upload and serve js files (#8188)
Browse files Browse the repository at this point in the history 
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:
https://github.com/rails/rails/blob/5-2-stable/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L278-L280
However, in Rails 6 `application` was added to regex:
https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L282-L284

This pull request is related to https://meta.discourse.org/t/uploaded-js-file-for-theme-causes-a-rejection/129753/8
  • Loading branch information
@lis2
lis2 committed Oct 14, 2019
1 parent e4fe864 commit 99086ed
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 0 deletions.
1 change: 1 addition & 0 deletions app/controllers/uploads_controller.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions spec/fixtures/themes/test.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log("test");
7 changes: 7 additions & 0 deletions spec/requests/uploads_controller_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,13 @@ def upload_file(file, folder = "images")
.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")
Expand Down

6 comments on commit 99086ed

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SamSaffron posted:

[quote="lis2, post:1, topic:6155"]
+console.log("test");
[/quote]

rogue commit ;p

[quote="lis2, post:1, topic:6155"]
ActionDispatch::FileHandler.any_instance.stubs(:match?).returns(false)
[/quote]

Can we do this thing without that stub?

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lis2 posted:

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?

ActionDispatch::Static

ActionDispatch::Static

ActionDispatch::Static

ActionDispatch::Static

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SamSaffron posted:

[quote="lis2, post:4, topic:6155"]
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.
[/quote]

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.

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lis2 posted:

Sorry, I needed to revert that change #8196

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

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lis2 posted:

Tags are a little bit more complicated than categories because the topic can have multiple tags. We need to make a decision about what to do in the case when we got two tags tag_a and tag_b.

Let's say I mute tag_a and the new post is arriving with both tags.

Should we mark that post as new and display in the latest section?

I think that yes and we should hide it only if both tags are muted, but that is making the solution more complex and I am not sure if that is worth.

An easier option would be to just skip any topic if any of the tags is muted

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SamSaffron posted:

[quote="lis2, post:7, topic:6155"]
An easier option would be to just skip any topic if any of the tags is muted
[/quote]

Feels like the wrong topic to me :)

I think if there is a message that there is a new topic, and you have the tag muted, no matter what the "1 new or updated topics" thing should not show up.

As to what happens in the list ... that is a bit more complex.

TopicTrackingState query though is a more complicated problem, we don't want to add cost there :(

Please sign in to comment.