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

upgrade spring-cloud-release from F to G, feign urlencode error #879

Closed
dragontree101 opened this issue Jan 19, 2019 · 7 comments · Fixed by #882
Closed

upgrade spring-cloud-release from F to G, feign urlencode error #879

dragontree101 opened this issue Jan 19, 2019 · 7 comments · Fixed by #882
Assignees
Labels
bug Unexpected or incorrect behavior regression Bugs and issues related to unintended breaking changes spring-cloud Issues related to Spring Cloud OpenFeign

Comments

@dragontree101
Copy link

dragontree101 commented Jan 19, 2019

upgrade spring-cloud-release from F to G, feign urlencode error

my app server code is

 @PostMapping(value = "/user/userinfo")
    public BaseResult userInfoSignatureCheck(
            @NotBlank @RequestHeader(WxConstants.HN_APP_ID) String hnAppId,
            @NotBlank @RequestParam(WxConstants.SESSION_KEY) String sessionKey,
            @NotBlank @RequestParam String rawData,
            @NotBlank @RequestParam String signature,
            @NotBlank @RequestParam String encryptedData,
            @NotBlank @RequestParam String iv) {
        try {
            ...
            return BaseResult.success(userInfo);
        } catch (WxErrorException e) {
            log.error("WxErrorException", e);
            return BaseResult.fail(e.getError().getErrorCode(), e.getError().getErrorMsg());
        }
    }

and app client code is

@FeignClient(name = "mini-program-api")
public interface MiniProgramApi {

    @RequestMapping(value = "/mini-program-api/user/userinfo", method = RequestMethod.POST, consumes = MediaType.APPLICATION_FORM_URLENCODED_VALUE)
    BaseResult<Map<String,Object>> userinfo2(@RequestHeader("hn-app-id") String hnAppId,
                                             @RequestParam("sessionKey") String sessionKey,
                                             @RequestParam("rawData") String rawData,
                                             @RequestParam("signature") String signature,
                                             @RequestParam("encryptedData") String encryptedData,
                                             @RequestParam("iv") String iv);

in spring-cloud-release F version
and i request sessionKey param is aaa+bbb
i check okhttp3 request log , sessionKey param has been change to aaa%2B%bbb
and app server accept sessionKey param is aaa+bbb

in spring-cloud-release G version
and i request sessionKey param is aaa+bbb
i check okhttp3 request log , sessionKey param has been change to aaa+bbb (no encode)
and app server accept sessionKey param is aaa bbb

how can i solve this problem?

thanks

@davidgoate
Copy link

We've also experienced this.

One thing that is not clear to me is that it would appear that not all URL encoding of params have stopped after upgrading from finchley.SR2 to greenwich.release, some characters still are.

using finchley before the + char appears to have been converted to %2B which , I thought was desirable, https://tools.ietf.org/html/rfc3986 (section 2.2. Reserved Characters) .

Using Greenwich, the + is now a literal +, but certain other characters (if present in the input string) are still encoded, e.g. ABC+&? -> ABC+%26%3F. Before (Finchley.SR2) this was ABC%2B%26%3F .

I can use java.net.URLEncoder#encode(java.lang.String, java.lang.String) prior to calling the feign client to encode any params values which might contain reserved characters but it's not clear to me whether it is advisable to do so in terms of the intended behaviour of feign (i.e. it feels like feign does take care of this for at least some chars ?, &).

For the time being, I've modified my calling code to first URL encode any strings with potential for reserved characters before calling feign

@kdavisk6 kdavisk6 self-assigned this Jan 23, 2019
@kdavisk6
Copy link
Member

I'm looking into this. It appears that we are not applying path and query encoding correctly.

@kdavisk6 kdavisk6 added bug Unexpected or incorrect behavior spring-cloud Issues related to Spring Cloud OpenFeign regression Bugs and issues related to unintended breaking changes labels Jan 23, 2019
@dragontree101
Copy link
Author

i create a new RequestInterceptor to encode all query param to solve this problem.

@Bean
  public RequestInterceptor feignRequestInterceptor() {
    return template -> {
      if (urlEncode) {
        template.queries().forEach((k, v) -> {
          template.query(k, Lists.newArrayList());
          template.query(k, v.stream().map(l -> URLEncoder.encode(l, StandardCharsets.UTF_8)).collect(
                  Collectors.toList()));
        });
        log.info("url is {}, urlEncode is {}", template.url(), urlEncode);
      }
    };
  }

i hope feign official can solve this problem next version, thanks

@kdavisk6
Copy link
Member

kdavisk6 commented Jan 24, 2019

I would caution blindly encoding this way. Only some of the encoding is incorrect and your approach will double encode if a value is handled correctly.

@kdavisk6
Copy link
Member

And one more important note, URLEncoder does not apply url encoding, it is used to encode form parameters and will incorrectly encode certain values. Use with caution.

@dragontree101
Copy link
Author

@kdavisk6 thanks!

kdavisk6 added a commit to kdavisk6/feign that referenced this issue Jan 29, 2019
Fixes OpenFeign#879

URI encoding introduced in Feign 10.x was refactored to be more in line
with URI and URI Template specifications respectively.  One change was to
ensure that certain reserved characters were not encoded incorrectly.

The result was that path segment specific reserved characters were being
preserved on the query string as well.  This change updates the `UriTemplate`
and `Expression` classes to recognize the segment of the URI that is being processed
and apply the segment specific encoding correctly.

One important change regarding the `+` sign.  Per the URI specification, a `+` sign
is allowed in both the path and query segments of a URI, however, handling of
the symbol on the query can be inconsistent.  In some legacy systems, the `+` is
equivalent to the a space.  Feign takes the approach of modern systems, where a
`+` symbol should not reprsent a space and is explicitly encoded as `%2B` when
found on a query string.

If you wish to use `+` as a space, then use the literal ` ` character or encode
the value directly as `%20`
@kdavisk6
Copy link
Member

PR #882 is now open. This PR attempts to correct this issue by making sure that each segment of the uri, be it the path or query, are correctly encoded based on the URI and URI Template specifications. With regards to the + symbol, when this appears on the query string, it will be encoded as %2B.

If your intention is to use + as a space, you must use the space character or encode it yourself as %20.

If you are interested in more detail about where all this comes from, I invite you to look at RFC 3986 - Appendix A. It describes what characters are acceptable on each segment of the uri.

kdavisk6 added a commit that referenced this issue Feb 1, 2019
* Adding URI segment specific encoding

Fixes #879

URI encoding introduced in Feign 10.x was refactored to be more in line
with URI and URI Template specifications respectively.  One change was to
ensure that certain reserved characters were not encoded incorrectly.

The result was that path segment specific reserved characters were being
preserved on the query string as well.  This change updates the `UriTemplate`
and `Expression` classes to recognize the segment of the URI that is being processed
and apply the segment specific encoding correctly.

One important change regarding the `+` sign.  Per the URI specification, a `+` sign
is allowed in both the path and query segments of a URI, however, handling of
the symbol on the query can be inconsistent.  In some legacy systems, the `+` is
equivalent to the a space.  Feign takes the approach of modern systems, where a
`+` symbol should not reprsent a space and is explicitly encoded as `%2B` when
found on a query string.

If you wish to use `+` as a space, then use the literal ` ` character or encode
the value directly as `%20`
velo pushed a commit that referenced this issue Oct 7, 2024
* Adding URI segment specific encoding

Fixes #879

URI encoding introduced in Feign 10.x was refactored to be more in line
with URI and URI Template specifications respectively.  One change was to
ensure that certain reserved characters were not encoded incorrectly.

The result was that path segment specific reserved characters were being
preserved on the query string as well.  This change updates the `UriTemplate`
and `Expression` classes to recognize the segment of the URI that is being processed
and apply the segment specific encoding correctly.

One important change regarding the `+` sign.  Per the URI specification, a `+` sign
is allowed in both the path and query segments of a URI, however, handling of
the symbol on the query can be inconsistent.  In some legacy systems, the `+` is
equivalent to the a space.  Feign takes the approach of modern systems, where a
`+` symbol should not reprsent a space and is explicitly encoded as `%2B` when
found on a query string.

If you wish to use `+` as a space, then use the literal ` ` character or encode
the value directly as `%20`
velo pushed a commit that referenced this issue Oct 8, 2024
* Adding URI segment specific encoding

Fixes #879

URI encoding introduced in Feign 10.x was refactored to be more in line
with URI and URI Template specifications respectively.  One change was to
ensure that certain reserved characters were not encoded incorrectly.

The result was that path segment specific reserved characters were being
preserved on the query string as well.  This change updates the `UriTemplate`
and `Expression` classes to recognize the segment of the URI that is being processed
and apply the segment specific encoding correctly.

One important change regarding the `+` sign.  Per the URI specification, a `+` sign
is allowed in both the path and query segments of a URI, however, handling of
the symbol on the query can be inconsistent.  In some legacy systems, the `+` is
equivalent to the a space.  Feign takes the approach of modern systems, where a
`+` symbol should not reprsent a space and is explicitly encoded as `%2B` when
found on a query string.

If you wish to use `+` as a space, then use the literal ` ` character or encode
the value directly as `%20`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected or incorrect behavior regression Bugs and issues related to unintended breaking changes spring-cloud Issues related to Spring Cloud OpenFeign
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants