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 #7415

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
Member

schneems commented Aug 22, 2012

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

ATP Action Mailer, Actionpack, and Railties

Owner

pixeltrix commented Aug 22, 2012

How about we extract the protocol from the host and set both properly? We know what the developer is trying to do so why raise an error?

Contributor

josevalim commented Aug 22, 2012

@pixeltrix that's also a fine approach.

About the pull request, I like the general direction but I don't like much the implementation. However, I still couldn't think of a better way to handle this, so it isn't necessarily a blocker.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Oct 7, 2012

actionmailer/test/url_test.rb
@@ -84,4 +84,14 @@ def test_signed_up_with_url
delivered.message_id = '<123@456>'
assert_equal expected.encoded, delivered.encoded
end
+
+ def test_improperly_assigned_options_raise_an_error
+ original_host = UrlTestMailer.default_url_options[:host]
+ UrlTestMailer.default_url_options = {:host => 'http://www.basecamphq.com'}
@rafaelfranca

rafaelfranca Oct 7, 2012

Owner

Use the Ruby 1.9 hash syntax

Contributor

frodsan commented Oct 26, 2012

@pixeltrix @josevalim Hey guys, could you review this again? Thanks.

Member

schneems commented Dec 5, 2012

ping, in or out.

@schneems probably in, can you add a changelog entry? I can't think of anything different for that right now as well. Will ping others to check it. Thanks!

Member

schneems commented Dec 5, 2012

Added changelog, ping if you need changes.

@schneems schneems Fix improperly configured host
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 is 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. 

ATP Action Mailer, Actionpack, and Railties
759335b
Contributor

timraymond commented Dec 17, 2012

👍 I'm so used to typing out http:// before urls, I'm amazed this hasn't bit me in the past.

Owner

pixeltrix commented Dec 17, 2012

@schneems like @josevalim I'm not keen on the implementation - the best place for the check to happen is in build_host_url as all url building goes through it. Can you reimplement it as part of that method - thanks! ❤️

Member

schneems commented Dec 17, 2012

@pixeltrix that seems like a worthy rabbit hole to go down, i'll add this to my hopper and try it out. Thanks for the direction.

Contributor

JonRowe commented Mar 10, 2013

Ping! What's happening with this?

Owner

pixeltrix commented Mar 19, 2013

Closing in favour of #9794

@pixeltrix pixeltrix closed this Mar 19, 2013

@schneems schneems added a commit to schneems/rails that referenced this pull request Mar 19, 2013

@schneems schneems 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

ATP Action Mailer and Action Pack
334549b

@schneems schneems added a commit to schneems/rails that referenced this pull request Mar 20, 2013

@schneems schneems 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
3f11317
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment