Repository: labstack/echo
Pull request #1463 (2019-12-30 20:18:20)

round-trip pvalues without exploding
When upgrading a codebase from echo v3 to v4, I bumped into this: calling `SetParamValues` doesn't protect against resizing`pvalues`, which causes subsequent calls to `Reset` to panic if it becomes smaller than `*e.maxParam`.

It's entirely possible this codebase is doing something it shouldn't, to end up in this state (specifically where `*e.maxParam` != `len(c.pvalues)`?) Regardless, I believe `SetParamValues` shouldn't allow callers to screw up the internals of the context.

dlowe: @vishr please let me know if I need to do anything else, here. I'm eager to get this fixed so that we can continue migrating to v4 :)
dlowe: Ping! I'm still hoping to get someone's attention on this...
vishr: @dlowe Give me few hours :)
vishr: @dlowe Can you look at the failing build?
dlowe: @vishr oooops! Fixed now and rebased.
codecov[bot]: # [Codecov](https://codecov.io/gh/labstack/echo/pull/1463?src=pr&el=h1) Report
> Merging [#1463](https://codecov.io/gh/labstack/echo/pull/1463?src=pr&el=desc) into [master](https://codecov.io/gh/labstack/echo/commit/5bf6888444a4491eb1a46506044bbae0fb0abf39?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/labstack/echo/pull/1463/graphs/tree.svg?width=650&token=vqnT76l79i&height=150&src=pr)](https://codecov.io/gh/labstack/echo/pull/1463?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1463      +/-   ##
==========================================
+ Coverage   84.34%   84.35%   +<.01%     
==========================================
  Files          27       27              
  Lines        2076     2077       +1     
==========================================
+ Hits         1751     1752       +1     
  Misses        212      212              
  Partials      113      113
```


| [Impacted Files](https://codecov.io/gh/labstack/echo/pull/1463?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [context.go](https://codecov.io/gh/labstack/echo/pull/1463/diff?src=pr&el=tree#diff-Y29udGV4dC5nbw==) | `91.16% <100%> (+0.03%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/labstack/echo/pull/1463?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/labstack/echo/pull/1463?src=pr&el=footer). Last update [5bf6888...748dc63](https://codecov.io/gh/labstack/echo/pull/1463?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/context.go b/context.go
index 27da5ff..0046e5d 100644
--- a/context.go
+++ b/context.go
@@ -312,7 +312,10 @@ func (c *context) ParamValues() []string {
 }
 
 func (c *context) SetParamValues(values ...string) {
-	c.pvalues = values
+	// NOTE: Don't just set c.pvalues = values, because it has to have length c.echo.maxParam at all times
+	for i, val := range values {
+		c.pvalues[i] = val
+	}
 }
 
 func (c *context) QueryParam(name string) string {


Test patch:
diff --git a/bind_test.go b/bind_test.go
index b9fb9de..943cfd5 100644
--- a/bind_test.go
+++ b/bind_test.go
@@ -332,6 +332,7 @@ func TestBindbindData(t *testing.T) {
 
 func TestBindParam(t *testing.T) {
 	e := New()
+	*e.maxParam = 2
 	req := httptest.NewRequest(GET, "/", nil)
 	rec := httptest.NewRecorder()
 	c := e.NewContext(req, rec)
@@ -362,6 +363,7 @@ func TestBindParam(t *testing.T) {
 	// Bind something with param and post data payload
 	body := bytes.NewBufferString(`{ "name": "Jon Snow" }`)
 	e2 := New()
+	*e2.maxParam = 2
 	req2 := httptest.NewRequest(POST, "/", body)
 	req2.Header.Set(HeaderContentType, MIMEApplicationJSON)
 
diff --git a/context_test.go b/context_test.go
index 47be19c..bb4a9ed 100644
--- a/context_test.go
+++ b/context_test.go
@@ -93,6 +93,7 @@ func (responseWriterErr) WriteHeader(statusCode int) {
 
 func TestContext(t *testing.T) {
 	e := New()
+	*e.maxParam = 1
 	req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(userJSON))
 	rec := httptest.NewRecorder()
 	c := e.NewContext(req, rec).(*context)
@@ -471,6 +472,7 @@ func TestContextPath(t *testing.T) {
 
 func TestContextPathParam(t *testing.T) {
 	e := New()
+	*e.maxParam = 2
 	req := httptest.NewRequest(http.MethodGet, "/", nil)
 	c := e.NewContext(req, nil)
 
@@ -487,6 +489,26 @@ func TestContextPathParam(t *testing.T) {
 	testify.Equal(t, "", c.Param("undefined"))
 }
 
+func TestContextGetAndSetParam(t *testing.T) {
+	e := New()
+	*e.maxParam = 2
+	req := httptest.NewRequest(http.MethodGet, "/:foo", nil)
+	c := e.NewContext(req, nil)
+	c.SetParamNames("foo")
+
+	// round-trip param values with modification
+	paramVals := c.ParamValues()
+	testify.EqualValues(t, []string{""}, c.ParamValues())
+	paramVals[0] = "bar"
+	c.SetParamValues(paramVals...)
+	testify.EqualValues(t, []string{"bar"}, c.ParamValues())
+
+	// shouldn't explode during Reset() afterwards!
+	testify.NotPanics(t, func() {
+		c.Reset(nil, nil)
+	})
+}
+
 func TestContextFormValue(t *testing.T) {
 	f := make(url.Values)
 	f.Set("name", "Jon Snow")
diff --git a/middleware/jwt_test.go b/middleware/jwt_test.go
index 7f15bd4..f7f089f 100644
--- a/middleware/jwt_test.go
+++ b/middleware/jwt_test.go
@@ -60,6 +60,8 @@ func TestJWTRace(t *testing.T) {
 
 func TestJWT(t *testing.T) {
 	e := echo.New()
+	r := e.Router()
+	r.Add("GET", "/:jwt", func(echo.Context) error { return nil })
 	handler := func(c echo.Context) error {
 		return c.String(http.StatusOK, "test")
 	}