ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

prepend protection loop to json responses in app ajax calls 

We are now prepending all json responses with "while(1);" to protect
against browsers that allow stealing this information from a <script>
tag on third-party sites, by overriding constructors or property
getters/setters.

this loop is not prepended to API requests, unless those requests are
authenticated via a session cookie (canvas itself makes API
requests using the user's session, but we don't want third-party apps to
have to remove the loop before parsing).

fixes #6459

Change-Id: Icf00056d4d7fba198a8957892af09cdd84d55bc4
testplan:
  * Do anything in the application that results in a AJAX request
    returning JSON -- for instance, load your list of conversations.
  * Use a web inspector to verify that the canvas is returning the JSON
    response with this prepended loop, but that the javascript code
    handles that and still can parse the response.
  * Make API calls to Canvas, verify that nothing is prepended to the
    JSON responses.
Reviewed-on: https://gerrit.instructure.com/7144
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Jon Jensen <jon@instructure.com>
This commit is contained in:
Brian Palmer 2011-11-23 13:49:27 -07:00
parent df55776d07
commit c1bfe404fe
22 changed files with 156 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
app/controllers/accounts_controller.rb
View File

@ -324,9 +324,9 @@ class AccountsController < ApplicationController
@account.current_sis_batch_id = batch.id
@account.save
batch.process
render :text => batch.to_json(:include => :sis_batch_log_entries)
render :json => batch.to_json(:include => :sis_batch_log_entries)
else
render :text => {:error=>true, :error_message=> t(:sis_import_in_process_notice, "An SIS import is already in process."), :batch_in_progress=>true}.to_json
render :json => {:error=>true, :error_message=> t(:sis_import_in_process_notice, "An SIS import is already in process."), :batch_in_progress=>true}.to_json
end
end
end

21
app/controllers/application_controller.rb
View File

@ -1049,4 +1049,25 @@ class ApplicationController < ActionController::Base
yield if block_given? && (@bank = bank)
bank
end
def render(options = nil, extra_options = {}, &block)
if options && options.key?(:json)
json = options.delete(:json)
json = ActiveSupport::JSON.encode(json) unless json.is_a?(String)
# prepend our CSRF protection to the JSON response, unless this is an API
# call that didn't use session auth.
if @pseudonym_session && !@pseudonym_session.used_basic_auth?
json = "while(1);#{json}"
end
# fix for IE not properly handling json responses to multipart file
# upload forms -- we'll respond with text instead.
if request.headers['CONTENT_TYPE'].to_s =~ %r{multipart/form-data} && params[:format].to_s != 'json'
options[:text] = json
else
options[:json] = json
end
end
super
end
end

10
app/controllers/content_imports_controller.rb
View File

@ -77,7 +77,7 @@ class ContentImportsController < ApplicationController
render :json => upload_params
else
@migration.export_content
render :text => @migration.to_json
render :json => @migration.to_json
end
else
render :json => @migration.errors, :status => :bad_request
@ -126,7 +126,7 @@ class ContentImportsController < ApplicationController
send_file_or_data(stream, :type => :json, :disposition => 'inline')
else
logger.error "There was no overview.json file for this content_migration."
render :text => {:success=>false}.to_json
render :json => {:success=>false}.to_json
end
end
end
@ -141,7 +141,7 @@ class ContentImportsController < ApplicationController
@content_migration.migration_settings[:migration_ids_to_import] = params
@content_migration.save
@content_migration.import_content
render :text => {:success => true}.to_json
render :json => {:success => true}.to_json
else
render :json => @content_migration.to_json
end
@ -282,9 +282,9 @@ class ContentImportsController < ApplicationController
@attachment = @migration.attachment
if block_given?
if @attachment && yield
render_for_text @attachment.to_json(:allow => :uuid, :methods => [:uuid,:readable_size,:mime_class,:currently_locked,:scribdable?])
render :json => @attachment.to_json(:allow => :uuid, :methods => [:uuid,:readable_size,:mime_class,:currently_locked,:scribdable?])
else
render_for_text "", :status => :bad_request
render :text => "", :status => :bad_request
end
end
end

