1238382 - cert error page: Error code link in 'Technical details' shown as html code

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox 46.0a1 and 45.0a2 20160110 on Android 4.1.2

1. Visit https://badssl.com/
2. Open one of the test pages, e.g. wrong.host
3. On the cert error page, expand 'Technical Details'.

Actual result:
Explanation contains
Error code: <a id="errorCode" title="SSL_ERROR_BAD_CERT_DOMAIN">SSL_ERROR_BAD_CERT_DOMAIN</a>

Expected:
No HTML code, either a working link or no link.

Comment 1

[:Margaret Leibovic]

This looks like a regression from bug 1207146.

That patch is missing changes to the mobile versions of these pages.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

This does sound like a regression from bug 1207146. I'll work on a fix ASAP.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Bring Android's about:certerror and about:neterror closer to the desktop versions

This copies just the error code handling changes from bug 1207146 and fixes both this bug and bug 1238384.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8709920 [details] [diff] [review]
Bring Android's about:certerror and about:neterror closer to the desktop versions

Review of attachment 8709920 [details] [diff] [review]:
-----------------------------------------------------------------

This is a pretty rubber stamp-y review. 

I often wish these pages weren't forked, but that would require making sure we have responsive styles that work across both platforms. But maybe we can do that one day!

::: mobile/locales/en-US/overrides/netError.dtd
@@ +207,5 @@
> +
> +<!ENTITY weakCryptoUsed.title "Your connection is not secure">
> +<!-- LOCALIZATION NOTE (weakCryptoUsed.longDesc) - Do not translate
> +     "SSL_ERROR_NO_CYPHER_OVERLAP". -->
> +<!ENTITY weakCryptoUsed.longDesc "Advanced info: SSL_ERROR_NO_CYPHER_OVERLAP">

It's unfortunate that this landed in 45, so we can't uplift these strings... would it be possible to make a slightly different version of this patch for uplift that just says the error code directly?

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1292ce950e07

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Margaret Leibovic from comment #5)
> I often wish these pages weren't forked, but that would require making sure
> we have responsive styles that work across both platforms. But maybe we can
> do that one day!

We have bumped the priority of bug 1220481 and we may be able to get to it in the 47 cycle.

> It's unfortunate that this landed in 45, so we can't uplift these strings...
> would it be possible to make a slightly different version of this patch for
> uplift that just says the error code directly?

This hunk and the relevant changes in netError.xhtml are to fix bug 1238384. One option is to just use the nssFailure2 title and description: the title is a great fit, the description less so. I'll see if I can do what you suggested first.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: full-message.jpg

This is what it looks like with my suggestion.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: error-code.jpg

This is what it looks like when using the error code.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Aurora patch, option 1

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Aurora patch, option 2

I'm posting both versions to speed things up, so you can choose the one you prefer.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

The only difference is this hunk in the second patch:

+        if (err == "sslv3Used" || err == "weakCryptoUsed") {
+          errDesc.textContent = err;
+        }
+

Comment 13

[:Margaret Leibovic]

Comment on attachment 8710529 [details] [diff] [review]
Aurora patch, option 1

Review of attachment 8710529 [details] [diff] [review]:
-----------------------------------------------------------------

Good suggestion! I think this is a much more user-friendly option.

Comment 14

[:Margaret Leibovic]

Comment on attachment 8710531 [details] [diff] [review]
Aurora patch, option 2

Review of attachment 8710531 [details] [diff] [review]:
-----------------------------------------------------------------

Let's use the other patch.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1292ce950e07

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8710529 [details] [diff] [review]
Aurora patch, option 1

Approval Request Comment
[Feature/regressing bug #]: bug 1207146 and bug 1207137
[User impact if declined]: error pages for secure connections will be confusing, containing HTML markup or generic messages
[Describe test coverage new/current, TreeHerder]: manual testing in m-c and aurora
[Risks and why]: the risk is small, as the changes are contained in the error pages that are problematic to begin with
[String/UUID change made/needed]: none

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8710529 [details] [diff] [review]
Aurora patch, option 1

Tomcat pinged me and said that there is a string warning here.

Flod, I guess you don't want me to take this patch? Thanks

Comment 18

[Carsten Book [:Tomcat]]

remote: * File used for localization (mobile/locales/en-US/overrides/netError.dtd) altered in this changeset *
remote: 
remote: This repository is string frozen. Please request explicit permission from
remote: release managers to break string freeze in your bug.


was the warning i got, just in case :)

Comment 19

[Francesco Lodolo [:flod]]

The patch doesn't seem to have string changes, but changes a .dtd removing an empty line.

Either you fix the patch by removing that change, or land it with the l10n=sylvestre flag (which would be a bit awkward, given there are no strings). Either choice is fine by me, the latter is probably faster.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Aurora patch, option 1

Here is the approved patch without the spurious whitespace change.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8711576 [details] [diff] [review]
Aurora patch, option 1

Approval was already granted, but I can't mark it as such, so I'll just go ahead and ask again.

Approval Request Comment
[Feature/regressing bug #]: bug 1207146 and bug 1207137
[User impact if declined]: error pages for secure connections will be confusing, containing HTML markup or generic messages
[Describe test coverage new/current, TreeHerder]: manual testing in m-c and aurora
[Risks and why]: the risk is small, as the changes are contained in the error pages that are problematic to begin with
[String/UUID change made/needed]: none

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Panos Astithas [:past] from comment #21)
> Comment on attachment 8711576 [details] [diff] [review]
> Aurora patch, option 1
> 
> Approval was already granted, but I can't mark it as such, so I'll just go
> ahead and ask again.
> 
> Approval Request Comment
> [Feature/regressing bug #]: bug 1207146 and bug 1207137
> [User impact if declined]: error pages for secure connections will be
> confusing, containing HTML markup or generic messages
> [Describe test coverage new/current, TreeHerder]: manual testing in m-c and
> aurora
> [Risks and why]: the risk is small, as the changes are contained in the
> error pages that are problematic to begin with
> [String/UUID change made/needed]: none


landed as https://hg.mozilla.org/releases/mozilla-aurora/rev/68328ff256b6

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Tapping 'Technical details' will display: "Error code: SSL_ERROR_BAD_CERT_DOMAIN", so:
Verified as fixed using:
Device: One A2001 (Android 5.1.1) 
Build: Firefox for Android 46.0a2 (2016-02-16)

Comment 24

[Teodora Vermesan (:TeoVermesan)]

Tapping 'Technical details' will display: "Error code: SSL_ERROR_BAD_CERT_DOMAIN", so:
Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 45.0b9 build 2