1170476 - Avoid logging multiple SHA-1 warnings per page

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Sebastian Zartner [:sebo]]

Since logging of SHA-1 weakness warnings was implemented in bug 1068949, they are flooding the console on pages that still use SHA-1, as the message is shown for each request.

Instead it would be enough to show the message once per page, i.e. once for all requests falling under the same origin policy.

Sebastian

Comment 1

[wheany]

This error is especially annoying since often it's caused by services that I have nothing to do with and have no control over. If I open developer tools on Youtube (when I'm developing a Greasemonkey user script, for example), the console is flooded with completely useless warnings for each thumbnail the browser loads.

Comment 2

[Spown]

And since they are often generated by ajax responses - they never get grouped

Comment 3

[vaispavel]

Attachment: Example of error - It's driving web developers crazy.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 4

[Daniele "Mte90" Scasciafratte]

very annoying!

Comment 5

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1170476.patch

Panos, does this seem like a sensible approach for de-duping these errors?

In the case of tls errors, the error relates to the connection, not the request - because of this it doesn't make sense to have the error for each resource loaded, but there's no way of getting this through from the underlying tls connection to the webconsole. 

My approach here is to get the prePath of the URI (which effectively represents an origin) from the request and check to see if it's been seen previously before logging an entry. We may (probably in fact) want to do something similar for other TLS errors and duplicate CSP warnings, so my sketch here includes the possiblity of doing this for more than one error type.

Thanks.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8626418 [details] [diff] [review]
Bug1170476.patch

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

I'm fine with the check itself, but I think we should move this logic to the _filterRepeatedMessage method, which exists for precisely this reason.

::: browser/devtools/webconsole/webconsole.js
@@ +372,5 @@
>    // Width of the monospace characters in Web Console's input box.
>    _inputCharWidth: 0,
>  
> +  // we track some security errors to de-duplicate by host / resource
> +  _observedSecurityErrors: {},

Nit: we prefer maps to dictionary objects.

@@ +1462,5 @@
> +      let observedOrigins
> +          = this._observedSecurityErrors[aScriptError.category];
> +      // Do we have observed origins?
> +      if (observedOrigins) {
> +        for (observedOrigin of observedOrigins) {

for (let observedOrigin of observedOrigins)

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Turns out that _filterRepeatedMessage isn't getting called for CATEGORY_SECURITY messages and I need to investigate why that happened in the first place. If that's still the right thing to do, then this approach is fine.

Comment 8

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1170476.patch

Nits addressed (Assuming we stick with this approach).

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

I've gone through bug 837351 that introduced the security category and bug 760876, which modified the conditional that decides the categories that concern _filterRepeatedMessage and I don't see any rationale for excluding CATEGORY_SECURITY from that conditional. Bug 760876 landed after the patch in bug 837351 and didn't include CATEGORY_SECURITY, but it was an unrelated change so that is to be expected.

In light of the above I think we should try to include CATEGORY_SECURITY in the repeatNode construction inside createMessageNode, so that we have a single mechanism to filter duplicate nodes.

Comment 10

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1170476_alternate.patch

(In reply to Panos Astithas [:past] from comment #9)
> In light of the above I think we should try to include CATEGORY_SECURITY in
> the repeatNode construction inside createMessageNode, so that we have a
> single mechanism to filter duplicate nodes.

Looking at the repeatNode construction, those are != operators - in other words, the de-duping already works for security messages provided the URI is the same (not sure what was going on with my step-through the other day).

This alternate patch is a few lines of change to reduce sha-1 message URIs to the prePath component of the URI.

Does this need a test? It's a tiny change.

Comment 11

[Kevin Hunt]

Hi guys, thanks a lot for working this. It's a big pita for debugging interactions with Google API's. I just wanted to check that this would catch initial page loads as well as the ajax calls to the same api targets, ignoring parameter lists.

Comment 12

[Mark Goodwin [:mgoodwin]]

(In reply to Kevin Hunt from comment #11)
> Hi guys, thanks a lot for working this. 

No problem; where possible, we like features to be useful and not annoying.

> I just wanted to check that this would catch
> initial page loads as well as the ajax calls to the same api targets,
> ignoring parameter lists.

The current patch on this bug is intended to give you a single line per server.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8628757 [details] [diff] [review]
Bug1170476_alternate.patch

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

I prefer this much more and I agree that a test is not critical in this case.

::: browser/devtools/webconsole/webconsole.js
@@ +1453,5 @@
> +    let sourceURI = Services.io.newURI(aScriptError.sourceName, null, null).QueryInterface(Ci.nsIURL);
> +    let displayOrigin = aScriptError.sourceName;
> +
> +
> +    if (aScriptError.category == "SHA-1 Signature") {

It would be nice to add a comment about TLS errors being related to the connection and not the request here.

Comment 14

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1170476_alternate.patch

Moved the creation of the URI into the if statement to address test weirdness.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70c952093c62

Comment 16

[Mark Goodwin [:mgoodwin]]

Try run, for reference https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef0aa9cb7fcb

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70c952093c62

Comment 18

[Mike Kaply [:mkaply]]

Does this patch have any dependencies or could it be backported at least to Aurora for developers?

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8630043 [details] [diff] [review]
Bug1170476_alternate.patch

An uplift request seems reasonable to me.

Approval Request Comment
[Feature/regressing bug #]: bug 1068949
[User impact if declined]: SHA-1 warnings can be logged many times per tab
[Describe test coverage new/current, TreeHerder]: On m-c
[Risks and why]: Low
[String/UUID change made/needed]: None

Comment 20

[Sebastian Zartner [:sebo]]

Attachment: SHA-1 message repetition counter displayed

After testing today's Nightly I see just entry for the SHA-1 message. Hurray!

Though I still see the repetition counter counting up. See the attached screenshot.
Can this be avoided, too? Should I file another bug for that?

Sebastian

Comment 21

[Mark Goodwin [:mgoodwin]]

(In reply to Sebastian Zartner [:sebo] from comment #20)
> I still see the repetition counter counting up. See the attached
> screenshot.
> Can this be avoided, too? Should I file another bug for that?

My original patch did avoid this; Panos' take on this was that it's neater to use the existing de-duplication functionality in the webconsole (I'm inclined to agree with him).

The downside is that the counter goes up - but I'm undecided on whether that's actually a bad thing: On the one hand, the error relates to a connection, not a resource but, on the other, it's an indicator of how many resources come from that origin and that might be useful to someone.

Comment 22

[Kate Glazko]

Comment on attachment 8630043 [details] [diff] [review]
Bug1170476_alternate.patch

Approving for uplift because has been on m-c and is low risk.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea81274ad510

Comment 24

[Spown]

>>Target Milestone: 	Firefox 42 

Any chance this fix can be moved a little bit into an earlier release? I've tried to use nightly with firebug 3a just to profit from this fix but as of now they are unusable. Please?

Comment 25

[Daniele "Mte90" Scasciafratte]

(In reply to Spown from comment #24)
> >>Target Milestone: 	Firefox 42 
> 
> Any chance this fix can be moved a little bit into an earlier release? 

+1

Is very annoying work with this messages

Comment 26

[petr.pavel]

(In reply to Daniele "Mte90" Scasciafratte from comment #25)
> (In reply to Spown from comment #24)
> > >>Target Milestone: 	Firefox 42 
> > 
> > Any chance this fix can be moved a little bit into an earlier release? 
> 
> +1
> 
> Is very annoying work with this messages

+1

Comment 27

[Mark Goodwin [:mgoodwin]]

(In reply to Spown from comment #24)
> >>Target Milestone: 	Firefox 42 
> 
> Any chance this fix can be moved a little bit into an earlier release? I've
> tried to use nightly with firebug 3a just to profit from this fix but as of
> now they are unusable. Please?

yes; see comments related to uplift.

Comment 28

[Spown]

oh, so thats what "uplift" means... sorry.

Comment 29

[patheticcockroach]

Hello,
If I understand well, this is supposed to be fixed in the current version? I'm using Firebug 2.0.13 in Firefox 42 but I still have many, many SHA-1 warnings in a single page (requests go to 2 servers so I believe with this patch I should get at most 2 warnings). "Group log messages" is checked, did I miss another setting?

Comment 30

[Mark Goodwin [:mgoodwin]]

(In reply to patheticcockroach from comment #29)
> did I miss another setting?

No; this bug deals with the Firefox web console, not other tools (like Firebug) that are able to consume messages from Firefox (Firefox can't control how those tools handle the messages they receive).

If Firebug is supposed to group messages and isn't, it could be that you've found a problem with Firebug.