7
app/controllers/conversations_controller.rb
View File

@ -786,11 +786,4 @@ class ConversationsController < ApplicationController
hash
}
end
def render(options = {}, extra_options = {}, &block)
if options.keys == [:json] && request.headers['CONTENT_TYPE'].to_s =~ %r{multipart/form-data} && params[:format].to_s != 'json'
options[:text] = options.delete(:json).to_json
end
super
end
end

10
app/controllers/files_controller.rb
View File

@ -444,12 +444,12 @@ class FilesController < ApplicationController
if @attachment && details
deleted_attachments = @attachment.handle_duplicates(params[:duplicate_handling])
@attachment.process_s3_details!(details)
render_for_text({
render :json => {
:attachment => @attachment,
:deleted_attachment_ids => deleted_attachments.map(&:id)
}.to_json(:allow => :uuid, :methods => [:uuid,:readable_size,:mime_class,:currently_locked,:scribdable?], :permissions => {:user => @current_user, :session => session}, :include_root => false))
}.to_json(:allow => :uuid, :methods => [:uuid,:readable_size,:mime_class,:currently_locked,:scribdable?], :permissions => {:user => @current_user, :session => session}, :include_root => false)
else
render_for_text ""
render :text => ""
end
end
@ -511,8 +511,8 @@ class FilesController < ApplicationController
if success
@attachment.move_to_bottom
format.html { return_to(params[:return_to], named_context_url(@context, :context_files_url)) }
format.json { render_for_text({ :attachment => @attachment, :deleted_attachment_ids => deleted_attachments.map(&:id) }.to_json(:allow => :uuid, :methods => [:uuid,:readable_size,:mime_class,:currently_locked,:scribdable?,:thumbnail_url], :permissions => {:user => @current_user, :session => session}, :include_root => false))}
format.text { render_for_text({ :attachment => @attachment, :deleted_attachment_ids => deleted_attachments.map(&:id) }.to_json(:allow => :uuid, :methods => [:uuid,:readable_size,:mime_class,:currently_locked,:scribdable?,:thumbnail_url], :permissions => {:user => @current_user, :session => session}, :include_root => false))}
format.json { render :json => { :attachment => @attachment, :deleted_attachment_ids => deleted_attachments.map(&:id) }.to_json(:allow => :uuid, :methods => [:uuid,:readable_size,:mime_class,:currently_locked,:scribdable?,:thumbnail_url], :permissions => {:user => @current_user, :session => session}, :include_root => false)}
format.text { render :json => { :attachment => @attachment, :deleted_attachment_ids => deleted_attachments.map(&:id) }.to_json(:allow => :uuid, :methods => [:uuid,:readable_size,:mime_class,:currently_locked,:scribdable?,:thumbnail_url], :permissions => {:user => @current_user, :session => session}, :include_root => false)}
else
format.html { render :action => "new" }
format.json { render :json => @attachment.errors.to_json }

4
app/controllers/gradebooks_controller.rb
View File

@ -282,14 +282,14 @@ class GradebooksController < ApplicationController
render :json => @submissions.to_json(Submission.json_serialization_full_parameters), :status => :created, :location => course_gradebook_url(@assignment.context)
}
format.text {
render_for_text @submissions.to_json(Submission.json_serialization_full_parameters), :status => :created, :location => course_gradebook_url(@assignment.context)
render :json => @submissions.to_json(Submission.json_serialization_full_parameters), :status => :created, :location => course_gradebook_url(@assignment.context)
}
else
flash[:error] = t('errors.submission_failed', "Submission was unsuccessful: %{error}", :error => @error_message || t('errors.submission_failed_default', 'Submission Failed'))
format.html { render :action => "show", :course_id => @assignment.context.id }
format.xml { render :xml => {:errors => {:base => @error_message}}.to_xml }
format.json { render :json => {:errors => {:base => @error_message}}.to_json, :status => :bad_request }
format.text { render_for_text({:errors => {:base => @error_message}}.to_json) }
format.text { render :json => {:errors => {:base => @error_message}}.to_json, :status => :bad_request }
end
end
end

4
app/controllers/outcome_groups_controller.rb
View File

@ -47,9 +47,9 @@ class OutcomeGroupsController < ApplicationController
outcome_hash = outcome_hash.with_indifferent_access
outcome = group.learning_outcomes.create(params)
end
render :text => group.to_json(:include => :learning_outcomes)
render :json => group.to_json(:include => :learning_outcomes)
else
render :text => {:errors => {:base => t(:invalid_file, "Invalid outcome group file")}}, :status => :bad_request
render :json => {:errors => {:base => t(:invalid_file, "Invalid outcome group file")}}, :status => :bad_request
end
end
end

2
app/controllers/submissions_controller.rb
View File

@ -272,7 +272,7 @@ class SubmissionsController < ApplicationController
format.html { render :action => "show", :id => @assignment.context.id }
format.xml { render :xml => {:errors => {:base => @error_message}}.to_xml }
format.json { render :json => {:errors => {:base => @error_message}}.to_json, :status => :bad_request }
format.text { render_for_text({:errors => {:base => @error_message}}.to_json) }
format.text { render :json => {:errors => {:base => @error_message}}.to_json, :status => :bad_request }
end
end
end

26
public/javascripts/jquery.instructure_jquery_patches.js
View File

@ -19,7 +19,9 @@
(function($){
// have UI dialogs default to modal:true
$.widget('instructure.dialog', $.ui.dialog, { options: {modal: true} });
if ($.ui) {
$.widget('instructure.dialog', $.ui.dialog, { options: {modal: true} });
}
// This is so that if you disable an element, that it also gives it the class disabled.
// that way you can add css classes for our friend IE6. so rather than using selector:disabled,
@ -44,6 +46,24 @@
});
});
// monkey patch jquery's JSON parsing so we can have all of our ajax responses return with
// 'while(1);' prepended to them to protect against a CSRF attack vector.
var _parseJSON = $.parseJSON;
$.parseJSON = function() {
"use strict";
if (arguments[0]) {
try {
var newData = arguments[0].replace(/^while\(1\);/, '');
arguments[0] = newData;
} catch (err) {
// data was not a string or something, just pass along to the real parseJSON
// and let it handle errors.
}
}
return _parseJSON.apply($, arguments);
};
$.ajaxSettings.converters["text json"] = $.parseJSON;
// this is a patch so you can set the "method" atribute on rails' REST-ful forms.
$.attrHooks.method = $.extend($.attrHooks.method, {
set: function( elem, value ) {
@ -71,7 +91,7 @@
$.windowScrollTop = function() {
return ($.browser.safari ? $("body") : $("html")).scrollTop();
};
define('jquery.instructure_jquery_patches', [], function() { return $ });
})(jQuery);
})(jQuery);

7
spec/apis/oauth_spec.rb
View File

@ -57,9 +57,14 @@ describe "OAuth2", :type => :integration do
# don't need developer key when we have an actual application session
post '/login', 'pseudonym_session[unique_id]' => 'test1@example.com', 'pseudonym_session[password]' => 'test123'
response.should redirect_to("http://www.example.com/?login_success=1")
get "/api/v1/courses.json", {}
response.should be_success
JSON.parse(response.body).size.should == 1
# because this is a normal application session, the response is prepended
# with our anti-csrf measure
json = response.body
json.should match(%r{^while\(1\);})
JSON.parse(json.sub(%r{^while\(1\);}, '')).size.should == 1
reset!
post "/api/v1/courses/#{@course.id}/assignments.json", { :assignment => { :name => 'test assignment', :points_possible => '5.3', :grading_type => 'points' } }

35
spec/apis/v1/general_auth_spec.rb
View File

@ -117,4 +117,39 @@ describe CoursesController, :type => :integration do
'calendar' => { 'ics' => "http://www.example.com/feeds/calendars/user_#{@student.uuid}.ics" },
}
end
it "should not prepend the CSRF protection to API requests" do
user_with_pseudonym(:user => @user)
raw_api_call(:get, "/api/v1/users/self/profile",
:controller => "profile", :action => "show", :user_id => "self", :format => "json")
response.should be_success
raw_json = response.body
raw_json.should_not match(%r{^while\(1\);})
json = JSON.parse(raw_json)
json['id'].should == @user.id
end
it "should not prepend the CSRF protection to HTTP Basic API requests" do
user_with_pseudonym(:active_user => true, :username => 'test1@example.com', :password => 'test123')
get "/api/v1/users/self/profile", {}, { :authorization => ActionController::HttpAuthentication::Basic.encode_credentials('test1@example.com', 'test123') }
response.should be_success
raw_json = response.body
raw_json.should_not match(%r{^while\(1\);})
json = JSON.parse(raw_json)
json['id'].should == @user.id
end
it "should prepend the CSRF protection for API endpoints, when session auth is used" do
user_with_pseudonym(:active_user => true, :username => 'test1@example.com', :password => 'test123')
post "/login", "pseudonym_session[unique_id]" => "test1@example.com",
"pseudonym_session[password]" => "test123"
assert_response 302
get "/api/v1/users/self/profile"
response.should be_success
raw_json = response.body
raw_json.should match(%r{^while\(1\);})
expect { JSON.parse(raw_json) }.to raise_error
json = JSON.parse(raw_json.sub(%r{^while\(1\);}, ''))
json['id'].should == @user.id
end
end

8
spec/coffeescripts/jQuery.instructureJqueryPatches.coffee Normal file
View File

@ -0,0 +1,8 @@
define [
'jquery.instructure_jquery_patches'
], (jQuery) ->
module 'instructure jquery patches'
test 'parseJSON', ->
deepEqual(jQuery.parseJSON('{ "var1": "1", "var2" : 2 }'), { "var1": "1", "var2" : 2 }, 'should still parse without the prefix')
deepEqual(jQuery.parseJSON('while(1);{ "var1": "1", "var2" : 2 }'), { "var1": "1", "var2" : 2 }, 'should parse with the prefix')

10
spec/controllers/files_controller_spec.rb
View File

@ -138,7 +138,7 @@ describe FilesController do
a1 = folder_file
get 'index', :course_id => @course.id, :format => 'json'
response.should be_success
data = JSON.parse(response.body) rescue nil
data = json_parse
data.should_not be_nil
data['contexts'].length.should eql(1)
data['contexts'][0]['course']['id'].should eql(@course.id)
@ -240,7 +240,7 @@ describe FilesController do
it "should mark files as viewed for module progressions if the file is previewed inline" do
file_in_a_module
get 'show', :course_id => @course.id, :id => @file.id, :inline => 1
response.body.should eql({:ok => true}.to_json)
json_parse.should == {'ok' => true}
@module.reload
@module.evaluate_for(@user, true, true).state.should eql(:completed)
end
@ -420,7 +420,7 @@ describe FilesController do
response.should be_success
assigns[:attachment].should_not be_nil
assigns[:attachment].id.should_not be_nil
json = JSON.parse(response.body) rescue nil
json = json_parse
json.should_not be_nil
json['id'].should eql(assigns[:attachment].id)
json['upload_url'].should_not be_nil
@ -447,7 +447,7 @@ describe FilesController do
response.should be_success
assigns[:attachment].should_not be_nil
assigns[:attachment].id.should_not be_nil
json = JSON.parse(response.body) rescue nil
json = json_parse
json.should_not be_nil
json['id'].should eql(assigns[:attachment].id)
json['upload_url'].should_not be_nil
@ -498,7 +498,7 @@ describe FilesController do
response.should be_success
assigns[:attachment].should_not be_nil
assigns[:attachment].id.should_not be_nil
json = JSON.parse(response.body) rescue nil
json = json_parse
json.should_not be_nil
json['id'].should eql(assigns[:attachment].id)
json['upload_url'].should_not be_nil

4
spec/controllers/folders_controller_spec.rb
View File

