TykTechnologies / tyk

Tyk Open Source API Gateway written in Go, supporting REST, GraphQL, TCP and gRPC protocols
Other
9.6k stars 1.08k forks source link

Meta data header value of integer type produces a panic #944

Closed c1784497 closed 7 years ago

c1784497 commented 7 years ago

Do you want to request a feature or report a bug? bug What is the current behavior?

[Jul 25 16:22:54] ERROR Regex error: error parsing regexp: missing argument to repetition operator: `*`
2017/07/25 16:22:54 http: panic serving 1.2.3.4:50818: interface conversion: interface is float64, not string
goroutine 9736 [running]:
net/http.(*conn).serve.func1(0xc421423d80)
        /usr/local/go/src/net/http/server.go:1491 +0x12a
panic(0xccdc60, 0xc4213cecc0)
        /usr/local/go/src/runtime/panic.go:458 +0x243
main.(*TransformHeaders).iterateAddHeaders(0xc42191e908, 0xc421218030, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/middleware_modify_headers.go:81 +0x3ea
main.(*TransformHeaders).ProcessRequest(0xc42191e908, 0x12b3220, 0xc4202c7c70, 0xc42153b590, 0x0, 0x0, 0x19, 0x2, 0x4)
        /src/github.com/TykTechnologies/tyk/middleware_modify_headers.go:157 +0x49b
main.CreateMiddleware.func1.1(0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/middleware.go:76 +0x637
net/http.HandlerFunc.ServeHTTP(0xc4222f4cc0, 0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /usr/local/go/src/net/http/server.go:1726 +0x44
main.CreateMiddleware.func1.1(0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/middleware.go:100 +0x83c
net/http.HandlerFunc.ServeHTTP(0xc4222f4d00, 0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /usr/local/go/src/net/http/server.go:1726 +0x44
main.CreateMiddleware.func1.1(0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/middleware.go:100 +0x83c
net/http.HandlerFunc.ServeHTTP(0xc4222f4d40, 0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /usr/local/go/src/net/http/server.go:1726 +0x44
main.CreateMiddleware.func1.1(0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/middleware.go:100 +0x83c
net/http.HandlerFunc.ServeHTTP(0xc4222f4d80, 0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /usr/local/go/src/net/http/server.go:1726 +0x44
main.CreateMiddleware.func1.1(0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/middleware.go:100 +0x83c
net/http.HandlerFunc.ServeHTTP(0xc4222f4dc0, 0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /usr/local/go/src/net/http/server.go:1726 +0x44
main.CreateMiddleware.func1.1(0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/middleware.go:100 +0x83c
net/http.HandlerFunc.ServeHTTP(0xc4222f4e00, 0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /usr/local/go/src/net/http/server.go:1726 +0x44
main.CreateMiddleware.func1.1(0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/middleware.go:100 +0x83c
net/http.HandlerFunc.ServeHTTP(0xc4222f4e40, 0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /usr/local/go/src/net/http/server.go:1726 +0x44
main.CreateMiddleware.func1.1(0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/middleware.go:100 +0x83c
net/http.HandlerFunc.ServeHTTP(0xc4222f4e80, 0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /usr/local/go/src/net/http/server.go:1726 +0x44
main.CreateMiddleware.func1.1(0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/middleware.go:100 +0x83c
net/http.HandlerFunc.ServeHTTP(0xc4222f4ec0, 0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /usr/local/go/src/net/http/server.go:1726 +0x44
github.com/TykTechnologies/tyk/vendor/github.com/gorilla/mux.(*Router).ServeHTTP(0xc420218f00, 0x12b3220, 0xc4202c7c70, 0xc42153b590)
        /src/github.com/TykTechnologies/tyk/vendor/github.com/gorilla/mux/mux.go:114 +0x10d
net/http.(*ServeMux).ServeHTTP(0xc422302f30, 0x12b3220, 0xc4202c7c70, 0xc42153b3b0)
        /usr/local/go/src/net/http/server.go:2022 +0x7f
net/http.serverHandler.ServeHTTP(0xc420a2a000, 0x12b3220, 0xc4202c7c70, 0xc42153b3b0)
        /usr/local/go/src/net/http/server.go:2202 +0x7d
net/http.(*conn).serve(0xc421423d80, 0x12b42a0, 0xc4213ce4c0)
        /usr/local/go/src/net/http/server.go:1579 +0x4b7
created by net/http.(*Server).Serve
        /usr/local/go/src/net/http/server.go:2293 +0x44d

What is the expected behavior? No error. If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem Add meta data key:value pair: contact:0 Add header:value pair: X-Contact: $tyk_meta.contact Which versions of Tyk affected by this issue? Did this work in previous versions of Tyk? 2.3.5 / 1.3.5

Problem does not exist with value 1 instead of 0.

buger commented 7 years ago

Thank you for reporting!

mvdan commented 7 years ago

@c1784497 when you say Add meta data key:value pair, exactly what do you mean? How did you add it?

Could you share your API definition?

mvdan commented 7 years ago

Please note that there are two issues here - I'm not convinced that the regex error and the interface panic are directly related.

In any case, I've found the cause of the panic.

The good news is that this is fixed in master, in a way. In stable, MetaData is a map[string]interface{}, but in master it's a map[string]string.

@c1784497 I assume you put in there something like "meta_data": {"contact": 0}. Is that correct?

That would explain why it was float64 instead of string - that's what encoding/json decodes a number to interface{} as.

@buger this leads to a question - should we support just strings, or any type? In other words, should we support "contact": 0 too, or just "contact": "0"?

In favor of doing it is that nothing other than a string ever worked. So we do know for a fact that noone relies on it.

Another thing in favor of it is that these are used in strings, like headers. Not all JSON types can be stringified easily. What about objects? Not even numbers are safe - if they're floating point, which all JSON numbers are, the string might be different (e.g. missing zeros, rounding, etc).

One point against only supporting strings is that the docs seem to say otherwise: This field is a string key/value map that can store any kind of information about the underlying identity of a session https://tyk.io/docs/concepts/session-meta-data/

When I read that, any kind of information says any type to me. We should clarify that values can only be strings, if we want to enforce this.

mvdan commented 7 years ago

I forgot to add - I clearly think we should only support strings here. It makes the JSON config easier, it makes the docs simpler and it makes the code less tricky. And I see no reason to support other types like numbers and objects.

mvdan commented 7 years ago

As to the regex error, this is a regex compiled directly from your API definition. So I can't help you further unless you provide your entire API definition. But it seems like the issue is there, unless there's a bug in the code that I'm missing.

mvdan commented 7 years ago

Friendly ping @c1784497 - could you confirm what meta_data JSON object were you using?

And also see my comment above about the regex error, which I believe is unrelated and due to an issue in your API definition.

buger commented 7 years ago

In my view, if we can not restrict user on value types, this will be the best option. In theory, it can be fixed with smth like fmt.Sprintf("%v", tempVal) instead of direct type conversion.

mvdan commented 7 years ago

After some discussion, we've agreed to just make the code work as the docs describe - at least, for 2.3.8. The docs say string key/value map, so only strings are allowed.

In other words, use "0" instead of 0.

These values are only to be used in headers, and headers are strings, so any other type makes little sense.

I'll submit a patch for 2.3.8 to avoid the panic, though.

mvdan commented 7 years ago

Merged.