762593 - Add warning/error Message to Web Console when the page includes Insecure Password fields

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Tanvi Vyas[:tanvi]]

When an http page includes an input type=password, add a warning or error to webconsole with a link to more information that describes the risks to users credentials.

Comment 1

[Paul Rouget [:paul]]

In which section? "Logging"? "Net"?

Comment 2

[Tanvi Vyas[:tanvi]]

Good question.  But I don't have an answer yet :)

Mihai, Paul - where do you think it should go?

Comment 3

[Paul Rouget [:paul]]

Maybe under a "Security" flag (as we have "error", "warning", …) in Logging.

Comment 4

[Mihai Sucan [:msucan]]

Paul that's exactly what I wanted to suggest and came back to see your already wrote down the idea. Good!

I think we should add Security to Logging - we consider those as "severities". Security messages are on their own a kind of log messages that are of "severe" importance. Not sure how to better explain it, but I believe it really fits.

Comment 5

[Rob Campbell [:rc] (:robcee)]

sounds like another candidate for the Security category.

Filter on GUNGNIR.

Comment 6

[Ivan Alagenchev :ialagenchev]

Attachment: proposed patch

Comment 7

[Garrett Robinson [:grobinson]]

Comment on attachment 765941 [details] [diff] [review]
proposed patch

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

This design looks good and the patch makes minor but useful changes to related interfaces. Once you've made the changes I suggest, I recommend splitting this patch into two attachments: one for the devtools code, and one for the DOM code. These are modules (https://wiki.mozilla.org/Modules/All) and changes to each need to be r+'ed by a peer from that module for the patch to land. If you don't know which module your changes affect, ask in IRC. Upload the two patches, labeled appropriately (Devtools 1 and DOM 1), and r? a peer from the relevant module for each patch. I recommend Mihai (:msucan) for the devtools code and mounir for the DOM code.

::: browser/devtools/webconsole/test/Makefile.in
@@ +137,5 @@
>  	browser_console_addonsdk_loader_exception.js \
>  	browser_console_error_source_click.js \
>  	browser_console_clear_on_reload.js \
>  	head.js \
> +	browser_webconsole_bug_762593_insecure_passwords.js \

Why not _insecure_passwords_web_console_warning.js or something else more explicit?

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords.js
@@ +3,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +  * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/test-bug-762593-insecure-passwords.html";
> +const INSECURE_PASSWORD_MSG = "The information on this page will be transmitted over an insecure channel and there are password fields present on it. This is considered a security risk! For more information navigate to this link";

We can workshop this, but I think this message could be shorter and clearer. I would maybe use "Password fields present on an insecure (http://) page. This is a security risk that allows user's login credentials to be stolen."

Remove the stuff about the "More Info" link for now. While that is a really nice idea, and one that I would like to see implemented, it is not trivial to do and we should probably open a separate bug for that. See the comments on Bug 875456.

@@ +18,5 @@
> +        name: "Insecure password error displayed successfully",
> +        text: INSECURE_PASSWORD_MSG,
> +        category: CATEGORY_SECURITY,
> +        severity: SEVERITY_WARNING
> +      },

Indentation

::: browser/devtools/webconsole/webconsole.js
@@ +4318,5 @@
>          return CATEGORY_CSS;
>  
>        case "Mixed Content Blocker":
>        case "CSP":
> +      case "PWD":

Use something more descriptive, like "Insecure Password Field".

::: content/html/content/src/HTMLInputElement.cpp
@@ +3419,5 @@
> +
> +  nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, "PWD", doc,
> +      nsContentUtils::eSECURITY_PROPERTIES, "InsecurePasswordsPresent");
> +
> +}

Style nit: there are some unnecessary newlines in this function. Otherwise, looks good.

@@ +3440,5 @@
>    // We already have a copy of the value, lets free it and changes the type.
>    FreeData();
>    mType = aNewType;
>  
> +  CheckForInsecurePasswordInput();

Why this location for the call? Might want to comment.

Comment 8

[Ivan Alagenchev :ialagenchev]

Attachment: DOM module patch

Comment 9

[Ivan Alagenchev :ialagenchev]

Attachment: DEVTOOLS module patch

Comment 10

[Ivan Alagenchev :ialagenchev]

Attachment: NECKO module patch

Comment 11

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

Comment on attachment 766189 [details] [diff] [review]
DOM module patch

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

::: content/html/content/src/HTMLInputElement.cpp
@@ +3409,5 @@
> +  bool isSSL;
> +  nsresult rv = httpChannel->GetUsingSSL(&isSSL);
> +  if(NS_FAILED(rv) || isSSL) {
> +    return;
> +  }

You need to use the document's principal for this, instead of the document's channel. In general, when making decisions with respect to security, you need to use principals.

I believe changing this will make the Necko patch unnecessary.

Comment 12

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

(In reply to Brian Smith (:bsmith) from comment #11)
> You need to use the document's principal for this, instead of the document's
> channel. In general, when making decisions with respect to security, you
> need to use principals.

And, it's probably best to use the node principal, instead of the document principal.

Comment 13

[Ivan Alagenchev :ialagenchev]

Attachment: DEVTOOLS module patch

Comment 14

[Ivan Alagenchev :ialagenchev]

Attachment: DOM module patch

This obsoletest the previous necko and dom patches since I am using the node principal now as per Brian's suggestions

Comment 15

[Tanvi Vyas[:tanvi]]

(In reply to Garrett Robinson [:grobinson] from comment #7)
> We can workshop this, but I think this message could be shorter and clearer.
> I would maybe use "Password fields present on an insecure (http://) page.
> This is a security risk that allows user's login credentials to be stolen."
> 
We should run this text by Larissa or someone who can help us make it clear.  Here's my first crack at it...

"Password fields present on an insecure (http://) page pose a security risk that allow attackers to steal login credentials."


> Remove the stuff about the "More Info" link for now. While that is a really
> nice idea, and one that I would like to see implemented, it is not trivial
> to do and we should probably open a separate bug for that. See the comments
> on Bug 875456.
>
I think this is one case where the More Info button is essential.  We have it for Mixed Content, but even if we didn't, a developer can easily look up what Mixed Content is and find an answer.  For insecure password fields this is a little tougher.  Developers (and even security experts) don't understand why collecting passwords on an HTTP page is a problem.  They see why they should post to an HTTPS destination, but don't understand the MITM threat to the HTTP page.

For now, can we hack a link together the way we did for Mixed Content?  As part of this bug, we will also have to write an mdn page describing what this new webconsole message means.  We'd link to that yet-to-be-created page.

Comment 16

[Tanvi Vyas[:tanvi]]

- We should also define and check the behavior in the case of iframes.  What should happen when a password field is in:
* an HTTP iframe embedded in an HTTPS page
* an HTTP iframe embedded in an HTTP page
* an HTTPS iframe embedded in an HTTP page
* an HTTPS iframe embedded in an HTTPS page

We may also decide we don't want to get into these details right now.  In case, we should just check what the current implementation does in these cases and make sure it is semi-sane.

- What if the page isn't http or https?  Do we need to check other schemes?  Can about: or ftp: pages have password input fields?

Comment 17

[Ivan Alagenchev :ialagenchev]

If the page is anything different than https, the warning message will be sent. I believe that takes care of the other schemes.

Comment 18

[Tanvi Vyas[:tanvi]]

(In reply to Ivan Alagenchev :ialagenchev from comment #17)
> If the page is anything different than https, the warning message will be
> sent. I believe that takes care of the other schemes.

On an about: page, you wouldn't want the warning to show up.  I can't think of an about: page that asks for a password today, but maybe that will happen in the future (hypothetical example: about:addons may have a way to login to addons.mozilla.org).

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 766559 [details] [diff] [review]
DEVTOOLS module patch

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

Patch looks good, thank you!

There's one problem: the string used includes the quotes. I think you need to not include them in the previous patch.

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 766561 [details] [diff] [review]
DOM module patch

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

Only one comment here:

::: dom/locales/en-US/chrome/security/security.properties
@@ +8,5 @@
>  # LOCALIZATION NOTE: Do not translate "X-Content-Security-Policy", "X-Content-Security-Policy-Report-Only",  "Content-Security-Policy" or "Content-Security-Policy-Report-Only"
>  OldCSPHeaderDeprecated=The X-Content-Security-Policy and X-Content-Security-Report-Only headers will be deprecated in the future. Please use the Content-Security-Policy and Content-Security-Report-Only headers with CSP spec compliant syntax instead.
>  # LOCALIZATION NOTE: Do not translate "X-Content-Security-Policy/Report-Only" or "Content-Security-Policy/Report-Only"
>  BothCSPHeadersPresent=This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-Content-Security-Policy/Report-Only header(s) will be ignored.
> +InsecurePasswordsPresent="Password fields present on an insecure (http://) page. This is a security risk that allows user's login credentials to be stolen."

None of the other messages are wrapped in quotes. You should take them out.

Comment 21

[Ivan Alagenchev :ialagenchev]

Mihai, thanks for the catch. I will fix that.

Comment 22

[Mounir Lamouri (:mounir)]

Comment on attachment 766561 [details] [diff] [review]
DOM module patch

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

In my opinion, this feature should not leave in the DOM code but in the devtools code. I do not want every <input> to have to run that code once or twice per page load.
Could the devtool code listen to the page load and simply does something like:
var inputs = document.getElementsByTagName('input');
inputs.forEach(function(i) {
  if (i.type != 'password') {
    return;
  }
  var uri = i.nodePrincipal.URI;
  // Check if the URI is HTTPS.
  // Report to console if not.
});

That way, this code will run only if the console is open and will prevent bloating HTMLInputElement with non-DOM related code.

WDYT?

(And thanks for trying to educate web developers about password and https, this is really appreciated :))

::: content/html/content/src/HTMLInputElement.cpp
@@ +665,5 @@
>    AddStatesSilently(NS_EVENT_STATE_ENABLED |
>                      NS_EVENT_STATE_OPTIONAL |
>                      NS_EVENT_STATE_VALID);
> +
> +  CheckForInsecurePasswordInput();

This is going to slow down page load.

@@ +3385,5 @@
>    // And now make sure our state is up to date
>    UpdateState(false);
>  }
> +bool
> +HTMLInputElement::IsHttps(nsIPrincipal *principal)

Arguments should use an "a" prefix -> aPrincipal.
We usually have the following coding style: "Foo* bar;", not "Foo *bar;".

@@ +3387,5 @@
>  }
> +bool
> +HTMLInputElement::IsHttps(nsIPrincipal *principal)
> +{
> +

Remove empty line.

@@ +3388,5 @@
> +bool
> +HTMLInputElement::IsHttps(nsIPrincipal *principal)
> +{
> +
> +  nsIURI *aRequestingLocation = NULL;

"a"-prefixed variable are used for arguments.
NULL should be nullptr.

@@ +3389,5 @@
> +HTMLInputElement::IsHttps(nsIPrincipal *principal)
> +{
> +
> +  nsIURI *aRequestingLocation = NULL;
> +  if (principal) {

Do we really expect valid calls to pass a null principal?

@@ +3393,5 @@
> +  if (principal) {
> +    nsCOMPtr<nsIURI> principalUri;
> +    nsresult rvalue = principal->GetURI(getter_AddRefs(principalUri));
> +
> +    if (NS_SUCCEEDED(rvalue)) {

Shouldn't you do NS_ENSURE_SUCCESS(rvalue, false); instead?

@@ +3408,5 @@
> +  if (NS_FAILED(rv)) {
> +    return false;
> +  }
> +
> +  return isHttps;

return NS_FAILED(rv) ? false : isHttps;

or 

NS_ENSURE_SUCCESS(rv, false);
return isHttps;

@@ +3431,5 @@
> +  nsDocument* doc = static_cast<nsDocument*>(iDocument.get());
> +  nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();
> +  if(!principal) {
> +    return;
> +  }

Shouldn't you get the principal with nsINode::NodePrincipal() on the HTMLInputElement instead of the document?

@@ +3433,5 @@
> +  if(!principal) {
> +    return;
> +  }
> +  bool isSSL = IsHttps(principal);
> +  if(isSSL) {

if (IsHttps(principal)) {
  return;
}

@@ +3462,5 @@
>  
> +  //password inputs change their type when the site is loaded, so we need
> +  //a check here to make sure we don't have insecure password fields on our
> +  //hands
> +  CheckForInsecurePasswordInput();

This is going to slow down page load and scripts.

::: content/html/content/src/HTMLInputElement.h
@@ +1173,5 @@
>    bool                     mIsDraggingRange     : 1;
>  
>  private:
>  
> +  bool IsHttps(nsIPrincipal *principal);

IsHttps() should be static because it doesn't use anything related to HTMLInputElement. Actually, it should probably not live in HTMLInputElement.

Comment 23

[Ivan Alagenchev :ialagenchev]

I've started working on figuring out how to access the page from the web console. The web console uses the remote debugging protocol, so accessing the page directly would not work in this case. In order to be able to achieve this, I would have to modify the web console actor. Here is a copy of the relevant discussion on irc for book-keeping purposes: 
 
msucan -> ialagenchev
the web console uses the remote debugging protocol and overall structure
which means you have the client in browser/devtools and the server in toolkit/devtools
from webconsole.js in browser/devtools/webconsole you cannot directly access the content of the page
you will need to change the web console actor

Comment 24

[Ivan Alagenchev :ialagenchev]

(In reply to Tanvi Vyas [:tanvi] from comment #16)
> - We should also define and check the behavior in the case of iframes.  What
> should happen when a password field is in:
> * an HTTP iframe embedded in an HTTPS page
> * an HTTP iframe embedded in an HTTP page
> * an HTTPS iframe embedded in an HTTP page
> * an HTTPS iframe embedded in an HTTPS page
> 
> We may also decide we don't want to get into these details right now.  In
> case, we should just check what the current implementation does in these
> cases and make sure it is semi-sane.
> 
> - What if the page isn't http or https?  Do we need to check other schemes? 
> Can about: or ftp: pages have password input fields?

Here is what I decided to do with iframes:
If the page is insecure - show a warning saying that there are passwords on an insecure page.
If the password fields are within iframes with http source - show a warning saying the passwords are within an insecure iframe
If the form action is insecure - show a warning indicating that. 
If all of the above are true, then the user will see 3 separate warnings informing them of each problem. I haven't contacted Larissa about the exact text yet.

Comment 25

[Ivan Alagenchev :ialagenchev]

(In reply to Tanvi Vyas [:tanvi] from comment #15)
> (In reply to Garrett Robinson [:grobinson] from comment #7)
> > We can workshop this, but I think this message could be shorter and clearer.
> > I would maybe use "Password fields present on an insecure (http://) page.
> > This is a security risk that allows user's login credentials to be stolen."
> > 
> We should run this text by Larissa or someone who can help us make it clear.
> Here's my first crack at it...
> 
> "Password fields present on an insecure (http://) page pose a security risk
> that allow attackers to steal login credentials."
> 
> 
> > Remove the stuff about the "More Info" link for now. While that is a really
> > nice idea, and one that I would like to see implemented, it is not trivial
> > to do and we should probably open a separate bug for that. See the comments
> > on Bug 875456.
> >
> I think this is one case where the More Info button is essential.  We have
> it for Mixed Content, but even if we didn't, a developer can easily look up
> what Mixed Content is and find an answer.  For insecure password fields this
> is a little tougher.  Developers (and even security experts) don't
> understand why collecting passwords on an HTTP page is a problem.  They see
> why they should post to an HTTPS destination, but don't understand the MITM
> threat to the HTTP page.
> 
> For now, can we hack a link together the way we did for Mixed Content?  As
> part of this bug, we will also have to write an mdn page describing what
> this new webconsole message means.  We'd link to that yet-to-be-created page.

So are we all agreeing that the more info should be part of this bug, or do we want to create a separate one for that part?

Comment 26

[Garrett Robinson [:grobinson]]

(In reply to Ivan Alagenchev :ialagenchev from comment #25)

> So are we all agreeing that the more info should be part of this bug, or do
> we want to create a separate one for that part?

Let's make a separate bug for that functionality, to keep the reviews simple. I would make the bug about creating a reusable mechanism to do what Bug 737873 does (i.e., to add informative links more than just mixed content). It would also be good to depend on Bug 875456, since that will probably remove the code from 737873 and consolidate the mixed content messages in the Security Panel.

Comment 27

[Ivan Alagenchev :ialagenchev]

(In reply to Garrett Robinson [:grobinson] from comment #26)
> (In reply to Ivan Alagenchev :ialagenchev from comment #25)
> 
> > So are we all agreeing that the more info should be part of this bug, or do
> > we want to create a separate one for that part?
> 
> Let's make a separate bug for that functionality, to keep the reviews
> simple. I would make the bug about creating a reusable mechanism to do what
> Bug 737873 does (i.e., to add informative links more than just mixed
> content). It would also be good to depend on Bug 875456, since that will
> probably remove the code from 737873 and consolidate the mixed content
> messages in the Security Panel.

That sound good to me, especially since that will allow me to get this one out the door faster.

Comment 28

[Ivan Alagenchev :ialagenchev]

Attachment: Feedback Patch

I went ahead and put in the logic for the more info link. I'm having hard time figuring out how to test if the url is correct in the mochitests, since hud.outputNode.childNodes is always empty even though the messages appear in the console and manually clicking on them gets you to the right page. hud.outputNode.querySelector doesn't seem to be doing anything for me either. I will get help from Tanvi on figuring out the mochitest, but I think this should be ready for feedback now.

Comment 29

[Ivan Alagenchev :ialagenchev]

Attachment: patch with a mochitest that tests the url used for the messages is correct

I added checks in the mochitest to validate the url that is opened when messages are clicked, so this obsoletes the previous patch.

Comment 30

[Garrett Robinson [:grobinson]]

Comment on attachment 772195 [details] [diff] [review]
patch with a mochitest that tests the url used for the messages is correct

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

Overall this looks pretty good. My main problems are with the web console integration.

First, there are some UX problems:

1. All of the text in the message is underlined, including the timestamp. This is because you added hud-clickable to the _entire_ node. It looks like a mistake.
2. My cursor does not turn into a pointer when I hover over the link. Therefore, there is no clear cue to the user that this is a clickable link (it just looks like it's underlined). Not sure why this is happening - if you look in browser/themes/shared/devtools/webconsole.inc.css, .hud-clickable sets cursor:pointer. There may be a conflict with some of the other styles.

More broadly, I'm not sure if it is the best UX to make the whole message a link to docs. I think it would be clearer to append a "Learn More" label at the end of the message that is a link to the docs. This should have a strong visual cue that it is a link (blue, cursor turns into a pointer, maybe underlined). This would be easy to do if you added a "Learn More" string to security.properties, so i10n will be trivial.

My second problem is with the way you target this particular aScriptError to have the documentation link added. Here's how I would recommend doing it:

1. Get rid of CATEGORY_INSECURE_PASSWORDS. The purpose of categoryForScriptError is to map the various types of (string) categories on nsIScriptErrors to the (constant) categories that represent panels in the Web Console. You still have aScriptError in reportPageError, so you don't even need to do the category-swap trick.
2. Write a function, addLearnMoreLink or similar, and call it after createMessageNode in reportPageError. Pass it the node and aScriptError.category (this is still "Insecure Password Fields" or whatever). In this function, you can branch based on the _scriptError's_ category, and manipulate the node to add the LearnMore link.

General sytle nit: put a space in between comment symbols and the start of the comment.

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warning.js
@@ +44,5 @@
> +
> +function endTest() {
> +  //restore MCB settings
> +  Services.prefs.setBoolPref("security.mixed_content.block_display_content", origBlockDisplay);
> +  Services.prefs.setBoolPref("security.mixed_content.block_active_content", origBlockActive);

I recommend using SpecialPowers.pushPrefEnv instead of manually setting and restoring the prefs (for all of these tests).

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js
@@ +62,5 @@
> +  let linkOpened = false;
> +  let oldOpenUILinkIn = window.openUILinkIn;
> +
> +  window.openUILinkIn = function(aLink) {
> +    if (aLink == "https://developer.mozilla.org/en-US/docs/Security/InsecurePasswords") {

This should be a constant defined at the top of the file so it's easier to change later if we move the info page.

::: browser/devtools/webconsole/test/test-bug-762593-insecure-passwords-about-blank-web-console-warning.html
@@ +8,5 @@
> +    http://creativecommons.org/publicdomain/zero/1.0/ -->
> +  </head>
> +  <body>
> +    <p>This insecure page is served with an about:blank iframe. A script then adds a
> +    passowrd field to it.</p>

What is the rationale behind this test?

::: browser/devtools/webconsole/webconsole.js
@@ +86,5 @@
>  const CATEGORY_WEBDEV = 3;
>  const CATEGORY_INPUT = 4;   // always on
>  const CATEGORY_OUTPUT = 5;  // always on
>  const CATEGORY_SECURITY = 6;
> +const CATEGORY_INSECURE_PASSWORDS = 7;

These categories map to panels in the Web Console (for the most part - I don't know about _WEBDEV, _INPUT, and _OUTPUT). Adding CATEGORY_INSECURE_PASSWORDS here is unnecessary and confusing. Notice how these are also used to index the MESSAGE_PREFERENCE_KEYS array below - this is not the right place for this.

@@ +1244,5 @@
>      }
>  
> +    let showPasswordInfo = false;
> +    if(aCategory == CATEGORY_INSECURE_PASSWORDS) {
> +      aCategory = CATEGORY_SECURITY;

This is a hack, and unnecessary.

@@ +1260,5 @@
> +        this.owner.openLink(INSECURE_PASSWORDS_LEARN_MORE);
> +        aEvent.preventDefault();
> +        aEvent.stopPropagation();
> +      }.bind(this));
> +    }

These is highly specific code and should not be interspered with the rest of the relatively generic code in reportPageError.

@@ +4434,5 @@
>          return CATEGORY_SECURITY;
>  
> +      case "Insecure Password Field":
> +        return CATEGORY_INSECURE_PASSWORDS;
> +

This should return CATEGORY_SECURITY.

::: dom/locales/en-US/chrome/security/security.properties
@@ +10,5 @@
>  # LOCALIZATION NOTE: Do not translate "X-Content-Security-Policy/Report-Only" or "Content-Security-Policy/Report-Only"
>  BothCSPHeadersPresent=This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-Content-Security-Policy/Report-Only header(s) will be ignored.
> +InsecurePasswordsPresentOnPage=Password fields present on an insecure (http://) page. This is a security risk that allows user's login credentials to be stolen.
> +InsecureFormActionPasswordsPresent=Password fields present in a form with an insecure (http://) form action.
> +InsecurePasswordsPresentOnIframe=Password fields present on an insecure (http://) iframe.

Make sure to update these to reflect the email thread.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +116,5 @@
> +      /*all web console messages are warnings for now
> +       * so I decided to set the flag here and save a bit of
> +       * the flag creation in the callers. It's easy to expose
> +       * this later if needed*/
> +      let flag = Ci.nsIScriptError.warningFlag ;

Nit: space before semicolon

@@ +177,5 @@
> +
> +      return isSafe;
> +    },
> +
> +    /*goes through the dom document and checks if there are insecure password

Nit: missing space, capitalize first letter of sentence. The first /* is commonly put on its own line (observe style guidelines in the rest of the file).

@@ +181,5 @@
> +    /*goes through the dom document and checks if there are insecure password
> +     * fields present on the document i.e. passwords inside forms with http
> +     * action, inside iframes with http src, or the page uri itself is
> +     * insecure. If insecure password fields are present, a log message is
> +     * sent to the web console to warn developers*/

Likewise, the last */ in a block comment should be on its own line.

@@ +194,5 @@
> +        isSafePage = this._checkIfURIisSecure(pageURI);
> +      }
> +      catch(err) {
> +        //it doesn't make sense to continue with the warning
> +        //message if something went wrong with validating the uri

Under what circumstances would validating the URI fail? I would prefer this not to fail silently, as that can make it very difficult to catch future regressions.

@@ +229,5 @@
> +            consoleService.logMessage(consoleMsg);
> +          }
> +        }
> +      }
> +    },

This is a significant amount of code! I think you should place it in its own .jsm, and just include and run checkForInsecurePasswords in LoginManagerContent.jsm.

Comment 31

[Ivan Alagenchev :ialagenchev]

Attachment: Feedback Patch

Implements all of the suggestions from comments.

Comment 32

[Ivan Alagenchev :ialagenchev]

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warning.js
@@ +44,5 @@
> +
> +function endTest() {
> +  //restore MCB settings
> +  Services.prefs.setBoolPref("security.mixed_content.block_display_content", origBlockDisplay);
> +  Services.prefs.setBoolPref("security.mixed_content.block_active_content", origBlockActive);

I recommend using SpecialPowers.pushPrefEnv instead of manually setting and restoring the prefs (for all of these tests).

Thanks for the suggestion. I ended up not needing this, since that was needed only when the page is secure and the iframe, or form action insecure. It was a left-over from a mixed case test, which isn't needed for the final patch. Comments have been added to explain rationale behind tests and other suggestions have been implemented. Thanks to you and everyone else for their suggestions.

Comment 33

[Garrett Robinson [:grobinson]]

Comment on attachment 776163 [details] [diff] [review]
Feedback Patch

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

Looks great! Mostly style nits, a few places where you could do a slight clean or refactor. Get this reviewed!

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warning.js
@@ +10,5 @@
> +  addTab(TEST_URI);
> +  browser.addEventListener("load", function onLoad(aEvent) {
> +    browser.removeEventListener(aEvent.type, onLoad, true);
> +    openConsole(null, function testInsecurePasswordErrorLogged (hud) {
> +

Unnecessary newline.

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js
@@ +14,5 @@
> +  addTab(TEST_URI);
> +  browser.addEventListener("load", function onLoad(aEvent) {
> +    browser.removeEventListener(aEvent.type, onLoad, true);
> +    openConsole(null, function testInsecurePasswordErrorLogged (hud) {
> +

Unnecessary newline.

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +76,5 @@
>  webConsoleMixedContentWarning=Mixed Content
>  
> +# LOCALIZATION NOTE (webConsoleInsecurePasswordsWarning): the insecure
> +# passwords warning tag displayed after the web console message about insecure
> +# passwords.The main message text warns about insecure passwords

Space after period.

@@ +81,5 @@
> +# i.e. passwords present in a non-https web page, iframe or inside forms with
> +# non-https actions. The purpose of the message tag is to form a clickable
> +# region that users can use to navigate to an MDN page where they can learn
> +# more about the security risks associated with insecure password fields.
> +webConsoleInsecurePasswordsWarning=Insecure Passwords

This is incorrect. This is the text of the label/tag, not the text of the warning. You should change the name of the variable to webConsoleInsecurePasswordsWarningMoreInfoLabel or similar. Also, might it be clearer if this text was "Learn More"?

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +17,5 @@
> +let l10n = new WebConsoleUtils.l10n(STRINGS_URI);
> +
> +this.InsecurePasswordUtils = {
> +
> +  _buildWebConsoleMessage : function (messageTag, windowId, category, domDoc) {

If domDoc is not used, it should be removed. Or you could pass in domDoc _instead of_ windowId and extract the innerWindowID in this function - up to you.

@@ +86,5 @@
> +  //checks whether the passed document has insecure iframes
> +  _hasInsecureIframes : function(domDoc) {
> +    var uri = Services.io.newURI(domDoc.location, null, null);
> +    if (domDoc.defaultView == domDoc.defaultView.parent) {
> +      //we are at the top, nothing to check here

Put a space between the comment marker and the start of the comment (here, and elsewhere).

@@ +128,5 @@
> +
> +    let  innerId = WebConsoleUtils.getInnerWindowId(domDoc.defaultView);
> +    let category = "Insecure Password Field";
> +
> +    if (innerId) {

Is this check necessary?

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +102,5 @@
>        // Only process things which might have HTML forms.
>        if (!(domDoc instanceof Ci.nsIDOMHTMLDocument))
>            return;
>  
> +      InsecurePasswordUtils.checkForInsecurePasswords(domDoc);

This might be a dumb question, but is this code sync or async? If it is sync, can we make it async?

Comment 34

[Ivan Alagenchev :ialagenchev]

(In reply to Garrett Robinson [:grobinson] from comment #33)
> Comment on attachment 776163 [details] [diff] [review]
> Feedback Patch
> 
> Review of attachment 776163 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great! Mostly style nits, a few places where you could do a slight
> clean or refactor. Get this reviewed!
> 
> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warn
> ing.js
> @@ +10,5 @@
> > +  addTab(TEST_URI);
> > +  browser.addEventListener("load", function onLoad(aEvent) {
> > +    browser.removeEventListener(aEvent.type, onLoad, true);
> > +    openConsole(null, function testInsecurePasswordErrorLogged (hud) {
> > +
> 
> Unnecessary newline.

For this one, I followed the style that was in this file: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser_webconsole_bug_837351_securityerrors.js#14

> 
> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js
> @@ +14,5 @@
> > +  addTab(TEST_URI);
> > +  browser.addEventListener("load", function onLoad(aEvent) {
> > +    browser.removeEventListener(aEvent.type, onLoad, true);
> > +    openConsole(null, function testInsecurePasswordErrorLogged (hud) {
> > +
> 
> Unnecessary newline.
> 
This one is the same as above. 

Could I possibly get a run down on some general rules about empty lines? I've had comments about new lines in my code before, even in places where I thought the new lines add readability and I am a bit confused now since I followed an existing example. I find that my newline preferences are different than what appears to be the general practice in this code base and I think that knowing some of the logic behind newlines would help me reduce style nits in the future. If there isn't any set logic other than individual preference then that's fine too.  

> ::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
> @@ +76,5 @@
> >  webConsoleMixedContentWarning=Mixed Content
> >  
> > +# LOCALIZATION NOTE (webConsoleInsecurePasswordsWarning): the insecure
> > +# passwords warning tag displayed after the web console message about insecure
> > +# passwords.The main message text warns about insecure passwords
> 
> Space after period.
> 
> @@ +81,5 @@
> > +# i.e. passwords present in a non-https web page, iframe or inside forms with
> > +# non-https actions. The purpose of the message tag is to form a clickable
> > +# region that users can use to navigate to an MDN page where they can learn
> > +# more about the security risks associated with insecure password fields.
> > +webConsoleInsecurePasswordsWarning=Insecure Passwords
> 
> This is incorrect. This is the text of the label/tag, not the text of the
> warning. You should change the name of the variable to
> webConsoleInsecurePasswordsWarningMoreInfoLabel or similar. Also, might it
> be clearer if this text was "Learn More"?
> 

Which part is incorrect? I feel like my comment describes things accurately. As far as the naming of the tag itself - I followed the style set forth by mixed content. The mixed content warning tag is webConsoleMixedContentWarning and so I named mine webConsoleInsecurePasswordsWarning to remain consistent with the rest of the code. About the text of the warning node itself - "Insecure Passwords" is consistent with "Mixed Content" for mixed content. That's what :tanvi and I agreed upon when we were working on coming up with the text for the tag, though to be fair, we didn't put much thought into it. I was just going for consistency with the mixed content warning and I suspect Tanvi's reasoning was the same. She can probably chime in with more background information on this one. 

> 
> ::: toolkit/components/passwordmgr/LoginManagerContent.jsm
> @@ +102,5 @@
> >        // Only process things which might have HTML forms.
> >        if (!(domDoc instanceof Ci.nsIDOMHTMLDocument))
> >            return;
> >  
> > +      InsecurePasswordUtils.checkForInsecurePasswords(domDoc);
> 
> This might be a dumb question, but is this code sync or async? If it is
> sync, can we make it async?

That is not a dumb question! It's sync. I think it would be fine async and I think that's a good idea. I'll look into this.

Comment 35

[Ivan Alagenchev :ialagenchev]

Another thought about the text of the warning nodes ... If we change the warning node text to "Learn More" as has been suggested by grobinson, we can move away from the practice of having a specific text for each type of message that we are logging to the console. This is going to make things more sane when going forward with any new types of security messages. I can also change the code for mixed content to use the same "Learn More" tag as the insecure passwords code as part of 875456. I can't tell if there was a specific reason why the "Mixed Content" text was chosen by reading the comments in 737873. I'll let :tanvi comment on that.

Comment 36

[Garrett Robinson [:grobinson]]

(In reply to Ivan Alagenchev :ialagenchev from comment #34)
> (In reply to Garrett Robinson [:grobinson] from comment #33)
> > Comment on attachment 776163 [details] [diff] [review]
> > Feedback Patch
> > 
> > Review of attachment 776163 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks great! Mostly style nits, a few places where you could do a slight
> > clean or refactor. Get this reviewed!
> > 
> > :::
> > browser/devtools/webconsole/test/
> > browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warn
> > ing.js
> > @@ +10,5 @@
> > > +  addTab(TEST_URI);
> > > +  browser.addEventListener("load", function onLoad(aEvent) {
> > > +    browser.removeEventListener(aEvent.type, onLoad, true);
> > > +    openConsole(null, function testInsecurePasswordErrorLogged (hud) {
> > > +
> > 
> > Unnecessary newline.
> 
> For this one, I followed the style that was in this file:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/
> test/browser_webconsole_bug_837351_securityerrors.js#14
> 

That's a mistake in that file and it's a mistake here. Exercise your judgement.

> > 
> > :::
> > browser/devtools/webconsole/test/
> > browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js
> > @@ +14,5 @@
> > > +  addTab(TEST_URI);
> > > +  browser.addEventListener("load", function onLoad(aEvent) {
> > > +    browser.removeEventListener(aEvent.type, onLoad, true);
> > > +    openConsole(null, function testInsecurePasswordErrorLogged (hud) {
> > > +
> > 
> > Unnecessary newline.
> > 
> This one is the same as above. 
> 
> Could I possibly get a run down on some general rules about empty lines?
> I've had comments about new lines in my code before, even in places where I
> thought the new lines add readability and I am a bit confused now since I
> followed an existing example. I find that my newline preferences are
> different than what appears to be the general practice in this code base and
> I think that knowing some of the logic behind newlines would help me reduce
> style nits in the future. If there isn't any set logic other than individual
> preference then that's fine too.  
> 

There are no general rules, rather common sense. In this example, IMO this newline is unnecessary because it's the first line in a new block, which is already marked by a block marker, "{", and a level of indentation. This is a nit though, and since coding style is somewhat subjective, you will always have reviewers bringing up nits. I typically implement all nit-related changes unless I have a really compelling reason not to.

> > ::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
> > @@ +76,5 @@
> > >  webConsoleMixedContentWarning=Mixed Content
> > >  
> > > +# LOCALIZATION NOTE (webConsoleInsecurePasswordsWarning): the insecure
> > > +# passwords warning tag displayed after the web console message about insecure
> > > +# passwords.The main message text warns about insecure passwords
> > 
> > Space after period.
> > 
> > @@ +81,5 @@
> > > +# i.e. passwords present in a non-https web page, iframe or inside forms with
> > > +# non-https actions. The purpose of the message tag is to form a clickable
> > > +# region that users can use to navigate to an MDN page where they can learn
> > > +# more about the security risks associated with insecure password fields.
> > > +webConsoleInsecurePasswordsWarning=Insecure Passwords
> > 
> > This is incorrect. This is the text of the label/tag, not the text of the
> > warning. You should change the name of the variable to
> > webConsoleInsecurePasswordsWarningMoreInfoLabel or similar. Also, might it
> > be clearer if this text was "Learn More"?
> > 
> 
> Which part is incorrect? I feel like my comment describes things accurately.

The comment is accurate. My comments always pertain to the last line in the hunk quoted from the patch, which in this case is the naming of the tag.

> As far as the naming of the tag itself - I followed the style set forth by
> mixed content. The mixed content warning tag is
> webConsoleMixedContentWarning and so I named mine
> webConsoleInsecurePasswordsWarning to remain consistent with the rest of the
> code. About the text of the warning node itself - "Insecure Passwords" is
> consistent with "Mixed Content" for mixed content. That's what :tanvi and I
> agreed upon when we were working on coming up with the text for the tag,
> though to be fair, we didn't put much thought into it. I was just going for
> consistency with the mixed content warning and I suspect Tanvi's reasoning
> was the same. She can probably chime in with more background information on
> this one. 
> 

Ok, but her use case was appending a label to a Network Panel message, whereas you are appending a label to what is specifically a warning message. The point here is that this text is not a warning, and so should not be labeled as one.

Comment 37

[Ivan Alagenchev :ialagenchev]

> >  
> > +      InsecurePasswordUtils.checkForInsecurePasswords(domDoc);
> 
> This might be a dumb question, but is this code sync or async? If it is
> sync, can we make it async?

I looked into this and also spoke with the jsapi team about what's the best way to go about doing it. Basically, there is no easy way to make this async. Javascript generally executes synchronously and there isn't a well defined mechanism for executing in parallel. The closest thing that comes to mind is to use settimeout of 0, but that would essentially queue up the javascript for later execution, which isn't something that I think is good for us. 
I was told that the DOM team might know of some tricks where the dom can be cloned and passed to the function that needs access to it to achieve some parallelism, but I think that would add a lot of complexity for something that does a few trivial things. 
I think we should move forward as it is and try to make this async only if we have specific performance implications. Thanks for the good idea though.

Comment 38

[Ivan Alagenchev :ialagenchev]

(In reply to Garrett Robinson [:grobinson] from comment #36)
> (In reply to Ivan Alagenchev :ialagenchev from comment #34)
> > (In reply to Garrett Robinson [:grobinson] from comment #33)
> > > Comment on attachment 776163 [details] [diff] [review]
> > > Feedback Patch
> > > 
> > > Review of attachment 776163 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Looks great! Mostly style nits, a few places where you could do a slight
> > > clean or refactor. Get this reviewed!
> > > 
> > > :::
> > > browser/devtools/webconsole/test/
> > > browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warn
> > > ing.js
> > > @@ +10,5 @@
> > > > +  addTab(TEST_URI);
> > > > +  browser.addEventListener("load", function onLoad(aEvent) {
> > > > +    browser.removeEventListener(aEvent.type, onLoad, true);
> > > > +    openConsole(null, function testInsecurePasswordErrorLogged (hud) {
> > > > +
> > > 
> > > Unnecessary newline.
> > 
> > For this one, I followed the style that was in this file:
> > http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/
> > test/browser_webconsole_bug_837351_securityerrors.js#14
> > 
> 
> That's a mistake in that file and it's a mistake here. Exercise your
> judgement.
> 
> > > 
> > > :::
> > > browser/devtools/webconsole/test/
> > > browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js
> > > @@ +14,5 @@
> > > > +  addTab(TEST_URI);
> > > > +  browser.addEventListener("load", function onLoad(aEvent) {
> > > > +    browser.removeEventListener(aEvent.type, onLoad, true);
> > > > +    openConsole(null, function testInsecurePasswordErrorLogged (hud) {
> > > > +
> > > 
> > > Unnecessary newline.
> > > 
> > This one is the same as above. 
> > 
> > Could I possibly get a run down on some general rules about empty lines?
> > I've had comments about new lines in my code before, even in places where I
> > thought the new lines add readability and I am a bit confused now since I
> > followed an existing example. I find that my newline preferences are
> > different than what appears to be the general practice in this code base and
> > I think that knowing some of the logic behind newlines would help me reduce
> > style nits in the future. If there isn't any set logic other than individual
> > preference then that's fine too.  
> > 
> 
> There are no general rules, rather common sense. In this example, IMO this
> newline is unnecessary because it's the first line in a new block, which is
> already marked by a block marker, "{", and a level of indentation. This is a
> nit though, and since coding style is somewhat subjective, you will always
> have reviewers bringing up nits. I typically implement all nit-related
> changes unless I have a really compelling reason not to.
> 
> > > ::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
> > > @@ +76,5 @@
> > > >  webConsoleMixedContentWarning=Mixed Content
> > > >  
> > > > +# LOCALIZATION NOTE (webConsoleInsecurePasswordsWarning): the insecure
> > > > +# passwords warning tag displayed after the web console message about insecure
> > > > +# passwords.The main message text warns about insecure passwords
> > > 
> > > Space after period.
> > > 
> > > @@ +81,5 @@
> > > > +# i.e. passwords present in a non-https web page, iframe or inside forms with
> > > > +# non-https actions. The purpose of the message tag is to form a clickable
> > > > +# region that users can use to navigate to an MDN page where they can learn
> > > > +# more about the security risks associated with insecure password fields.
> > > > +webConsoleInsecurePasswordsWarning=Insecure Passwords
> > > 
> > > This is incorrect. This is the text of the label/tag, not the text of the
> > > warning. You should change the name of the variable to
> > > webConsoleInsecurePasswordsWarningMoreInfoLabel or similar. Also, might it
> > > be clearer if this text was "Learn More"?
> > > 
> > 
> > Which part is incorrect? I feel like my comment describes things accurately.
> 
> The comment is accurate. My comments always pertain to the last line in the
> hunk quoted from the patch, which in this case is the naming of the tag.
> 
> > As far as the naming of the tag itself - I followed the style set forth by
> > mixed content. The mixed content warning tag is
> > webConsoleMixedContentWarning and so I named mine
> > webConsoleInsecurePasswordsWarning to remain consistent with the rest of the
> > code. About the text of the warning node itself - "Insecure Passwords" is
> > consistent with "Mixed Content" for mixed content. That's what :tanvi and I
> > agreed upon when we were working on coming up with the text for the tag,
> > though to be fair, we didn't put much thought into it. I was just going for
> > consistency with the mixed content warning and I suspect Tanvi's reasoning
> > was the same. She can probably chime in with more background information on
> > this one. 
> > 
> 
> Ok, but her use case was appending a label to a Network Panel message,
> whereas you are appending a label to what is specifically a warning message.
> The point here is that this text is not a warning, and so should not be
> labeled as one.

Thank you for the explanations. I'm going to make the changes now.

Comment 39

[Garrett Robinson [:grobinson]]

(In reply to Ivan Alagenchev :ialagenchev from comment #35)
> Another thought about the text of the warning nodes ... If we change the
> warning node text to "Learn More" as has been suggested by grobinson, we can
> move away from the practice of having a specific text for each type of
> message that we are logging to the console. This is going to make things
> more sane when going forward with any new types of security messages. I can
> also change the code for mixed content to use the same "Learn More" tag as
> the insecure passwords code as part of 875456. I can't tell if there was a
> specific reason why the "Mixed Content" text was chosen by reading the
> comments in 737873. I'll let :tanvi comment on that.

Again, that text was chosen in a different context, when these messages were being added on to Network Panel messages logging requests. Now that we have a dedicated security panel and are logging specific messages, IMO it doesn't make sense anymore. I would advocate for changing the message text to simply "Learn More" so we're as clear as possible.

Comment 40

[Tanvi Vyas[:tanvi]]

Just had a discussion on #security about this and concluded that we should go with "Learn More" for now.

Here were some other thoughts:
* Right now, the security panel wraps long messages to new lines.  We could replace this with elipses so that really long urls are not displayed in full.  The Learn More link should not get hidden in the elipses.  And if a user/developer wants to copy the entry, the whole url should get copied (without the elipses).
* We want to add Learn More links to most of the things we add to the Security Panel.  If this gets to be too repetitive at some point, we could change the "Learn More" text to an icon that represents "Learn More" (blue lightbulb?) and includes a tooltip on hover over.
* It would be useful to have an example that shows all the possible security violations that could come up in the Security panel.  We could use this for testing/demonstration purposes, in a blog post, or in a training.  We need a web server where we can add headers (since some of the security violations are header based).  Hence, people.mozilla.com won't work for this.

Comment 41

[Ivan Alagenchev :ialagenchev]

Attachment: TOOLKIT patch

Here is the try submission: https://tbpl.mozilla.org/?tree=Try&rev=c3325f660e4a

Comment 42

[Ivan Alagenchev :ialagenchev]

Attachment: DOM module patch

Comment 43

[Ivan Alagenchev :ialagenchev]

Attachment: DEVTOOLS module patch

Comment 44

[Mihai Sucan [:msucan]]

Comment on attachment 777555 [details] [diff] [review]
DEVTOOLS module patch

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

Patch looks good. Some cleanups needed. See comments below.

::: browser/devtools/webconsole/test/Makefile.in
@@ +142,5 @@
>  	browser_console_dead_objects.js \
>  	browser_console_iframe_messages.js \
>  	browser_console_variables_view_while_debugging_and_inspecting.js \
>  	head.js \
> +	browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js \

Please add these tests before head.js.

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warning.js
@@ +1,2 @@
> +/* Tests that errors about insecure passwords are logged
> + *  to the web console */

This comment should show after the PD license.

(the same applies for other files)

@@ +15,5 @@
> +        webconsole: hud,
> +        messages: [
> +          {
> +            name: "Insecure password error displayed successfully",
> +            text:INSECURE_PASSWORD_MSG,

nit: space after colon.

(this comment applies to multiple places in this patch)

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js
@@ +36,5 @@
> +            category: CATEGORY_SECURITY,
> +            severity: SEVERITY_WARNING
> +          },
> +        ],
> +      }).then( () => endTest(hud));

When I read this I was confused. I thought endTest would call finishTest...

@@ +61,5 @@
> +  ok(linkOpened, "Clicking the Insecure Passwords Warning node opens the desired page");
> +  window.openUILinkIn = oldOpenUILinkIn;
> +}
> +
> +function endTest(hud) {

Can you please merge endTest into testClickOpenNewTab? It would be less confusing.

::: browser/devtools/webconsole/test/test-bug-762593-insecure-passwords-about-blank-web-console-warning.html
@@ +1,1 @@
> +<!-- This test tests the scenario where a javascript adds password fields to

Please also add a PD license block to all new test files and test assets.

(it is also slightly odd to have comments before the doctype...)

::: browser/devtools/webconsole/webconsole.js
@@ +1247,5 @@
>                                        aScriptError.sourceName,
>                                        aScriptError.lineNumber, null, null,
>                                        aScriptError.timeStamp);
> +
> +    //select the body of the message node that is displayed in the console

nit: please start comments with uppercase, add a space after "//" and end them with a period.

(for consistency with the existing comments)

@@ +1250,5 @@
> +
> +    //select the body of the message node that is displayed in the console
> +    let msgBody = node.querySelector(".webconsole-msg-body");
> +    //add the more info link node to messages that belong to certain categories
> +    this.addMoreInfoLink(msgBody, aScriptError);

Please move the if condition and the call to addInsecure...() from addMoreInfoLink() and put it here. Makes it more clear what's happening.

Comment 45

[Ivan Alagenchev :ialagenchev]

> Please move the if condition and the call to addInsecure...() from
> addMoreInfoLink() and put it here. Makes it more clear what's happening.

:msucan and I talked about this off of bugzilla and I described the reasoning behind putting the if statement inside addMoreInfoLink. We are going to be adding new warnings nodes with different links based on the category pretty soon. As a matter of fact the first one will be as part of another bug I'm currently working on. To be able to contain the logic for the creation of all of the different "Learn More" nodes into an individual function and make it easy to create all those nodes, the branching logic will be in addMoreInfoLink. As a result of the discussion, we agreed that we are moving forward with the code as it is for this particular hunk.

Comment 46

[Ivan Alagenchev :ialagenchev]

Attachment: DEVTOOLS module patch

I've made the changes to address msucan's comments from the previous review

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 777554 [details] [diff] [review]
DOM module patch

r=me

Comment 48

[Mihai Sucan [:msucan]]

Comment on attachment 777923 [details] [diff] [review]
DEVTOOLS module patch

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

Looks good. Just some nits. r+ (no need to ask for re-review)

::: browser/devtools/webconsole/test/Makefile.in
@@ +141,5 @@
>  	browser_console_filters.js \
>  	browser_console_dead_objects.js \
>  	browser_console_iframe_messages.js \
>  	browser_console_variables_view_while_debugging_and_inspecting.js \
> +  browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js \

nit: there's indentation issue here. this file name is not aligned with the other names.

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warning.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +/*
> + * Tests that errors about insecure passwords are logged
> + *  to the web console

nit: double space.

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +/*
> + * Tests that errors about insecure passwords are logged
> + *  to the web console

nit: double space.

@@ +11,5 @@
> +const INSECURE_IFRAME_MSG = "Password fields present on an insecure (http://) iframe. This is a security risk that allows user login credentials to be stolen.";
> +const INSECURE_PASSWORDS_URI = "https://developer.mozilla.org/en-US/docs/Security/InsecurePasswords";
> +
> +function test() {
> +  requestLongerTimeout(5);

Is this needed?

::: browser/devtools/webconsole/webconsole.js
@@ +1441,5 @@
> +    }
> +  },
> +
> +  addInsecurePasswordsWarningNode:
> +    function WCF_addInsecurePasswordsWarningNode(aNode)

nit: please align |add...| with |function|.

Also, please add a comment that describes the function.

Comment 49

[Ivan Alagenchev :ialagenchev]

Thank you for your r+


> > +
> > +function test() {
> > +  requestLongerTimeout(5);
> 
> Is this needed?

No, it's a left-over from development. I'll take care of it and the other nits.

Comment 50

[Justin Dolske [:Dolske]]

Comment on attachment 777552 [details] [diff] [review]
TOOLKIT patch

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

(Apologies for the review delay.)

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +11,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "WebConsoleUtils",
> +                                  "resource://gre/modules/devtools/WebConsoleUtils.jsm");

You immediately use WebConsoleUtils, so might as well just Ci.import it.

@@ +26,5 @@
> +     */
> +
> +    let flag = Ci.nsIScriptError.warningFlag;
> +    let message = l10n.getStr(messageTag);
> +    let consoleMsg = Components.classes["@mozilla.org/scripterror;1"]

s/Components.classes/Cc/

@@ +64,5 @@
> +   */
> +  _checkIfURIisSecure : function(uri) {
> +    let isSafe = false;
> +    let netutil = Cc["@mozilla.org/network/util;1"].getService(Ci.nsINetUtil);
> +    URI_IS_LOCAL_RESOURCE = Ci.nsIProtocolHandler.URI_IS_LOCAL_RESOURCE;

This pattern (CAPS = foo.bar.CAPS) is duplicative and makes things harder to read. I assume you're doing it to make the netutil.URIChainHasFlags() lines not be so long?

I'd either just let them be long, or use a trick like:

  let ph = Ci.nsIProtocolHandler;
  if (netutil.URIChainHasFlags(uri, ph.URI_IS_LOCAL_RESOURCE) ||
      netutil.URIChainHasFlags(uri, ph.URI_DOES_NOT_RETURN_DATA) ...

@@ +77,5 @@
> +          netutil.URIChainHasFlags(uri, URI_INHERITS_SECURITY_CONTEXT) ||
> +            netutil.URIChainHasFlags(uri, URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT)) {
> +
> +      isSafe = true;
> +    }

TBH, this is overkill. Since it's just a console message, just a simple check for http or https would be adequate (ie, cover 99+% of likely cases). But here we are, so if this works and more precise, yay.

@@ +93,5 @@
> +    else if (!this._checkIfURIisSecure(uri)) {
> +      // We are insecure
> +      return true;
> +    }
> +    else {

Style nits -- |else| on same line as braces.

But you don't need them at all, due to early return...

  if (a) {
    return x;
  }
  if (b) {
    return y;
  }
  return z;

@@ +95,5 @@
> +      return true;
> +    }
> +    else {
> +      // I am secure, but check my parent
> +      return this._hasInsecureIframes(domDoc.defaultView.parent.document);

Why do we care if an http:// page includes an https:// iframe?

@@ +108,5 @@
> +   * insecure. If insecure password fields are present, a log message is
> +   * sent to the web console to warn developers
> +   */
> +  checkForInsecurePasswords : function (domDoc) {
> +    let pageURI = Services.io.newURI(domDoc.defaultView.top.document.location, null, null);

This should work:

  let pageURI = domDoc.defaultView.top.document.documentURIObject

@@ +117,5 @@
> +    }
> +    catch(err) {
> +      /*
> +       * netutil.URIChainHasFlags inside _checkIfURIisSecure
> +       * can throw an exception when used from javascript

It can? When? Sounds like a bug.

@@ +125,5 @@
> +       */
> +      return;
> +    }
> +    let  innerId = WebConsoleUtils.getInnerWindowId(domDoc.defaultView);
> +    let category = "Insecure Password Field";

Neither innerId nor category is useful in this scope -- I'd suggest just passing in docDoc, and letting buildWebConsoleMessage() have the hardcoded category and fetch the innerId.

Maybe move the Services.console.logMessage() call there too.

@@ +137,5 @@
> +        let consoleMsg = this._buildWebConsoleMessage(messageTag,
> +                                                      innerId, category);
> +        Services.console.logMessage(consoleMsg);
> +      }
> +

Return after logging a message? One message per document (or form, with DOMFormHasPassword) should be enough.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +102,5 @@
>        // Only process things which might have HTML forms.
>        if (!(domDoc instanceof Ci.nsIDOMHTMLDocument))
>            return;
>  
> +      InsecurePasswordUtils.checkForInsecurePasswords(domDoc);

This isn't a great place to put this check... At a minimum it should go farther up, since here it's unintentionally affected by the gEnabled check (disabling password manager would disable this check too).

A bit more of a problem is that onContentLoaded() will go away soon -- see bug 355063. A DOMFormHasPassword listener replaces it. Should be an easy change -- instead of having to trudge through the whole document looking for forms and password fields, you simply get an even when a password field is added to a form (including after the page has loaded -- bonus). See the dependencies in that bug.

Since this check doesn't actually involve password management, I'd suggest putting the call in a browser/base/content/content.js event listener for DOMFormHasPassword (as the patch in bug 355063 does).

Comment 51

[Ivan Alagenchev :ialagenchev]

>   let ph = Ci.nsIProtocolHandler;
>   if (netutil.URIChainHasFlags(uri, ph.URI_IS_LOCAL_RESOURCE) ||
>       netutil.URIChainHasFlags(uri, ph.URI_DOES_NOT_RETURN_DATA) ...
> 
> @@ +77,5 @@
> > +          netutil.URIChainHasFlags(uri, URI_INHERITS_SECURITY_CONTEXT) ||
> > +            netutil.URIChainHasFlags(uri, URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT)) {
> > +
> > +      isSafe = true;
> > +    }
> 
> TBH, this is overkill. Since it's just a console message, just a simple
> check for http or https would be adequate (ie, cover 99+% of likely cases).
> But here we are, so if this works and more precise, yay.
This is using the same logic that is used in the nsMixedContentBlocker to determine if we have mixed content. The code handles other types of schemes as well, like mailto,data,moz-safe-about etc. 

> 
> @@ +95,5 @@
> > +      return true;
> > +    }
> > +    else {
> > +      // I am secure, but check my parent
> > +      return this._hasInsecureIframes(domDoc.defaultView.parent.document);
> 
> Why do we care if an http:// page includes an https:// iframe?

This is checking to see if we have http:// iframes in https:// pages or other iframes. Is it unclear? Do I need to add a comment to make it clearer? 

> @@ +117,5 @@
> > +    }
> > +    catch(err) {
> > +      /*
> > +       * netutil.URIChainHasFlags inside _checkIfURIisSecure
> > +       * can throw an exception when used from javascript
> 
> It can? When? Sounds like a bug.
nsMixedContentBlocker has a NS_FAILED check around NS_URIChainHasFlags. When we make a call from .js and the call fails, we would get exceptions instead. The try/catch is there to account for the case when NS_FAILED would have returned true.
Feel free to ping me on irc, if I need to elaborate further. 

> 
> Return after logging a message? One message per document (or form, with
> DOMFormHasPassword) should be enough.

Actually not, we can have a password field inside a http://iframe, inside an https://iframe inside an http://page for example... and even have a form with http:// action too. We want to log all of those messages at once.

> 
> ::: toolkit/components/passwordmgr/LoginManagerContent.jsm
> @@ +102,5 @@
> >        // Only process things which might have HTML forms.
> >        if (!(domDoc instanceof Ci.nsIDOMHTMLDocument))
> >            return;
> >  
> > +      InsecurePasswordUtils.checkForInsecurePasswords(domDoc);
> 
> This isn't a great place to put this check... At a minimum it should go
> farther up, since here it's unintentionally affected by the gEnabled check
> (disabling password manager would disable this check too).
> 
> A bit more of a problem is that onContentLoaded() will go away soon -- see
> bug 355063. A DOMFormHasPassword listener replaces it. Should be an easy
> change -- instead of having to trudge through the whole document looking for
> forms and password fields, you simply get an even when a password field is
> added to a form (including after the page has loaded -- bonus). See the
> dependencies in that bug.
> 
> Since this check doesn't actually involve password management, I'd suggest
> putting the call in a browser/base/content/content.js event listener for
> DOMFormHasPassword (as the patch in bug 355063 does).

Great! Thanks a lot for this tip. I've made the changes (and fixed the nits too) and I agree this is much more elegant. 

Let me know how you feel about my responses to your other comments and I can submit the new patch for you to review.

Comment 52

[Ivan Alagenchev :ialagenchev]

Attachment: TOOLKIT_762593.patch

I decided to go ahead and upload the new patch for you. If we decide to do things differently based on your replies to the comments, then I will redo the patch.

Comment 53

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 781951 [details] [diff] [review]
TOOLKIT_762593.patch

>diff --git a/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm b/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm

>+  /*
>+   * Checks whether the passed uri is secure
>+   * Check Protocol Flags to determine if scheme is secure:
>+   * URI_DOES_NOT_RETURN_DATA - e.g.
>+   *   "mailto"
>+   * URI_IS_LOCAL_RESOURCE - e.g.
>+   *   "data",
>+   *   "resource",
>+   *   "moz-icon"
>+   * URI_INHERITS_SECURITY_CONTEXT - e.g.
>+   *   "javascript"
>+   * URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT - e.g.
>+   *   "https",
>+   *   "moz-safe-about"

This is a pretty strange mapping, and you don't define what you mean by "secure". I understand you're copying the code from nsMixedContentBlocker, but this could really use a comment as to why each of these protocol flags imply "secure" (they're mostly unrelated, so it's very strange to see them grouped this way). The examples are also more confusing than helpful, since there is overlap (e.g. data: URIs are also URI_INHERITS_SECURITY_CONTEXT). It would also be ideal to be able to share this code with nsMixedContentBlocker somehow, worth filing a followup bug at least.

>+  // Checks whether the passed document has insecure iframes

This doesn't seem to match the implementation. If the passed-in document's URI is "insecure" this method returns true, which has nothing to do with it having insecure iframes. Seems like you need to fix either the comment or the implementation.

>+  _hasInsecureIframes : function(domDoc) {
>+    var uri = Services.io.newURI(domDoc.location, null, null);

domDoc.documentURIObject

> nsMixedContentBlocker has a NS_FAILED check around NS_URIChainHasFlags. When
> we make a call from .js and the call fails, we would get exceptions instead.
> The try/catch is there to account for the case when NS_FAILED would have
> returned true.

That NS_FAILED check is to handle exceptional circumstances like OOM, there's no reason why this method would fail under normal circumstances. If those tests fail you'd very likely not be running script at all, so you don't need to put this in a try/catch.

Comment 54

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Can we add any part to share the result of appraisal by `InsecurePasswordUtils`?
I think it's useful if we share its result via callback, shared variable, observer message, or any styles.

Comment 55

[Ivan Alagenchev :ialagenchev]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #53)
> Comment on attachment 781951 [details] [diff] [review]
> TOOLKIT_762593.patch
> 
> >diff --git a/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm b/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
> 
> >+  /*
> >+   * Checks whether the passed uri is secure
> >+   * Check Protocol Flags to determine if scheme is secure:
> >+   * URI_DOES_NOT_RETURN_DATA - e.g.
> >+   *   "mailto"
> >+   * URI_IS_LOCAL_RESOURCE - e.g.
> >+   *   "data",
> >+   *   "resource",
> >+   *   "moz-icon"
> >+   * URI_INHERITS_SECURITY_CONTEXT - e.g.
> >+   *   "javascript"
> >+   * URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT - e.g.
> >+   *   "https",
> >+   *   "moz-safe-about"
> 
> This is a pretty strange mapping, and you don't define what you mean by
> "secure". I understand you're copying the code from nsMixedContentBlocker,
> but this could really use a comment as to why each of these protocol flags
> imply "secure" (they're mostly unrelated, so it's very strange to see them
> grouped this way). The examples are also more confusing than helpful, since
> there is overlap (e.g. data: URIs are also URI_INHERITS_SECURITY_CONTEXT).
> It would also be ideal to be able to share this code with
> nsMixedContentBlocker somehow, worth filing a followup bug at least.
I will work with people who are more familiar with the mixed content code to get a better understanding of why things are done this way in MCB and to be able to add a better comment to the code. 

> 
> >+  // Checks whether the passed document has insecure iframes
> 
> This doesn't seem to match the implementation. If the passed-in document's
> URI is "insecure" this method returns true, which has nothing to do with it
> having insecure iframes. Seems like you need to fix either the comment or
> the implementation.

This works the way it does, because the top level document is already checked at 
isSafePage = this._checkIfURIisSecure(pageURI); inside checkForInsecurePasswords. 
There pageURI was assigned domDoc.defaultView.top.document.documentURIObject, which is the top document URI. So the check for secure URI inside hasInsecureFrames will be called only on documents which aren't equal to the top document. This can happen only for "sub-documents", which happens only when we have iframes. That's also the reason why I have the first if statement where I check if hasInsecureIframes was called for the top level doc, aside from the fact that it terminates the recursion. 

Would changing the method name to "checkForInsecureNestedDocuments", or something similar make more sense?
 
> >+  _hasInsecureIframes : function(domDoc) {
> >+    var uri = Services.io.newURI(domDoc.location, null, null);
> 
> domDoc.documentURIObject
> 
> > nsMixedContentBlocker has a NS_FAILED check around NS_URIChainHasFlags. When
> > we make a call from .js and the call fails, we would get exceptions instead.
> > The try/catch is there to account for the case when NS_FAILED would have
> > returned true.
> 
> That NS_FAILED check is to handle exceptional circumstances like OOM,
> there's no reason why this method would fail under normal circumstances. If
> those tests fail you'd very likely not be running script at all, so you
> don't need to put this in a try/catch.

I see. Thanks for the explanation, I will remove the try/catch block. What's OOM?

Comment 56

[Ivan Alagenchev :ialagenchev]

Here is a link to the newly created bug to create a way to share the code between InsecurePasswordUtils and nsMixedContentBlocker: https://bugzilla.mozilla.org/show_bug.cgi?id=899099

Comment 57

[Ivan Alagenchev :ialagenchev]

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #54)
> Can we add any part to share the result of appraisal by
> `InsecurePasswordUtils`?
> I think it's useful if we share its result via callback, shared variable,
> observer message, or any styles.

I spent some time thinking about it and I think using an observer topic would be the best approach for this. Please file a separate bug, if you would like this feature.

Comment 58

[Ivan Alagenchev :ialagenchev]

Attachment: TOOLKIT_762593.patch

New patch that addresses Gavin's comments

Comment 59

[Tanvi Vyas[:tanvi]]

Comment on attachment 783198 [details] [diff] [review]
TOOLKIT_762593.patch

>From: Ivan Alagenchev <[e-mail]>
>TOOLKIT patch for 762593
>diff --git a/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm b/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
>new file mode 100644
>index 0000000..4ba8534
>--- /dev/null
>+++ b/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
>+  /*
>+   * Goes through the dom document and checks if there are insecure password
>+   * fields present on the document i.e. passwords inside forms with http
>+   * action, inside iframes with http src, or the page uri itself is
>+   * insecure. If insecure password fields are present, a log message is
>+   * sent to the web console to warn developers
>+   */
>+  checkForInsecurePasswords : function (domDoc) {
>+    let pageURI = domDoc.defaultView.top.document.documentURIObject;
>+    let isSafePage = this._checkIfURIisSecure(pageURI);
>+
>+    for(var i = 0; i < domDoc.forms.length; i++) {
>+      if (!isSafePage) {
>+        this._sendWebConsoleMessage("InsecurePasswordsPresentOnPage", domDoc);
>+      }
>+
>+      if (this._checkForInsecureNestedDocuments(domDoc)) {
>+        this._sendWebConsoleMessage("InsecurePasswordsPresentOnIframe", domDoc);
>+      }
>+
>+      if (domDoc.forms[i].action.match(/^http:\/\//)) {
>+        this._sendWebConsoleMessage("InsecureFormActionPasswordsPresent",
>+                                    domDoc);
>+      }

This will end up going through all the forms in the page, regardless of whether there is a password field in a specific form or not.  Spoke to Ivan about this and he's going to try and fix this by passing the event.target instead of event.target.ownerDocument to checkForInsecurePasswords.

Comment 60

[Justin Dolske [:Dolske]]

(In reply to Ivan Alagenchev :ialagenchev from comment #51)

> > > +      // I am secure, but check my parent
> > > +      return this._hasInsecureIframes(domDoc.defaultView.parent.document);
> > 
> > Why do we care if an http:// page includes an https:// iframe?
> 
> This is checking to see if we have http:// iframes in https:// pages or
> other iframes. Is it unclear? Do I need to add a comment to make it clearer? 

Either I'm asking the wrong question or you've misunderstood. IIRC, this code is running when the current domDoc has a password field, it's a secure document (https://), so the question was why we'd need to keep looking upwards. Let me see if this is still confusing in the new patch.


> > Return after logging a message? One message per document (or form, with
> > DOMFormHasPassword) should be enough.
> 
> Actually not, we can have a password field inside a http://iframe, inside an
> https://iframe inside an http://page for example... and even have a form
> with http:// action too. We want to log all of those messages at once.

I don't think that has to be a requirement, and if it lets us bail out early and avoid doing extra work it's worth it.

Comment 61

[Justin Dolske [:Dolske]]

(In reply to Tanvi Vyas [:tanvi] from comment #59)

> This will end up going through all the forms in the page, regardless of
> whether there is a password field in a specific form or not.  Spoke to Ivan
> about this and he's going to try and fix this by passing the event.target
> instead of event.target.ownerDocument to checkForInsecurePasswords.

Err, right. Part of the point of using that event is to minimize the work needed. You shouldn't need to look at any other form beyond the one you got the event for. If there are other forms with password fields, you'll get additional events for them.

Comment 62

[Justin Dolske [:Dolske]]

Comment on attachment 783198 [details] [diff] [review]
TOOLKIT_762593.patch

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

Per last comment.

Comment 63

[Ivan Alagenchev :ialagenchev]

(In reply to Justin Dolske [:Dolske] from comment #61)
> (In reply to Tanvi Vyas [:tanvi] from comment #59)
> 
> > This will end up going through all the forms in the page, regardless of
> > whether there is a password field in a specific form or not.  Spoke to Ivan
> > about this and he's going to try and fix this by passing the event.target
> > instead of event.target.ownerDocument to checkForInsecurePasswords.
> 
> Err, right. Part of the point of using that event is to minimize the work
> needed. You shouldn't need to look at any other form beyond the one you got
> the event for. If there are other forms with password fields, you'll get
> additional events for them.

Right, that logic was a left over from when I didn't use the event. I simply forgot to remove it.

Comment 64

[Ivan Alagenchev :ialagenchev]

(In reply to Justin Dolske [:Dolske] from comment #60)
> (In reply to Ivan Alagenchev :ialagenchev from comment #51)
> 
> > > > +      // I am secure, but check my parent
> > > > +      return this._hasInsecureIframes(domDoc.defaultView.parent.document);
> > > 
> > > Why do we care if an http:// page includes an https:// iframe?
> > 
> > This is checking to see if we have http:// iframes in https:// pages or
> > other iframes. Is it unclear? Do I need to add a comment to make it clearer? 
> 
> Either I'm asking the wrong question or you've misunderstood. IIRC, this
> code is running when the current domDoc has a password field, it's a secure
> document (https://), so the question was why we'd need to keep looking
> upwards. Let me see if this is still confusing in the new patch.
> 
The idea is that even if this particular document is secure, we could be inside an insecure document, which is still a security risk. That's why we need to check all documents up to the top most one to make sure that everyone on the chain is secure. 
Either, I am misunderstanding your questions, or there is something about the documents that you know and I don't.

> 
> > > Return after logging a message? One message per document (or form, with
> > > DOMFormHasPassword) should be enough.
> > 
> > Actually not, we can have a password field inside a http://iframe, inside an
> > https://iframe inside an http://page for example... and even have a form
> > with http:// action too. We want to log all of those messages at once.
> 
> I don't think that has to be a requirement, and if it lets us bail out early
> and avoid doing extra work it's worth it.
That's one thing that I think developers would appreciate. If I have multiple security issues with my website, I would like to see them all at once, rather than have to go through the process of see error->fix error->see another error->repeat. 
If bailing out early is more important here, than that makes sense; however from a user perspective, I would prefer to get more information at once rather than less.
Let me know what you think and thank you for your comments.

Comment 65

[Tanvi Vyas[:tanvi]]

(In reply to Justin Dolske [:Dolske] from comment #60)
> (In reply to Ivan Alagenchev :ialagenchev from comment #51)
> 
> > > > +      // I am secure, but check my parent
> > > > +      return this._hasInsecureIframes(domDoc.defaultView.parent.document);
> > > 
> > > Why do we care if an http:// page includes an https:// iframe?
> > 
> > This is checking to see if we have http:// iframes in https:// pages or
> > other iframes. Is it unclear? Do I need to add a comment to make it clearer? 
> 
> Either I'm asking the wrong question or you've misunderstood. IIRC, this
> code is running when the current domDoc has a password field, it's a secure
> document (https://), so the question was why we'd need to keep looking
> upwards. Let me see if this is still confusing in the new patch.
> 
> 

Suppose bank.com iframes their login page.  When you go to http://bank.com there is an iframe to https://bank.com/login.php.  https://bank.com/login.php contains a form that requests a username and password and posts them over SSL.

A MITM attacker could replace the https://bank.com/login.php frame with https://attacker.com/login.php because the main page is over HTTP, and hence subject to tampering.  The user doesn't have any idea that the frame is from attacker.com and not bank.com.  They enter their username and password and it is submitted securely to attacker.com's servers.  Note that the user doesn't get a Mixed Content Blocker message here because the main page is HTTP and it is framing an HTTPS page (and not vice versa). 

We hope to educate web developers by including a message in the webconsole if a password field is on an insecure page or embedded in an insecure page.  The "Learn More" link provided should outline these potential attack scenarios (or maybe link to them) so that developers understand the risk they open up to their users.

That is why we are checking up the chain of frame ancestors.

Comment 66

[Justin Dolske [:Dolske]]

(In reply to Tanvi Vyas [:tanvi] from comment #65)

> [...] That is why we are checking up the chain of frame ancestors.

Thanks. This would be worth (briefly) noting as a comment.


(In reply to Ivan Alagenchev :ialagenchev from comment #64)
> > I don't think that has to be a requirement, and if it lets us bail out early
> > and avoid doing extra work it's worth it.
> That's one thing that I think developers would appreciate. If I have
> multiple security issues with my website, I would like to see them all at
> once, rather than have to go through the process of see error->fix
> error->see another error->repeat. 

Sure. But if it's penalizing page load performance for the 99.999999% of users who are not the developers of the site, it's not worth the extra cost.

Properly driving this all via DOMFormHasPassword should make this moot.

Comment 67

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Ivan Alagenchev :ialagenchev from comment #57)
> (In reply to Tetsuharu OHZEKI [UTC+9] from comment #54)
> 
> I spent some time thinking about it and I think using an observer topic
> would be the best approach for this. Please file a separate bug, if you
> would like this feature.

OK! I'll file a bug.

Comment 68

[Ivan Alagenchev :ialagenchev]

Attachment: TOOLKIT_762593.patch

Changes in this patch: Removed the for loop and added more comments that describe the reason behind the check for nested insecure iframes, renamed the method checkForInsecureIframes to checkForInsecureNestedDocuments. We are still checking for the 3 types of errors that can be displayed for a password field - inside insecure iframe, doc and form at once as per an IRC discussion.

Comment 69

[Justin Dolske [:Dolske]]

Comment on attachment 783927 [details] [diff] [review]
TOOLKIT_762593.patch

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

Just some style nitpicks, otherwise fine. Please update and land, no need for another review.

::: browser/base/content/content.js
@@ +40,5 @@
>    addEventListener("DOMContentLoaded", function(event) {
>      LoginManagerContent.onContentLoaded(event);
> +});
> +addEventListener("DOMFormHasPassword", function(event) {
> +  InsecurePasswordUtils.checkForInsecurePasswords(event.target);

Your indentation is off, here.

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +10,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/devtools/WebConsoleUtils.jsm");
> +
> +var STRINGS_URI = "chrome://global/locale/security/security.properties";

s/var/let/, for consistency. Though I suppose |const| would be suitable too.

@@ +71,5 @@
> +
> +    if (netutil.URIChainHasFlags(uri, ph.URI_IS_LOCAL_RESOURCE) ||
> +        netutil.URIChainHasFlags(uri, ph.URI_DOES_NOT_RETURN_DATA) ||
> +          netutil.URIChainHasFlags(uri, ph.URI_INHERITS_SECURITY_CONTEXT) ||
> +            netutil.URIChainHasFlags(uri, ph.URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT)) {

Consider removing the staggered indent -- makes it easier to see that the 4 lines are the same except for last arg when it all lines up.

@@ +83,5 @@
> +  // Checks whether the passed nested document is insecure
> +  // or is inside an insecure parent document.
> +  //
> +  // We check the chain of frame ancestors all the way until the top document
> +  // because MITM attackers could replace https://iframes if they are nested inside

"https:// iframes" (missing space)

@@ +88,5 @@
> +  // http:// documents with their own content, thus creating a security risk
> +  // and potentially stealing user data. Under such scenario, a user might not
> +  // get a Mixed Content Blocker message, if the main document is served over HTTP
> +  // and framing an HTTPS page as it would under the reverse scenario (http
> +  // inside https).

You're using line-comments here, but block-comments for other multiline descriptions.

@@ +111,5 @@
> +   * or on insecure web pages. If insecure password fields are present,
> +   * a log message is sent to the web console to warn developers.
> +   */
> +  checkForInsecurePasswords : function (aForm) {
> +    var domDoc = aForm.ownerDocument;

s/var/let/

Comment 70

[Ivan Alagenchev :ialagenchev]

(In reply to Justin Dolske [:Dolske] from comment #69)
> Comment on attachment 783927 [details] [diff] [review]
> TOOLKIT_762593.patch
> 
> Review of attachment 783927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some style nitpicks, otherwise fine. Please update and land, no need
> for another review.
> 
> ::: browser/base/content/content.js
> @@ +40,5 @@
> >    addEventListener("DOMContentLoaded", function(event) {
> >      LoginManagerContent.onContentLoaded(event);
> > +});
> > +addEventListener("DOMFormHasPassword", function(event) {
> > +  InsecurePasswordUtils.checkForInsecurePasswords(event.target);
> 
> Your indentation is off, here.
> 
> ::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
> @@ +10,5 @@
> > +
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/devtools/WebConsoleUtils.jsm");
> > +
> > +var STRINGS_URI = "chrome://global/locale/security/security.properties";
> 
> s/var/let/, for consistency. Though I suppose |const| would be suitable too.
> 
> @@ +71,5 @@
> > +
> > +    if (netutil.URIChainHasFlags(uri, ph.URI_IS_LOCAL_RESOURCE) ||
> > +        netutil.URIChainHasFlags(uri, ph.URI_DOES_NOT_RETURN_DATA) ||
> > +          netutil.URIChainHasFlags(uri, ph.URI_INHERITS_SECURITY_CONTEXT) ||
> > +            netutil.URIChainHasFlags(uri, ph.URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT)) {
> 
> Consider removing the staggered indent -- makes it easier to see that the 4
> lines are the same except for last arg when it all lines up.
> 
> @@ +83,5 @@
> > +  // Checks whether the passed nested document is insecure
> > +  // or is inside an insecure parent document.
> > +  //
> > +  // We check the chain of frame ancestors all the way until the top document
> > +  // because MITM attackers could replace https://iframes if they are nested inside
> 
> "https:// iframes" (missing space)
> 
> @@ +88,5 @@
> > +  // http:// documents with their own content, thus creating a security risk
> > +  // and potentially stealing user data. Under such scenario, a user might not
> > +  // get a Mixed Content Blocker message, if the main document is served over HTTP
> > +  // and framing an HTTPS page as it would under the reverse scenario (http
> > +  // inside https).
> 
> You're using line-comments here, but block-comments for other multiline
> descriptions.
> 
> @@ +111,5 @@
> > +   * or on insecure web pages. If insecure password fields are present,
> > +   * a log message is sent to the web console to warn developers.
> > +   */
> > +  checkForInsecurePasswords : function (aForm) {
> > +    var domDoc = aForm.ownerDocument;
> 
> s/var/let/

Thank you for your review. I will fix the the nits.

Comment 71

[Ivan Alagenchev :ialagenchev]

Attachment: 762593_DOM.patch

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=d7bf8fcff1c0

Comment 72

[Ivan Alagenchev :ialagenchev]

Attachment: 762593_DEVTOOLS.patch

Comment 73

[Ivan Alagenchev :ialagenchev]

Attachment: 762593_TOOLKIT.patch

Comment 74

[Tanvi Vyas[:tanvi]]

Ivan, can you carry over the r+'s in bugzilla and update the patch commit messages to indicate who reviewed the patch. (Format: Bug 762593 - details. r=name_of_reviewer).

Also, you can set the keyword 'checkin-needed' instead of flagging me for check in.

Comment 75

[Ivan Alagenchev :ialagenchev]

Attachment: 762593_DOM.patch

r=bz

Comment 76

[Ivan Alagenchev :ialagenchev]

Attachment: 762593_DEVTOOLS.patch

r=msucan

Comment 77

[Ivan Alagenchev :ialagenchev]

Attachment: 762593_TOOLKIT.patch

r=dolske

Comment 78

[Tanvi Vyas[:tanvi]]

Checked into inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bde98e3a7194
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2c9adef1e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbba341b5faf

Comment 79

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/becab2ea9794

https://tbpl.mozilla.org/php/getParsedLog.php?id=26175493&tree=Mozilla-Inbound

Comment 80

[Ivan Alagenchev :ialagenchev]

Attachment: 762593_DEVTOOLS.patch

uploading a new devtools patch that fixes a minor merge conflict that arose after the back out

Comment 81

[Ivan Alagenchev :ialagenchev]

Attachment: 762593_TOOLKIT.patch

Unfortunately for me, the patch for bug 877262 landed at the perfect timing between my tblp run and the checkin. This caused the breakage reported by RyanVM. 
This patch fixes the cause for the breakage by using a different way for including WebConsoleUtils inside InsecurePasswordUtils. I used the patch in bug 877262 as an example and the relevant changes are highlighted here: http://pastebin.mozilla.org/2790198
Mihai, could you please comment if I am doing this right, before I flag dolske for re-review?

Comment 82

[Dave Camp (:dcamp)]

Comment on attachment 786042 [details] [diff] [review]
762593_TOOLKIT.patch

Shouldn't need to re-ping dolske for review.

Comment 83

[Ivan Alagenchev :ialagenchev]

(In reply to Dave Camp (:dcamp) from comment #82)
> Comment on attachment 786042 [details] [diff] [review]
> 762593_TOOLKIT.patch
> 
> Shouldn't need to re-ping dolske for review.

Thank you sir! If this gives me the green light on try, I'll flag it for checkin again :-)

Comment 84

[Ivan Alagenchev :ialagenchev]

New tbpl submission: https://tbpl.mozilla.org/?tree=Try&rev=47c07e77552f

Comment 85

[Mihai Sucan [:msucan]]

Thanks Dave for the quick r+ on Ivan's patch.

Comment 86

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a46b44af697f
https://hg.mozilla.org/integration/fx-team/rev/2b03e95df6d3
https://hg.mozilla.org/integration/fx-team/rev/e173f137b3cc

Comment 87

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a46b44af697f
https://hg.mozilla.org/mozilla-central/rev/2b03e95df6d3
https://hg.mozilla.org/mozilla-central/rev/e173f137b3cc

Comment 88

[Ian Melven :imelven]

Nice work landing this, Ivan !

Comment 89

[Ivan Alagenchev :ialagenchev]

(In reply to Ian Melven :imelven from comment #88)
> Nice work landing this, Ivan !

Thanks :-)

Comment 90

[Dionysis Zindros]

This should not be limited to the developer console. Moxie Marlinspike has illustrated since 4 years now [1] important security flaws in typing and/or submitting passwords via HTTP instead of HTTPS. As many websites still don't use HSTS, users must be informed of the potential threat that typing their password in an HTTP form entails.

Let's modify the Firefox URL bar to show a broken lock icon when password fields are present in non-HTTPS forms.

Should we open another bug for that?

[1] http://vimeo.com/50018478

Comment 91

[Ivan Alagenchev :ialagenchev]

(In reply to Dionysis Zindros from comment #90)
> This should not be limited to the developer console. Moxie Marlinspike has
> illustrated since 4 years now [1] important security flaws in typing and/or
> submitting passwords via HTTP instead of HTTPS. As many websites still don't
> use HSTS, users must be informed of the potential threat that typing their
> password in an HTTP form entails.
> 
> Let's modify the Firefox URL bar to show a broken lock icon when password
> fields are present in non-HTTPS forms.
> 
> Should we open another bug for that?
> 
> [1] http://vimeo.com/50018478

I agree with you. I think Mozilla should make similar changes as to what you suggest to the UI. While that happens, I'm adding support to expose this to add-ons: https://bugzilla.mozilla.org/show_bug.cgi?id=899983
and I'm working on an add-on myself that will do precisely that: https://github.com/alagenchev/ArktikFoxAddOn

Comment 92

[Ivan Alagenchev :ialagenchev]

I didn't find an existing bug, so I opened a new one: https://bugzilla.mozilla.org/show_bug.cgi?id=947471

Comment 93

[Dionysis Zindros]

Filing a bug for chromium to implement this:

https://code.google.com/p/chromium/issues/detail?id=327032

Comment 94

[sdf]

CodeIgniter Framework is an amazing PHP framework with an exceptionally little impression, worked for designers who need a straightforward and exquisite toolbox to make full-included web applications. CodeIgniter was made by EllisLab and is currently a venture of the British Columbia Institute of Technology. It is a PHP MVC framework for creating applications quickly. CodeIgniter gives out of the crate libraries for associating with the database and performing different tasks. Like sending messages, transferring documents, overseeing meetings, and so on.
for more info visit https://thedigizones.com/blog/codeigniter-amazing-framework/