1294336 - Shared Worker should respect OriginAttributes (Tor 15564)

Status: RESOLVED DUPLICATE of bug 1268726

(Core :: DOM: Security, defect, P1)

Description

[Ethan Tseng [:ethan]]

Shared Workers should be segregated by userContextId in the OriginAttributes.

Comment 1

[Ethan Tseng [:ethan]]

From the perspective of implementation, this bug might be identical to bug 1268726 (isolate shared worker by first party domain). Once we resolve this bug by making Shared Worker OriginAttributes aware, bug 1268726 should be resolved as well.

However, we still file independent bugs to track this feature for two different projects (Containers and Tor integration) in case they have various requirements or priorities, etc.

Comment 2

[Tim Huang[:timhuang]]

Attachment: Bug 1294336 - Segregating shared workers by the originAttributes.

Review commit: https://reviewboard.mozilla.org/r/71556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71556/

Comment 3

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=379cff5b6a4e

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8781043 [details]
Bug 1294336 - Segregating shared workers by the originAttributes.

https://reviewboard.mozilla.org/r/71556/#review69070

::: dom/workers/RuntimeService.cpp:244
(Diff revision 1)
> -                        bool aPrivateBrowsing, nsCString& aKey)
> +                        bool aPrivateBrowsing, nsIPrincipal* aPrincipal,
> +                        nsCString& aKey)
>  {
> +  MOZ_ASSERT(aPrincipal);
> +
> +  nsCString originSuffix;

nsAutoCString

::: dom/workers/RuntimeService.cpp:245
(Diff revision 1)
> +                        nsCString& aKey)
>  {
> +  MOZ_ASSERT(aPrincipal);
> +
> +  nsCString originSuffix;
> +  nsresult rv;

nsresult rv = ...

Comment 5

[Dave Huseby [:huseby]]

Attachment: Bug_1268726.patch

I think this is the right way to do the origin attributes isolation because the private browsing flag is in the origin attributes already (I think).

Comment 6

[Dave Huseby [:huseby]]

If we go with my patch and remove the IsInPrivateBrowsing from the cache key, we can probably remove it completely from the worker info struct altogether.  If we do that, then we need to figure out what to do with the CacheCreator class: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#353

That's the only caller of the function that isn't related to the creation of the SharedWorker cache key.

Comment 7

[Kamil Jozwiak [:kjozwiak]]

I've added two manual test cases that illustrate that shared workers can propagate changes through different containers.

Test Case #1:

* open https://arthuredelstein.github.io/tordemos/sharedworker-parent.html in a personal container
* the page will generate and display a random number within the iframe
* right click on the second link (listed below) within the personal container and select "Open New Link in New Container Tab -> Work"
** https://arthurtordemos.github.io/tordemos/sharedworker-parent.html

The second tab (Work) will read the random number from the first container/tab (Personal), and will display the same generated number.

Test Case #2:

* open https://whatwg.org/demos/workers/multiviewer/page.html in a personal container
* click on the "open a new viewer" button which will open a new viewer within the personal container (Viewer 0 created)
* open a new work container via "File -> New Container Tab -> Work"
* load https://whatwg.org/demos/workers/multiviewer/viewer.html in the work container (Viewer 1 created)
* open a new shopping container via "File -> New Container Tab -> Shopping"
* load https://whatwg.org/demos/workers/multiviewer/viewer.html in the shopping contaier (Viewer 2 created)
* switch into the personal container that has "Viewer 0" and click on "Set 1"
* take a look at the other Viewers within the different containers and you'll notice that the change has propagated throughout the different containers
* switch into the personal container that has "Viewer 0" and type something into the "Public Chat" textbox and hit "Enter"
* take a look at the other Viewers within the different containers and you'll notice that the change has propagated throughout the different containers

Comment 8

[Tim Huang[:timhuang]]

Comment on attachment 8781339 [details] [diff] [review]
Bug_1268726.patch

I think it is a good idea for using the originAttirbutes instead of the nsIPrincipal in the GenerateSharedWorkerKey(). And yes, we should fix the private browsing flags here. For the CacheCreator, I think we could still get the mPrivateBrowsingId from the principal of the workerPrivate. We only have to modify the WorkerPrivate.IsInPrivateBrowsing() to check the originAttributes of the workerPrivate's principal.

Comment 9

[Dave Huseby [:huseby]]

Hi Tim,

Since I'm blocked on your test framework bug, can I take this bug and push it forward to review.  It's mostly done.  I'll just confirm whether or not we can get rid of the IsInPrivateBrowsing bool and accessor.  Once I know the answer to that, I'll update the patch and submit it for review.

--dave

Comment 10

[Tim Huang[:timhuang]]

I'm ok with that. Please take this.