1230462 - Clarify actual origin in prompt for cross-origin subresource HTTP authentication

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dragana Damjanovic [:dragana]]

This is connected to the bug 647010.

It is about subresources asking for authentication.
They can be cross-origin and not cross-origin.
We tried disabling the authentication prompting for cross-origin subresources but there were a lot of complains.

So we will need a new string that better shows that the resource asking for authentication is a subresource and another one that the resource is a cross-origin subresource.

Comment 1

[Tanvi Vyas[:tanvi]]

Dragana, can you point me to the current strings in the codebase?

Comment 2

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #1)
> Dragana, can you point me to the current strings in the codebase?

Needinfo'ing Dragana for this.

And adding Philipp since Aislinn is out.

Comment 3

[Dragana Damjanovic [:dragana]]

http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/commonDialogs.properties

for EnterLoginForRealm

Comment 4

[Tanvi Vyas[:tanvi]]

The existing dialog says:
EnterLoginForRealm=A username and password are being requested by %2$S. The site says: "%1$S"

Where %2$S is the name of the site requesting authentication (it could be a top level site or an iframe).  And %1$S is a message from the site about logging in.

Here are screenshots of what this looks like:
https://people.mozilla.org/~tvyas/Basic-Auth-Prompt.png
https://people.mozilla.org/~tvyas/Framed-Basic-Auth-Prompt.png

Notice the two examples look very similar.  In the second example, the site requesting credentials is different than the domain the url bar.  This can be confusing to users, since they coudl get tricked into entering credentials for the top level site into an evil cross domain iframe.

We need to redesign this dialog so that:
1) It is clear which website is requesting credentials.
* If the website requesting credentials is the same domain as the top level site, then only one domain needs to appear in the dialog.
* If the website requesting credentials is a different domain that the top level site, we need to highlight that (perhaps by making the website name bold or in it's own separate line).

2) The second sentence in the existing UI is in its own separate paragraph and separate from the domain name.  By putting all the text into one paragraph, users may skip reading the whole message.  We want users to at least see the domain name.  Note that Chrome doesn't show the second sentence with the message from the site; they leave it out completely.  Safari does show the message.

Test pages:
https://www.httpwatch.com/httpgallery/authentication/authenticatedimage/default.aspx
https://people.mozilla.org/~tvyas/test-basic-auth2.html
https://www.httpwatch.com/httpgallery/authentication/ (Scroll down and click "Display Image")
https://people.mozilla.org/~tvyas/test-basic-auth.html (Scroll down and click "Display Image")

Philipp, do you have time to mock something up?  I'm happy to provide more context/details if needed.

Comment 5

[Tanvi Vyas[:tanvi]]

We could also start small and just change the string for now.  Here is a proposal.

1) Change the first sentence to:
"https://site.com is requesting a username and password."

2) If frame requesting auth (%2$S) is not same domain as the top level page, append the following string to the first sentence:
"Note this is different domain than the main webpage you are visiting."
(We shouldn't put in the domain name of the top level page because that will only add to confusion.  I don't want the user to read the top level domain name in the message and ignore the other words).

