Defect #3692: URL-generation is brittle with trailing slashes - Redmine

Actions

Copy link

Defect #3692

open

URL-generation is brittle with trailing slashes

Added by Wolfgang Schnerring over 15 years ago. Updated over 14 years ago.

Status:

New

Priority:

Normal

Assignee:

Category:

Email notifications

Target version:

Start date:

2009-07-29

Due date:

% Done:

Estimated time:

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.

Files

Download all files

mailer_trailing_slash_fix.patch (1.71 KB) mailer_trailing_slash_fix.patch	Trailing-slash fix + test to cover change.	Olafur Gislason, 2010-06-19 02:23
settings_hostname_trailing_slash_fix.diff (1.54 KB) settings_hostname_trailing_slash_fix.diff	Settings_controller modification and additional test.	Olafur Gislason, 2010-06-21 22:20
3692.patch (1.65 KB) 3692.patch		Gregor Schmidt, 2010-06-21 23:11

Actions

Copy link

Updated by Olafur Gislason over 14 years ago

File mailer_trailing_slash_fix.patch mailer_trailing_slash_fix.patch added

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).

Actions

Copy link

Updated by Holger Just over 14 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).

Actions

Copy link

Updated by Olafur Gislason over 14 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.

Actions

Copy link

Updated by Olafur Gislason over 14 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.

Actions

Copy link

Updated by Olafur Gislason over 14 years ago

File settings_hostname_trailing_slash_fix.diff settings_hostname_trailing_slash_fix.diff added

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.

Actions

Copy link