Create a SecurityReporter component for TLS error reports

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mgoodwin, Assigned: mgoodwin)

Tracking

unspecified
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
TLS Error reports are currently created and sent with a set of interactions between the TLS error pages (aboutNetError.xhtml and aboutCertError.xhtml), content.js and browser.js... which means, currently, it's only possible to get error reports connections which pertain to actual documents.

We want this same functionality for subresources (bug 1241455) but first we need to build a component containing the reporting funtionality.

This may also be used for SafeBrowsing reports in the future (see bug 1214859).
(Assignee)

Updated

3 years ago
Summary: Create a SecurityErrorReporter component for TLS error reports → Create a SecurityReporter component for TLS error reports
(Assignee)

Comment 2

3 years ago
Created attachment 8712153 [details]
MozReview Request: Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler

This takes the TLS Error Reporting functionality used in the aboutNetError.xhtml
and aboutCertError.xhtml error pages and moves it to its own component. This
allows us to make use of this same error reporting functionality from elsewhere.
Notably, this allows us to send error reports for issues that occur when loading
subresources.
The xpcshell test included is in security/manager/ssl/tests because we need to
make use of tlsserver functionality from the PSM tests.

Review commit: https://reviewboard.mozilla.org/r/32437/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32437/
Attachment #8712153 - Flags: review?(dtownsend)
Attachment #8712153 - Flags: review?(dkeeler)
(Assignee)

Updated

3 years ago
Attachment #8712148 - Attachment is obsolete: true
Comment on attachment 8712153 [details]
MozReview Request: Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler

https://reviewboard.mozilla.org/r/32437/#review29229

LGTM!

::: b2g/installer/package-manifest.in:716
(Diff revision 1)
> +@RESPATH@/components/nsSecurityReporter.js

Let's not use the ns prefix for anything new (except for interface names I guess?)

::: security/manager/ssl/tests/unit/test_toolkit_securityreporter.js:66
(Diff revision 1)
> +const registrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);

I think we have very similar code elsewhere in PSM tests - can we refactor this to head_psm.js?

::: security/manager/ssl/tests/unit/test_toolkit_securityreporter.js:91
(Diff revision 1)
> +                              function(request, response) {

nit: indentation
Attachment #8712153 - Flags: review?(dkeeler) → review+
Comment on attachment 8712153 [details]
MozReview Request: Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler

https://reviewboard.mozilla.org/r/32437/#review29499

Before I look closer, is there a particular reason this needs to be an XPCOM component or could it just work as SecurityReporter.jsm in toolkit/modules?

::: security/manager/ssl/tests/unit/test_toolkit_securityreporter.js:70
(Diff revision 1)
> +                          XULAPPINFO_CONTRACTID, XULAppInfoFactory);

You can probably just use AppInfo.jsm rather than all this.
Attachment #8712153 - Flags: review?(dtownsend)
(Assignee)

Comment 5

3 years ago
(In reply to Dave Townsend [:mossop] from comment #4)
> Before I look closer, is there a particular reason this needs to be an XPCOM
> component or could it just work as SecurityReporter.jsm in toolkit/modules?

Yes; this will be called from JS (e.g. browser/base/content/browser.js) and C++ (bits of netwerk; initially netwerk/protocol/http/nsHttpChannel.cpp).
(Assignee)

Comment 6

3 years ago
Comment on attachment 8712153 [details]
MozReview Request: Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32437/diff/1-2/
Attachment #8712153 - Flags: review?(dtownsend)
Comment on attachment 8712153 [details]
MozReview Request: Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler

https://reviewboard.mozilla.org/r/32437/#review29593

::: toolkit/components/securityreporter/SecurityReporter.js:13
(Diff revision 2)
> +const nsIIOService = Cc["@mozilla.org/network/io-service;1"]

You can use Services.io for this.

::: toolkit/components/securityreporter/SecurityReporter.js:15
(Diff revision 2)
> +const Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services;

Use const { Services } instead.

::: toolkit/components/securityreporter/SecurityReporter.js:17
(Diff revision 2)
> +const appInfo = Cc["@mozilla.org/xre/app-info;1"]

This is just Services.appinfo
Attachment #8712153 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 8

3 years ago
Comment on attachment 8712153 [details]
MozReview Request: Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32437/diff/2-3/
Attachment #8712153 - Attachment description: MozReview Request: Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r?mossop, keeler → MozReview Request: Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdecb787f1a2c12be5f0059afe214868402d2479
Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler
And a free ESLint fix because it's a slow day:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae7246d654c8
(Assignee)

Comment 15

3 years ago
Ah. I see the problem - -api-11 builds don't happen on try so I missed my android sanity check. Here's another (all good this time): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed926e92ba62
Flags: needinfo?(mgoodwin)
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c99d5b0f40f64632e5590c71af6e90bfec1833ce
Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c99d5b0f40f6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1245320
You need to log in before you can comment on or make changes to this bug.