1275896 - Replace hard-coded support link for SEC_ERROR_UNKNOWN_ISSUER

Status: RESOLVED FIXED

(Firefox :: Security, defect, P1)

Description

[Johann Hofmann [:johannh]]

In https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#279 we hard-code the SUMO URL for the "Learn more" link.

Bug 1275788 tells us that's not ideal. The correct way to do it can be seen here: https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#29

Comment 1

[Aakanxu Shah]

Hi ! If this is still up for grabs , I would like to work on this bug. 
Do I need to setup/build Desktop Firefox or can I request changes directly to Mozilla-central tree file - content.js ?

And do I need to make the following change in the code? :

Original Code in Switch case-
let learnMoreLink = content.document.getElementById("learnMoreLink");
learnMoreLink.href = "https://support.mozilla.org/kb/troubleshoot-SEC_ERROR_UNKNOWN_ISSUER";

Updated Code-
let learnMoreLink = content.document.getElementById("learnMoreLink");
let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "troubleshoot-SEC_ERROR_UNKNOW_ISSUER";
learnMoreLink.setAttribute("href", url);



How can I safely make changes to this file ?

Comment 2

[Johann Hofmann [:johannh]]

Hey there, thanks for contributing!

Your proposed solution looks good to me. I would recommend you to get a Firefox build running [0] and try if it works as expected.

You can test a page with SEC_ERROR_UNKNOW_ISSUER here: https://self-signed.badssl.com/

If you have _any_ questions or run into a problem you can ask here or ping me (johannh) on IRC [1].

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
[1] https://wiki.mozilla.org/IRC

Comment 3

[Sebastin Santy [:seban]]

Hi Johann,

I was just looking through the bug. I logged the url in the console and found that the url was actually going to "https://support.mozilla.org/1/firefox/50.0a1/Darwin/en-US/tracking-protection" and then getting redirected to "https://support.mozilla.org/en-US/kb/tracking-protection-firefox". In the case of "troubleshoot-SEC_ERROR_UNKNOW_ISSUER" there was no redirection at the Darwin URL to the KB URL and hence was showing an error. Any ideas of how to call the KB URL directly instead?.

Comment 4

[Johann Hofmann [:johannh]]

Hey Sebastin!

Yes, good question. Chris, do you know why that link is not working? (https://support.mozilla.org/1/firefox/42.1/osx/en-US/troubleshoot-SEC_ERROR_UNKNOWN_ISSUER)

If you'd like to tackle a very similar problem while we wait for this one to resolve, Bug 1282455 is also up for grabs! I checked that the link works this time ;)

Thanks for contributing!

Comment 5

[Johann Hofmann [:johannh]]

Aakanxu, I'm unassigning you from the Bug for now. Please let us know if you'd still like to work on it.

Comment 6

[Chris Ilias [:cilias]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> Hey Sebastin!
> 
> Yes, good question. Chris, do you know why that link is not working?
> (https://support.mozilla.org/1/firefox/42.1/osx/en-US/troubleshoot-
> SEC_ERROR_UNKNOWN_ISSUER)

"troubleshoot-SEC_ERROR_UNKNOWN_ISSUER" is not a help-topic; it's the article slug. We need a sumo admin to set up the help-topic redirect.

Comment 7

[Joni Chan]

Thanks for the needinfo, cilias! I've set up the redirect. (An aside: For more context on how we work with in-product links, please see https://support.mozilla.org/en-US/kb/a-guide-to-linking-to-support-articles.) 

Johannh and Sebastin, please try this URL: https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/security-error

It should redirect to that knowledge base article now, but please let me know if it's still not working.

Comment 8

[Sebastin Santy [:seban]]

Attachment: bug1275896_hardcodedlearnlink.diff

Joni: The link is working perfectly fine. Thanks for setting up the redirect.

Johann: Replaced hard-coded link. Modified tests also.

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8768299 [details] [diff] [review]
bug1275896_hardcodedlearnlink.diff

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

Looks good. Two nits about missing semicolons.

When you upload the fixed patch, please request another review from e.g. :Gijs, since we need a Firefox peer to sign it off.

Thanks!

::: browser/base/content/content.js
@@ +274,4 @@
>      let div = content.document.getElementById("certificateErrorText");
>      div.textContent = msg.data.info;
>      let learnMoreLink = content.document.getElementById("learnMoreLink");
> +    let url = Services.urlFormatter.formatURLPref("app.support.baseURL")

nit: missing semicolon

@@ +278,4 @@
>  
>      switch (msg.data.code) {
>        case SEC_ERROR_UNKNOWN_ISSUER:
> +        learnMoreLink.setAttribute("href", url  + "security-error")

another missing semicolon, you can run ./mach eslint path/to/your/file to check for style errors :)

Comment 10

[Sebastin Santy [:seban]]

Attachment: bug1275896_hardcodedlearnlink.diff

Johann: I had tested with eslint. Surprisingly didn't show any errors. I have put the semicolon.

Gijs: Hi Gijs, Can you please review this?

Comment 11

[:Gijs (he/him)]

Comment on attachment 8768306 [details] [diff] [review]
bug1275896_hardcodedlearnlink.diff

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

r=me with the nits below fixed.

::: browser/base/content/content.js
@@ +274,4 @@
>      let div = content.document.getElementById("certificateErrorText");
>      div.textContent = msg.data.info;
>      let learnMoreLink = content.document.getElementById("learnMoreLink");
> +    let url = Services.urlFormatter.formatURLPref("app.support.baseURL");

Nit: I'd call it "baseURL" like the pref you're getting this from.

@@ +278,4 @@
>  
>      switch (msg.data.code) {
>        case SEC_ERROR_UNKNOWN_ISSUER:
> +        learnMoreLink.setAttribute("href", url  + "security-error");

Nit: it should be possible to use the property here like the code was already doing:

learnMoreLink.href = baseURL + "security-error";

(and the same below for the time errors.)

::: browser/base/content/test/general/browser_aboutCertError.js
@@ +356,4 @@
>      let learnMoreLink = content.document.getElementById("learnMoreLink");
>      return learnMoreLink.href;
>    });
> +  ok(href.includes("security-error"), "security-error in the Learn More URL");

Nit: href.endsWith

Comment 12

[Sebastin Santy [:seban]]

Attachment: bug1275896_hardcodedlearnlink.diff

Thanks Gijs for the review. I have made the changes.

Johann: Please can you help me with pushing it into the try. Thanks in advance.

Comment 13

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=439e77264a51

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/d8e36b37d5cc
Replace hard-coded support link for SEC_ERROR_UNKNOWN_ISSUER r=Gijs

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/d8e36b37d5cc