Exempt files#show_relative from protect_from_forgery

Rails 4.1 introduces CSRF protection from remote <script> tags. This
prevents locally-stored JavaScript files uploaded as part of a brand
config from loading.

This change enforces CSRF protection for non-Account-context files, but
allows Account-context files to be downloaded.

Test plan:

1. Create a new brand config.
2. Add a JS file to the brand config.
3. Save and apply the brand config.
4. The JS file should load and execute on page-load, and should not be
blocked by CSRF.

closes CORE-844
closes GH-945

Change-Id: Id59c139a379b286af610947824fedad63b6b7113
Reviewed-on: https://gerrit.instructure.com/137416
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
Tested-by: Jenkins
QA-Review: Tucker McKnight <tmcknight@instructure.com>
Product-Review: Brent Burgoyne <bburgoyne@instructure.com>

app/controllers/files_controller.rb

@@ -108,7 +108,11 @@
 #     }
 #
 class FilesController < ApplicationController
-  protect_from_forgery :except => [:api_capture], with: :exception
+  # show_relative is exempted from protect_from_forgery in order to allow 
+  # brand-config-uploaded JS to work
+  # verify_authenticity_token is manually-invoked where @context is not
+  # an Account in show_relative
+  protect_from_forgery :except => [:api_capture, :show_relative], with: :exception
 
   before_action :require_user, only: :create_pending
   before_action :require_context, except: [
@@ -569,6 +573,10 @@ def show_relative
     file_id = params[:file_id]
     file_id = nil unless file_id.to_s =~ Api::ID_REGEX
 
+    # Manually-invoke verify_authenticity_token for non-Account contexts
+    # This is to allow Account-level file downloads to skip request forgery protection
+    verify_authenticity_token unless @context.is_a?(Account)
+
     #if the relative path matches the given file id use that file
     if file_id && @attachment = @context.attachments.where(id: file_id).first
       unless @attachment.matches_full_display_path?(path) || @attachment.matches_full_path?(path)

spec/controllers/files_controller_spec.rb

@@ -53,6 +53,10 @@ def user_file
     @file = factory_with_protected_attributes(@user.attachments, :uploaded_data => io)
   end
 
+  def account_js_file
+    @file = factory_with_protected_attributes(@account.attachments, :uploaded_data => fixture_file_upload('test.js', 'text/javascript', false))
+  end
+
   def folder_file
     @file = @folder.active_file_attachments.build(:uploaded_data => io)
     @file.context = @course
@@ -652,6 +656,35 @@ def test_path(path)
         assert_unauthorized
       end
     end
+
+    context "account-context files" do
+      before :once do
+        @account = account_model
+      end
+
+      before :each do
+        allow(HostUrl).to receive(:file_host).and_return('files.test')
+        request.host = 'files.test'
+        user_session(@teacher)
+      end
+
+      it "should skip verification for an account-context file" do
+        account_js_file
+        verifier = Attachments::Verification.new(@file).verifier_for_user(nil)
+        ts, sf_verifier = @teacher.access_verifier
+        get 'show_relative', params: { :download => 1, :inline => 1, :sf_verifier => sf_verifier, :ts => ts, :user_id => @teacher.id, :verifier => verifier, :account_id => @account.id, :file_id => @file.id, :file_path => @file.full_path }
+        expect(response).to be_success
+      end
+
+      it "should enforce verification for contexts other than account" do
+        course_file
+        verifier = Attachments::Verification.new(@file).verifier_for_user(nil)
+        ts, sf_verifier = @teacher.access_verifier
+        get 'show_relative', params: { :download => 1, :inline => 1, :sf_verifier => sf_verifier, :ts => ts, :user_id => @teacher.id, :verifier => verifier, :account_id => @account.id, :file_id => @file.id, :file_path => @file.full_path }
+        assert_unauthorized
+      end
+
+    end
   end
 
   describe "POST 'create'" do

spec/fixtures/test.js

@@ -0,0 +1 @@
+console.log('This is totally not a malicious JavaScript file.')