Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

This ensures no illegal cookies are send to okhttp #17569

Closed

Conversation

erikpoort
Copy link

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-bot facebook-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
Author

Fixes: #17568

davidaurelio and 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
Closes facebook#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: Closes facebook#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.
Closes facebook#17807

Differential Revision: D6865976

Pulled By: hramos

fbshipit-source-id: 48607a80dcf8cac1c3c033c18bf5d6dd4cd8e6bf
Summary: Closes facebook#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
Closes facebook#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 facebook#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
-->
Closes facebook#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 facebook#16541, for addressing issues facebook#11853/facebook#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 facebook#10423/facebook#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 facebook#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
Closes facebook#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
mdvacca and 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
@sdwilsh sdwilsh removed the cla signed label Mar 1, 2018
mdvacca and 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
-->
Closes facebook#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.
Closes facebook#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

pull-bot commented Mar 5, 2018

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
Author

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

@erikpoort erikpoort closed this Mar 5, 2018
@erikpoort erikpoort deleted the hotfix/filter-webview-cookies branch March 5, 2018 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This 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.