Replace hard-coded support link for SEC_ERROR_UNKNOWN_ISSUER

RESOLVED FIXED in Firefox 50

Status

()

defect
P1
trivial
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: johannh, Assigned: seban, Mentored)

Tracking

(Blocks 1 bug)

unspecified
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

Whiteboard: good first bug → [fxprivacy] [good first bug]
Priority: -- → P3

Comment 1

3 years ago
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 ?
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
Assignee: nobody → shahaakanxu5
Status: NEW → ASSIGNED
Assignee

Comment 3

3 years ago
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?.
Flags: needinfo?(jhofmann)
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!
Flags: needinfo?(jhofmann) → needinfo?(bmo2010)
Aakanxu, I'm unassigning you from the Bug for now. Please let us know if you'd still like to work on it.
Assignee: shahaakanxu5 → nobody
Status: ASSIGNED → NEW
(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.
Flags: needinfo?(bmo2010) → needinfo?(jsavage)
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.
Flags: needinfo?(jsavage) → needinfo?(jhofmann)
Assignee

Comment 8

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

Johann: Replaced hard-coded link. Modified tests also.
Attachment #8768299 - Flags: review?(jhofmann)
Assignee: nobody → sebastinssanty
Flags: needinfo?(jhofmann)
Status: NEW → ASSIGNED
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 :)
Attachment #8768299 - Flags: review?(jhofmann) → review+
Assignee

Comment 10

3 years ago
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?
Attachment #8768299 - Attachment is obsolete: true
Attachment #8768306 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8768306 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Comment 12

3 years ago
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.
Attachment #8768306 - Attachment is obsolete: true
Flags: needinfo?(jhofmann)
Attachment #8768376 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed
Flags: needinfo?(jhofmann)

Comment 14

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/d8e36b37d5cc
Replace hard-coded support link for SEC_ERROR_UNKNOWN_ISSUER r=Gijs
Keywords: checkin-needed

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8e36b37d5cc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.3 - Jul 18
Priority: P3 → P1
You need to log in before you can comment on or make changes to this bug.