Move reportTLS functionality for cert error pages into content.js

RESOLVED FIXED in Firefox 59

Status

()

Firefox
Security
P1
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

(Blocks: 2 bugs)

58 Branch
Firefox 59
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox59 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
This would prevent the child from sending serialized certificate information to the parent process and reduce some complexity.
Flags: qe-verify-
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Blocks: 1297630

Comment 3

2 months ago
mozreview-review
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)
Attachment #8934169 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 4

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

https://reviewboard.mozilla.org/r/205112/#review210634
Attachment #8934169 - Flags: review?(mgoodwin) → review+
(Assignee)

Comment 5

2 months ago
(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 hidden (mozreview-request)

Comment 7

2 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/705d11b6aea6
Move reportTLS functionality for cert error pages into content.js. r=Gijs,mgoodwin

Comment 8

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/705d11b6aea6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.