ChuckerTeam / chucker

🔎 An HTTP inspector for Android & OkHTTP (like Charles but on device)
Apache License 2.0
3.97k stars 351 forks source link

Solution: method skipPaths on builder does not work as expected, issue #1237 #1290

Closed Ridje closed 1 month ago

Ridje commented 1 month ago

:page_facing_up: Context

The Issue occurs when we add path segments delimited by '/' as part of the filtering mechanism, and HttpUrl treats them as a single, large path segment.

:pencil: Changes

I added methods to safely add skips_path.

Why not use HttpUrl::addPathSegments?

The problem with addPathSegments is that it replaces a leading '/' with an empty path segment, turning '/' into '//'. This causes issues for HttpUrl in general.

For example

Both MockWebServer::url and HttpUrl.Builder::resolve don't handle '//' segments properly.

private fun executeRequestForPath(
        okHttpClient: OkHttpClient,
        path: String,
        responseBody: String,
    ) {
        val httpUrl =
            HttpUrl.Builder()
                .scheme("https")
                .host("testexample.com")
                .addPathSegments(path)
                .build()

        val request = Request.Builder().url(server.url(httpUrl.encodedPath)).build()
        server.enqueue(MockResponse().setBody(responseBody))
        okHttpClient.newCall(request).execute().readByteStringBody()
  }
Path request.url
"" http://localhost:52347/
" " http://localhost:52347/%20%20%20%20
"/skip/path" http://skip/path
"//skip//path" http://skip//path

I consider these results problematic for our tests as the logic is changed here.

Even if we found a workaround, user inputs would still be transformed from '/path/to/test' to '//path/to/test', which leads to unexpected behavior.

Comparison table for user input conversion using HttpUrl method and custom extension

User filter with HttpUrl::addPathSegments with addNonBlankPathSegments
"" / /
" " /%20%20%20%20
"example" /example /example
"www.example.com/skip/path" /www.example.com/skip/path /www.example.com/skip/path
"example.com/skip/path" /example.com/skip/path /example.com/skip/path
"90" /90 /90
"https://example/" /https://example/ /https:/example
"/skip/path" //skip/path /skip/path
"/skip//" //skip// /skip
"http://localhost:8080/skip/path/ext" /http://localhost:8080/skip/path/ext /http:/localhost:8080/skip/path/ext
"//skip//path" ///skip//path// /skip/path

Should we consider removing full URLs with hosts from tests, since there is a separate field for that purpose - 'skipDomain'?

:no_entry_sign: Breaking

Duplicated and empty path segments will be ignored once this change is applied.

Ridje commented 1 month ago

Updated:

  1. Modified commentary.
  2. Updated CHANGELOG.MD and listed this fix in it.