Bug 30996 – `localStorage.setItem` can overwrite `localStorage` methods

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[kangax]

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.

[kangax]

Can anyone please comment on this? Still happens in r53036.

[Jeremy Orlow]

There have been some WhatWG threads on this.  I'm waiting on clarification by the spec editor.

[Peter Leonov]

Still present in r80833.

[Ben Murdoch]

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.

[Ben Murdoch]

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.

[Kentaro Hara]

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.

[Ben Murdoch]

(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.

[Kentaro Hara]

(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.

[Ben Murdoch]

(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

[WebKit Review Bot]

Comment on attachment 137546 [details]
patch and layout test

Clearing flags on attachment: 137546

Committed r114427: <http://trac.webkit.org/changeset/114427>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.