Closed Bug 1463759 Opened 2 years ago Closed Last year

Change the copy of certificate error pages

Categories

(Firefox :: Security, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: trisha, Assigned: trisha)

References

(Blocks 1 open bug)

Details

User Story

https://drive.google.com/drive/folders/1JpgtfOTFUGggfPudMS5Rux0iPspuxPPn
https://docs.google.com/document/d/1PGZFR5fpXAjwJ8B3zg7wmZENYrDL3gWj9qmetJC2XFk/edit

Attachments

(2 files)

This bug is to update the copy of certificate error pages, see user story for planned design.
Attached patch bug1463759.patchSplinter Review
Attachment #8990489 - Flags: feedback?(jhofmann)
Comment on attachment 8990489 [details] [diff] [review]
bug1463759.patch

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

This looks like it's going in a good direction, I think there are two major pieces of work left here:

- Make the "What can you do about it?" description be specific about the error that occurred and whether there's HSTS turned on

- Update netError.dtd, NetErrorContent.jsm and pipnss.properties to use separate string IDs and respect the browser.security.newcerterrorpage.enabled pref

I haven't noted all code style issues in the patch, please run "./mach eslint browser" to find them yourself :)

Thank you for the work!

::: browser/base/content/aboutNetError-new.xhtml
@@ +101,5 @@
> +        <div id="edd_nssBadCert">&certerror.whatCanYouDoAboutItTitle;</div>
> +      </div>
> +    <div id="whatCanYouDoAboutItContainer">
> +        <div id="es_nssBadCert">&certerror.whatCanYouDoAboutIt;</div>
> +        </div>

Please indent this properly :)

@@ +173,3 @@
>            <div class="exceptionDialogButtonContainer">
>              <button id="exceptionDialogButton">&securityOverride.exceptionButtonLabel;</button>
>            </div>

This needs an indent

@@ +174,5 @@
>              <button id="exceptionDialogButton">&securityOverride.exceptionButtonLabel;</button>
>            </div>
> +          </div>
> +        </div>
> +        

You have a bunch of trailing whitespace in this file.

::: browser/locales/en-US/chrome/overrides/netError.dtd
@@ +7,4 @@
>  
>  <!ENTITY loadError.label "Problem loading page">
>  <!ENTITY retry.label "Try Again">
> +<!ENTITY returnToPreviousPage.label "Go Back (Recommended)">

In this file, when updating copy, please keep the old version around so that we can keep the old error page working for now. Just append a 1 to make the new id, e.g.

<!ENTITY returnToPreviousPage.label "Go Back">
<!ENTITY returnToPreviousPage1.label "Go Back (Recommended)">

(Again, please do this for all updated entities in this file)

@@ +182,4 @@
>  <!ENTITY corruptedContentErrorv2.longDesc "<p>The page you are trying to view cannot be shown because an error in the data transmission was detected.</p><ul><li>Please contact the website owners to inform them of this problem.</li></ul>">
>  
>  
> +<!ENTITY securityOverride.exceptionButtonLabel "Accept the Risk and Add Exception">

We should probably move this change into a follow-up bug.

::: browser/modules/NetErrorContent.jsm
@@ +104,4 @@
>        hostString = uri.hostPort;
>      }
>  
> +    let msg1 = "";

Since this file is not forked, we need to add code everywhere that checks the browser.security.newcerterrorpage.enabled pref and only uses the new copy if the new cert error pages are enabled.

::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties
@@ +275,4 @@
>  certErrorIntro=%S uses an invalid security certificate.
>  
>  certErrorTrust_SelfSigned=The certificate is not trusted because it is self-signed.
> +certErrorTrust_UnknownIssuer=Someone could be trying to impersonate the site and you should not continue.

Same as netError.dtd, please don't actually update the strings here.
Attachment #8990489 - Flags: feedback?(jhofmann) → feedback+
Comment on attachment 8991696 [details]
Bug 1463759 Change the copy of certificate error pages

https://reviewboard.mozilla.org/r/256636/#review263476

::: browser/locales/en-US/chrome/overrides/netError.dtd:151
(Diff revision 1)
>    <li>Please contact the website owners to inform them of this problem.</li>
>  </ul>
>  ">
>  
>  <!ENTITY certerror.longpagetitle1 "Your connection is not secure">
> -<!-- Localization note (certerror.introPara) - The text content of the span tag
> +<!ENTITY certerror.longpagetitle2 "Warning: Potential Security Risk ahead">

