Implement Report-to header support

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-needed})

Trunk
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [domsecurity-backlog1] [domsecurity-active])

Attachments

(13 attachments, 13 obsolete attachments)

14.16 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.70 KB, patch
smaug
: review+
Details | Diff | Splinter Review
28.12 KB, patch
smaug
: review+
Details | Diff | Splinter Review
28.03 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.34 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.24 KB, patch
smaug
: review+
Details | Diff | Splinter Review
21.71 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.87 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.04 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.15 KB, patch
smaug
: review+
Details | Diff | Splinter Review
18.47 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.36 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Assignee

Description

7 months ago
This is part of the Reporting API.
Assignee

Comment 1

7 months ago
Posted patch part 1 - parser (obsolete) — Splinter Review
The spec requires to check the type of a set of attributes. This blocks me from using the right type in the webIDL dictionaries. I have to use 'any' and do the check 'manually'.
Attachment #9026132 - Flags: review?(bugs)
Assignee

Comment 2

7 months ago
Attachment #9026133 - Flags: review?(bugs)
Assignee

Comment 3

7 months ago
Posted patch part 3 - JSON serialization (obsolete) — Splinter Review
ReportBody objects must be serialized in JSON because they need to be sent as JSON in the fetch requests.
Attachment #9026135 - Flags: review?(bugs)
Assignee

Comment 4

7 months ago
Note that the spec requires reportBody to be fully serializable to JSON, but we have a 'date' which cannot be sent as it is. I'll file a spec bug.
Assignee

Comment 5

7 months ago
Posted patch part 4 - delivering (obsolete) — Splinter Review
I use fetch API to deliver reports. Currently I don't support subdomains.
This patch must be extended to block endpoint URLs, known as trackers from the TP list.
Attachment #9026137 - Flags: review?(bugs)
Assignee

Comment 6

7 months ago
Posted patch part 5 - mochitests (obsolete) — Splinter Review
Attachment #9026138 - Flags: review?(bugs)
Assignee

Updated

7 months ago
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog1] [domsecurity-active]
Comment on attachment 9026138 [details] [diff] [review]
part 5 - mochitests

I think we definitely need some more tests. Especially to ensure nothing too bad happens when invalid values
are coming from the network.

Basically all the properties in the json format should be tested with valid and invalid types in case the property
is defined to have some specific type. But need to also test what happens if bogus json is sent from the server.

Should be quite easy to write such test with that .sjs setup you have here.
Attachment #9026138 - Flags: review?(bugs) → review-
(I know gtest has some tests, but we need to test what we ship, not only partial piece of the software.)

Updated

7 months ago
Attachment #9026133 - Flags: review?(bugs) → review+
Comment on attachment 9026137 [details] [diff] [review]
part 4 - delivering

So this uses the main thread in the parent process for reporting. Why? That is the worst place to do any work.
(We're had recently quite bad issues where occasionally running main-thread-in-parent code causes severe jank)

Could you try some worst case scenario to see whether this causes jank. Like calling tons of deprecated methods in a row or so? 

If you can prove this can't cause jank, ask review again.
Attachment #9026137 - Flags: review?(bugs) → review-
Comment on attachment 9026135 [details] [diff] [review]
part 3 - JSON serialization

>+DeprecationReportBody::ToJSONInternal(JSONWriter& aWriter) const
>+{
>+  aWriter.StringProperty("id", NS_ConvertUTF16toUTF8(mId).get());
>+  // TODO: anticipatedRemoval?
Want to add this too? Not sure why this was left undone. Can be a followup patch.


> ReportingUtils::Report(nsPIDOMWindowInner* aWindow,
>                        nsAtom* aType,
>+                       const nsAString& aGroupName,
>                        const nsAString& aURL,
>                        ReportBody* aBody)
> {
>   MOZ_ASSERT(NS_IsMainThread());
>   MOZ_ASSERT(aWindow);
>   MOZ_ASSERT(aBody);
> 
>+  nsDependentAtomString type(aType);
>+
>   RefPtr<mozilla::dom::Report> report =
>-    new mozilla::dom::Report(aWindow, nsDependentAtomString(aType), aURL,
>-                             aBody);
>+    new mozilla::dom::Report(aWindow, type, aURL, aBody);
>   aWindow->BroadcastReport(report);
>+
>+  // Sending the message to the PBackground service.
>+  mozilla::ipc::PBackgroundChild* actorChild =
>+    mozilla::ipc::BackgroundChild::GetOrCreateForCurrentThread();
>+  if (NS_WARN_IF(!actorChild)) {
>+    return;
>+  }
>+
>+  nsAutoString reportBodyJSON;
>+  aBody->ToJSON(reportBodyJSON);
>+
>+  nsCOMPtr<nsIPrincipal> principal =
>+    nsGlobalWindowInner::Cast(aWindow)->GetPrincipal();
>+  if (NS_WARN_IF(!principal)) {
>+    return;
>+  }
>+
>+  mozilla::ipc::PrincipalInfo principalInfo;
>+  nsresult rv = PrincipalToPrincipalInfo(principal, &principalInfo);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return;
>+  }
>+
>+  Unused << actorChild->SendReport(nsString(type), nsString(aGroupName),
>+                                   principalInfo, nsString(aURL),
>+                                   reportBodyJSON);
This stuff may need to change depending on where we want to do the reporting, so r-, at least for now.
Attachment #9026135 - Flags: review?(bugs) → review-
Comment on attachment 9026132 [details] [diff] [review]
part 1 - parser

>+/* static */ UniquePtr<ReportingHeader::Client>
>+ReportingHeader::ParseHeader(nsIHttpChannel* aChannel,
>+                             nsIURI* aURI,
>+                             const nsACString& aHeaderValue)
>+{
>+  MOZ_ASSERT(aURI);
>+  // aChannel can be null in gtest
>+
>+  AutoJSAPI jsapi;
>+
>+  JSObject* cleanGlobal = SimpleGlobalObject::Create(SimpleGlobalObject::GlobalType::BindingDetail);
>+  if (NS_WARN_IF(!cleanGlobal)) {
>+    return nullptr;
>+  }
>+
>+  if (NS_WARN_IF(!jsapi.Init(cleanGlobal))) {
>+    return nullptr;
>+  }
>+
>+  // WebIDL dictionary parses single items. Let's create a object to parse the
>+  // header.
>+  nsAutoString json;
>+  json.AppendASCII("{ \"items\": [");
>+  json.Append(NS_ConvertUTF8toUTF16(aHeaderValue));
>+  json.AppendASCII("]}");
>+
>+  JSContext* cx = jsapi.cx();
>+  JS::Rooted<JS::Value> jsonValue(cx);
>+  bool ok = JS_ParseJSON(cx, PromiseFlatString(json).get(), json.Length(), &jsonValue);
>+  if (!ok) {
>+    LogToConsoleInvalidJSON(aChannel, aURI);
>+    return nullptr;
>+  }
>+
>+  dom::ReportingHeaderValue data;
>+  if (!data.Init(cx, jsonValue)) {
Can't you just use the Init() which takes a string as a param?

(not sure if this link stays valid for long, but as an example of a dictionary https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/EventBinding.cpp#127)



>+    LogToConsoleInvalidJSON(aChannel, aURI);
>+    return nullptr;
>+  }
>+
>+  if (!data.mItems.WasPassed() || data.mItems.Value().IsEmpty()) {
>+    return nullptr;
>+  }
>+
>+  UniquePtr<Client> client;
>+  client.reset(new Client());
MakeUnique?


Minor things only, but I guess I could take a look at this too once the delivery is clear.
Attachment #9026132 - Flags: review?(bugs) → review-
Assignee

Comment 12

7 months ago
> >+  dom::ReportingHeaderValue data;
> >+  if (!data.Init(cx, jsonValue)) {
> Can't you just use the Init() which takes a string as a param?

No, I cannot. The Init() with a JSON string is available only if there are no attributes with type 'any'.
I need 'any' because I need to evaluate the type of attributes and report warning, errors, and ignore endpoints if the types do not match.
Assignee

Comment 13

7 months ago
Posted patch part 1 - parser (obsolete) — Splinter Review
Patch updated with MakeUnique. All the rest is untouched.
Attachment #9026132 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Assignee

Comment 14

7 months ago
> >+  // TODO: anticipatedRemoval?

Date objects cannot be serialized to JSON. See https://github.com/w3c/reporting/issues/132
I keep this out of the patch. It will be a follow up.
Assignee

Comment 15

7 months ago
Posted patch part 3 - JSON serialization (obsolete) — Splinter Review
This is still needed.
Attachment #9026135 - Attachment is obsolete: true
Assignee

Comment 16

7 months ago
Attachment #9026137 - Attachment is obsolete: true
Assignee

Comment 17

7 months ago
Posted patch part 5 - delivering (obsolete) — Splinter Review
Attachment #9026138 - Attachment is obsolete: true
Assignee

Comment 18

7 months ago
Posted patch part 7 - remove endpoint (obsolete) — Splinter Review
Assignee

Comment 19

7 months ago
Posted patch part 1 - parser (obsolete) — Splinter Review
Attachment #9027301 - Attachment is obsolete: true
Attachment #9027495 - Flags: review?(bugs)
Assignee

Comment 20

7 months ago
Attachment #9027395 - Attachment is obsolete: true
Attachment #9027497 - Flags: review?(bugs)
Assignee

Comment 21

7 months ago
Attachment #9027396 - Attachment is obsolete: true
Attachment #9027498 - Flags: review?(bugs)
Assignee

Comment 22

7 months ago
Posted patch part 5 - delivering (obsolete) — Splinter Review
Attachment #9027397 - Attachment is obsolete: true
Attachment #9027499 - Flags: review?(bugs)
Assignee

Comment 23

7 months ago
Attachment #9027398 - Attachment is obsolete: true
Attachment #9027500 - Flags: review?(bugs)
Assignee

Comment 24

7 months ago
Attachment #9027501 - Flags: review?(bugs)
Assignee

Comment 25

7 months ago
Attachment #9027502 - Flags: review?(bugs)
Assignee

Comment 26

7 months ago
Attachment #9027503 - Flags: review?(bugs)
Comment on attachment 9027495 [details] [diff] [review]
part 1 - parser

>+
>+    bool found = false;
>+    for (const Group& group : client->mGroups) {
>+      if (group.mName == item.mGroup)

The spec says mGroup must be type string.
Using .webidl dictionary for parsing may do conversion from other types too.

>+// Used internally to process the JSON
>+dictionary ReportingItem {
>+  // This is a long.
>+  any max_age;
>+  // This is a sequence of ReportingEndpoint.
>+  any endpoints;
>+  DOMString group = "default";
So unfortunately this needs to be any too, so that we can check the actual type.

>+  boolean include_subdomains = false;

The spec is unclear whether this must be a boolean, or whether conversion can happen. Want to file a spec bug?


>+};
>+
>+// Used internally to process the JSON
>+dictionary ReportingEndpoint {
>+  DOMString url;
This must be a string too, per spec



Please add tests when json doesn't use strings, but something else, say numbers or arrays or something weird which webidl layer may convert to string.
Flags: needinfo?(bugs)
Attachment #9027495 - Flags: review?(bugs) → review-

Updated

7 months ago
Attachment #9027497 - Flags: review?(bugs) → review+

Updated

7 months ago
Attachment #9027501 - Flags: review?(bugs) → review+
Comment on attachment 9027502 [details] [diff] [review]
part 8 - ReportingHeaderInspector for testing

Sorry but better to add this kind of method to DOMWindowUtils. Should cause way less code bloating (binary size).
And DOMWindowUtils was after all created initially for methods which are needed for testing.
Attachment #9027502 - Flags: review?(bugs) → review-
Comment on attachment 9027503 [details] [diff] [review]
part 9 - test cleanup

r+, but I'm not convinced we want ReportingHeaderInspector webidl just because of testing. Why cause more bloat than needed.
So I'd prefer just using existing interfaces for testing, and add a new method there. ChromeUtils or DOMWindowUtils
Attachment #9027503 - Flags: review?(bugs) → review+

Updated

7 months ago
Attachment #9027500 - Flags: review?(bugs) → review+
Assignee

Comment 30

7 months ago
Attachment #9027495 - Attachment is obsolete: true
Assignee

Comment 31

7 months ago
Attachment #9027502 - Attachment is obsolete: true
Comment on attachment 9027499 [details] [diff] [review]
part 5 - delivering

>+  Sequence<nsCString>* headers =
>+    init.mHeaders.Value().SetAsByteStringSequenceSequence().AppendElement(fallible);
Perhaps call headers something else, given that headers mean just content type header.
So, maybe contentTypeHeader ?


>+  init.mReferrer.Construct();
>+  init.mReferrer.Value().Assign(aReportData.mURL);
Where is this defined?

>+
>+  init.mReferrerPolicy.Construct();
>+  init.mReferrerPolicy.Value() = ReferrerPolicy::Origin;
>+
Where is this defined?

Internally referrer may be set, but I think we should try to follow the spec as much as possible to keep this somehow readable.
If our fetch is working as defined in the spec, referrer should be set without all this stuff, I think.


>+  init.mCache.Construct();
>+  init.mCache.Value() = RequestCache::No_cache;
Where is this defined?

>+
>+  init.mRedirect.Construct();
>+  init.mRedirect.Value() = RequestRedirect::Error;
where is this defined?
Attachment #9027499 - Flags: review?(bugs) → review-
Comment on attachment 9027498 [details] [diff] [review]
part 4 - IPC endpoint for report

>+ReportingHeader::GetEndpointForReportInternal(const ReportingHeader::Group& aGroup,
>+                                              nsACString& aEndpointURI)
>+{
>+  TimeDuration diff = TimeStamp::Now() - aGroup.mCreationTime;
>+  if (diff.ToSeconds() > aGroup.mTTL) {
>+    // Expired.
>+    return;
>+  }
So I assume some patch will actually remove expired groups?


>+  for (const Endpoint& endpoint : aGroup.mEndpoints) {
>+    if (minPriority == -1 || minPriority > endpoint.mPriority) {
>+      minPriority = endpoint.mPriority;
>+      weight = endpoint.mWeight;
>+    } else if (minPriority == endpoint.mPriority) {
>+      weight += endpoint.mWeight;

Hmm, please call weight totalWeight to follow the hard-to-read spec more closely.
Attachment #9027498 - Flags: review?(bugs) → review+
Assignee

Updated

7 months ago
Attachment #9027850 - Flags: review?(bugs)
Assignee

Updated

7 months ago
Attachment #9027849 - Flags: review?(bugs)

Updated

7 months ago
Attachment #9027850 - Flags: review?(bugs) → review+
Comment on attachment 9027849 [details] [diff] [review]
part 1 - parser

>+  obs->AddObserver(service, NS_HTTP_ON_EXAMINE_RESPONSE_TOPIC, false);
Tiny bit annoying to observe always, even if we don't use the data for anything if pref is unset.
But given we have the ClearSiteData, I guess this is fine.

>+
>+  // Let's remove the previous entries.
>+  mOrigins.Remove(origin);
>+
>+  UniquePtr<Client> client = ParseHeader(aChannel, uri, headerValue);
Old client shouldn't be remove if parsing the header fails. So, isn't just calling Put enough. It should replace the object when we have client.


>+// Used internally to process the JSON
>+dictionary ReportingEndpoint {
>+  // This is a string.
>+  any url;
>+  // This is unsigned long.
>+  any priority;
>+  // This is unsigned long.
>+  any weight;
>+};
I guess it should be "This is an unsigned long." in both cases
Attachment #9027849 - Flags: review?(bugs) → review+
Assignee

Updated

7 months ago
Attachment #9027977 - Flags: review?(bugs)
Assignee

Updated

7 months ago
Attachment #9027978 - Flags: review?(bugs)
Assignee

Updated

7 months ago
Attachment #9027979 - Flags: review?(bugs)
Comment on attachment 9027979 [details] [diff] [review]
part C - cleanup timeout

>+NS_IMETHODIMP
>+ReportingHeader::Notify(nsITimer* aTimer)
>+{
>+  mCleanupTimer = nullptr;
>+
>+  RemoveOriginsForTTL();
>+  MaybeCreateCleanupTimer();
Tiny bit hackish, but I guess 1h timer isn't too bad


>+  uint32_t timeout = StaticPrefs::dom_reporting_cleanup_timeout() * 1000;
>+  nsresult rv = NS_NewTimerWithCallback(getter_AddRefs(mCleanupTimer),
>+                                        this, timeout,
>+                                        nsITimer::TYPE_ONE_SHOT,
this should be low priority timer. *_LOW_PRIORITY or some such
Attachment #9027979 - Flags: review?(bugs) → review+

Updated

7 months ago
Attachment #9027977 - Flags: review?(bugs) → review+
Comment on attachment 9027978 [details] [diff] [review]
part B - limit number of reports

You said Chrome also coalesces some reports. Does it keep the most recent one or the oldest one?

I am still worried that some loop calling deprecated method may become slow if we call
mReportQueue.AppendElement(aReportData, fallible) all the time.
Attachment #9027978 - Flags: review?(bugs) → review+
Assignee

Comment 40

7 months ago
Posted patch part 5 - delivering (obsolete) — Splinter Review
There is 1 TODO about nsContentType TYPE_REPORT. This is not implemented yet, but it can be done as follow up.
Attachment #9027499 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9028234 - Flags: review?(bugs)
Comment on attachment 9028234 [details] [diff] [review]
part 5 - delivering

>+  // The body
>+  nsAutoCString body;
>+  ReportJSONWriter w(body);
>+
>+  w.Start();
>+
>+  w.IntProperty("age",
>+                (TimeStamp::Now() - aReportData.mCreationTime).ToMilliseconds());
>+  w.StringProperty("type", NS_ConvertUTF16toUTF8(aReportData.mType).get());
>+  w.StringProperty("user_agent",
>+                   NS_ConvertUTF16toUTF8(aReportData.mUserAgent).get());
>+  w.JSONProperty("body", aReportData.mReportBodyJSON.get());
>+  w.End();
So where is "url"? Per spec there should be "age", "type", "url", "user_agent" and "body"

But hey, look https://w3c.github.io/reporting/#report-url
So report-to header is supposed to work with workers. How does this code deal with that.
(Sorry, this is quite never ending... so complicated spec)


>+ReportDeliver::AppendReportData(const ReportData& aReportData)
>+{
>+  if (aReportData.mFailures >
>+        StaticPrefs::dom_reporting_delivering_maxFailures()) {
>+    return;
>+  }
>+
>+  if (NS_WARN_IF(!mReportQueue.AppendElement(aReportData, fallible))) {
>+    return;
>+  }
>+
>+  if (!mTimer) {
>+    uint32_t timeout = StaticPrefs::dom_reporting_delivering_timeout() * 1000;
>+    nsresult rv = NS_NewTimerWithCallback(getter_AddRefs(mTimer),
>+                                          this, timeout,
>+                                          nsITimer::TYPE_ONE_SHOT,
This should be low priority
Attachment #9028234 - Flags: review?(bugs) → review-
Assignee

Comment 42

7 months ago
"url" attribute added. Report-to header in workers is already supported. I'm going to add a test.
Attachment #9028234 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9028561 - Flags: review?(bugs)
Assignee

Updated

7 months ago
Attachment #9028562 - Flags: review?(bugs)
Comment on attachment 9028562 [details] [diff] [review]
part D - test header in workers


>+function runTests(extraParams = "") {
>+  // Call a deprecating operation.
>+  for (let i = 0; i < 100; ++i) {
>+    new TestingDeprecatedInterface();
>+  }
>+  ok(true, "Created a deprecated interface");
>+
>+  // Check if the report has been received.
>+  return checkReport()
>+  .then(reports => {
>+    is(reports.length, 3, "We have 1 report");
3 reports, no?
Attachment #9028562 - Flags: review?(bugs) → review+

Updated

7 months ago
Attachment #9028561 - Flags: review?(bugs) → review+

Comment 45

7 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c435d7c0173e
Implement Report-to header support - part 1 - Header parser, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e709a8076f
Implement Report-to header support - part 2 - Header parser gtest, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbcbf470d213
Implement Report-to header support - part 3 - JSON serialization of ReportBody, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2c155ce351
Implement Report-to header support - part 4 - IPC to get endpoint from content process, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a00225dfc414
Implement Report-to header support - part 5 - Report delivering, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2492c852777
Implement Report-to header support - part 6 - Remove endpoints, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/696af9c24623
Implement Report-to header support - part 7 - site-data cleanups endpoints, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/33bb5a1368e8
Implement Report-to header support - part 8 - ChromeUtils methods for testing, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e1002895a6
Implement Report-to header support - part 9 - cleanup tests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d09c38b173c
Implement Report-to header support - part 10 - delivering tests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec7421212c22
Implement Report-to header support - part 11 - Limit the number of reports, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/77156cbacc33
Implement Report-to header support - part 12 - Cleanup out-of-date endpoints, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/870d4a920aed
Implement Report-to header support - part 13 - Test for header in workers, r=smaug
Note to writer's team

This is part of CSP and the Reporting API. It is not available by default yet in 65 (so no rel note), but should be documented.
You need to log in before you can comment on or make changes to this bug.