bug #64441 fixed #328

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
8 participants
@Syra
Contributor

Syra commented Apr 21, 2013

@DaveRandom

This comment has been minimized.

Show comment Hide comment
@DaveRandom

DaveRandom Apr 21, 2013

Contributor

Do you have reference RFC that states this to be valid? An FQDN is not mandated to end with a dot in any URL specifications I can find, and neither does it appear to be legal - the only place that does allow this syntax that I am aware of is the DNS zone files as specified in RFC1035.

I could be wrong, but I suspect that it is parse_url() that is at "fault" here for accepting them, rather than the other way around - although I would say this is not a problem, a parser should be more forgiving than a validator.

Contributor

DaveRandom commented Apr 21, 2013

Do you have reference RFC that states this to be valid? An FQDN is not mandated to end with a dot in any URL specifications I can find, and neither does it appear to be legal - the only place that does allow this syntax that I am aware of is the DNS zone files as specified in RFC1035.

I could be wrong, but I suspect that it is parse_url() that is at "fault" here for accepting them, rather than the other way around - although I would say this is not a problem, a parser should be more forgiving than a validator.

@Syra

This comment has been minimized.

Show comment Hide comment
@Syra

Syra Apr 22, 2013

Contributor

Yes, it's caused by FQDN. In RFC 1738 "host" is "The fully qualified domain name of a network host".
So valid url may ended by a dot.

Contributor

Syra commented Apr 22, 2013

Yes, it's caused by FQDN. In RFC 1738 "host" is "The fully qualified domain name of a network host".
So valid url may ended by a dot.

@Syra

This comment has been minimized.

Show comment Hide comment
@Syra

Syra Apr 22, 2013

Contributor

Updated RFC2396 still the same. RFC3986 supports the same strategy.

Contributor

Syra commented Apr 22, 2013

Updated RFC2396 still the same. RFC3986 supports the same strategy.

@DaveRandom

This comment has been minimized.

Show comment Hide comment
@DaveRandom

DaveRandom Apr 22, 2013

Contributor

After re-reading the relevant RFCs I now agree that this is probably correct behaviour, although it does look a little odd, but since the final empty label refers to the "root domain" this is probably technically valid. It certainly makes sense in terms of how the DNS resolver works.

I will try compiling this later and see if I can break your fix :-P (although at first glance it looks sensible). However, you should definitely add a test to the PR.

Contributor

DaveRandom commented Apr 22, 2013

After re-reading the relevant RFCs I now agree that this is probably correct behaviour, although it does look a little odd, but since the final empty label refers to the "root domain" this is probably technically valid. It certainly makes sense in terms of how the DNS resolver works.

I will try compiling this later and see if I can break your fix :-P (although at first glance it looks sensible). However, you should definitely add a test to the PR.

@lstrojny

This comment has been minimized.

Show comment Hide comment
@lstrojny

lstrojny Apr 22, 2013

Contributor

Yes, please don’t merge without a test.

Contributor

lstrojny commented Apr 22, 2013

Yes, please don’t merge without a test.

@Syra

This comment has been minimized.

Show comment Hide comment
@Syra

Syra Apr 22, 2013

Contributor

I'm new on this project and I'm glad to get your responses.

Contributor

Syra commented Apr 22, 2013

I'm new on this project and I'm glad to get your responses.

@smalyshev

This comment has been minimized.

Show comment Hide comment
@smalyshev

smalyshev Jun 17, 2013

Contributor

This passes http://a./ as a correct URL. I don't think it is.

Contributor

smalyshev commented Jun 17, 2013

This passes http://a./ as a correct URL. I don't think it is.

@smalyshev

This comment has been minimized.

Show comment Hide comment
@smalyshev

smalyshev Jun 17, 2013

Contributor

Reading RFC, I think I was wrong, trailing dot is valid. But some servers can't handle it properly as it seems: http://stackoverflow.com/questions/3314192/how-do-i-allow-trailing-dot-in-host-headers-in-iis/3483411#3483411
So I'm not sure what to do here...

Contributor

smalyshev commented Jun 17, 2013

Reading RFC, I think I was wrong, trailing dot is valid. But some servers can't handle it properly as it seems: http://stackoverflow.com/questions/3314192/how-do-i-allow-trailing-dot-in-host-headers-in-iis/3483411#3483411
So I'm not sure what to do here...

@lt

This comment has been minimized.

Show comment Hide comment
@lt

lt Jun 17, 2013

Contributor

@smalyshev We should be server agnostic right? If a particular server has a bug that means it does not conform to the RFC, then the maintainers of that server should fix it.

My personal opinion is that we should allow the trailing dot as a valid hostname, as it is valid per the RFC.

Unrelated but interesting side note, Firefox will complain about hosts with a trailing dot over SSL, because the certificate does not match the hostname. Chrome is happy to accept the host with a trailing dot when the certificate does not match the hostname.

It seems "issues" related to this trailing dot are widespread. Again I think we should comply with the RFC

Contributor

lt commented Jun 17, 2013

@smalyshev We should be server agnostic right? If a particular server has a bug that means it does not conform to the RFC, then the maintainers of that server should fix it.

My personal opinion is that we should allow the trailing dot as a valid hostname, as it is valid per the RFC.

Unrelated but interesting side note, Firefox will complain about hosts with a trailing dot over SSL, because the certificate does not match the hostname. Chrome is happy to accept the host with a trailing dot when the certificate does not match the hostname.

It seems "issues" related to this trailing dot are widespread. Again I think we should comply with the RFC

@dsp

This comment has been minimized.

Show comment Hide comment
@dsp

dsp Sep 16, 2013

Member

I agree we should comply with the RFC, particularly as it's backed by popular DNS software like bind and browsers. I've queued it for 5.4.21 unless there are objections.

Member

dsp commented Sep 16, 2013

I agree we should comply with the RFC, particularly as it's backed by popular DNS software like bind and browsers. I've queued it for 5.4.21 unless there are objections.

@php-pulls

This comment has been minimized.

Show comment Hide comment
@php-pulls

php-pulls Oct 9, 2013

Comment on behalf of mike at php.net:

Already merged.

Comment on behalf of mike at php.net:

Already merged.

@php-pulls php-pulls closed this Oct 9, 2013

@kaplanlior

This comment has been minimized.

Show comment Hide comment
@kaplanlior

kaplanlior Oct 10, 2013

Contributor

For future reference commit SHA1 is 18b54a2

Contributor

kaplanlior commented Oct 10, 2013

For future reference commit SHA1 is 18b54a2

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