832837 - Move "insecure form submission" UI to a toolkit module

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.0 (WIP)

From bug 832834:

* Spin off the "insecure form submission" stuff to a separate toolkit module. There's little reason for it to live deep down in PSM code.

I'm not entirely convinced it's work keeping this (it's pref'd off by default, and pretty annoying to turn on). But neither is it very hard to implement, so maybe I'll just do that. For the first version, I've simply removed it from nsSecureBrowserUIImpl to help clarify other changes for bug 832834.

Comment 1

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

(In reply to Justin Dolske [:Dolske] from comment #0)
> I'm not entirely convinced it's work keeping this (it's pref'd off by
> default, and pretty annoying to turn on).

Is it? I didn't see anything that is conditional on a pref at all. IIRC, I removed all the PSM dialog boxes that were off by default, and I left this one only because it is on by default.

I agree that this code shouldn't live in PSM and I hate prompts but AFAICT this prompt rarely shows up, I guess because it is so annoying that developers try to do the right thing (or, more likely, XHR) to avoid it. Note that the new mixed content blocker will prevent the XHR case so IMO it is somewhat reasonable to keep this prompt to support the mixed content blocker's goals.

Comment 2

[Honza Bambas (:mayhemer)]

Yes, this should be kept.  I usually use "secure login" feature if the page offers it where you send your credentials via form post from a fully secured page.  I really want to know the post has gone through a secure channel and want to STOP it when it shouldn't.

Anyway, won't the new CSP based mixed content blocker stop insecure posts from secure context?

Comment 3

[Justin Dolske [:Dolske]]

I didn't look very closely at things, I was just ripping code out left and right. :) If it's on by default this bug should definitely just move the implementation.

Comment 4

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

Attachment: patch

Since there's only one remaining case where we show one of these dialogs (secure -> insecure form post), I figured it might be worth it to just move it directly into the implementation of the HTML form element (that way, we don't have to add an entirely new toolkit module and make sure it gets properly loaded/hooked up in each product).
I was going to ask bz for feedback, but it looks like he's about to go on vacation, so if you (jst) could take a look, that would be great. Thanks!

Comment 5

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

Attachment: patch rebased

Rebased for bitrot. Also, looks like jst is busy. Blake - would you mind having a look at this? Thanks. (Also, I can push this to reviewboard if you'd prefer that.)

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I'll get to this review/feedback on Monday.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8549785 [details] [diff] [review]
patch rebased

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

f+ on this patch, but I have a few questions I'd like answered before we can land this.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +614,5 @@
>  appmenu.restartBrowserButton.label = Restart %S
>  appmenu.downloadUpdateButton.label = Download Update
> +
> +formPostSecureToInsecureWarning.title = Security Warning
> +formPostSecureToInsecureWarning.message = The information you have entered on this page will be sent over an insecure connection and could be read by a third party.\n\nAre you sure you want to send this information?

Any reason for the slight change in wording?

::: dom/html/HTMLFormElement.cpp
@@ +924,5 @@
> +      MOZ_UTF16("formPostSecureToInsecureWarning.message"),
> +      getter_Copies(message));
> +    bool userWantsToContinue;
> +    rv = promptSvc->Confirm(window, title.get(), message.get(),
> +                            &userWantsToContinue);

I'm not sure, but I think this counts as a usability regression. In particular, it looks like Confirm will use Ok/Cancel for the button text instead of something more appropriate (the old code had Continue/Cancel).

We should probably fix that before landing.

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ -87,5 @@
> -    return NS_OK;
> -  }
> -  
> -  MOZ_ASSERT(NS_IsMainThread());
> -  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI, aBucket);

Do we still care about this probe?

Comment 8

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

Thanks for the feedback!

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #7)
> ::: browser/locales/en-US/chrome/browser/browser.properties
> Any reason for the slight change in wording?

Here's the original:

Although this page is encrypted, the information you have entered is to be sent over an unencrypted connection and could easily be read by a third party.##Are you sure you want to continue sending this information?##

Here's the new version:

The information you have entered on this page will be sent over an insecure connection and could be read by a third 
party.\n\nAre you sure you want to send this information?

Basically, I removed unnecessary phrases and changed jargon into what I thought was more intuitively understandable language. Should I get a UX review on this?

> ::: dom/html/HTMLFormElement.cpp
> @@ +924,5 @@
> > +      MOZ_UTF16("formPostSecureToInsecureWarning.message"),
> > +      getter_Copies(message));
> > +    bool userWantsToContinue;
> > +    rv = promptSvc->Confirm(window, title.get(), message.get(),
> > +                            &userWantsToContinue);
> 
> I'm not sure, but I think this counts as a usability regression. In
> particular, it looks like Confirm will use Ok/Cancel for the button text
> instead of something more appropriate (the old code had Continue/Cancel).
> 
> We should probably fix that before landing.

