Open Bug 1607302 Opened 5 years ago Updated 2 years ago

Move the access checks for IDB to the IDBFactory methods which access the database

Categories

(Core :: Storage: IndexedDB, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: ehsan.akhgari, Unassigned)

References

(Blocks 1 open bug)

Details

Right now we do the storage access checks immediately when the web page code access the indexedDB global property. However Chromium and WebKit both perform these checks when the user tries to access a database (e.g. by calling open). This means when using third-party cookie blocking or ETP we may cause more page breakage than other engines.

(The behaviour isn't completely consistent across other engines: WebKit throws from open, Chromium returns an IDBRequest which will fail with error DOMException: The user denied permission to access the database.)

Could we change our behaviour to align it with other engines?

Flags: needinfo?(amarchesini)

I was confused by this when starting to work on implementing IDBFactory.databases (not done yet). Web platform test cases such as https://searchfox.org/mozilla-central/source/testing/web-platform/tests/IndexedDB/idbfactory-databases-opaque-origin.html and https://searchfox.org/mozilla-central/source/testing/web-platform/tests/IndexedDB/idbfactory-open-opaque-origin.html do not specifically check whether the databases resp. open operations throw a SecurityError, but if this happens in a larger code block, which includes accessing indexedDB (not sure how this could be differentiated better).

However, these tests are for one particular case of a SecurityError, but the only one mentioned in the spec explicitly (https://w3c.github.io/IndexedDB/#dom-idbfactory-open): If origin is an opaque origin, throw a "SecurityError" DOMException and abort these steps.

Maybe the spec should clarify this first for other SecurityErrors, and then we change our implementation to comply with that?

(In reply to Simon Giesecke [:sg] [he/him] from comment #1)

Maybe the spec should clarify this first for other SecurityErrors, and then we change our implementation to comply with that?

Yes, I think it makes sense to pursue this through the spec. The IDB spec editor has been incredibly responsive and I expect would be very receptive to normalizing this, especially if we're willing to help write up the revised spec text.

One interesting thing is that in specs where we're using Promises is that the WebIDL binding layer normalizes our synchronous error returns into Promise reject()ions (per spec). So we didn't so much as go out of our way to be consistent with other implementations with newer APIs, but instead WebIDL and Promises did it for us. (If the return values were objects instead of promises, we'd still be throwing synchronously much of the time when it's due to a storage access check failure.)

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #2)

(In reply to Simon Giesecke [:sg] [he/him] from comment #1)

Maybe the spec should clarify this first for other SecurityErrors, and then we change our implementation to comply with that?

Yes, I think it makes sense to pursue this through the spec. The IDB spec editor has been incredibly responsive and I expect would be very receptive to normalizing this, especially if we're willing to help write up the revised spec text.

That would be great! It would be nice to loop in the other implementers as well to agree on what behaviour we want to have.

Maybe the spec should clarify this first for other SecurityErrors, and then we change our implementation to comply with that?

This is a good point. wondering if we want to do something similar for other APIs such as localStorage, BroadcastChannel.postMessage(), etc.

Flags: needinfo?(amarchesini)
Priority: -- → P3

(In reply to Andrea Marchesini [:baku] from comment #4)

Maybe the spec should clarify this first for other SecurityErrors, and then we change our implementation to comply with that?

This is a good point. wondering if we want to do something similar for other APIs such as localStorage, BroadcastChannel.postMessage(), etc.

I filed a similar bug for localStorage.

For BroadcastChannel, Blink throws ReferenceError: BroadcastChannel is not defined when trying to construct a new object where storage access is denied. We throw a SecurityError.

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.