Bug 49329 - Do not allow access to session and local storage when in private browsing mode
Summary: Do not allow access to session and local storage when in private browsing mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-10 10:51 PST by Anton D'Auria
Modified: 2010-12-01 09:18 PST (History)
11 users (show)

See Also:


Attachments
[PATCH] checks privateBrowsingEnabled() on local/session storage access (1.36 KB, patch)
2010-11-10 10:59 PST, Anton D'Auria
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Do not allow access to session and local storage when in private browsing mode (with ChangeLog) (1.99 KB, patch)
2010-11-10 17:31 PST, Anton D'Auria
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Deny local storage access in private browsing (page null check before checking settings) (2.07 KB, patch)
2010-11-10 17:51 PST, Anton D'Auria
no flags Details | Formatted Diff | Diff
Adding layout test update (4.67 KB, patch)
2010-11-15 18:34 PST, Anton D'Auria
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anton D'Auria 2010-11-10 10:51:36 PST
WebKit shouldn't allow read or write access to session and local storage when in private browsing mode.
Comment 1 Anton D'Auria 2010-11-10 10:59:00 PST
Created attachment 73510 [details]
[PATCH] checks privateBrowsingEnabled() on local/session storage access

Check privateBrowsingEnabled() on local/session storage reads from WebCore::Storage. WebCore::StorageAreaImpl checks the privateBrowsingEnabled setting on writes, so perhaps this patch should be in StorageAreaImpl, though that would require a change to the interface to pass down a frame pointer. I would actually prefer moving all the checks to WebCore::Storage. If the reviewer thinks we should deny access at the same level, -r this patch.
Comment 2 Darin Adler 2010-11-10 17:04:39 PST
Comment on attachment 73510 [details]
[PATCH] checks privateBrowsingEnabled() on local/session storage access

Patch looks good, but it’s missing a change log entry.

review- because of lack of ChangeLog
Comment 3 Anton D'Auria 2010-11-10 17:31:26 PST
Created attachment 73561 [details]
[PATCH] Do not allow access to session and local storage when in private browsing mode (with ChangeLog)

Now includes ChangeLog.
Comment 4 Darin Adler 2010-11-10 17:35:25 PST
Comment on attachment 73561 [details]
[PATCH] Do not allow access to session and local storage when in private browsing mode (with ChangeLog)

View in context: https://bugs.webkit.org/attachment.cgi?id=73561&action=review

> WebCore/storage/Storage.cpp:59
> -    if (!m_frame)
> +    if (!m_frame || m_frame->page()->settings()->privateBrowsingEnabled())

We need to null-check page too. It’s possible for a frame to outlive its page and thus have 0 for a page. But settings is guaranteed to be non-zero for any page.

Sorry, I missed that earlier.
Comment 5 Anton D'Auria 2010-11-10 17:51:40 PST
Created attachment 73563 [details]
[PATCH] Deny local storage access in private browsing (page null check before checking settings)

Adding Page pointer null check before checking private browsing setting.
Comment 6 Darin Adler 2010-11-11 07:12:09 PST
Comment on attachment 73563 [details]
[PATCH] Deny local storage access in private browsing (page null check before checking settings)

Could also have done the early return was page was 0, but this should be OK.
Comment 7 WebKit Commit Bot 2010-11-11 09:30:35 PST
The commit-queue encountered the following flaky tests while processing attachment 73563 [details]:

storage/domstorage/localstorage/private-browsing-affects-storage.html
media/video-reverse-play-duration.html

Please file bugs against the tests.  These tests were authored by beidson@apple.com, eric.carlson@apple.com, and mrowe@apple.com.  The commit-queue is continuing to process your patch.
Comment 8 WebKit Commit Bot 2010-11-11 11:14:28 PST
Comment on attachment 73563 [details]
[PATCH] Deny local storage access in private browsing (page null check before checking settings)

Rejecting patch 73563 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
......................
printing/css2.1 .........
scrollbars ..................
security ....
storage ..................................
storage/domstorage ......
storage/domstorage/events .....
storage/domstorage/localstorage .....
storage/domstorage/localstorage/private-browsing-affects-storage.html -> failed

Exiting early after 1 failures. 18370 tests run.
465.07s total testing time

18369 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
10 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/5469115
Comment 9 Anton D'Auria 2010-11-15 18:34:26 PST
Created attachment 73951 [details]
Adding layout test update
Comment 10 Eric Seidel (no email) 2010-11-16 03:20:46 PST
Comment on attachment 73563 [details]
[PATCH] Deny local storage access in private browsing (page null check before checking settings)

Cleared Darin Adler's review+ from obsolete attachment 73563 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 11 Kevin Decker 2010-11-30 19:10:14 PST
Comment on attachment 73951 [details]
Adding layout test update

r=me
Comment 12 WebKit Commit Bot 2010-11-30 20:45:37 PST
The commit-queue encountered the following flaky tests while processing attachment 73951 [details]:

fast/dom/onerror-img.html
fast/preloader/script.html

Please file bugs against the tests.  These tests were authored by abarth@webkit.org.  The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2010-12-01 03:29:57 PST
The commit-queue encountered the following flaky tests while processing attachment 73951 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html
animations/dynamic-stylesheet-loading.html

Please file bugs against the tests.  These tests were authored by cmarrin@apple.com and dumi@chromium.org.  The commit-queue is continuing to process your patch.
Comment 14 WebKit Commit Bot 2010-12-01 04:05:47 PST
Comment on attachment 73951 [details]
Adding layout test update

Clearing flags on attachment: 73951

Committed r73015: <http://trac.webkit.org/changeset/73015>
Comment 15 WebKit Commit Bot 2010-12-01 04:05:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Commit Bot 2010-12-01 04:19:21 PST
The commit-queue encountered the following flaky tests while processing attachment 73951 [details]:

fast/preloader/script.html
animations/dynamic-stylesheet-loading.html

Please file bugs against the tests.  These tests were authored by abarth@webkit.org and cmarrin@apple.com.  The commit-queue is continuing to process your patch.
Comment 17 WebKit Review Bot 2010-12-01 09:18:16 PST
http://trac.webkit.org/changeset/73015 might have broken GTK Linux 32-bit Debug