685104 - StoragePolicy should use white-list approach

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

We currently grant storage when a page starts loading and deny it when the page has loaded and we found a no-store header or the like.

What we should do is deny storage in the first place and allow it again if we don't find such a header. Otherwise there could be sensitive thumbnail data stored in the cache while the page is loading (this happens).

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

This patch denies storage by default and grants it when we read the crucial header information. This patch additionally fixes two little problems:

1) For about:blank "request" is not an instance of Ci.nsIHttpChannel. So we don't need to check any headers and can just grant storage.

2) There was a minor mistake in the following condition:

"if (cacheControlHeader && !(/public/i).test(cacheControlHeader))"

Correct is:

"if (!cacheControlHeader || !(/public/i).test(cacheControlHeader))"

Because deny storage only if there's no "public" in the Cache-Control header. So we have to deny it of course when there's no such header at all.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 558784 [details] [diff] [review]
patch v2

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

r=me w/ the noted change

::: browser/base/content/tabview/content.js
@@ +133,5 @@
> +          // Check if the "Cache-Control" header is "no-store". In this case we're
> +          // not allowed to store information about the current page.
> +          if (this._isNoStoreResponse(request)) {
> +            this._denyStorage("no-store");
> +            return;

the code looks mostly ok. however, can you rearrange the blocks to avoid the early return? i'm generally not a fan of early return in methods with more than a few lines of code, since they can be overlooked due to code-folding or not enough patch context, etc.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

(In reply to Dietrich Ayala (:dietrich) from comment #3)
> the code looks mostly ok. however, can you rearrange the blocks to avoid the
> early return? i'm generally not a fan of early return in methods with more
> than a few lines of code, since they can be overlooked due to code-folding
> or not enough patch context, etc.

Rearranged.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

browser_tabview_bug597248.js was constantly failing on try because that test closes the window before the third tab's storage has been granted.

I introduced afterStorageGrantedForAllTabs() as a new helper method in head.js to allow specific tests to wait until all tabs are allowed to store their thumbnails. With the new white list approach that's at least in the case of browser_tabview_bug597248.js necessary.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

browser_tabview_bug627288.js times out on try mostly on debug machines - probably a timing issue - investigating.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Panorama does now use the thumbnail service (bug 745303).