Move "insecure form submission" UI to a toolkit module

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Dolske, Assigned: keeler)

Tracking

(Blocks 1 bug)

unspecified
mozilla38
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

6 years ago
Posted patch Patch v.0 (WIP) (obsolete) — Splinter Review
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.
(Reporter)

Updated

6 years ago
Blocks: 832834
(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.
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?
(Reporter)

Comment 3

6 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.
Posted patch patch (obsolete) — Splinter Review
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!
Assignee: nobody → dkeeler
Attachment #704436 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8535860 - Flags: feedback?(jst)
Posted patch patch rebased (obsolete) — Splinter Review
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.)
Attachment #8535860 - Attachment is obsolete: true
Attachment #8535860 - Flags: feedback?(jst)
Attachment #8549785 - Flags: feedback?(mrbkap)
I'll get to this review/feedback on Monday.
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?
Attachment #8549785 - Flags: feedback?(mrbkap) → feedback+
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.
Posted patch patch v2 (obsolete) — Splinter Review
See previous comment for responses to feedback on the previous patch.
Attachment #8549785 - Attachment is obsolete: true
Attachment #8553344 - Flags: review?(mrbkap)
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.
Attachment #8553344 - Flags: review?(mrbkap) → review+
Posted patch patch v2.1 (obsolete) — Splinter Review
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)
Attachment #8553344 - Attachment is obsolete: true
Attachment #8554650 - Flags: ui-review?(philipp)
Attachment #8554650 - Flags: review+
See Also: → 616849
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).
(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.
Flags: needinfo?(philipp)
Looks good! Cutting some of the words definitely makes this easier to parse.
Flags: needinfo?(philipp)
(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.
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!
Attachment #8557221 - Attachment is obsolete: true
Attachment #8558068 - Flags: review?(mrbkap)
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.
Attachment #8558068 - Flags: review?(mrbkap) → review+
(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.)
https://hg.mozilla.org/mozilla-central/rev/03fed89b215d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.