Repository: gin-gonic/gin
Pull request #3969 (2024-05-15 19:30:13)

fix(context): verify URL is Non-nil in initQueryCache()
## Verify URL is Non-nil in initQueryCache()

When using a test context (as returned by `gin.CreateTestContext(w)`) it is possible that not only the Request object is nil, but also the URL object may be nil.

Ran into this in the wild in unit tests. Stack trace:

```
1. --- FAIL: Test_ListOrgs (1.09s)
2. panic: runtime error: invalid memory address or nil pointer dereference [recovered]
3.     panic: runtime error: invalid memory address or nil pointer dereference
4. [signal SIGSEGV: segmentation violation code=0x1 addr=0x60 pc=0x6e51ce]
5.  
6. goroutine 124 [running]:
7. testing.tRunner.func1.2({0x206ad60, 0x3dd8c30})
8.     /opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1631 +0x24a
9. testing.tRunner.func1()
10.     /opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1634 +0x377
11. panic({0x206ad60?, 0x3dd8c30?})
12.     /opt/hostedtoolcache/go/1.22.2/x64/src/runtime/panic.go:770 +0x132
13. net/url.(*URL).Query(0x1000?)
14.     /opt/hostedtoolcache/go/1.22.2/x64/src/net/url/url.go:1122 +0xe
15. github.com/gin-gonic/gin.(*Context).initQueryCache(...)
16.     /home/runner/go/pkg/mod/github.com/gin-gonic/gin@v1.9.1/context.go:461
17. github.com/gin-gonic/gin.(*Context).GetQueryArray(0xc000126a00, {0x2392c3d, 0xf})
18.     /home/runner/go/pkg/mod/github.com/gin-gonic/gin@v1.9.1/context.go:471 +0x47
19. github.com/gin-gonic/gin.(*Context).GetQuery(...)
20.     /home/runner/go/pkg/mod/github.com/gin-gonic/gin@v1.9.1/context.go:445
21. github.com/gin-gonic/gin.(*Context).Query(0x2b5c688?, {0x2392c3d?, 0x2b6f428?})
22.     /home/runner/go/pkg/mod/github.com/gin-gonic/gin@v1.9.1/context.go:416 +0x18
23. github.com/HIDDEN_GH_ORG_NAME/api/internal/app/api/handlers/v1.OrganizationAPI.ListOrgsHandler({{0x2b5b880, 0xc0002b6150}, {{0x2b5c688, 0xc000cde910}, {0x2b6f428, 0xc000cde920}, {0x2b565b0, 0xc000cde930}, {0x2b666e0, 0xc000cde940}, ...}, ...}, ...)
24.     /home/runner/work/api/api/internal/app/api/handlers/v1/organizations.go:217 +0x2fe
25. github.com/HIDDEN_GH_ORG_NAME/api/internal/app/api/handlers/v1.Test_ListOrgs(0xc00101ab60)
26.     /home/runner/work/api/api/internal/app/api/handlers/v1/organizations_test.go:88 +0x95b
27. testing.tRunner(0xc00101ab60, 0x24f1320)
28.     /opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1689 +0xfb
29. created by testing.(*T).Run in goroutine 1
30.     /opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1742 +0x390
31. FAIL    github.com/HIDDEN_GH_ORG_NAME/api/internal/app/api/handlers/v1  10.886s
```


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/gin-gonic/gin/pull/3969?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 99.05%. Comparing base [(`3dc1cd6`)](https://app.codecov.io/gh/gin-gonic/gin/commit/3dc1cd6572b4e3a0cd170a15debe546c2c72294f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) to head [(`aae1263`)](https://app.codecov.io/gh/gin-gonic/gin/pull/3969?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic).
> Report is 58 commits behind head on master.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #3969      +/-   ##
==========================================
- Coverage   99.21%   99.05%   -0.16%     
==========================================
  Files          42       44       +2     
  Lines        3182     2751     -431     
==========================================
- Hits         3157     2725     -432     
+ Misses         17       15       -2     
- Partials        8       11       +3     
```

| [Flag](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | Coverage Δ | |
|---|---|---|
| [](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `?` | |
| [-tags "sonic avx"](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `99.04% <100.00%> (?)` | |
| [-tags go_json](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `99.04% <100.00%> (?)` | |
| [-tags nomsgpack](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `99.03% <100.00%> (?)` | |
| [go-1.18](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `?` | |
| [go-1.19](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `?` | |
| [go-1.20](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `?` | |
| [go-1.21](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `99.05% <100.00%> (-0.16%)` | :arrow_down: |
| [go-1.22](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `99.05% <100.00%> (?)` | |
| [macos-latest](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `99.05% <100.00%> (-0.16%)` | :arrow_down: |
| [ubuntu-latest](https://app.codecov.io/gh/gin-gonic/gin/pull/3969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | `99.05% <100.00%> (-0.16%)` | :arrow_down: |

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=gin-gonic#carryforward-flags-in-the-pull-request-comment) to find out more.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/gin-gonic/gin/pull/3969?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic).

adrianosela: Hey @RedCrazyGhost - not sure how to get the codecov test to pass here? I tested all branches of the added code and existing code in this function?
RedCrazyGhost: I checked the relevant information of codecov and found that someone had modified the configuration file of codecov two days ago, but this configuration file could not make the code review of codecov meet the normal expectations.

For previous reports on codecov code review not meeting expectations, please refer to: https://github.com/gin-gonic/gin/pull/3956

Now we mainly use manual code review, ignoring the failure of codecov 

Patch:
diff --git a/context.go b/context.go
index d2d5497..baa4b0f 100644
--- a/context.go
+++ b/context.go
@@ -475,7 +475,7 @@ func (c *Context) QueryArray(key string) (values []string) {
 
 func (c *Context) initQueryCache() {
 	if c.queryCache == nil {
-		if c.Request != nil {
+		if c.Request != nil && c.Request.URL != nil {
 			c.queryCache = c.Request.URL.Query()
 		} else {
 			c.queryCache = url.Values{}


Test patch:
diff --git a/context_test.go b/context_test.go
index b700389..8bbf270 100644
--- a/context_test.go
+++ b/context_test.go
@@ -423,6 +423,49 @@ func TestContextQuery(t *testing.T) {
 	assert.Empty(t, c.PostForm("foo"))
 }
 
+func TestContextInitQueryCache(t *testing.T) {
+	validURL, err := url.Parse("https://github.com/gin-gonic/gin/pull/3969?key=value&otherkey=othervalue")
+	assert.Nil(t, err)
+
+	tests := []struct {
+		testName           string
+		testContext        *Context
+		expectedQueryCache url.Values
+	}{
+		{
+			testName: "queryCache should remain unchanged if already not nil",
+			testContext: &Context{
+				queryCache: url.Values{"a": []string{"b"}},
+				Request:    &http.Request{URL: validURL}, // valid request for evidence that values weren't extracted
+			},
+			expectedQueryCache: url.Values{"a": []string{"b"}},
+		},
+		{
+			testName:           "queryCache should be empty when Request is nil",
+			testContext:        &Context{Request: nil}, // explicit nil for readability
+			expectedQueryCache: url.Values{},
+		},
+		{
+			testName:           "queryCache should be empty when Request.URL is nil",
+			testContext:        &Context{Request: &http.Request{URL: nil}}, // explicit nil for readability
+			expectedQueryCache: url.Values{},
+		},
+		{
+			testName:           "queryCache should be populated when it not yet populated and Request + Request.URL are non nil",
+			testContext:        &Context{Request: &http.Request{URL: validURL}}, // explicit nil for readability
+			expectedQueryCache: url.Values{"key": []string{"value"}, "otherkey": []string{"othervalue"}},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.testName, func(t *testing.T) {
+			test.testContext.initQueryCache()
+			assert.Equal(t, test.expectedQueryCache, test.testContext.queryCache)
+		})
+	}
+
+}
+
 func TestContextDefaultQueryOnEmptyRequest(t *testing.T) {
 	c, _ := CreateTestContext(httptest.NewRecorder()) // here c.Request == nil
 	assert.NotPanics(t, func() {