Closed Bug 1284835 Opened 3 years ago Closed 8 months ago

Move "Learn More..." link changes to NetErrorChild.jsm

Categories

(Firefox :: Security, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 67
Tracking Status
firefox50 --- wontfix
firefox67 --- fixed

People

(Reporter: johannh, Assigned: aqadri, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy] )

Attachments

(3 files)

We assign custom URLs to "learnMoreLink" on several occasions in aboutNetError.xhtml. (http://searchfox.org/mozilla-central/search?q=support.mozilla&case=false&regexp=false&path=aboutneterror.xhtml)

As Bug 1275788 tells us, these URLs should be generated using Services.urlFormatter.formatURLPref instead. We already do this in content.js for some links (http://searchfox.org/mozilla-central/source/browser/base/content/content.js#272).

We can't do it in aboutNetError.xhtml, however, so we should move all these assignments and the checks for their respective conditions to content.js.
Hi Johann,

Are there urls, to test them? (Like expired.badssl.com)
Flags: needinfo?(jhofmann)
Hi seban! Thanks for your interest in this. Fair warning: this might be a little more challenging.

You can find Mochitest URLs for testing sslv3Used at https://dxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt#259. If you want to play around with your Mochitest browser you can add the --keep-open option to ./mach mochitest. That way you can visit these URLs without having to run a test every time.

You can use https://rc4.badssl.com/ for testing WeakCrypto. Note that you have to set "security.tls.unrestricted_rc4_fallback" to true in about:config to show the correct error for rc4. For me the correct warning didn't show on this one though, I'll try to find out why.

You can test mozilla_pkix_error_key_pinning_failure at https://pinning-test.badssl.com/. Note that the current implementation is broken, it should check for all uppercase (MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE) instead of lowercase.

Important: you can't add these errors to the same location as in the previous bugs, because these belong to what used to be the neterror page, not the certerror page. For now it should do to add them to onPageLoad (http://searchfox.org/mozilla-central/source/browser/base/content/content.js#350). You might also have to copy the getError() and getDescription() to AboutNetAndCertErrorListener.

I generally get the impression that the NetError/CertError pages and friends could use a bit of refactoring, though.
Flags: needinfo?(jhofmann)
Thinking about this again, it seems inefficient to simply dump and duplicate code into content.js. If you feel adventurous, please look into this, but I fear there is too much here that needs cleaning up before we can start this bug in earnest. I'll look into this, sorry for that.
Hi Johann,

Thanks for the info. I'm looking into it. But I hope I may be able to solve this (I guess).
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Assignee: prathikshaprasadsuman → nobody
Status: ASSIGNED → NEW
This looks interesting! Are you still looking for someone to work on this, because I think I could look into it?
That would be a great help!
Assignee: nobody → guptatrisha97
Status: NEW → ASSIGNED
Summary: Move "Learn More..." link changes to content.js → Move "Learn More..." link changes to NetErrorChild.jsm
Attached patch bug1284835.patchSplinter Review
Hey, is this essentially what we're looking to do in this bug? I am guessing we remove the hard-coded sumo URIs from code in Bug 1275788? I'll be happy to get feedback so I know how to proceed with this. Thanks!
Attachment #9012472 - Flags: feedback?(jhofmann)
Comment on attachment 9012472 [details] [diff] [review]
bug1284835.patch

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

From quick inspection that looks good to me, but please do remove the hardcoded URLs in this patch. Bug 1275788 is just tracking the removal of all hardcoded SUMO URLs.

::: browser/actors/NetErrorChild.jsm
@@ +352,5 @@
>      let errWhatToDo = doc.getElementById("es_nssBadCert_" + msg.data.codeString);
>      let es = doc.getElementById("errorWhatToDoText");
>      let errWhatToDoTitle = doc.getElementById("edd_nssBadCert");
>      let est = doc.getElementById("errorWhatToDoTitleText");
> +    let searchParams = new URLSearchParams(doc.documentURI.split("?")[1]);

It would be great if eventually we didn't have to extract this from the documentURI anymore, but for now this should work :)
Attachment #9012472 - Flags: feedback?(jhofmann) → feedback+
Assignee: guptatrisha97 → nobody
Status: ASSIGNED → NEW

Hey, Can I take up this issue?

(In reply to Syeda Asra Arshia Qadri [:aqadri] from comment #9)

Hey, Can I take up this issue?

Hi aqadri, absolutely, that would be great. Please take a look at Trisha's patch and replace the hardcoded URLs with Services.urlFormatter.formatURLPref as done here for example: https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/browser/actors/BlockedSiteChild.jsm#105

I can assign you once you have a new patch for it. Let me know if you need any help!

(In reply to Johann Hofmann [:johannh] from comment #10)

(In reply to Syeda Asra Arshia Qadri [:aqadri] from comment #9)

Hey, Can I take up this issue?

Hi aqadri, absolutely, that would be great. Please take a look at Trisha's patch and replace the hardcoded URLs with Services.urlFormatter.formatURLPref as done here for example: https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/browser/actors/BlockedSiteChild.jsm#105

I can assign you once you have a new patch for it. Let me know if you need any help!

Thanks Johann! am working on it.

Hi Johann,
Adding a patch, please let me know how to proceed.

Comment on attachment 9047061 [details] [diff] [review]
Bug 1284835: Replacing Hardcoded URLs

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

Hi aqadri, I'm not sure how to understand this patch. Is it based on Trisha's patch? In that case please submit a single patch that contains all changes.

In any case, you will need to submit your patch using Phabricator. There's a guide for getting started here: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Bug 1284835: Replaced Hardcoded URLs with Services.urlFormatter.formatURLPref r=reviewers

I'm a bit embarrassed that I missed out on doing this so far, but Joni, would you mind giving us SUMO "vanity" URLs for the following:

https://support.mozilla.org/kb/what-does-your-connection-is-not-secure-mean
https://support.mozilla.org/kb/how-resolve-sslv3-error-messages-firefox
https://support.mozilla.org/kb/certificate-pinning-reports

Thank you!

Flags: needinfo?(jsavage)
Assignee: nobody → asra.qadri
Status: NEW → ASSIGNED

Let's get this landed!

Keywords: checkin-needed

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3a2f408c40df
Replaced Hardcoded URLs with Services.urlFormatter.formatURLPref r=johannh

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Regressions: 1555627
You need to log in before you can comment on or make changes to this bug.