path pattern matching is case sensitive

[serbrech]

🐛 Bug Report

URLs are not supposed to be case sensitive, but when defining the gateways with :

• selector: some.service.v1.svc.GetItem
  get: "/some/{param}/CaseSensitive/{other_param}"

the GET request needs to match the casing of the pattern provided.
Of we don't match the casing, the gateway returns 404 Not Found

To Reproduce

see above

Expected behavior

• default to case insensitive
• provide an option to make it case sensitive

(Write what you thought would happen.)

Actual Behavior

(Write what happened. Add screenshots, if applicable.)

Your Environment

(Environment name, version and operating system.)

[serbrech]

I also notice that https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/pattern_test.go
does not include any case sensitivity tests.

[serbrech]

my workaround is to have nginx do a case-insensitive location matching, and rewrite the url using regex captures to match the expected case of the gateway :(.

like this : https://serverfault.com/questions/804533/nginx-case-insensitive-location-with-case-sensitive-proxy-pass

I'm happy to contribute if it's something that makes sense, but maybe I'm the odd one here?
Am I missing something obvious?

[adambabik]

Hi @serbrech, thanks for the report.

URLs are not supposed to be case sensitive, but when defining the gateways with :

Do you have any link to an RFC or similar to support this? I dig a bit around and domains are case insensitive (RFC 4343), although, I haven't found any strict requirements for the rest of the URL. On stackoverflow there was a similar discussion with some examples where various sites implement it differently.

I think this can be translated into a feature request. We should be backward compatible so we shouldn't change the current behaviour as others may depend on it but I don't see any problem with introducing a flag which would enable case insensitivity as you described it.

[serbrech]

I looked at the Pattern matcher and hoped for a simple fix. I naively tried to change the OpLit = to EqualFold, but that did not have the expected outcome.
I'm happy to implement it, but some quick pointers would go a long way to fast track me.

Of course it needs to be backward compatible to release it as a patch, but I do think that the right default should be case insensitive in a future version.
I don't think it's a good api behavior to return 404 because the user entered /User instead of /user, no matter what the spec says, it's highly unpractical, and rarely wanted IMO.

So, as a feature, can we simply add this as an option argument to the muxer, like the lastMatchWins bool ?

if so, here is my minimal proposal :

option 1: mux option

mux := runtime.NewServeMux(runtime.WithCaseInsensitveRouteMatching())

The Pattern struct is created in the template as a var. the template generation doesn't know about the mux options, so we need to use the option when calling h.pat.Match(components, verb), which is in ServeHTTP(...)

We can either modify the Match signature, or we could do h.pat.ignoreCase = true before calling it.

and finally, we change the pattern matching accordingly.

Downside is, it's ON or OFF for the entire gateway.

option 2: generator option

Another possibility is to make it a generation parameter.
That allows us to pass set the case sensitivity directly on the Pattern by setting it directly on the constructor in the template :

grpc-gateway/protoc-gen-grpc-gateway/internal/gengateway/template.go

Line 730 in 7db4bb7

pattern_{{$svc.GetName}}_{{$m.GetName}}_{{$b.Index}} = runtime.MustPattern(runtime.NewPattern({{$b.PathTmpl.Version}}, {{$b.PathTmpl.OpCodes | printf "%#v"}}, {{$b.PathTmpl.Pool | printf "%#v"}}, {{$b.PathTmpl.Verb | printf "%q"}}))

That also allows to define the option per route, and it can even become an annotation.

The option still need to be exposed on mux.HandlePath() as well:

grpc-gateway/runtime/mux.go

Line 182 in 7db4bb7

func (s *ServeMux) HandlePath(meth string, pathPattern string, h HandlerFunc) error {

I think that option 2 is much cleaner and more powerful, so that's what I'd suggest doing. full support for annotations and per route handling doesn't need to be there for a minimal implementation either.

WDYT?

[adambabik]

@serbrech Thank you for the analysis!

I agree that the option 2 is more powerful but I am afraid that it will break the backward compatibility. If we change runtime.NewPattern then it won't be possible to upgrade grpc-gateway without regenerating the files.

The option 1 seems less powerful but, following the logic from the issue description, maybe it makes more sense to turn it on or off globally so that all paths are uniform. I don't quite see any use case why someone would like to decide that by path, excluding some legacy cases which I still think would be better to solve in nginx or similar. It is also safer in terms of backward compatibility.

[stale]

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.