1765337 - Make StorageDisabledByAntiTracking variants private to the antitracking component

Status: NEW

(Core :: Privacy: Anti-Tracking, enhancement, P3)

Description

[Benjamin VanderSloot [:bvandersloot]]

These functions modify an argument uint32_t& aRejectedReason to pass a significant information to the caller about their internals. This is very leaky and pushes a lot of logic to the call site in how to handle the various flags listed here:

https://searchfox.org/mozilla-central/rev/4b3039b48c3cb67774270ebcc2a7d8624d888092/uriloader/base/nsIWebProgressListener.idl#328-348

Comment 1

[Tim Huang[:timhuang]]

The content blocking event flags will be used not only by the AT module but also by the UI and the Geckoview. So, we need to be careful about that if we want to move them to somewhere else.

Comment 2

[Dan Mehic]

Checked this against current central while triaging bug 1765153. It needs re-scoping rather than closing.

The named symbol is gone. StorageDisabledByAntiTracking has zero matches anywhere in the tree.

The concern it describes is not. The leaky uint32_t& aRejectedReason out-parameter that comment 0 objects to now lives in StorageAccess.h:

bool ShouldAllowAccessFor(nsPIDOMWindowInner* a3rdPartyTrackingWindow,
                          nsIURI* aURI, bool aCookies,
                          uint32_t* aRejectedReason);

bool ShouldAllowAccessFor(nsIChannel* aChannel, nsIURI* aURI,
                          uint32_t* aRejectedReason);

plus StorageAllowedForWindow, which takes the same out-parameter. The header even documents the five nsIWebProgressListener::STATE_COOKIES_BLOCKED_* values a caller has to interpret, which is precisely the "pushes a lot of logic to the call site" problem in comment 0.

Six files outside the antitracking component call these: netwerk/cookie/CookieCommons.cpp, caps/BasePrincipal.cpp, dom/security/ReferrerInfo.cpp, dom/base/ThirdPartyUtil.cpp, dom/base/nsGlobalWindowInner.cpp and dom/base/Document.cpp. So the goal of keeping these variants private to the component is not met — it just applies to differently named functions now.

Worth noting this now overlaps heavily with bug 1765340, which is explicitly about auditing ShouldAllowAccessFor. They may be better handled as one piece of work, or 1765337 duplicated into it.