Closed Bug 1241821 Opened 8 years ago Closed 8 years ago

Create a SecurityReporter component for TLS error reports

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

Details

Attachments

(1 file, 1 obsolete file)

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).
Summary: Create a SecurityErrorReporter component for TLS error reports → Create a SecurityReporter component for TLS error reports
Attached patch Bug1241821.patch (obsolete) — Splinter Review
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)
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)
(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).
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+
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdecb787f1a2c12be5f0059afe214868402d2479
Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler
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)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c99d5b0f40f64632e5590c71af6e90bfec1833ce
Bug 1241821 - Create a SecurityReporter component for TLS Error Reports r=mossop, keeler
https://hg.mozilla.org/mozilla-central/rev/c99d5b0f40f6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1245320
Depends on: 1501680
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: