1492036 - Considering implementing the Reporting API

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P3)

Description

[Andrea Marchesini [:baku]]

Reporting API is needed for FeaturePolicy and other new APIs.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - Header Parsing

This first patch is about parsing the Report-To header and create a data struct is memory.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - WebIDL interfaces

Main WebIDL interfaces

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - DeprecationReportBody interface

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - Report Deprecation API usage

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 5 - IPC

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 6 - partially data sending

WIP, patch unfinished.

Comment 8

[Andrea Marchesini [:baku]]

I still don't know if I have time to work on this. If somebody wants to continue these patches, I'm happy to help.

Comment 9

[Mats Palmgren (inactive)]

Has our legal team / privacy team reviewed this feature?
In particular, its compliance with the GDPR laws in Europe.

Personally, I find the discussion of Privacy in the spec under
https://w3c.github.io/reporting/#privacy
rather naive.  It fails to recognize that some of those practices
could very well be illegal under the GDPR.  Implementing these
features in the UA instead might make the UA vendor liable,
which it would not be if the web page itself did the tracking.

Also, https://w3c.github.io/reporting/#disable
says "User agents MUST allow users to disable reporting".
The GDPR requires that the user has given explicit consent using
a "positive opt-in" mechanism before collecting any personal data.
An opt-out mechanism is not good enough.
https://ico.org.uk/for-organisations/guide-to-the-general-data-protection-regulation-gdpr/lawful-basis-for-processing/consent/

Comment 10

[Andrea Marchesini [:baku]]

Thanks for pointing out this issue. Privacy is one of the reason why I stopped the implementation before implementing the sending of the reports.

Just yesterday I was discussing with ckerschb the idea of landing just part of this API.
Reporting API is composed, basically, by 3 independent blocks:
 - the report-to header and the delivering of report messages to a server.
 - ReportingObserver webIDL component which allow a context to receive reports for its own origin (similar to BroadcastChannel).
 - A set of ReportBody: deprecated APIs, feature policy violations, and so on.

What I propose is to implement just the last 2 parts, without sending reports to servers.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

But is that compatible with other browsers? Though, perhaps we could pretend that as if user had always opted-out. What do other browsers do (is that Chrome in practice)?

If the spec doesn't comply with GDPR, it needs to be changed. Has anyone filed a spec issue?
Or contacted legal to ask feedback to this issue?

Comment 12

[Andrea Marchesini [:baku]]

Marshall, can you please help us with this API? We would like to know if this API is OK to be implemented from a GDPR point of view. The spec doesn't say anything in particular. Note that we have a similar implementation for CSP3 violation reporting.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 1 - WebIDL

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - WebIDL for DeprecationReportBody

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 3 - DOM Bindings + Deprecation reports

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 4 - IPC

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 5 - tests

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 5 - tests

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part 6 - FeaturePolicy violation

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 7 - FeaturePolicy WPTs

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 1 - WebIDL

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9015899 [details] [diff] [review]
part 1 - WebIDL

>+ReportingObserver::Constructor(const GlobalObject& aGlobal,
>+                               ReportingObserverCallback& aCallback,
>+                               const ReportingObserverOptions& aOptions,
>+                               ErrorResult& aRv)
>+{
>+  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
>+  MOZ_ASSERT(global);
>+
>+  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(global);
>+  if (NS_WARN_IF(!window)) {
>+    aRv.Throw(NS_ERROR_UNEXPECTED);
>+    return nullptr;
>+  }
>+
>+  // ReportingObserver behaves as BroadcastChannel and we don't want to allow
>+  // the communication between contexts blocked by anti-tracking policies.
>+  if (nsContentUtils::IsThirdPartyWindowOrChannel(window, nullptr,
>+                                                  nullptr) &&
>+      nsContentUtils::StorageAllowedForWindow(window) !=
>+        nsContentUtils::StorageAccess::eAllow) {
>+    aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
>+    return nullptr;
I don't like throwing from a constructor which per spec can't throw. This could break pages (sure, those would be 3rd party but still).
Could we rather make the ReportingObserver somehow dummy or so?
(looks like BroadcastChannel is similarly broken)

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9015795 [details] [diff] [review]
part 3 - DOM Bindings + Deprecation reports


>+++ b/dom/reporting/ReportingUtils.cpp
>@@ -0,0 +1,19 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "mozilla/dom/ReportingUtils.h"
>+#include "mozilla/dom/ReportBody.h"
>+
>+using namespace mozilla::dom;
>+
>+/* static */ void
>+ReportingUtils::Report(const nsAString& aType,
Shouldn't this take nsAtom*. Or, perhaps enum value. That way we'd guarantee no weird things get passed here.
... maybe enum is too restricting and converting to string is harder. So, use atom here?

You'd need to add deprecation to xpcom/ds/StaticAtoms.py

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9015889 [details] [diff] [review]
part 7 - FeaturePolicy WPTs


>-  assert_equals(report.body.feature, "camera");
>+  assert_equals(report.body.featureId, "camera");
there is no featureId in any ReportBodies

check also other properties.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 1 - WebIDL

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 9015889 [details] [diff] [review]
part 7 - FeaturePolicy WPTs

I and smaug have discussed the comments on IRC.

Comment 27

[Andrea Marchesini [:baku]]

Note that FeaturePolicyViolationReportBody is part of https://wicg.github.io/feature-policy

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9015889 [details] [diff] [review]
part 7 - FeaturePolicy WPTs

>-  assert_equals(typeof report.body.message, "string");
>-  assert_equals(typeof report.body.lineNumber, "number");
>-  assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.lineNumber, 25);
>+  assert_equals(report.body.columnNumber, 24);
>+  assert_equals(report.body.disposition, "enforce");
Do all the browsers report exactly the same lines/columns? If not, could you not do this change.
(I guess .message check should be dropped, and disposition check is fine)

> var check_report_format = (reports, observer) => {
>   let report = reports[0];
>-  assert_equals(report.type, "feature-policy");
>+  assert_equals(report.type, "feature-policy-violation");
>   assert_equals(report.url, document.location.href);
>-  assert_equals(report.body.feature, "document-write");
>+  assert_equals(report.body.featureId, "document-write");
>   assert_equals(report.body.sourceFile, document.location.href);
>-  assert_equals(typeof report.body.message, "string");
>-  assert_equals(typeof report.body.lineNumber, "number");
>-  assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.lineNumber, 27);
>+  assert_equals(report.body.columnNumber, 18);
>+  assert_equals(report.body.disposition, "enforce");
similar here


> var check_report_format = (reports, observer) => {
>   let report = reports[0];
>-  assert_equals(report.type, "feature-policy");
>+  assert_equals(report.type, "feature-policy-violation");
>   assert_equals(report.url, document.location.href);
>-  assert_equals(report.body.feature, "fullscreen");
>+  assert_equals(report.body.featureId, "fullscreen");
>   assert_equals(report.body.sourceFile, document.location.href);
>-  assert_equals(typeof report.body.message, "string");
>-  assert_equals(typeof report.body.lineNumber, "number");
>-  assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.lineNumber, 24);
>+  assert_equals(report.body.columnNumber, 32);
>+  assert_equals(report.body.disposition, "enforce");
and here


> var check_report_format = (reports, observer) => {
>   let report = reports[0];
>-  assert_equals(report.type, "feature-policy");
>+  assert_equals(report.type, "feature-policy-violation");
>   assert_equals(report.url, document.location.href);
>-  assert_equals(report.body.feature, "geolocation");
>-  assert_equals(report.body.sourceFile, document.location.href);
>-  assert_equals(typeof report.body.message, "string");
>+  assert_equals(report.body.featureId, "geolocation");
>   assert_equals(typeof report.body.lineNumber, "number");
>   assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.disposition, "enforce");
(and here you do keep typeof checks, which is fine)


> 
> var check_report_format = (reports, observer) => {
>   let report = reports[0];
>-  assert_equals(report.type, "feature-policy");
>+  assert_equals(report.type, "feature-policy-violation");
>   assert_equals(report.url, document.location.href);
>-  assert_equals(report.body.feature, "microphone");
>+  assert_equals(report.body.featureId, "microphone");
>   assert_equals(report.body.sourceFile, document.location.href);
>-  assert_equals(typeof report.body.message, "string");
>-  assert_equals(typeof report.body.lineNumber, "number");
>-  assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.lineNumber, 25);
>+  assert_equals(report.body.columnNumber, 24);
>+  assert_equals(report.body.disposition, "enforce");
but here not.
> var check_report_format = (reports, observer) => {
>   let report = reports[0];
>-  assert_equals(report.type, "feature-policy");
>+  assert_equals(report.type, "feature-policy-violation");
>   assert_equals(report.url, document.location.href);
>-  assert_equals(report.body.feature, "midi");
>+  assert_equals(report.body.featureId, "midi");
>   assert_equals(report.body.sourceFile, document.location.href);
>-  assert_equals(typeof report.body.message, "string");
>-  assert_equals(typeof report.body.lineNumber, "number");
>-  assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.lineNumber, 25);
>+  assert_equals(report.body.columnNumber, 44);
>+  assert_equals(report.body.disposition, "enforce");
ditto

>     <script>
> var t = async_test("PaymentRequest Report Format");
> 
> var check_report_format = (reports, observer) => {
>   let report = reports[0];
>-  assert_equals(report.type, "feature-policy");
>+  assert_equals(report.type, "feature-policy-violation");
>   assert_equals(report.url, document.location.href);
>-  assert_equals(report.body.feature, "payment");
>+  assert_equals(report.body.featureId, "payment");
>   assert_equals(report.body.sourceFile, document.location.href);
>-  assert_equals(typeof report.body.message, "string");
>-  assert_equals(typeof report.body.lineNumber, "number");
>-  assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.lineNumber, 27);
>+  assert_equals(report.body.columnNumber, 8);
>+  assert_equals(report.body.disposition, "enforce");
ditto

> var t = async_test("Sync-xhr Report Format");
> 
> var check_report_format = (reports, observer) => {
>   let report = reports[0];
>-  assert_equals(report.type, "feature-policy");
>+  assert_equals(report.type, "feature-policy-violation");
>   assert_equals(report.url, document.location.href);
>-  assert_equals(report.body.feature, "sync-xhr");
>+  assert_equals(report.body.featureId, "sync-xhr");
>   assert_equals(report.body.sourceFile, document.location.href);
>-  assert_equals(typeof report.body.message, "string");
>-  assert_equals(typeof report.body.lineNumber, "number");
>-  assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.lineNumber, 29);
>+  assert_equals(report.body.columnNumber, 15);
>+  assert_equals(report.body.disposition, "enforce");
ditto

> var t = async_test("USB Report Format");
> 
> var check_report_format = (reports, observer) => {
>   let report = reports[0];
>-  assert_equals(report.type, "feature-policy");
>+  assert_equals(report.type, "feature-policy-violation");
>   assert_equals(report.url, document.location.href);
>-  assert_equals(report.body.feature, "usb");
>+  assert_equals(report.body.featureId, "usb");
>   assert_equals(report.body.sourceFile, document.location.href);
>-  assert_equals(typeof report.body.message, "string");
>-  assert_equals(typeof report.body.lineNumber, "number");
>-  assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.lineNumber, 25);
>+  assert_equals(report.body.columnNumber, 15);
ditto


>     <script>
> var check_report_format = (reports, observer) => {
>   let report = reports[0];
>-  assert_equals(report.type, "feature-policy");
>+  assert_equals(report.type, "feature-policy-violation");
>   assert_equals(report.url, document.location.href);
>-  assert_equals(report.body.feature, "vr");
>+  assert_equals(report.body.featureId, "vr");
>   assert_equals(report.body.sourceFile, document.location.href);
>-  assert_equals(typeof report.body.message, "string");
>-  assert_equals(typeof report.body.lineNumber, "number");
>-  assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.lineNumber, 25);
>+  assert_equals(report.body.columnNumber, 44);
>+  assert_equals(report.body.disposition, "enforce");
ditto

>   <body>
>     <script>
> var check_report_format = (reports, observer) => {
>   let report = reports[0];
>-  assert_equals(report.type, "feature-policy");
>+  assert_equals(report.type, "feature-policy-violation");
>   assert_equals(report.url, document.location.href);
>-  assert_equals(report.body.feature, "vr");
>+  assert_equals(report.body.featureId, "vr");
>   assert_equals(report.body.sourceFile, document.location.href);
>-  assert_equals(typeof report.body.message, "string");
>-  assert_equals(typeof report.body.lineNumber, "number");
>-  assert_equals(typeof report.body.columnNumber, "number");
>+  assert_equals(report.body.lineNumber, 25);
>+  assert_equals(report.body.columnNumber, 55);
>+  assert_equals(report.body.disposition, "enforce");
and here


So, don't make the lineNumber and columnNumber changes unless browsers report those values the same way.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9015796 [details] [diff] [review]
part 4 - IPC

(needs changes per IRC)

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9015886 [details] [diff] [review]
part 6 - FeaturePolicy violation

(needs changes per IRC)

Comment 31

[Andrea Marchesini [:baku]]

Attachment: part 4 - reporting

Comment 32

[Andrea Marchesini [:baku]]

Attachment: part 6 - FeaturePolicy violation

Comment 33

[Andrea Marchesini [:baku]]

Attachment: part 8 - memory-pressure

Comment 34

[Andrea Marchesini [:baku]]

Attachment: part 1 - WebIDL

Comment 35

[Andrea Marchesini [:baku]]

Attachment: part 2 - WebIDL for DeprecationReportBody

Comment 36

[Andrea Marchesini [:baku]]

Attachment: part 3 - DOM Bindings + Deprecation reports

Comment 37

[Andrea Marchesini [:baku]]

Attachment: part 4 - reporting

Comment 38

[Andrea Marchesini [:baku]]

Attachment: part 6 - FeaturePolicy violation

Comment 39

[Andrea Marchesini [:baku]]

Attachment: part 4 - reporting

Comment 40

[Andrea Marchesini [:baku]]

Attachment: part 5 - tests

Comment 41

[Andrea Marchesini [:baku]]

Attachment: part 6 - FeaturePolicy violation

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9021767 [details] [diff] [review]
part 8 - memory-pressure

So ReportingObserver is kept alive as long as the window, even if disconnect() is called, since observer service has a strong reference to the object. window's unlink must get called to ensure no runtime leaks. But, in general unlink may not be called ever.
And calling Shutdown() in dtor is rather useless, since sure the object isn't registered to observer service at that point anymore.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9021790 [details] [diff] [review]
part 1 - WebIDL

Need to then in other patches ensure that ReportingObserver which is observing changes is kept alive even if GC runs.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9021884 [details] [diff] [review]
part 6 - FeaturePolicy violation


>+  nsAutoCString fileName;
>+  Nullable<int32_t> lineNumber;
>+  Nullable<int32_t> columnNumber;
>+  uint32_t line = 0;
>+  uint32_t column = 0;
>+  if (nsJSUtils::GetCallingLocation(cx, fileName, &line, &column)) {
>+    lineNumber.SetValue(static_cast<int32_t>(line));
>+    columnNumber.SetValue(static_cast<int32_t>(column));
>+  }
lease test how this all works when JS code in window A calls some API in window B and that operation isn't
allowed because of feature policy.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9021792 [details] [diff] [review]
part 3 - DOM Bindings + Deprecation reports


>@@ -4065,16 +4180,18 @@ private:
>     WorkerPrivate* wp = aWorkerPrivate;
>     while (wp->GetParent()) {
>       wp = wp->GetParent();
>     }
> 
>     nsPIDOMWindowInner* window = wp->GetWindow();
>     if (window && window->GetExtantDoc()) {
>       window->GetExtantDoc()->WarnOnceAbout(mOperation);
>+      MaybeReportDeprecation(aWorkerPrivate, mOperation, mFileName,
>+                             mLineNumber, mColumnNumber);
>     }
Why do we post this to the main thread?
The spec says
"Note: reporting observers can only observe reports from the same environment settings object"

>@@ -4096,28 +4213,55 @@ DeprecationWarning(JSContext* aCx, JSObj
> void
> DeprecationWarning(const GlobalObject& aGlobal,
>                    nsIDocument::DeprecatedOperations aOperation)
> {
>   if (NS_IsMainThread()) {
>     nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aGlobal.GetAsSupports());
>     if (window && window->GetExtantDoc()) {
>       window->GetExtantDoc()->WarnOnceAbout(aOperation);
>+
>+      nsAutoCString fileName;
>+      Nullable<uint32_t> lineNumber;
>+      Nullable<uint32_t> columnNumber;
>+      uint32_t line = 0;
>+      uint32_t column = 0;
>+      if (nsJSUtils::GetCallingLocation(aGlobal.Context(), fileName,
>+                                        &line, &column)) {
>+        lineNumber.SetValue(line);
>+        columnNumber.SetValue(column);
>+      }
>+
>+      MaybeReportDeprecation(window, aOperation,
>+                             NS_ConvertUTF8toUTF16(fileName), lineNumber,
>+                             columnNumber);
>     }
> 
>     return;
>   }
> 
>   WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aGlobal.Context());
>   if (!workerPrivate) {
>     return;
>   }
> 
>+  nsAutoCString fileName;
>+  Nullable<uint32_t> lineNumber;
>+  Nullable<uint32_t> columnNumber;
>+  uint32_t line = 0;
>+  uint32_t column = 0;
>+  if (nsJSUtils::GetCallingLocation(aGlobal.Context(), fileName,
>+                                    &line, &column)) {
>+    lineNumber.SetValue(line);
>+    columnNumber.SetValue(column);
>+  }
>+
this code duplication is rather annoying, but I don't see easy way to avoid it. Though, it is unclear to me why we do worker reporting the way the patch does it.



r- because I don't understand why worker reports are sent to the main thread.
Should ReportingObserver  be exposed to workers too perhaps?

Comment 46

[Andrea Marchesini [:baku]]

Attachment: part 8 - memory-pressure

Comment 47

[Andrea Marchesini [:baku]]

Attachment: part 3 - DOM Bindings + Deprecation reports

I filed https://github.com/w3c/reporting/issues/131 to expose ReportingObserver to workers. In the meantime, let's ignore deprecate reports for workers.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d5d9f9ecc8
Reporting API - part 1 - WebIDL, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a599388cbfe7
Reporting API - part 2 - WebIDL for DeprecationReportBody, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a86e9cde89bb
Reporting API - part 3 - Deprecate reports, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c200472121fd
Reporting API - part 4 - Reporting, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e311bf1af40
Reporting API - part 5 - tests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3b5f933b97
Reporting API - part 6 - FeaturePolicy, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6119216d9038
Reporting API - part 7 - WPTs, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2751ee1bdac6
Reporting API - part 8 - memory-pressure, r=smaug

Comment 49

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14065 for changes under testing/web-platform/tests

Comment 50

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/14065
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/455264711?utm_source=github_status&utm_medium=notification)

Comment 51

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e8d5d9f9ecc8
https://hg.mozilla.org/mozilla-central/rev/a599388cbfe7
https://hg.mozilla.org/mozilla-central/rev/a86e9cde89bb
https://hg.mozilla.org/mozilla-central/rev/c200472121fd
https://hg.mozilla.org/mozilla-central/rev/3e311bf1af40
https://hg.mozilla.org/mozilla-central/rev/ed3b5f933b97
https://hg.mozilla.org/mozilla-central/rev/6119216d9038
https://hg.mozilla.org/mozilla-central/rev/2751ee1bdac6

Comment 52

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 53

[Chris Mills [:cmills]]

Note to MDN writer's team:

This API is not enabled in release yet, so I'm not adding a note to the Fx65 rel notes. The API should be documented, however, and an entry should be added to our experimental features page.