917659 - localStorage not isolated for background thumbnail captures

Status: NEW

(Firefox :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Since we no longer use private browsing to capture thumbnails in the background, pages loaded in the background for a capture have full access to localStorage - eg, storage set by a page in the foreground is seen when the page is loaded for the background capture, and conversely, localStorage set while the thumbnails are being captured in the background is seen when the page is loaded normally.

Given that cookies are *not* shared in this way, it's unlikely this will cause sensitive information to be captured, but theoretically it is possible.  Also, some might be confused or upset if the cookie state doesn't match storage state - eg, a site loaded for b/g capture might see no cookies and reset some localStorage.  When the page is next loaded normally, that localStorage will have been removed, which might make the page page not act as the user expects (eg, maybe the page might delete "draft" messages in that case)

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-917659-part-0-add-localStorage-tests-which-use-t.patch

This patch demonstrates the problem - it introduces 2 new todo_is() calls which should work if this bug didn't exist.  I figure we might as well get this committed even if it takes some time to resolve the bug itself?

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 806427 [details] [diff] [review]
0001-Bug-917659-part-0-add-localStorage-tests-which-use-t.patch

Review of attachment 806427 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know whether we'll need a test like this.  Whatever Gecko mechanism we end up using to prevent localStorage writes, presumably that mechanism comes with tests of its own.  In that case we likely wouldn't need a bg thumbnails test because it would not test anything that the Gecko test doesn't.  I would not be in favor of adding more test code, even if it's end-to-end, if it doesn't actually prove anything new.  So I hope it's OK if I cancel the review for now.

Comments on the patch follow.

Is there a reason you added a new HTML file instead of using the existing sjs?

::: toolkit/components/thumbnails/test/Makefile.in
@@ +17,5 @@
>  	background_red_scroll.html \
>  	background_red_redirect.sjs \
>  	privacy_cache_control.sjs \
> +	storage.html \
> +	storage.html^headers^ \

These should be in the ifndef RELEASE_BUILD section.

::: toolkit/components/thumbnails/test/browser_thumbnails_background.js
@@ +304,5 @@
> +
> +  // check that if a site captured in the background sets localStorage, a
> +  // normal tab doesn't see that storage.
> +  function noLocalStorageStored() {
> +    // first clear localStorage.

I may be picking a nit here, but it seems better to check that localStorage isn't set than to clear it.  (1) The previous test cleans up after itself so the clear isn't actually needed, and (2) it's not verifying that the state is what the test expects it to be.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Drew Willcoxon :adw from comment #2)
> Comment on attachment 806427 [details] [diff] [review]
> 0001-Bug-917659-part-0-add-localStorage-tests-which-use-t.patch

> I don't know whether we'll need a test like this.  Whatever Gecko mechanism
> we end up using to prevent localStorage writes, presumably that mechanism
> comes with tests of its own.  In that case we likely wouldn't need a bg
> thumbnails test because it would not test anything that the Gecko test
> doesn't.  I would not be in favor of adding more test code, even if it's
> end-to-end, if it doesn't actually prove anything new.  So I hope it's OK if
> I cancel the review for now.

Using the same argument, we shouldn't have cookie or private-browsing tests either.  I'm inclined to think that both them, and this test, adds value and confidence for the thumbnails code.

> Is there a reason you added a new HTML file instead of using the existing sjs?

Using the sjs to write out javascript to be executed by the client seems very clunky, and these test don't require different headers based on what exactly the test is doing, so plain old HTML seemed a better idea than further complicating the .sjs when we don't actually need any .sjs features.

> I may be picking a nit here, but it seems better to check that localStorage
> isn't set than to clear it.  (1) The previous test cleans up after itself so
> the clear isn't actually needed, and (2) it's not verifying that the state is
> what the test expects it to be.

If I understand your suggestion, I felt this was dangerous given localStorage is per-origin and not related to the path - so if some previously run test using example.com - even one added in the future - wrote to localStorage and didn't clean it up, it might impact this unrelated test.

Comment 4

[Drew Willcoxon :adw]

(In reply to Mark Hammond (:markh) from comment #3)
> Using the same argument, we shouldn't have cookie or private-browsing tests
> either.

Yeah, now that you landed the load flags patch and we're not relying on PB mode anymore, we can evaluate whether we still need those tests, and if not -- it seems like not, but I haven't looked at those flags' test coverage -- I think we should remove them.

> Using the sjs to write out javascript to be executed by the client seems
> very clunky,

Oh right, it accesses window.localStorage.  Duh.

> If I understand your suggestion, I felt this was dangerous given
> localStorage is per-origin and not related to the path

OK.