Skip to content

Observe reports from JavaScript #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
RByers opened this issue Feb 1, 2017 · 29 comments
Closed

Observe reports from JavaScript #29

RByers opened this issue Feb 1, 2017 · 29 comments
Assignees

Comments

@RByers
Copy link
Collaborator

RByers commented Feb 1, 2017

Some people have asked for a way to see the reports from JS (eg. to make it easier to deploy RUM tooling: https://github.com/WICG/reporting/issues/27#issuecomment-274673657), though perhaps #28 is enough?

The main argument I've heard against this is that, by being post-hoc-only, the Reporting API can more safely include data that's not rigidly defined by the standard (such as error message strings) with less risk of web pages taking a dependency on the exact details (eg. by parsing strings).

@RByers
Copy link
Collaborator Author

RByers commented Feb 3, 2017

CSP has a DOM event for this. I was thinking we might want something generic and unstructured, but having a fully typed event does seem nicer from a programming model perspective. Perhaps every report type should just define it's own event the way CSP does (independent from the reporting API)?

@juliatuttle
Copy link
Contributor

I think it would be fine for Reporting to define a single "ReportingObserver" interface, as was suggested elsewhere. I'd rather it simply be a one-time hook to see reports when they're queued, as opposed to giving access to queued (or even delivered) reports, for simplicity, but I'm open to discussing that.

@juliatuttle
Copy link
Contributor

(To be clear, this would be in addition to normal HTTPS report delivery.)

@patrickkettner
Copy link
Collaborator

@juliatuttle to make sure I understand, are you proposing a "report created" type-hook that can be intercepted when any individual report is generated?

How would this work with reports that are being sent after the document has crashed/been closed? Will the developer create an event handler that will be run out of band as well? If not, does that mean some reports will be able to modified and others will not?

@igrigorik
Copy link
Member

@patrickkettner some reports can't be seen by JS.. no matter how hard you try. If the browser crashes, we can queue the report in the background, but there is no guarantee that the user will try opening the same page again to trigger the JS, nor do we want to retain the report indefinitely.

As @RByers pointed out in another thread.. It may make sense to expose a consistent interface for all runtime errors/warnings, which can be observed via JS and reported via existing analytics. Optionally, we can also allow developers to register a reporting endpoint. However, there are some type of reports (e.g. nav failures, crashes), that will never be seen by JS and will only be available via reporting API.

@mikewest
Copy link
Member

mikewest commented Feb 8, 2017

FWIW: I'd be perfectly happy to move CSP's reporting bits over to a generic interface and eventually deprecate/remove the specialized violation event. Developers who care about CSP violations should/will care about other kind of reported violations as well (and can filter on type if they don't).

I'm less thrilled with the idea of giving the page the ability to inspect the list of queued reports, or to fire events on a page to account for reports that were sent at some point inbetween the last navigation and now.

This is a long way of just agreeing with @igrigorik. :)

@juliatuttle
Copy link
Contributor

Yeah, I'm proposing a "report created" hook, since it's the simplest way to make this work. If we make pages able to view the whole queue, then it would only make sense for them to be able to mark reports delivered as well, and that would end up being really complex. (I'm happy to reconsider if the complexity sounds like it's worth it, but I don't think it is right now.)

@RByers
Copy link
Collaborator Author

RByers commented Mar 28, 2017

A generic ReportingObserver SGTM. I can start by putting a proposal into the explainer.

The main outstanding design question here for me is what the data type should be for the report. Do we just return the same JSON string as the report (leaving it up to devs to deserialize it themselves)? Or do we try to define a per-report-type interface (in which case maybe we turn the report definition around to say it's just a JSON serialization of that interface to avoid the redundancy)? Or do we cheat and say any with some spec text saying that the browser automatically deserializes the report into a JavaScript object (similar to some of the web payments APIs with all the debate that's caused). Thoughts?

@mikewest
Copy link
Member

I would prefer to give developers a deserialized something, more or less mapping to what CSP already provides via 3.3 of https://w3c.github.io/webappsec-csp/#report-violation.

One way of doing that would be to require that anything passed into the reporting API is a "serializable object", and to define serialization/deserialization steps for both. That seems pretty straightforward; I'd be happy to do so in CSP.

@juliatuttle
Copy link
Contributor

I'd prefer we use the same structure for the JSON sent to the server and observed in JS.

RByers added a commit that referenced this issue May 29, 2017
An initial design sketch corresponding to the discussion in #29
@RByers
Copy link
Collaborator Author

RByers commented May 29, 2017

I've finally added a rough sketch of ReportingObserver to the explainer. Any thoughts?

One design question I struggled with a little is the interaction of multiple contexts. Eg. in Chrome, if a content script uses a deprecated API, should that be exposed to ReportingObservers in the main world or vice versa? I believe the API will be more useful if they're not - and so (unlike, for example PerformanceObserver) the API observes only it's local context.

One possible special case here are Worklets. Since worklets don't necessarily have a long-lived context of their own, using a ReportingObserver from within a worklet doesn't really make sense. Instead any reports generated from a worklet should be observable only from the main world.

I've started hacking on an implementation for this. Here's my (currently untested) IDL.
callback ReportingObserverCallback = void (sequence<Report> reports, ReportingObserver observer);

[
    CustomConstructor(ReportingObserverCallback callback),
    RuntimeEnabled=ReportingObserver,
] interface ReportingObserver {
    void observe();
    void disconnect();
};

// TODO(rbyers): Since these are just dictionaries, there's no need for constructors, right?
[
    NoInterfaceObject,
] interface Report {
    readonly attribute string type;
    readonly attribute ReportBody report;   
};

[
    NoInterfaceObject,
] interface ReportBody {
};

[
    NoInterfaceObject,
] interface DeprecationReport : ReportBody {
  // TODO(rbyers): add structured data - id, anticipatedRemoval, sourceFile, lineNumber
  readonly attribute string message;
};

Any feedback?

@mikewest
Copy link
Member

I've finally added a rough sketch of ReportingObserver to the explainer. Any thoughts?

I'm curious about the observer model as opposed to something like an event. For CSP, we trigger securitypolicyviolation events on the element that caused the violation, which makes it possible to track down what's going wrong (or reflect additional metadata to the server). That seems like it would be valuable for DeprecationReport as well, and it's something I'd like not to lose when migrating over to this model.

Is there an advantage to the observer model over an event that bubbles up to window.addEventListener('report', e => { ... })?

One design question I struggled with a little is the interaction of multiple contexts. Eg. in Chrome, if a content script uses a deprecated API, should that be exposed to ReportingObservers in the main world or vice versa? I believe the API will be more useful if they're not - and so (unlike, for example PerformanceObserver) the API observes only it's local context.

CSP attempts to allow isolated worlds to bypass the page's policy. That turns out to be quite difficult. I understand why you'd want to ensure that there's no leakage between the two, but it's a tough problem to solve. Consider an extension that injects a script/event listener/etc into the main world that uses a deprecated API. That's the extension's fault, but by the time you execute the API you're out of the extension's context.

@RByers
Copy link
Collaborator Author

RByers commented May 30, 2017

I'm curious about the observer model as opposed to something like an event. For CSP, we trigger securitypolicyviolation events on the element that caused the violation, which makes it possible to track down what's going wrong (or reflect additional metadata to the server). That seems like it would be valuable for DeprecationReport as well, and it's something I'd like not to lose when migrating over to this model.

Is there an advantage to the observer model over an event that bubbles up to window.addEventListener('report', e => { ... })?

There's two questions here I think: event vs. observer and synchronous vs. asynchronous (potentially even delayed). Typically observers imply async but that doesn't need to be the case.

@ojanvafai argued for the observer pattern over an event based on lower overhead. I don't think we should really be delivering a lot of these in any common scenario, so I personally don't find that too compelling.

The more potentially compelling performance argument IMHO is that a handler might do something expensive, so by using an async/batched pattern we reduce the perf risk. That comes at the cost of not being able to collect state at the exact time the report occurred. When talking with Ojan I said I was OK with an observer pattern over DOM events, but we might find it necessary (unlike our other Observer APIs) to make it synchronous for this reason.

In practice what sort of information do people collect in their securitypolicyviolation handlers? If we had an async Observer that included a stacktrace would that get most of the value?

@RByers
Copy link
Collaborator Author

RByers commented May 30, 2017

CSP attempts to allow isolated worlds to bypass the page's policy. That turns out to be quite difficult. I understand why you'd want to ensure that there's no leakage between the two, but it's a tough problem to solve. Consider an extension that injects a script/event listener/etc into the main world that uses a deprecated API. That's the extension's fault, but by the time you execute the API you're out of the extension's context.

Oh yeah, I know attributing blame to an extension is often a lost cause. I think the main value in keeping the observation per-world is it enables an extension to get warnings of deprecated API usage from it's own (non-injected) code, and it reduces (but won't eliminate) the problem of page reports being contaminated by extensions.

I think it's also slightly easier to implement in blink - I only have to create a single v8::Object for each report (and pass that out to the content layer), rather than create some other data structure (eg. a JSONObject) which is then turned into a v8::Object for each v8::Context and pass eg. a JSON string out to the content layer. But it's also unusual to have a per-world data structure like this, so that might add more complexity (haven't gotten that far yet).

Is there any reason to prefer a global per-Frame view?

@mikewest
Copy link
Member

The more potentially compelling performance argument IMHO is that a handler might do something expensive, so by using an async/batched pattern we reduce the perf risk. That comes at the cost of not being able to collect state at the exact time the report occurred. When talking with Ojan I said I was OK with an observer pattern over DOM events, but we might find it necessary (unlike our other Observer APIs) to make it synchronous for this reason.

I'm totally behind the notion of an async handler for the reasons you've outlined. CSP's events are actually async already (we "queue a task" to send the event in https://w3c.github.io/webappsec-csp/#report-violation, and post a task in Chrome's implementation). In practice, that seems like it's been good enough.

In practice what sort of information do people collect in their securitypolicyviolation handlers? If we had an async Observer that included a stacktrace would that get most of the value?

@arturjanc, @mikispag, and @lweichselbaum will likely have more thoughts here than I, as they're responsible for a lot of Google's collection. I know they were interested in getting the element on which the violation occurred as part of their reports, as that was useful in tracking down what caused the violation in the first plact, but I don't know whether they just serialize it's location in the page tree/id/classes/etc. or do something more complicated.

Is there any reason to prefer a global per-Frame view?

I made my point poorly, sorry for that: I am in favor of segregating the extensions events into its own isolated world. Pages should have as little information about extensions running within them as possible, so this is indeed the right thing to do from a number of perspectives.

Blindly guessing (because I don't have a good feel for all the deprecations we have floating around at the moment), I think that segregation is going to turn out to be a hard thing to do well, and pages are going to end up with violations initiated from content scripts. So, I'd caution against making promises in the spec that we're not going to be able to keep.

@RByers
Copy link
Collaborator Author

RByers commented May 30, 2017

I'm totally behind the notion of an async handler for the reasons you've outlined. CSP's events are actually async already (we "queue a task" to send the event in https://w3c.github.io/webappsec-csp/#report-violation, and post a task in Chrome's implementation). In practice, that seems like it's been good enough.

Ah, OK. So if we agree on async then I personally think it's primarily a style question of DOM events vs. Observers (unless someone can argue why the DOM dispatch overhead of a few events makes any real difference compared to the 1000s of input DOM events dispatched during a normal application session). Since there is no useful target in these cases (no use for capture, bubbling, etc.) and no use for preventDefault, DOM events do seem like overkill to me.

BTW the other reason I like async (for now at least) is it lowers the risk that applications will change behavior based on reports.

I made my point poorly, sorry for that: I am in favor of segregating the extensions events into its own isolated world. Pages should have as little information about extensions running within them as possible, so this is indeed the right thing to do from a number of perspectives.

Blindly guessing (because I don't have a good feel for all the deprecations we have floating around at the moment), I think that segregation is going to turn out to be a hard thing to do well, and pages are going to end up with violations initiated from content scripts. So, I'd caution against making promises in the spec that we're not going to be able to keep.

Perfect, thanks - we're on exactly the same page here. In the spec perhaps we just need to talk about this in terms of the "relevant Realm" similar to the DOM spec. We're definitely not promising anything about extensions (if we want to mention extensions explicitly it would presumably only be in a Note as an example). Just that JS can observe reports associated with it's Realm.

@mikewest
Copy link
Member

Since there is no useful target in these cases (no use for capture, bubbling, etc.) and no use for preventDefault, DOM events do seem like overkill to me.

In which cases? There's certainly a useful target for many CSP violations. See https://github.com/w3c/web-platform-tests/blob/master/content-security-policy/securitypolicyviolation/targeting.html for example. It seems like there would be a useful target for many deprecations/warnings. What cases don't have useful targets?

We're definitely not promising anything about extensions (if we want to mention extensions explicitly it would presumably only be in a Note as an example). Just that JS can observe reports associated with it's Realm.

SGTM.

@RByers
Copy link
Collaborator Author

RByers commented May 30, 2017

There's certainly a useful target for many CSP violations. See https://github.com/w3c/web-platform-tests/blob/master/content-security-policy/securitypolicyviolation/targeting.html for example. It seems like there would be a useful target for many deprecations/warnings. What cases don't have useful targets?

Oh! I just glanced at the CSP spec and saw:

Given a violation (violation), this algorithm reports it to the endpoint specified in violation’s policy, and fires a SecurityPolicyViolationEvent at violation’s global object.

But looking more carefully I see that it's often not just at the global object (spec bug?). So for example does this mean people may listen for events on a particular <script> element only in order to try to handle warnings only for certain scripts (eg. a library might install a listener on document.currentScript)?

If there are legitimate scenarios where someone would want to listen to reports only associated with some DOM node, then I agree that DOM events make more sense. It would take some additional plumbing, but I think we could probably support that for many deprecation and intervention warnings (some may be hard though).

What's your thinking for securitypolicyviolation events generally? I assume we'd continue to fire them, but would that be just for compatibility and we'd encourage people to look for the generic report events with type==='csp'? Or would they always add a level of semantics on top of the generic report events and so continue to evolve as the preferred mechanism for CSP?

@mikewest
Copy link
Member

But looking more carefully I see that it's often not just at the global object (spec bug?)

Yup. I apparently updated the algorithm's steps, but missed the description. :(

So for example does this mean people may listen for events on a particular <script> element only in order to try to handle warnings only for certain scripts (eg. a library might install a listener on document.currentScript)?

No. Instead, it means that if an element generates a request which CSP blocks (e.g. img-src 'none' + <img src='whatever.js'>) then the securitypolicyviolation will fire on the relevant <img> element (and the element will be accessible as the event's target). Likewise for inline event handlers, inline style, etc.

If a violation is triggered by a non-connected element (e.g. document.createElement('img').src='whatever.js';) or from script (e.g. a banned call to eval()), then the event will fire on the global object.

I think I'm ok with moving away from events if y'all prefer an observer model, but I'm less thrilled with the idea of dropping the violation's target. If we run with an observer model, it would be nice to find some way to retain that information.

What's your thinking for securitypolicyviolation events generally?

I was hoping to deprecate them over time in favor of whatever we built for reporting. For some period of time we'd fire both, with the goal of not firing both at some point in the future. Since Firefox still hasn't implemented the CSP event, and Safari hasn't implemented any of the targeting, that seems doable.

@RByers
Copy link
Collaborator Author

RByers commented May 30, 2017

No. Instead, it means that if an element generates a request which CSP blocks (e.g. img-src 'none' + <img src='whatever.js'>) then the securitypolicyviolation will fire on the relevant <img> element (and the element will be accessible as the event's target). Likewise for inline event handlers, inline style, etc.

So in that case the target is really just useful as some meta-information about the event, right? I.e. there's not some common scenario where you'd want to install your event listener below the window to filter the events you get to only specific targets? It's when you get into library composition / capturing scenarios (eg. where APIs like stopPropagation are important) that the power of DOM events is really compelling IMHO.

I think I'm ok with moving away from events if y'all prefer an observer model, but I'm less thrilled with the idea of dropping the violation's target. If we run with an observer model, it would be nice to find some way to retain that information.

Oh yeah, for sure - we could totally add a relatedNode property on the report or something. It may not be set in many scenarios, but having something 1st class here that's specific to the observer API (not present in the JSON report format) makes more sense to me than trying to have the type-specific reports have additional information in observer contexts than the HTTP context.

I was hoping to deprecate them over time in favor of whatever we built for reporting.

Ok, then it's definitely critical that we capture all the securitypolicyviolation use cases here :-). It looks like everything else defined on SecurityPolicyViolationEvent maps to the report JSON, so target is really the only special case we need to worry about to get parity with the existing API, right?

@mikewest
Copy link
Member

So in that case the target is really just useful as some meta-information about the event, right? I.e. there's not some common scenario where you'd want to install your event listener below the window to filter the events you get to only specific targets?

Yes. I would be surprised if anyone was just hooking one branch of their pages' DOM tree.

Oh yeah, for sure - we could totally add a relatedNode property on the report or something.

I'm equally happy with it being a property on the ReportBody specialization if you don't think other reporting types will care about it. That basically matches what CSP is doing now between the DOM event and the POSTed report.

Still, it seems like it might be useful for deprecations, so... shrug

It looks like everything else defined on SecurityPolicyViolationEvent maps to the report JSON, so target is really the only special case we need to worry about to get parity with the existing API, right?

I believe that's correct. I'm pretty sure the other interesting bits of the event are just strings that would hang off of the ReportBody specialization for CSP.

@RByers
Copy link
Collaborator Author

RByers commented May 31, 2017

I'm equally happy with it being a property on the ReportBody specialization if you don't think other reporting types will care about it. That basically matches what CSP is doing now between the DOM event and the POSTed report. Still, it seems like it might be useful for deprecations, so... shrug

Yeah I can definitely see there being an analogy for multiple different types of reports. Most deprecations are just about some JS API (eg. window.webkitStorageInfo) and so don't necessarily have a related Node, but some certainly do. I don't have a strong preference on whether we have a generic relatedNode on Report or a property on the ReportBody specialization. The latter seems harder to spec elegantly to me (since we can't just say to JSON.stringify the object in order to generate the report body), but I suppose that should be a minor concern. Either seems fine for developers, though the latter might result in a more useful property name for some types of reports.

@RByers RByers changed the title Observe reports from JavaScript? Observe reports from JavaScript Jun 8, 2017
@tdresser
Copy link

There are a few minor differences between ReportingObserver and PerformanceObserver that seem like they might be worth aligning on. They look similar enough that the API differences may be confusing.

  • PerformanceObserver passes .observe an object with a list of entryTypes to observe.
  • PerformanceObserver doesn't pass the observer to the observing callback.

@jkarlin
Copy link

jkarlin commented Jul 17, 2017

TransferSizePolicy would benefit from ReportingObserver. Has there been any recent work on it?

@RByers RByers self-assigned this Jul 18, 2017
@RByers
Copy link
Collaborator Author

RByers commented Jul 18, 2017

Yes @paulmeyer90 is actively working on an implementation. But I have spec work to do. Having a complete implementation landed behind a flag and complete spec written are P1 on feature-control team's Q3 OKRs.

@jkarlin
Copy link

jkarlin commented Jul 19, 2017

Great!

@RByers
Copy link
Collaborator Author

RByers commented Aug 17, 2017

@tdresser, sorry for the delay.

There are a few minor differences between ReportingObserver and PerformanceObserver that seem like they might be worth aligning on. They look similar enough that the API differences may be confusing.

PerformanceObserver passes .observe an object with a list of entryTypes to observe.

Good point. I was looking primarily at IntersectionObserver which doesn't have this. Filed #46 to discuss further.

PerformanceObserver doesn't pass the observer to the observing callback.

IntersectionObserver and MutationObserver both follow this pattern, but IDBObserver also doesn't. I think this would only rarely be useful. For example, perhaps I want to provide a library that manages multiple observers for different scenarios and

it's only rarely useful (eg. for a one-shot observer which invokes unobserve when complete, but can have multiple pending instances). But I believe it's pretty standard practice in observer patterns to do this, so I'd argue we should keep this. WDYT?

@tdresser
Copy link

Either I was wrong about PO not taking the observer, or it's changed recently. It now does pass the observer to the callback.

Passing the observer to the callback SGTM.

@RByers
Copy link
Collaborator Author

RByers commented Aug 1, 2018

This is now done and shipped in Chrome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants