Repository: favonia/cloudflare-ddns (go)

Issue:
Quote everything from Cloudflare
DNS record IDs are currently not quoted before printing, but they can potentially contain ASCII control characters.

Pull request:
fix(setter): quote DNS record IDs to prevent injection attacks
Closes #496.

Patch:
diff --git a/internal/setter/setter.go b/internal/setter/setter.go
index 7ac74d0..aa72260 100644
--- a/internal/setter/setter.go
+++ b/internal/setter/setter.go
@@ -109,7 +109,7 @@ func (s *setter) Set(ctx context.Context, ppfmt pp.PP,
 			if ok := s.Handle.UpdateRecord(ctx, ppfmt, domain, ipnet, id, ip); !ok {
 				// If the updating fails, we will delete it.
 				if s.Handle.DeleteRecord(ctx, ppfmt, domain, ipnet, id) {
-					ppfmt.Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)",
+					ppfmt.Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)",
 						recordType, domainDescription, id)
 
 					// Only when the deletion succeeds, we decrease the counter of remaining stale records.
@@ -122,7 +122,7 @@ func (s *setter) Set(ctx context.Context, ppfmt pp.PP,
 
 			// If the updating succeeds, we can move on to the next stage!
 			ppfmt.Noticef(pp.EmojiUpdateRecord,
-				"Updated a stale %s record of %q (ID: %s)", recordType, domainDescription, id)
+				"Updated a stale %s record of %q (ID: %q)", recordType, domainDescription, id)
 			monitorMessage = fmt.Sprintf("Set %s %s to %s", recordType, domainDescription, ip.String())
 
 			// Now it's up to date! Note that matchedIDs must be empty; otherwise uptodate would have been true.