3) Make the second sentence (the site's message) its own string with a separate paragraph

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Tanvi Vyas [:tanvi] from comment #5)
> We could also start small and just change the string for now.  Here is a
> proposal.
> 
> 1) Change the first sentence to:
> "https://site.com is requesting a username and password."
> 
> 2) If frame requesting auth (%2$S) is not same domain as the top level page,
> append the following string to the first sentence:
> "Note this is different domain than the main webpage you are visiting."
> (We shouldn't put in the domain name of the top level page because that will
> only add to confusion.  I don't want the user to read the top level domain
> name in the message and ignore the other words).
> 
> 3) Make the second sentence (the site's message) its own string with a
> separate paragraph

Good idea, I think that's a good start! Separating the message into paragraphs will do a lot for the readability of this dialog.
There's a lot we could do to further improve this prompt in the future (for example not make it block out other tabs).

Comment 7

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #5)
> We could also start small and just change the string for now.  Here is a
> proposal.
> 
> 1) Change the first sentence to:
> "https://site.com is requesting a username and password."
> 
> 2) If frame requesting auth (%2$S) is not same domain as the top level page,
> append the following string to the first sentence:
> "Note this is different domain than the main webpage you are visiting."
> (We shouldn't put in the domain name of the top level page because that will
> only add to confusion.  I don't want the user to read the top level domain
> name in the message and ignore the other words).
> 
> 3) Make the second sentence (the site's message) its own string with a
> separate paragraph

Dragana, do you want to take this on?

Comment 8

[Dragana Damjanovic [:dragana]]

taking it.

Comment 9

[Dragana Damjanovic [:dragana]]

Sorry for a delay.

I have a patch just wanted to check one thing:

Current code uses
NS_FAILED(loadingPrincipal->CheckMayLoad(mURI, false, false))

to check if it is the same origin.
I am not sure if it is a correct way?

Comment 10

[Tanvi Vyas[:tanvi]]

(In reply to Dragana Damjanovic [:dragana] from comment #9)
> Sorry for a delay.
> 
> I have a patch just wanted to check one thing:
> 
> Current code uses
> NS_FAILED(loadingPrincipal->CheckMayLoad(mURI, false, false))
> 
> to check if it is the same origin.
> I am not sure if it is a correct way?

Dragana and I discussed this over irc.  CheckMayLoad isn't the right check here, and I advised her to look at Subsumes() instead.  Dragana, you will have to figure out which principal to call Subsumes on and which principal to pass in as aOther.  As long as they are the same tld, it doesn't matter if top level or the frame is the subdomain.  i.e. sub.a.com top level with a.com frame should be allowed.  And a.com top level with sub.a.com shoudl be allowed.

Comment 11

[Dragana Damjanovic [:dragana]]

Attachment: bug_1230462_v1.patch

Comment 12

[Tanvi Vyas[:tanvi]]

Comment on attachment 8728914 [details] [diff] [review]
bug_1230462_v1.patch

diff --git a/netwerk/base/nsIAuthInformation.idl b/netwerk/base/nsIAuthInformation.idl
--- a/netwerk/base/nsIAuthInformation.idl
+++ b/netwerk/base/nsIAuthInformation.idl
@@ -8,17 +8,17 @@
  * A object that hold authentication information. The caller of
  * nsIAuthPrompt2::promptUsernameAndPassword or
  * nsIAuthPrompt2::promptPasswordAsync provides an object implementing this
  * interface; the prompt implementation can then read the values here to prefill
  * the dialog. After the user entered the authentication information, it should
  * set the attributes of this object to indicate to the caller what was entered
  * by the user.
  */
-[scriptable, uuid(0d73639c-2a92-4518-9f92-28f71fea5f20)]
+[scriptable, uuid(50461064-2fcf-4cbc-b0ff-e8aa05f8abb5)]

You no longer need to rev the uuid.

 bool
-nsHttpChannelAuthProvider::BlockPrompt()
+nsHttpChannelAuthProvider::BlockPrompt(bool top, bool crossOrigin)

Why move some of the BlockPrompt() code to PromptForIdentity()?



diff --git a/toolkit/components/prompts/src/nsPrompter.js b/toolkit/components/prompts/src/nsPrompter.js
--- a/toolkit/components/prompts/src/nsPrompter.js
+++ b/toolkit/components/prompts/src/nsPrompter.js
+        } else {
+            if (!isCrossOrg) {
+                text = PromptUtils.getLocalizedString("EnterUserPasswordFor",
+                                                      [displayHost]);
+            } else {
+                text = PromptUtils.getLocalizedString("EnterUserPasswordForWithCrossOrigin",
+                                                      [displayHost]);
+            }
+        }

What conditions cause us to get into this else block?  What is this for?


-EnterLoginForRealm=A username and password are being requested by %2$S. The site says: "%1$S"
+EnterLoginForRealm=%2$S is requesting a username and password.\n\nThe site says: "%1$S"
+EnterLoginForRealmWithCrossOrigin=%2$S is requesting a username and password.\n\nNote this is a different domain than the main webpage you are visiting.\n\nThe site says: "%1$S"
Remove the \n\n before "Note" and replace it with a space.  The Note should be the second sentence in the first paragraph.

 EnterLoginForProxy=The proxy %2$S is requesting a username and password. The site says: "%1$S"
-EnterUserPasswordFor=Enter username and password for %1$S
+EnterUserPasswordFor=%1$s is requesting a username and password.
+EnterUserPasswordForWithCrossOrigin=%1$s is requesting a username and password.\n\nNote this is a different domain than the main webpage you are visiting.
Same here.

I will do a closer review once questions above are answered.  Thanks!

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8728914 [details] [diff] [review]
bug_1230462_v1.patch

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

::: netwerk/base/nsIAuthInformation.idl
@@ +12,5 @@
>   * the dialog. After the user entered the authentication information, it should
>   * set the attributes of this object to indicate to the caller what was entered
>   * by the user.
>   */
> +[scriptable, uuid(50461064-2fcf-4cbc-b0ff-e8aa05f8abb5)]

no longer needed..

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +1002,5 @@
> +        }
> +
> +        if (!topDoc) {
> +            nsCOMPtr<nsIPrincipal> principal;
> +            // If this is not the top window, we need to find the top window.

If this is not a top window

@@ +1008,5 @@
> +            if (loadInfo->GetParentOuterWindowID() !=
> +                loadInfo->GetOuterWindowID()) {
> +                nsCOMPtr<nsPIDOMWindowOuter> window =
> +                    nsGlobalWindow::GetOuterWindowWithId(loadInfo->GetParentOuterWindowID())->GetTop();
> +                nsCOMPtr<nsIDocument> doc = window->GetDoc();

assert or check non-null on window.  I think it can be null...   also, how does this work in e10s and you are on the parent process?  I somehow think this is not going to work.

@@ +1019,5 @@
> +
> +            if (principal) {
> +                nsCOMPtr<nsIURI> principalURI;
> +                principal->GetURI(getter_AddRefs(principalURI));
> +                if (!NS_SecurityCompareURIs(principalURI, mURI, true)) {

so, no subsumes?  actually, I don't think you could ever use it, since you are loading something you only have a URL in hands for and a principal is built later (AFAIU).  So, this seems correct to me, assuming you have the right principal.

@@ +1037,5 @@
> +      } else {
> +          Telemetry::Accumulate(Telemetry::HTTP_AUTH_DIALOG_STATS,
> +              HTTP_AUTH_DIALOG_CROSS_ORIGIN_SUBRESOURCE);
> +      }
> +    }

I kinda don't follow why this has been moved here.  It was nice to hide it inside its own method.  You can also have two methods or let BlockPrompt return whether this is a cross origin subresource.

::: toolkit/components/prompts/src/nsPrompter.js
@@ +254,5 @@
>  
>      makeAuthMessage : function (channel, authInfo) {
>          let isProxy    = (authInfo.flags & Ci.nsIAuthInformation.AUTH_PROXY);
>          let isPassOnly = (authInfo.flags & Ci.nsIAuthInformation.ONLY_PASSWORD);
> +        let isCrossOrg = (authInfo.flags &

isCrossOrig

::: toolkit/locales/en-US/chrome/global/commonDialogs.properties
@@ +26,5 @@
>  # take advantage of sentence structure in order to mislead the user (see
>  # bug 244273). %1 should be integrated into the translated sentences as
>  # little as possible. %2 is the url of the site being accessed.
> +EnterLoginForRealm=%2$S is requesting a username and password.\n\nThe site says: "%1$S"
> +EnterLoginForRealmWithCrossOrigin=%2$S is requesting a username and password.\n\nNote this is a different domain than the main webpage you are visiting.\n\nThe site says: "%1$S"

WARNING: the password will not be sent to the web page you are currently visiting!

and maybe also:

Be careful, this can be an attempt to _*/steel your password/*_!  

...or something like this...

Comment 14

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> Comment on attachment 8728914 [details] [diff] [review]
> bug_1230462_v1.patch
> 
> Review of attachment 8728914 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +1008,5 @@
> > +            if (loadInfo->GetParentOuterWindowID() !=
> > +                loadInfo->GetOuterWindowID()) {
> > +                nsCOMPtr<nsPIDOMWindowOuter> window =
> > +                    nsGlobalWindow::GetOuterWindowWithId(loadInfo->GetParentOuterWindowID())->GetTop();
> > +                nsCOMPtr<nsIDocument> doc = window->GetDoc();
> 
> assert or check non-null on window.  I think it can be null...   also, how
> does this work in e10s and you are on the parent process?  I somehow think
> this is not going to work.
> 

I have tested it on e10s and it works :)


> @@ +1019,5 @@
> > +
> > +            if (principal) {
> > +                nsCOMPtr<nsIURI> principalURI;
> > +                principal->GetURI(getter_AddRefs(principalURI));
> > +                if (!NS_SecurityCompareURIs(principalURI, mURI, true)) {
> 
> so, no subsumes?  actually, I don't think you could ever use it, since you
> are loading something you only have a URL in hands for and a principal is
> built later (AFAIU).  So, this seems correct to me, assuming you have the
> right principal.
> 

Yes subsumes does not work here - we only have uri but not a principal, I figured out that why writing this patch.
I did test it with couple of options topdoc, iframe, iframe inside iframe... 

> @@ +1037,5 @@
> > +      } else {
> > +          Telemetry::Accumulate(Telemetry::HTTP_AUTH_DIALOG_STATS,
> > +              HTTP_AUTH_DIALOG_CROSS_ORIGIN_SUBRESOURCE);
> > +      }
> > +    }
> 
> I kinda don't follow why this has been moved here.  It was nice to hide it
> inside its own method.  You can also have two methods or let BlockPrompt
> return whether this is a cross origin subresource.
> 

I moved it because i need 3 return values: block - not block, is topDoc, is crossOrigin and I did not want to pass references. 
I will rework this.

> 
> ::: toolkit/locales/en-US/chrome/global/commonDialogs.properties
> @@ +26,5 @@
> >  # take advantage of sentence structure in order to mislead the user (see
> >  # bug 244273). %1 should be integrated into the translated sentences as
> >  # little as possible. %2 is the url of the site being accessed.
> > +EnterLoginForRealm=%2$S is requesting a username and password.\n\nThe site says: "%1$S"
> > +EnterLoginForRealmWithCrossOrigin=%2$S is requesting a username and password.\n\nNote this is a different domain than the main webpage you are visiting.\n\nThe site says: "%1$S"
> 
> WARNING: the password will not be sent to the web page you are currently
> visiting!
> 
> and maybe also:
> 
> Be careful, this can be an attempt to _*/steel your password/*_!  
> 
> ...or something like this...

I will let Philipp decide on this.

Comment 15

[Dragana Damjanovic [:dragana]]

I double check if e10s was on, and it was not (I was testing something and forgot to turn it back on).

So this does not work with e10s as Honza suggested :(

I will make a solution using LoadInfo and storing cross origin (between top window uri and loadingPrincipal uri... not sure is the best solution)
Tanvi, Honza, do you know if there is a way to get topDoc uri in parent process, probably not :) ?

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Dragana Damjanovic [:dragana] from comment #15)
> So this does not work with e10s as Honza suggested :(

you know, for few secs I was hoping this has been fixed when you claimed this worked for you :)

having a mirror of docshell/loadgroup structures was something I was asking for from the very start of the e10s effort.  I no longer track any changes in that area and last time I checked there was no way to get this info on parent.  however, ask around (boris, jdm, bkelly), they may know?

Comment 17

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> (In reply to Dragana Damjanovic [:dragana] from comment #15)
> > So this does not work with e10s as Honza suggested :(
> 
> you know, for few secs I was hoping this has been fixed when you claimed
> this worked for you :)
> 
> having a mirror of docshell/loadgroup structures was something I was asking
> for from the very start of the e10s effort.  I no longer track any changes
> in that area and last time I checked there was no way to get this info on
> parent.  however, ask around (boris, jdm, bkelly), they may know?

It is funny how much parameters are transfer from child to parent in asyncOpen args :)
And one of them is topWindowURI :)

Comment 18

[Dragana Damjanovic [:dragana]]

(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #12)
> Comment on attachment 8728914 [details] [diff] [review]
> 
> 
> diff --git a/toolkit/components/prompts/src/nsPrompter.js
> b/toolkit/components/prompts/src/nsPrompter.js
> --- a/toolkit/components/prompts/src/nsPrompter.js
> +++ b/toolkit/components/prompts/src/nsPrompter.js
> +        } else {
> +            if (!isCrossOrg) {
> +                text =
> PromptUtils.getLocalizedString("EnterUserPasswordFor",
> +                                                      [displayHost]);
> +            } else {
> +                text =
> PromptUtils.getLocalizedString("EnterUserPasswordForWithCrossOrigin",
> +                                                      [displayHost]);
> +            }
> +        }
> 
> What conditions cause us to get into this else block?  What is this for?
> 
> 


If realm is not set and it is not a proxy realm will be null.

Comment 19

[Dragana Damjanovic [:dragana]]

Before I submit the new patch I want to decide about the text:

Tanvi's suggestion:

%2$S is requesting a username and password. Note this is a different domain than the main webpage you are visiting.

The site says: "%1$S


Honza's suggestion 1:

%2$S is requesting a username and password. 

WARNING: the password will not be sent to the web page you are currently visiting!

The site says: "%1$S

Honza's suggestion 2:

Be careful, this can be an attempt to _*/steel your password/*_!


So I prefer Honza's suggestion 1 and writing the  "WARNING" part in a separate line.

Can we agree on one?

Comment 20

[Honza Bambas (:mayhemer)]

My suggestion is just one (both together), something like:

"""
%2$S is requesting a username and password.

WARNING: the password will not be sent to the web page you are currently visiting!
Be careful, this can be an attempt to _*/steel your password/*_!

The site says: "%1$S"
"""

I prefer mine :D

Comment 21

[Tanvi Vyas[:tanvi]]

(In reply to Dragana Damjanovic [:dragana] from comment #19)
> Before I submit the new patch I want to decide about the text:
> 
> Tanvi's suggestion:
> 
> %2$S is requesting a username and password. Note this is a different domain
> than the main webpage you are visiting.
> 
> The site says: "%1$S
> 
> 
> Honza's suggestion 1:
> 
> %2$S is requesting a username and password. 
> 
> WARNING: the password will not be sent to the web page you are currently
> visiting!
> 
> The site says: "%1$S

We need Matej's feedback here.  the "WARNING" text might be a bit much, but it depends on what percentage of basic auth requests are cross domain.  What does the telemetry indicate?  If it is <1%, then I think "WARNING" is fine.

New version for Matej's review:
"
%2$S is requesting a username and password. WARNING: the password will not be sent to the web page you are currently visiting.

The site says: "%1$S
"

I'm not sure how to convey to the user what we mean by 'the web page you are currently visiting'.  They are technically visiting both the top level page and the framed page.  Using 'top level' is confusing, but maybe we can say something about the site in the location bar.  'the password will not be sent to the web page listed in your location bar'?

Matej, please provide your thoughts.  Context: Basic auth prompts ask users for usernames and passwords.  In some cases, the prompt comes from a cross origin iframe.  In that case, we want to communicate to the user that the credentials they enter aren't being sent to "mozilla.org" but to "third-party-frame.com".

Comment 22

[Dragana Damjanovic [:dragana]]

Telemetry stats:
https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-02-10&keys=__none__!__none__!__none__&max_channel_version=release%252F44&measure=HTTP_AUTH_DIALOG_STATS&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-01-23&table=0&trim=1&use_submission_date=0


it is 1.77 on ff44 and 1.92 on ff43, but I think this stats are wrong, because how we compare uri in current implementation (this will be fixed with this bug). I expect it to be more.

Comment 23

[Matej Novak [:matej]]

(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #21)
> (In reply to Dragana Damjanovic [:dragana] from comment #19)
> > Before I submit the new patch I want to decide about the text:
> > 
> > Tanvi's suggestion:
> > 
> > %2$S is requesting a username and password. Note this is a different domain
> > than the main webpage you are visiting.
> > 
> > The site says: "%1$S
> > 
> > 
> > Honza's suggestion 1:
> > 
> > %2$S is requesting a username and password. 
> > 
> > WARNING: the password will not be sent to the web page you are currently
> > visiting!
> > 
> > The site says: "%1$S
> 
> We need Matej's feedback here.  the "WARNING" text might be a bit much, but
> it depends on what percentage of basic auth requests are cross domain.  What
> does the telemetry indicate?  If it is <1%, then I think "WARNING" is fine.
> 
> New version for Matej's review:
> "
> %2$S is requesting a username and password. WARNING: the password will not
> be sent to the web page you are currently visiting.
> 
> The site says: "%1$S
> "
> 
> I'm not sure how to convey to the user what we mean by 'the web page you are
> currently visiting'.  They are technically visiting both the top level page
> and the framed page.  Using 'top level' is confusing, but maybe we can say
> something about the site in the location bar.  'the password will not be
> sent to the web page listed in your location bar'?
> 
> Matej, please provide your thoughts.  Context: Basic auth prompts ask users
> for usernames and passwords.  In some cases, the prompt comes from a cross
> origin iframe.  In that case, we want to communicate to the user that the
> credentials they enter aren't being sent to "mozilla.org" but to
> "third-party-frame.com".

What feels confusing to me is that it's a warning about something that *isn't* going to happen. Is there a way we can reword it to indicate what *will* happen and why that's bad? As it is, it isn't clear to me what the threat or potential negative outcome might be.

Comment 24

[Dragana Damjanovic [:dragana]]

"
%2$S is requesting a username and password. WARNING: the web page requesting you password is not the same as the web page you are currently visiting.

The site says: "%1$S
"

(The first "the web page" is not really correct, but is "the server" understandable - I am not a good ui person)

"
%2$S is requesting a username and password. WARNING: the web page requesting you password is not the same as the web page you are currently visiting. Are you sure you want to send your password to %2$S?

The site says: "%1$S
"

Can we find a good wording here, soon?

Comment 25

[Tanvi Vyas[:tanvi]]

Matej and Philipp, please provide your thoughts on the below text.

<bold title>Authentication Required for %2$S</bold title>

<p1>%2$S is requesting your username and password. WARNING: Your password will be sent to %2$S, not the website you are currently visiting.</p1>

<p2>The site says: "%1$S".</p2>


Assume user is at a.com and a b.com iframe is requesting credentials.  The text will read:

Authentication Required for b.com

b.com is requesting your username and password.  WARNING: Your password will be sent to b.com, not the website you are currently visiting.

The site says: "message-from-website".

Comment 26

[Matej Novak [:matej]]

That looks great to me. Thanks.

Comment 27

[Dragana Damjanovic [:dragana]]

Attachment: bug_1230462_v1.patch

Justin, Matt,
I made a minimal change to the prompter. The text is split into 2 lines using \n\n (2 of them for readability). If you want to make 2 separate texts I would try to do it in a separate patch for easier review.

Comment 28

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=368ab124f9d4

Comment 29

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8740814 [details] [diff] [review]
bug_1230462_v1.patch

Passing review to Margaret for the Android patch.

Comment 30

[Tanvi Vyas[:tanvi]]

Comment on attachment 8740814 [details] [diff] [review]
bug_1230462_v1.patch

>diff --git a/toolkit/locales/en-US/chrome/global/commonDialogs.properties b/toolkit/locales/en-US/chrome/global/commonDialogs.properties
>--- a/toolkit/locales/en-US/chrome/global/commonDialogs.properties
>+++ b/toolkit/locales/en-US/chrome/global/commonDialogs.properties
>@@ -1,31 +1,34 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> Alert=Alert
> Confirm=Confirm
> ConfirmCheck=Confirm
> Prompt=Prompt
>-PromptUsernameAndPassword2=Authentication Required
>+PromptUsernameAndPassword2=Authentication Required for %1$S
Shouldn't this be %2$S?  That is what is used for the site name in the strings below.
> PromptPassword2=Password Required
...
> ScriptDialogPreventTitle=Confirm Dialog Preference
>-# LOCALIZATION NOTE (EnterLoginForRealm, EnterLoginForProxy):
>+# LOCALIZATION NOTE (EnterLoginForRealm, EnterLoginForRealmWithCrossOrigin,
>+# EnterLoginForProxy, EnterUserPasswordForWithCrossOrigin):
> # %1 is an untrusted string provided by a remote server. It could try to
> # take advantage of sentence structure in order to mislead the user (see
> # bug 244273). %1 should be integrated into the translated sentences as
> # little as possible. %2 is the url of the site being accessed.
>-EnterLoginForRealm=A username and password are being requested by %2$S. The site says: "%1$S"
>+EnterLoginForRealm=%2$S is requesting your username and password.\n\nThe site says: "%1$S"
>+EnterLoginForRealmWithCrossOrigin=%2$S is requesting your username and password. WARNING: Your password will be sent to %2$S, not the website you are currently visiting.\n\nThe site says: "%1$S".
> EnterLoginForProxy=The proxy %2$S is requesting a username and password. The site says: "%1$S"
>-EnterUserPasswordFor=Enter username and password for %1$S
>+EnterUserPasswordFor=%1$S is requesting your username and password.
>+EnterUserPasswordForWithCrossOrigin=%2$S is requesting your username and password. WARNING: Your password will be sent to %2$S, not the website you are currently visiting.
> EnterPasswordFor=Enter password for %1$S on %2$S

Comment 31

[Honza Bambas (:mayhemer)]

Attachment: v1 output (screenshot)

This is what I get with the patch.  Not sure this is what we should do.  Note that the two blurred domains on the first two lines are identical.  It may appear a bit weird..

Comment 32

[Honza Bambas (:mayhemer)]

Attachment: v1 - adjusted

What about:

%2$S is requesting your username and password.\n\nThe site says: "%1$S"\n\nWARNING: Your password will not be sent to the website you are currently visiting!

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 8740814 [details] [diff] [review]
bug_1230462_v1.patch

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

Few questions raised + the string is probably not exactly what we want.

::: netwerk/base/nsIAuthInformation.idl
@@ +55,5 @@
>     */
>    const uint32_t PREVIOUS_FAILED = 16;
> +
> +  /**
> +   * A subresource with a different domain requests an authentication.

A cross-origin sub-resource requests...

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +856,5 @@
> +
> +        if (!topDoc && !xhr) {
> +            nsCOMPtr<nsIURI> topURI;
> +            nsCOMPtr<nsIHttpChannelInternal> chanInternal = do_QueryInterface(mAuthChannel);
> +            MOZ_ASSERT(chanInternal);

and what about websockets?  I think you need a null check here..

@@ +867,5 @@
> +                    loadingPrinc->GetURI(getter_AddRefs(topURI));
> +                }
> +            }
> +
> +            if (!NS_SecurityCompareURIs(topURI, mURI, true)) {

and if we still don't have topURI?

shouldn't we rather use checkmayload here?  or what there a problem with it?  because checking on urls is generally a bad thing to do in the world with principals.

::: netwerk/protocol/http/nsHttpChannelAuthProvider.h
@@ +153,5 @@
>      uint32_t                          mTriedProxyAuth           : 1;
>      uint32_t                          mTriedHostAuth            : 1;
>      uint32_t                          mSuppressDefensiveAuth    : 1;
>  
> +    uint32_t                          mCrossOrigin              : 1;

some comments please.

Comment 34

[:Margaret Leibovic]

Comment on attachment 8740814 [details] [diff] [review]
bug_1230462_v1.patch

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

/mobile changes look fine to me.

Comment 35

[Tanvi Vyas[:tanvi]]

(In reply to Honza Bambas (:mayhemer) from comment #31)
> Created attachment 8741410 [details]
> v1 output (screenshot)
> 
> This is what I get with the patch.  Not sure this is what we should do. 
> Note that the two blurred domains on the first two lines are identical.  It
> may appear a bit weird..

The blurred domains are the same as the subject line, right?  test.local2 i7?  So we repeat the domain 3 times, showing the user that it is different than the top level page.

I don't see the problem here.  Is it the redundancy?  Can you elaborate.


(In reply to Honza Bambas (:mayhemer) from comment #32)
> Created attachment 8741413 [details]
> v1 - adjusted
> 
> What about:
> 
> %2$S is requesting your username and password.\n\nThe site says:
> "%1$S"\n\nWARNING: Your password will not be sent to the website you are
> currently visiting!

Per Comment 23, Matej would prefer if we state what will happen instead of what won't happen in the warning message.

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to Tanvi Vyas [:tanvi] from comment #35)
> (In reply to Honza Bambas (:mayhemer) from comment #31)
> > Created attachment 8741410 [details]
> > v1 output (screenshot)
> > 
> > This is what I get with the patch.  Not sure this is what we should do. 
> > Note that the two blurred domains on the first two lines are identical.  It
> > may appear a bit weird..
> 
> The blurred domains are the same as the subject line, right?  test.local2
> i7?  So we repeat the domain 3 times, showing the user that it is different
> than the top level page.

Yep, exactly.

> 
> I don't see the problem here.  Is it the redundancy?  Can you elaborate.

I think it's simply too much in the text.  Somehow I don't feel it's right.

> 
> 
> (In reply to Honza Bambas (:mayhemer) from comment #32)
> > Created attachment 8741413 [details]
> > v1 - adjusted
> > 
> > What about:
> > 
> > %2$S is requesting your username and password.\n\nThe site says:
> > "%1$S"\n\nWARNING: Your password will not be sent to the website you are
> > currently visiting!
> 
> Per Comment 23, Matej would prefer if we state what will happen instead of
> what won't happen in the warning message.

OK, I'm not the UX person here anyway ;)  Just giving my few cents.

Comment 37

[Justin Dolske [:Dolske]]

Comment on attachment 8740814 [details] [diff] [review]
bug_1230462_v1.patch

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

You should also add a test to exercise the thing you're adding.

test_bug625187.html does something like this already. Although note I'm refactoring the prompter tests in bug 1265194, so I'd suggest basing on top of that. Renaming and extending that test is probably best, but if you're actually doing things differently enough it could be a separate new test. But the former would be preferred if possible.

(Actually, now that I think of it, there are also some auth prompt tests under passwordmgr/, but I'm not sure offhand if that's a better place.)

::: mobile/android/components/PromptService.js
@@ +398,5 @@
>  
>    promptAuth: function promptAuth(aChannel, aLevel, aAuthInfo) {
>      let checkMsg = null;
>      let check = { value: false };
> +    let [title, message] = PromptUtils.makeDialogText(aChannel, aAuthInfo);

Whyyyyyy does mobile have a fork of this code. :(

::: toolkit/components/prompts/src/nsPrompter.js
@@ +297,5 @@
>  
> +        let title;
> +        title = PromptUtils.getLocalizedString("PromptUsernameAndPassword2",
> +                                               [displayHost]);
> +        return [title, text];

I think you should leave the prompt title unchanged.

The string here is normally in the dialog window titlebar, which I'd expect most people are just going to ignore, and should generally just be a short string. (OS X is the exception here -- window sheets don't have a titlebar, so it's displayed in bold in the prompt's body.) It would also just be duplicating the info that's already in the main message body.

@@ +860,2 @@
>          else
> +            ok = this.nsIPrompt_promptUsernameAndPassword(title, message, userParam, passParam, checkLabel, checkValue);

I don't understand why you're changing this. (from _promptPassword to _promptUsernameAndPassword). You're just changing the message (in certain cases), but these functions do different things.

@@ +928,2 @@
>          else
> +            ok = this.oldPrompter.promptUsernameAndPassword(title, message, authTarget, Ci.nsIAuthPrompt.SAVE_PASSWORD_PERMANENTLY, userParam, passParam);

Ditto.

::: toolkit/components/prompts/test/test_modal_prompts.html
@@ +691,5 @@
>  
>        case 100:
>          // PromptAuth (no realm, ok, with checkbox)
>          state = {
> +            msg : 'http://example.com is requesting your username and password.',

Just FYI: this test was recently majorly refactored for E10S, although your change will be easy to port.

::: toolkit/locales/en-US/chrome/global/commonDialogs.properties
@@ +25,5 @@
>  # %1 is an untrusted string provided by a remote server. It could try to
>  # take advantage of sentence structure in order to mislead the user (see
>  # bug 244273). %1 should be integrated into the translated sentences as
>  # little as possible. %2 is the url of the site being accessed.
> +EnterLoginForRealm=%2$S is requesting your username and password.\n\nThe site says: "%1$S"

You need to change the property name when changing strings. Otherwise the L10N tools won't retrigger localization.

You can just append/increment a number at the end (eg EnterLoginForRealm2). You'll also need to update all callsites referencing the old property name (yes, I know this is annoying :).

@@ +31,3 @@
>  EnterLoginForProxy=The proxy %2$S is requesting a username and password. The site says: "%1$S"
> +EnterUserPasswordFor=%1$S is requesting your username and password.
> +EnterUserPasswordForWithCrossOrigin=%2$S is requesting your username and password. WARNING: Your password will be sent to %2$S, not the website you are currently visiting.

Couple comments on the new strings:

1) Are you sure we want to say the sitename twice? This seems a little wordy, especially if it's a long name. (And consider that might be a scammy/phishy domain like http://yourbank.totallysafe.trustme.fake.ru.

I'd suggest:

  A username and password are being requested. WARNING: Your password
  will be sent to %2$S, not the website you are currently visiting.

  The site says: "%1$S"

and maybe add an extra linebreak to make the warning more visible:

  A username and password are being requested.

  WARNING: Your password will be sent to %2$S, not the website
  you are currently visiting.

  The site says: "%1$S"


2) Should we take the opportunity to entirely remove the realm string when it's cross-origin? This would shorten the overall prompt, and I'd suspect (with zero data) that it's more likely to abused when it's cross-origin. I'm not sure it's even of significant benefit when same-origin, so this might be a good place to experiment with its removal.

So just:

  A username and password are being requested.

  WARNING: Your password will be sent to %S, not the website
  you are currently visiting.

Comment 38

[:Margaret Leibovic]

(In reply to Justin Dolske [:Dolske] from comment #37)
> Comment on attachment 8740814 [details] [diff] [review]
> bug_1230462_v1.patch
> 
> Review of attachment 8740814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should also add a test to exercise the thing you're adding.
> 
> test_bug625187.html does something like this already. Although note I'm
> refactoring the prompter tests in bug 1265194, so I'd suggest basing on top
> of that. Renaming and extending that test is probably best, but if you're
> actually doing things differently enough it could be a separate new test.
> But the former would be preferred if possible.
> 
> (Actually, now that I think of it, there are also some auth prompt tests
> under passwordmgr/, but I'm not sure offhand if that's a better place.)
> 
> ::: mobile/android/components/PromptService.js
> @@ +398,5 @@
> >  
> >    promptAuth: function promptAuth(aChannel, aLevel, aAuthInfo) {
> >      let checkMsg = null;
> >      let check = { value: false };
> > +    let [title, message] = PromptUtils.makeDialogText(aChannel, aAuthInfo);
> 
> Whyyyyyy does mobile have a fork of this code. :(

Because we use native prompt dialogs, but the interface designed such that the UI is easily abstracted away.

Although this does date back to well before our native UI: http://hg.mozilla.org/mozilla-central/rev/5c00ae3b2184

Comment 39

[Justin Dolske [:Dolske]]

(In reply to :Margaret Leibovic from comment #38)

> > Whyyyyyy does mobile have a fork of this code. :(
> 
> Because we use native prompt dialogs, but the interface designed such that
> the UI is easily abstracted away.
> 
> Although this does date back to well before our native UI:
> http://hg.mozilla.org/mozilla-central/rev/5c00ae3b2184

Ideally this would be forked further down at the openPrompt() / commonDialog.js/xul level, so that all the XPCOM API goop is the same. Just grumbling, not a huge deal (nor something to fix in this bug ;).

[And unrelated, revising title of this bug since it's kinda vague.]

Comment 40

[Dragana Damjanovic [:dragana]]

(In reply to Tanvi Vyas [:tanvi] from comment #30)
> Comment on attachment 8740814 [details] [diff] [review]
> bug_1230462_v1.patch
> 
> >diff --git a/toolkit/locales/en-US/chrome/global/commonDialogs.properties b/toolkit/locales/en-US/chrome/global/commonDialogs.properties
> >--- a/toolkit/locales/en-US/chrome/global/commonDialogs.properties
> >+++ b/toolkit/locales/en-US/chrome/global/commonDialogs.properties
> >@@ -1,31 +1,34 @@
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # License, v. 2.0. If a copy of the MPL was not distributed with this
> > # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> > Alert=Alert
> > Confirm=Confirm
> > ConfirmCheck=Confirm
> > Prompt=Prompt
> >-PromptUsernameAndPassword2=Authentication Required
> >+PromptUsernameAndPassword2=Authentication Required for %1$S
> Shouldn't this be %2$S?  That is what is used for the site name in the
> strings below.


%1$S is for the first argument and %2$S is for the second. For creating this string we need just one argument therefore I past just one and then it is the first (%1$S).

Comment 41

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #33)
> Comment on attachment 8740814 [details] [diff] [review]
> bug_1230462_v1.patch
> 
> Review of attachment 8740814 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
> @@ +856,5 @@
> > +
> > +        if (!topDoc && !xhr) {
> > +            nsCOMPtr<nsIURI> topURI;
> > +            nsCOMPtr<nsIHttpChannelInternal> chanInternal = do_QueryInterface(mAuthChannel);
> > +            MOZ_ASSERT(chanInternal);
> 
> and what about websockets?  I think you need a null check here..
> 

There is the same check already there now, just couple of lines above - I have just ntice it that I could remove these lines since we already have chanInternal:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#824


> @@ +867,5 @@
> > +                    loadingPrinc->GetURI(getter_AddRefs(topURI));
> > +                }
> > +            }
> > +
> > +            if (!NS_SecurityCompareURIs(topURI, mURI, true)) {
> 
> and if we still don't have topURI?

It will look like cross-origin :) I can change that but I am not sure it can happen.

> 
> shouldn't we rather use checkmayload here?  or what there a problem with it?
> because checking on urls is generally a bad thing to do in the world with
> principals.

We can not use principals, e.g. :
if we load this example
https://people.mozilla.org/~tvyas/test-basic-auth2.html
loadingPrincipal has uri  https://people.mozilla.org/~tvyas/test-basic-auth2.html but actually we are loading https://www.httpwatch.com/httpgallery/authentication/authenticatedimage/default.aspx

I notice one other confusing example: If a page has a link to e.g. https://www.httpwatch.com/httpgallery/authentication/authenticatedimage/default.aspx and if you click on the link the load is not cross-origin because we are loading a new page but address bar still shows the old address because it is not updated yet.

Comment 42

[Dragana Damjanovic [:dragana]]

About the text - I will leave it to Matej:
please take a look at comment #31 #32 #35 #36 #37(also against the page uri in the title and some other text changes)

I could pick my favorite :)
I will fix thinks commented in #37 - fix property name (EnterLoginForRealm2) and write a test

Comment 43

[Dragana Damjanovic [:dragana]]

(In reply to Justin Dolske [:Dolske] from comment #37)
> Comment on attachment 8740814 [details] [diff] [review]
> bug_1230462_v1.patch
> 
> Review of attachment 8740814 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +860,2 @@
> >          else
> > +            ok = this.nsIPrompt_promptUsernameAndPassword(title, message, userParam, passParam, checkLabel, checkValue);
> 
> I don't understand why you're changing this. (from _promptPassword to
> _promptUsernameAndPassword). You're just changing the message (in certain
> cases), but these functions do different things.
> 

I have just changed null to title, I have not change the functions. Am I missing something? Anyway probably I will revert this changes if the are not changing the title.


> @@ +928,2 @@
> >          else
> > +            ok = this.oldPrompter.promptUsernameAndPassword(title, message, authTarget, Ci.nsIAuthPrompt.SAVE_PASSWORD_PERMANENTLY, userParam, passParam);
> 
> Ditto.

Comment 44

[Tanvi Vyas[:tanvi]]

(In reply to Dragana Damjanovic [:dragana] from comment #41)
> I notice one other confusing example: If a page has a link to e.g.
> https://www.httpwatch.com/httpgallery/authentication/authenticatedimage/
> default.aspx and if you click on the link the load is not cross-origin
> because we are loading a new page but address bar still shows the old
> address because it is not updated yet.

That's not good.  Can we suppress the prompt until the address bar has updated?

Comment 45

[Tanvi Vyas[:tanvi]]

(In reply to Justin Dolske [:Dolske] from comment #37)
> 2) Should we take the opportunity to entirely remove the realm string when
> it's cross-origin? This would shorten the overall prompt, and I'd suspect
> (with zero data) that it's more likely to abused when it's cross-origin. I'm
> not sure it's even of significant benefit when same-origin, so this might be
> a good place to experiment with its removal.
> 
> So just:
> 
>   A username and password are being requested.
> 
>   WARNING: Your password will be sent to %S, not the website
>   you are currently visiting.

Yes, I think its a good opportunity t remove the string specified by the site, as it just adds more text and makes the the user more likely to ignore the content.  Chrome has removed it.

But I still think we should include the site name in the first sentence, in case users don't read the second.  Particularly because the "WARNING" isn't styled differently to call attention to it; it supposed to be temporary until we can get a real UX mockup.  I actually like the way Chrome does it, where the URL is the first thing in the sentence.  So how about:

>   SITE_NAME is requesting a username and password.

>   WARNING: Your password will be sent to SITE_NAME, not the website
>   you are currently visiting.

Comment 46

[Justin Dolske [:Dolske]]

(In reply to Dragana Damjanovic [:dragana] from comment #43)

> > I don't understand why you're changing this. (from _promptPassword to
> > _promptUsernameAndPassword). You're just changing the message (in certain
> > cases), but these functions do different things.
> > 
> 
> I have just changed null to title, I have not change the functions. Am I
> missing something? Anyway probably I will revert this changes if the are not
> changing the title.

Huh, I must have been confused, looking at the diff again I'm not sure why I thought that!


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

> That's not good.  Can we suppress the prompt until the address bar has
> updated?

It's all just software, but I'd expect that's likely to be tricky. We haven't actually started loading the new page yet, and the old page is still showing -- the request failed (401), and we're prompting the user before trying again. This has come up before in other spoofing-type bugs...


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

> But I still think we should include the site name in the first sentence, in
> case users don't read the second.

I'll defer to Matej here, I'm just a bit dubious that repeating something twice is adding much value. I'll defer to Matej here, I'm just a bit dubious that repeating something twice is adding much value. [;-)] I'd generally think the most value comes from making it look different enough from normal that users might actually stop and read any of it, and then making it concise for the users who do read it. The patch actually was showing it 3 times (including the title), so I wasn't sure if everything had been considered together.

Comment 47

[Dragana Damjanovic [:dragana]]

(In reply to Tanvi Vyas [:tanvi] from comment #44)
> (In reply to Dragana Damjanovic [:dragana] from comment #41)
> > I notice one other confusing example: If a page has a link to e.g.
> > https://www.httpwatch.com/httpgallery/authentication/authenticatedimage/
> > default.aspx and if you click on the link the load is not cross-origin
> > because we are loading a new page but address bar still shows the old
> > address because it is not updated yet.
> 
> That's not good.  Can we suppress the prompt until the address bar has
> updated?

This is separate issue. Open another bug for this. I am not sure how easy is to resolve this.

Comment 48

[Matej Novak [:matej]]

The version in comment 37 looks good to me. I just wonder if it should say "Your username and password…" rather than "A username…"

Comment 49

[Tanvi Vyas[:tanvi]]

> (In reply to Tanvi Vyas [:tanvi] from comment #45)
> 
> > But I still think we should include the site name in the first sentence, in
> > case users don't read the second.
> 
> I'll defer to Matej here, I'm just a bit dubious that repeating something
> twice is adding much value. I'll defer to Matej here, I'm just a bit dubious
> that repeating something twice is adding much value. [;-)] I'd generally
> think the most value comes from making it look different enough from normal
> that users might actually stop and read any of it, and then making it
> concise for the users who do read it. The patch actually was showing it 3
> times (including the title), so I wasn't sure if everything had been
> considered together.

Yes, ideally we would do something to highlight the domain mismatch in the UI, but this is just a temporary string-change-only solution right now (per comment 4-6).  Really, we need to redesign the whole prompt.

I'd still prefer the first sentence include the domain name.  That would mean its repeated in the second sentence; we could leave it out of the title.

Comment 50

[Dragana Damjanovic [:dragana]]

Actually I prefer sentence with domain name as well. I am sure if users read something it will be the first word so it is more likely that they are going to see the domain name.

No cross-domain (removing realm only for cross domain):
DOMAIN  is requesting your username and password.

The site says: REALM

For cross-domain:
DOMAIN  is requesting your username and password.

WARNING: Your password will be sent to %S, not the website you are currently visiting.

Or maybe add the current site name:
WARNING: Your password will be sent to DOMAIN, not the website you are currently visiting (CURRENT_DOMAIN).

Comment 51

[Ash Grigas]

I suggest going forward with v1 adjusted to avoid redundancy of the website in both the request text and the warning text.

Comment 52

[Tanvi Vyas[:tanvi]]

(In reply to agrigas from comment #51)
> I suggest going forward with v1 adjusted to avoid redundancy of the website
> in both the request text and the warning text.

v1 adjusted for cross-domain frames is:

DOMAIN is requesting your username and password.

WARNING: Your password will not be sent to the website you are currently visiting!

Comment 53

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cf53fdad305

Comment 54

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2595b3f8f7a8

Comment 55

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcb7dbbbf4d4

Comment 56

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce0bc1b5a127

Comment 57

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10ebfb76fa21

Comment 58

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9a402c01fa0

Comment 59

[Dragana Damjanovic [:dragana]]

Attachment: bug_1230462_v2.patch

Comment 60

[:Margaret Leibovic]

Comment on attachment 8751607 [details] [diff] [review]
bug_1230462_v2.patch

I defer to someone with more domain expertise than me to review the mobile part of the patch in addition to the desktop part.

Comment 61

[Justin Dolske [:Dolske]]

Comment on attachment 8751607 [details] [diff] [review]
bug_1230462_v2.patch

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

Looks fine to me, with fixes. I'd call it r+ except for the one "This seems surprising" comment about the checkbox and password manager. Can you explain why that's happening?

::: toolkit/components/prompts/test/test_modal_prompts.html
@@ +1095,5 @@
> +        defButton   : "button0",
> +    };
> +    action = {
> +        buttonClick : "ok",
> +        setCheckbox : true,

Minor nit: I'd suggest not setting the checkbox, since it's not really involved in what you're interested in testing here.

@@ +1138,5 @@
> +        defButton   : "button0",
> +    };
> +    action = {
> +        buttonClick : "ok",
> +        setCheckbox : true,

Ditto.

::: toolkit/components/prompts/test/test_subresources_prompts.html
@@ +114,5 @@
> +  // cross-origin subresources load
> +
> +  // Force parent to not look for tab-modal prompts, as they're not
> +  // used for auth prompts.
> +  isTabModal =false;

Nit, spacing after '='.

@@ +116,5 @@
> +  // Force parent to not look for tab-modal prompts, as they're not
> +  // used for auth prompts.
> +  isTabModal =false;
> +
> +  var e10sMode = SpecialPowers.Services.appinfo.browserTabsRemoteAutostart;

There should already be an |isE10S| in scope, and used by other tests. (Coming from the top of prompt_common.js)

@@ +138,5 @@
> +
> +  if (e10sMode) {
> +    state.checkHidden = false;
> +    state.checkMsg = "Use Password Manager to remember this password."
> +  }

This seems surprising, why would Password Manager not be offering to save the login in non-E10S mode?

Also, you should probably uncheck it when it shows, to prevent having password manager save it. (Otherwise you'd need to remove it in this test, to leave a clean state. Or if you actually want to test password manager's involvement, moving your test additions into the passwordmgr test dir would probably be more appropriate.)

@@ +148,5 @@
> +  };
> +
> +  promptDone = handlePrompt(state, action);
> +
> +  function checkEchoedAuthInfo(expectedState) {

This function already exists in toolkit/components/passwordmgr/test/mochitest/test_prompt.html, which is presumably where you copied it from. :)

Instead of duplicating it multiple times, please move that code to toolkit/components/prompts/test/prompt_common.js. (That file is already loaded by the pwmgr test_prompt.html, so it should just work there.) Similarly, that file is loaded by this test, so you won't need to duplicate it here.

@@ +164,5 @@
> +  yield promptDone;
> +  yield iframe3Loaded;
> +  checkEchoedAuthInfo({user: "mochiuser1", pass: "mochipass1"});
> +
> +  // Corss-origin subresourse test.

Nit: "Cross", "subresource"

Comment 62

[Dragana Damjanovic [:dragana]]

(In reply to Justin Dolske [:Dolske] from comment #61)
> Comment on attachment 8751607 [details] [diff] [review]
> bug_1230462_v2.patch
> 
> Review of attachment 8751607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me, with fixes. I'd call it r+ except for the one "This seems
> surprising" comment about the checkbox and password manager. Can you explain
> why that's happening?
> 

I was hoping you could tell me :) I have not touched anything related to the password manager in this bug so I expect that the behaviour is the same without this patch. For me it looks like it is not a platform problem.
toolkit/components/passwordmgr/test/mochitest/test_prompt.html uses a similar way to test prompts but it is turned off on e10s so it does not see the same problem as here.

I will take a look and open another bug for this if it is not related to this patch. I think it is not related.

> ::: toolkit/components/prompts/test/test_modal_prompts.html
> @@ +1095,5 @@
> > +        defButton   : "button0",
> > +    };
> > +    action = {
> > +        buttonClick : "ok",
> > +        setCheckbox : true,
> 
> Minor nit: I'd suggest not setting the checkbox, since it's not really
> involved in what you're interested in testing here.
> 
> @@ +1138,5 @@
> > +        defButton   : "button0",
> > +    };
> > +    action = {
> > +        buttonClick : "ok",
> > +        setCheckbox : true,
> 
> Ditto.
> 
> ::: toolkit/components/prompts/test/test_subresources_prompts.html
> @@ +114,5 @@
> > +  // cross-origin subresources load
> > +
> > +  // Force parent to not look for tab-modal prompts, as they're not
> > +  // used for auth prompts.
> > +  isTabModal =false;
> 
> Nit, spacing after '='.
> 
> @@ +116,5 @@
> > +  // Force parent to not look for tab-modal prompts, as they're not
> > +  // used for auth prompts.
> > +  isTabModal =false;
> > +
> > +  var e10sMode = SpecialPowers.Services.appinfo.browserTabsRemoteAutostart;
> 
> There should already be an |isE10S| in scope, and used by other tests.
> (Coming from the top of prompt_common.js)
> 
> @@ +138,5 @@
> > +
> > +  if (e10sMode) {
> > +    state.checkHidden = false;
> > +    state.checkMsg = "Use Password Manager to remember this password."
> > +  }
> 
> This seems surprising, why would Password Manager not be offering to save
> the login in non-E10S mode?
> 
> Also, you should probably uncheck it when it shows, to prevent having
> password manager save it. (Otherwise you'd need to remove it in this test,
> to leave a clean state. Or if you actually want to test password manager's
> involvement, moving your test additions into the passwordmgr test dir would
> probably be more appropriate.)

Thanks for the feedback, I am new to these firefox, not platform only, tests. I do not need to test password manager here so i will uncheck the checkbox and leave the tests where it is currently.

> 
> @@ +148,5 @@
> > +  };
> > +
> > +  promptDone = handlePrompt(state, action);
> > +
> > +  function checkEchoedAuthInfo(expectedState) {
> 
> This function already exists in
> toolkit/components/passwordmgr/test/mochitest/test_prompt.html, which is
> presumably where you copied it from. :)
> 
> Instead of duplicating it multiple times, please move that code to
> toolkit/components/prompts/test/prompt_common.js. (That file is already
> loaded by the pwmgr test_prompt.html, so it should just work there.)
> Similarly, that file is loaded by this test, so you won't need to duplicate
> it here.
> 

Thanks, I will move it.

Comment 63

[Justin Dolske [:Dolske]]

(In reply to Dragana Damjanovic [:dragana] from comment #62)
> (In reply to Justin Dolske [:Dolske] from comment #61)

> > Looks fine to me, with fixes. I'd call it r+ except for the one "This seems
> > surprising" comment about the checkbox and password manager. Can you explain
> > why that's happening?
> > 
> 
> I was hoping you could tell me :) I have not touched anything related to the
> password manager in this bug so I expect that the behaviour is the same
> without this patch. 

I'm not sure why this would happen and would have to debug it. Maybe this rings a bell for MattN?

I would suggest looking at / logging what's happening in promptUsernameAndPassword() in nsLoginManagerPrompter.js. If it ends up being an existing issue it doesn't need to be fixed here, but we should understand why it's happening.

Comment 64

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #63)
> (In reply to Dragana Damjanovic [:dragana] from comment #62)
> > (In reply to Justin Dolske [:Dolske] from comment #61)
> 
> > > Looks fine to me, with fixes. I'd call it r+ except for the one "This seems
> > > surprising" comment about the checkbox and password manager. Can you explain
> > > why that's happening?
> > > 
> > 
> > I was hoping you could tell me :) I have not touched anything related to the
> > password manager in this bug so I expect that the behaviour is the same
> > without this patch. 
> 
> I'm not sure why this would happen and would have to debug it. Maybe this
> rings a bell for MattN?
> 
> I would suggest looking at / logging what's happening in
> promptUsernameAndPassword() in nsLoginManagerPrompter.js. If it ends up
> being an existing issue it doesn't need to be fixed here, but we should
> understand why it's happening.

Funny that I just filed a bug on this (bug 1274042) as I happened to notice the e10s regression while investigating bug 1272654.

My understanding is that the checkbox should only show in certain cases (I think when we don't have doorhangers?) and the e10s logic for that seems to be broken.

Comment 65

[Justin Dolske [:Dolske]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #64)

> My understanding is that the checkbox should only show in certain cases (I
> think when we don't have doorhangers?) and the e10s logic for that seems to
> be broken.

Oh, right! Duh! Yeah, we shouldn't be showing this checkbox at all in Firefox.

Cool -- test is fine for now, we'll fix the issue in the other bugs.

[This is why it's good to investigate unexpected things!]

Comment 66

[Tanvi Vyas[:tanvi]]

Comment on attachment 8751607 [details] [diff] [review]
bug_1230462_v2.patch

I reviewed only the string changes.  They look good.

>diff --git a/toolkit/locales/en-US/chrome/global/commonDialogs.properties 
>@@ -15,17 +15,18 @@ Yes=&Yes
>+# LOCALIZATION NOTE (EnterLoginForRealm2, EnterLoginForProxy2):
> # %1 is an untrusted string provided by a remote server. It could try to
> # take advantage of sentence structure in order to mislead the user (see
> # bug 244273). %1 should be integrated into the translated sentences as
> # little as possible. %2 is the url of the site being accessed.
...
> EnterPasswordFor=Enter password for %1$S on %2$S
Are %1$S and %2$S listed above accurate for EnterPasswordFor?  I'm not familiar with the use cases for password-only-HTTP-Auth, but we may want to add a similar warning note for cross origin requests as a followup bug.  If the occurrence is low enough, we may be able to just disable cross origin password-only-HTTP-Auth prompts.

Something like:

} else if (isPassOnly && isCrossOrigin) {

            text = PromptUtils.getLocalizedString("EnterPasswordForCrossOrigin", [username, displayHost]);
} else if (isPassOnly) {
            text = PromptUtils.getLocalizedString("EnterPasswordFor", [username, displayHost]);
} ..

EnterPasswordForCrossOrigin=Enter password for %1$S on %2$S.\n\nWARNING: Your password will not be sent to the website you are currently visiting!

Comment 67

[Honza Bambas (:mayhemer)]

Comment on attachment 8751607 [details] [diff] [review]
bug_1230462_v2.patch

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

The dialog is broken for the CORS case (at least on win).  Its height must be made bigger, otherwise the buttons are cut at the bottom.

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +866,5 @@
>      nsCOMPtr<nsIChannel> chan = do_QueryInterface(mAuthChannel);
>      nsCOMPtr<nsILoadInfo> loadInfo;
>      chan->GetLoadInfo(getter_AddRefs(loadInfo));
> +
> +    // Loads without a loadInfo we will treat as a top level document.

We will treat loads w/o loadInfo as a top level document.

@@ +876,5 @@
> +            nsIContentPolicy::TYPE_DOCUMENT) {
> +            topDoc = false;
> +        }
> +        if (loadInfo->GetExternalContentPolicyType() ==
> +              nsIContentPolicy::TYPE_XMLHTTPREQUEST) {

nit: indent

::: netwerk/test/unit/test_authentication.js
@@ +57,5 @@
> +      if (text.indexOf(this.expectedRealm) == -1)
> +        do_throw("Text must indicate the realm");
> +    } else {
> +      if (text.indexOf(this.expectedRealm) != -1)
> +        do_throw("There should not be realm for cross origin");

maybe change to if () { }

::: toolkit/components/prompts/test/mochitest.ini
@@ -8,5 @@
>  
>  [test_bug619644.html]
>  [test_bug620145.html]
>  skip-if = toolkit == 'android' #TIMED_OUT
> -[test_bug625187.html]

why has this been removed?

@@ +10,5 @@
>  [test_bug619644.html]
>  [test_bug620145.html]
>  skip-if = toolkit == 'android' #TIMED_OUT
> +[test_subresources_prompts.html]
> +skip-if = toolkit == 'android'

why not on android?

::: toolkit/locales/en-US/chrome/global/commonDialogs.properties
@@ +24,5 @@
>  # %1 is an untrusted string provided by a remote server. It could try to
>  # take advantage of sentence structure in order to mislead the user (see
>  # bug 244273). %1 should be integrated into the translated sentences as
>  # little as possible. %2 is the url of the site being accessed.
> +EnterLoginForRealm2=%2$S is requesting your username and password.\n\nThe site says: “%1$S”

why are you renaming the token?  Is that some l10n rule?  the arguments have not changed, so what is the reason?  to highlight it needs to be translated?

Comment 68

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #67)
> Comment on attachment 8751607 [details] [diff] [review]
> bug_1230462_v2.patch
> 
> Review of attachment 8751607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The dialog is broken for the CORS case (at least on win).  Its height must
> be made bigger, otherwise the buttons are cut at the bottom.

I will see to fix that.

> 
> ::: toolkit/components/prompts/test/mochitest.ini
> @@ -8,5 @@
> >  
> >  [test_bug619644.html]
> >  [test_bug620145.html]
> >  skip-if = toolkit == 'android' #TIMED_OUT
> > -[test_bug625187.html]
> 
> why has this been removed?

it is renamed to test_subresources_prompts.html and extened.

> 
> @@ +10,5 @@
> >  [test_bug619644.html]
> >  [test_bug620145.html]
> >  skip-if = toolkit == 'android' #TIMED_OUT
> > +[test_subresources_prompts.html]
> > +skip-if = toolkit == 'android'
> 
> why not on android?

There are couple of test that are turned of on andoid and there is a bug for them, so this will fix along.

> 
> ::: toolkit/locales/en-US/chrome/global/commonDialogs.properties
> @@ +24,5 @@
> >  # %1 is an untrusted string provided by a remote server. It could try to
> >  # take advantage of sentence structure in order to mislead the user (see
> >  # bug 244273). %1 should be integrated into the translated sentences as
> >  # little as possible. %2 is the url of the site being accessed.
> > +EnterLoginForRealm2=%2$S is requesting your username and password.\n\nThe site says: “%1$S”
> 
> why are you renaming the token?  Is that some l10n rule?  the arguments have
> not changed, so what is the reason?  to highlight it needs to be translated?

Yes, otherwise they will not be translated.

Comment 69

[Dragana Damjanovic [:dragana]]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #66)
> Comment on attachment 8751607 [details] [diff] [review]
> bug_1230462_v2.patch
> 
> I reviewed only the string changes.  They look good.
> 
> >diff --git a/toolkit/locales/en-US/chrome/global/commonDialogs.properties 
> >@@ -15,17 +15,18 @@ Yes=&Yes
> >+# LOCALIZATION NOTE (EnterLoginForRealm2, EnterLoginForProxy2):
> > # %1 is an untrusted string provided by a remote server. It could try to
> > # take advantage of sentence structure in order to mislead the user (see
> > # bug 244273). %1 should be integrated into the translated sentences as
> > # little as possible. %2 is the url of the site being accessed.
> ...
> > EnterPasswordFor=Enter password for %1$S on %2$S
> Are %1$S and %2$S listed above accurate for EnterPasswordFor?  I'm not
> familiar with the use cases for password-only-HTTP-Auth, but we may want to
> add a similar warning note for cross origin requests as a followup bug.  If
> the occurrence is low enough, we may be able to just disable cross origin
> password-only-HTTP-Auth prompts.
> 

%1$S and %2$S are not the same for EnterPasswordFor as described above. The note above is only for EnterLoginForRealm2, EnterLoginForProxy2 

> Something like:
> 
> } else if (isPassOnly && isCrossOrigin) {
> 
>             text =
> PromptUtils.getLocalizedString("EnterPasswordForCrossOrigin", [username,
> displayHost]);
> } else if (isPassOnly) {
>             text = PromptUtils.getLocalizedString("EnterPasswordFor",
> [username, displayHost]);
> } ..
> 
> EnterPasswordForCrossOrigin=Enter password for %1$S on %2$S.\n\nWARNING:
> Your password will not be sent to the website you are currently visiting!

We can do this in follow up bug, I would like to land this on 49.

Comment 70

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26aee5e2d735

Comment 71

[Dragana Damjanovic [:dragana]]

Attachment: bug_1230462_v2.patch

I have addressed all comments, except about windows dialog height.
On Linux and Mac the dialog looks just fine. Honza commented that on Windows the dialog is too short so that the buttons are cut. Can you help me fix that?
I was searching for fix but have not succeeded. Do I need to change something in ./toolkit/components/prompts/content/commonDialog* files. I do not see any height limitation.

Comment 72

[Justin Dolske [:Dolske]]

Comment on attachment 8756291 [details] [diff] [review]
bug_1230462_v2.patch

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

This can be fixed by calling window.sizeToContent(). And we should probably already be doing that, I'd sorta suspect there's already a bug here with long HTTP realms.

Somewhat strangely, toolkit/components/prompts/content/selectDialog.js is already doing this, but commonDialog.js is not.

I would add this call to commonDialogOnload(), right between the |Dialog.onLoad(dialog);| and |window.getAttention();|

r+ assuming that's enough to fix the problem. :)

Comment 73

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b7e8ce3562

Comment 74

[Dragana Damjanovic [:dragana]]

(In reply to Dragana Damjanovic [:dragana] from comment #73)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b7e8ce3562

Honza, sorry for bothering you, you saw that dialog on widows is cut (comment #67). I could not reproduce it. Can you retest build from the try build from comment 73 (I have added window.sizeToContent())? Thanks.

Comment 75

[Honza Bambas (:mayhemer)]

Attachment: cut-auth-dialog-1.png

This is what I get with your test build.

Comment 76

[Honza Bambas (:mayhemer)]

OK, I think I've spent already too much time on this.

The problem is for me 100% reproducible on two different Windows 10 machines.  Someone who know our UI code have to take a look at this.  If irreproducible, please just land and let's how this goes in the wild.

Comment 77

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #76)
> OK, I think I've spent already too much time on this.
> 
> The problem is for me 100% reproducible on two different Windows 10
> machines.  Someone who know our UI code have to take a look at this.  If
> irreproducible, please just land and let's how this goes in the wild.

Thanks,
I am going to land this and open another bug for the issue, and put it in the right module.
Probably they will need to know resolution and DPI, but we can do that in the other bug.
As a note, I have try to reproduce this on Win7 with different resolution and DPI, and also cople of people on #introduction tried it as well on WinXP and Win10, nobody could reproduce it.

Comment 78

[Dragana Damjanovic [:dragana]]

Attachment: bug_1230462_v2.patch

Comment 79

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/490d3460a197
Change the authentiation dialog message. r=mayhemer,dolske,margaret,tanvi

Comment 80

[Carsten Book [:Tomcat]]

seems this caused https://treeherder.mozilla.org/logviewer.html#?job_id=29503435&repo=mozilla-inbound - could you take a look, thanks !

Comment 81

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9213f375a3f
Backed out changeset 490d3460a197 for test failures in test_modal_prompts.html

Comment 82

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aebf0c8cc2d

Comment 83

[Dragana Damjanovic [:dragana]]

Attachment: bug_1230462_v2.patch

Only test fixed.

Comment 84

[Dragana Damjanovic [:dragana]]

Sorry only test needed a fix.

Comment 85

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3278ed4eb3
Change the authentiation dialog message. r=mayhemer,dolske,margaret,tanvi

Comment 86

[Carsten Book [:Tomcat]]

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

Comment 87

[Justin Dolske [:Dolske]]

(In reply to Justin Dolske [:Dolske] from comment #72)

> I would add this call to commonDialogOnload(), right between the
> |Dialog.onLoad(dialog);| and |window.getAttention();|
> 
> r+ assuming that's enough to fix the problem. :)

(In reply to Dragana Damjanovic [:dragana] from comment #77)
> Thanks,
> I am going to land this and open another bug for the issue, and put it in
> the right module.

For the record - my review was conditional on this actually fixing the problem, so it's not great to be rushing to land a patch where it's unclear if the problem was actually fixed. Especially right before an uplift, but thankfully this missed the Aurora merge.

As it turns out, it's a little unclear if anyone other than Honza can reproduce this, so I don't think it's worth backing out. But you do need to drive the followup to a conclusion -- it's not acceptable to ship a regression like this.