Bug 30996 - `localStorage.setItem` can overwrite `localStorage` methods
Summary: `localStorage.setItem` can overwrite `localStorage` methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P2 Normal
Assignee: Ben Murdoch
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-01 13:24 PST by kangax
Modified: 2012-04-17 13:32 PDT (History)
9 users (show)

See Also:


Attachments
patch and layout test (7.50 KB, patch)
2012-04-17 08:59 PDT, Ben Murdoch
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description kangax 2009-11-01 13:24:07 PST
It's currently possible to overwrite methods of `localStorage` simply by setting values of same-named keys through `setItem`:

localStorage.setItem('clear', 'x'); 
localStorage.clear; // "x", not original "clear" method.

This is rather destructive and probably shouldn't happen. Mozilla, for example, allows to overwrite methods of `localStorage` but only through direct accessors (localStorage.clear = 'x'). Using getItem/setItem allows to use any key and not worry about one of them overwriting actual `clearStorage` method.

This still happens in r50383.

Thank you.
Comment 1 kangax 2010-01-09 12:40:46 PST
Can anyone please comment on this? Still happens in r53036.
Comment 2 Jeremy Orlow 2010-10-25 13:04:39 PDT
There have been some WhatWG threads on this.  I'm waiting on clarification by the spec editor.
Comment 3 Peter Leonov 2011-03-16 17:11:13 PDT
Still present in r80833.
Comment 4 Ben Murdoch 2012-04-17 08:43:10 PDT
There is a Chromium bug for this issue: http://code.google.com/p/chromium/issues/detail?id=110216

I have a patch for both JSC and V8 bindings with a layout test to follow shortly.
Comment 5 Ben Murdoch 2012-04-17 08:59:36 PDT
Created attachment 137546 [details]
patch and layout test

Patch and layout test for this issue. This brings us in line with the implementation in FF and IE.
Comment 6 Kentaro Hara 2012-04-17 09:12:04 PDT
Comment on attachment 137546 [details]
patch and layout test

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

> Source/WebCore/bindings/v8/custom/V8StorageCustom.cpp:77
> +    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())

Nit: .IsEmpty() is not necessary.
Comment 7 Ben Murdoch 2012-04-17 09:53:21 PDT
(In reply to comment #6)

Thanks for the review!

> (From update of attachment 137546 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137546&action=review
> 
> > Source/WebCore/bindings/v8/custom/V8StorageCustom.cpp:77
> > +    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())
> 
> Nit: .IsEmpty() is not necessary.

Hm, I want to test whether there is a prototype property present (note the ! at the start of the condition) - removing the ! and .isEmpty() gives me a compiler error.
Comment 8 Kentaro Hara 2012-04-17 09:54:36 PDT
(In reply to comment #7)
> Hm, I want to test whether there is a prototype property present (note the ! at the start of the condition) - removing the ! and .isEmpty() gives me a compiler error.

Ah, got it. Then let's use .isEmpty(). Sorry for the confusion.
Comment 9 Ben Murdoch 2012-04-17 10:01:46 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Hm, I want to test whether there is a prototype property present (note the ! at the start of the condition) - removing the ! and .isEmpty() gives me a compiler error.
> 
> Ah, got it. Then let's use .isEmpty(). Sorry for the confusion.

No problem, thanks! Will put it into the commit queue. Cheers, Ben
Comment 10 WebKit Review Bot 2012-04-17 13:32:41 PDT
Comment on attachment 137546 [details]
patch and layout test

Clearing flags on attachment: 137546

Committed r114427: <http://trac.webkit.org/changeset/114427>
Comment 11 WebKit Review Bot 2012-04-17 13:32:46 PDT
All reviewed patches have been landed.  Closing bug.