Bug 32369 - Support for storage and databases in sandboxed iframes
Summary: Support for storage and databases in sandboxed iframes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-10 02:18 PST by Patrik Persson
Modified: 2010-03-15 10:14 PDT (History)
5 users (show)

See Also:


Attachments
Patch for updated HTML5 sandbox support (10.43 KB, patch)
2010-03-12 06:52 PST, Patrik Persson
abarth: review-
Details | Formatted Diff | Diff
Patch that builds with Chromium (13.96 KB, patch)
2010-03-15 07:37 PDT, Patrik Persson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrik Persson 2009-12-10 02:18:50 PST
This is a followup to bug 21288, which concerned the implementation of
the HTML5 iframe sandbox attribute.

How should WebKit interpret the HTML5 spec regarding sandboxed storage
and databases?  I believe the HTML5 spec does not say much explicitly
on this, but rather relies on the origin sandboxing.  Here is my
interpretation.

* I think sessionStorage would make sense with sandboxed origins.

* I think localStorage would end up equivalent to sessionStorage in a
  sandboxed frame, making it somewhat less useful. (The unique origin
  of a sandboxed frame means, in my interpretation, that the same
  frame would not be able to access its own localStorage in another
  session.)

* Similarly, I think a sandboxed database would be useful only within
  a session.  The database could be reclaimed when the session ends.
  This defeats much of the purpose of databases, but perhaps it would
  still be useful for compatibility.

The current implementation disables storage and databases in sandboxed
frames.  There is some more discussion in the thread for bug 21288,
comments 43..49:

https://bugs.webkit.org/show_bug.cgi?id=21288#c43
Comment 1 Adam Barth 2010-01-09 00:38:33 PST
I'll forward this question to the What WG.
Comment 2 Patrik Persson 2010-03-12 06:52:07 PST
Created attachment 50593 [details]
Patch for updated HTML5 sandbox support
Comment 3 Patrik Persson 2010-03-12 06:53:58 PST
(This comment really concerns the patch just submitted in comment #2.
I was a bit quick on the "submit" button.)

Adam, thanks for bringing this up in the What WG.

  http://www.mail-archive.com/whatwg@lists.whatwg.org/msg19786.html

This is a patch to match the updated HTML5 spec wrt. iframe
sandboxing.  The patch does two things:

- Enables window.sessionStorage in sandboxed iframes.  (Removed
  sandboxing check.)

- Raises SECURITY_ERR exceptions when window.localStorage or
  window.openDatabase() is accessed in a sandboxed iframe.  (Note:
  window.sessionStorage does not raise exceptions.)

The patched behavior is in accordance with HTML5:

  http://dev.w3.org/html5/webstorage/   (sections 4.2 and 4.3)
  http://dev.w3.org/html5/webdatabase/  (section 4.1)

Raising exceptions in situations where WebKit would previously return
null could raise questions about compatibility.  However, iframe
sandboxing is a recent experimental feature, currently implemented
only by WebKit.
Comment 4 WebKit Review Bot 2010-03-12 07:10:43 PST
Attachment 50593 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/656025
Comment 5 Adam Barth 2010-03-12 09:06:28 PST
Comment on attachment 50593 [details]
Patch for updated HTML5 sandbox support

This patch looks good, thanks.  If you'd be willing to fix the chromium build error, we'll be good to go.
Comment 6 Patrik Persson 2010-03-15 07:37:52 PDT
Created attachment 50700 [details]
Patch that builds with Chromium

Sorry about the build error.  Here's an updated patch that builds on
Chromium.  Apart from the Chromium fix, this patch is identical to the
previous one.
Comment 7 Darin Adler 2010-03-15 09:51:22 PDT
Comment on attachment 50700 [details]
Patch that builds with Chromium

> -    bool isLocalStorage = (frame->domWindow()->localStorage() == storage);
> +    ExceptionCode ec = 0;
> +    bool isLocalStorage = (frame->domWindow()->localStorage(ec) == storage && !ec);'

> -    bool isLocalStorage = storage->frame()->domWindow()->localStorage() == storage;
> +    ExceptionCode ec = 0;
> +    bool isLocalStorage = (storage->frame()->domWindow()->localStorage(ec) == storage && !ec);

Since the localStorage function returns 0 when there is an error, the !ec check seems unnecessary.

> -        for (unsigned i = 0; i < frames.size(); ++i)
> -            frames[i]->document()->enqueueStorageEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, sourceFrame->document()->url(), frames[i]->domWindow()->localStorage()));
> +        for (unsigned i = 0; i < frames.size(); ++i) {
> +            ExceptionCode ec = 0;
> +            Storage* storage = frames[i]->domWindow()->localStorage(ec);
> +            if (!ec)
> +                frames[i]->document()->enqueueStorageEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, sourceFrame->document()->url(), storage));
> +        }

I think it would be cleaner to just check the returned storage object to see if it's 0 rather than checking ec. We do need the ExceptionCode to communicate with the JavaScript binding, but I don't think we need to use it inside the engine itself.

Minor issues.

r=me based on my own reading of the code, the EWS results, and Adam Barth’s earlier review
Comment 8 WebKit Commit Bot 2010-03-15 10:14:16 PDT
Comment on attachment 50700 [details]
Patch that builds with Chromium

Clearing flags on attachment: 50700

Committed r56002: <http://trac.webkit.org/changeset/56002>
Comment 9 WebKit Commit Bot 2010-03-15 10:14:21 PDT
All reviewed patches have been landed.  Closing bug.