Closed Bug 1508310 Opened 2 years ago Closed 2 years ago

Implement Report-to header support

Categories

(Core :: DOM: Security, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

(Keywords: dev-doc-needed, Whiteboard: [domsecurity-backlog1] [domsecurity-active])

Attachments

(13 files, 13 obsolete files)

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
This is part of the Reporting API.
Attached 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)
Attached patch part 2 - gtestSplinter Review
Attachment #9026133 - Flags: review?(bugs)
Attached 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)
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.
Attached 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)
Attached patch part 5 - mochitests (obsolete) — Splinter Review
Attachment #9026138 - Flags: review?(bugs)
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.)
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-
> >+  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.
Attached 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)
> >+  // 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.
Attached patch part 3 - JSON serialization (obsolete) — Splinter Review
This is still needed.
Attachment #9026135 - Attachment is obsolete: true
Attached patch part 4 - IPC endpoint for report (obsolete) — Splinter Review
Attachment #9026137 - Attachment is obsolete: true
Attached patch part 5 - delivering (obsolete) — Splinter Review
Attachment #9026138 - Attachment is obsolete: true
Attached patch part 7 - remove endpoint (obsolete) — Splinter Review
Attached patch part 1 - parser (obsolete) — Splinter Review
Attachment #9027301 - Attachment is obsolete: true
Attachment #9027495 - Flags: review?(bugs)
Attachment #9027395 - Attachment is obsolete: true
Attachment #9027497 - Flags: review?(bugs)
Attachment #9027396 - Attachment is obsolete: true
Attachment #9027498 - Flags: review?(bugs)
Attached patch part 5 - delivering (obsolete) — Splinter Review
Attachment #9027397 - Attachment is obsolete: true
Attachment #9027499 - Flags: review?(bugs)
Attachment #9027398 - Attachment is obsolete: true
Attachment #9027500 - Flags: review?(bugs)
Attachment #9027501 - Flags: review?(bugs)
Attachment #9027502 - Flags: review?(bugs)
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-
Attachment #9027497 - Flags: review?(bugs) → review+
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+
Attachment #9027500 - Flags: review?(bugs) → review+
Attached patch part 1 - parserSplinter Review
Attachment #9027495 - Attachment is obsolete: true
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+
Attachment #9027850 - Flags: review?(bugs)
Attachment #9027849 - Flags: review?(bugs)
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+
Attachment #9027977 - Flags: review?(bugs)
Attachment #9027978 - Flags: review?(bugs)
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+
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+
Attached 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
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-
"url" attribute added. Report-to header in workers is already supported. I'm going to add a test.
Attachment #9028234 - Attachment is obsolete: true
Attachment #9028561 - Flags: review?(bugs)
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+
Attachment #9028561 - Flags: review?(bugs) → review+
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.