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

404s using colons in the middle of the last path segment #224

Closed
charleschenster opened this issue Sep 17, 2016 · 35 comments · Fixed by #708
Closed

404s using colons in the middle of the last path segment #224

charleschenster opened this issue Sep 17, 2016 · 35 comments · Fixed by #708

Comments

@charleschenster
Copy link

I have the following endpoint and user_ids of the format "user:"

  rpc GetUser(GetUserRequest) returns (GetUserResponse) {
    option (google.api.http) = {
      get: "/v0/users/{user_id}"
    };
  };

I'm getting 404 status code and it doesn't hit my GetUser handler when I call this endpoint with user_ids of the format above. However, if I call the endpoint with a user_id that doesn't have a colon it (e.g. "user123") it does hit my GetUser handler. Url encoding the colon doesn't work either.

What's even more interesting is if I add another colon to the end of the path, grpc-gateway parses the user_id into the correct format that I want. ("/v0/users/user:123:" -> user_id = "user:123"). Given this data point, I believe grpc-gateway is treating colons differently in the last path segment.

For example here: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-grpc-gateway/httprule/parse.go#L86 it strips off the last colon in the segment. Though this looks like the rule for parsing the endpoint path definition and not what is being used to parse incoming requests.

@yugui
Copy link
Member

yugui commented Sep 17, 2016

Thank you for your report.
But I don't understand what are you trying to say.

I guess probably the facts you have reported were different from your expectation, but I don't know what you expected and why you expected so. Could you explain what exactly you expected and its reason?
Also if you have not yet, please read the FAQ at first.

@yugui yugui added the question label Sep 17, 2016
@charleschenster
Copy link
Author

@yugui Thanks for the response!

Given the endpoint definition above here are the use cases with the expected and actual result

  • curl /v0/users/user:123

    Expected: GetUser handler receives request with user_id = 'user:123'
    Actual: 404 status code is returned

  • curl /v0/users/user123

    Expected: GetUser handler receives request with user_id = 'user123'
    Actual: (Matches expected) GetUser handler receives request with user_id = 'user123'

  • curl /v0/users/user:123:

    Expected: GetUser handler receives request with user_id = 'user:123:'
    Actual: GetUser handler receives request with user_id = 'user:123'

@ambarc
Copy link

ambarc commented Sep 17, 2016

+1 seeing the same issue.

Colons in the URL path are part of the standard spec:

http://stackoverflow.com/questions/2053132/is-a-colon-safe-for-friendly-url-use

and there's likely some miscommunication happening at the gateway layer as it tries to speak to gRPC.

@yugui
Copy link
Member

yugui commented Sep 18, 2016

Colons in the URL path are part of the standard spec:

Right. But it does not always mean that our spec supports the URL.

The problem is something similar to shift/reduce conflict between a VARIABLE value and VERB in the spec at runtime. /v0/users/user:123 actually looks like a sequence path components [v0, users, user] and a verb 123.
So I doubt if we should parse user:123 as a variable value based on the fact that there's no other template /v0/users/*:123 defined because it would mean that the interpretation of a URL pattern depends on other patterns if we parsed so.

As a workaround, you can still escape colon with percent-encoding or write a custom wrapper.

@charleschenster
Copy link
Author

charleschenster commented Sep 18, 2016

Thanks @yugui

I mentioned in the first comment that I tried percent encoding the id and it didn't work. Here's the expected and actual use case here.

  • curl /v0/users/user%3A123

    Expected: GetUser handler receives request with user_id = 'user:123'
    Actual: 404 status code is returned

It looks like here: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/mux.go#L60 the path used should be the raw path to check for the colon. Then the path should be url decoded for the rest of the method.

Thanks for pointing me to the custom wrapper, I'll see if it'll match my needs in the meantime.

@charleschenster
Copy link
Author

@yugui Any updates on this? As described in the previous post, the raw path should be used to check for the colon and presence of a verb.

@ghost
Copy link

ghost commented Feb 14, 2017

Same issue here and the link to the "spec" is broken (I suppose its this file https://github.com/googleapis/googleapis/blob/master/google/api/http.proto ). But its not clean what is a verb anyway.

@hoveychen
Copy link

Any update on this issue? Path template seems to be broken by some undocumented Verb stuff.
Encoding colons with %3A doesn't fix.

@tmc
Copy link
Collaborator

tmc commented Apr 19, 2017

A good first step here would be to submit or modify an executable test case that demonstrates this problem. Making contributions easy for beginner contributors is important so if you hit snags on this path please open issues to improve documentation.

@Azuka
Copy link

Azuka commented May 5, 2018

I was able to get around this by wrapping the mux itself with a hack handler that appends a : to the request path if there's none, because all my ids are urns:

func wrapMux(h http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if !strings.HasSuffix(r.URL.Path, "/") && !strings.HasSuffix(r.URL.Path, ":") {
			r.URL.Path += ":"
		}
		h.ServeHTTP(w, r)
	})
}

Confirmed that both paths like /users and /users/urn:user:123 work

@kolotaev
Copy link

kolotaev commented Jun 5, 2018

I have the same issue, because of the usage of URN as User ID. Not ID with bare colon, nor url-encoded one doesn't match the route and results in 404.

Middleware suggested by @Azuka helps a lot, but, to my mind, this case should be considered and handled in grpc-gateway internally.

jfhamlin added a commit to jfhamlin/grpc-gateway that referenced this issue Jul 24, 2018
jfhamlin added a commit to jfhamlin/grpc-gateway that referenced this issue Jul 24, 2018
jfhamlin added a commit to jfhamlin/grpc-gateway that referenced this issue Jul 24, 2018
jfhamlin added a commit to jfhamlin/grpc-gateway that referenced this issue Jul 24, 2018
…onent

Parsing out a "verb" from the input path is a convenience for
parsing. Whether the colon and string are _actually_ a verb depends on
the matching HTTP rule. This change gives a verb-less pattern a chance
to match a path by assuming the colon+string suffix are a part of the
final component.

Fixes grpc-ecosystem#224

Signed-off-by: James Hamlin <[email protected]>
jfhamlin added a commit to jfhamlin/grpc-gateway that referenced this issue Jul 24, 2018
…onent

Parsing out a "verb" from the input path is a convenience for
parsing. Whether the colon and string are _actually_ a verb depends on
the matching HTTP rule. This change gives a verb-less pattern a chance
to match a path by assuming the colon+string suffix are a part of the
final component.

Fixes grpc-ecosystem#224

Signed-off-by: James Hamlin <[email protected]>
jfhamlin added a commit to jfhamlin/grpc-gateway that referenced this issue Jul 24, 2018
…onent

Parsing out a "verb" from the input path is a convenience for
parsing. Whether the colon and string are _actually_ a verb depends on
the matching HTTP rule. This change gives a verb-less pattern a chance
to match a path by assuming the colon+string suffix are a part of the
final component.

Fixes grpc-ecosystem#224

Signed-off-by: James Hamlin <[email protected]>
achew22 pushed a commit that referenced this issue Aug 1, 2018
… (and string) (#708)

* If a pattern has no verb, match with a verb arg appended to last component

Parsing out a "verb" from the input path is a convenience for
parsing. Whether the colon and string are _actually_ a verb depends on
the matching HTTP rule. This change gives a verb-less pattern a chance
to match a path by assuming the colon+string suffix are a part of the
final component.

Fixes #224

Signed-off-by: James Hamlin <[email protected]>
dmacthedestroyer pushed a commit to dmacthedestroyer/grpc-gateway that referenced this issue Aug 2, 2018
* Add explicit dependency versions (grpc-ecosystem#696)

Version constraints are copied from existing Bazel rules. In the
future, these version upgrades must be performed atomically.

* Add OpenTracing support to docs (grpc-ecosystem#705)

* protoc-gen-swagger: support all well-known wrapper types

There were a few well-known wrapper types missing from
the wkSchemas map. In specific UInt32Value, UInt64Value
and BytesValue. This change handles them (and maps them to
the same swagger types as the non-wrapped values)

This also fixes the mapping of Int64Value. The Int64Value handling
maps the value to a swagger integer. The documentation for
Int64Value indicates that it should be mapped to a JSON
string (also the mapping for normal int64 in protoc-gen-swagger
maps it to a string, so this was inconsistent.)

* Add test case and proposed fix for path component with trailing colon (and string) (grpc-ecosystem#708)

* If a pattern has no verb, match with a verb arg appended to last component

Parsing out a "verb" from the input path is a convenience for
parsing. Whether the colon and string are _actually_ a verb depends on
the matching HTTP rule. This change gives a verb-less pattern a chance
to match a path by assuming the colon+string suffix are a part of the
final component.

Fixes grpc-ecosystem#224

Signed-off-by: James Hamlin <[email protected]>

* Fix up examples

* Make support paths option

Make protoc-gen-grpc-gateway support paths option such like golang/protobuf#515.
@kassiansun
Copy link

@Azuka @kolotaev I don't think URN is valid inside URL, they are exclusive forms of resource identifying.

@Azuka
Copy link

Azuka commented Sep 21, 2018

@kassiansun, I'm not technically using urns (the prefix isn't urn:): I'm using something similar to Amazon's ARNs.

@kassiansun
Copy link

Are you serious about technically, or just some kind of "speechcraft"?

Anyway, I can fork this repo and maintain it by myself, thank OSS.

@Tommy-42
Copy link

Tommy-42 commented Oct 3, 2018

Hi guys,

We have this use case in our API where our aliases can contains a : but it shouldn't be treated as verb I think

For example :

proto : 
  get: "/alias/{alias}"
curl : 
  localhost:8080/alias/toto:tata:tutu

#708 was fixing the problem

@johanbrandhorst
Copy link
Collaborator

Hi @Tommy-42. Yes, we want to get in the best of both worlds, the "last one wins" as suggested by @jfhamlin. Unfortunately, because it breaks backwards compatibility, until we release v2 we will have to hide it behind a generator flag. Would you be interested in contributing a fix for this?

@Azuka
Copy link

Azuka commented Oct 3, 2018

@kassiansun, sorry I didn't realize the use-case for this. Looks like we're working towards supporting both. Thanks everyone.

@Tommy-42
Copy link

Tommy-42 commented Oct 5, 2018

hi @johanbrandhorst ,
I can try something, But I cannot promise you anything as I don't know if I will have time to work on this saddly ( a bit overwhelm this time )

adasari pushed a commit to adasari/grpc-gateway that referenced this issue Apr 9, 2020
… (and string) (grpc-ecosystem#708)

* If a pattern has no verb, match with a verb arg appended to last component

Parsing out a "verb" from the input path is a convenience for
parsing. Whether the colon and string are _actually_ a verb depends on
the matching HTTP rule. This change gives a verb-less pattern a chance
to match a path by assuming the colon+string suffix are a part of the
final component.

Fixes grpc-ecosystem#224

Signed-off-by: James Hamlin <[email protected]>
adasari pushed a commit to adasari/grpc-gateway that referenced this issue Apr 9, 2020
The solution for grpc-ecosystem#224 turned out to break backwards
compatibility, so we're going to have to find another
solution for users who desire this behaviour.
Also adds test cases from grpc-ecosystem#760.
adasari pushed a commit to adasari/grpc-gateway that referenced this issue Apr 9, 2020
… flags) (grpc-ecosystem#949)

* Support colon in final path segment, last match wins behavior (behind flags)

Signed-off-by: James Hamlin <[email protected]>

* Update examples

Signed-off-by: James Hamlin <[email protected]>

Fixes grpc-ecosystem#224
@johanbrandhorst
Copy link
Collaborator

We'd like to make this the default behaviour in v2, please come forward if you'd like to contribute this!

@HirdrWit
Copy link

Old problems require modern solutions. You can add allow_colon_final_segments=true to your grpc-gateway_out in the generate file.
I.e.
--grpc-gateway_out=logtostderr=true,allow_colon_final_segments=true:generated
This works with protoc2:2.0.0 >

@johanbrandhorst
Copy link
Collaborator

Yeah, but we want to remove the need for this to be a flag and enable it by default.

@alexandrem
Copy link

I confirm that the grpc-gateway_out argument allow_colon_final_segments=true works well with the latest v2.

It's a good enough workaround for my use case, but I think it would be nice to have this as some kind of option to the gateway runtime mux server.

@johanbrandhorst
Copy link
Collaborator

Any reason we can't just make it the default behaviour?

@alexandrem
Copy link

Having it as the default would be even better, it'd have saved me some trouble :)

johanbrandhorst added a commit that referenced this issue May 23, 2020
In accordance with the http.proto spec, we should
use last-match-wins to determine which handler
is chosen if two are matched.

See #224
for more discussion.
johanbrandhorst added a commit that referenced this issue May 23, 2020
In accordance with the http.proto spec, we should
use last-match-wins to determine which handler
is chosen if two are matched.

See #224
for more discussion.
johanbrandhorst added a commit that referenced this issue May 23, 2020
In accordance with the http.proto spec, we should
use last-match-wins to determine which handler
is chosen if two are matched.

See #224
for more discussion.
@johanbrandhorst
Copy link
Collaborator

This was made the default behaviour in v2: #1384

@OscarHanzely
Copy link

OscarHanzely commented Jan 28, 2021

I am still having issue with colons in the url path. My last segment is parsing IP address. The Google IPv6 address breaks the gateway behavior.

        option (google.api.http) = {
          get: "/lookup/{ip_addr}"
        };
    }

Request like [GET] {{geoip_api_dsn}}/lookup/2001:4860:4860::8888
returns 501 Not Implemented status as it breaks completely the parser.

solution with allow_colon_final_segments=true worked though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet