Summary: | drop support for sessionStorage in sandbox iframes | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | eduardo <evn> | ||||||
Component: | Frames | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, ap, commit-queue, dglazkov, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
eduardo
2010-04-26 17:20:37 PDT
the previous code will set document.title to "busted", if the page is saving sensitive information in there, then it may introduce security problems. According to http://dev.w3.org/html5/webstorage/#the-sessionstorage-attribute, we should have one per origin. a sandboxed iframe is considered to have the same origin, or a new "unique" origin? from http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html "When the attribute is set, the content is treated as being from a unique origin" We're interested in the case where it has a unique origin. The "correct" behavior is probably to create a new storage area for the unique origin. I'm working up a patch that throws a security exception because that matches what we're doing for localStorage. is it useful to have sessionStorage on a window that has a unique origin every time is created? oh nvm, I just read your comment. a sec error is cool Here's the patch that turned this on: http://trac.webkit.org/changeset/56002 Created attachment 55155 [details]
Patch
Attachment 55155 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2223001 Created attachment 55170 [details]
Patch
Comment on attachment 55170 [details]
Patch
WebCore/storage/StorageEventDispatcher.cpp:60
+ if (!ec)
Could/should this be an early return instead of further indent to the line-that-never-wrapped?
WebCore/storage/StorageEventDispatcher.cpp:61
+ frames[i]->document()->enqueueEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, sourceFrame->document()->url(), storage));
I'm rather anti-wrap, but I think even I might have used a local variable here.
WebKit/chromium/src/StorageAreaProxy.cpp:130
+ Storage* storage = frames[i]->domWindow()->sessionStorage(ec);
Why not just check storage != NULL?
Seems OK, even if the EC handling is slightly inelegant.
Comment on attachment 55170 [details]
Patch
Adam should see my comments first.
Comment on attachment 55170 [details]
Patch
I think its ok for now. I copied the style from localStorage below. If we want to change it, we should change both at once.
Comment on attachment 55170 [details] Patch Clearing flags on attachment: 55170 Committed r58873: <http://trac.webkit.org/changeset/58873> All reviewed patches have been landed. Closing bug. |