Exempt files#show_relative from protect_from_forgery

[grahamb]

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.

The security implications of exempting show_relative needs to be discussed. It would be nice if this could be scoped to account-context files only.

Test plan:

• Create a new brand config
• Upload a JavaScript file
• Save and apply the brand config
• Result: the JS file should load

A signed CLA is on file under my employer, Simon Fraser University.

[grahamb]

Any feedback?

[simong]

FWIW, if this fixes downloading files (stored on the local backend) this would be greatly helpful to our team too. If someone can provide an alternative work-around through configuration that would be great too for now.

[omarkhan]

Thanks for the contribution @grahamb, sorry it's taken us so long to look at this. I have asked someone on the canvas team to review it. I know that it won't be merged without tests at the very least, but it's probably worth looking into the security implications first.

[grahamb]

@omarkhan fair enough! I opened this to start that discussion; we've been running with this patch since November but it's a very blunt instrument. If there's a better way to handle this I'm all for it.

My branch has diverged quite a bit from master since November, so I just rebased and pushed up again.

[omarkhan]

Hi @grahamb - unfortunately I couldn't get any buy-in from product to work on this. It's definitely a legit bug but since we don't use local storage in this way it's not something that affects us directly. If you have time to come up with a fix that is more targeted I'd be happy to revisit this, but until then I'm going to close the PR. Thanks again for the contribution though, and feel free to get in touch if you have any questions!

[grahamb]

Hi @omarkhan - that's disappointing to hear that you couldn't get any buy-in. As you note, it's a real bug. Using local storage is fairly common in the open-source Canvas world, and it's one of the largest sources of bugs due to it not being used by Instructure.

[omarkhan]

That's a fair point @grahamb. I'll leave this pull request open to track the issue.

[grahamb]

Thanks for keeping this on the radar, @omarkhan. We'll try here to see if we can find a good solution; any input from your side is always appreciated ;)

[christopher-b]

