Repository: favonia/cloudflare-ddns
Pull request #58 (2021-08-05 02:25:55)

fix(api): full coverage of fqdn

codecov[bot]: # [Codecov](https://codecov.io/gh/favonia/cloudflare-ddns/pull/58?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia) Report
> Merging [#58](https://codecov.io/gh/favonia/cloudflare-ddns/pull/58?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia) (c2b340a) into [main](https://codecov.io/gh/favonia/cloudflare-ddns/commit/526b4abfa5c8d84605afc83b59d519289009287b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia) (526b4ab) will **increase** coverage by `13.01%`.
> The diff coverage is `94.11%`.

[![Impacted file tree graph](https://codecov.io/gh/favonia/cloudflare-ddns/pull/58/graphs/tree.svg?width=650&height=150&src=pr&token=90Cwhy7OkI&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia)](https://codecov.io/gh/favonia/cloudflare-ddns/pull/58?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia)

```diff
@@             Coverage Diff             @@
##             main      #58       +/-   ##
===========================================
+ Coverage   28.46%   41.48%   +13.01%     
===========================================
  Files           7        7               
  Lines         274      270        -4     
===========================================
+ Hits           78      112       +34     
+ Misses        196      158       -38     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `41.48% <94.11%> (+13.01%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/favonia/cloudflare-ddns/pull/58?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia) | Coverage Δ | |
|---|---|---|
| [internal/api/cloudflare.go](https://codecov.io/gh/favonia/cloudflare-ddns/pull/58/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia#diff-aW50ZXJuYWwvYXBpL2Nsb3VkZmxhcmUuZ28=) | `0.00% <0.00%> (ø)` | |
| [internal/api/fqdn.go](https://codecov.io/gh/favonia/cloudflare-ddns/pull/58/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia#diff-aW50ZXJuYWwvYXBpL2ZxZG4uZ28=) | `100.00% <100.00%> (+100.00%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/favonia/cloudflare-ddns/pull/58?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/favonia/cloudflare-ddns/pull/58?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia). Last update [526b4ab...c2b340a](https://codecov.io/gh/favonia/cloudflare-ddns/pull/58?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=favonia).


Patch:
diff --git a/README.markdown b/README.markdown
index 2dcc92e..aa46cd7 100644
--- a/README.markdown
+++ b/README.markdown
@@ -300,7 +300,7 @@ In most cases, `CF_ACCOUNT_ID` is not needed.
 > <details>
 > <summary>📍 At least one of <code>DOMAINS</code> and <code>IP4/6_DOMAINS</code> must be non-empty.</summary>
 >
-> At least one domain should be listed in `DOMAINS`, `IP4_DOMAINS`, or `IP6_DOMAINS`, for otherwise this updater has nothing to do. It is fine to list the same domain in both `IP4_DOMAINS` and `IP6_DOMAINS`, which is equivalent to listing it in `DOMAINS`. This updater supports internationalized domain names; [Punycode](https://en.wikipedia.org/wiki/Punycode) will be decoded and processed using [x/net/idna](https://pkg.go.dev/golang.org/x/net/idna).
+> At least one domain should be listed in `DOMAINS`, `IP4_DOMAINS`, or `IP6_DOMAINS`, for otherwise this updater has nothing to do. It is fine to list the same domain in both `IP4_DOMAINS` and `IP6_DOMAINS`, which is equivalent to listing it in `DOMAINS`. This updater supports internationalized domain names using the non-transitional IDNA2008 processing.
 > </details>
 
 </details>
diff --git a/internal/api/cloudflare.go b/internal/api/cloudflare.go
index 23e531b..cde15bc 100644
--- a/internal/api/cloudflare.go
+++ b/internal/api/cloudflare.go
@@ -103,7 +103,7 @@ func (h *CloudflareHandle) ZoneOfDomain(ctx context.Context, indent pp.Indent, d
 
 zoneSearch:
 	for s := NewFQDNSplitter(domain); s.IsValid(); s.Next() {
-		zoneName := s.AfterPeriodString()
+		zoneName := s.Suffix()
 		zones, ok := h.ActiveZones(ctx, indent, zoneName)
 		if !ok {
 			return "", false
diff --git a/internal/api/fqdn.go b/internal/api/fqdn.go
index 90248f5..34b5825 100644
--- a/internal/api/fqdn.go
+++ b/internal/api/fqdn.go
@@ -7,36 +7,49 @@ import (
 	"golang.org/x/net/idna"
 )
 
-type FQDN string
-
-func (f FQDN) String() string {
-	return string(f)
-}
+//nolint: gochecknoglobals
+// profile does C2 in UTS#46 with all checks on + removing leading dots.
+// This is the main conversion profile in use.
+var profile = idna.New(
+	idna.MapForLookup(),
+	idna.BidiRule(),
+	// idna.Transitional(false), // https://go-review.googlesource.com/c/text/+/317729/
+	idna.RemoveLeadingDots(true),
+)
 
-func NewFQDN(domain string) FQDN {
-	// Remove the final period for consistency
-	domain = strings.TrimSuffix(domain, ".")
+// FQDN is a fully qualified domain in its ASCII or Unicode (when unambiguous) form.
+type FQDN string
 
-	// Process Punycode
-	normalized, err := idna.ToUnicode(domain)
-	if err != nil {
-		// Something appears to be wrong, but we can live with it
-		return FQDN(domain)
+// safelyToUnicode takes an ASCII form and returns the Unicode form
+// when the round trip gives the same ASCII form back without errors.
+// Otherwise, the input ASCII form is returned.
+func safelyToUnicode(ascii string) string {
+	unicode, errToA := profile.ToUnicode(ascii)
+	roundTrip, errToU := profile.ToASCII(unicode)
+	if errToA != nil || errToU != nil || roundTrip != ascii {
+		return ascii
 	}
 
-	return FQDN(normalized)
+	return unicode
 }
 
-type FQDNSlice []FQDN
+func (f FQDN) String() string { return string(f) }
+
+// NewFQDN normalizes a domain to its ASCII form and then stores
+// the normalized domain in its Unicode form when the round trip
+// gives back the same ASCII form without errors. Otherwise,
+// the ASCII form (possibly using Punycode) is stored to avoid ambiguity.
+func NewFQDN(domain string) (FQDN, error) {
+	normalized, err := profile.ToASCII(domain)
 
-func (x FQDNSlice) Len() int           { return len(x) }
-func (x FQDNSlice) Less(i, j int) bool { return x[i] < x[j] }
-func (x FQDNSlice) Swap(i, j int)      { x[i], x[j] = x[j], x[i] }
+	// Remove the final dot for consistency
+	normalized = strings.TrimSuffix(normalized, ".")
 
-func SortFQDNs(s []FQDN) {
-	sort.Sort(FQDNSlice(s))
+	return FQDN(safelyToUnicode(normalized)), err
 }
 
+func SortFQDNs(s []FQDN) { sort.Slice(s, func(i, j int) bool { return s[i] < s[j] }) }
+
 type FQDNSplitter struct {
 	domain    string
 	cursor    int
@@ -51,15 +64,13 @@ func NewFQDNSplitter(domain FQDN) *FQDNSplitter {
 	}
 }
 
-func (s *FQDNSplitter) IsValid() bool {
-	return !s.exhausted
-}
-
+func (s *FQDNSplitter) IsValid() bool  { return !s.exhausted }
+func (s *FQDNSplitter) Suffix() string { return s.domain[s.cursor:] }
 func (s *FQDNSplitter) Next() {
 	if s.cursor == len(s.domain) {
 		s.exhausted = true
 	} else {
-		shift := strings.IndexRune(s.AfterPeriodString(), '.')
+		shift := strings.IndexRune(s.Suffix(), '.')
 		if shift == -1 {
 			s.cursor = len(s.domain)
 		} else {
@@ -67,7 +78,3 @@ func (s *FQDNSplitter) Next() {
 		}
 	}
 }
-
-func (s *FQDNSplitter) AfterPeriodString() string {
-	return s.domain[s.cursor:]
-}
diff --git a/internal/config/env.go b/internal/config/env.go
index 0aa0811..91fa899 100644
--- a/internal/config/env.go
+++ b/internal/config/env.go
@@ -94,7 +94,7 @@ func ReadNonnegInt(quiet quiet.Quiet, indent pp.Indent, key string, field *int)
 
 // ReadDomains reads an environment variable as a comma-separated list of domains.
 // Spaces are trimed.
-func ReadDomains(_ quiet.Quiet, _ pp.Indent, key string, field *[]api.FQDN) bool {
+func ReadDomains(_ quiet.Quiet, indent pp.Indent, key string, field *[]api.FQDN) bool {
 	rawList := strings.Split(Getenv(key), ",")
 
 	*field = make([]api.FQDN, 0, len(rawList))
@@ -104,7 +104,12 @@ func ReadDomains(_ quiet.Quiet, _ pp.Indent, key string, field *[]api.FQDN) bool
 			continue
 		}
 
-		*field = append(*field, api.NewFQDN(item))
+		item, err := api.NewFQDN(item)
+		if err != nil {
+			pp.Printf(indent, pp.EmojiUserError, "Domain %q was added but it is ill-formed: %v", item, err)
+		}
+
+		*field = append(*field, item)
 	}
 
 	return true


Test patch:
diff --git a/internal/api/fqdn_test.go b/internal/api/fqdn_test.go
new file mode 100644
index 0000000..850fdbf
--- /dev/null
+++ b/internal/api/fqdn_test.go
@@ -0,0 +1,127 @@
+package api_test
+
+import (
+	"sort"
+	"testing"
+	"testing/quick"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/favonia/cloudflare-ddns-go/internal/api"
+)
+
+func TestFQDNString(t *testing.T) {
+	t.Parallel()
+
+	require.NoError(t, quick.Check(
+		func(s string) bool {
+			return api.FQDN(s).String() == s
+		},
+		nil,
+	))
+}
+
+func TestNewFQDN(t *testing.T) {
+	t.Parallel()
+	for _, tc := range [...]struct {
+		input     string
+		expected  api.FQDN
+		ok        bool
+		errString string
+	}{
+		// The following examples were reproduced from https://unicode.org/cldr/utility/idna.jsp
+		{"fass.de", "fass.de", true, ""},
+		{"faß.de", "faß.de", true, ""},
+		{"fäß.de", "fäß.de", true, ""},
+		{"xn--fa-hia.de", "faß.de", true, ""},
+		{"₹.com", "₹.com", true, ""},
+		{"𑀓.com", "𑀓.com", true, ""},
+		{"\u0080.com", "xn--a.com", false, "idna: disallowed rune U+0080"},
+		{"xn--a.com", "xn--a.com", false, "idna: invalid label \"\\u0080\""},
+		{"a\u200Cb", "xn--ab-j1t", false, "idna: invalid label \"a\\u200cb\""},
+		{"xn--ab-j1t", "xn--ab-j1t", false, "idna: invalid label \"a\\u200cb\""},
+		{"öbb.at", "öbb.at", true, ""},
+		{"ÖBB.at", "öbb.at", true, ""},
+		{"ÖBB.at", "öbb.at", true, ""},
+		{"ȡog.de", "ȡog.de", true, ""},
+		{"☕.de", "☕.de", true, ""},
+		{"I♥NY.de", "i♥ny.de", true, ""},
+		{"ＡＢＣ・日本.co.jp", "abc・日本.co.jp", true, ""},
+		{"日本｡co｡jp", "日本.co.jp", true, ""},
+		{"日本｡co．jp", "日本.co.jp", true, ""},
+		{"日本⒈co．jp", "xn--co-wuw5954azlb.jp", false, "idna: disallowed rune U+2488"},
+		{"x\u0327\u0301.de", "x̧́.de", true, ""},
+		{"x\u0301\u0327.de", "x̧́.de", true, ""},
+		{"σόλος.gr", "σόλος.gr", true, ""},
+		{"Σόλος.gr", "σόλος.gr", true, ""},
+		{"ΣΌΛΟΣ.grﻋﺮﺑﻲ.de", "xn--wxaikc6b.xn--gr-gtd9a1b0g.de", false, "idna: invalid label \"σόλοσ.grعربي.de\""},
+		{"عربي.de", "عربي.de", true, ""},
+		{"نامهای.de", "نامهای.de", true, ""},
+		{"نامه\u200Cای.de", "نامه\u200cای.de", true, ""},
+		// some other test cases
+		{"xn--a.xn--a.xn--a.com", "xn--a.xn--a.xn--a.com", false, "idna: invalid label \"\\u0080\""},
+		{"a.com...｡", "a.com...", true, ""},
+		{"..｡..a.com", "a.com", true, ""},
+		{"O\u0308", "\u00F6", true, ""},
+	} {
+		tc := tc
+		t.Run(tc.input, func(t *testing.T) {
+			t.Parallel()
+			normalized, err := api.NewFQDN(tc.input)
+			require.Equal(t, tc.expected, normalized)
+			if tc.ok {
+				require.NoError(t, err)
+			} else {
+				require.EqualError(t, err, tc.errString)
+			}
+		})
+	}
+}
+
+func TestSortFQDNs(t *testing.T) {
+	t.Parallel()
+
+	require.NoError(t, quick.Check(
+		func(s []api.FQDN) bool {
+			copied := make([]api.FQDN, len(s))
+			copy(copied, s)
+			api.SortFQDNs(s)
+			switch {
+			case !assert.ElementsMatch(t, copied, s):
+				return false
+			case !sort.SliceIsSorted(s, func(i, j int) bool { return s[i] < s[j] }):
+				return false
+			default:
+				return true
+			}
+		},
+		nil,
+	))
+}
+
+func TestSortFQDNSplitter(t *testing.T) {
+	t.Parallel()
+	type ss = []string
+	for _, tc := range [...]struct {
+		input    string
+		expected []string
+	}{
+		// The following examples were adapted from https://unicode.org/cldr/utility/idna.jsp
+		{"...", ss{"...", "..", ".", ""}},
+		{"aaa...", ss{"aaa...", "..", ".", ""}},
+		{".aaa..", ss{".aaa..", "aaa..", ".", ""}},
+		{"..aaa.", ss{"..aaa.", ".aaa.", "aaa.", ""}},
+		{"...aaa", ss{"...aaa", "..aaa", ".aaa", "aaa", ""}},
+	} {
+		tc := tc
+		t.Run(tc.input, func(t *testing.T) {
+			t.Parallel()
+			var ss []string
+			for s := api.NewFQDNSplitter(api.FQDN(tc.input)); s.IsValid(); s.Next() {
+				ss = append(ss, s.Suffix())
+			}
+			require.Equal(t, tc.expected, ss)
+		})
+	}
+}