Defect #3692

URL-generation is brittle with trailing slashes

Added by Wolfgang Schnerring about 11 years ago. Updated about 10 years ago.

Status:NewStart date:2009-07-29
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:Email notifications
Target version:-
Resolution: Affected version:

Description

When the setting Adminstration/Settings/General/"Hostname and path" has a trailing slash (e. g. "http://example.com/redmine/", the links in the "issue updated" emails contain duplicate slashes (e. g. "http://example.com/redmine//repositories/revision/myproject/17") which cannot be routed and give 404 instead.

I've scrutinized the code, and the underlying reason is that url_for does not care about a trailing slash in the "host" part of the URL and simply appends the full path even if that duplicates the slash. I'm not sure whether it should do so, or if Redmine should be careful to strip any trailing slashes instead (in app/models/mailer.rb Mailer::default_url_options).

Either way, this behaviour is quite brittle and also rather non-obvious to figure out, so it would be nice if Redmine did the Right Thing(tm) regardless of trailing slashes in the "hostname" setting.

mailer_trailing_slash_fix.patch Magnifier - Trailing-slash fix + test to cover change. (1.71 KB) Olafur Gislason, 2010-06-19 02:23

settings_hostname_trailing_slash_fix.diff Magnifier - Settings_controller modification and additional test. (1.54 KB) Olafur Gislason, 2010-06-21 22:20

3692.patch Magnifier (1.65 KB) Gregor Schmidt, 2010-06-21 23:11

History

#1 Updated by Olafur Gislason about 10 years ago

Created a small patch to clean up trailing slash when using the mailer (in app/models/mailer.rb).
A simple regular expression used to cut the slash of if it exists.

Test added into mailer_test.rb (in test/unit/mailer_test.rb) named "test_generated_links_in_emails_with_trailing_slash".

All test ran without failures.

NOTE that using a ready-made library (e.g. UrlWriter / UrlRewriter if they support this kind of operation) might be better than doing a manual string modification, but it's debatable.

Patch created against redmine-trunk (rev. 3771).

#2 Updated by Holger Just about 10 years ago

People probably use Setting.hostname throughout the code and in plugins. Maybe you want to put the patch there (into the Setting model).

#3 Updated by Olafur Gislason about 10 years ago

Holger Just wrote:

People probably use Setting.hostname throughout the code and in plugins. Maybe you want to put the patch there (into the Setting model).

I tested that also. I wanted this kind of feedback to decide where to do the modification.
The reason I chose mailer.rb is that this trailing-slash bug is only in the mailer. It seems that the rest of the system is able to fix this automatically.

I can post the setting-controller fix also if people want.

#4 Updated by Olafur Gislason about 10 years ago

Holger Just wrote:

People probably use Setting.hostname throughout the code and in plugins. Maybe you want to put the patch there (into the Setting model).

Come to think of it, I agree with you. Better to remove the problem at the source rather than hoping that people will check for this in all modules.

I'll post a patch shortly.

#5 Updated by Olafur Gislason about 10 years ago

I've created a patch which cuts of a trailing forward-slash of the host-name before saving it down into the model.
I could not find a clean way of doing this in the model class, but it is most likely possible.
I included additional test for the settings_controller to proof the functionality.

#6 Updated by Gregor Schmidt about 10 years ago

Attached is an alternative patch at model level. Although I'm not sure, which is better.

Also available in: Atom PDF