Justin Dolske [:Dolske] Reporter
	Description • 12 years ago

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.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 1 • 12 years ago

(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.

	Honza Bambas (:mayhemer)
	Comment 2 • 12 years ago

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?

	Justin Dolske [:Dolske] Reporter
	Comment 3 • 12 years ago

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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 4 • 10 years ago

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!

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 5 • 10 years ago

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.)

	Blake Kaplan (:mrbkap) (inactive)
	Comment 6 • 10 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 7 • 9 years ago

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?

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 8 • 9 years ago

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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 9 • 9 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 10 • 9 years ago

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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 11 • 9 years ago

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)

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 12 • 9 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 13 • 9 years ago

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).

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 16 • 9 years ago

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 17 • 9 years ago

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

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 18 • 9 years ago

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

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 19 • 9 years ago

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

	neil@parkwaycc.co.uk
	Comment 20 • 9 years ago

(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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 21 • 9 years ago

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!

	Blake Kaplan (:mrbkap) (inactive)
	Comment 22 • 9 years ago

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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 23 • 9 years ago

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

	neil@parkwaycc.co.uk
	Comment 24 • 9 years ago

(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.)

	Carsten Book [:Tomcat]
	Comment 25 • 9 years ago

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

	Tanvi Vyas[:tanvi]
	Comment 26 • 9 years ago

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.