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

VERIFIED FIXED in Firefox 45

Status

()

Firefox for Android
General
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: aryx, Assigned: past)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 verified, firefox46 verified, fennec45+)

Details

Attachments

(4 attachments, 2 obsolete attachments)

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

2 years ago
This looks like a regression from bug 1207146.

That patch is missing changes to the mobile versions of these pages.
Blocks: 1207146
Flags: needinfo?(past)
Flags: needinfo?(jmoradi)

Updated

2 years ago
Depends on: 1238384
(Assignee)

Comment 2

2 years ago
This does sound like a regression from bug 1207146. I'll work on a fix ASAP.
Assignee: nobody → past
Status: NEW → ASSIGNED
Flags: needinfo?(past)
(Assignee)

Comment 3

2 years ago
Created attachment 8709920 [details] [diff] [review]
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.
Attachment #8709920 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

2 years ago
Flags: needinfo?(jmoradi)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1238384

Comment 5

2 years ago
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?
Attachment #8709920 - Flags: review?(margaret.leibovic) → review+

Updated

2 years ago
status-firefox45: --- → affected

Comment 6

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/1292ce950e07
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
Created attachment 8710527 [details]
full-message.jpg

This is what it looks like with my suggestion.
(Assignee)

Comment 9

2 years ago
Created attachment 8710528 [details]
error-code.jpg

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

Comment 10

2 years ago
Created attachment 8710529 [details] [diff] [review]
Aurora patch, option 1
Attachment #8710529 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 11

2 years ago
Created attachment 8710531 [details] [diff] [review]
Aurora patch, option 2

I'm posting both versions to speed things up, so you can choose the one you prefer.
Attachment #8710531 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 12

2 years ago
The only difference is this hunk in the second patch:

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

Updated

2 years ago
tracking-fennec: --- → 45+

Comment 13

2 years ago
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.
Attachment #8710529 - Flags: review?(margaret.leibovic) → review+

Comment 14

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

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

Let's use the other patch.
Attachment #8710531 - Flags: review?(margaret.leibovic) → review-

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1292ce950e07
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(Assignee)

Updated

2 years ago
Attachment #8710531 - Attachment is obsolete: true
(Assignee)

Comment 16

2 years ago
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
Attachment #8710529 - Flags: approval-mozilla-aurora?
Attachment #8710529 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Flags: needinfo?(francesco.lodolo)
Attachment #8710529 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
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 :)
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.
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 20

2 years ago
Created attachment 8711576 [details] [diff] [review]
Aurora patch, option 1

Here is the approved patch without the spurious whitespace change.
Attachment #8711576 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8710529 - Attachment is obsolete: true
Attachment #8710529 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 21

2 years ago
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
Attachment #8711576 - Flags: approval-mozilla-aurora?
(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
status-firefox45: affected → fixed
Attachment #8711576 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

2 years ago
Depends on: 1241478
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)
status-firefox46: fixed → verified
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
Status: RESOLVED → VERIFIED
status-firefox45: fixed → verified

Updated

a year ago
See Also: → bug 1324204
You need to log in before you can comment on or make changes to this bug.