@@ -141,7 +141,7 @@ func (s *setter) Set(ctx context.Context, ppfmt pp.PP,
 	if !uptodate {
 		if id, ok := s.Handle.CreateRecord(ctx, ppfmt,
 			domain, ipnet, ip, ttl, proxied); ok {
-			ppfmt.Noticef(pp.EmojiCreateRecord, "Added a new %s record of %q (ID: %s)", recordType, domainDescription, id)
+			ppfmt.Noticef(pp.EmojiCreateRecord, "Added a new %s record of %q (ID: %q)", recordType, domainDescription, id)
 			monitorMessage = fmt.Sprintf("Set %s %s to %s", recordType, domainDescription, ip.String())
 
 			// Now it's up to date! matchedIDs and unmatchedIDsToUpdate must both be empty at this point
@@ -152,7 +152,7 @@ func (s *setter) Set(ctx context.Context, ppfmt pp.PP,
 	// Now, we should try to delete all remaining stale records.
 	for _, id := range unmatchedIDsToUpdate {
 		if s.Handle.DeleteRecord(ctx, ppfmt, domain, ipnet, id) {
-			ppfmt.Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", recordType, domainDescription, id)
+			ppfmt.Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", recordType, domainDescription, id)
 			numUndeletedUnmatched--
 		}
 	}
@@ -161,7 +161,7 @@ func (s *setter) Set(ctx context.Context, ppfmt pp.PP,
 	// This has lower priority than deleting the stale records.
 	for _, id := range duplicateMatchedIDs {
 		if s.Handle.DeleteRecord(ctx, ppfmt, domain, ipnet, id) {
-			ppfmt.Noticef(pp.EmojiDeleteRecord, "Deleted a duplicate %s record of %q (ID: %s)",
+			ppfmt.Noticef(pp.EmojiDeleteRecord, "Deleted a duplicate %s record of %q (ID: %q)",
 				recordType, domainDescription, id)
 		}
 	}
@@ -207,7 +207,7 @@ func (s *setter) Clear(ctx context.Context, ppfmt pp.PP, domain domain.Domain, i
 			continue
 		}
 
-		ppfmt.Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", recordType, domainDescription, id)
+		ppfmt.Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", recordType, domainDescription, id)
 	}
 	if !allOk {
 		ppfmt.Errorf(pp.EmojiError,


Test patch:
diff --git a/internal/setter/setter_test.go b/internal/setter/setter_test.go
index 841c338..6a9e605 100644
--- a/internal/setter/setter_test.go
+++ b/internal/setter/setter_test.go
@@ -48,7 +48,7 @@ func TestSet(t *testing.T) {
 			1,
 			false,
 			func(m *mocks.MockPP) {
-				m.EXPECT().Noticef(pp.EmojiCreateRecord, "Added a new %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1)
+				m.EXPECT().Noticef(pp.EmojiCreateRecord, "Added a new %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1)
 			},
 			func(ctx context.Context, ppfmt pp.PP, m *mocks.MockHandle) {
 				gomock.InOrder(
@@ -65,7 +65,7 @@ func TestSet(t *testing.T) {
 			false,
 			func(m *mocks.MockPP) {
 				m.EXPECT().Noticef(pp.EmojiUpdateRecord,
-					"Updated a stale %s record of %q (ID: %s)",
+					"Updated a stale %s record of %q (ID: %q)",
 					"AAAA",
 					"sub.test.org",
 					record1,
@@ -86,8 +86,8 @@ func TestSet(t *testing.T) {
 			false,
 			func(m *mocks.MockPP) {
 				gomock.InOrder(
-					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1), //nolint:lll
-					m.EXPECT().Noticef(pp.EmojiCreateRecord, "Added a new %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2),
+					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1), //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiCreateRecord, "Added a new %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2),
 				)
 			},
 			func(ctx context.Context, ppfmt pp.PP, m *mocks.MockHandle) {
@@ -121,7 +121,7 @@ func TestSet(t *testing.T) {
 			func(m *mocks.MockPP) {
 				m.EXPECT().Noticef(
 					pp.EmojiDeleteRecord,
-					"Deleted a duplicate %s record of %q (ID: %s)",
+					"Deleted a duplicate %s record of %q (ID: %q)",
 					"AAAA",
 					"sub.test.org",
 					record2,
@@ -158,12 +158,12 @@ func TestSet(t *testing.T) {
 				gomock.InOrder(
 					m.EXPECT().Noticef(
 						pp.EmojiUpdateRecord,
-						"Updated a stale %s record of %q (ID: %s)",
+						"Updated a stale %s record of %q (ID: %q)",
 						"AAAA",
 						"sub.test.org",
 						record1,
 					),
-					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2), //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2), //nolint:lll
 				)
 			},
 			func(ctx context.Context, ppfmt pp.PP, m *mocks.MockHandle) {
@@ -182,10 +182,10 @@ func TestSet(t *testing.T) {
 			false,
 			func(m *mocks.MockPP) {
 				gomock.InOrder(
-					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1), //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1), //nolint:lll
 					m.EXPECT().Noticef(
 						pp.EmojiUpdateRecord,
-						"Updated a stale %s record of %q (ID: %s)",
+						"Updated a stale %s record of %q (ID: %q)",
 						"AAAA",
 						"sub.test.org",
 						record2,
@@ -209,9 +209,9 @@ func TestSet(t *testing.T) {
 			false,
 			func(m *mocks.MockPP) {
 				gomock.InOrder(
-					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1), //nolint:lll
-					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2), //nolint:lll
-					m.EXPECT().Noticef(pp.EmojiCreateRecord, "Added a new %s record of %q (ID: %s)", "AAAA", "sub.test.org", record3),
+					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1), //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2), //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiCreateRecord, "Added a new %s record of %q (ID: %q)", "AAAA", "sub.test.org", record3),
 				)
 			},
 			func(ctx context.Context, ppfmt pp.PP, m *mocks.MockHandle) {
@@ -234,8 +234,8 @@ func TestSet(t *testing.T) {
 			false,
 			func(m *mocks.MockPP) {
 				gomock.InOrder(
-					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2),                      //nolint:lll
-					m.EXPECT().Noticef(pp.EmojiCreateRecord, "Added a new %s record of %q (ID: %s)", "AAAA", "sub.test.org", record3),                          //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2),                      //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiCreateRecord, "Added a new %s record of %q (ID: %q)", "AAAA", "sub.test.org", record3),                          //nolint:lll
 					m.EXPECT().Errorf(pp.EmojiError, "Failed to complete updating of %s records of %q; records might be inconsistent", "AAAA", "sub.test.org"), //nolint:lll
 				)
 			},
@@ -259,8 +259,8 @@ func TestSet(t *testing.T) {
 			false,
 			func(m *mocks.MockPP) {
 				gomock.InOrder(
-					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1),                      //nolint:lll
-					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2),                      //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1),                      //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2),                      //nolint:lll
 					m.EXPECT().Errorf(pp.EmojiError, "Failed to complete updating of %s records of %q; records might be inconsistent", "AAAA", "sub.test.org"), //nolint:lll
 				)
 			},
@@ -351,7 +351,7 @@ func TestClear(t *testing.T) {
 			true,
 			`Deleted AAAA sub.test.org`,
 			func(m *mocks.MockPP) {
-				m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1) //nolint:lll
+				m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1) //nolint:lll
 			},
 			func(ctx context.Context, ppfmt pp.PP, m *mocks.MockHandle) {
 				gomock.InOrder(
@@ -378,8 +378,8 @@ func TestClear(t *testing.T) {
 			`Deleted AAAA sub.test.org`,
 			func(m *mocks.MockPP) {
 				gomock.InOrder(
-					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1), //nolint:lll
-					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2), //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1), //nolint:lll
+					m.EXPECT().Noticef(pp.EmojiDeleteRecord, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2), //nolint:lll
 				)
 			},
 			func(ctx context.Context, ppfmt pp.PP, m *mocks.MockHandle) {