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

freedns: fix domain splitting for multi-part domains #1807

Closed
wants to merge 2 commits into from

Conversation

frebib
Copy link

@frebib frebib commented Aug 21, 2018

This should address issue #1086.
Supercedes previous PR #1454.

This should fix the situation mentioned in #1454 (comment)

@JonnyTech
Copy link

This works perfectly for my .co.uk domains - @Neilpang can this be merged please?

@frebib
Copy link
Author

frebib commented Oct 1, 2018

I understand that there might be some code-style issues with this PR, if so please leave a review and I will get them fixed and squashed

@chrishas35
Copy link

My FreeDNS domain is a subdomain of a .net. Had the same issue and this PR solves it. Thanks @frebib.

@frebib
Copy link
Author

frebib commented Mar 29, 2019

Bump @Neilpang, any reason this (and the other 100+ PRs) can't be merged?

_err "Please export as FREEDNS_User / FREEDNS_Password and try again."
# Check if there is a dot in the top_domain
# If not, we can't split any more and should assume the domain doesn't exist
if ! echo "$top_domain" | grep -qF '.'; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please not use grep -qF options here ?
it's dangerous

return 1
fi

subdomain_csv="$(echo "$htmlpage" | tr -d "\n\r" | _egrep_o '<form .*</form>' | sed 's/<tr>/@<tr>/g' | tr '@' '\n' | grep edit.php | grep "$top_domain")"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you trim \r\n before piping to _egrep_o() function, it will not work on solaris.

On solaris, _egrep_o is implemented by sed, but sed on solaris can not work without \n.

please consider replacement

@Neilpang
Copy link
Member

sorry, I was just too busy to review code.

@dkerr64
Copy link
Contributor

dkerr64 commented May 19, 2019

This PR just came to my attention. As the original author of the freedns script a user recently reached out to me with this bug. And I just implemented a fix. When considering creating a PR I find that a fix is already proposed. We solve it in a similar manner... looping through all the possible TLD's until one is found to work. Only I did it in-line whereas this PR creates a new function -- which is probably more elegant. Moral is that I should have looked for an existing issue before trying to fix it myself.

All that said. I have found additional problems that need addressed. Specifically when we grep for "www.example.com" we (I) forgot that dot is a wildcard. The correct way to grep would be to first escape the dots.
search_domain="$(echo "$top_domain" | sed 's/\./\\./g')"
and then use $search_domain where we grep or sed.

Also during testing with a new domain I found that the domain name is not tightly bound by
<td> and </td>
tags. So...
_contains "$line" "<td>$full_domain</td>"
fails. And the fix is...
_contains "$line" "<td>$search_domain.*</td>"
which allows for text immediately following the domain to be present (which it is when first created and before FreeDNS has validated that the DNS is set correctly).

Okay, now what. I could create a PR with my own version of a fix, and @Neilpang can chose between them, or @frebib can address the questions raised by @Neilpang and my comments above.

Thoughts?

@dkerr64
Copy link
Contributor

dkerr64 commented May 19, 2019

For reference, my version of the fix is here... dkerr64@465189c

David

@frebib
Copy link
Author

frebib commented May 19, 2019

It's been a long time since I looked at this, and I don't use freedns any more. Please feel free to adapt/ignore this PR and open another one. I don't have the time or bother to take this any further. I'm glad someone else is picking this up again :)

@frebib frebib closed this May 19, 2019
@dkerr64
Copy link
Contributor

dkerr64 commented May 20, 2019

Thank you @frebib. I will do some cleanup on my version of the fix and then submit a new pull request. This is a pretty serious bug so I want to get that done soon -- I just wish it had come to my attention sooner.
Thanks, David

@dkerr64
Copy link
Contributor

dkerr64 commented May 23, 2019

I have made the cleanups that I wanted to do. If anyone is monitoring this can you please test the version here... https://raw.githubusercontent.com/dkerr64/acme.sh/FreeDNS/dnsapi/dns_freedns.sh
Thank you.

@felixgonsug
Copy link

I tested and it works after adding the flag "--dnssleep 120". FreeDNS has a propgation time of 60 seconds, so the default of 20 is to short. In previous versions the default was 120, but that problem is not related to this PR. The PR is working correctly, for multi TLD and for single TLD. Sorry for the dealy of the response and thanks for fixing this.

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.

6 participants