Skip to content
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

feat: enable reporting api #18255

Merged
merged 3 commits into from May 29, 2019

Conversation

@nornagon
Copy link
Member

nornagon commented May 11, 2019

Description of Change

Fixes #18252.

Checklist

Release Notes

Notes: Enabled the W3C Reporting API.

@nornagon nornagon requested a review from deepak1556 May 11, 2019
@@ -435,7 +435,7 @@ bool AtomNetworkDelegate::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
// https://crbug.com/704259
bool AtomNetworkDelegate::OnCanQueueReportingReport(
const url::Origin& origin) const {
return false;
return true;

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound May 11, 2019

Member

Is this something we might want to allow users to override. E.g. return Emit("will-queue-report")

Idk what the surface of this is, is it just a web feature we should have enabled a while back or is it more 🤔

This comment has been minimized.

Copy link
@nornagon

nornagon May 13, 2019

Author Member

is it just a web feature we should have enabled a while back

yup. https://w3c.github.io/reporting

This comment has been minimized.

Copy link
@nornagon

nornagon May 16, 2019

Author Member

I'm not sure it makes a ton of sense to expose this as something users might want to override. Maybe? But this is a synchronous API on the IO thread and it's unclear how to trigger an event for that effectively and get a response.

I think it makes sense to defer adding a feature for this until such a time as it is needed. Otherwise we'll likely build the wrong API.

This comment has been minimized.

Copy link
@nornagon

nornagon May 23, 2019

Author Member

On further consideration, it probably does make sense to expose some of this stuff.

@electron-cation electron-cation bot removed the new-pr 🌱 label May 12, 2019
Copy link
Member

deepak1556 left a comment

https://bugs.chromium.org/p/chromium/issues/detail?id=704259 is the implementation bug, based on it.

If we only want to read reports this looks good, any reason not to add support for sending reports OnCanSendReportingReports , most of the work seems to be achieved by the reporting service, we just need to respond based on permissions ?

Also if we want to provide a way to clear reporting cache , we can extend session.clearStorageData and add a call to net::ReportingService::RemoveBrowsingData

atom/browser/net/atom_network_delegate.cc Outdated Show resolved Hide resolved
@nornagon nornagon force-pushed the enable-network-logging branch from d793d2e to f92467d May 23, 2019
@nornagon nornagon marked this pull request as ready for review May 23, 2019
@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented May 29, 2019

macos is a flake, passed on non-mas.

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented May 29, 2019

@nornagon would you like to make a few more changes to this or is it ok to merge as-is?

@nornagon

This comment has been minimized.

Copy link
Member Author

nornagon commented May 29, 2019

I haven't fully thought through the implications of enabling the reporting API by default with no option to disable it in apps. At first blush I don't think there's an issue (if you're loading a website with headers you don't control, then you're probably going to be running JS you don't control also...) but I haven't thought through all the potential ways it could be dangerous.

Adding @electron/wg-security for their thoughts.

@nornagon nornagon requested a review from electron/wg-security May 29, 2019
@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented May 29, 2019

I haven't fully thought through the implications of enabling the reporting API by default with no option to disable it in apps

Isn't it still behind a feature flag ? Can't apps disable it via --disable-features

but I haven't thought through all the potential ways it could be dangerous.

Not sure either, but the reporting types clearly seems harmless https://w3c.github.io/reporting/#report-types . If apps are loading remote content not they control, then they should be running in a sandboxed environment, also they still have the ability to control headers via the webRequest api

@nornagon

This comment has been minimized.

Copy link
Member Author

nornagon commented May 29, 2019

Ah yeah, I hadn't considered the disable-features route. That should work.

In that case, I think I'll go ahead and merge this. We can add configuration options and permissions requests / filtering handlers in a followup. Until then, apps can disable the feature if they want.

@nornagon nornagon merged commit f4c792d into master May 29, 2019
12 of 13 checks passed
12 of 13 checks passed
appveyor: win-x64-testing AppVeyor build failed
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190524.1 succeeded
Details
electron-arm64-testing Build #20190524.1 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented May 29, 2019

Release Notes Persisted

Enabled the W3C Reporting API.

@nornagon nornagon deleted the enable-network-logging branch May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.