Summary: | Support for storage and databases in sandboxed iframes | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrik Persson <patrik.j.persson> | ||||||
Component: | Frames | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, commit-queue, darin, dglazkov, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Patrik Persson
2009-12-10 02:18:50 PST
I'll forward this question to the What WG. Created attachment 50593 [details]
Patch for updated HTML5 sandbox support
(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. Attachment 50593 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/656025 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.
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 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 on attachment 50700 [details] Patch that builds with Chromium Clearing flags on attachment: 50700 Committed r56002: <http://trac.webkit.org/changeset/56002> All reviewed patches have been landed. Closing bug. |