Closed Bug 1282455 Opened 8 years ago Closed 8 years ago

Use dedicated "Learn more..." link on time related cert error pages

Categories

(Core :: Security: PSM, defect, P1)

48 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla50
Iteration:
50.3 - Jul 18
Tracking Status
firefox50 --- verified

People

(Reporter: philipp, Assigned: seban, Mentored)

References

Details

(Whiteboard: [fxprivacy] [good first bug])

Attachments

(1 file, 3 obsolete files)

similar like in bug 1242886, i would request that on "Your Connection is not Secure" error pages for time-related issues we use a "learn more" link that leads users to a dedicated article explaining amongst other things how to fix the system clock so that there is a greater chance that affected users are able to easily solve the issue.

this would be related to the following error codes (which make up ~14% of all cert errors according to telemetry):
SEC_ERROR_EXPIRED_CERTIFICATE
SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE
SEC_ERROR_OCSP_FUTURE_RESPONSE
SEC_ERROR_OCSP_OLD_RESPONSE
MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE
MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE

we would have the following article in place for this on sumo:
https://support.mozilla.org/kb/troubleshoot-time-errors-secure-websites
hi, could i ask you to help with this bug again? thank you...
Flags: needinfo?(jhofmann)
Of course! This would be an excellent good first bug.

I'll add some instructions tomorrow.
Mentor: jhofmann
Flags: needinfo?(jhofmann)
Priority: -- → P3
Whiteboard: [fxprivacy] [good first bug]
Philip, in the future please remember to CC the Sumo content manager if you want to add an in-product link. The name of the article has not been reviewed, and is liable to change, and the help-topic string has to be linked to the article by an admin.
Flags: needinfo?(jsavage)
Ok, can you leave a comment once this is sorted out? :)
Whiteboard: [fxprivacy] [good first bug] → [fxprivacy]
Thanks for the need info, Chris, and to Philipp for filing this bug.

Johann, please use this as the in-product link: https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/time-errors

This will redirect to the article that Philipp created. This way, we can change the article link on our end without any code changes.
Flags: needinfo?(jsavage)
Thanks for the link Joni!

Here's how to solve this:

The file to change is https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js.

We're already modifying the "Learn More" link in that file: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#279. We'll need to do almost the same thing in the switch statement below. 

Almost, because that is not the entirely correct way of doing it. Instead of hardcoding the URL we should generate it from the baseURL, like it's done here: https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#29.

The keyword you'll have to append to the generated URL is "time-errors".

You can check if the "Learn More" link changed correctly by visiting https://expired.badssl.com/

There's also Bug 1275896 to solve the other hardcoded URL.
Whiteboard: [fxprivacy] → [fxprivacy] [good first bug]
Hi Johann,

I would like to work on this bug. Can you please assign this to me?
Assignee: nobody → sebastinssanty
Status: NEW → ASSIGNED
Hi Johann,

Thanks for assigning the bug to me. I have added the dedicated time-related cert error page link in the patch. Can you please review this?
Attachment #8767459 - Flags: review?(jhofmann)
Comment on attachment 8767459 [details] [diff] [review]
bug1282455_learnlink_for_timerelated_errors.diff

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

This looks good overall, but we need some polish to make it perfect :)

It'd be great if you could add a small assertion that "learnMoreLink" has the correct URL to this test: http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_aboutCertError.js#110

Maybe you shouldn't test the full url (as that will change with the system it's running on), but only if the url includes "time-errors".

You can find more info on mochitesting here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest

Mochitests can be a bit complicated at first, so don't hesitate to reach out to me via IRC or in Bugzilla.

::: browser/base/content/content.js
@@ +286,4 @@
>        case SEC_ERROR_OCSP_FUTURE_RESPONSE:
>        case SEC_ERROR_OCSP_OLD_RESPONSE:
>        case MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE:
> +      case MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE:

This is being handled in Bug 1281129. Could you remove this line and upload a separate patch there instead? Thanks!

Note that you will also have to add the constant in this line http://searchfox.org/mozilla-central/source/browser/base/content/content.js#232. The value is MOZILLA_PKIX_ERROR_BASE + 6.

@@ +312,5 @@
>                .style.display = "block";
> +
> +              let learnMoreLink = content.document.getElementById("learnMoreLink");
> +              let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "time-errors";
> +              learnMoreLink.setAttribute("href", url);

