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

Fix path parsing to use URL.EscapedPath() instead of Path and unescape individual components #660

Closed
wants to merge 5 commits into from

Conversation

jessesuen
Copy link

@jessesuen jessesuen commented May 24, 2018

This addresses issue #308.

When parsing the request URL, ServeHTTP() currently uses URL.Path instead of URL.EscapedPath(). This makes it impossible for grpc-gateway to support having slashes / in a path even if they are encoded (e.g. http://example.com/some%2Fpath/resource). This change will switch parsing to use URL.EscapedPath(), then unescape the individual components after they have been split by /.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@jessesuen
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@achew22
Copy link
Collaborator

achew22 commented May 24, 2018

This looks like a really interesting patch. Can you add some tests that demonstrate the new behavior?

@ivucica
Copy link
Collaborator

ivucica commented May 25, 2018

Possibly more importantly, can you demonstrate old broken behavior and how the new code fixes it?

@jessesuen
Copy link
Author

I will most definitely add a test. I just wanted to solicit feedback on this to see if this would be something acceptable upstream before investing more time in this.

@jessesuen jessesuen changed the title Fix path parsing to use URL.RawPath instead of Path and unescape individual components Fix path parsing to use URL.EscapedPath() instead of Path and unescape individual components May 30, 2018
@jessesuen
Copy link
Author

@achew22, @ivucica -- I added a unit test which can reproduce broken behavior (before my fix) and verifies the desired behavior (after my fix).

However, I am having trouble understanding the remaining node integration test failures:

1) ABitOfEverythingService Create should assign id
1.1) Failed: Object({ url: 'http://localhost:8080/v1/example/a_bit_of_everything/1.5/2.5/4294967296/separator/9223372036854775807/-2147483648/9223372036854775807/4294967295/true/strprefix%2Ffoo/4294967295/2147483647/-4611686018427387904/2147483647/4611686018427387903/camelCase', method: 'POST', headers: Object({ content-type: 'text/plain; charset=utf-8' }), errObj: Error: Not Found, status: 404, statusText: 'Not Found
', data: 'Not Found
' })
1.2) Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
2) ABitOfEverythingService Create should echo the request back
2.1) Failed: Object({ url: 'http://localhost:8080/v1/example/a_bit_of_everything/1.5/2.5/4294967296/separator/9223372036854775807/-2147483648/9223372036854775807/4294967295/true/strprefix%2Ffoo/4294967295/2147483647/-4611686018427387904/2147483647/4611686018427387903/camelCase', method: 'POST', headers: Object({ content-type: 'text/plain; charset=utf-8' }), errObj: Error: Not Found, status: 404, statusText: 'Not Found
', data: 'Not Found

It looks like this ABitOfEverything service has a {string_value=strprefix/*} as part of the path option. However, I don't understand why this wouldn't just be {string_value}.

	rpc Create(ABitOfEverything) returns (ABitOfEverything) {
		// TODO add enum_value
		option (google.api.http) = {
			post: "/v1/example/a_bit_of_everything/{float_value}/{double_value}/{int64_value}/separator/{uint64_value}/{int32_value}/{fixed64_value}/{fixed32_value}/{bool_value}/{string_value=strprefix/*}/{uint32_value}/{sfixed32_value}/{sfixed64_value}/{sint32_value}/{sint64_value}/{nonConventionalNameValue}"
		};
	}

It seems that the test case itself would need to change. Can someone explain the meaning behind {string_value=strprefix/*} ?

@achew22
Copy link
Collaborator

achew22 commented May 31, 2018

It seems that the test case itself would need to change. Can someone explain the meaning behind {string_value=strprefix/*} ?

@yugui, now that I look at that, do you know why that is like that? My guess is that it strips the "strprefix/" from the beginning of the string value, but I don't understand why you wouldn't just put that outside the capture. Do you have any hints?

@ivucica
Copy link
Collaborator

ivucica commented Jun 4, 2018

It seems that the test case itself would need to change. Can someone explain the meaning behind {string_value=strprefix/*} ?

@yugui, now that I look at that, do you know why that is like that? My guess is that it strips the "strprefix/" from the beginning of the string value, but I don't understand why you wouldn't just put that outside the capture. Do you have any hints?

I first thought this might be a case of https://tools.ietf.org/html/rfc6570 page 8 and page 21. My (untested) understanding was that passing strprefix/a,b,c might result in a list { "a", "b", "c" }.

However, I remembered seeing this syntax before in the wild. Here it is in pubsub v1's pubsub.proto.

I think this might mean that the value strprefix/ is included in the output string string_value, but that it is mandatory. If you would move this outside, strprefix/ would still be mandatory, but no longer included in string_value.

Here's the documentation for the linked RPC.

@ivucica
Copy link
Collaborator

ivucica commented Jun 4, 2018

I think that interpretation is correct. Cloud Endpoints's ESP:

@achew22
Copy link
Collaborator

achew22 commented Jun 5, 2018

Reading through, I think I agree with your interpretation of the spec @ivucica.

@jessesuen, I think that should be sufficient information to update the tests to reflect the new behavior. Does that work for you?

@jessesuen
Copy link
Author

Sorry, I was OOO the past week. If you mean moving strprefix/ outside the capture, I will be happy to make this change. I'll update the PR.

@jessesuen
Copy link
Author

Alternatively, do you believe that strprefix has any value in the example? In other words, could we simplify a_bit_of_everything_service.spec.js from:

post: "..../{bool_value}/strprefix/{string_value}/{uint32_value}..."

into this:

post: "..../{bool_value}/{string_value}/{uint32_value}..."

@ivucica
Copy link
Collaborator

ivucica commented Jun 19, 2018 via email

@johanbrandhorst
Copy link
Collaborator

Bump, what's left to do before we can merge this?

@ivucica
Copy link
Collaborator

ivucica commented Sep 9, 2018

I believe error from #660 (comment) needs to be fixed?

@johanbrandhorst
Copy link
Collaborator

Please rebase on master for new CI functionality.

@stale
Copy link

stale bot commented Sep 9, 2019

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 Sep 9, 2019
@stale stale bot closed this Sep 16, 2019
@kindermoumoute
Copy link

This MR is still relevant and it is actually fixing a bug.

@jessesuen any update on this?

@johanbrandhorst
Copy link
Collaborator

I'd be happy to review any new PRs to fix this.

remyleone added a commit to remyleone/grpc-gateway that referenced this pull request Jul 30, 2021
…th and unescape individual components

Basically we want to route for instance http://example.com/resources/some%2Fpath/something from this http annotation :

    get: /resources/{key}/something

We want the  key field in gRPC to be able to contain 0 or many slashes

grpc-ecosystem#660
@v3n
Copy link
Contributor

v3n commented Nov 12, 2021

@johanbrandhorst Would it be possible to get a new version cut that contains this fix?

@johanbrandhorst
Copy link
Collaborator

@v3n thanks for the prompting, I can cut v2.7.0 once #2418 has landed or within the next week, whichever comes soonest. Does that sound okay?

@johanbrandhorst
Copy link
Collaborator

@v3n I've just released v2.7.0

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

Successfully merging this pull request may close these issues.

7 participants