Bug 227564 - WebSocket traffic should be associated with content in Safari rather than the app itself
Summary: WebSocket traffic should be associated with content in Safari rather than the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Critical
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-30 20:36 PDT by Richard Houle
Modified: 2021-07-23 11:23 PDT (History)
5 users (show)

See Also:


Attachments
Proposed fix (1.10 KB, patch)
2021-06-30 20:36 PDT, Richard Houle
no flags Details | Formatted Diff | Diff
test case app initiated (1.30 KB, text/plain)
2021-07-08 20:52 PDT, Kate Cheney
no flags Details
test case user initiated (1.30 KB, text/plain)
2021-07-08 20:52 PDT, Kate Cheney
no flags Details
V2 (6.90 KB, patch)
2021-07-22 12:12 PDT, Richard Houle
no flags Details | Formatted Diff | Diff
V3 (6.99 KB, patch)
2021-07-22 13:44 PDT, Richard Houle
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
V4 (6.84 KB, patch)
2021-07-22 17:51 PDT, Richard Houle
katherine_cheney: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
V4 with Changeling (9.26 KB, patch)
2021-07-23 10:24 PDT, Richard Houle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Houle 2021-06-30 20:36:22 PDT
Created attachment 432654 [details]
Proposed fix

<rdar://79307301>

WebSocket traffic is currently assigned to Safari. Someone browsing on a website using web sockets as messaging will be reported as Safari contacting a website rather than the content contacting a website.
Comment 1 Kate Cheney 2021-07-01 11:43:46 PDT
Looks good. I think we should write a test before landing.
Comment 2 Radar WebKit Bug Importer 2021-07-07 20:37:16 PDT
<rdar://problem/80303362>
Comment 3 Kate Cheney 2021-07-08 20:52:29 PDT
Created attachment 433197 [details]
test case app initiated
Comment 4 Kate Cheney 2021-07-08 20:52:50 PDT
Created attachment 433198 [details]
test case user initiated
Comment 5 Kate Cheney 2021-07-08 20:54:57 PDT
Comment on attachment 432654 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=432654&action=review

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1696
> +    if (!request.isAppBound()) {

I did a rename so you will likely have to rebase this patch.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1701
> +#endif

I think you'll also need something like this:

appPrivacyReportTestingData().didLoadAppInitiatedRequest(nsRequest.get().attribution == NSURLRequestAttributionDeveloper);

here to send the attribution data to the test runner.
Comment 6 Richard Houle 2021-07-22 12:12:54 PDT
Created attachment 434027 [details]
V2
Comment 7 Richard Houle 2021-07-22 13:44:07 PDT
Created attachment 434032 [details]
V3
Comment 8 Kate Cheney 2021-07-22 15:29:11 PDT
(In reply to Richard Houle from comment #7)
> Created attachment 434032 [details]
> V3

You will need to update test expectations to skip the new tests in open source because our EWS bots are iOS 14 only. You can either move them to the LayoutTests/http/tests/app-privacy-report/ directory which is already skipped or add the test path you made to LayoutTests/TestExpectations (just make sure they pass locally).
Comment 9 Richard Houle 2021-07-22 17:51:49 PDT
Created attachment 434049 [details]
V4
Comment 10 Richard Houle 2021-07-22 17:54:25 PDT
I got the following output when I ran them locally:

Test configuration: <, x86_64, release>
Placing test results in /Volumes/Work/Safari/OpenSource/WebKitBuild/Release-iphonesimulator/layout-test-results
Using Release build
Pixel tests disabled
Regular timeout: 30000, slow test timeout: 150000
Command line: /Volumes/Work/Safari/OpenSource/WebKitBuild/Release-iphonesimulator/WebKitTestRunnerApp.app -

--lint-test-files warnings:                                           
LayoutTests/platform/ios/TestExpectations:2652 Path does not exist. imported/w3c/web-platform-tests/html/webappapis/timers/type-long-setinterval.html
LayoutTests/platform/ios-wk2/TestExpectations:1204 Path does not exist. imported/w3c/web-platform-tests/html/webappapis/timers/negative-settimeout.html
LayoutTests/platform/ios-wk2/TestExpectations:1278 Path does not exist. imported/w3c/web-platform-tests/websockets/Create-Secure-verify-url-set-non-default-port.any.html

Found 2 tests; running 2, skipping 0.
                        
Verbose baseline search path: platform/iphone-se (1st generation)-simulator-15-wk2 -> platform/iphone-se (1st generation)-simulator-15 -> platform/iphone-se (1st generation)-simulator-wk2 -> platform/iphone-se (1st generation)-simulator -> platform/iphone-simulator-15-wk2 -> platform/iphone-simulator-15 -> platform/iphone-simulator-wk2 -> platform/iphone-simulator -> platform/ios-simulator-15-wk2 -> platform/ios-simulator-15 -> platform/ios-simulator-wk2 -> platform/ios-simulator -> platform/iphone-se (1st generation)-15-wk2 -> platform/iphone-se (1st generation)-15 -> platform/iphone-se (1st generation)-wk2 -> platform/iphone-se (1st generation) -> platform/iphone-15-wk2 -> platform/iphone-15 -> platform/iphone-wk2 -> platform/iphone -> platform/ios-15-wk2 -> platform/ios-15 -> platform/ios-wk2 -> platform/ios -> platform/wk2 -> generic

Baseline search path: platform/ios-simulator-wk2 -> platform/ios-simulator -> platform/ios-wk2 -> platform/ios -> platform/wk2 -> generic

Running 2 tests for iPhone SE (1st generation) running iOS 15

Running 1 WebKitTestRunnerApp.app.

[1/2] http/tests/app-privacy-report/websocket-isappinitiated.html passed
[2/2] http/tests/app-privacy-report/websocket-isnotappinitiated.html passed
                        
All 2 tests ran as expected.
Comment 11 Kate Cheney 2021-07-23 10:09:36 PDT
Comment on attachment 434049 [details]
V4

Looks good! r=me
Comment 12 EWS 2021-07-23 10:10:42 PDT
Unable to find any modified ChangeLog in Attachment 434049 [details]
Comment 13 Kate Cheney 2021-07-23 10:11:39 PDT
Ah, missing a ChangeLog, can you upload a new patch with one?
Comment 14 Richard Houle 2021-07-23 10:24:43 PDT
Created attachment 434097 [details]
V4 with Changeling
Comment 15 EWS 2021-07-23 11:23:22 PDT
Committed r280250 (239917@main): <https://commits.webkit.org/239917@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434097 [details].