"ahead" lowercase doesn't seem right here?

::: browser/locales/en-US/chrome/overrides/netError.dtd:157
(Diff revision 1)
> +<!-- Localization note (certerror.introPara, certerror.introPara1) - The text content of the span tag
>  will be replaced at runtime with the name of the server to which the user
>  was trying to connect. -->
>  <!ENTITY certerror.introPara "The owner of <span class='hostname'/> has configured their website improperly.  To protect your information from being stolen, &brandShortName; has not connected to this website.">
> +<!ENTITY certerror.introPara1 "&brandShortName; detected a potential security threat and did not continue to <span class='hostname'/>. If you visit this site, attackers could try to steal information like your passwords, emails, or credit cards.
> +">

Can you avoid adding an unnecessary line break at the end of the string? It won't have any practical effect on HTML, but it's part of the string, and it's confusing (this comment applies to other strings as well),

So:

```
<!ENTITY certerror.introPara1 "&brandShortName; detected a potential security threat and did not continue to <span class='hostname'/>. If you visit this site, attackers could try to steal information like your passwords, emails, or credit cards.">
```

::: browser/locales/en-US/chrome/overrides/netError.dtd:162
(Diff revision 1)
> +">
> +
> +<!ENTITY certerror.expiredCert.secondPara "This issue is most likely because your computer clock is set to the wrong time, which would prevent &brandShortName; from connecting securely.
> +">
> +
> +<!ENTITY certerror.whatCanYouDoAboutItTitle "<strong> What can you do about it?</strong>">

Unnecessary space before "What". I don't think exposing the markup (<strong></strong>) is necessary here, it could be left in the XHTML file.

::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties:282
(Diff revision 1)
>  certErrorTrust_SelfSigned=The certificate is not trusted because it is self-signed.
>  certErrorTrust_UnknownIssuer=The certificate is not trusted because the issuer certificate is unknown.
>  certErrorTrust_UnknownIssuer2=The server might not be sending the appropriate intermediate certificates.
>  certErrorTrust_UnknownIssuer3=An additional root certificate may need to be imported.
> +certErrorTrust_UnknownIssuer4=Someone could be trying to impersonate the site and you should not continue.
> +certErrorTrust_UnknownIssuer5=Websites prove their identity via security certificates. Firefox does not trust %S because its security certificate issuer is unknown, the certificate is self-signed, or the server is not sending the correct intermediate certificates.

Don't hardcode Firefox in strings, it should be a variable replaced at run-time.

P.S. since there will be two placeholders, use %1$S and %2$S, not %S twice.

::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties:293
(Diff revision 1)
>  certErrorTrust_MitM=Your connection is being intercepted by a TLS proxy. Uninstall it if possible or configure your device to trust its root certificate.
>  
>  certErrorMismatch=The certificate is not valid for the name %S.
>  # LOCALIZATION NOTE (certErrorMismatchSinglePrefix): %S is replaced by the domain for which the certificate is valid
>  certErrorMismatchSinglePrefix=The certificate is only valid for %S.
> +certErrorMismatchMultiple1=Websites prove their identity via security certificates. Firefox does not trust %S because it uses a security certificate that is not valid for %S. The certificate is only valid for the following names:

