Skip to content

Commit

Permalink
fix(setter): quote DNS record IDs to prevent injection attacks (#502)
Browse files Browse the repository at this point in the history
  • Loading branch information
@favonia
favonia committed May 25, 2023
1 parent e87cc52 commit d978c68
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 25 deletions.
12 changes: 6 additions & 6 deletions internal/setter/setter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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--
}
}
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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,
Expand Down
38 changes: 19 additions & 19 deletions internal/setter/setter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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,
Expand All @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
Expand All @@ -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) {
Expand All @@ -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
)
},
Expand All @@ -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
)
},
Expand Down Expand Up @@ -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(
Expand All @@ -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) {
Expand Down

0 comments on commit d978c68

Please sign in to comment.