| Summary: | `localStorage.setItem` can overwrite `localStorage` methods | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | kangax <kangax> | ||||
| Component: | DOM | Assignee: | Ben Murdoch <benm> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abarth, benm, gojpeg, haraken, japhet, jorlow, michaeln, oliver, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Mac (Intel) | ||||||
| OS: | OS X 10.6 | ||||||
| Attachments: |
|
||||||
Can anyone please comment on this? Still happens in r53036. There have been some WhatWG threads on this. I'm waiting on clarification by the spec editor. 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. 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 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. (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. (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. (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 on attachment 137546 [details] patch and layout test Clearing flags on attachment: 137546 Committed r114427: <http://trac.webkit.org/changeset/114427> All reviewed patches have been landed. Closing bug. |
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.