Same here: don't hardcode Firefox.
(In reply to Francesco Lodolo [:flod] (PTO until Jul 16) from comment #4)
> Comment on attachment 8991696 [details]
> Bug 1463759 Change the copy of certificate error pages
> 
> https://reviewboard.mozilla.org/r/256636/#review263476
> 
> ::: browser/locales/en-US/chrome/overrides/netError.dtd:151
> (Diff revision 1)
> >    <li>Please contact the website owners to inform them of this problem.</li>
> >  </ul>
> >  ">
> >  
> >  <!ENTITY certerror.longpagetitle1 "Your connection is not secure">
> > -<!-- Localization note (certerror.introPara) - The text content of the span tag
> > +<!ENTITY certerror.longpagetitle2 "Warning: Potential Security Risk ahead">
> 
> "ahead" lowercase doesn't seem right here?
> 
> ::: browser/locales/en-US/chrome/overrides/netError.dtd:157
> (Diff revision 1)
> > +<!-- Localization note (certerror.introPara, certerror.introPara1) - The text content of the span tag
> >  will be replaced at runtime with the name of the server to which the user
> >  was trying to connect. -->
> >  <!ENTITY certerror.introPara "The owner of <span class='hostname'/> has configured their website improperly.  To protect your information from being stolen, &brandShortName; has not connected to this website.">
> > +<!ENTITY certerror.introPara1 "&brandShortName; detected a potential security threat and did not continue to <span class='hostname'/>. If you visit this site, attackers could try to steal information like your passwords, emails, or credit cards.
> > +">
> 
> Can you avoid adding an unnecessary line break at the end of the string? It
> won't have any practical effect on HTML, but it's part of the string, and
> it's confusing (this comment applies to other strings as well),
> 
> So:
> 
> ```
> <!ENTITY certerror.introPara1 "&brandShortName; detected a potential
> security threat and did not continue to <span class='hostname'/>. If you
> visit this site, attackers could try to steal information like your
> passwords, emails, or credit cards.">
> ```
> 
> ::: browser/locales/en-US/chrome/overrides/netError.dtd:162
> (Diff revision 1)
> > +">
> > +
> > +<!ENTITY certerror.expiredCert.secondPara "This issue is most likely because your computer clock is set to the wrong time, which would prevent &brandShortName; from connecting securely.
> > +">
> > +
> > +<!ENTITY certerror.whatCanYouDoAboutItTitle "<strong> What can you do about it?</strong>">
> 
> Unnecessary space before "What". I don't think exposing the markup
> (<strong></strong>) is necessary here, it could be left in the XHTML file.
> 
> ::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties:282
> (Diff revision 1)
> >  certErrorTrust_SelfSigned=The certificate is not trusted because it is self-signed.
> >  certErrorTrust_UnknownIssuer=The certificate is not trusted because the issuer certificate is unknown.
> >  certErrorTrust_UnknownIssuer2=The server might not be sending the appropriate intermediate certificates.
> >  certErrorTrust_UnknownIssuer3=An additional root certificate may need to be imported.
> > +certErrorTrust_UnknownIssuer4=Someone could be trying to impersonate the site and you should not continue.
> > +certErrorTrust_UnknownIssuer5=Websites prove their identity via security certificates. Firefox does not trust %S because its security certificate issuer is unknown, the certificate is self-signed, or the server is not sending the correct intermediate certificates.
> 
> Don't hardcode Firefox in strings, it should be a variable replaced at
> run-time.
> 
> P.S. since there will be two placeholders, use %1$S and %2$S, not %S twice.
> 
> ::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties:293
> (Diff revision 1)
> >  certErrorTrust_MitM=Your connection is being intercepted by a TLS proxy. Uninstall it if possible or configure your device to trust its root certificate.
> >  
> >  certErrorMismatch=The certificate is not valid for the name %S.
> >  # LOCALIZATION NOTE (certErrorMismatchSinglePrefix): %S is replaced by the domain for which the certificate is valid
> >  certErrorMismatchSinglePrefix=The certificate is only valid for %S.
> > +certErrorMismatchMultiple1=Websites prove their identity via security certificates. Firefox does not trust %S because it uses a security certificate that is not valid for %S. The certificate is only valid for the following names:
> 
> Same here: don't hardcode Firefox.

Thanks for the review. Will fix them right away :)
Comment on attachment 8991696 [details]
Bug 1463759 Change the copy of certificate error pages

https://reviewboard.mozilla.org/r/256636/#review263612

::: browser/locales/en-US/chrome/overrides/netError.dtd:13
(Diff revision 2)
>  <!ENTITY loadError.label "Problem loading page">
>  <!ENTITY retry.label "Try Again">
>  <!ENTITY returnToPreviousPage.label "Go Back">
> +<!ENTITY returnToPreviousPage1.label "Go Back (Recommended)">
>  <!ENTITY advanced.label "Advanced">
> +<!ENTITY advanced1.label "Continue">

This the label changes completely, it makes sense to change the label ID as well (continue.label), instead of versioning the existing one.

