Bug 49332 – Do not allow access to existing HTML5 databases in private browsing mode

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

[Anton D'Auria]

WebKit shouldn't allow access to existing HTML5 databases when in private browsing mode.

[Anton D'Auria]

Created attachment 73515 [details]
[PATCH] Denies read access to databases in private browsing mode

[Darin Adler]

Comment on attachment 73515 [details]
[PATCH] Denies read access to databases in private browsing mode

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

review- because of lack of ChangeLog

[Anton D'Auria]

Created attachment 73560 [details]
[PATCH] Denies read access to databases in private browsing mode (with ChangeLog)

Patch now includes ChangeLog.

[WebKit Commit Bot]

Comment on attachment 73560 [details]
[PATCH] Denies read access to databases in private browsing mode (with ChangeLog)

Rejecting patch 73560 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:
...
fast/tokenizer ........................................
fast/transforms .......................
fast/url ....................
fast/workers .............................................
fast/workers/storage ....................
fast/workers/storage/read-and-write-transactions-dont-run-together.html -> failed

Exiting early after 1 failures. 16196 tests run.
274.85s total testing time

16195 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/5610009

[Anton D'Auria]

Created attachment 75831 [details]
[PATCH] separate read-only transaction state from private browsing no read-write state

Previously, read-only transactions and private browsing mode were represented by the same SQLStatement and DatabaseAuthorizer states. This patch removes the m_readOnly member variable from SQLStatement and DatabaseAuthorizer, and replaces it with m_permissions whose bit fields are initialized by a DatabaseAuthorizer enum Permissions (ReadWrite, ReadOnly, NoAccess). A read-only transaction sets permissions to ReadOnly, and if !m_database->scriptExecutionContext()->allowDatabaseAccess(), then permissions also set to NoAccess.

[WebKit Review Bot]

Attachment 75831 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anton D'Auria]

Created attachment 76132 [details]
[PATCH] separate read-only transaction state from private browsing no read-write state (renamed mask variables)

[Anton D'Auria]

Created attachment 76135 [details]
[PATCH] separate read-only transaction state from private browsing no read-write state (fixed ChangeLog conflict)

[Eric Seidel (no email)]

Comment on attachment 73560 [details]
[PATCH] Denies read access to databases in private browsing mode (with ChangeLog)

Cleared Darin Adler's review+ from obsolete attachment 73560 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Darin Adler]

Comment on attachment 76135 [details]
[PATCH] separate read-only transaction state from private browsing no read-write state (fixed ChangeLog conflict)

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

r=me, but the enum/bit combo is a bit ugly, the bitifield use here is sloppy and there is some incorrectly formatted code

> WebCore/storage/DatabaseAuthorizer.h:97
> +    void setPermissions(int permissions);

It doesn’t seem right to use int for permissions, but use an enum for the values. I think we normally have a better idiom for this sort of thing.

> WebCore/storage/DatabaseAuthorizer.h:114
> +    int m_permissions;
>      bool m_securityEnabled : 1;

All these bools are set to be a single bit to save memory, I suppose, and then the three bit permissions is taking up an entire 32-bit int! I think the use of bitfields on the bools is excessive. You could use a bitfield for permissions, but because we are using a signed type, int, we’d need to use four bits. It would be better to use unsigned and three bits. Or not use bitfields at all in this class.

> WebCore/storage/SQLTransactionSync.cpp:94
> +    if (!m_database->scriptExecutionContext()->allowDatabaseAccess())
> +      permissions |= DatabaseAuthorizer::NoAccessMask;
> +    else if (m_readOnly)
> +      permissions |= DatabaseAuthorizer::ReadOnlyMask;

This is not indented correctly.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 76135 [details]:

inspector/styles-new-API.html bug 50889 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 76135 [details]
[PATCH] separate read-only transaction state from private browsing no read-write state (fixed ChangeLog conflict)

Clearing flags on attachment: 76135

Committed r74093: <http://trac.webkit.org/changeset/74093>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.