This ensures no illegal cookies are send to okhttp

[erikpoort]

Motivation

When a website in a ReactNative WebView sets a cookie with an illegal
character, this cookie will automatically be added to any request to the
same domain.

This happens through:
BridgeInterceptor.java (l.84)
ReactCookieJarContainer.java (l.44)
JavaNetCookieJar.java (l.59)
ForwardingCookieHandler.java (l.57)
ForwardingCookieHandler.java (l.168)
CookieManager.java (l.39)

The BridgeInterceptor.java then tries to set a Cookie header, which
validates both keys and values, and then crashes.
okhttp3.6.0 Headers.java (l.320)

This fix will strip illegal characters from any cookie that is being
passed to the okhttp request.

Test Plan

To demonstrate how to crash the app, you can find an example app here:
https://github.com/erikpoort/react-native-test-illegal-cookie

Or you can load the following url into a webview: https://invalidcookietest.us.dev.monkapps.com/
Press the 'Set cookie' button.
Then try to fetch the same url.

Release Notes

[ANDROID] [BREAKING] [ReactCookieJarContainer.java] - I'm filtering cookies containing illegal characters from any request.

[erikpoort]

Fixes #17568

[erikpoort]

Hey there,

A while ago I opened a PR: #17569
The bot then told me after a few weeks it would be best to rebase to the latest master and see if it still happens. I accidentally did a merge, which made the PR hard to read, so I reopened this one.

The bot also told me it would be best to tag someone to take a look.
@AndrewJack I'm tagging you, because you created the file I changed.
@emilsjolander I'm tagging you, because you made the changes on the build.gradle deciding which okhttp to use.

Could you guys please take a look?

Thanks!

[erikpoort]

After taking a better look at the guys I tagged, Andrew is apparently working somewhere else, and
Emil seems to be working on different projects as he's inactive on GitHub.

So I'm tagging you guys: @amnn and @hramos ! I saw you have both reviewed and accepted some networking related PRs recently.

[hramos]

Thanks for the PR! Any chance you can add a test for this?

[erikpoort]

Hi @hramos,

Thanks for the reply!
The easiest way for me to explain the crash, is with an example. So I have updated the Test Plan in the PR, to point to a GitHub repo which demonstrates the crash.

I'm aware the server is setting cookie values that are outside of the accepted range, but sometimes you can't control what a server is doing!

Another solution could be to urlencode the invalid cookies, instead of ignoring them, please let me know what you think :)

[erikpoort]

Hi @hramos,
I was wondering if the test is clear, or if I can help you in another way to check this PR.
Thanks!

[hramos]

For an example, see my PR at #18382 where I add a regression test to ensure the issue fixed in 4f8328b does not get re-introduced to the codebase.

The test plan there should give you a good idea of how to set this up.

[erikpoort]

Hey @hramos,
Thanks for the pointer!
I just added regression tests.

Not sure why circleci checks don't pass, seems like the cache is corrupt?

Locally all tests pass:

erikpoort:react-native erik$ ./scripts/run-android-local-unit-tests.sh 
Warning: The test suite expects buck version d743d2d0229852ce7c029ec257532d8916f6b2b7 to be installed
Fetching dependencies...
Action graph will be rebuilt because files have been added or removed.
Building: finished in 0.0 sec (100%) 52/52 jobs, 0 updated, 0.0% cache miss
  Total time: 0.5 sec
Running unit tests...
Building: finished in 1.3 sec (100%) 266/266 jobs, 4 updated, 0.8% cache miss
  Total time: 1.4 sec
Testing: finished in 55.2 sec (154 PASS/0 FAIL)    
...    
PASS      1.9s  4 Passed   0 Skipped   0 Failed   com.facebook.react.modules.network.ReactCookieJarContainerTest
...    
Updated test logs: buck-out/log/test.log
TESTS PASSED

I'm not used to working with mockito, so let me know if you need any changes! 👍

[hramos]

Sorry for the trouble! Circle had some issues. If you rebase past 6f6084d your tests should pass.

[hramos]

Thanks for adding the regression test 👍

[erikpoort]

I merged the latest master, which fails on something (in my eyes) unrelated to my changes.
But it did stop my tests from being ran, can you restart those test?

[erikpoort]

Hey @hramos I'm not sure what is causing the circleci fail, do you know how I can verify that my regression tests are passing?

[hramos]

Looks like Circle is going through an outage right now. They had a similar outage, affecting macOS jobs, the other day.

[erikpoort]

@hramos Shall I wait for it to be resolve and then rebase on master, or can you review the PR as is? :)

[hramos]

Failing tests will only hold back the PR from being merged. It shouldn't hold back any reviews.

I should also make it clear that I'm not actively reviewing pull requests (as evidenced by the number of open PRs). I'm around to support the landing of PRs that require it (i.e. the import failed), as well as to make sure new PRs are in a good state so that the community may be able to review them.

[erikpoort]

Allright, thanks for the pointers!
Maybe @janicduplessis can check it out? You seem a bit more active landing PRs :)

[swankjesse]

Nice PR. Y’all should merge this.

[facebook-github-bot]

@erikpoort I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[erikpoort]

@hramos I don't know if you've seen it, but the PR has been reviewed by @swankjesse from okhttp :) (thanks!! 👍 ) I have also rebased on the latest master.

You mentioned: "Failing tests will only hold back the PR from being merged.", is there any way I can help to resolve these failing tests? They seem unrelated to my code.

[facebook-github-bot]

@erikpoort I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[facebook-github-bot]

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[ghost]

Does this affect the cookies sent by a fetch request inside of an app(not inside webview)?

Cookies were automatically being sent from my fetch calls, but it stopping doing so when I did an upgrade of react-native from 0.55 to 0.56 around 2 weeks back.

[erikpoort]

No, fetch is using okhttp3 and doesn't save cookies with illegal characters. Only the android webview saves those cookies. RN combines okhttp3 and android webview cookies to do a new request. Before this PR, those webview cookies weren't filtered and okhttp3 would respond by throwing an exception on those illegal characters, thus crashing the app.