Bug 238158 – Check if origin can access storage in Storage API

• Status: RESOLVED
• Resolution: FIXED
• Component: Website Storage

[Sihui Liu]

https://storage.spec.whatwg.org/#storage-keys

[Sihui Liu]

Created attachment 455287 [details]
Patch

[Sihui Liu]

Created attachment 455288 [details]
Patch

[Chris Dumez]

Comment on attachment 455288 [details]
Patch

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

> Source/WebCore/Modules/storage/StorageManager.cpp:73
> +    if (!origin->canAccessStorage())

The spec merely says to check if the origin is opaque (which we call unique in WebKit). Why not simply check `origin->isUnique()` ?

> Source/WebCore/page/SecurityOrigin.h:154
> +    bool canAccessStorage() const { return canAccessStorage(nullptr); }

There is a comment inside canAccessStorage() saying that we should stop passing nullptr so this may not be a change in the right direction.

[Sihui Liu]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 455288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455288&action=review
> 
> > Source/WebCore/Modules/storage/StorageManager.cpp:73
> > +    if (!origin->canAccessStorage())
> 
> The spec merely says to check if the origin is opaque (which we call unique
> in WebKit). Why not simply check `origin->isUnique()` ?

I see the other APIs using canAccessStorage to check so I use it for consistency.
I guess we can only check if origin is unique, though I don't know why we can ignore storage blocking policy for Storage API.

> 
> > Source/WebCore/page/SecurityOrigin.h:154
> > +    bool canAccessStorage() const { return canAccessStorage(nullptr); }
> 
> There is a comment inside canAccessStorage() saying that we should stop
> passing nullptr so this may not be a change in the right direction.

[Chris Dumez]

Comment on attachment 455288 [details]
Patch

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

>>> Source/WebCore/Modules/storage/StorageManager.cpp:73
>>> +    if (!origin->canAccessStorage())
>> 
>> The spec merely says to check if the origin is opaque (which we call unique in WebKit). Why not simply check `origin->isUnique()` ?
> 
> I see the other APIs using canAccessStorage to check so I use it for consistency.
> I guess we can only check if origin is unique, though I don't know why we can ignore storage blocking policy for Storage API.

Ok, so you want to do more checks that simply opaque origin, is that right? The change log doesn't say anything about that (and neither does the spec).

The change may well be correct if so.

[Sihui Liu]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 455288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455288&action=review
> 
> >>> Source/WebCore/Modules/storage/StorageManager.cpp:73
> >>> +    if (!origin->canAccessStorage())
> >> 
> >> The spec merely says to check if the origin is opaque (which we call unique in WebKit). Why not simply check `origin->isUnique()` ?
> > 
> > I see the other APIs using canAccessStorage to check so I use it for consistency.
> > I guess we can only check if origin is unique, though I don't know why we can ignore storage blocking policy for Storage API.
> 
> Ok, so you want to do more checks that simply opaque origin, is that right?
> The change log doesn't say anything about that (and neither does the spec).
> 
> The change may well be correct if so.

Yes, I would like to make this consistent with IndexedDB, whose spec only says about opaque origin(https://www.w3.org/TR/IndexedDB/#dom-idbfactory-open), but we also check storage blocking policy of the origin (we don't check its top origin, so we pass nullptr). I will update changelog to include this.

[Chris Dumez]

Comment on attachment 455288 [details]
Patch

Ok.

[Sihui Liu]

Created attachment 455429 [details]
Patch for landing

[EWS]

Committed r291726 (248758@main): <https://commits.webkit.org/248758@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455429 [details].

[Radar WebKit Bug Importer]

<rdar://problem/90667430>