StoragePolicy should use white-list approach

RESOLVED WONTFIX

Status

Firefox Graveyard
Panorama
RESOLVED WONTFIX
7 years ago
2 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Dependency tree / graph

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
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).
(Assignee)

Comment 2

7 years ago
Created attachment 558784 [details] [diff] [review]
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.
Attachment #558759 - Attachment is obsolete: true
Attachment #558784 - Flags: review?(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.
Attachment #558784 - Flags: review?(dietrich) → review+
(Assignee)

Comment 4

7 years ago
Created attachment 562946 [details] [diff] [review]
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.
Attachment #558784 - Attachment is obsolete: true
Attachment #562946 - Flags: review?(dietrich)
(Assignee)

Comment 5

7 years ago
Created attachment 563072 [details] [diff] [review]
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.
Attachment #562946 - Attachment is obsolete: true
Attachment #562946 - Flags: review?(dietrich)
Attachment #563072 - Flags: review?(dietrich)
Attachment #563072 - Flags: review?(dietrich) → review+
(Assignee)

Comment 6

7 years ago
browser_tabview_bug627288.js times out on try mostly on debug machines - probably a timing issue - investigating.
(Assignee)

Comment 7

6 years ago
Panorama does now use the thumbnail service (bug 745303).
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.