UriUtils.encode should not skip already encoded strings

[Zordid]

I ended up here using feign because of a seemingly simple task: encode a String using RFC 3986.

Against the odds, this turns out to be quite complicated. Java fails because the URLEncoder is not for URIs, due to the Space=+...
Apache does not work, Guava does not have a decoder...

Now feign... I couldn't believe my eyes when my property based tests failed to properly encode the literal String "%07"...
I look into the code and find stuff like "skips already encoded strings"... :(

This is a very bad design decision in itself, but even worse: how does one now encode such a literal string? I cannot find an option to disable this (for me) unwanted behaviour...

Thanks!

[kdavisk6]

Much of Feign's uri-template handling adheres to RFC 6570, where states Section 1.2

URI Templates are similar to a macro language with a fixed set of
macro definitions: the expression type determines the expansion
process. The default expression type is simple string expansion,
wherein a single named variable is replaced by its value as a string
after pct-encoding any characters not in the set of unreserved URI
characters (Section 1.5).

In our library, we will ensure that all items are pct-encoded when present on a URI. This is different from URL encoding slightly, and as you are now aware, not consistently supported due to variances in server technology, age, and general misuse across applications. We have an entire section in our documentation dedicated to this issue, specifically around slashes and plus signs.

Feign makes a best effort to ensure that the resulting URI will pct-encode per RFC 6570. While we understand this may not work for every single scenario, we've found that it works for the vast majority of them. If you find that the results are not what you expect, you can override the handling by either implementing a custom Expander for the troublesome parameter, or implement a RequestInterceptor to change the entire URI before processing.

[jebeaudet]

I just hit the same problem as @Zordid today to find that a simple test test path parameter gets properly encoded to test%20test but if I pass test%20test, Feign will detect it as already encoded here and skip the encoding all together so I end up with the same test%20test in the end instead of the proper test%2520test.

I understand the "best effort" tentative here but with no easy way to disable this behavior, it's really unfortunate. With a RequestInterceptor, I have no way of determining if the test%20test in the template was already encoded by Feign or if it was determined to be already encoded. An Expander would work only on @Param. Is there any other option out there?

[jebeaudet]

OK I managed to get a Expander working, however, I still hit some issues with encoding path parameters myself for example, test = test within a path segment gets encoded to test%20=%20test but the method UriUtils::isEncoded detects this string as non encoded because of the = and double encode it. Having a = not encoded within a path segment is totally valid as there are different rules for path/query/fragment (I love this source for a easy to read reference).

I'm actually using org.springframework.web.util.UriUtils which correctly consider the differences in encoding depending if you're encoding a path segment, query, queryString, etc.

Feign cannot accurately guess if the passed string is encoded or not by just scanning it like it does right now. IMO, it should never consider them encoded (is there a use case where you get a method parameter that is already encoded?), or it should have a switch to tell feign that the parameter passed are already encoded, either at the parameter level of the client level.

WDYT?

Thanks

[kdavisk6]

It's been a little, but just in case anyone is still around, there's a general conflation of URL encoding vs URI encoding. @jebeaudet is correct that there are different encoding rules based on which part of the URL is being evaluated, like the path segment, fragment, etc...

When it comes to Feign, encoding is performed on URI templates, specifically Level 1 templates in general.

URI Templates are similar to a macro language with a fixed set of
macro definitions: the expression type determines the expansion
process. The default expression type is simple string expansion,
wherein a single named variable is replaced by its value as a string
after pct-encoding any characters not in the set of unreserved URI
characters (Section 1.5)

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
"SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
document are to be interpreted as described in [RFC2119].

This specification uses the Augmented Backus-Naur Form (ABNF)
notation of [RFC5234]. The following ABNF rules are imported from
the normative references [RFC5234], [RFC3986], and [RFC3987].

 ALPHA          =  %x41-5A / %x61-7A   ; A-Z / a-z
 DIGIT          =  %x30-39             ; 0-9
 HEXDIG         =  DIGIT / "A" / "B" / "C" / "D" / "E" / "F"
                   ; case-insensitive

 pct-encoded    =  "%" HEXDIG HEXDIG
 unreserved     =  ALPHA / DIGIT / "-" / "." / "_" / "~"
 reserved       =  gen-delims / sub-delims
 gen-delims     =  ":" / "/" / "?" / "#" / "[" / "]" / "@"
 sub-delims     =  "!" / "$" / "&" / "'" / "(" / ")"
                /  "*" / "+" / "," / ";" / "="

 ucschar        =  %xA0-D7FF / %xF900-FDCF / %xFDF0-FFEF
                /  %x10000-1FFFD / %x20000-2FFFD / %x30000-3FFFD
                /  %x40000-4FFFD / %x50000-5FFFD / %x60000-6FFFD
                /  %x70000-7FFFD / %x80000-8FFFD / %x90000-9FFFD
                /  %xA0000-AFFFD / %xB0000-BFFFD / %xC0000-CFFFD
                /  %xD0000-DFFFD / %xE1000-EFFFD

 iprivate       =  %xE000-F8FF / %xF0000-FFFFD / %x100000-10FFFD

This ruleset governs all basic URI encoding in Feign and typically why we get issues stating "encoding is broken".

It is in-fact, not broken, just not necessarily using the ruleset you may be expecting.

The URI template specification addresses the gap between URI encoding and URL encoding by defining additional Levels of encoding, requiring the use of certain special indicator characters in the URI template to indicate which ruleset to use. This includes the use of / to indicate path segment rules, # to indicate fragment rules, ? for query segment rules, etc...

With all that said, Feign supports the most basic of rulesets and does so to as stated, using a best-effort encoding check.

I do understand that may not be ideal. The good news is, we're open source, so we are open to contributions in this space. One solution is to support all levels defined in RFC6570. This would allows users to be more explicit in how expansion should be handled.

If you're still reading this, I hope this helps explain why things are the way they are now and if you are inspired, please submit a pull-request or help build a better solution here.

[Zordid]

I think it all boils down to failing for one important aspect of clean code: if you have a method named encode, it should encode what you call it with. It should not act like a function I would call ensureEncoded... Having code that tries to be smart and just ensure the thing we pass it is encoded does not help people who need a function that actually encodes whatever you pass in.

[kdavisk6]

And this is core of the issue, expectations. Your expectation is reasonable, valid and sensible. However, historically, users of this library have expressed the opposite, that Feign should accept already encoded strings and not double encode: See #264 , #225, #693, #675, #637, #607, #735, #503, and #429

The approach decided was to detect already pct-encoded URI values and implemented in #778, with the understanding that certain reserved characters like =, &, %, and the like would be skipped. This was the balance we struck with dealing with the most common use cases.

While we aim to be as flexible as possible, this is one area where the feedback given dictated the compromise that is now frustrating you.

I'd like to help drive this to a resolution. We could consider renaming the function encodeIfNecessary and update the javadoc to be explicit about it's intention. Would this be acceptable? Do you have any other approaches we could consider?

[jebeaudet]

Thanks for feedback.

To me though, trying to be smart kind of makes the problem worse, since encoding works for simple unencoded parameters like foo foo but will not work for parameters that seems encoded to Feign like foo%20foo which is obviously different. It obscures a lot the library behavior.

To me, the sensible solution is having an encoded parameter like here. When interacting with an api, the caller should know and have to care if parameters are already encoded or not.

I've fixed it on my end using a custom expander. Here is my IT setting my expectations if it can help the discussion : https://gist.github.com/jebeaudet/603f4efb15e45257b60cee3d0d60cb63.

[velo]

I tend to agree that "trying to be smart" causes problems.

Maybe we need to rework this and provide a default implementation and the smart ones as alternatives....

Gonna break compatibility, but that is what major releases are for

[kdavisk6]

I’ll take ownership of this issue. I’ve reviewed the examples and will spend the next few days working through options to bring back to this group

[Zordid]

I think it would be good to offer the "smart" "keep existing encodings" function as a separate one with a different name, but offer just a plain "encode this string" function that does what it should: create an encoded version of its input, no matter what that might be. This is in the end what got me into trouble: I am not in control at all on the input I need to encode so I write a property test that every possible string needs to pass encoding/decoding and be the same again. Which horribly failed on the existing function. ;-)