@ -38,13 +38,13 @@ describe FoldersController do
file.save!
get 'show', :course_id => @course.id, :id => @folder.id, :format => 'json'
json = JSON.parse(response.body)
json = json_parse
json['files'].count.should eql(1)
file.hidden = true
file.save!
get 'show', :course_id => @course.id, :id => @folder.id, :format => 'json'
json = JSON.parse(response.body)
json = json_parse
json['files'].count.should eql(0)
end
end

2
spec/controllers/gradebooks_controller_spec.rb
View File

@ -100,7 +100,7 @@ describe GradebooksController do
assignment2 = @course.assignments.create(:title => "Assignment 2", :group_category => group_category2)
get 'show', :course_id => @course.id, :init => 1, :assignments => 1, :format => 'json'
response.should be_success
data = JSON.parse(response.body) rescue nil
data = json_parse
data.should_not be_nil
data.size.should == 4 # 2 assignments + an assignment group + a total
data.first(2).sort_by{ |a| a['assignment']['title'] }.map{ |a| a['assignment']['group_category'] }.

20
spec/controllers/groups_controller_spec.rb
View File

@ -489,7 +489,7 @@ describe GroupsController do
get 'unassigned_members', :course_id => @course.id, :category_id => group.group_category.id
response.should be_success
data = JSON.parse(response.body) rescue nil
data = json_parse
data.should_not be_nil
data['users'].map{ |u| u['user_id'] }.sort.
should == [u1, u2, u3].map{ |u| u.id }.sort
@ -516,21 +516,21 @@ describe GroupsController do
get 'unassigned_members', :course_id => @course.id, :category_id => group1.group_category.id
response.should be_success
data = JSON.parse(response.body) rescue nil
data = json_parse
data.should_not be_nil
data['users'].map{ |u| u['user_id'] }.sort.
should == [u2, u3].map{ |u| u.id }.sort
get 'unassigned_members', :course_id => @course.id, :category_id => group2.group_category.id
response.should be_success
data = JSON.parse(response.body) rescue nil
data = json_parse
data.should_not be_nil
data['users'].map{ |u| u['user_id'] }.sort.
should == [u1, u3].map{ |u| u.id }.sort
get 'unassigned_members', :course_id => @course.id, :category_id => group3.group_category.id
response.should be_success
data = JSON.parse(response.body) rescue nil
data = json_parse
data.should_not be_nil
data['users'].map{ |u| u['user_id'] }.should == [ u1.id ]
end
@ -544,7 +544,7 @@ describe GroupsController do
group.add_user(u1)
get 'unassigned_members', :course_id => @course.id, :category_id => group.group_category.id
data = JSON.parse(response.body) rescue nil
data = json_parse
data['users'].first['section_id'].should == @course.default_section.id
data['users'].first['section_code'].should == @course.default_section.section_code
end
@ -558,7 +558,7 @@ describe GroupsController do
group.add_user(u1)
get 'context_group_members', :group_id => group.id
data = JSON.parse(response.body) rescue nil
data = json_parse
data.first['section_id'].should == @course.default_section.id
data.first['section_code'].should == @course.default_section.section_code
end
@ -643,7 +643,7 @@ describe GroupsController do
# group2 instead of group1
post 'assign_unassigned_members', :course_id => @course.id, :category_id => category.id
response.should be_success
data = JSON.parse(response.body) rescue nil
data = json_parse
data.size.should == 1
data.first['id'].should == group2.id
end
@ -660,7 +660,7 @@ describe GroupsController do
# student1 shouldn't get assigned, already being in a group
post 'assign_unassigned_members', :course_id => @course.id, :category_id => category.id
response.should be_success
data = JSON.parse(response.body) rescue nil
data = json_parse
data.map{ |g| g['new_members'] }.flatten.map{ |u| u['user_id'] }.should_not be_include(student1.id)
end
@ -676,7 +676,7 @@ describe GroupsController do
# student2 should get assigned, not being in a group
post 'assign_unassigned_members', :course_id => @course.id, :category_id => category.id
response.should be_success
data = JSON.parse(response.body) rescue nil
data = json_parse
data.map{ |g| g['new_members'] }.flatten.map{ |u| u['user_id'] }.should be_include(student2.id)
end
@ -698,7 +698,7 @@ describe GroupsController do
# bring them both to three
post 'assign_unassigned_members', :course_id => @course.id, :category_id => category.id
response.should be_success
data = JSON.parse(response.body) rescue nil
data = json_parse
data.size.should == 2
data.map{ |g| g['id'] }.sort.should == [group1.id, group2.id].sort

