5824: Fix path and query escapes.

Paths encode spaces as "%20", not "+".

Rails to_query helper does undesirable things like
"disposition[]=attachment".

diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index 0806a0a4297e57c9c546a3a5d724c56a19e6c32f..f8b359c89060cd9d2703b9303cffaf40f5eef54f 100644 (file)
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -1,6 +1,5 @@
 require "arvados/keep"
 require "uri"
-require "cgi"
 
 class CollectionsController < ApplicationController
   include ActionController::Live
@@ -369,9 +368,9 @@ class CollectionsController < ApplicationController
       uri.path += 't=' + opts[:path_token] + '/'
     end
     uri.path += '_/'
-    uri.path += CGI::escape(file)
+    uri.path += URI.escape(file)
 
-    query = CGI::parse(uri.query || '')
+    query = Hash[URI.decode_www_form(uri.query || '')]
     { query_token: 'api_token',
       disposition: 'disposition' }.each do |opt, param|
       if opts.include? opt
@@ -379,7 +378,7 @@ class CollectionsController < ApplicationController
       end
     end
     unless query.empty?
-      uri.query = query.to_query
+      uri.query = URI.encode_www_form(query)
     end
 
     uri.to_s

diff --git a/apps/workbench/test/controllers/collections_controller_test.rb b/apps/workbench/test/controllers/collections_controller_test.rb
index 8c42145962af30718a4b6aa52c6b23e7dcd35307..978a5133578c9bafa2adb98e6b7b86d455adfe09 100644 (file)
--- a/apps/workbench/test/controllers/collections_controller_test.rb
+++ b/apps/workbench/test/controllers/collections_controller_test.rb
@@ -534,7 +534,7 @@ class CollectionsControllerTest < ActionController::TestCase
       id = api_fixture('collections')['w_a_z_file'][id_type]
       get :show_file, {uuid: id, file: "w a z"}, session_for(:active)
       assert_response :redirect
-      assert_equal "https://#{id.sub '+', '-'}.example/_/w+a+z?api_token=#{tok}", @response.redirect_url
+      assert_equal "https://#{id.sub '+', '-'}.example/_/w%20a%20z?api_token=#{tok}", @response.redirect_url
     end
 
     test "Redirect to keep_web_url via #{id_type} with reader token" do
@@ -543,7 +543,7 @@ class CollectionsControllerTest < ActionController::TestCase
       id = api_fixture('collections')['w_a_z_file'][id_type]
       get :show_file, {uuid: id, file: "w a z", reader_token: tok}, session_for(:expired)
       assert_response :redirect
-      assert_equal "https://#{id.sub '+', '-'}.example/t=#{tok}/_/w+a+z", @response.redirect_url
+      assert_equal "https://#{id.sub '+', '-'}.example/t=#{tok}/_/w%20a%20z", @response.redirect_url
     end
 
     test "Redirect to keep_web_url via #{id_type} with no token" do
@@ -552,7 +552,7 @@ class CollectionsControllerTest < ActionController::TestCase
       id = api_fixture('collections')['public_text_file'][id_type]
       get :show_file, {uuid: id, file: "Hello World.txt"}
       assert_response :redirect
-      assert_equal "https://#{id.sub '+', '-'}.example/_/Hello+World.txt", @response.redirect_url
+      assert_equal "https://#{id.sub '+', '-'}.example/_/Hello%20World.txt", @response.redirect_url
     end
 
     test "Redirect to keep_web_url via #{id_type} with disposition param" do
@@ -565,7 +565,7 @@ class CollectionsControllerTest < ActionController::TestCase
         disposition: 'attachment',
       }
       assert_response :redirect
-      assert_equal "https://#{id.sub '+', '-'}.example/_/Hello+World.txt?disposition=attachment", @response.redirect_url
+      assert_equal "https://#{id.sub '+', '-'}.example/_/Hello%20World.txt?disposition=attachment", @response.redirect_url
     end
 
     test "Redirect to keep_web_download_url via #{id_type}" do
@@ -575,7 +575,7 @@ class CollectionsControllerTest < ActionController::TestCase
       id = api_fixture('collections')['w_a_z_file'][id_type]
       get :show_file, {uuid: id, file: "w a z"}, session_for(:active)
       assert_response :redirect
-      assert_equal "https://download.example/c=#{id.sub '+', '-'}/_/w+a+z?api_token=#{tok}", @response.redirect_url
+      assert_equal "https://download.example/c=#{id.sub '+', '-'}/_/w%20a%20z?api_token=#{tok}", @response.redirect_url
     end
   end
 
@@ -600,7 +600,7 @@ class CollectionsControllerTest < ActionController::TestCase
         disposition: 'attachment',
       }, session_for(:active)
       assert_response :redirect
-      expect_url = "https://download.example/c=#{id.sub '+', '-'}/_/Hello+world.txt"
+      expect_url = "https://download.example/c=#{id.sub '+', '-'}/_/Hello%20world.txt"
       if not anon
         expect_url += "?api_token=#{tok}"
       end
@@ -623,6 +623,6 @@ class CollectionsControllerTest < ActionController::TestCase
     id = api_fixture('collections')['w_a_z_file']['uuid']
     get :show_file, {uuid: id, file: "w a z"}, session_for(:active)
     assert_response :redirect
-    assert_equal "https://download.example/c=#{id.sub '+', '-'}/_/w+a+z?api_token=#{tok}", @response.redirect_url
+    assert_equal "https://download.example/c=#{id.sub '+', '-'}/_/w%20a%20z?api_token=#{tok}", @response.redirect_url
   end
 end