Skip to content

This ensures no illegal cookies are send to okhttp - #17569

Closed
erikpoort wants to merge 381 commits into
react:masterfrom
MediaMonksForks:hotfix/filter-webview-cookies
Closed

This ensures no illegal cookies are send to okhttp#17569
erikpoort wants to merge 381 commits into
react:masterfrom
MediaMonksForks:hotfix/filter-webview-cookies

Conversation

@erikpoort

Copy link
Copy Markdown

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

I don't know how to create a testplan for this, as the cookies are loaded through the android WebView.

Release Notes

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

@facebook-github-botfacebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Jan 12, 2018
@erikpoort

Copy link
Copy Markdown
Author

Fixes: #17568

davidaurelioand others added 27 commits January 31, 2018 03:04
Summary:
We want applying deltas to be an atomic operation, from incrementing the delta message ID to updating the relevant maps.
This is a simple approach to synchronize the corrsponding method.
We will probably need to go with a more sophisticated approach, that makes sure that deltas are applied in order. That would also allow us to lock only on writes.
Reviewed By: kathryngray
Differential Revision: D6846560
fbshipit-source-id: 175a80b4e39223883e397d75e20109fc12a2a878
Differential Revision: D6856807
fbshipit-source-id: 36c527ee00eef56a5912ad6e4233d6cd61cb5170
Summary:
So in v0.52.0 space-evenly is introduced but not yet implemented (1050e0b by woehrl01). This pull request implements the space-evenly.
Manual Testing.
![notes marker](https://i.imgur.com/IXmezVY.png)
[IOS] [FEATURE] [Yoga] Adding space-evenly on justify-content in iOS
[ANDROID] [FEATURE] [Yoga] - Adding space-evenly on justify-content in Android
Closesreact#17805
Differential Revision: D6858294
Pulled By: shergin
fbshipit-source-id: 7a705ca05f58603ef4588e1bfd16c16a78f8a390
Summary:
This makes RCTTouchHandler follow the same logic when sending touch event
coordinates as `UIManager.measure`.
This is important as UIManager.measure is used in `Touchable.js` aka the mother
of all touch events in RN.
In particular, this will make touch events correctly handled for places where RN is embedded into other screens.
Reviewed By: shergin
Differential Revision: D6838102
fbshipit-source-id: 70cad52606ea931cb637d8aa2d4845818eea0647
Summary: Closesreact#17794
Differential Revision: D6850015
Pulled By: hramos
fbshipit-source-id: bd230a5aa6fe14fb760f7b0c5f0989bf6ee1e8ea
Summary:
Trivial.
Special thanks to janicduplessis
Created from Diffusion's 'Open in Editor' feature.
Reviewed By: fkgozali
Differential Revision: D6863932
fbshipit-source-id: d40a30271adc5c8d47149ab920e2ac11158ab756
Summary:
No logic change here. Part of a plan to consolidate CI-only files amongst .circleci and bots/ directories.
Closesreact#17807
Differential Revision: D6865976
Pulled By: hramos
fbshipit-source-id: 48607a80dcf8cac1c3c033c18bf5d6dd4cd8e6bf
Summary: Closesreact#17808
Differential Revision: D6866937
Pulled By: hramos
fbshipit-source-id: 34c9aa93a5274b57da81eb5bbece2ce3e5296846
Reviewed By: richardjrossiii
Differential Revision: D6866064
fbshipit-source-id: a44828cedb5045b6c97179fb30ffd8c8dafcbfbe
Reviewed By: pakoito
Differential Revision: D6807480
fbshipit-source-id: d71f2cecd882c47e79bb71dfb9d03d3597fa4068
Reviewed By: mhorowitz
Differential Revision: D6846268
fbshipit-source-id: 2f750a05ee63a744e7f64b3fe67736df3614ef3d
Summary:
Always build the JavaScript bundle, to ensure failures here are surfaced regardless of failures that may happen earlier in this workflow
Closesreact#17824
Differential Revision: D6872794
Pulled By: hramos
fbshipit-source-id: bab3b9cfa6cecb578e9a3cffae27e1ce355588d2
Summary:
Even if we don't support this prop yet, we have to expose this in RCTUITextView to conform the contract between ViewManager and ShadowView.
Same story as D6842304.
Depends on D6842304.
Reviewed By: fmoo
Differential Revision: D6869750
fbshipit-source-id: 9b35f1a38040319ec66f1ec12f97ac739f8b204f
Reviewed By: wwalser
Differential Revision: D6858376
fbshipit-source-id: a9ff9c71cb4ad56a4f5af73a4e86de52ddf75700
Reviewed By: gaearon
Differential Revision: D6875052
fbshipit-source-id: 516f46f1b78bd8ca3323ba119d3afda491d76497
Summary:
This PR fixes the issue react#12237 relative to react-native-git-upgrade.
When the user adds new files to the .gitignore file, those files are deleted during the upgrade process because it starts by creating a fresh .gitignore file without user's modification, so the user's ignored files are no longer ignored and they are deleted during the upgrading process.
The best solution I've found to keep the user's ignored files... ignored, consists in appending the content of the .gitignore file in the `<TEMP DIR>/.git-rn/info/exclude` file.
The user's ignored files are now ignored at the repository level, they no longer interfere with the upgrade process.
Reminder: The tool uses a temporary local Git repository generated on the fly, so we can do whatever we want in it.
- Publish react-native-git-upgrade to sinopia
- `npm install -g react-native-git-upgrade`
- Init a new project with an old version: `react-native init MyApp --version=0.50.0`
- Create a new file named `dummy-ignored-file.json` in the project's root
- Append `dummy-ignored-file.json` to the `.gitignore` file
- Run `react-native-git-upgrade`
👉 The `dummy-ignored-file.json` is not deleted.
[CLI][BUGFIX][react-native-git-upgrade] - Keep the user's ignored files while upgrading
<!--
Help reviewers and the release process by writing your own release notes
**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**
CATEGORY
[----------] TYPE
[ CLI ] [-------------] LOCATION
[ DOCS ] [ BREAKING ] [-------------]
[ GENERAL ] [ BUGFIX ] [-{Component}-]
[ INTERNAL ] [ ENHANCEMENT ] [ {File} ]
[ IOS ] [ FEATURE ] [ {Directory} ] |-----------|
[ ANDROID ] [ MINOR ] [ {Framework} ] - | {Message} |
[----------] [-------------] [-------------] |-----------|
[CATEGORY] [TYPE] [LOCATION] - MESSAGE
EXAMPLES:
[IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
[ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
[CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
[DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
[GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
[INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Closesreact#17393
Differential Revision: D6839856
Pulled By: hramos
fbshipit-source-id: e4e9d759b59790e3cbc52408cef8314bf0a9c772
Summary:
The problem was recently introduced in the last refactoring of the Text module.
There are two problem actually:
(1) Compare this current code with stable version.
In the stable version the event is only triggered here:
https://github.com/facebook/react-native/blob/0.52-stable/Libraries/Text/RCTTextField.m#L132-L140
In the new version the event is triggered in two places (which is obviously wrong).
(2) Executing `[_eventDispatcher sendTextEventWithType:RCTTextEventTypeChange:...]` and _onChange() actually do the same thing.
Historically, the single-line <TextInput> used the first approach and multi-line used second one. When we unify the logic, mixed both of them, which was apparenly wrong.
So, we have to remove another call of `[_eventDispatcher sendTextEventWithType:RCTTextEventTypeChange:...]`.
The the future we have to completly remove usage of `_eventDispatcher` from this component.
Depends on D6854770.
Reviewed By: fkgozali
Differential Revision: D6869678
fbshipit-source-id: 6705ee8dda2681ae184ef6716238cc8b62efeff1
Summary:
Documentation only.
Created from Diffusion's 'Open in Editor' feature.
Reviewed By: sahrens
Differential Revision: D6878890
fbshipit-source-id: 23f58246625ab6664bc3dcd4490f24ee50e410c8
Reviewed By: TheSavior
Differential Revision: D6869985
fbshipit-source-id: 836134ae0919d49b161d1a75d5743e977e6eb3f4
Reviewed By: davidaurelio
Differential Revision: D6819226
fbshipit-source-id: 6436855cc5dce9ce7922d89df99c3cb90abb095e
Reviewed By: emilsjolander
Differential Revision: D6856812
fbshipit-source-id: e4724d80702cc75c1894e348e137b24e663573d2
Summary:
Documentation only. Actual behaviour was never changed.
Created from Diffusion's 'Open in Editor' feature.
Reviewed By: sahrens
Differential Revision: D6869466
fbshipit-source-id: 6e964433bb2b04b288736a3f01244285bc8c3fe8
Reviewed By: sahrens
Differential Revision: D6885608
fbshipit-source-id: c153fcb5c2552982481d8af8b9755ae035e9b293
…h on closed connection
Summary:
This PR includes the same changes made in react#16541, for addressing issues react#11853/react#15724. It adds upload progress updates for uploads with any request body type, and not just form-data.
Additionally, this PR also includes a commit for fixing an `IllegalStateException` when a user's connection gets closed or times out (issues react#10423/react#11016). Since this exception was occurring within the progress updates logic, it started being thrown more frequently as a result of adding progress updates to all uploads, which was why the original PR was reverted.
To test the upload progress updates, run the following JS to ensure events are now being dispatched:
```
const fileUri = 'file:///my_file.dat';
const url = 'http://my_post_url.com/';
const xhr = new XMLHttpRequest();
xhr.upload.onprogress = (event) => {
console.log('progress: ' + event.loaded + ' / ' + event.total);
}
xhr.onreadystatechange = () => {if (xhr.readyState === 4) console.log('done');}
console.log('start');
xhr.open('POST', url);
// sending a file (wasn't sending progress)
xhr.setRequestHeader('Content-Type', 'image/jpeg');
xhr.send({ uri: fileUri });
// sending a string (wasn't sending progress)
xhr.setRequestHeader('Content-Type', 'text/plain');
xhr.send("some big string");
// sending form data (was already working)
xhr.setRequestHeader('Content-Type', 'multipart/form-data');
const formData = new FormData(); formData.append('test', 'data');
xhr.send(formData);
```
To test the crash fix:
In the RN Android project, before this change, set a breakpoint at `mRequestBody.writeTo(mBufferedSink);` of `ProgressRequestBody`, and wait a short while for a POST request with a non-null body to time out before resuming the app. Once resumed, if the connection was closed (the `closed` variable will be set to true in `RealBufferedSink`), an `IllegalStateException` will be thrown, which crashes the app. After the changes, an `IOException` will get thrown instead, which is already being properly handled.
As mentioned above, includes the same changes as react#16541, with an additional commit.
[ANDROID] [BUGFIX] [XMLHttpRequest] - Added progress updates for all XMLHttpRequest upload types / fix crash on closed connection
Previously, only form-data request bodies emitted upload progress updates. Now, other request body types will also emit updates. Also, Android will no longer crash on certain requests when user has a poor connection.
Addresses issues: 11853/15724/10423/11016
Closesreact#17312
Differential Revision: D6712377
Pulled By: mdvacca
fbshipit-source-id: bf5adc774703e7e66f7f16707600116f67201425
Reviewed By: gaearon
Differential Revision: D6886480
fbshipit-source-id: 09ca9ea8e20bc050ba30ae4e5f73495cace426f5
…nction
Reviewed By: emilsjolander
Differential Revision: D6797640
fbshipit-source-id: ad9757e7d603c0ce57f452b1e5c404037605bed9
Reviewed By: emilsjolander
Differential Revision: D6819719
fbshipit-source-id: e5e77c21d1dca2255433da3388887d9db3f7b642
mdvaccaand others added 2 commits March 1, 2018 10:33
Reviewed By: fkgozali
Differential Revision: D7114865
fbshipit-source-id: f0a1c47c983e610fe0dba3051ed8aa350ac052cc
Reviewed By: mdvacca
Differential Revision: D7126177
fbshipit-source-id: bafa6e2b3dabf39d2ca0d9a8830b877fc5ae97ec
mdvaccaand others added 16 commits March 1, 2018 16:57
…s included as a child of a non Text component
Reviewed By: achen1
Differential Revision: D7120188
fbshipit-source-id: 553a26d04a62dceb86d791bcdcb3a5e16a12f64b
Reviewed By: mdvacca
Differential Revision: D7127321
fbshipit-source-id: a8215fda0d854471bed9aa5476141dfffc4dc11c
Reviewed By: mdvacca
Differential Revision: D7128443
fbshipit-source-id: 4eedea4df2b636eb9589cbe5e84c5c6a8aa33539
Reviewed By: sahrens
Differential Revision: D7116184
fbshipit-source-id: 4fd1654028e52f5aafad348546b889f1737c7399
Differential Revision: D7112419
fbshipit-source-id: 1d80c0c13144dd19bbcd5535383befc6567cacf7
Summary:
Add RCTGetReactNativeVersion() to expose version in native code. Right now, version is exposed internally to RN using a MACRO constant. This exposes a symbol (function) that can be called to retrieve the React Native version in iOS.
Also exposed RCTVersion.h as a public header in the React project so it is available to developers.
The motivation behind this is for https://github.com/wix/detox —we need to know what RN version the user has, if any, so we can properly handle support and abstract differences.
Ran bump-oss-version.js to ensure the template is applied properly. Also compiled the project to make sure nothing is broken.
[IOS] [ENHANCEMENT] [RCTVersion.h] - Expose version as a compile-time symbol for native queries
<!--
Help reviewers and the release process by writing your own release notes
**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**
CATEGORY
[----------] TYPE
[ CLI ] [-------------] LOCATION
[ DOCS ] [ BREAKING ] [-------------]
[ GENERAL ] [ BUGFIX ] [-{Component}-]
[ INTERNAL ] [ ENHANCEMENT ] [ {File} ]
[ IOS ] [ FEATURE ] [ {Directory} ] |-----------|
[ ANDROID ] [ MINOR ] [ {Framework} ] - | {Message} |
[----------] [-------------] [-------------] |-----------|
[CATEGORY] [TYPE] [LOCATION] - MESSAGE
EXAMPLES:
[IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
[ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
[CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
[DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
[GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
[INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Closesreact#18136
Differential Revision: D7141076
Pulled By: hramos
fbshipit-source-id: 18a92b8c60d7b43fa0ed22597ea46a35cff73c56
Reviewed By: mmmulani
Differential Revision: D7141568
fbshipit-source-id: 6c8679790309e72ca220443ecf693087f82ece85
Reviewed By: fkgozali
Differential Revision: D7125829
fbshipit-source-id: 46f9722a20e0bbf7f99a0cc36067035b009d73d8
Reviewed By: compnerd
Differential Revision: D7144899
fbshipit-source-id: d40bda0e9225734398e35adc582b8932c0280b24
…rfaceHostingView
Summary:
To help with migration from direct usages of RCTRootView to RCTSurfaceHostingView, RCTSurfaceHostingProxyRootView is added, which is simply a custom impl of RCTSurfaceHostingView, but will all RCTRootView APIs preserved. This makes it easy to do a drop-in replacement in native callsites:
```
// before:
RCTRootView *rootView = [[RCTRootView alloc] init...];
// after:
RCTRootView *rootView = (RCTRootView *)[[RCTSurfaceHostingProxyRootView alloc] init...];
```
Reviewed By: shergin
Differential Revision: D7141696
fbshipit-source-id: db8c447749eaa896efaa37774a9babef132128eb
Differential Revision: D7144192
fbshipit-source-id: 4001930b194421682fa9bdc9b097434910f339b6
Reviewed By: sahrens
Differential Revision: D7117137
fbshipit-source-id: a55a04928a0073a17e0709e851aa8b11678042ba
Reviewed By: yungsters
Differential Revision: D7131703
fbshipit-source-id: d8e37fcd0c743057e04760b1e50f8d879bd2826a
Summary:
Although the test suites have a handful of failing tests, the jobs themselves do not fail.
Let's get these tests back into the fold so that we may track our progress getting these back to a good state.
cc dlowder-salesforce
Run tests on Circle, and confirm everything is green: https://circleci.com/workflow-run/4dd1a84b-502d-4ad6-aa41-64c768392a6b
If you go into the test iOS and test tvOS jobs, you will see that we are collecting test results at the top. These results show the failing individual tests.
Closesreact#18171
Differential Revision: D7151558
Pulled By: hramos
fbshipit-source-id: f105ec8bc97e80ed1b8358cde3f13a1ad3b271c2
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.
…Forks/react-native into hotfix/filter-webview-cookies
* 'hotfix/filter-webview-cookies' of github.com:MediaMonksForks/react-native:
This ensures no illegal cookies are send to okhttp
@pull-bot

Copy link
Copy Markdown
Warnings
⚠️

🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

⚠️

❗ Big PR - This PR is extremely unlikely to get reviewed because it touches 93496 lines.

Generated by 🚫 dangerJS

@erikpoort

Copy link
Copy Markdown
Author

Looks like something went wrong with the rebase, opening a new PR.

@erikpoort
erikpoort deleted the hotfix/filter-webview-cookies branch March 5, 2018 18:39
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@erikpoort@facebook-github-bot@pull-bot@rikur@shergin@purplecabbage@dantman@vovkasm@hramos@doskey@sunnylqm@neiker@turnrye@tasomaniac@douchuan@oguzbilgener@bxbxbai@aleclarson@grabbou@janicduplessis