2
spec/controllers/users_controller_spec.rb
View File

@ -59,7 +59,7 @@ describe UsersController do
get 'manageable_courses', :user_id => @teacher.id, :term => "MyCourse"
response.should be_success
courses = ActiveSupport::JSON.decode(response.body)
courses = json_parse
courses.map { |c| c['id'] }.should == [course2.id]
end

8
spec/integration/course_copy_spec.rb
View File

@ -34,16 +34,16 @@ describe ContentImportsController, :type => :integration do
post "/courses/#{@copy_to.id}/imports/copy", :copy => {:course_id => @copy_from.id, :all_assignments => 1}
response.should be_success
data = JSON.parse(response.body)
data = json_parse
dj = Delayed::Job.last
api_call(:get, data['status_url'], { :controller => 'content_imports', :action => 'copy_course_status', :course_id => @copy_to.to_param, :id => data['id'].to_param, :format => 'json' })
JSON.parse(response.body)['workflow_state'].should == 'created'
json_parse['workflow_state'].should == 'created'
dj.invoke_job
api_call(:get, data['status_url'], { :controller => 'content_imports', :action => 'copy_course_status', :course_id => @copy_to.to_param, :id => data['id'].to_param, :format => 'json' })
JSON.parse(response.body)['workflow_state'].should == 'completed'
json_parse['workflow_state'].should == 'completed'
@copy_to.reload
@copy_to.assignments.count.should == 1
@ -51,4 +51,4 @@ describe ContentImportsController, :type => :integration do
end
end
end
end

2
spec/integration/security_spec.rb
View File

@ -383,7 +383,7 @@ describe "security" do
user_session(@teacher)
post "/courses/#{@course.id}/user_lists.json", :user_list => "A1234567, A345678"
assert_response :success
ActiveSupport::JSON.decode(response.body).should == {
json_parse.should == {
"duplicates" => [],
"errored_users" => [{"address" => "A345678", "details" => "not_found"}],
"users" => [{ "address" => "A1234567", "name" => "test user", "type" => "pseudonym", "user_id" => u.id.to_s }]

15
spec/javascripts/jQuery.instructureJqueryPatches.js Normal file
View File

@ -0,0 +1,15 @@
(function() {
define(['jquery.instructure_jquery_patches'], function(jQuery) {
module('instructure jquery patches');
return test('parseJSON', function() {
deepEqual(jQuery.parseJSON('{ "var1": "1", "var2" : 2 }'), {
"var1": "1",
"var2": 2
}, 'should still parse without the prefix');
return deepEqual(jQuery.parseJSON('while(1);{ "var1": "1", "var2" : 2 }'), {
"var1": "1",
"var2": 2
}, 'should parse with the prefix');
});
});
}).call(this);

3
spec/javascripts/support/specs.js
View File

@ -6,6 +6,7 @@ require([
'specs/jQuery.instructureMiscPluginsSpec',
'specs/userNamePartsSpec',
'specs/objectCollectionSpec',
'specs/util/BackoffPollerSpec'
'specs/util/BackoffPollerSpec',
'specs/jQuery.instructureJqueryPatches'
]);

5
spec/spec_helper.rb
View File

@ -251,6 +251,7 @@ Spec::Runner.configure do |config|
session = mock()
session.stubs(:record).returns(pseudonym)
session.stubs(:session_credentials).returns(nil)
session.stubs(:used_basic_auth?).returns(false)
PseudonymSession.stubs(:find).returns(session)
end
@ -490,4 +491,8 @@ Spec::Runner.configure do |config|
@attachment.save!
@attachment
end
def json_parse(json_string = response.body)
JSON.parse(json_string.sub(%r{^while\(1\);}, ''))
end
end