Ok - I'll fix that.

> ::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
> @@ -87,5 @@
> > -    return NS_OK;
> > -  }
> > -  
> > -  MOZ_ASSERT(NS_IsMainThread());
> > -  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI, aBucket);
> 
> Do we still care about this probe?

I imagine it might be useful to keep it, so I'll add it back in.

Comment 9

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

Attachment: patch v2

See previous comment for responses to feedback on the previous patch.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8553344 [details] [diff] [review]
patch v2

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

I think it's worth getting a UX review on the new wording. This looks great to me. Lots of nice code removal here!

::: dom/html/HTMLFormElement.cpp
@@ +948,5 @@
> +                                   telemetryBucket);
> +    // Return early if the submission is cancelled.
> +    if (*aCancelSubmit) {
> +      return NS_OK;
> +    } else {

Nit: No need for else after if.

Comment 11

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

Attachment: patch v2.1

Thanks for the review! I fixed the else-after-return issue.
Philipp, could you review the string change, please? (see comment 8 for a comparison of the original and the new version)

Comment 12

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=852d10a0fe44

Comment 13

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

Comment on attachment 8554650 [details] [diff] [review]
patch v2.1

You should put the strings in toolkit/locales/en-US/chrome/global/browser.properties instead of browser/locales/en-US/chrome/browser/browser.properties, and adjust the reference in HTMLFormElement accordingly (chrome://browser/locale/browser.properties -> chrome://global/locale/browser.properties).

Comment 14

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

Attachment: before.png

Comment 15

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

Attachment: after.png

Comment 16

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

(In reply to :Gavin Sharp [email: [e-mail]] from comment #13)
> Comment on attachment 8554650 [details] [diff] [review]
> patch v2.1
> 
> You should put the strings in
> toolkit/locales/en-US/chrome/global/browser.properties instead of
> browser/locales/en-US/chrome/browser/browser.properties, and adjust the
> reference in HTMLFormElement accordingly
> (chrome://browser/locale/browser.properties ->
> chrome://global/locale/browser.properties).

Thanks, Gavin. I'll be sure to take care of that.

Philipp, I've attached before and after pictures of the wording. Would you mind having a look when you get a chance? Thanks.

Comment 17

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

Looks good! Cutting some of the words definitely makes this easier to parse.

Comment 18

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

Attachment: patch as landed

Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0ec3afca5a

Comment 19

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

Backed out for breaking things:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41e3793d80e2

Comment 20

[[e-mail]]

(In reply to David Keeler from comment #19)
> Backed out for breaking things

Because the prompt service is now written in JS, confirmEx's inout parameter has to be able to cross XPConnect, in particular you need to initialise it even though it will never be read on the JS side.

Comment 21

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

Attachment: patch refactored

It turns out I was making some incorrect assumptions about how this worked before. In particular, this check only happens if the docshell has an nsISecureBrowserUI, which is only the case on root documents. I modified the refactoring to behave like the original implementation (this is safe because the mixed content blocker takes care of all other such cases of secure -> insecure form submission).

Here's a try run with actual tests this time:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28972a1f155e

If you could have another look (in particular to make sure I'm doing root document detection correctly/safely), that would be great. Thanks!

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8558068 [details] [diff] [review]
patch refactored

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

::: dom/html/HTMLFormElement.cpp
@@ +937,5 @@
> +  stringBundle->GetStringFromName(
> +    MOZ_UTF16("formPostSecureToInsecureWarning.continue"),
> +    getter_Copies(cont));
> +  int32_t buttonPressed;
> +  bool checkState; // this is unused (ConfirmEx requires this parameter)

As Neil pointed out, please initialize this inout parameter.

Comment 23

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

Updated and pushed to inbound. Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/03fed89b215d

Comment 24

[[e-mail]]

(In reply to David Keeler from comment #21)
> It turns out I was making some incorrect assumptions about how this worked
> before. In particular, this check only happens if the docshell has an
> nsISecureBrowserUI, which is only the case on root documents.
(More precisely, root browser documents, but I can't see anyone wanting to submit a form in any other sort of root document.)

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03fed89b215d

Comment 26

[Tanvi Vyas[:tanvi]]

Adding some example testcases:

* https://people.mozilla.org/~tvyas/form_post_http.html - form post to http shows the warning

* https://people.mozilla.org/~tvyas/javascript_form_post_http.html - action set to http in javascript and we get the warning

* https://people.mozilla.org/~dkeeler/tests/testInsecureFormFrame.html
https://people.mozilla.org/~tvyas/frame_insecureform.html
Mixed content blocks insecure form submission because it considers it an navigation of the frame to an http location.

* http://people.mozilla.org/~tvyas/frame_insecureform.html - https iframe inside http page.  no warning for https posting to http because the top level page is http.