fix url parsing for forwarded url after route-service pass#247
Conversation
|
❌ Hey otaviojacobi! All pull request submitters and commit authors must have a Contributor License Agreement (CLA). Click here for details on the CLA process. The following github user @otaviojacobi is not covered by a CLA. After the CLA process is complete, this pull request will need to be closed & reopened. DreddBot will then validate the CLA(s). |
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/164023544 The labels on this github issue will be updated when the story is started. |
|
❌ Hey otaviojacobi! All pull request submitters and commit authors must have a Contributor License Agreement (CLA). Click here for details on the CLA process. The following github user @otaviojacobi is not covered by a CLA. After the CLA process is complete, this pull request will need to be closed & reopened. DreddBot will then validate the CLA(s). |
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/164047160 The labels on this github issue will be updated when the story is started. |
|
✅ Hey otaviojacobi! The commit authors and yourself have already signed the CLA. |
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/164047662 The labels on this github issue will be updated when the story is started. |
| It("strips headers and sends the request to the backend instance", func() { | ||
| handler.ServeHTTP(resp, req) | ||
|
|
||
| Expect(resp.Code).To(Equal(http.StatusTeapot)) |
There was a problem hiding this comment.
It seems like everything passed this code test could be removed. This is really what we are testing. All the rest if just the same as the test just above. We don't need to test again that it "strips headers and sends the request to the backend instance", but just that there are no errors.
There was a problem hiding this comment.
My idea for the other checks was that we assure that we were not breaking anything from the test above when using the special sequence of characters. But I can see why it is test duplication, therefore I'll remove the other checks.
There was a problem hiding this comment.
Hi, I have updated the PR so now the test doesn't test again the same as the above, but only the necessary.
cf-gitbot
added
in progress
delivered
accepted
and removed
in progress
delivered
labels
5 participants
A short explanation of the proposed change:
See #246 - This PR changes the url.Parse to url.ParseRequestURI in the handlers.validateRouteServicePool method. The main reason for that is that url.Parse tries to strip out the # from the unescaped ForwardedUrl which when followed by a % character results in an error. The url.ParseRequestURI doesn't have this problem since it considers that the request doesn't have a #fragment.
An explanation of the use cases your change solves
Requests that pass through the gorouter and have a bound route-service, during the second pass through the gorouter, if the request has a escaped character # followed by a escaped % (as
https://my-cf-route.my.cf.domain/test?findById='%23%25') throw anInvalid Signature Errorbut the request is fine and should be properly forwarded to the backing service.Instructions to functionally test the behavior change using operator interfaces (BOSH manifest, logs, curl, and metrics)
?testquery=%23%25Expected result after the change
All requests with well formed URL should pass through the gorouter with a route-service and the request above should be forwarded to the backend service
Current result before the change
Impossible to make requests that have a escaped character
#(%23) followed by a escaped character%(%25) as the gorouter throws anInvalid Signature ErrorLinks to any other associated PRs
This fixes #246
I have viewed signed and have submitted the Contributor License Agreement
I have made this pull request to the
masterbranchI have run all the unit tests using
bin/testI have run CF Acceptance Tests on bosh lite