898712 - Add error category to SSL error messages sent to Browser Console (aka. Error Console)

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Kailas]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/28.0.1500.71 Chrome/28.0.1500.71 Safari/537.36

Steps to reproduce:

SSL error messages are sent to Browser console as string messages. They are not instance of nsIScriptError and does not include error category in the message such as "SSL" or "Secure Socket Layer" category should be associated with the SSL errors or warning generated by browser. 



Expected results:

SSL errors should have error category associated with them.

Comment 1

[Kailas]

Attachment: bug-898712.patch

Patch adds "SSL" category to SSL errors and creates them an instance of nsIScriptError.

Comment 2

[Kailas]

Attachment: bug-898712.patch

Patch adds "SSL" category to SSL errors and creates them an instance of nsIScriptError.

Comment 3

[Kailas]

How can I change status (UNCONFIRMED) of this bug ? Status drop-down box only shows me two options: UNCONFIRMED and RESOLVED.

Comment 4

[Tanvi Vyas[:tanvi]]

(In reply to Kailas from comment #3)
> How can I change status (UNCONFIRMED) of this bug ? Status drop-down box
> only shows me two options: UNCONFIRMED and RESOLVED.

Not sure why you are seeing this, but I've changed the status to NEW and assigned this bug to you.

Comment 5

[Mark Goodwin [:mgoodwin]]

Comment on attachment 788476 [details] [diff] [review]
bug-898712.patch

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

Fix the whitespace issue and it's OK with me. You'll want review from grobinson or someone, I expect.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +226,5 @@
> +                       EmptyString(), 
> +                       EmptyString(), 
> +                       0, 
> +                       0, 
> +                       nsIScriptError::errorFlag, 

Nit: All args to Init so far have trailing white space.

Comment 6

[Kailas]

Attachment: bug-898712.patch

Trailing white space removed from all args of Init

Comment 7

[Camilo Viecco (:cviecco)]

Comment on attachment 790344 [details] [diff] [review]
bug-898712.patch

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

drive by review.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +217,5 @@
>    
>    if (!message.IsEmpty()) {
>      nsCOMPtr<nsIConsoleService> console;
>      console = do_GetService(NS_CONSOLESERVICE_CONTRACTID);
> +    nsCOMPtr<nsIScriptError> error;

I would rename this variable.. error is too generic in a function that handles errors. Better scriptErr (?) I am wide open to suggestions.

@@ +220,5 @@
>      console = do_GetService(NS_CONSOLESERVICE_CONTRACTID);
> +    nsCOMPtr<nsIScriptError> error;
> +    error = do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
> +
> +    if (console && error) {

This block is hard to read. I think it is clrearer to have it:

if(console) {
  nsCOMPtr<nsIScriptError> error;
  scriptError = do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
  if (scriptError) {
    rv=scrptError->Init....
  }
  else {
    console->Log.....
  }
}

Comment 8

[Kailas]

(In reply to Camilo Viecco (:cviecco) from comment #7)
> Comment on attachment 790344 [details] [diff] [review]
> bug-898712.patch
> 
> Review of attachment 790344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> drive by review.
> 
> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +217,5 @@
> >    
> >    if (!message.IsEmpty()) {
> >      nsCOMPtr<nsIConsoleService> console;
> >      console = do_GetService(NS_CONSOLESERVICE_CONTRACTID);
> > +    nsCOMPtr<nsIScriptError> error;
> 
> I would rename this variable.. error is too generic in a function that
> handles errors. Better scriptErr (?) I am wide open to suggestions.
> 
How about sslScriptErr?

> @@ +220,5 @@
> >      console = do_GetService(NS_CONSOLESERVICE_CONTRACTID);
> > +    nsCOMPtr<nsIScriptError> error;
> > +    error = do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
> > +
> > +    if (console && error) {
> 
> This block is hard to read. I think it is clrearer to have it:
> 
> if(console) {
>   nsCOMPtr<nsIScriptError> error;
>   scriptError = do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
>   if (scriptError) {
>     rv=scrptError->Init....
>   }
>   else {
>     console->Log.....
>   }
> }

Comment 9

[Kailas]

Attachment: bug-898712.patch1

cviecco: updated patch according to your suggestions. Thanks!
I created this patch on hg changeset:142619:a8daa428ccbc

Comment 10

[Tanvi Vyas[:tanvi]]

Comment on attachment 790634 [details] [diff] [review]
bug-898712.patch1

>diff -r a8daa428ccbc security/manager/ssl/src/SSLServerCertVerification.cpp
>--- a/security/manager/ssl/src/SSLServerCertVerification.cpp	Wed Aug 14 17:04:38 2013 -0400
>+++ b/security/manager/ssl/src/SSLServerCertVerification.cpp	Thu Aug 15 09:41:08 2013 +0530
>@@ -116,6 +116,8 @@
> #include "nsIConsoleService.h"
> #include "PSMRunnable.h"
> #include "SharedSSLState.h"
>+#include "nsError.h"
>+#include "nsIScriptError.h"
> 
> #include "ssl.h"
> #include "secerr.h"
>@@ -210,13 +212,29 @@
>                     nsIX509Cert* ix509)
> {
>   nsString message;
>+  nsresult rv;
>   socketInfo->GetErrorLogMessage(errorCode, errorMessageType, message);
>   
>   if (!message.IsEmpty()) {
>     nsCOMPtr<nsIConsoleService> console;
>     console = do_GetService(NS_CONSOLESERVICE_CONTRACTID);
>     if (console) {
>-      console->LogStringMessage(message.get());
>+      nsCOMPtr<nsIScriptError> scriptError;
>+      scriptError = do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
>+      if (scriptError) {
>+        rv = scriptError->Init(message,
>+                         EmptyString(),
>+                         EmptyString(),
>+                         0,
>+                         0,
>+                         nsIScriptError::errorFlag,
>+                         "SSL");
"SSL" is the category you are using.  You shoudl add that category here so that it gets mapped to CATEGORY_SECURITY instead of the default CATEGORY_JS:
https://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.js#4533 


>+        if (NS_SUCCEEDED(rv)) {
>+          console->LogMessage(scriptError);
>+          return;
>+        }
>+      }
>+       console->LogStringMessage(message.get());
>     }
>   }
> }

Comment 11

[Tanvi Vyas[:tanvi]]

Comment on attachment 788476 [details] [diff] [review]
bug-898712.patch

Should this patch be marked as obsolete?

Comment 12

[Ivan Alagenchev :ialagenchev]

drive by review:

diff -r a8daa428ccbc security/manager/ssl/src/SSLServerCertVerification.cpp
--- a/security/manager/ssl/src/SSLServerCertVerification.cpp	Wed Aug 14 17:04:38 2013 -0400
+++ b/security/manager/ssl/src/SSLServerCertVerification.cpp	Thu Aug 15 09:41:08 2013 +0530

+        rv = scriptError->Init(message,
+                         EmptyString(),
+                         EmptyString(),
+                         0,
+                         0,
+                         nsIScriptError::errorFlag,
+                         "SSL");

Please align all consecutive arguments with the first(message) argument. 
+      }
+       console->LogStringMessage(message.get());

console is indented one extra space here.

Comment 13

[Kailas]

Attachment: bug-898712.patch

removed extra space and uploaded updated patch.

Comment 14

[Kailas]

(In reply to Tanvi Vyas [:tanvi] from comment #11)
> Comment on attachment 788476 [details] [diff] [review]
> bug-898712.patch
> 
> Should this patch be marked as obsolete?

Yes. Done

Comment 15

[Ivan Alagenchev :ialagenchev]

Hi Kailas, did you upload the right patch? It doesn't appear that you addressed the issues that we brought up above.

Comment 16

[Ivan Alagenchev :ialagenchev]

Also, tanvi, cviecco and I aren't module peers for the module. You should feedback? us and r? at the end once we are done with our feedback. The people that you can request r+ from for each module are listed here: https://wiki.mozilla.org/Modules/All#Submodules. This module is under Security - Mozilla PSM Glue.

Comment 17

[Kailas]

Attachment: bug-898712.patch

updated patch.

Comment 18

[Camilo Viecco (:cviecco)]

Comment on attachment 791394 [details] [diff] [review]
bug-898712.patch

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

r+ With comments addressed

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +211,5 @@
>                      ::mozilla::psm::SSLErrorMessageType errorMessageType,
>                      nsIX509Cert* ix509)
>  {
>    nsString message;
> +  nsresult rv;

minor issue. Move this declaration to the block where you actually use it.

Comment 19

[Kailas]

Attachment: bug-898712.patch

cviecco: comments addressed. 
Thanks to all for your valuable feedback. I will avoid those mistakes in future patches.

Comment 20

[Camilo Viecco (:cviecco)]

Comment on attachment 791594 [details] [diff] [review]
bug-898712.patch

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

Is there a way to add a test for this feature?

Comment 21

[Ivan Alagenchev :ialagenchev]

(In reply to Camilo Viecco (:cviecco) from comment #20)
> Comment on attachment 791594 [details] [diff] [review]
> bug-898712.patch
> 
> Review of attachment 791594 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there a way to add a test for this feature?

Yes. Kailas, please take a look at my tests for Bug 762593 to help you get started. Feel free to ask me questions if you have any.

Comment 22

[Kailas]

(In reply to Ivan Alagenchev :ialagenchev from comment #21)
> (In reply to Camilo Viecco (:cviecco) from comment #20)
> > Comment on attachment 791594 [details] [diff] [review]
> > bug-898712.patch
> > 
> > Review of attachment 791594 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Is there a way to add a test for this feature?
> 
> Yes. Kailas, please take a look at my tests for Bug 762593 to help you get
> started. Feel free to ask me questions if you have any.
I looked at the test cases for Bug 762593 and understood how to write test cases. However, it is not clear to me how to send SSL certificates with various errors (such as expired certificate, wrong certificate, etc). In particular,  Is there any way in mochitest to establish a https site and send different types of certificate?

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Kailas - these should work: https://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt#94

Comment 24

[Camilo Viecco (:cviecco)]

An example test with all errors is:
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html?force=1

Comment 25

[Tanvi Vyas[:tanvi]]

Comment on attachment 791594 [details] [diff] [review]
bug-898712.patch

For the /devtools/webconsole changes, you need review from someone on the devtools team (like msucan).

For the /security/manager/ssl/src changes, Brian could do the review.

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 791594 [details] [diff] [review]
bug-898712.patch

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +221,5 @@
> +      nsCOMPtr<nsIScriptError> scriptError;
> +      scriptError = do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
> +      if (scriptError) {
> +        nsresult rv;
> +        rv = scriptError->Init(message,

nit: nsresult rv = scriptError->Init(...), or simplify the code like this:

    if (console && NS_SUCCEEDED(scriptError->Init(...))

nit Please change the text wrapping so that we fit as much text on the line up to 80 chars as possible. i.e. don't put each new argument on its own line.

@@ +237,1 @@
>        console->LogStringMessage(message.get());

Please remove the console->LogStringMessage() call. If there is an error creating scriptError or there is an error initializing it, it is fine to just not log the message in those error cases. That's what other code is doing. Then, you can also remove the early return.

r+ with the above changes.

It seems like a similar changes is needed in HandshakCallback (in nsNSSCallbacks.cpp) and nsHandleSSLError (in nsNSSIOLayer.cpp). If/when you make those changes, please factor the common boilerplate logic into a separate function. These changes in this second paragraph can happen either in a follow-up patch in this bug, or in a new bug.

I will defer to others about whether we require an automated test for this.

Comment 27

[Kailas]

Attachment: bug-898712.patch

Patch generated on hg tip: changeset:   143295:d136c8999d96

Updated patch incorporated brian's suggestions. 

I tested this patch manually using security report tool extension I am developing on https://csptest.computerist.org website. 

Now I will try to create automated test cases.

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 793402 [details] [diff] [review]
bug-898712.patch

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

Good work, but like I said in the previous review, we should avoid the code duplication. I show you how below.

Please speak up if you run into trouble with the test. I wouldn't be surprised if you were to run into trouble that would make writing a test disproportionately difficult in relation to the value of having an automatic test.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +228,1 @@
>      }

Please factor this logic out into a function, like this:

void
LogSimpleConsoleError(const nsAString & message,
                      const char * classification)
  nsCOMPtr<nsIScriptError> scriptError = 
    do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
  if (scriptError) {
    nsCOMPtr<nsIConsoleService> console = 
      do_GetService(NS_CONSOLESERVICE_CONTRACTID);
    if (console && NS_SUCCEEDED(scriptError->Init(message, EmptyString(),
		EmptyString(), 0, 0,
		nsIScriptError::errorFlag,
		classification))) {
      console->LogMessage(scriptError);
    }
  }
}


Then you should be able to re-use this function to make all the logging calls one-liners:

LogSimpleConsoleError(message, "SSL");

IMO, such a function would be best put in nsContentUtils, but if you want to stuff it in PSM for now you could put it in nsNSSIOLayer.h (declaration) / nsNSSIOLayer.cpp (definition).

Comment 29

[Kailas]

Attachment: bug-898712.patch

Thanks! brain for your feedback! It helped me to improve the patch code.

Comment 30

[Kailas]

Attachment: bug-898712.patch

Sorry, previous patch had some nits (extra blank space).

Comment 31

[Garrett Robinson [:grobinson]]

Comment on attachment 793958 [details] [diff] [review]
bug-898712.patch

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

Drive by feedback!

There is a lot of overlap with this bug (and also with Bug 890224) and with the Security Console Utils work in Bug 897240. This is partially because the work in 897240 is designed to help your use case. Is it possible for you to reuse some of that work to achieve your goals here? A big difference seems to be that you want to report the errors generated by the underlying code directly, while the Security Console Utils work concerns itself with localization (a prerequisite for reporting anything to the Developer Tools, and indeed, ideally, anywhere).

::: browser/devtools/webconsole/webconsole.js
@@ +4549,5 @@
>        case "Mixed Content Blocker":
>        case "CSP":
>        case "Invalid HSTS Headers":
>        case "Insecure Password Field":
> +      case "SSL":

There is no reason to include this. Since you always init your nsIScriptError's with a innerWindowID of 0 in LogSimpleConsoleError, they will never make it any window's Web Console (they don't know which Web Console to be delivered to).

If you goal here was to report these messages to the Web Console, you're going to have to rethink some things. (They will still appear in the Browser Console no matter what).

Comment 32

[Kailas]

(In reply to Garrett Robinson [:grobinson] from comment #31)
> Comment on attachment 793958 [details] [diff] [review]
> bug-898712.patch
> 
> Review of attachment 793958 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive by feedback!
> 
> There is a lot of overlap with this bug (and also with Bug 890224) and with
> the Security Console Utils work in Bug 897240. This is partially because the
> work in 897240 is designed to help your use case. Is it possible for you to
> reuse some of that work to achieve your goals here? A big difference seems
> to be that you want to report the errors generated by the underlying code
> directly, while the Security Console Utils work concerns itself with
> localization (a prerequisite for reporting anything to the Developer Tools,
> and indeed, ideally, anywhere).
> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +4549,5 @@
> >        case "Mixed Content Blocker":
> >        case "CSP":
> >        case "Invalid HSTS Headers":
> >        case "Insecure Password Field":
> > +      case "SSL":
> 
> There is no reason to include this. Since you always init your
> nsIScriptError's with a innerWindowID of 0 in LogSimpleConsoleError, they
> will never make it any window's Web Console (they don't know which Web
> Console to be delivered to).
> 
According to comment #10, I included it to associate SSL errors with Security Category instead of JS category. 

> If you goal here was to report these messages to the Web Console, you're
> going to have to rethink some things. (They will still appear in the Browser
> Console no matter what).

Comment 33

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Garrett Robinson [:grobinson] from comment #31)
> There is a lot of overlap with this bug (and also with Bug 890224) and with
> the Security Console Utils work in Bug 897240. This is partially because the
> work in 897240 is designed to help your use case. Is it possible for you to
> reuse some of that work to achieve your goals here? A big difference seems
> to be that you want to report the errors generated by the underlying code
> directly, while the Security Console Utils work concerns itself with
> localization (a prerequisite for reporting anything to the Developer Tools,
> and indeed, ideally, anywhere).

I suggest we just land Kailas patch as-is, and then we file another bug for the localization and/or conversion to the utils class.
> ::: browser/devtools/webconsole/webconsole.js
> @@ +4549,5 @@
> >        case "Mixed Content Blocker":
> >        case "CSP":
> >        case "Invalid HSTS Headers":
> >        case "Insecure Password Field":
> > +      case "SSL":
> 
> There is no reason to include this. Since you always init your
> nsIScriptError's with a innerWindowID of 0 in LogSimpleConsoleError, they
> will never make it any window's Web Console (they don't know which Web
> Console to be delivered to).

Browser console doesn't care about the classification of messages?

> If you goal here was to report these messages to the Web Console, you're
> going to have to rethink some things. (They will still appear in the Browser
> Console no matter what).

Also worth filing a bug for.

Comment 34

[Garrett Robinson [:grobinson]]

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #33)
> > ::: browser/devtools/webconsole/webconsole.js
> > @@ +4549,5 @@
> > >        case "Mixed Content Blocker":
> > >        case "CSP":
> > >        case "Invalid HSTS Headers":
> > >        case "Insecure Password Field":
> > > +      case "SSL":
> > 
> > There is no reason to include this. Since you always init your
> > nsIScriptError's with a innerWindowID of 0 in LogSimpleConsoleError, they
> > will never make it any window's Web Console (they don't know which Web
> > Console to be delivered to).
> 
> Browser console doesn't care about the classification of messages?
> 

My mistake - I was thinking of the old Error Console, and forgot that the new Browser Console uses the same filtering code as the Web Console. I retract my earlier comment.

Comment 35

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 793958 [details] [diff] [review]
bug-898712.patch

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

grobinson, do you see any problem with us checking this in now, given the discussion above?

Comment 36

[Garrett Robinson [:grobinson]]

Comment on attachment 793958 [details] [diff] [review]
bug-898712.patch

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

Nope! The logging situation is kind of all over the place now. Bug 897240 was intended to be a neat solution to this as well as other logging needs, but I don't think we should block Kaila's work on it. I say check it in!

Comment 37

[Frederik Braun [:freddy]]

This has feedback+ and review+. Are we OK with checking it in?

Comment 38

[Garrett Robinson [:grobinson]]

(In reply to Frederik Braun [:freddyb] from comment #37)
> This has feedback+ and review+. Are we OK with checking it in?

I believe we are all in consensus on this.

Comment 39

[Frederik Braun [:freddy]]

Kailas, can you rechange the patch so it meets the criteria from this document (e.g. you need to specify bug number, describe the patch and say who reviewed it)? https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

After that, please re-add the checkin-needed keyword :)

Comment 40

[Kailas]

Attachment: Bug-898712.patch

Addresed comment #39: rechange the patch so it meets the criteria.

Comment 41

[Frederik Braun [:freddy]]

Thanks for looking again. I think this still needs the title (commit message)...
Example: hg qrefresh -m "Bug 12345 - Test bug, r=grobinson"
It's explained on the MDN link in comment #39.

Comment 42

[Kailas]

Freddyb: I did exactly the same, but I realize my mistake. After hg qrefresh -m " ...." I used command "hg qdiff > bug-898712.patch" command to generate a patch. Instead I should have copied patch from ".hg/patches" folder.

Comment 43

[Kailas]

Attachment: Bug-898712

patch ready for checked in.

Comment 44

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Kailas from comment #42)
> Freddyb: I did exactly the same, but I realize my mistake. After hg qrefresh
> -m " ...." I used command "hg qdiff > bug-898712.patch" command to generate
> a patch. Instead I should have copied patch from ".hg/patches" folder.

For future reference, `hg export qtip > somebug.patch` will do the right thing with the topmost applied patch in your queue.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f57dd2d6ec4d

Thanks for the patch! In the future, can you please include your name in your patches as well?

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f57dd2d6ec4d