Retrofit 2.5 fails to validate paths with strings including some url encoded data

[carterkozak]

java.lang.IllegalArgumentException: @Path parameters shouldn't perform path traversal ('.' or '..'): foo+bar

	at retrofit2.RequestBuilder.addPathParam(RequestBuilder.java:113)
	at retrofit2.ParameterHandler$Path.apply(ParameterHandler.java:99)
	at retrofit2.RequestFactory.create(RequestFactory.java:108)
	at retrofit2.OkHttpCall.createRawCall(OkHttpCall.java:190)
	at retrofit2.OkHttpCall.execute(OkHttpCall.java:173)

These are valid parameters, they just need to be encode prior to being sent to a server.

[iamdanfox]

2.5.0:

  /**
   * Matches strings that contain {@code .} or {@code ..} as a complete path segment. This also
   * matches dots in their percent-encoded form, {@code %2E}.
   *
   * <p>It is okay to have these strings within a larger path segment (like {@code a..z} or {@code
   * index.html}) but when alone they have a special meaning. A single dot resolves to no path
   * segment so {@code /one/./three/} becomes {@code /one/three/}. A double-dot pops the preceding
   * directory, so {@code /one/../three/} becomes {@code /three/}.
   *
   * <p>We forbid these in Retrofit paths because they're likely to have the unintended effect.
   * For example, passing {@code ..} to {@code DELETE /account/book/{isbn}/} yields {@code DELETE
   * /account/}.
   */
  private static final Pattern PATH_TRAVERSAL = Pattern.compile("(.*/)?(\\.|%2e|%2E){1,2}(/.*)?");

  //...

  void addPathParam(String name, String value, boolean encoded) {
    if (relativeUrl == null) {
      // The relative URL is cleared when the first query parameter is set.
      throw new AssertionError();
    }
    String replacement = canonicalizeForPath(value, encoded);
    String newRelativeUrl = relativeUrl.replace("{" + name + "}", replacement);
    if (PATH_TRAVERSAL.matcher(newRelativeUrl).matches()) {
      throw new IllegalArgumentException(
          "@Path parameters shouldn't perform path traversal ('.' or '..'): " + value);
    }
    relativeUrl = newRelativeUrl;
  }

2.4.0

  void addPathParam(String name, String value, boolean encoded) {
    if (relativeUrl == null) {
      // The relative URL is cleared when the first query parameter is set.
      throw new AssertionError();
    }
    relativeUrl = relativeUrl.replace("{" + name + "}", canonicalizeForPath(value, encoded));
  }

[stale[bot]]

This issue has been automatically marked as stale because it has not been touched in the last 60 days. Please comment if you'd like to keep it open, otherwise it'll be closed in 7 days time.

[Shedon]

I'd like the issue to be fixed

[ash211]

Because of this issue this repo is still on retrofit 2.4.0 -- https://github.com/palantir/conjure-java-runtime/blob/cf9e9c7f484c0e1d5e5205134a2d29bee7ca69cb/versions.props#L51-L52

But now 2.4.0 has a known CVE: https://nvd.nist.gov/vuln/detail/CVE-2018-1000844 which is being flagged in internal vulnerability scanners, so we need to get off 2.4.0 shortly.

[bmarcaur]

No longer an issue with #1853, CJR no longer uses retrofit.