Project

General

Profile

Actions

Defect #31831

closed

Back url parse in validation

Added by Tibinko H over 4 years ago. Updated about 1 month ago.

Status:
Closed
Priority:
Normal
Category:
Code cleanup/refactoring
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
Affected version:

Description

Hello,

for example on issues screen, when you apply filters, it generates an url containing:

utf8=✓

Because of this bulk_edit (for instance) is unable to redirect back to this url.
The reason is a function in application_controller.rb - validate_back_url

URI.parse(back_url) is unable to parse the special character mentioned above.

The quicke workaround would be

URI.parse(URI.encode(back_url))

but i am not sure that is sufficient.


Files

31831.patch (1.2 KB) 31831.patch Go MAEDA, 2024-02-13 10:16

Related issues

Related to Redmine - Defect #31552: View switches from gantt to list after editing an issueClosedGo MAEDA

Actions
Related to Redmine - Feature #40190: Stop appending the utf8 checkmark parameter to form URLsClosedMarius BĂLTEANU

Actions
Actions #1

Updated by Mizuki ISHIKAWA over 4 years ago

It looks good.
I have also suggested the same fix (#31552#note-2).

Actions #2

Updated by Tibinko H over 4 years ago

Oh, ok. Did not see that, I am going to put this isssue to resolved then.

Actions #3

Updated by Tibinko H over 4 years ago

  • Status changed from New to Resolved
Actions #4

Updated by Marius BĂLTEANU over 4 years ago

  • Status changed from Resolved to New

Tibinko H wrote:

Oh, ok. Did not see that, I am going to put this isssue to resolved then.

It was committed another solution in that ticket.

Actions #5

Updated by Tibinko H over 4 years ago

Marius BALTEANU wrote:

Tibinko H wrote:

Oh, ok. Did not see that, I am going to put this isssue to resolved then.

It was committed another solution in that ticket.

Is it not exactly the same as I wrote above?

Actions #6

Updated by Mizuki ISHIKAWA over 4 years ago

Tibinko H wrote:

Oh, ok. Did not see that, I am going to put this isssue to resolved then.

I think this problem has not been solved yet.

Sorry, my comment(#31831#note-1) may have misled you.
In #31552 issue, #31552#note-3 changes were finally committed, not #31552#note-2 changes.
#31552#note-3 changes were a partial fix and does not solve the bulk_edit probrem.

URI.parse(URI.encode(back_url))

I thought that your suggestion for solving the problem that occurs with bulk_edit is good.

Actions #7

Updated by Go MAEDA over 4 years ago

  • Target version set to Candidate for next minor release

Tibinko H, thank you for reporting this issue.

Here is a diff of your workaround.

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 06e2d702c..afbb30f3e 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -440,7 +440,7 @@ class ApplicationController < ActionController::Base
     end

     begin
-      uri = URI.parse(back_url)
+      uri = URI.parse(URI.encode(back_url))
     rescue URI::InvalidURIError
       return false
     end
Actions #8

Updated by Go MAEDA over 4 years ago

  • Related to Defect #31552: View switches from gantt to list after editing an issue added
Actions #9

Updated by Go MAEDA about 1 month ago

You can fix this problem by preventing Rails from appending a "utf8" parameter to form URLs. This can be achieved by setting config.action_view.default_enforce_utf8 to false.
https://guides.rubyonrails.org/v7.1/configuring.html#config-action-view-default-enforce-utf8

We can safely remove the "utf8=✓". This is because this is a workaround for Internet Explorer <=8 and Microsoft and Redmine no longer support the browser.

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 9bd6b831a..ad16b0e77 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -475,11 +475,6 @@ class ApplicationController < ActionController::Base
     url = params[:back_url]
     if url.nil? && referer = request.env['HTTP_REFERER']
       url = CGI.unescape(referer.to_s)
-      # URLs that contains the utf8=[checkmark] parameter added by Rails are
-      # parsed as invalid by URI.parse so the redirect to the back URL would
-      # not be accepted (ApplicationController#validate_back_url would return
-      # false)
-      url.gsub!(/(\?|&)utf8=\u2713&?/, '\1')
     end
     url
   end
diff --git a/config/application.rb b/config/application.rb
index 069796185..1beeb2db2 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -45,6 +45,9 @@ module RedmineApp

     config.action_mailer.delivery_job = "ActionMailer::MailDeliveryJob" 

+    # Stop appending "utf8=✓" to form URLs
+    config.action_view.default_enforce_utf8 = false
+
     # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone.
     # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC.
     # config.time_zone = 'Central Time (US & Canada)'
Actions #10

Updated by Go MAEDA about 1 month ago

  • Related to Feature #40190: Stop appending the utf8 checkmark parameter to form URLs added
Actions #11

Updated by Go MAEDA about 1 month ago

I have opened #40190 and posted the patch pasted in #note-9.

Apart from that patch, I suggest replacing URI.parse with Addressable::URI.parse(url) to address the reported issue. This change is useful for handling URLs containing non-ASCII characters, as demonstrated below.

irb(main):001> url = 'http://www.example.com/?utf8=✓'
=> "http://www.example.com/?utf8=✓" 
irb(main):002> URI.parse(url)
/Users/maeda/.rbenv/versions/3.1.4/lib/ruby/3.1.0/uri/rfc3986_parser.rb:20:in `split': URI must be ascii only "http://www.example.com/?utf8=\u2713" (URI::InvalidURIError)
irb(main):003> Addressable::URI.parse(url)
=> #<Addressable::URI:0x9b14 URI:http://www.example.com/?utf8=>

The replacement not only fixes the immediate issue but also enhances the robustness of validate_back_url. It remains beneficial even after the patch in #40190 is merged.

Actions #12

Updated by Marius BĂLTEANU about 1 month ago

  • Assignee set to Marius BĂLTEANU
Actions #13

Updated by Marius BĂLTEANU about 1 month ago

  • Category set to Code cleanup/refactoring
  • Status changed from New to Resolved
  • Resolution set to Fixed

Committed the patch after I've committed the one from #40190. Go MAEDA, are you thinking to merge this to stable branches without the changes from #40190?

Actions #14

Updated by Go MAEDA about 1 month ago

  • Target version changed from Candidate for next minor release to 6.0.0

Marius BĂLTEANU wrote in #note-13:

Go MAEDA, are you thinking to merge this to stable branches without the changes from #40190?

I think we don't need to merge this change to stable branches because the issue is very minor.

Actions #15

Updated by Marius BĂLTEANU about 1 month ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF