1515693 - Check allow-storage-access-by-user-activation sandbox flag only if StorageAccess API is enabled

Status: VERIFIED FIXED

(Firefox :: Protections UI, defect)

Description

[Andrea Marchesini [:baku]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1505571#c40

We need to check allow-storage-access-by-user-activation sandbox flag only if Storage Access API is enabled.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: a.patch

Comment 2

[(no longer active)]

Comment on attachment 9032740 [details] [diff] [review]
a.patch

Review of attachment 9032740 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindowInner.cpp
@@ +5804,5 @@
>    return topLevelPrincipal;
>  }
>  
>  nsIPrincipal* nsGlobalWindowInner::GetTopLevelStorageAreaPrincipal() {
> +  if (StaticPrefs::dom_storage_access_enabled() &&

No, this does the wrong thing.  You're ANDing this condition with the result of ORing the sandbox check and the private browsing check.  Do you see the problem?

In order to ensure this kind of mistake doesn't happen, let's add an inline helper to nsIDocument for determining whether the document has its storage access sandboxed: StorageAccessSandboxed() and use it here.

::: netwerk/base/LoadInfo.cpp
@@ +176,5 @@
>                innerWindow->GetTopLevelStorageAreaPrincipal();
>          } else if (contextOuter->IsTopLevelWindow()) {
>            nsIDocument* doc = innerWindow->GetExtantDoc();
>            if (!doc ||
> +              !StaticPrefs::dom_storage_access_enabled() ||

A similar bug exists here, FWIW.

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +73,5 @@
>    }
>  
>    nsIDocument* doc = a3rdPartyTrackingWindow->GetDocument();
>    // Make sure storage access isn't disabled
> +  if (StaticPrefs::dom_storage_access_enabled() &&

Here too!

Comment 3

[Andrea Marchesini [:baku]]

Attachment: a.patch

Yeah, I don't know why I wrote that patch in that way. Maybe I was just too tired.

Comment 4

[(no longer active)]

Comment on attachment 9032862 [details] [diff] [review]
a.patch

Review of attachment 9032862 [details] [diff] [review]:
-----------------------------------------------------------------

Please make sure this patch is formatted correctly before landing.  r=me on the properly formatted version of the patch.

::: dom/base/nsIDocument.h
@@ +1342,5 @@
>  
> +  // Helper method that returns true if the document has storage-access sandbox
> +  // flag.
> +  bool StorageAccessSandboxed() const
> +  {

Please modify your text editor to run clang-format when you save the files, so that your patches don't corrupt the formatting of the code base from now on!

Thank you.

Comment 5

[Andrea Marchesini [:baku]]

> Please modify your text editor to run clang-format when you save the files,
> so that your patches don't corrupt the formatting of the code base from now
> on!

I do. but this needs to be uplift to beta and I wrote and tested the patch directly on the beta-tree.

Comment 6

[Andrea Marchesini [:baku]]

I just pushed this patch to try. Ehsan, can you uplift if it lands to m-c?

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/23941690a630

Comment 8

[(no longer active)]

Comment on attachment 9032862 [details] [diff] [review]
a.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Not a regression

User impact if declined: Comment 0

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: Comment 0

While testing, make sure the dom.storage_access.enabled pref is set to false.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just adds a pref check.

String changes made/needed: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9032862 [details] [diff] [review]
a.patch

[Triage Comment]
Fix needed for the Storage Access API. Approved for 65.0b7.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3bd5407d6283

Comment 11

[Kanchan Kumari QA]

I could reproduce the issue mentioned in bug 1505571 on 65.0b5 (Build ID 20181217180946).

I verified this bug fix (dom.storage_access.enabled pref is set to false)on the latest nightly 66.0a1 (Build ID 20181227092648) and beta 65.0b7(Build ID 20181227144402) on Windows 10 x64, Mac OS 10.14.2 and Ubuntu 16.04.