::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties:282
(Diff revision 2)
>  certErrorTrust_SelfSigned=The certificate is not trusted because it is self-signed.
>  certErrorTrust_UnknownIssuer=The certificate is not trusted because the issuer certificate is unknown.
>  certErrorTrust_UnknownIssuer2=The server might not be sending the appropriate intermediate certificates.
>  certErrorTrust_UnknownIssuer3=An additional root certificate may need to be imported.
> +certErrorTrust_UnknownIssuer4=Someone could be trying to impersonate the site and you should not continue.
> +certErrorTrust_UnknownIssuer5=Websites prove their identity via security certificates. %1$S does not trust %2$S because its security certificate issuer is unknown, the certificate is self-signed, or the server is not sending the correct intermediate certificates.

Sorry, forgot one important bit: when there are multiple placeholders, it's important to add a localization comment explaining what's used to replace them. You can see an example for certErrorMismatchSinglePrefix below

::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties:282
(Diff revision 2)
>  certErrorTrust_SelfSigned=The certificate is not trusted because it is self-signed.
>  certErrorTrust_UnknownIssuer=The certificate is not trusted because the issuer certificate is unknown.
>  certErrorTrust_UnknownIssuer2=The server might not be sending the appropriate intermediate certificates.
>  certErrorTrust_UnknownIssuer3=An additional root certificate may need to be imported.
> +certErrorTrust_UnknownIssuer4=Someone could be trying to impersonate the site and you should not continue.
> +certErrorTrust_UnknownIssuer5=Websites prove their identity via security certificates. %1$S does not trust %2$S because its security certificate issuer is unknown, the certificate is self-signed, or the server is not sending the correct intermediate certificates.
Comment on attachment 8991696 [details]
Bug 1463759 Change the copy of certificate error pages

https://reviewboard.mozilla.org/r/256636/#review263646

Thank you, this seems to work well already, but the code can be tuned a bit :)

One other thing I noticed was that the whole thing didn't seem exactly centered anymore, I believe that's because you're changing the height of the container in NetErrorContent.jsm after applying the JS style workaround on the page. It might be necessary to copy that centering function over into NetErrorContent.jsm for now.

::: browser/base/content/aboutNetError-new.xhtml:185
(Diff revision 3)
>  
>        <div id="advancedPanelContainer">
>          <div id="badCertAdvancedPanel" class="advanced-panel">
>            <p id="badCertTechnicalInfo"/>
> +          <div id="certErrorAndCaptivePortalButtonContainer1" class="button-container">
> +            <button id="returnButton" class="primary" autocomplete="off">&returnToPreviousPage1.label;</button>

