Repository: cloudflare/cloudflare-go (go)

Issue:
ListDNSRecords overwrites proxied value when paginated
### Confirmation

- [X] My issue isn't already found on the issue tracker.
- [X] I have replicated my issue using the latest version of the library and it is still present.

### cloudflare-go version

0.61.0

### Go environment

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/vscode/.cache/go-build"
GOENV="/home/vscode/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/workspaces/dangling-dns-checker/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2684787559=/tmp/go-build -gno-record-gcc-switches"

### Expected output

When listing the DNS records for a zone that has sufficient records as to require pagination, the Proxied field on all the DNS records returned match that shown on the web portal.

### Actual output

As subsequent pages are fetched from the API, the values for the Proxied field from the latest page overwrites the values in the records from the previous pages.

For example, say you have 8 records and a page size of 3, looking at just the Proxied field the records might be:
1. true
2. false
3. true
4. true
5. true
6. false
7. false
8. false

The API returns the correct values, however, the returned values for ListDNSRecords would be
1. false
2. false
3. false
4. false
5. false
6. false
7. false
8. false

Stepping through the code, as each page is appended to the `records []DNSRecord` slice in `dns.go`, it overwrites previous pages values:
```
Page 1         Page 2      Page 3
1. true   <-  4. true  <- 7. false
2. false  <-  5. true  <- 8. false
3. true  <-   6. false
```
Records 1 & 4 get the value from record 7 (false)
Records 2 & 5 get the value from record 8 (false)
Record 3 gets the value from record 6 (false)

I hope that is a clear enough explanation, but happy to explain further if not.

### Code demonstrating the issue

token := "<api-token>"
zone := "<zone-name>" // Use zone with >100 records
api, err := cloudflare.NewWithAPIToken(token)
if err != nil {
  log.Fatalf("failed to log into cloudflare: %v\n", err)
}
id, _ := api.ZoneIDByName(zone)
ctx := context.Background()
resouces := cloudflare.ZoneIdentifier(id)
opts := cloudflare.ListDNSRecordsParams{}
records, _, _ := api.ListDNSRecords(ctx, resouces, opts)

for idx, record := range records {
  if record.Proxied != nil {
    log.Printf("%d: %v %s\n", idx, *record.Proxied, record.Name)
  }
}


### Steps to reproduce

1. Have a DNS zone with over 100 record with various records set to Proxied/Not Proxied
2. Setup api client and call ListDNSRecords with default options
3. Observe the returned result's `Proxied` field is repeating pattern as described in Actual output

### References

I have forked the repo and have created a potential fix:
https://github.com/MrMortbury/cloudflare-go/tree/master

I also updated the TestListDNSRecordsPagination in `dns_test.go` to test for this situation and validate the records returned by the client matches that returned by the HTTP requests:
https://github.com/MrMortbury/cloudflare-go/blob/master/dns_test.go#L348

Pull request:
Use new DNSListResponse object when paginating
Removes the reuse of the DNSListResponse object when collating multiple pages of API requests as this was reusing the pointers used for the Proxied field. 

## Description
Fixes #1221 

Move the declaration of the DNSListResponse variable to within the pagination loop to for go to create a new variable on each loop and stop the Proxied pointer of DNS records from being reused. A new variable of lastResultInfo was added outside the loop to allow the return of the ResultsInfo from the last page of the requests.

## Has your change been tested?

Updated the TestListDNSRecordsPagination test and associated fixtures to replicate the issue describe in the bug. Test now pulls 2 pages and a total of 5 test records configured to replicate #1221. Test failed before the change and passed afterwards.
All other DNS tests have been run and pass without issue

## Types of changes

What sort of change does your code introduce/modify?

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Checklist:

- [x] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.
- [x] This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs.

[1]: https://help.github.com/articles/closing-issues-using-keywords/


