Bug 49329 – Do not allow access to session and local storage when in private browsing mode

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Anton D'Auria]

WebKit shouldn't allow read or write access to session and local storage when in private browsing mode.

[Anton D'Auria]

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.

[Darin Adler]

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

[Anton D'Auria]

Created attachment 73561 [details]
[PATCH] Do not allow access to session and local storage when in private browsing mode (with ChangeLog)

Now includes ChangeLog.

[Darin Adler]

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.

[Anton D'Auria]

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.

[Darin Adler]

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.

[WebKit Commit Bot]

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 [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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

[Anton D'Auria]

Created attachment 73951 [details]
Adding layout test update

[Eric Seidel (no email)]

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.

[Kevin Decker]

Comment on attachment 73951 [details]
Adding layout test update

r=me

[WebKit Commit Bot]

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 [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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 [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 73951 [details]
Adding layout test update

Clearing flags on attachment: 73951

Committed r73015: <http://trac.webkit.org/changeset/73015>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

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 [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Review Bot]

http://trac.webkit.org/changeset/73015 might have broken GTK Linux 32-bit Debug