Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatibility with DNS RFC 1034 #137

Closed
wants to merge 7 commits into from
Closed

Compatibility with DNS RFC 1034 #137

wants to merge 7 commits into from

Conversation

LeShadow
Copy link

In one of my commits, there is a wrong RFC specification, this should be RFC 1034.

According to RFC 1034, every DNS query needs to have a . at the end of
the name record you are requesting.

This will show that that name record is the root of the domain you are
asking
In some cases, when the . is missing, the hostname of the server the
code is running on will be appended to the name record when
checkdnsrr() is being executed and this will in turn return a false
positive when there is a wildcard DNS record for the root domain of the
hostname of the server.

The first commit adds the . to the hostname when you do a dns query with checkdnsrr()
The second commit fixes a few deprecation warnings in the tests

LeShadow and others added 3 commits January 25, 2017 16:05
According to RFC2181, every DNS query needs to have a . at the end of
the name record you are requesting.

This will show that that name record is the root of the domain you are
asking
In some cases, when the . is missing, the hostname of the server the
code is running on will be appended to the name record when
checkdnsrr() is being executed and this will in turn return a false
positive when there is a wildcard DNS record for the root domain of the
hostname of the server.
@egulias
Copy link
Owner

egulias commented Jan 25, 2017

Hi @LeShadow
Which was the warning you were getting?
Another PR (#136) has brought conflicts with yours. Do you mind fixing them with a rebase (to have a clean history?).
Thanks!

@egulias
Copy link
Owner

egulias commented Jan 25, 2017

You can also take the chance to amend the wrong RFC number on the commit message.

According to RFC2181, every DNS query needs to have a . at the end of
the name record you are requesting.

This will show that that name record is the root of the domain you are
asking
In some cases, when the . is missing, the hostname of the server the
code is running on will be appended to the name record when
checkdnsrr() is being executed and this will in turn return a false
positive when there is a wildcard DNS record for the root domain of the
hostname of the server.
@LeShadow
Copy link
Author

@egulias My apologies for the mess. My experience with git is limited and I haven't done much with it.
The PR should be good now. Although I tried to edit my commit message and somehow failed.

I didn't really get a warning, I found out that this library didn't followed the RFC because of a code my client was executing.

He was using Drupal 8.x, which uses this library for email validation. Every time he checked an email and he didn't add a . to the hostname (name record), the server hostname's root domain was added to the request.

This is how I found out that you should add a . to the hostname

@nielsaers
Copy link

If we want to fix this for Drupal 8 we'd have to fix version 1.2.* Currently Drupal 8 still uses version 1.2 (https://github.com/drupal/drupal/blob/8.3.x/core/composer.json)

I don't know what your policy is of applying fixes to older versions?

@egulias egulias mentioned this pull request Jan 30, 2017
@egulias
Copy link
Owner

egulias commented Jan 30, 2017

Hi @nielsaers
I'm keeping 1.2.* as much as possible, however porting fixes require a work explicitly for 1.2 as it is not compatible with v2.
I've created the issue #138 to port it. PR's are welcome!

@egulias
Copy link
Owner

egulias commented Jan 30, 2017

Hey @LeShadow don't be so harsh to yourself :). I'm happy to help you here.
For some reason you just brought all the other change without respecting the original commits and it is now looking that you are bringing a lot of changes.

As @nielsaers pointed out, these should be done in branch 1.2 as well.

Either way, I'll try fix the PR. I might need to do a new one on your behalf.

@egulias egulias mentioned this pull request Jan 30, 2017
@egulias
Copy link
Owner

egulias commented Jan 30, 2017

See #139

@egulias egulias closed this in #139 Jan 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants