Fix the issue encountered when the value of queries starting with '{' by zhujf1989 · Pull Request #555 · OpenFeign/feign

zhujf1989 · 2017-04-25T09:36:54Z

When using RibbonClient, the starting %7B in the value of queries would be replaced with {. For example, a request URL like http://example.com/q?p=%7Baaa%7D would become http://example.com/q?p={aaa} after passing toRequest() in feign.ribbon.LBClient.

This issue could make feign not able to play with newer versions of Tomcat after fixing CVE-2016-6816.

…different from OpenFeign#540.

codefromthecrypt · 2017-04-26T07:54:52Z

I feel like this is yuk. Yuk that it requires so much care, but this is sort of par for the course. Since adding a public api, probably good to javadoc when you need this. Also note in comment where used. If there is a way to make ribbon break without. Test there would be nice too

…

On 25 Apr 2017 17:36, "Echo19890615" ***@***.***> wrote: When using RibbonClient, the starting %7B in the value of queries would be replaced with {. For example, a request URL like http://example.com/q?p=%7Baaa%7D would become http://example.com/q?p={aaa} after passing toRequest() in feign.ribbon.LBClient. This issue could make feign not able to play with newer versions of Tomcat after fixing CVE-2016-6816. ------------------------------ You can view, comment on, or merge this pull request online at: #555 Commit Summary - fix: support the query values starting with {, when use RibbonClient。different from #540. File Changes - *M* core/src/main/java/feign/RequestTemplate.java <https://github.com/OpenFeign/feign/pull/555/files#diff-0> (45) - *M* core/src/test/java/feign/RequestTemplateTest.java <https://github.com/OpenFeign/feign/pull/555/files#diff-1> (10) - *M* ribbon/src/main/java/feign/ribbon/LBClient.java <https://github.com/OpenFeign/feign/pull/555/files#diff-2> (2) Patch Links: - https://github.com/OpenFeign/feign/pull/555.patch - https://github.com/OpenFeign/feign/pull/555.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#555>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD614c7R-glXxEUtwYQ-xcZR8wfgIkKks5rzb63gaJpZM4NHOyx> .

fengchj · 2017-04-27T09:08:26Z

i am puzzled, is this a bug?

zhujf1989 · 2017-04-27T13:46:45Z

Hi, adriancole

IMHO this is a serious issue. That makes a request with queries whose value is a json string have chance to get a response with code 400 since the request line contains "unwise" characters referring to RFC 2396. We want to use Ribbon in our project & are really appreciate that Feign can reduce our work. So we do this to make Feign suitable for wider use.

RequestTemplate.appendDirectly() is added just for RibbonClient & there is no other place to call, therefore there is no side effect to be seen.

codefromthecrypt · 2017-04-27T13:56:17Z

just an aside, I think there's other types of things that will probably break similarly.. iirc there was some outstanding work due to elasticsearch's query api as well.

codefromthecrypt · 2017-04-27T13:58:34Z

  }

+  /**
+   * Append the specified {@code value} to {@code url} without decoding or encoding. Append all 


lots of doc. but no example. forward referencing to ribbon probably won't help people understand.

Try to shorten this to maybe two succinct sentences and an example.

Ex.

Like {@link #foo} except .. <p> <-- note p not br and the newline break makes it easy to read, too.. ex if your input is X unless you call this method, it would become Y

codefromthecrypt · 2017-04-27T13:59:07Z

 import java.util.Map;
 import java.util.Map.Entry;

-import static feign.Util.CONTENT_LENGTH;


revert the re-order

codefromthecrypt · 2017-04-27T13:59:37Z

+
+  @Test
+  public void appendDirectlytest() throws Exception {
+    String url = "http://test.feign.com/query?params=%7b%22int%22%3a1%2c%22string%22%3a%22str%22%7d";


comment why we care. ex people won't necessarily be able to understand that this is escaped json (if it is)

codefromthecrypt · 2017-04-27T14:00:33Z

      return new RequestTemplate()
          .method(request.method())
-          .append(getUri().toASCIIString())
+          .appendDirectly(getUri().toASCIIString())


since this is only added for ribbon, there should be a test here.

the junit for RibbonRequest.toRequest() has been added, please review

codefromthecrypt · 2017-04-27T14:03:56Z

      return new RequestTemplate()
          .method(request.method())
-          .append(getUri().toASCIIString())
+          .appendDirectly(getUri().toASCIIString())


wait.. couldn't this be much easier by just making a request?

like this..

Request toRequest() { return Request.create( request.method(), getUri().toASCIIString(), request.headers(), request.body(), request.charset() ); }

Using Request.create() is exactly easier to make an orignal request, thanks for reminding :) . But the old code(use append) has the logic that add the header "Content-Length" to the request. Base on keeping the old logic and code reuse, should we continue to use appendDirectly()?

codefromthecrypt · 2017-04-30T02:11:23Z

I think based on how confusing the alternative is, and that it requires a new public api to implement it.. stick with Request.create until that becomes a problem. That way, this can be a patch release

…junit" This reverts commit c48d8c8.

…junit test" This reverts commit 821c9d4.

…nClient。different from OpenFeign#540." This reverts commit 6ccea8a.

…t, by using Request.create()

zhujf1989 · 2017-05-03T11:28:21Z

revert all the codes of RequestTemplate.appendDirectly() by using 'git revert'
keep the junit test for RibbonRequest.toRequest()
fix the issue by using Request.create(), and keeping the old logic(add the header 'Content-Length' manually).

codefromthecrypt · 2017-05-03T12:20:22Z

+      final byte[] body = request.body();
+      final int bodyLength = body != null ? body.length : 0;
+      // create a new Map to avoid side effect, not to change the old headers
+      Map<String, Collection<String>> headers = new HashMap<String, Collection<String>>();


please use LinkedHashMap as this ensures tests don't flake on iteration order

codefromthecrypt · 2017-05-03T12:21:44Z

+    assertEquals(requestOrign.charset(), requestGetFormToRequest.charset());
+    assertNotNull(requestOrign.headers());
+    assertEquals(0, requestOrign.headers().size()); // double check the baseline Request
+    assertNotNull(requestGetFormToRequest.headers()); // double check the baseline Request, header is not null, but empty


avoid checks that are duplicated from constructor (I think it isn't possible that this is null)

codefromthecrypt · 2017-05-03T12:22:08Z

+    Request requestGetFormToRequest = ribbonRequest.toRequest();
+
+    // test the same attributes
+    assertEquals(requestOrign.method(), requestGetFormToRequest.method());


this test is kindof hard to read. Can you look at other unit tests? you'll notice we use assertj which is helpful

codefromthecrypt · 2017-05-04T01:17:35Z

if you don't mind, try and cleanup for merge in the next couple days so we can get this out over the weekend. Will be doing a release then. cc @jfuerth

…st by using assertj

codefromthecrypt · 2017-05-05T02:28:47Z

LGTM let me know if you are ready for merge

zhujf1989 · 2017-05-05T06:17:01Z

Of course! Feel free to merge it.

…OpenFeign#555) support the query values starting with {, when use RibbonClient。different from OpenFeign#540.

…#555) support the query values starting with {, when use RibbonClient。different from #540.

fix: support the query values starting with {, when use RibbonClient。…

6ccea8a

…different from OpenFeign#540.

add javadoc to RequestTemplate.appendDirectly() and add some junit test

821c9d4

codefromthecrypt reviewed Apr 27, 2017

View reviewed changes

zhujf1989 added 2 commits April 29, 2017 16:56

revert the import re-order & modify javadoc & add comment to junit

c48d8c8

add junit for LBClinet.RibbonRequest.toRequest()

903ed2c

zhujf1989 added 4 commits May 3, 2017 19:17

Revert "revert the import re-order & modify javadoc & add comment to …

24caff4

…junit" This reverts commit c48d8c8.

Revert "add javadoc to RequestTemplate.appendDirectly() and add some …

674397b

…junit test" This reverts commit 821c9d4.

Revert "fix: support the query values starting with {, when use Ribbo…

5b398e2

…nClient。different from OpenFeign#540." This reverts commit 6ccea8a.

fix: support the query values starting with '{', when use RibbonClien…

9a9bd94

…t, by using Request.create()

codefromthecrypt reviewed May 3, 2017

View reviewed changes

Request.toRequest() change headers type to LinkedHashMap & optimiz te…

b269992

…st by using assertj

codefromthecrypt merged commit 82b57a3 into OpenFeign:master May 6, 2017

lowzj mentioned this pull request May 17, 2017

Update feign to 9.5.0 spring-cloud/spring-cloud-netflix#1956

Closed

phymbert pushed a commit to phymbert/feign that referenced this pull request Aug 22, 2017

Fix the issue encountered when the value of queries starting with '{' (…

3eca281

…OpenFeign#555) support the query values starting with {, when use RibbonClient。different from OpenFeign#540.

velo pushed a commit that referenced this pull request Oct 7, 2024

Fix the issue encountered when the value of queries starting with '{' (…

ac30d81

…#555) support the query values starting with {, when use RibbonClient。different from #540.

velo pushed a commit that referenced this pull request Oct 8, 2024

Fix the issue encountered when the value of queries starting with '{' (…

94b8707

…#555) support the query values starting with {, when use RibbonClient。different from #540.

Uh oh!

Conversation

zhujf1989 commented Apr 25, 2017

Uh oh!

codefromthecrypt commented Apr 26, 2017 via email

Uh oh!

fengchj commented Apr 27, 2017

Uh oh!

zhujf1989 commented Apr 27, 2017

Uh oh!

codefromthecrypt commented Apr 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Apr 30, 2017 via email

Uh oh!

zhujf1989 commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented May 4, 2017

Uh oh!

codefromthecrypt commented May 5, 2017

Uh oh!

zhujf1989 commented May 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants