introduce different request matchers to match recorded request and WebResourceRequest #27
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It turned out that the JS functions hand over "undefined", which leads to the JSONObject creation failing with an exception. Now we catch that (or any other incompatible string) and simply create an empty object.
This refactoring is a preparation to allow different matchers to connect the recordedRequests with the WebResourceRequest. It introduces an interface to allow different matchers to be easily exchanged and adds the already existing matching by URL as a first matcher.
Since we want to introduce a new dependency on it's newest version, it's time to update all the other dependencies. Therefore we update to Java 17, update the gradle and gradle plugin version and adapted deprecated gradle functionality. This change also requires to replace `targetSdk`, which is removed from the AGP for the `com.android.library` plugin, with a targetSdk for `testOptions` and `lint`.
Matching by URL doesn't work properly for 2 or more parallel GraphQL queries to the same url. These requests are send to the same url and only differ by the body that is send. If the second request is send to early, before the first request is matched to it's body, the first request will get the body of the second one (because of the `findLast` in the match function) and therefore return something unexpected. The second request will work since it is using the expected body. Therefore a new matcher is required. It will generate a uuid, add it as a header to the request, before sending it. This way the request and body can be properly matched. A downside of this approach is that neither form submission nor CORS requests will work. The reason for this is that the browser engine doesn't allow to add custom headers for from submissions. Regarding CORS: For XHR and fetch the browser engine knows about the additional header and adds it as a value to the preflight `Access-Control-Request-Headers` header, but even though the preflight is successful, it doesn't return that header as allowed. So the browser engine won't send the CORS request but let's it fail. Cleaning up the headers when intercepting the request (and matching it to it's body) doesn't work, because the browser engine doesn't know about the cleanup and, still thinking the header is not allowed, blocks the CORS request. Therefore this solution checks if it's a request to the same origin before adding the header.
The old logic with the uuid in a custom header didn't work for CORS requests, but adding the uuid as a query param should work. Downside is that the uuid will look different in the web inspector. So we restructure the RequestUuidMatcher into an abstract class, with two implementations RequestUuidInHeaderMatcher.kt (with the previous logic) and RequestUuidInQueryParamMatcher.kt with the new logic. Then the user of the library can decide which one to use.
The `startWith` check could return false positives. E.g. if the origin is `https://example.com` and we check a request to `https://example.com.evil.org`, it would return true. This shouldn't be a security issue (in contrast to the evil url in the example). But it could simply wrongfully miss a CORS request, which would then not be executed because of the additional, not allowed header.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a rather big PR and it is best reviewed commit by commit.
The first commit
catch failing JSONObject creationis a simple fix. For some requests there was an error in the logsJSONException: Value undefined of type java.lang.String cannot be converted to JSONObject. It turned out that the JS functions hand over "undefined", which leads to the JSONObject creation failing with an exception. Now we catch that (or any other incompatible string) and simply create an empty object.(This could be a separate PR, but I didn't want to create two PRs and having to rebase the second one before merging. Even though that don't build on each other, they might create merge conflicts. But if you prefer 2 PRs, I can still do that.)
The other 4 commits (further named as 1., 2., 3. and 4.) are contributing to fix one issue: Matching the recorded request to the WebResourceRequest. Currently that is simply done url, so the last recorded request for the same url is used. But if there are two parallel requests to the same url that just differ by the body, like it is common with GraphQL where the query is in the body, it doesn't work reliable anymore.
Basically we have a race condition, if the first request is matched to it's recorded request first, all works. But if the second request is added to the recordedRequests first, the first request is mapped to the second recordedRequest, so basically the first request is sent with the body of the second one. With GraphQL this means, the first request comes back with a completely unexpected response. The second request works fine, because it's matched with it's proper recorded request.
So we have to find a new way of fixing this.
The 1. commit is just about refactoring. I wanted to preserve the current implementation, but move it to it's own class so that it can be easily replaced later. I also added the possibility to change the matcher in the constructor of
RequestInspectorWebViewClient, but used the current implementation as a default. That means it will still work the same for all current users of this library.The 2. commit is preparation for the two following commits. Since I want to introduce a new dependency (androidx.core:core-ktx), I updated the existing dependencies to current versions first.
The 3. commit implements the first new Matcher. Basically the idea is to generate a uuid that can be used to identify and match the two corresponding requests. Now we need a way to add the uuid to both, the recordedRequest (easy) and the WebResourceRequest. The way it is done is by adding a custom header
x-request-inspector-idthat contains the uuid. This works fine forfetchandxhr, but not forform submitsbecause we are not allowed to add a header there.Another downside is also that it doesn't work for CORS requests, because the browser engine will send a preflight to check if the request is allowed. The server doesn't know about the custom header, so it will not include it in the
Access-Control-Allow-Headersresponds header. Therefore the browser engine will not send the "real" CORS request. Cleaning up the request by removing the custom header after matching the WebResourceRequest and recorded request doesn't help, because the browser engine see the request before we can intercept it inRequestInspectorWebViewClientand therefore already knows about the custom header (see pic below).The "fix" for this is, that we only add the custom header if the origin of the WebView content and the request are the same, because then it's not a CORS request. But this means that we can't access the body for CORS requests when we intercept them.
The 4. commit deals with the downsides of the 3. commit. Instead of adding a custom header, we add the uuid as a query param to the url. This also works for
form inputand CORS requests. We also clean up the request url after matching, so the server can't see any difference.The downside of this approach is that it pollutes the Inspect view far more than the previous solution (the custom header is also visible there, but only if you look into the details). This means that this library can't be fully transparent or only barely visible anymore.
Edit:
I just added another commit that makes the origin check more reliable.