Closed Bug 1238382 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
firefox45 --- verified
firefox46 --- verified
fennec 45+ ---

People

(Reporter: aryx, Assigned: past)

References

Details

Attachments

(4 files, 2 obsolete files)

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.
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)
Depends on: 1238384
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)
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)
Flags: needinfo?(jmoradi)
Duplicate of this bug: 1238384
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+
(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.
Attached image full-message.jpg
This is what it looks like with my suggestion.
Attached image error-code.jpg
This is what it looks like when using the error code.
Attached patch Aurora patch, option 1 (obsolete) — Splinter Review
Attachment #8710529 - Flags: review?(margaret.leibovic)
Attached patch Aurora patch, option 2 (obsolete) — Splinter Review
I'm posting both versions to speed things up, so you can choose the one you prefer.
Attachment #8710531 - Flags: review?(margaret.leibovic)
The only difference is this hunk in the second patch:

+        if (err == "sslv3Used" || err == "weakCryptoUsed") {
+          errDesc.textContent = err;
+        }
+
tracking-fennec: --- → 45+
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 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-
https://hg.mozilla.org/mozilla-central/rev/1292ce950e07
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Attachment #8710531 - Attachment is obsolete: true
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)
Here is the approved patch without the spurious whitespace change.
Attachment #8711576 - Flags: review+
Attachment #8710529 - Attachment is obsolete: true
Attachment #8710529 - Flags: approval-mozilla-aurora?
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
Attachment #8711576 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
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
See Also: → 1324204
You need to log in before you can comment on or make changes to this bug.