Closed Bug 1096695 Opened 5 years ago Closed 5 years ago

update error page for disabled sslv3 case

Categories

(Firefox :: General, defect)

defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
36.3
Tracking Status
firefox34 + verified
firefox35 + verified
firefox36 --- wontfix

People

(Reporter: Gavin, Assigned: Gijs)

References

Details

Attachments

(4 files, 5 obsolete files)

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.
Flags: qe-verify+
Flags: firefox-backlog+
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 36.3
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? :-)
Attachment #8520923 - Flags: review?(gavin.sharp)
Attachment #8520923 - Flags: review?(bzbarsky)
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?
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
Depends on: 1089808
Attachment #8521648 - Flags: review?(gavin.sharp)
Attachment #8521648 - Flags: review?(bzbarsky)
Attachment #8520923 - Attachment is obsolete: true
Attachment #8520923 - Flags: review?(gavin.sharp)
Attachment #8520923 - Flags: review?(bzbarsky)
Attached image Screenshot of new page (obsolete) —
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.
Attachment #8521648 - Flags: review?(bzbarsky) → review+
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.
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.
Attachment #8521795 - Flags: review?(gavin.sharp)
Attachment #8521648 - Attachment is obsolete: true
Attachment #8521648 - Flags: review?(gavin.sharp)
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.
Attachment #8521742 - Attachment is obsolete: true
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.
Attachment #8521795 - Flags: review?(gavin.sharp) → review+
In case it's not obvious, yes, those are the old strings.
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)
Attachment #8521795 - Flags: approval-mozilla-beta?
Fixed a booboo in the commit msg.
Attachment #8521795 - Attachment is obsolete: true
Attachment #8521795 - Flags: approval-mozilla-beta?
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)
Attachment #8521821 - Flags: approval-mozilla-beta?
Attached patch Patch for aurora (obsolete) — Splinter Review
This was slightly non-trivial because all the about:neterror files moved...
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)
Attachment #8521823 - Flags: review+
Attachment #8521823 - Flags: approval-mozilla-aurora?
> I'll update all the code here to use URLSearchParams when I do a 'proper' trunk patch

Sounds like a plan.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
Depends on: 1084986
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+
Attachment #8521821 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached patch Patch for auroraSplinter Review
Attachment #8521823 - Attachment is obsolete: true
Attachment #8521823 - Flags: approval-mozilla-aurora?
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
Attachment #8522437 - Flags: approval-mozilla-aurora?
Since this doesn't appear to have landed on trunk, marking affected there - we need this to land on Nightly before Aurora.
(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?
Depends on: 1098371
Flags: needinfo?(lsblakk)
Flags: needinfo?(gavin.sharp)
Comment on attachment 8522437 [details] [diff] [review]
Patch for aurora

That's fine then, thanks for clarifying.
Flags: needinfo?(lsblakk)
Attachment #8522437 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer depends on: 1084986
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb67769e73f2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(gavin.sharp)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Depends on: 1099123
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
Status: RESOLVED → VERIFIED
QA Contact: andrei.vaida
Depends on: 1113780
You need to log in before you can comment on or make changes to this bug.