This duplicates the id (and it shouldn't), when you give this a new ID don't forget to add it here:

https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/browser/base/content/browser.js#3025

::: browser/locales/en-US/chrome/overrides/netError.dtd:13
(Diff revision 3)
>  <!ENTITY loadError.label "Problem loading page">
>  <!ENTITY retry.label "Try Again">
>  <!ENTITY returnToPreviousPage.label "Go Back">
> +<!ENTITY returnToPreviousPage1.label "Go Back (Recommended)">
>  <!ENTITY advanced.label "Advanced">
> +<!ENTITY continue.label "Continue">

To be honest I don't feel like "Continue" is the appropriate label here. We should talk to Meridel about this again, but let's avoid blocking this patch on it.

Can you just revert this to use the old advanced label again, please?

Thanks!

::: browser/modules/NetErrorContent.jsm:116
(Diff revision 3)
>        switch (input.data.code) {
>          // We only want to measure MitM rates for now. Treat it as unkown issuer.
>          case MOZILLA_PKIX_ERROR_MITM_DETECTED:
>          case SEC_ERROR_UNKNOWN_ISSUER:
> +          let brandName;
> +          const bundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");

Can you please make this bundle a lazy getter like gPipNSSBundle?

https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/browser/modules/NetErrorContent.jsm#16

::: browser/modules/NetErrorContent.jsm:118
(Diff revision 3)
>          case MOZILLA_PKIX_ERROR_MITM_DETECTED:
>          case SEC_ERROR_UNKNOWN_ISSUER:
> +          let brandName;
> +          const bundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");
> +          if (bundle) {
> +            brandName = bundle.GetStringFromName("brandFullName");

You probably want to use brandShortName instead.

::: browser/modules/NetErrorContent.jsm:121
(Diff revision 3)
> +          const bundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");
> +          if (bundle) {
> +            brandName = bundle.GetStringFromName("brandFullName");
> +          } else {
> +            brandName = null;
> +          }

I don't think you need to do the if ... else dance here, we have a ton of code that assumes this bundle is available already...

::: browser/modules/NetErrorContent.jsm:122
(Diff revision 3)
> +          if (bundle) {
> +            brandName = bundle.GetStringFromName("brandFullName");
> +          } else {
> +            brandName = null;
> +          }
> +          if (Services.prefs.getBoolPref("browser.security.newcerterrorpage.enabled", false)) {

You end up duplicating the Services.prefs.getBoolPref call so much that it might make sense to put the result in a local variable at the top of the function instead.

::: browser/modules/NetErrorContent.jsm:339
(Diff revision 3)
>      let learnMoreLink = doc.getElementById("learnMoreLink");
>      let baseURL = Services.urlFormatter.formatURLPref("app.support.baseURL");
>  
>      switch (msg.data.code) {
>        case SEC_ERROR_UNKNOWN_ISSUER:
> +        if (Services.prefs.getBoolPref("browser.security.newcerterrorpage.enabled", false)) {

Instead of gigantic if wrappers you should do:

if (!Services.prefs.getBoolPref("browser.security.newcerterrorpage.enabled", false)) {
  break;
}

and I think you can also store the result in a variable to avoid the code duplication

::: browser/modules/NetErrorContent.jsm:340
(Diff revision 3)
>      let baseURL = Services.urlFormatter.formatURLPref("app.support.baseURL");
>  
>      switch (msg.data.code) {
>        case SEC_ERROR_UNKNOWN_ISSUER:
> +        if (Services.prefs.getBoolPref("browser.security.newcerterrorpage.enabled", false)) {
> +        var errWhatToDo = doc.getElementById("es_nssBadCert_SEC_ERROR_UNKNOWN_ISSUER");

nit: let

::: browser/modules/NetErrorContent.jsm:341
(Diff revision 3)
>  
>      switch (msg.data.code) {
>        case SEC_ERROR_UNKNOWN_ISSUER:
> +        if (Services.prefs.getBoolPref("browser.security.newcerterrorpage.enabled", false)) {
> +        var errWhatToDo = doc.getElementById("es_nssBadCert_SEC_ERROR_UNKNOWN_ISSUER");
> +        var es = doc.getElementById("errorWhatToDoText");

nit: let

::: browser/modules/NetErrorContent.jsm:345
(Diff revision 3)
> +        var errWhatToDo = doc.getElementById("es_nssBadCert_SEC_ERROR_UNKNOWN_ISSUER");
> +        var es = doc.getElementById("errorWhatToDoText");
> +        if (es) {
> +          // eslint-disable-next-line no-unsanitized/property
> +          es.innerHTML = errWhatToDo.innerHTML;
> +          }

please check the indentation in this block and the others below

::: browser/modules/NetErrorContent.jsm:346
(Diff revision 3)
> +        var es = doc.getElementById("errorWhatToDoText");
> +        if (es) {
> +          // eslint-disable-next-line no-unsanitized/property
> +          es.innerHTML = errWhatToDo.innerHTML;
> +          }
> +        var errWhatToDoTitle = doc.getElementById("edd_nssBadCert");

please use let instead of var everywhere, except where you think it's absolutely needed

::: browser/modules/NetErrorContent.jsm:407
(Diff revision 3)
> +          break;
> +          }
> +
> +      case SEC_ERROR_OCSP_INVALID_SIGNING_CERT:
> +        if (Services.prefs.getBoolPref("browser.security.newcerterrorpage.enabled", false)) {
> +        errWhatToDo = doc.getElementById("es_nssBadCert_SSL_ERROR_BAD_CERT_DOMAIN");

It looks to me like you can combine SEC_ERROR_UNKNOWN_ISSUER, SSL_ERROR_BAD_CERT_DOMAIN and SEC_ERROR_OCSP_INVALID_SIGNING_CERT to a single piece of code by just modifying this line to:

let errWhatToDo = doc.getElementById("es_nssBadCert_" + msg.data.code);

and then making two of the three error codes fall through in the switch .. case:

case SSL_ERROR_BAD_CERT_DOMAIN:
case SEC_ERROR_OCSP_INVALID_SIGNING_CERT:
case SEC_ERROR_UNKNOWN_ISSUER:
  // le code

::: browser/themes/shared/aboutNetError-new.css:53
(Diff revision 3)
>  
>  #certErrorAndCaptivePortalButtonContainer {
>    display: none;
>  }
>  
> +#certErrorAndCaptivePortalButtonContainer1 {

nit: maybe give this a little more descriptive name like advancedPanelButtonContainer?

::: toolkit/themes/shared/in-content/info-pages.inc.css
(Diff revision 3)
>  }
>  
>  ul, ol {
>    margin: 1em 0;
>    padding: 0;
> -  margin-inline-start: 2em;

I don't think should just remove this from info-pages.inc.css, a lot of other pages get this style applied.

Why are we wrapping these texts in <ul> tags? Wouldn't it work to have them inside <div>s or not wrapping them at all?
Attachment #8991696 - Flags: review?(jhofmann) → review-
Comment on attachment 8991696 [details]
Bug 1463759 Change the copy of certificate error pages

https://reviewboard.mozilla.org/r/256636/#review264336

Great progress! I have a couple of nits but I'd like to take another closer look after fixing these so r- for now.

It would also be great to have a test that the "What can you do about it" section is shown, at least for one or two of the affected error codes.

Thank you for the quick turnaround!

::: browser/modules/NetErrorContent.jsm:57
(Diff revision 4)
>  const PREF_SERVICES_SETTINGS_LAST_FETCHED       = "services.settings.last_update_seconds";
>  
>  const PREF_SSL_IMPACT_ROOTS = ["security.tls.version.", "security.ssl3."];
>  
>  
> +let enabled = Services.prefs.getBoolPref("browser.security.newcerterrorpage.enabled", false);

If you want to have this as a global attribute (which sounds fine to me), please use a lazy pref getter, like this:

https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/browser/base/content/tabbrowser.js#71-72

Also please rename the variable to be a little more verbose than just "enabled", maybe newErrorPagesEnabled?

::: browser/modules/NetErrorContent.jsm:230
(Diff revision 4)
>              technicalInfo.appendChild(fragment);
>            }
>            technicalInfo.append("\n");
>          } else {
> -          let msg = gPipNSSBundle.GetStringFromName("certErrorMismatchMultiple") + "\n";
> +          let msg = "";
> +          let brandName = gBrandBundle.GetStringFromName("brandFullName");

nit: brandShortName

::: browser/modules/NetErrorContent.jsm:338
(Diff revision 4)
>  
>      switch (msg.data.code) {
> +      case SSL_ERROR_BAD_CERT_DOMAIN:
> +      case SEC_ERROR_OCSP_INVALID_SIGNING_CERT:
>        case SEC_ERROR_UNKNOWN_ISSUER:
> +        if (enabled) {

Please avoid the large if block and do

if (!enabled) {
  break;
}

instead :)

::: browser/modules/NetErrorContent.jsm:352
(Diff revision 4)
> +          if (est) {
> +            // eslint-disable-next-line no-unsanitized/property
> +            est.innerHTML = errWhatToDoTitle.innerHTML;
> +          }
> +          updateContainerPosition();
> +        break;

This is going to become obsolete with fixing the above mentioned issue, but note that this break should be outside the brackets.

::: browser/modules/NetErrorContent.jsm:363
(Diff revision 4)
>  
>        // In case the certificate expired we make sure the system clock
>        // matches the remote-settings service (blocklist via Kinto) ping time
>        // and is not before the build date.
>        case SEC_ERROR_EXPIRED_CERTIFICATE:
> +        if (enabled) {

Please use

if (!enabled) {
  break;
}
Attachment #8991696 - Flags: review?(jhofmann) → review-
Comment on attachment 8991696 [details]
Bug 1463759 Change the copy of certificate error pages

https://reviewboard.mozilla.org/r/256636/#review264352


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/modules/NetErrorContent.jsm:334
(Diff revision 5)
>      let div = doc.getElementById("certificateErrorText");
>      div.textContent = msg.data.info;
>      this._setTechDetails(msg, doc);
>      let learnMoreLink = doc.getElementById("learnMoreLink");
>      let baseURL = Services.urlFormatter.formatURLPref("app.support.baseURL");
> +    let errWhatToDo = doc.getElementById("es_nssBadCert_" + msg.data.codeString)

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8991696 [details]
Bug 1463759 Change the copy of certificate error pages

https://reviewboard.mozilla.org/r/256636/#review264726

A couple more comments! Thanks!

::: browser/base/content/browser.js:3134
(Diff revision 6)
>          goBackFromErrorPage();
>          break;
>  
> +      case "advancedPanelReturnButton":
> +        if (isTopFrame) {
> +          secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_GET_ME_OUT_OF_HERE);

It probably doesn't make sense to have this telemetry probe here.

::: browser/modules/NetErrorContent.jsm:23
(Diff revision 6)
>  });
> +XPCOMUtils.defineLazyGetter(this, "gBrandBundle", function() {
> +  return Services.strings.createBundle("chrome://branding/locale/brand.properties");
> +});
> +XPCOMUtils.defineLazyPreferenceGetter(this, "newErrorPagesEnabled",
> +"browser.security.newcerterrorpage.enabled");

Please either give this line two spaces of indentation or wrap it with the previous line.

::: browser/modules/NetErrorContent.jsm:114
(Diff revision 6)
>      let hostString = uri.host;
>      if (uri.port != 443 && uri.port != -1) {
>        hostString = uri.hostPort;
>      }
>  
> -    let msg1 = gPipNSSBundle.formatStringFromName("certErrorIntro",
> +    let msg1 = "";

I don't think you can remove this here. This message goes into all the other cases except UNKNOWN_ISSUER.

::: browser/modules/NetErrorContent.jsm:230
(Diff revision 6)
>                                                               input.data.certSubjectAltNames);
>              technicalInfo.appendChild(fragment);
>            }
>            technicalInfo.append("\n");
>          } else {
> -          let msg = gPipNSSBundle.GetStringFromName("certErrorMismatchMultiple") + "\n";
> +          let msg = "";

Shouldn't this be behind the pref as well?

::: browser/modules/NetErrorContent.jsm:232
(Diff revision 6)
>            }
>            technicalInfo.append("\n");
>          } else {
> -          let msg = gPipNSSBundle.GetStringFromName("certErrorMismatchMultiple") + "\n";
> +          let msg = "";
> +          let brandName = gBrandBundle.GetStringFromName("brandShortName");
> +          msg = gPipNSSBundle.formatStringFromName("certErrorMismatchMultiple1", [brandName, hostString, hostString], 3) + " ";

Are you sure you need to pass in hostString twice here?

::: browser/modules/NetErrorContent.jsm:260
(Diff revision 6)
> +          if (newErrorPagesEnabled) {
> +            msg += gPipNSSBundle.formatStringFromName("certErrorExpiredNow1",
> +                                                    [hostString], 1);
> +            msg += "\n";
> +          } else {
> +            msg = gPipNSSBundle.formatStringFromName("certErrorIntro",

Why are you adding this line here? Can this be removed when re-adding this line as mentioned in my comment above?

::: browser/modules/NetErrorContent.jsm:365
(Diff revision 6)
>          break;
>  
>        // In case the certificate expired we make sure the system clock
>        // matches the remote-settings service (blocklist via Kinto) ping time
>        // and is not before the build date.
>        case SEC_ERROR_EXPIRED_CERTIFICATE:

I think the code below should apply to all time related cases, not just SEC_ERROR_EXPIRED_CERTIFICATE.

::: browser/themes/shared/aboutNetError-new.css:57
(Diff revision 6)
>  
> +#advancedPanelButtonContainer {
> +  background-color: var(--exception-button-container-background);
> +  display: flex;
> +  justify-content: end;
> +}

Can you apply a bit of padding to this container? Maybe something between 5 to 10px (the mockup seems to have some padding here).

::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties:295
(Diff revision 6)
>  
>  certErrorMismatch=The certificate is not valid for the name %S.
>  # LOCALIZATION NOTE (certErrorMismatchSinglePrefix): %S is replaced by the domain for which the certificate is valid
>  certErrorMismatchSinglePrefix=The certificate is only valid for %S.
> +# LOCALIZATION NOTE (certErrorMismatchMultiple1): %1$S is replaced by the brand name, %2$S is replaced by host name.
> +certErrorMismatchMultiple1=Websites prove their identity via security certificates. %1$S does not trust %2$S because it uses a security certificate that is not valid for %2$S. The certificate is only valid for the following names:

I think you need to update the string for all variations of this (certErrorMismatch, certErrorMismatchSinglePrefix, certErrorMismatchMultiple)
Attachment #8991696 - Flags: review?(jhofmann) → review-
Comment on attachment 8991696 [details]
Bug 1463759 Change the copy of certificate error pages

https://reviewboard.mozilla.org/r/256636/#review265414

This looks great, but please

- Fix the remaining issues I flagged (feel free to get in touch if you have any questions)
- File the following bugs to:
  - Add a test for showing the "what can you do about it" section on the right error pages
  - Enable the new cert error pages by default in Nightly
  - Figure out a better label for the "Advanced" button

Thank you so much for all your work on this!

::: browser/base/content/aboutNetError-new.xhtml:198
(Diff revision 7)
> -            <button id="exceptionDialogButton">&securityOverride.exceptionButtonLabel;</button>
> +              <button id="exceptionDialogButton">&securityOverride.exceptionButtonLabel;</button>
> -          </div>
> +            </div>
> -        </div>
> +          </div>
> +        </div>
> +
> +        <div id="certificateErrorReporting">

Can you please give this element a bit of bottom padding? When opening the "advanced" section it looks a little squeezed.

::: browser/modules/NetErrorContent.jsm:167
(Diff revision 7)
>        if (numSubjectAltNames != 0) {
>          if (numSubjectAltNames == 1) {
> +          if (newErrorPagesEnabled) {
> +            technicalInfo.textContent = "";
> +            let brandName = gBrandBundle.GetStringFromName("brandShortName");
> +            msgPrefix = gPipNSSBundle.formatStringFromName("certErrorMismatchSinglePrefix1", [brandName, hostString], 3);

You still have 3 as a parameter for the number of elements in the array (it should be two).

This is actually causing an error on https://captive-portal.badssl.com/

::: browser/modules/NetErrorContent.jsm:382
(Diff revision 7)
>        case SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE:
>        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:
> -
> +      case SEC_ERROR_EXPIRED_CERTIFICATE:

Please move this line back on top of the other case clauses

::: browser/modules/NetErrorContent.jsm:402
(Diff revision 7)
> +          if (est) {
> +            // eslint-disable-next-line no-unsanitized/property
> +            est.innerHTML = errWhatToDoTitle.innerHTML;
> +          }
> +          updateContainerPosition();
> +        } else {

nit: please just break; at the end of this if block so that you can avoid using "else" here and indenting all the code below

You probably have to move

learnMoreLink.href = baseURL + "time-errors";

up a little then :)
Attachment #8991696 - Flags: review?(jhofmann) → review+
Comment on attachment 8991696 [details]
Bug 1463759 Change the copy of certificate error pages

https://reviewboard.mozilla.org/r/256636/#review265430

::: browser/locales/en-US/chrome/overrides/netError.dtd:167
(Diff revision 7)
> +<p>The issue is most likely with the website, and there is nothing you can do to resolve it.</p>
> +<p>If you are on a corporate network or using anti-virus software, you can reach out to the support teams for assistance. You can also notify the website’s administrator about the problem.</p>
> +">
> +
> +<!ENTITY certerror.expiredCert.whatCanYouDoAboutIt "
> +<p>Your computer clock is set to <span id='wrongSystemTime_systemDate'/>. Make sure your computer is set to the correct date, time, and time zone in your system settings, and then refresh <span class='hostname'/>.</p>

Make sure to run tests. You should need to add some of these new strings as exceptions to
https://searchfox.org/mozilla-central/source/browser/base/content/test/static/browser_misused_characters_in_strings.js since they contain straight quotes.
Keywords: checkin-needed
There are 29 open issues, in order to land this patch please fix them.
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93f90bc23a4a
Change the copy of certificate error pages r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/93f90bc23a4a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1484259
Blocks: 1487889
No longer blocks: 1487889
Depends on: 1487889
You need to log in before you can comment on or make changes to this bug.