Skip to content

Commit de2ce63

Browse files
maneframemaneframe
committed
raise error for unescaped ampersands in external tool urls
test plan: * Create an external tool "by xml", with a "launch_url" tag (or any custom property of name "url"), that includes an unescaped ampersand: (e.g. "www.example.com?a=1&b=2") * Confirm that an error is raised indicating the ampersand needs to be escaped (e.g. "www.example.com?a=1&amp;b=2") fixes #CNVS-1324 Change-Id: Id90d216e2d9039187e8a8f4327e2413d07164b04 Reviewed-on: https://gerrit.instructure.com/18872 Tested-by: Jenkins <jenkins@instructure.com> Product-Review: Bracken Mosbacker <bracken@instructure.com> Reviewed-by: Brad Humphrey <brad@instructure.com> QA-Review: Adam Phillipps <adam@instructure.com>
1 parent 64ec546 commit de2ce63

3 files changed

Lines changed: 78 additions & 5 deletions

File tree

‎lib/cc/importer/blti_converter.rb‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,32 @@ def convert_blti_link(doc)
101101
def convert_blti_xml(xml)
102102
doc = Nokogiri::XML(xml)
103103
begin
104-
convert_blti_link(doc)
104+
tool = convert_blti_link(doc)
105+
check_for_unescaped_url_properties(tool) if tool
105106
rescue Nokogiri::XML::XPath::SyntaxError
106-
raise CCImportError.new(I18n.t(:invalid_xml_syntax, "invalid xml syntax"))
107+
raise CCImportError.new(I18n.t(:invalid_xml_syntax, "Invalid xml syntax"))
107108
end
109+
tool
108110
end
109-
111+
112+
def check_for_unescaped_url_properties(obj)
113+
# Recursively look for properties named 'url'
114+
if obj.is_a?(Hash)
115+
obj.select{|k, v| k.to_s == 'url' && v.is_a?(String)}.each do |k, v|
116+
check_for_unescaped_url(v)
117+
end
118+
obj.each{|k, v| check_for_unescaped_url_properties(v)}
119+
elsif obj.is_a?(Array)
120+
obj.each{|o| check_for_unescaped_url_properties(o)}
121+
end
122+
end
123+
124+
def check_for_unescaped_url(url)
125+
if (url =~ /(.*[^\=]*\?*\=)[^\&]*\=/)
126+
raise CCImportError.new(I18n.t(:invalid_url_in_xml, "Invalid url in xml. Ampersands must be escaped."))
127+
end
128+
end
129+
110130
def retrieve_and_convert_blti_url(url)
111131
begin
112132
uri = URI.parse(url)

‎spec/controllers/external_tools_controller_spec.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -285,15 +285,15 @@ def new_valid_tool(course)
285285
response.should_not be_success
286286
assigns[:tool].should be_new_record
287287
json = json_parse(response.body)
288-
json['errors']['base'][0]['message'].should == I18n.t(:invalid_xml_syntax, 'invalid xml syntax')
288+
json['errors']['base'][0]['message'].should == I18n.t(:invalid_xml_syntax, 'Invalid xml syntax')
289289

290290
course_with_teacher_logged_in(:active_all => true)
291291
xml = "<a><b>c</b></a>"
292292
post 'create', :course_id => @course.id, :external_tool => {:name => "tool name", :url => "http://example.com", :consumer_key => "key", :shared_secret => "secret", :config_type => "by_xml", :config_xml => xml}
293293
response.should_not be_success
294294
assigns[:tool].should be_new_record
295295
json = json_parse(response.body)
296-
json['errors']['base'][0]['message'].should == I18n.t(:invalid_xml_syntax, 'invalid xml syntax')
296+
json['errors']['base'][0]['message'].should == I18n.t(:invalid_xml_syntax, 'Invalid xml syntax')
297297
end
298298

299299
it "should handle advanced xml configurations by URL retrieval" do

‎spec/lib/basic_lti_spec.rb‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,5 +334,58 @@
334334
BasicLTI.user_lti_data(teacher, @course2)['role_types'].should == ['urn:lti:sysrole:ims/lis/None']
335335
BasicLTI.user_lti_data(student, @course2)['role_types'].should == ['urn:lti:sysrole:ims/lis/None']
336336
end
337+
338+
it "xml converter should use raise an error when unescaped ampersands are used in launch url" do
339+
xml = <<-XML
340+
<?xml version="1.0" encoding="UTF-8"?>
341+
<cartridge_basiclti_link xmlns="http://www.imsglobal.org/xsd/imslticc_v1p0"
342+
xmlns:blti = "http://www.imsglobal.org/xsd/imsbasiclti_v1p0"
343+
xmlns:lticm ="http://www.imsglobal.org/xsd/imslticm_v1p0"
344+
xmlns:lticp ="http://www.imsglobal.org/xsd/imslticp_v1p0"
345+
xmlns:xsi = "http://www.w3.org/2001/XMLSchema-instance"
346+
xsi:schemaLocation = "http://www.imsglobal.org/xsd/imslticc_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticc_v1p0.xsd
347+
http://www.imsglobal.org/xsd/imsbasiclti_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imsbasiclti_v1p0.xsd
348+
http://www.imsglobal.org/xsd/imslticm_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticm_v1p0.xsd
349+
http://www.imsglobal.org/xsd/imslticp_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticp_v1p0.xsd">
350+
<blti:title>Other Name</blti:title>
351+
<blti:description>Description</blti:description>
352+
<blti:launch_url>http://example.com/other_url?unescapedampersands=1&arebadnews=2</blti:launch_url>
353+
<cartridge_bundle identifierref="BLTI001_Bundle"/>
354+
<cartridge_icon identifierref="BLTI001_Icon"/>
355+
</cartridge_basiclti_link>
356+
XML
357+
lti = CC::Importer::BLTIConverter.new
358+
lambda {lti.convert_blti_xml(xml)}.should raise_error
359+
end
360+
361+
it "xml converter should use raise an error when unescaped ampersands are used in custom url properties" do
362+
xml = <<-XML
363+
<?xml version="1.0" encoding="UTF-8"?>
364+
<cartridge_basiclti_link xmlns="http://www.imsglobal.org/xsd/imslticc_v1p0"
365+
xmlns:blti = "http://www.imsglobal.org/xsd/imsbasiclti_v1p0"
366+
xmlns:lticm ="http://www.imsglobal.org/xsd/imslticm_v1p0"
367+
xmlns:lticp ="http://www.imsglobal.org/xsd/imslticp_v1p0"
368+
xmlns:xsi = "http://www.w3.org/2001/XMLSchema-instance"
369+
xsi:schemaLocation = "http://www.imsglobal.org/xsd/imslticc_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticc_v1p0.xsd
370+
http://www.imsglobal.org/xsd/imsbasiclti_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imsbasiclti_v1p0.xsd
371+
http://www.imsglobal.org/xsd/imslticm_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticm_v1p0.xsd
372+
http://www.imsglobal.org/xsd/imslticp_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticp_v1p0.xsd">
373+
<blti:title>Other Name</blti:title>
374+
<blti:description>Description</blti:description>
375+
<blti:launch_url>http://example.com</blti:launch_url>
376+
<blti:extensions platform="canvas.instructure.com">
377+
<lticm:property name="privacy_level">public</lticm:property>
378+
<lticm:options name="course_navigation">
379+
<lticm:property name="url">https://example.com/attendance?param1=1&param2=2</lticm:property>
380+
<lticm:property name="enabled">true</lticm:property>
381+
</lticm:options>
382+
</blti:extensions>
383+
<cartridge_bundle identifierref="BLTI001_Bundle"/>
384+
<cartridge_icon identifierref="BLTI001_Icon"/>
385+
</cartridge_basiclti_link>
386+
XML
387+
lti = CC::Importer::BLTIConverter.new
388+
lambda {lti.convert_blti_xml(xml)}.should raise_error
389+
end
337390
end
338391
end

0 commit comments

Comments
 (0)