We would also like to see a real fix for this issue. If local storage is a supported feature, bugs like this (and #1064) should be resolved. Thanks all.

[defektive]

@grahamb What are your thoughts on adding a call to verify_authenticity_token if @context isn't an account in show_relative?

[grahamb]

@defektive I'll take a look at doing that, as soon as I can figure out why my docker env is throwing a ActionController::InvalidCrossOriginRequest for uploaded JS even with this mod in place.

[grahamb]

@defektive just want to confirm: are you suggesting that we:

• keep protect_from_forgery except: :show_relative
• add :show_relative to the method list for skip_before_action :verify_authenticity_token
• call verify_authenticity_token unless @context.is_a?(Account) in show_relative

That does seem to work (in that the JS file is allowed to be accessed). I'm not sure the second step is required if show_relative continues to be exempted from protect_from_forgery though. I'll have to come up with a test to make sure it isn't allowing anything it shouldn't. One question is, are theme uploads the only place where the context for a file would be Account? We use account-level groups here for ad-hoc collaboration spaces; I'll have to do some testing there to see what context a file uploaded into one of those groups gets.

[defektive]

@grahamb I am not sure about the 2nd step also. Ideally we could make this an opt-in setting, or specifically tag branding files to be allowed. The way it currently is (if your testing goes well), puts us in a nice place where open source isn't borked, and not all files are bypassing CSRF protections.

[grahamb]

Some quick testing shows that the @context for a file in an account-level group is Group. That means that forgery protection is still working for files uploaded into one of those groups while still allowing a brand JS file to be downloaded. Interestingly enough, the default protections prohibit a JS file from being downloaded at all from a course or group – I'm surprised that isn't an issue for CS classes.

[defektive]

@grahamb can you add a test to make sure we are/are not verifying authenticity for the appropriate contexts?

[grahamb]

I've been working on tests but I'm having trouble with it – I can create an attachment on the Account, but the test user can't download it; it throws a 401 Unauthorized. I posted a couple of times in IRC last week about it but didn't get a response:

Testing question: I'm trying to write some tests for #945 and I'm running into a permissions problem. I'm trying to test whether a user can download an Account-level JS file (e.g. one uploaded to a brand config). I've created the account, file, and user session, and uploaded the file in the proper before blocks, but when I try to do a get 'show_relative' with the params of the test file, it throws a 401 Unauthorized.

[grahamb]

@defektive ok -- got a couple of tests wired up.

[defektive]

Awesome! Thanks for getting that added!

[spencerolson]

Hi @grahamb, thanks for your contribution! Your code is being reviewed by our developers. Once the review is complete, I'll A) post code review comments here if changes are requested, or B) merge the code if it is deemed ready-to-merge.

[grahamb]

@spencerolson let me know if you'd like me to squash the commits.

[spencerolson]

@grahamb I squashed them into one commit on our side before passing onto the devs for review, thanks though!

[spencerolson]

@grahamb D'oh, it looks like 8dfd1f9 just landed and caused a merge conflict here. Would you mind pushing up a new commit with conflicts resolved? Also, if you could squash the commits, that would be great :D

Still waiting on a full code review from our developers on this one, but I did notice there were some trailing whitespaces in the commit; not a big deal but if you could remove those trailing whitespaces with the new commit, that would be excellent. Let me know if you have any questions.

Spencer

[grahamb]

@spencerolson done! You might want to have @lukfugl verify that his change still works as well with this in place as we're now both monkeying with the protect_from_forgery directive.

There are some files_controller tests failing now, but they're also failing on master without these changes applied. The tests I added earlier are still passing.

[spencerolson]

@grahamb awesome, I'm running your newest commit through the build right now.

Hopefully those failing files_controller tests were just being flaky (we shouldn't have any failing tests on master). I'll let you know if there are any tests failures after this runs through the build, and if so we can work through them & figure out what's going on.

Thanks for the heads up; I'll add @lukfugl as a reviewer. He took a gander at it in an earlier form (I saw your commit was going to conflict with his and gave him a heads up) and I think the two commits will jive together just fine.

I ran script/rlint on your commit and noticed a few relevant INFO comments:

[info] spec/controllers/files_controller_spec.rb:57 => [rubocop] Metrics/LineLength: Line is too long. [143/120]
[info] spec/controllers/files_controller_spec.rb:660 => [rubocop] RSpec/ContextWording: Start context description with 'when', 'with', or 'without'.
[info] spec/controllers/files_controller_spec.rb:675 => [rubocop] Metrics/LineLength: Line is too long. [238/120]
[info] spec/controllers/files_controller_spec.rb:683 => [rubocop] Metrics/LineLength: Line is too long. [238/120]

Doesn't seem like any of those are show-stoppers, but I'll leave that decision to the devs who review this one. Also seeing some trailing whitespaces here:

app/controllers/files_controller.rb:111
spec/controllers/files_controller_spec.rb:659
spec/controllers/files_controller_spec.rb:678

Thanks again for your contribution and I'll let you know if I see any test failures when the build finishes.

Spencer

[spencerolson]

Wohoo! All tests passed. those file_controller spec failures must have just been flaky specs. I'll see if I can get this in front of our devs for review tomorrow. If you do decide to push up a new commit re: the linter and whitespace comments above, feel free to @spencerolson me and I'll run the new commit through tests again on our side.

Thanks!

[grahamb]

I'm only seeing extra whitespace on app/controllers/files_controller.rb:111; the others don't have trailing whitespace (at least on my end).

As for the rlint issues, I can fix them, but in my experience with the Canvas codebase (and the rest of files_controller_spec.rb seem to bear this out), those linter issues seem to be all over the place in the specs.

[spencerolson]

Yeah @grahamb I wouldn't worry about it. I'll let you know when I hear back from reviewers on this one. Thanks again for your contribution!

[spencerolson]

Hi @grahamb your commit has been merged to the master branch and can be found here c18b389. We appreciate your contribution!

Thanks,
Spencer