1096695 - update error page for disabled sslv3 case

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

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

See bug 1089808. We need to fix this on beta, which is where we're disabling SSLv3. We can't change strings there, so let's do this for en-US only with hardcoded strings.

Comment 1

[:Gijs (he/him)]

Attachment: hardcode e10s strings for beta to give more info about sslv3 being dead,

This patch should work for beta. I've updated strings only on en-US, and replaced the button with a 'learn more' one. Note that I had to hack docshell in order for the error page to know about which locale we're using, because AFAIK about:neterror is unprivileged... Boris, does this look... if not good, at least acceptable to you? :-)

Comment 2

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

Comment on attachment 8520923 [details] [diff] [review]
hardcode e10s strings for beta to give more info about sslv3 being dead,

>diff --git a/browser/base/content/aboutneterror/netError.xhtml b/browser/base/content/aboutneterror/netError.xhtml

>+      function getLocale()

>+        console.log(url, matches);

Should remove this. 

>+          var learnMoreBtn = document.createElement("button");
>+          learnMoreBtn.id = "errorTryAgain";

Shouldn't this have a unique ID? Or is there some reason to use the same ID?

>+          learnMoreBtn.setAttribute("autocomplete", "off");

Huh?

>+          learnMoreBtn.addEventListener("click", function(e) {
>+            try {
>+              location.href = "https://blog.mozilla.org/security/2014/10/14/the-poodle-attack-and-the-end-of-ssl-3-0/";
>+            } catch (ex) {/* Avoid breaking in case this throws weird errors */}

Not sure what weird errors this could throw, and even if it did it seems like it wouldn't really break anything significant.

>+            e.target.disabled = true;

This doesn't seem necessary?

Comment 3

[:Gijs (he/him)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #2)
> Comment on attachment 8520923 [details] [diff] [review]
> hardcode e10s strings for beta to give more info about sslv3 being dead,
> 
> >diff --git a/browser/base/content/aboutneterror/netError.xhtml b/browser/base/content/aboutneterror/netError.xhtml
> 
> >+      function getLocale()
> 
> >+        console.log(url, matches);
> 
> Should remove this. 

D'oh.

> >+          var learnMoreBtn = document.createElement("button");
> >+          learnMoreBtn.id = "errorTryAgain";
> 
> Shouldn't this have a unique ID? Or is there some reason to use the same ID?

Styling:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/aboutNetError.css#46

> 
> >+          learnMoreBtn.setAttribute("autocomplete", "off");
> 
> Huh?
> 
> >+          learnMoreBtn.addEventListener("click", function(e) {
> >+            try {
> >+              location.href = "https://blog.mozilla.org/security/2014/10/14/the-poodle-attack-and-the-end-of-ssl-3-0/";
> >+            } catch (ex) {/* Avoid breaking in case this throws weird errors */}
> 
> Not sure what weird errors this could throw, and even if it did it seems
> like it wouldn't really break anything significant.
> 
> >+            e.target.disabled = true;
> 
> This doesn't seem necessary?

All three of these match the existing code, see:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutNetError.xhtml#76

and

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutNetError.xhtml#417

. I have 0 clue as to why this has autocomplete="off", nor am I really too sure what errors location.href = "foo" is going to throw, but considering this is last-beta, I am going with "safe rather than sorry".

Please let me know if you still want me to change anything besides the console.log.

Axel suggested going with pre-existing entities for learn more, and/or trying to update other messages based on pre-existing things. I guess that would not take me very long to do (although it'd have to be tomorrow morning). Please let me know if you think that's worth it.

Comment 4

[:Gijs (he/him)]

Attachment: hardcode e10s strings for beta to give more info about sslv3 being dead,

Comment 5

[:Gijs (he/him)]

Attachment: Screenshot of new page

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8521648 [details] [diff] [review]
hardcode e10s strings for beta to give more info about sslv3 being dead,

>+        var matches = url.match(/l\=([^&]+)\&/);

Is there a reason not to use URL/URLSearchParams here?  Seems like it would be safer/clearer.

r=me with that, I think.

Comment 7

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

Comment on attachment 8521648 [details] [diff] [review]
hardcode e10s strings for beta to give more info about sslv3 being dead,

>diff --git a/browser/base/content/aboutneterror/netError.xhtml b/browser/base/content/aboutneterror/netError.xhtml

>+        if (err == "nssFailure2" && getLocale() == "en-US" &&
>+            sd.textContent.contains("ssl_error_no_cypher_overlap")) {
>+          errTitle.textContent = "Unable to Connect Securely";
>+          sd.textContent = "Firefox cannot guarantee the safety of your data on " +
>+            location.hostname + " because it uses SSLv3, a broken security protocol.";
>+          errDesc.textContent = "Advanced info: ssl_error_no_cypher_overlap";

It bothers me a little bit that this is being definitive about it being SSLv3, but we can run into this in other cases. I think the alternative (waffle on whether it's SSLv3 or some other issue) is probably much less clear, and hopefully this will only be seen by users affected by SSLv3 compat issues 99% of the time. We'll fix it up properly in later versions.

>+            location.href = "https://support.mozilla.org/1/firefox/34.0/Darwin/en-US/sslv3-error-messages";

We shouldn't hardcode this Mac-specific version - let's use this version instead:

https://support.mozilla.org/kb/how-resolve-sslv3-error-messages-firefox

I mentioned on IRC that we should get the icon from the UX bug in here as well, if possible.

Comment 8

[:Gijs (he/him)]

Attachment: hardcode e10s strings for beta to give more info about sslv3 being dead,

NB, bz, because this is beta, and because the code is copiecause I copied the regex from the existing code (and altered for the name of the query param, of course), I'd like to leave it as-is for beta/aurora... I'll update all the code here to use URLSearchParams when I do a 'proper' trunk patch. Gavin, this should have the new icon and the updated url.

Comment 9

[:Gijs (he/him)]

Typing comments before the previous qref is finished leads to messed up bzexport comments. :-(

Fixed:

(In reply to :Gijs Kruitbosch from comment #8)
> Created attachment 8521795 [details] [diff] [review]
> hardcode e10s strings for beta to give more info about sslv3 being dead,
> 
> NB, bz, because this is beta, and because I copied
> the regex from the existing code (and altered for the name of the query
> param, of course), I'd like to leave it as-is for beta/aurora... I'll update
> all the code here to use URLSearchParams when I do a 'proper' trunk patch.
> Gavin, this should have the new icon and the updated url.

Comment 10

[:Gijs (he/him)]

Attachment: Screenshot of new page w/ new icon

Comment 11

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

Comment on attachment 8521795 [details] [diff] [review]
hardcode e10s strings for beta to give more info about sslv3 being dead,

Can you make sure to test a non-en-US build variant to make sure that is all working correctly? Should be ready for approval requests.

Comment 12

[:Gijs (he/him)]

Attachment: Screenshot of same build when using German

In case it's not obvious, yes, those are the old strings.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8521795 [details] [diff] [review]
hardcode e10s strings for beta to give more info about sslv3 being dead,

Approval Request Comment
[Feature/regressing bug #]: poodle/sslv3-disabling
[User impact if declined]: people on en-us will be very confused when they can't access websites they can normally access
[Describe test coverage new/current, TBPL]: some generic about:neterror testing
[Risks and why]: low, only changes things in very specific circumstances
[String/UUID change made/needed]: no, hardcoded strings used only if user's locale is set to en-US (and en-US is actually available and in use)

Comment 14

[:Gijs (he/him)]

Attachment: Patch for checkin on beta

Fixed a booboo in the commit msg.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8521821 [details] [diff] [review]
Patch for checkin on beta

Approval Request Comment
[Feature/regressing bug #]: poodle/sslv3-disabling
[User impact if declined]: people on en-us will be very confused when they can't access websites they can normally access
[Describe test coverage new/current, TBPL]: some generic about:neterror testing
[Risks and why]: low, only changes things in very specific circumstances
[String/UUID change made/needed]: no, hardcoded strings used only if user's locale is set to en-US (and en-US is actually available and in use)

Comment 16

[:Gijs (he/him)]

Attachment: Patch for aurora

This was slightly non-trivial because all the about:neterror files moved...

Comment 17

[:Gijs (he/him)]

Comment on attachment 8521823 [details] [diff] [review]
Patch for aurora

This checks out locally, so I'm carrying over r+ and requesting aurora approval, too

[Feature/regressing bug #]: poodle/sslv3-disabling
[User impact if declined]: people on en-us will be very confused when they can't access websites they can normally access
[Describe test coverage new/current, TBPL]: some generic about:neterror testing
[Risks and why]: low, only changes things in very specific circumstances
[String/UUID change made/needed]: no, hardcoded strings used only if user's locale is set to en-US (and en-US is actually available and in use)

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I'll update all the code here to use URLSearchParams when I do a 'proper' trunk patch

Sounds like a plan.

Comment 19

[Masatoshi Kimura [:emk]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #7)
> >+        if (err == "nssFailure2" && getLocale() == "en-US" &&
> >+            sd.textContent.contains("ssl_error_no_cypher_overlap")) {
> >+          errTitle.textContent = "Unable to Connect Securely";
> >+          sd.textContent = "Firefox cannot guarantee the safety of your data on " +
> >+            location.hostname + " because it uses SSLv3, a broken security protocol.";
> >+          errDesc.textContent = "Advanced info: ssl_error_no_cypher_overlap";
> 
> It bothers me a little bit that this is being definitive about it being
> SSLv3, but we can run into this in other cases. I think the alternative
> (waffle on whether it's SSLv3 or some other issue) is probably much less
> clear, and hopefully this will only be seen by users affected by SSLv3
> compat issues 99% of the time. We'll fix it up properly in later versions.

NSS should have provided finer error code...
For example, if we disable RC4 completely, we can't distinguish it from SSLv3 errors.

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8521821 [details] [diff] [review]
Patch for checkin on beta

Although it's late in the cycle, I think this is a useful change that will help reduce the risk of user confusion due to the SSLv3 change in 34. I do wish we would have thought about this option earlier but we'll take this in beta9. Beta+

Comment 21

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/d585e4e50468

Comment 22

[:Gijs (he/him)]

beta test fix:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/117eb4e49c72

Comment 23

[:Gijs (he/him)]

Attachment: Patch for aurora

Comment 24

[:Gijs (he/him)]

Comment on attachment 8522437 [details] [diff] [review]
Patch for aurora

[Feature/regressing bug #]: poodle/sslv3-disabling
[User impact if declined]: people on en-us will be very confused when they can't access websites they can normally access
[Describe test coverage new/current, TBPL]: some generic about:neterror testing
[Risks and why]: low, only changes things in very specific circumstances
[String/UUID change made/needed]: no, hardcoded strings used only if user's locale is set to en-US (and en-US is actually available and in use)

This includes the test fix. Try push on aurora to make sure I don't break a tree again...:

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=719fd7ce360c

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Since this doesn't appear to have landed on trunk, marking affected there - we need this to land on Nightly before Aurora.

Comment 26

[:Gijs (he/him)]

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #25)
> Since this doesn't appear to have landed on trunk, marking affected there -
> we need this to land on Nightly before Aurora.

I don't think we intend to land the current patch on nightly at all - I was planning to fix trunk separately, using actual strings, in bug 1098371. Gavin/Lukas, have I misunderstood something?

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8522437 [details] [diff] [review]
Patch for aurora

That's fine then, thanks for clarifying.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb67769e73f2

Comment 29

[Andrei Vaida [:avaida]]

Verified fixed on:
 * Firefox 34.0b10 (build1, en-US / 20141117202603),
 * Aurora 35.0a2 (en-US / 2014-11-17),
using this page [1] with Windows 7 64-bit, Ubuntu 12.04 LTS 32-bit and Mac OS X 10.9.5. 

There's one minor issue affecting low resolution displays or zoomed in/narrow browser windows: http://i.imgur.com/zEYvwnJ.png. This happens across platforms and it's somewhat similar to Bug 1017458.

I also looked at a few different locales [2] just to confirm that the proper error message is displayed although the new page design isn't.

Gijs, if you think there's more to look at here or I missed something, please let me know.


[1] https://sslv3.dshield.org/
[2] ast, da, de, es-AR, sl, fi, fr, fy-NL