[Blocked Security Review]Fixes #1144 Sharing of session amongst tabs and sharing of Authenticated Documents #1237

Brandon-T · 2019-07-11T19:58:31Z

BUG FIXES:

Fixes: Print button is unresponsive for Online PDF bank statements #1144
Fixed a background thread issue where "displayTitle" is being called on WKWebView on a non-main thread causing the PrintPreview to have undefined-behaviour.
Fixed opening Authenticated documents in Private-Browsing mode when it opens in a new tab.
Fixed opening Authenticated documents and attempting to share, or print them.
Fixes tabs not having the same data store as other tabs (IE: Log-In to any website on one tab should log you in on another tab).
Fixed destroying of data-store to only happen when ALL tabs are destroyed.
Changed Temporary Storage to pull out the document from the WebPage instead of making server calls outside the webview's session (IE: No need for cookie injecting, etc).. Instead, we get the webview to give us the document via Javascript injection which passes the document to iOS for display/saving, etc..

WHAT I TESTED:

Log into Email (gmail.com) in PrivateTab-A
- Refresh PrivateTab-B
- PrivateTab-B is also logged in.
Log out of Email (gmail.com) in PrivateTab-A
- Refresh PrivateTab-B
- PrivateTab-B is logged out.
Above tested in normal mode as well.
Log into PrivateTab-A
- Open Normal Tab
- Normal tab is NOT logged in.
- Switch back to private mode, all tabs are gone.
Log into Normal Tab-A
- Open PrivateTab
- Private Tab is not logged in
- Switch back to Normal Tab - Still logged in.
Log into banking website in PrivateTab-A
- View statements (It opens statements a PDF in PrivateTab-B [ authenticated document ])
- Success (PDF opened).
- Share PDF in PrivateTab-B (Works).
Log into banking website in PrivateTab-A
- Switched to Normal mode and back to PrivateMode.
- Tabs killed, cookies and sessions are cleared successfully for all Private Tabs.
Log into banking website in PrivateTab-A
- Open second PrivateTab-B.
- Kill both Tabs.
- Cookies and sessions are cleared successfully for all Private Tabs.
Log into banking website in PrivateTab-A
- View statements as PDF (opens in a new PrivateTab-B).
- Turn Internet OFF
- Share PDF and Print (works without internet because no requests are made)
- Kill tabs
- Cookies and sessions are cleared successfully for all Private Tabs.

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
My patch or PR title has a standard commit message that looks like Fix #123: This fixes the shattered coffee cup! (or No Bug: <message> if no relevant ticket)
Unit Tests are updated to cover new or changed functionality
User-facing strings use NSLocalizableString()
New files have MPL-2.0 license header.

Test Plan:

Screenshots:

Reviewer Checklist:

PR is linked to an issue via Zenhub.
Issues are assigned to at least one epic.
Issues include necessary QA labels:
- QA/(Yes|No)
- release-notes/(include|exclude)
- bug / enhancement
Necessary security reviews have taken place.
Adequate test coverage exists to prevent regressions.
Adequate test plan exists for QA to validate (if applicable)

Client/Frontend/Browser/TabManager.swift

Client/Frontend/Browser/TemporaryDocument.swift

danishjafri88 · 2019-07-11T22:29:55Z

Security review issue: https://github.com/brave/internal/issues/616

iccub

looks good overall, small changes requested

Client/Frontend/Browser/ResourceDownloadManager.swift

jumde · 2019-07-12T18:11:10Z

There are too many edge cases with cookie handling in private and normal tabs, I think we should add more tests for the cases that we have tested with this PR to avoid regressions in future.

jumde · 2019-07-12T19:36:41Z

Client/Frontend/Browser/ResourceDownloadManager.swift

+    init(from decoder: Decoder) throws {
+        let container = try decoder.container(keyedBy: CodingKeys.self)
+        self.statusCode = try container.decode(Int.self, forKey: .statusCode)
+        self.data = Data(base64Encoded: try container.decode(String.self, forKey: .base64Data))


minor-nit: we should probably add a maximum cap on how much data can be downloaded.

I can add that cap in Javascript but what would be the max file size? It will pull the "PDF" or image or whatever data from the WebKit's internal cache if available instead of "downloading" and it only does this when the user hits "share".

Also, what kind of tests did you have in mind? :)
I'll try to think of scenarios that I haven't tested already.
I've mostly been comparing various browsers' behaviours to our Brave-iOS.
Didn't realize how many problems arrises just trying to fix PDF :D lol

You can use the Content-Length header, if the request is from the cache you can check if the status code is 304.

The cache doesn't return a 304 ever (it's local). It returns a 0 if it fails. If that's the case, my code returns you null because there is no document available. In the case where a document is available to share, it returns a 200 always and the byte-data associated with it.

If the web-server returns a 304, the page would have loaded the document from the cache, this Javascript would pull it from the page returning a 200.

I checked Brave Desktop, there's no limit to what you can download and I haven't seen any limits in the old code before this PR so I'm not sure what the limit is.

danishjafri88 · 2019-07-12T20:09:39Z

There are too many edge cases with cookie handling in private and normal tabs, I think we should add more tests for the cases that we have tested with this PR to avoid regressions in future.

@jumde can you please help us getting together a list for the same. Its really difficult to figure out all possibilities :)

jumde · 2019-07-12T20:23:16Z

There are too many edge cases with cookie handling in private and normal tabs, I think we should add more tests for the cases that we have tested with this PR to avoid regressions in future.

@jumde can you please help us getting together a list for the same. Its really difficult to figure out all possibilities :)

@danishjafri88 -

Yes! Let's use this comment to compile the list, feel free to add the cases you think i've missed:

Navigate to http://site-a.com/store.html in private tab
Open http://site-a.com in normal tab and confirm that no storage is available.

Navigate to http://site-a.com/store.html in normal tab
Open http://site-a.com in private tab and confirm no storage is available
Open http://site-a.com using long press and confirm that no storage is available.

Navigate to http://site-a.com/store.html in private tab
Close all tabs
Confirm that no-storage is available for http://site-a.com in private tab

Navigate to http://site-a.com/store.html in private tab
Open http://site-a.com/ in a different private tab
Verify that storage is available for site-a in the second tab.

jumde · 2019-07-12T20:43:05Z

Client/Frontend/UserContent/UserScripts/ResourceDownloader.js

+        xhr.responseType = "arraybuffer";
+        xhr.onreadystatechange = function() {
+            if (this.readyState == XMLHttpRequest.DONE) {
+                if (this.status == 200) {


What about redirects here?

It shouldn't matter because it only triggers the Javascript when you press "Share" and it only injects the Javascript when the page is completely loaded (IE: 200 or 404).
In such a case, you won't be able to share a 404 page anyway as the Mime-Type is not a doc type.

There is some code that was written before that checks:
if response.mimeType?.isKindOfHTML == false before allowing you to "share" the page.
Basically, if the document was never loaded into a page with a 200, this Javascript won't ever be triggered because you won't be able to share in the first place.

In the case that the page is loaded and we attempt to get the document, I can't see it ever returning a redirect since the document is already downloaded. Also, if it does return a redirect for the weirdest possible reason, the JS would return you NULL as per the else statement.

Brandon-T · 2019-07-15T14:39:54Z

Navigate to http://site-a.com/store.html in private tab

Open http://site-a.com in normal tab and confirm that no storage is available.

Navigate to http://site-a.com/store.html in normal tab

Open http://site-a.com in private tab and confirm no storage is available

Open http://site-a.com using long press and confirm that no storage is available.

Navigate to http://site-a.com/store.html in private tab

Close all tabs

Confirm that no-storage is available for http://site-a.com in private tab

Navigate to http://site-a.com/store.html in private tab

Open http://site-a.com/ in a different private tab

Verify that storage is available for site-a in the second tab.

Hmm I tested these cases in my PR with gmail & safari dev tools.

Case 1 -> Opening a normal tab kills all private tabs and all their non-persistent stores along with them. Also tested in PR.

Case 2 -> Login gmail via Normal tab does NOT log you into a Private tab & vice-versa since they use different storage. Tested this in my PR.

Case 3 -> Killing the last private tab does the same as killing all private tabs and their non-persistent stores along with them.

Case 4 -> Verified in my PR via gmail log in. Login to tab-a automatically logs you in to tab-b.

iccub

Looks good, although this needs some serious testing from the QA team.

danishjafri88

lgtm

jumde · 2019-07-15T17:46:23Z

Navigate to http://site-a.com/store.html in private tab

Open http://site-a.com in normal tab and confirm that no storage is available.

Navigate to http://site-a.com/store.html in normal tab

Open http://site-a.com in private tab and confirm no storage is available

Open http://site-a.com using long press and confirm that no storage is available.

Navigate to http://site-a.com/store.html in private tab

Close all tabs

Confirm that no-storage is available for http://site-a.com in private tab

Navigate to http://site-a.com/store.html in private tab

Open http://site-a.com/ in a different private tab

Verify that storage is available for site-a in the second tab.

Hmm I tested these cases in my PR with gmail & safari dev tools.

Case 1 -> Opening a normal tab kills all private tabs and all their non-persistent stores along with them. Also tested in PR.

Case 2 -> Login gmail via Normal tab does NOT log you into a Private tab & vice-versa since they use different storage. Tested this in my PR.

Case 3 -> Killing the last private tab does the same as killing all private tabs and their non-persistent stores along with them.

Case 4 -> Verified in my PR via gmail log in. Login to tab-a automatically logs you in to tab-b.

I was thinking more of some automated tests, you can check the tests in ClientTests for reference. There are too many edge cases with storage, so just want to make sure we don't regress going forward.

Brandon-T · 2019-07-18T17:58:36Z

I was thinking more of some automated tests, you can check the tests in ClientTests for reference. There are too many edge cases with storage, so just want to make sure we don't regress going forward.

Fixed. I've added some important tests that cleared up a lot of confusion and discovered some bugs along the way.

class TabSessionTests: XCTestCase {
    /** Tests whether or not two private tabs are sharing the same data-store/cookie-store **/
    func testPrivateTabSessionSharing()

    /** Tests whether or not a private tab's data-store/cookie-store is truly non-persistent **/
    func testPrivateTabNonPersistence()

    /** Tests whether or not switching from private tab to normal tab destroys the data-store/cookie-store completely **/
    func testTabsPrivateToNormal()

    /** Tests whether or not switching from normal tab to private tab keeps the data-store/cookie-store separate **/
    func testTabsNormalToPrivate()
}

These seem to be the most important and have fixed any bugs related to their failures.
The last unit test has some code commented out because I couldn't solve why it was happening and I believe it to be a WebKit bug and not brave code related.

ClientTests/TabSessionTests.swift

danishjafri88 · 2019-07-19T16:31:19Z

@jumde tests have been added. Can you verify and update the security review status?

iccub · 2019-07-19T16:34:22Z

If you haven't done it already, this also needs to be rebased to latest development to pass CI

danishjafri88

Solid work!!!

Fixed a background thread issue where "displayTitle" is being called on WKWebView on a non-main thread causing the PrintPreview to have undefined-behaviour. Fixed opening Authenticated documents in Private-Browsing mode when it opens in a new tab. Fixed opening Authenticated documents and attempting to share, or print them. Fixes tabs not having the same data store as other tabs (IE: Log-In to any website on one tab should log you in on another tab). Fixed destroying of data-store to only happen when ALL tabs are destroyed. Changed Temporary Storage to pull out the document from the WebPage instead of making server calls outside the webview's session (IE: No need for cookie injecting, etc).. Instead, we get the webview to give us the document via Javascript injection which passes the document to iOS for display/saving, etc..

Minor change to filtering tabs. Implicitly use the error parameter.

Fixed a bug in `removeAllBrowsingDataForTab` where it only removed select data instead of ALL data.

…te tabs have been destroyed. - Discovered during unit tests.

Brandon-T requested review from kylehickinson, jumde, jhreis, danishjafri88 and iccub July 11, 2019 19:58

Brandon-T changed the title ~~Fixes https://github.com/brave/brave-ios/issues/1144~~ Fixes Sharing of session amongst tabs and sharing of Authenticated Documents Jul 11, 2019

danishjafri88 suggested changes Jul 11, 2019

View reviewed changes

Client/Frontend/Browser/TabManager.swift Outdated Show resolved Hide resolved

Client/Frontend/Browser/TemporaryDocument.swift Show resolved Hide resolved

danishjafri88 changed the title ~~Fixes Sharing of session amongst tabs and sharing of Authenticated Documents~~ Fixes #1144 Sharing of session amongst tabs and sharing of Authenticated Documents Jul 11, 2019

danishjafri88 changed the title ~~Fixes #1144 Sharing of session amongst tabs and sharing of Authenticated Documents~~ [Blocked Security Review]Fixes #1144 Sharing of session amongst tabs and sharing of Authenticated Documents Jul 11, 2019

iccub suggested changes Jul 12, 2019

View reviewed changes

Client/Frontend/Browser/ResourceDownloadManager.swift Show resolved Hide resolved

Client/Frontend/Browser/ResourceDownloadManager.swift Outdated Show resolved Hide resolved

Brandon-T force-pushed the feature/WebDocumentStorage branch from 0068fb5 to 54ca680 Compare July 12, 2019 14:37

jumde reviewed Jul 12, 2019

View reviewed changes

Brandon-T requested review from jumde, iccub and danishjafri88 July 15, 2019 14:51

iccub approved these changes Jul 15, 2019

View reviewed changes

Brandon-T force-pushed the feature/WebDocumentStorage branch from 54ca680 to 7be96be Compare July 15, 2019 15:37

danishjafri88 approved these changes Jul 15, 2019

View reviewed changes

Brandon-T force-pushed the feature/WebDocumentStorage branch from 7be96be to 7890d34 Compare July 17, 2019 16:35

Brandon-T requested review from danishjafri88 and iccub July 18, 2019 17:59

Brandon-T force-pushed the feature/WebDocumentStorage branch from 0382115 to 93b8b53 Compare July 18, 2019 18:01

Brandon-T force-pushed the feature/WebDocumentStorage branch 2 times, most recently from 629c541 to 44352e9 Compare July 18, 2019 19:04

danishjafri88 suggested changes Jul 18, 2019

View reviewed changes

ClientTests/TabSessionTests.swift Outdated Show resolved Hide resolved

ClientTests/TabSessionTests.swift Outdated Show resolved Hide resolved

ClientTests/TabSessionTests.swift Outdated Show resolved Hide resolved

Brandon-T force-pushed the feature/WebDocumentStorage branch from 9ea1e6c to a138dde Compare July 19, 2019 22:05

Brandon-T requested a review from danishjafri88 July 19, 2019 22:06

danishjafri88 approved these changes Jul 23, 2019

View reviewed changes

Brandon-T added 9 commits July 23, 2019 11:58

Added documentation.

6816e2a

Minor change to filtering tabs. Implicitly use the error parameter.

Added Unit Tests to test Private vs. Normal Mode.

8cb128a

Fixed a bug in `removeAllBrowsingDataForTab` where it only removed select data instead of ALL data.

Added test for sharing session between normal tabs.

1875d50

Removed unneeded code.

d07c5db

Fixed Non-Persistent Store being perfectly ALIVE even after all priva…

a4e1024

…te tabs have been destroyed. - Discovered during unit tests.

Fixed CI test.

82e7424

Added more time to test.

81eb44c

Removed persistent store reference counting.

be9e0f2

Brandon-T force-pushed the feature/WebDocumentStorage branch from 80e5d2e to be9e0f2 Compare July 23, 2019 15:59

jumde approved these changes Jul 23, 2019

View reviewed changes

Brandon-T merged commit 1c95231 into development Jul 23, 2019

Brandon-T deleted the feature/WebDocumentStorage branch July 23, 2019 19:27

Brandon-T mentioned this pull request Jan 8, 2020

Fix #2162 - Fixes a crash when javascript's window.open creates a new tab. #2161

Merged

7 tasks

iccub mentioned this pull request Jan 8, 2020

window.open(), target=_blank crashes #2162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Blocked Security Review]Fixes #1144 Sharing of session amongst tabs and sharing of Authenticated Documents #1237

[Blocked Security Review]Fixes #1144 Sharing of session amongst tabs and sharing of Authenticated Documents #1237

Brandon-T commented Jul 11, 2019 •

edited

Loading

danishjafri88 commented Jul 11, 2019

iccub left a comment

jumde commented Jul 12, 2019

jumde Jul 12, 2019

Brandon-T Jul 12, 2019 •

edited

Loading

jumde Jul 12, 2019

Brandon-T Jul 15, 2019 •

edited

Loading

danishjafri88 commented Jul 12, 2019

jumde commented Jul 12, 2019 •

edited

Loading

jumde Jul 12, 2019

Brandon-T Jul 15, 2019 •

edited

Loading

Brandon-T commented Jul 15, 2019 •

edited

Loading

iccub left a comment

danishjafri88 left a comment

jumde commented Jul 15, 2019

Brandon-T commented Jul 18, 2019 •

edited

Loading

danishjafri88 commented Jul 19, 2019

iccub commented Jul 19, 2019 •

edited

Loading

danishjafri88 left a comment

[Blocked Security Review]Fixes #1144 Sharing of session amongst tabs and sharing of Authenticated Documents #1237

[Blocked Security Review]Fixes #1144 Sharing of session amongst tabs and sharing of Authenticated Documents #1237

Conversation

Brandon-T commented Jul 11, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Screenshots:

Reviewer Checklist:

danishjafri88 commented Jul 11, 2019

iccub left a comment

Choose a reason for hiding this comment

jumde commented Jul 12, 2019

jumde Jul 12, 2019

Choose a reason for hiding this comment

Brandon-T Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

jumde Jul 12, 2019

Choose a reason for hiding this comment

Brandon-T Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

danishjafri88 commented Jul 12, 2019

jumde commented Jul 12, 2019 • edited Loading

jumde Jul 12, 2019

Choose a reason for hiding this comment

Brandon-T Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

Brandon-T commented Jul 15, 2019 • edited Loading

iccub left a comment

Choose a reason for hiding this comment

danishjafri88 left a comment

Choose a reason for hiding this comment

jumde commented Jul 15, 2019

Brandon-T commented Jul 18, 2019 • edited Loading

danishjafri88 commented Jul 19, 2019

iccub commented Jul 19, 2019 • edited Loading

danishjafri88 left a comment

Choose a reason for hiding this comment

Brandon-T commented Jul 11, 2019 •

edited

Loading

Brandon-T Jul 12, 2019 •

edited

Loading

Brandon-T Jul 15, 2019 •

edited

Loading

jumde commented Jul 12, 2019 •

edited

Loading

Brandon-T Jul 15, 2019 •

edited

Loading

Brandon-T commented Jul 15, 2019 •

edited

Loading

Brandon-T commented Jul 18, 2019 •

edited

Loading

iccub commented Jul 19, 2019 •

edited

Loading