1264577 - Tests for first-party isolation of cache (Tor 13749)

Status: RESOLVED FIXED

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

Description

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tor Browser WIP patch.

Tor Browser tests for first-party isolation of cache.
https://torpat.ch/13749

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Just for reference, there is a unit test for http cache isolation (by origin attributes). http://searchfox.org/mozilla-central/source/netwerk/test/unit/test_cache_jar.js

Comment 2

[Dave Huseby [:huseby]]

Tim,

I'm looking for something to pick up, so I think I'll take this one if you don't mind.

--dave

Comment 3

[Arthur Edelstein [:arthur]]

Something we justed added in Tor Browser is an extra patch and test to ensure that SharedWorker scripts are requested and cached with the correct first-party:

patch:
https://gitweb.torproject.org/tor-browser.git/patch/?id=22e6c33
test:
https://gitweb.torproject.org/tor-browser.git/patch/?id=cb2bd1d

Comment 4

[Tim Huang[:timhuang]]

Attachment: Bug 1264577 - Add a test case of caches for first-party isolation(adapted from Tor Browser patch 13749).

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

Comment 5

[Arthur Edelstein [:arthur]]

Comment on attachment 8803753 [details]
Bug 1264577 - Add a test case of caches for first-party isolation(adapted from Tor Browser patch 13749).

https://reviewboard.mozilla.org/r/87890/#review87182

See minor nits, but otherwise looks good to me. Thanks, Tim!

::: browser/components/originattributes/test/browser/browser_cache.js:2
(Diff revision 1)
> +/*
> + * Bug 1264577 - A test case for testing caches.

I would add more description here, something about how it is testing container/first-party isolation of caches and correct origin attributes assigned to network requests.

::: browser/components/originattributes/test/browser/browser_cache.js:117
(Diff revision 1)
> +let stopObservingChannels;
> +
> +// The init function, which clears image and network caches, and generates
> +// the random value for isolating video and audio elements across different
> +// test runs.
> +function* doInit(aMode) {

This function definition has a lot of empty lines. Maybe remove them?

::: browser/components/originattributes/test/browser/browser_cache.js:138
(Diff revision 1)
> +// In the test function, we dynamically generate the video and audio element,
> +// and assign a random suffix to their URL to isolate them across different
> +// test runs.
> +function* doTest(aBrowser) {
> +
> +  let argObj = {

Is this necessary? It seems like you should be able to pass the randomSuffix and urlPrefix in to the following spawned task as closures without an argObj.

::: browser/components/originattributes/test/browser/browser_cache.js:205
(Diff revision 1)
> +
> +  for (let suffix of suffixes) {
> +    let foundEntryCount = countMatchingCacheEntries(data, "example.net", suffix);
> +    let result = (suffix.startsWith("video") || suffix.startsWith("audio")) ?
> +                 // Video and audio elements aren't always cached, so
> +                 // tolerate fewer cached copies.

Is this still true? If not, maybe remove this special case?

Comment 6

[Arthur Edelstein [:arthur]]

(In reply to Arthur Edelstein [:arthuredelstein] from comment #5)
> > +  let argObj = {
> 
> Is this necessary? It seems like you should be able to pass the randomSuffix
> and urlPrefix in to the following spawned task as closures without an argObj.

I see now that a closure doesn't work here, so this comment was wrong.

Comment 7

[Tim Huang[:timhuang]]

Comment on attachment 8803753 [details]
Bug 1264577 - Add a test case of caches for first-party isolation(adapted from Tor Browser patch 13749).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87890/diff/1-2/

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8803753 [details]
Bug 1264577 - Add a test case of caches for first-party isolation(adapted from Tor Browser patch 13749).

https://reviewboard.mozilla.org/r/87890/#review87754

very nice!

Comment 9

[Tim Huang[:timhuang]]

Comment on attachment 8803753 [details]
Bug 1264577 - Add a test case of caches for first-party isolation(adapted from Tor Browser patch 13749).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87890/diff/2-3/

Comment 10

[Tim Huang[:timhuang]]

Comment on attachment 8803753 [details]
Bug 1264577 - Add a test case of caches for first-party isolation(adapted from Tor Browser patch 13749).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87890/diff/3-4/

Comment 11

[Tim Huang[:timhuang]]

Comment on attachment 8803753 [details]
Bug 1264577 - Add a test case of caches for first-party isolation(adapted from Tor Browser patch 13749).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87890/diff/4-5/

Comment 12

[Tim Huang[:timhuang]]

Try looks good.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4128e57e39bd
Add a test case of caches for first-party isolation(adapted from Tor Browser patch 13749). r=mayhemer, r=arthuredelstein

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4128e57e39bd