You can move this outside the if clause. You'll also have to move line 314 ("let learnMoreLink = ...") to above the switch statement and remove it from the other switch clause to get around the resulting "duplicate let assignment" errors.
Attachment #8767459 - Flags: review?(jhofmann) → feedback+
Hi Johann,

Thanks for the review. I have added the tests and made necessary changes. In the Bug1281129, you have mentioned that I add the "MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE" here itself, so I did.
Attachment #8767459 - Attachment is obsolete: true
Attachment #8767677 - Flags: review?(jhofmann)
Comment on attachment 8767677 [details] [diff] [review]
bug1282455_learnlink_for_timerelated_errors.diff

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

::: browser/base/content/content.js
@@ +312,5 @@
>              content.document.getElementById("wrongSystemTimePanel")
>                .style.display = "block";
>            }
> +            let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "time-errors";
> +            learnMoreLink.setAttribute("href", url);

You can go outside this "if" clause as well. We want to show this warning anytime there is a time-related issue, not just when we have clock skew data.

Nit: Please also fix the indentation here. :)
Attachment #8767677 - Flags: review?(jhofmann)
Made the necessary changes. Checked for errors using eslint.
Attachment #8767677 - Attachment is obsolete: true
Attachment #8767690 - Flags: review?(jhofmann)
Comment on attachment 8767690 [details] [diff] [review]
bug1282455_learnlink_for_timerelated_errors.diff

This looks great! Since I'm not technically a peer, we'll need another pair of eyes on this before we're good to go. Gijs, do you have a moment?
Attachment #8767690 - Flags: review?(jhofmann) → review?(gijskruitbosch+bugs)
Comment on attachment 8767690 [details] [diff] [review]
bug1282455_learnlink_for_timerelated_errors.diff

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

This looks OK, with the nit below addressed. Can we file a followup bug to move the setting of the target of this link entirely into content.js (even for other errors) ? Then we can avoid hardcoding the URL in the content page itself, which avoids bug 1275788.

::: browser/base/content/test/general/browser_aboutCertError.js
@@ +125,4 @@
>        let div = doc.getElementById("wrongSystemTimePanel");
>        let systemDateDiv = doc.getElementById("wrongSystemTime_systemDate");
>        let actualDateDiv = doc.getElementById("wrongSystemTime_actualDate");
> +      let learnMoreDiv = doc.getElementById("learnMoreLink");

Nit: can we call this variable "learnMoreLink" as it isn't a <div>? Make sure to also update it below.
Attachment #8767690 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #14)
> Comment on attachment 8767690 [details] [diff] [review]
> bug1282455_learnlink_for_timerelated_errors.diff
> 
> Review of attachment 8767690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks OK, with the nit below addressed. Can we file a followup bug to
> move the setting of the target of this link entirely into content.js (even
> for other errors) ? Then we can avoid hardcoding the URL in the content page
> itself, which avoids bug 1275788.

Sounds like a good idea. I'll look into it and file a bug if that looks feasible! We should also think about extracting AboutNetAndCertErrorListener into its own file.
nits solved. Thanks Gijs for the review and Johann for helping me!
Attachment #8767690 - Attachment is obsolete: true
Attachment #8767723 - Flags: review+
Great! Now this needs a run on our try server, to check if tests are still green. (https://wiki.mozilla.org/ReleaseEngineering/TryServer). I just did that for you, but in the future you might want to consider getting your own try access.

I've also set the checkin-needed flag. This flag signals that we want the patch merged. If the try looks good, someone will come around and check it in for us.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/516c82acb163
Use dedicated "Learn more..." link on time related cert error pages. r=johannh r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/516c82acb163
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
thank you for implementing this!
Iteration: --- → 50.3 - Jul 18
Priority: P3 → P1
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: paul.silaghi
Note that the URL also contains some parameters after the default link:
https://support.mozilla.org/en-US/kb/troubleshoot-time-errors-secure-websites?as=u&utm_source=inproduct
Is that ok?
Flags: needinfo?(sebastinssanty)
Yes, that's intentional; it's added by sumo. :)
https://github.com/mozilla/kitsune/blob/master/kitsune/inproduct/views.py

It tells sumo that visitors arriving from that link are coming from an in-product link.
Thank you!
Verified fixed FX 50.0a1 (2016-07-10) Win 7
Status: RESOLVED → VERIFIED
Flags: needinfo?(sebastinssanty)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: