Bug 229814 - Implement origin partitioning (top-origin/frame-origin) for BroadcastChannel
Summary: Implement origin partitioning (top-origin/frame-origin) for BroadcastChannel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://github.com/whatwg/html/issues...
Keywords: InRadar
Depends on:
Blocks: 161472
  Show dependency treegraph
 
Reported: 2021-09-02 10:05 PDT by Chris Dumez
Modified: 2021-09-10 12:04 PDT (History)
12 users (show)

See Also:


Attachments
Patch (35.89 KB, patch)
2021-09-02 10:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (34.37 KB, patch)
2021-09-02 10:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (34.08 KB, patch)
2021-09-02 10:44 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (32.51 KB, patch)
2021-09-02 13:50 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (33.76 KB, patch)
2021-09-02 16:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-09-02 10:05:53 PDT
Implement origin partitioning (top-origin/frame-origin) for BroadcastChannel to address privacy concerns, as discussed here:
- https://github.com/whatwg/html/issues/5803
Comment 1 Chris Dumez 2021-09-02 10:13:51 PDT
Created attachment 437163 [details]
Patch
Comment 2 Chris Dumez 2021-09-02 10:15:41 PDT
Created attachment 437164 [details]
Patch
Comment 3 Chris Dumez 2021-09-02 10:44:16 PDT
Created attachment 437166 [details]
Patch
Comment 4 Sam Weinig 2021-09-02 13:08:32 PDT
Comment on attachment 437166 [details]
Patch

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

> Tools/TestRunnerShared/TestFeatures.cpp:128
> +static bool shouldDisableBroadcastChannelOriginPartitioning(const std::string& pathOrURL)
> +{
> +    return pathContains(pathOrURL, "localhost:8800/") || pathContains(pathOrURL, "localhost:9443/");
> +}

We'd like to avoid adding more of these path based filters (the goal was to remove them all eventually).

Can we just disable it by default for all tests instead and use header comments to enable it where needed?
Comment 5 Chris Dumez 2021-09-02 13:10:05 PDT
(In reply to Sam Weinig from comment #4)
> Comment on attachment 437166 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437166&action=review
> 
> > Tools/TestRunnerShared/TestFeatures.cpp:128
> > +static bool shouldDisableBroadcastChannelOriginPartitioning(const std::string& pathOrURL)
> > +{
> > +    return pathContains(pathOrURL, "localhost:8800/") || pathContains(pathOrURL, "localhost:9443/");
> > +}
> 
> We'd like to avoid adding more of these path based filters (the goal was to
> remove them all eventually).
> 
> Can we just disable it by default for all tests instead and use header
> comments to enable it where needed?

We could. But since it would be enabled in the shipping configuration, ideally, I'd want it to be the default in our tests I think.
Comment 6 Chris Dumez 2021-09-02 13:50:54 PDT
Created attachment 437189 [details]
Patch
Comment 7 Chris Dumez 2021-09-02 13:58:55 PDT
(In reply to Sam Weinig from comment #4)
> Comment on attachment 437166 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437166&action=review
> 
> > Tools/TestRunnerShared/TestFeatures.cpp:128
> > +static bool shouldDisableBroadcastChannelOriginPartitioning(const std::string& pathOrURL)
> > +{
> > +    return pathContains(pathOrURL, "localhost:8800/") || pathContains(pathOrURL, "localhost:9443/");
> > +}
> 
> We'd like to avoid adding more of these path based filters (the goal was to
> remove them all eventually).
> 
> Can we just disable it by default for all tests instead and use header
> comments to enable it where needed?

I made this change.
Comment 8 Chris Dumez 2021-09-02 16:55:03 PDT
Created attachment 437220 [details]
Patch
Comment 9 Sam Weinig 2021-09-06 10:15:45 PDT
(In reply to Chris Dumez from comment #5)
> (In reply to Sam Weinig from comment #4)
> > Comment on attachment 437166 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=437166&action=review
> > 
> > > Tools/TestRunnerShared/TestFeatures.cpp:128
> > > +static bool shouldDisableBroadcastChannelOriginPartitioning(const std::string& pathOrURL)
> > > +{
> > > +    return pathContains(pathOrURL, "localhost:8800/") || pathContains(pathOrURL, "localhost:9443/");
> > > +}
> > 
> > We'd like to avoid adding more of these path based filters (the goal was to
> > remove them all eventually).
> > 
> > Can we just disable it by default for all tests instead and use header
> > comments to enable it where needed?
> 
> We could. But since it would be enabled in the shipping configuration,
> ideally, I'd want it to be the default in our tests I think.

The opposite would also be fine (enable by default, disable explicitly where needed).
Comment 10 Chris Dumez 2021-09-07 07:25:13 PDT
(In reply to Sam Weinig from comment #9)
> (In reply to Chris Dumez from comment #5)
> > (In reply to Sam Weinig from comment #4)
> > > Comment on attachment 437166 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=437166&action=review
> > > 
> > > > Tools/TestRunnerShared/TestFeatures.cpp:128
> > > > +static bool shouldDisableBroadcastChannelOriginPartitioning(const std::string& pathOrURL)
> > > > +{
> > > > +    return pathContains(pathOrURL, "localhost:8800/") || pathContains(pathOrURL, "localhost:9443/");
> > > > +}
> > > 
> > > We'd like to avoid adding more of these path based filters (the goal was to
> > > remove them all eventually).
> > > 
> > > Can we just disable it by default for all tests instead and use header
> > > comments to enable it where needed?
> > 
> > We could. But since it would be enabled in the shipping configuration,
> > ideally, I'd want it to be the default in our tests I think.
> 
> The opposite would also be fine (enable by default, disable explicitly where
> needed).

Put I cannot add header comments to disable the feature in WPT tests, which is why I was doing it some other way.
Comment 11 EWS 2021-09-07 12:38:15 PDT
Committed r282105 (241403@main): <https://commits.webkit.org/241403@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437220 [details].
Comment 12 Radar WebKit Bug Importer 2021-09-07 12:39:21 PDT
<rdar://problem/82834757>
Comment 13 Chris Dumez 2021-09-07 14:18:10 PDT
Follow-up fix in <https://commits.webkit.org/r282106>.
Comment 14 Chris Dumez 2021-09-09 17:02:08 PDT
Follow-up fix in <https://commits.webkit.org/r282246>.