[kdavisk6]

I've looked at the provided test and I've created a new Unit Test that uses those examples and then provides the expected results using pct-encoding for all values and the test case does successfully meet the expectations with our current functionality that does not double encode what appears to be already encoded values:

  @ParameterizedTest
  @MethodSource("provideValuesToEncode")
  void testVariousEncodingScenarios(String input, String expected) {
    assertThat(UriUtils.encode(input, UTF_8)).isEqualTo(expected);
  }

  private static Stream<Arguments> provideValuesToEncode() {
    // original, expected pct-encoded value
    return Stream.of(
        Arguments.of("foo", "foo"),
        Arguments.of("foo bar", "foo%20bar"),
        Arguments.of("foo%20bar", "foo%20bar"),
        Arguments.of("foo%2520bar", "foo%2520bar"),
        Arguments.of("foo&bar", "foo%26bar"),
        Arguments.of("foo& bar", "foo%26%20bar"),
        Arguments.of("foo = bar", "foo%20%3D%20bar"),
        Arguments.of("foo   ", "foo%20%20%20"),
        Arguments.of("foo/bar", "foo%2Fbar"),
        Arguments.of("foo!\"/$%?&   *( )  _%20^¨  >`:É.   ',.  é ;`^ ¸< nasty stuff here!",
            "foo%21%22%2F%24%25%3F%26%20%20%20%2A%28%20%29%20%20_%2520%5E%C2%A8%20%20%3E%60%3A%C3%89.%20%20%20%27%2C.%20%20%C3%A9%20%3B%60%5E%20%C2%B8%3C%20nasty%20stuff%20here%21"));

  }

Where I think items may be not what is expected, based on the feedback in this issue, are the use cases of foo%20bar and foo%2520bar. These could be seen as not meeting expectations and instead should return foo%2520 and foo%252520bar, essentially encoding the % character. This could be seen as double encoding, but is the correct answer if we follow RFC6570

Note that the percent character ("%") is only allowed
as part of a pct-encoded triplet and only for reserved/fragment
expansion: in all other cases, a value character of "%" MUST be pct-
encoded as "%25" by variable expansion.

Based on this test case, we should consider removing the pct-encoding detection and deal with the potential double-encoding situations. I'll work on a Pull Request to remove that detection by default, but enable it if desired.

However, folks may still have questions, so let's deep dive into how Feign deals with URI templates:

---

How URI Templating works in Feign and how it affects encoding

Feign expands URI Templates using the ruleset defined in RFC 6570 - Section 1.2, Level 1 Templates and follows the same documents variable expansion process.

No evaluation of the URI before expansion is done to determine where an expression lives within the URI.
This is intentional as a URI template is not considered a URI until it is expanded

Section 1.4 - Limitations

URI Templates are not URIs: they do not identify an abstract or
physical resource, they are not parsed as URIs, and they should not
be used in places where a URI would be expected unless the template
expressions will be expanded by a template processor prior to use.
Distinct field, element, or attribute names should be used to
differentiate protocol elements that carry a URI Template from those
that expect a URI reference.

Due to this limitation, if specific expansion is required, the expression must include the appropriate prefix to inform the template processor what character sets should be encoded. Let's walk use the example presented earlier in this thread, path specific expansion.

How to control expansion

Consider the following:

/{var}/here, with var being test=test.

Using Simple expression resolution the template would resolve to /test%3Dtest/here, as simple resolution calls for pct-encoding for all reserved characters.

If we want to avoid pct-encoding reserved characters, we need to add the appropriate prefix to our expression to tell the template processor what we want.

Using Reserved resolution the same template would be defined as /{+var}/here and would resolve to /test=test/here. This is due to the reserved resolution being identical to simple string expansion except that the substituted values may also contain pct-encoded triplets and characters in the reserved set.

Please Note: Feign supports Simple and Path Style Expansion only as of 13.x. We have discussed adding support for the additional expansion types and levels

Summary

What I'm trying to convey is that when dealing with URI Templates specifically, encoding of expanded template parameters is controlled by the prefix specified in the expression definition and not by parsing the entire template to identify the segment in the URI where the template exists and a URI template is not a URI until it is expanded by a template processor.

Much of what I think causes confusion here is that we, as writers of the template, know what we the resulting URI should be, and know which part of the URI we placed our expression, so we expect the template processor to do the same, but a URI template is not a URI and the template processor is forced to evaluate what we give based entirely on how the template is constructed.

If you made this this far, I hope I've been able to provide more context and transparency in our expansion capabilities, reasons behind our decisions, and a little bit more about the difference between URI templates, URIs and URLs.