Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix improperly configured host in generated urls #9838

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Member

schneems commented Mar 20, 2013

If the host in default_url_options is accidentally set with a protocol such as

host: "http://example.com"

then the generated url will have the protocol twice http://http://example.com which is not what the user intended. Likely they wanted to define a host host: "example.com" and a protocol: "http://" but did not know the convention.

This may not the most common problem, but when it happens it can go undetected for a while. I accidentally added http:// out of habit recently only to find all the links in my emails were broken after deploying a demo site to production. Rather than allow this accident go undetected, we can fix the problem in line by properly setting the protocol and host.

I was able to find this related question on stack overflow: http://stackoverflow.com/questions/5878329/rails-3-devise-how-do-i-make-the-email-confirmation-links-use-secure-https-n where the answer was highly upvoted.

This is based off of work in #7415 cc/ @pixeltrix

back port of #9794

ATP Action Mailer and Action Pack

Fix improperly configured host in generated urls
If the host in `default_url_options` is accidentally set with a protocol such as 

```
host: "http://example.com"
``` 

then the generated url will have the protocol twice `http://http://example.com` which is not what the user intended. Likely they wanted to define a host `host: "example.com"` and a `protocol: "http://"` but did not know the convention.

This may not the most common problem, but when it happens it can go undetected for a while. I accidentally added `http://` out of habit recently only to find all the links in my emails were broken after deploying a demo site to production. Rather than allow this accident go undetected, we can fix the problem in line by properly setting the protocol and host.


I was able to find this related question on stack overflow: http://stackoverflow.com/questions/5878329/rails-3-devise-how-do-i-make-the-email-confirmation-links-use-secure-https-n where the answer was highly upvoted.

This is based off of work in #7415 cc/ @pixeltrix

back port of #9794

ATP Action Mailer and Action Pack
Member

schneems commented Mar 20, 2013

@pixeltrix would also be nice if we ignored host's with backslashes or used a more intelligent joining method like File.joins otherwise we end up with this

bar_path(host: 'example.com/')

Turns into example.com//bar

Owner

pixeltrix commented Mar 21, 2013

We could always chomp it:

>> "http://www.example.com/".chomp('/')
=> "http://www.example.com"

schneems added a commit to schneems/rails that referenced this pull request May 30, 2013

Remove double trailing slash on url generation
Mentioned in #9838

Currently this:
```
bar_path(host: 'example.com/')
```

Turns into `example.com//bar`

Which is not correct, this PR fixes that issue.

ATP actionpack cc @pixeltrix
Contributor

josevalim commented May 30, 2013

Ok, let me do the grumpy old man job: it is to a certain point the responsibility of the user to provide correct data. However, it is our responsibility to document and name our API as clearly as possible.

That said, :host is a very clear name. A host does not contain a schema. A host does not contain paths. I think normalizing those things is a waste of resource for the 99% of the correct cases at the cost of teaching wrong semantics to users!

So I'm 👎 on this one. ❤️

Member

schneems commented May 30, 2013

The problem is that it is a very expensive yet silent lesson. If you configure the host wrong then your outbound links in emails suddenly quit working all without exception or warning.

If we know an invalid input causes an invalid output we should either fix the input or raise an instructive error.

Richard Schneeman
http://heroku.com
@schneems

Sent from the road

On Thursday, May 30, 2013 at 6:01 PM, José Valim wrote:

Ok, let me do the grumpy old man job: it is to a certain point the responsibility of the user to provide correct data. However, it is our responsibility to document and name our API as clearly as possible.
That said, :host is a very clear name. A host does not contain a schema. A host does not contain paths. I think normalizing those things is a waste of resource for the 99% of the correct cases at the cost of teaching wrong semantics to users!


Reply to this email directly or view it on GitHub (#9838 (comment)).

ekampp commented May 30, 2013

I have had this problem as well, back when I didn't know these things. An instructive error would have been lovely, to let me know that rails made that distinction.

knappe commented Jul 10, 2013

I've seen this error often in various applications. In fact, I've seen it enough that I ended up doing some checks and manipulating the url mailed out to ensure both a host and protocol were specified to prevent this particular error from happening in a recent application.

👍

Owner

pixeltrix commented Jul 10, 2013

I don't think we need to backport this now that Rails 4.0 has been released.

@pixeltrix pixeltrix closed this Jul 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment