Last Comment Bug 810673 - Deal with removal of SSL-related warning prompts
: Deal with removal of SSL-related warning prompts
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Security (show other bugs)
: unspecified
: All All
: -- normal (vote)
: seamonkey2.16
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 799009 813427
Blocks: 817441
  Show dependency treegraph
 
Reported: 2012-11-11 03:33 PST by neil@parkwaycc.co.uk
Modified: 2012-12-02 14:39 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Insecure form submission prompt, suite glue version (7.22 KB, patch)
2012-11-11 15:39 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Insecure form submission prompt, notificationbox version (7.34 KB, patch)
2012-11-11 16:06 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Possible patch (10.94 KB, patch)
2012-11-12 16:40 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
philip.chee: feedback+
iann_bugzilla: feedback+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2012-11-11 03:33:32 PST
Bug #799009 removed the following prompts:

1. Warning, you are about to enter a secure site
2. Warning, you are about to leave a secure site
3. Warning, you are about to submit a form to an insecure site, when you are already on an insecure site.
4. Warning, you are viewing a site with mixed content.
Comment 1 neil@parkwaycc.co.uk 2012-11-11 15:39:15 PST
Created attachment 680485 [details] [diff] [review]
Insecure form submission prompt, suite glue version
Comment 2 neil@parkwaycc.co.uk 2012-11-11 16:06:47 PST
Created attachment 680488 [details] [diff] [review]
Insecure form submission prompt, notificationbox version
Comment 3 neil@parkwaycc.co.uk 2012-11-12 16:40:29 PST
Created attachment 680870 [details] [diff] [review]
Possible patch

(Includes the insecure form submission prompt, notificationbox version)

This is still an alert but I was thinking that I could convert it into a notification in a followup bug.
Comment 4 Philip Chee 2012-11-17 08:27:22 PST
Comment on attachment 680870 [details] [diff] [review]
Possible patch

> +      <method name="notify">
> +        <parameter name="aFormElement"/>
> +        <parameter name="aWindow"/>
> +        <parameter name="aURI"/>
How about aActionURI ?

(tested on https://bug90392.bugzilla.mozilla.org/attachment.cgi?id=43175)

> +            if (!aFormElement || ! aWindow || !aURI)
Extraneous space.

> +            try {
> +              uri = aFormElement.nodePrincipal.URI;
> +            } catch (e) {}
When does this throw?

WFM so f=me
Comment 5 neil@parkwaycc.co.uk 2012-11-17 11:19:18 PST
(In reply to Philip Chee from comment #4)
> (From update of attachment 680870 [details] [diff] [review])
> > +            try {
> > +              uri = aFormElement.nodePrincipal.URI;
> > +            } catch (e) {}
> When does this throw?
I was just porting the PSM code, it checks the nsresult.
Comment 6 Bruno Harbulot 2012-11-17 13:06:49 PST
Bug 799009 says that the warning about leaving a secure site (#2) is "outdated".

I'm not convinced this is the case. Of course, this warning was effectively useless, since it occurred at a point where the action could no longer be prevented. As such there's no point keeping it indeed. However, having the ability to stop that action would be good (bug 289847).

There are sites out there that pass sensitive information (or authentication tokens) from an HTTPS page to an HTTP page (possibly relying on a subsequent automatic redirection to HTTPS), sometimes in an explicit link, sometimes just by changing window.location.href via JavaScript. Such behaviour should be warned against, and should also be preventable by the user.

Without any browser warnings, and more particularly when automatic redirections from HTTP to HTTPS are present on the site, information leakage can go completely unnoticed (including for the site developer).
Comment 7 neil@parkwaycc.co.uk 2012-11-18 16:09:29 PST
Comment on attachment 680870 [details] [diff] [review]
Possible patch

Bah, why didn't I just request r? in the first place...

Oh yeah, in case you wanted to go with attachment 680485 [details] [diff] [review].

I assume from the f+ that you are happy with this approach.
Comment 8 Ian Neal 2012-11-18 16:45:20 PST
(In reply to neil@parkwaycc.co.uk from comment #7)
> Comment on attachment 680870 [details] [diff] [review]
> Possible patch
> 
> Bah, why didn't I just request r? in the first place...
> 
> Oh yeah, in case you wanted to go with attachment 680485 [details] [diff] [review]
> [review].
> 
> I assume from the f+ that you are happy with this approach.

r=me with the already mentioned nits fixed.
Comment 9 neil@parkwaycc.co.uk 2012-11-20 05:59:46 PST
Pushed comm-central changeset 7a8e57d2fbc2.

Note You need to log in before you can comment on or make changes to this bug.