Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No check of length of parameter values #1531

Closed
3 tasks done
linux019 opened this issue Mar 10, 2020 · 7 comments
Closed
3 tasks done

No check of length of parameter values #1531

linux019 opened this issue Mar 10, 2020 · 7 comments
Labels

Comments

@linux019
Copy link

Issue Description

Checklist

  • Dependencies installed
  • No typos
  • Searched existing issues and docs

Working code to debug

package main

import (
	"fmt"
	"github.com/labstack/echo"
)

func main() {
	e := echo.New()
	ctx := e.NewContext(nil, nil)
	ctx.SetParamNames("a")
	fmt.Println(ctx.ParamValues())
}

Version/commit

echo/context.go

Line 316 in fc4b1c0

return c.pvalues[:len(c.pnames)]
- missing check

@lammel
Copy link
Contributor

lammel commented Mar 30, 2020

This should be fixed with PR #1535

@stevenvegt
Copy link

It seems the split between the names, values and the maxIndex is still causing problems.

I encountered problems before as described in this issue #1258

This is my middleware:

// DecodeURIPath is a echo middleware that decodes path parameters
func DecodeURIPath(next echo.HandlerFunc) echo.HandlerFunc {
	return func(c echo.Context) error {
		newValues := make([]string, len(c.ParamValues()))
		for i, value := range c.ParamValues() {
			path, err := url.PathUnescape(value)
			if err != nil {
				path = value
			}
			newValues[i] = path
		}
		c.SetParamValues(newValues...)
		return next(c)
	}
}

During execution maxParam is 1, pnames empty and pvalues contains 1 entry with value "".

However, c.ParamValues() returns nothing because instead of the maxParam, it uses the length of pnames: https://github.com/labstack/echo/pull/1535/files#diff-05a3aa0ab9e5687ff167e0ef3ae4c5a6R316

After calling SetParamValues(newValues...) pvalues becomes an empty arrray. But remember, the maxParam is still set to 1.

Then the during the next request Reset https://github.com/labstack/echo/blob/master/context.go#L636 still uses this value to iterate over the pvalues and causes an

echo: http: panic serving 127.0.0.1:55010: runtime error: index out of range [0] with length 0

@stevenvegt
Copy link

FYI: adding the line:

c.SetParamNames(c.ParamNames()...)

Seems to fix my problem by resetting the maxParam value.

@stale
Copy link

stale bot commented Jul 3, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 3, 2020
@stevenvegt
Copy link

A comment just to annoy out stale-bot and giving the echo team a bit of extra time to look at this problem :)

@stale stale bot removed the wontfix label Jul 7, 2020
@lammel lammel added the bug label Sep 4, 2020
@lammel
Copy link
Contributor

lammel commented Oct 1, 2020

We had the same issues in our tests where we are using ctx.Reset() which caused the index out of range panic on access to pvalues:

panic: runtime error: index out of range [0] with length 0 [recovered]

We are using a simple workaround in our test helpers now. Calling ctx.SetParamNames("") and ctx.SetParamValues("") before Reset() of the context ensure the length is set properly.

Example:

// Workaround required for echo v4.1.17, see GitHub #1531
h.Ctx.SetParamNames("")
h.Ctx.SetParamValues("")
h.Ctx.Reset(req, rec)

I will try to work out a PR to cleanup the param value handling and avoid the current pitfalls

@lammel
Copy link
Contributor

lammel commented Dec 14, 2020

PR #1659 has been merged addressing the symptoms so echo should not panic anymore.

@lammel lammel closed this as completed Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants