1422811 - Move reportTLS functionality for cert error pages into content.js

Status: RESOLVED FIXED

(Firefox :: Security, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

This would prevent the child from sending serialized certificate information to the parent process and reduce some complexity.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1422811 - Move reportTLS functionality for cert error pages into content.js.

This also ensures that we consider certificate error pages in frames as well as top-level.

Review commit: https://reviewboard.mozilla.org/r/205112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205112/

Comment 2

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9851db02d036e6fb9c0f35204a3b5a8c51bd81b

Comment 3

[:Gijs (he/him)]

Comment on attachment 8934169 [details]
Bug 1422811 - Move reportTLS functionality for cert error pages into content.js.

https://reviewboard.mozilla.org/r/205112/#review210614

::: browser/base/content/content.js:458
(Diff revision 1)
> +      let {securityInfo} = docShell.failedChannel;
> +      securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
> +      let {host, port} = win.document.mozDocumentURIIfNotForErrorPages;
> +      let errorReporter = Cc["@mozilla.org/securityreporter;1"]
> +                            .getService(Ci.nsISecurityReporter);
> +      errorReporter.reportTLSError(securityInfo, host, port);

Can you verify that if we bail early from the reporter in content processes only, https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_ssl_error_reports.js fails ? (Basically, I want to make sure the test coverage there actually verifies we send reports)

Comment 4

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8934169 [details]
Bug 1422811 - Move reportTLS functionality for cert error pages into content.js.

https://reviewboard.mozilla.org/r/205112/#review210634

Comment 5

[Johann Hofmann [:johannh]]

(In reply to :Gijs from comment #3)
> Comment on attachment 8934169 [details]
> Bug 1422811 - Move reportTLS functionality for cert error pages into
> content.js.
> 
> https://reviewboard.mozilla.org/r/205112/#review210614
> 
> ::: browser/base/content/content.js:458
> (Diff revision 1)
> > +      let {securityInfo} = docShell.failedChannel;
> > +      securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
> > +      let {host, port} = win.document.mozDocumentURIIfNotForErrorPages;
> > +      let errorReporter = Cc["@mozilla.org/securityreporter;1"]
> > +                            .getService(Ci.nsISecurityReporter);
> > +      errorReporter.reportTLSError(securityInfo, host, port);
> 
> Can you verify that if we bail early from the reporter in content processes
> only,
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> general/browser_ssl_error_reports.js fails ? (Basically, I want to make sure
> the test coverage there actually verifies we send reports)

Thanks, that's a good idea, I checked whether the test was passing but I didn't try to break it! I verified that it timeouts now.

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8934169 [details]
Bug 1422811 - Move reportTLS functionality for cert error pages into content.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205112/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/705d11b6aea6
Move reportTLS functionality for cert error pages into content.js. r=Gijs,mgoodwin

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/705d11b6aea6