Closed Bug 1092055 Opened 5 years ago Closed 5 years ago

Some security messages in the web console only show for top level channels

Categories

(Core :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

Details

Attachments

(2 files, 4 obsolete files)

HPKP and HSTS related security messages appear for the top level loads but not for any other resources.
STR:
1) Create a resource which sets an invalid HPKP or HSTS header (e.g. an image)
2) Create a document which uses this resource
3) Open the web console
4) Visit the document from step #2
Attached patch bug1092055_test.patch (obsolete) — Splinter Review
A mochitest to test for successful inclusion of a non-top-level security message (HSTS, in this case)
Depends on: 1038756
Component: Developer Tools: Console → General
Product: Firefox → Core
Assignee: mgoodwin → nobody
Assignee: nobody → mgoodwin
Attached patch Bug1092055.patchSplinter Review
Send messages directly to the console so that subresources' security error messages end up in the devtools webconsole for the correct tab.
Attachment #8561392 - Flags: review?(mcmanus)
Attached patch bug1092055_test.patch (obsolete) — Splinter Review
Devtools mochitest to ensure that security messages are sent to the web console for subresources
Attachment #8514853 - Attachment is obsolete: true
Attachment #8561393 - Flags: review?(past)
Attachment #8561392 - Flags: review?(mcmanus) → review+
Comment on attachment 8561393 [details] [diff] [review]
bug1092055_test.patch

Review of attachment 8561393 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/webconsole/test/browser_webconsole_show_subresource_security_errors.js
@@ +2,5 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + *
> + * ***** END LICENSE BLOCK ***** */

Nit: no need for the BEGIN/END lines.

::: browser/devtools/webconsole/test/test_bug1092055_shouldwarn.html
@@ +3,5 @@
> +<head>
> +  <meta charset="UTF-8">
> +  <title>Bug 1092055 - Log console messages for non-top-level security errors</title>
> +  <script src="test_bug1092055_shouldwarn.js"></script>
> +</head>

Nit: we put license comments even in html files (in the <head>), like this:
<!-- Any copyright is dedicated to the Public Domain.
     http://creativecommons.org/publicdomain/zero/1.0/ -->
Attachment #8561393 - Flags: review?(past) → review+
Attached patch bug1092055_test.patch (obsolete) — Splinter Review
Nits addressed
Attachment #8561393 - Attachment is obsolete: true
Attachment #8561959 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e88d62a7bf4 

The Win7 failures seem to be as a result of Bug 1087726: bobowen reports that tests pass with the patches for this bug, but fail with the patches for Bug 1087726 applied.
Keywords: checkin-needed
So yeah, let's run this through Try next time to verify that the failures are actually fixed rather than just assuming so.
Attached patch Bug1092055_test.patch (obsolete) — Splinter Review
Modified browser/devtools/webconsole/test/browser_webconsole_certificate_messages.js to deal with webconsole message leakage on Windows 7.
Attachment #8561959 - Attachment is obsolete: true
... and trying again, this time with no subresources (waiting for messages-cleared isn't enough to win the race)
Attachment #8563322 - Attachment is obsolete: true
Comment on attachment 8563337 [details] [diff] [review]
Bug1092055_test.patch

Panos, this is changed only to add a data: favicon in test-certificate-messages.html (to prevent sub-resource loads as a workaround for the warning message leakage discussed earlier).
Attachment #8563337 - Flags: review?(past)
Comment on attachment 8563337 [details] [diff] [review]
Bug1092055_test.patch

Review of attachment 8563337 [details] [diff] [review]:
-----------------------------------------------------------------

Yup.
Attachment #8563337 - Flags: review?(past) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dac58ad1d7f2
https://hg.mozilla.org/mozilla-central/rev/d393df30168d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1319118
You need to log in before you can comment on or make changes to this bug.