Patch:
diff --git a/.changelog/1222.txt b/.changelog/1222.txt
new file mode 100644
index 000000000..5d9a668a0
--- /dev/null
+++ b/.changelog/1222.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+dns: dont reuse DNSListResponse when using pagination to avoid Proxied pointer overwrite
+```
\ No newline at end of file
diff --git a/dns.go b/dns.go
index aa8cc2284..d7bd308d7 100644
--- a/dns.go
+++ b/dns.go
@@ -172,7 +172,7 @@ func (api *API) ListDNSRecords(ctx context.Context, rc *ResourceContainer, param
 	}
 
 	var records []DNSRecord
-	var listResponse DNSListResponse
+	var lastResultInfo ResultInfo
 
 	for {
 		uri := buildURI(fmt.Sprintf("/zones/%s/dns_records", rc.Identifier), params)
@@ -180,17 +180,19 @@ func (api *API) ListDNSRecords(ctx context.Context, rc *ResourceContainer, param
 		if err != nil {
 			return []DNSRecord{}, &ResultInfo{}, err
 		}
+		var listResponse DNSListResponse
 		err = json.Unmarshal(res, &listResponse)
 		if err != nil {
 			return []DNSRecord{}, &ResultInfo{}, fmt.Errorf("%s: %w", errUnmarshalError, err)
 		}
 		records = append(records, listResponse.Result...)
+		lastResultInfo = listResponse.ResultInfo
 		params.ResultInfo = listResponse.ResultInfo.Next()
 		if params.ResultInfo.Done() || !autoPaginate {
 			break
 		}
 	}
-	return records, &listResponse.ResultInfo, nil
+	return records, &lastResultInfo, nil
 }
 
 // ErrMissingDNSRecordID is for when DNS record ID is needed but not given.


Test patch:
diff --git a/dns_test.go b/dns_test.go
index 41b7c3335..46871f162 100644
--- a/dns_test.go
+++ b/dns_test.go
@@ -347,7 +347,7 @@ func TestListDNSRecordsSearch(t *testing.T) {
 
 func TestListDNSRecordsPagination(t *testing.T) {
 	// change listDNSRecordsDefaultPageSize value to 1 to force pagination
-	listDNSRecordsDefaultPageSize = 1
+	listDNSRecordsDefaultPageSize = 3
 
 	setup()
 	defer teardown()
@@ -378,7 +378,45 @@ func TestListDNSRecordsPagination(t *testing.T) {
 	require.NoError(t, err)
 	assert.True(t, page1Called)
 	assert.True(t, page2Called)
-	assert.Len(t, actual, 2)
+	assert.Len(t, actual, 5)
+
+	type ls struct {
+		Results []map[string]interface{} `json:"result"`
+	}
+
+	expectedRecords := make(map[string]map[string]interface{})
+
+	response1 := loadFixture("dns", "list_page_1")
+	var fixtureDataPage1 ls
+	err = json.Unmarshal([]byte(response1), &fixtureDataPage1)
+	assert.NoError(t, err)
+	for _, record := range fixtureDataPage1.Results {
+		expectedRecords[record["id"].(string)] = record
+	}
+
+	response2 := loadFixture("dns", "list_page_2")
+	var fixtureDataPage2 ls
+	err = json.Unmarshal([]byte(response2), &fixtureDataPage2)
+	assert.NoError(t, err)
+	for _, record := range fixtureDataPage2.Results {
+		expectedRecords[record["id"].(string)] = record
+	}
+
+	for _, actualRecord := range actual {
+		expected, exist := expectedRecords[actualRecord.ID]
+		assert.True(t, exist, "DNS record doesn't exist in fixtures")
+		assert.Equal(t, expected["type"].(string), actualRecord.Type)
+		assert.Equal(t, expected["name"].(string), actualRecord.Name)
+		assert.Equal(t, expected["content"].(string), actualRecord.Content)
+		assert.Equal(t, expected["proxiable"].(bool), actualRecord.Proxiable)
+		assert.Equal(t, expected["proxied"].(bool), *actualRecord.Proxied)
+		assert.Equal(t, int(expected["ttl"].(float64)), actualRecord.TTL)
+		assert.Equal(t, expected["locked"].(bool), actualRecord.Locked)
+		assert.Equal(t, expected["zone_id"].(string), actualRecord.ZoneID)
+		assert.Equal(t, expected["zone_name"].(string), actualRecord.ZoneName)
+		assert.Equal(t, expected["data"], actualRecord.Data)
+		assert.Equal(t, expected["meta"], actualRecord.Meta)
+	}
 }
 
 func TestGetDNSRecord(t *testing.T) {
diff --git a/testdata/fixtures/dns/list_page_1.json b/testdata/fixtures/dns/list_page_1.json
index 15acc5533..505577398 100644
--- a/testdata/fixtures/dns/list_page_1.json
+++ b/testdata/fixtures/dns/list_page_1.json
@@ -25,13 +25,59 @@
                 "tag1",
                 "tag2extended"
             ]
+        },
+        {
+            "id": "7eb0a9821aec4b1395bd8cc03d88c17d",
+            "type": "A",
+            "name": "sub1.example.com",
+            "content": "198.51.100.5",
+            "proxiable": true,
+            "proxied": false,
+            "ttl": 120,
+            "locked": false,
+            "zone_id": "d56084adb405e0b7e32c52321bf07be6",
+            "zone_name": "example.com",
+            "created_on": "2014-01-01T05:20:00Z",
+            "modified_on": "2014-01-01T05:20:00Z",
+            "data": {},
+            "meta": {
+                "auto_added": true,
+                "source": "primary"
+            },
+            "tags": [
+                "tag1",
+                "tag2extended"
+            ]
+        },
+        {
+            "id": "4c2c40857e334a2d903dd28f65a99682",
+            "type": "A",
+            "name": "sub2.example.com",
+            "content": "198.51.100.6",
+            "proxiable": true,
+            "proxied": true,
+            "ttl": 120,
+            "locked": false,
+            "zone_id": "d56084adb405e0b7e32c52321bf07be6",
+            "zone_name": "example.com",
+            "created_on": "2014-01-01T05:20:00Z",
+            "modified_on": "2014-01-01T05:20:00Z",
+            "data": {},
+            "meta": {
+                "auto_added": true,
+                "source": "primary"
+            },
+            "tags": [
+                "tag1",
+                "tag2extended"
+            ]
         }
     ],
     "result_info": {
-        "count": 1,
+        "count": 3,
         "page": 1,
-        "per_page": 1,
-        "total_count": 2,
+        "per_page": 3,
+        "total_count": 5,
         "total_pages": 2
     }
 }
diff --git a/testdata/fixtures/dns/list_page_2.json b/testdata/fixtures/dns/list_page_2.json
index b7836c01c..bb3d8f727 100644
--- a/testdata/fixtures/dns/list_page_2.json
+++ b/testdata/fixtures/dns/list_page_2.json
@@ -4,12 +4,35 @@
     "messages": [],
     "result": [
         {
-            "id": "372e67954025e0ba6aaa6d586b9e0b59",
+            "id": "97e1dc2d19204b448b6ee04724f005ba",
             "type": "A",
-            "name": "www.example.com",
-            "content": "198.51.100.4",
+            "name": "sub3.example.com",
+            "content": "198.51.100.7",
             "proxiable": true,
-            "proxied": true,
+            "proxied": false,
+            "ttl": 120,
+            "locked": false,
+            "zone_id": "d56084adb405e0b7e32c52321bf07be6",
+            "zone_name": "example.com",
+            "created_on": "2014-01-01T05:20:00Z",
+            "modified_on": "2014-01-01T05:20:00Z",
+            "data": {},
+            "meta": {
+                "auto_added": true,
+                "source": "primary"
+            },
+            "tags": [
+                "tag1",
+                "tag2extended"
+            ]
+        },
+        {
+            "id": "5bafaa7059d3480da9f6e2ecd8468c33",
+            "type": "A",
+            "name": "sub4.example.com",
+            "content": "198.51.100.8",
+            "proxiable": true,
+            "proxied": false,
             "ttl": 120,
             "locked": false,
             "zone_id": "d56084adb405e0b7e32c52321bf07be6",
@@ -28,10 +51,10 @@
         }
     ],
     "result_info": {
-        "count": 1,
+        "count": 2,
         "page": 2,
-        "per_page": 1,
-        "total_count": 2,
+        "per_page": 3,
+        "total_count": 5,
         "total_pages": 2
     }
 }