should not improperly escape imported urls

test plan:
* in html content (such as a wiki page), add a link
 to reference an external tool by url (the tool itself
 doesn't really need to exist, but the link won't work)
 passing in an escaped url that itself has a
 query string referencing an escaped url

e.g.:
if you had a tool configured for http://www.example.com,
you could add a link to

```
<a href="/courses/COURSE_ID/external_tools/retrieve?url=
http%3A%2F%2Fwww.example.com%3Furl%3Dhttp%253A%252F%252F
www.anotherurl.com">Link</a>
```

(replacing COURSE_ID with the course's id)

* copy the course
* the link in the new page should be identical in form
to the old

closes #CNVS-13185

Change-Id: I6bd57e10a7306823061f0d38c8214655a2e197c6
Reviewed-on: https://gerrit.instructure.com/42117
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>
QA-Review: Jahnavi Yetukuri <jyetukuri@instructure.com>

lib/imported_html_converter.rb

@@ -21,7 +21,7 @@ class ImportedHtmlConverter
   include HtmlTextHelper
 
   CONTAINER_TYPES = ['div', 'p', 'body']
-
+  REFERENCE_KEYWORDS = %w{CANVAS_COURSE_REFERENCE CANVAS_OBJECT_REFERENCE WIKI_REFERENCE IMS_CC_FILEBASE}
   # yields warnings
   def self.convert(html, context, migration=nil, opts={})
     doc = Nokogiri::HTML(html || "")
@@ -43,7 +43,10 @@ class ImportedHtmlConverter
           new_url = nil
           missing_relative_url = nil
 
-          val = URI.unescape(node[attr]) rescue node[attr]
+          val = node[attr].dup
+          REFERENCE_KEYWORDS.each do |ref|
+            val.gsub!("%24#{ref}%24", "$#{ref}$")
+          end
 
           if val =~ /wiki_page_migration_id=(.*)/
             # This would be from a BB9 migration. 
@@ -57,12 +60,12 @@ class ImportedHtmlConverter
           elsif val =~ /discussion_topic_migration_id=(.*)/
             if topic_migration_id = $1
               if linked_topic = context.discussion_topics.where(migration_id: topic_migration_id).first
-                new_url = URI::escape("#{course_path}/discussion_topics/#{linked_topic.id}")
+                new_url = "#{course_path}/discussion_topics/#{linked_topic.id}"
               end
             end
           elsif val =~ %r{\$CANVAS_COURSE_REFERENCE\$/modules/items/(.*)}
             if tag = context.context_module_tags.where(:migration_id => $1).select('id').first
-              new_url = URI::escape "#{course_path}/modules/items/#{tag.id}"
+              new_url = "#{course_path}/modules/items/#{tag.id}"
             end
           elsif val =~ %r{(?:\$CANVAS_OBJECT_REFERENCE\$|\$WIKI_REFERENCE\$)/([^/]*)/(.*)}
             type = $1
@@ -74,18 +77,18 @@ class ImportedHtmlConverter
               new_url = "#{course_path}/#{context.feature_enabled?(:draft_state) ? 'pages' : 'wiki'}/#{migration_id}"
             elsif type == 'attachments'
               if att = context.attachments.where(migration_id: migration_id).first
-                new_url = URI::escape("#{course_path}/files/#{att.id}/preview")
+                new_url = "#{course_path}/files/#{att.id}/preview"
               end
             elsif context.respond_to?(type) && context.send(type).respond_to?(:where)
               if object = context.send(type).where(migration_id: migration_id).first
-                new_url = URI::escape("#{course_path}/#{type_for_url}/#{object.id}")
+                new_url = "#{course_path}/#{type_for_url}/#{object.id}"
               end
             end
           elsif val =~ %r{\$CANVAS_COURSE_REFERENCE\$/(.*)}
             section = $1
-            new_url = URI::escape("#{course_path}/#{section}")
+            new_url = "#{course_path}/#{section}"
           elsif val =~ %r{\$IMS_CC_FILEBASE\$/(.*)}
-            rel_path = $1
+            rel_path = URI.unescape($1)
             if attr == 'href' && node['class'] && node['class'] =~ /instructure_inline_media_comment/
               new_url = replace_media_comment_data(node, rel_path, context, opts) {|warning, data| yield warning, data if block_given?}
               unless new_url
@@ -120,8 +123,9 @@ class ImportedHtmlConverter
           else
             begin
               if relative_url?(node[attr])
-                unless new_url = replace_relative_file_url(val, context)
-                  missing_relative_url = val
+                unescaped = URI.unescape(val)
+                unless new_url = replace_relative_file_url(unescaped, context)
+                  missing_relative_url = unescaped
                 end
               else
                 new_url = node[attr]

spec/models/content_migration/course_copy_external_tools_spec.rb

@@ -142,5 +142,15 @@ describe ContentMigration do
           {"id" =>0 }, {"id" => "context_external_tool_#{@tool_from.id}"}, {"id" => 14}
       ]
     end
+
+    it "should not double-escape retrieval urls" do
+      url = "http://www.example.com?url=http%3A%2F%2Fwww.anotherurl.com"
+
+      @copy_from.syllabus_body = "<p><iframe src=\"/courses/#{@copy_from.id}/external_tools/retrieve?url=#{CGI.escape(url)}\" width=\"320\" height=\"240\" style=\"width: 553px; height: 335px;\"></iframe></p>"
+
+      run_course_copy
+
+      @copy_to.syllabus_body.should == @copy_from.syllabus_body.sub("/courses/#{@copy_from.id}/", "/courses/#{@copy_to.id}/")
+    end
   end
 end