Closed Bug 1476509 Opened 3 years ago Closed 3 years ago

Implement a new clock skew error page

Categories

(Firefox :: Security, enhancement, P1)

Product:
Firefox
Component:
Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: trisha, Assigned: trisha, Mentored)

References

Details

Attachments

(1 file)

Bug 1476509 - Implement a new clock skew error page
59 bytes, text/x-review-board-request
flod
: review+
Details 
The plan is to add a special error page where we are 100% sure it is a clock error and to also fix the current SEC_ERROR_OCSP_FUTURE_RESPONSE error page.
Assignee: nobody → guptatrisha97
Status: NEW → ASSIGNED
Maybe "fix" isn't exactly the right word for it :)
Priority: -- → P1
Summary: Fix the clock skew error pages → Implement a new clock skew error page
Comment on attachment 8997640 [details]
Bug 1476509 - Implement a new clock skew error page

https://reviewboard.mozilla.org/r/261334/#review268522

This is looking pretty good already, just a couple of notes.

::: browser/base/content/aboutNetError.js:118
(Diff revision 1)
> +      if (panel.style.display == "block") {
> +        // send event to trigger telemetry ping
> +        var event = new CustomEvent("AboutNetErrorUIExpanded", {bubbles: true});
> +        document.dispatchEvent(event);
> +      }
> +    });

Since this function seems to be an exact copy of https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/browser/base/content/aboutNetError.js#86 can't you just do

document.getElementById("moreInformationButton").addEventListener("click", togglePanelVisibility);

::: browser/base/content/illustrations/blue-berror.svg:1
(Diff revision 1)
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 221.72 184.08">

I've sent you a version of this file optimized with svgo, please use that one instead :)

::: browser/modules/NetErrorContent.jsm:460
(Diff revision 1)
> +        if (!newErrorPagesEnabled) {
> +          break;
> +        }
> +        let dateOptions = { year: "numeric", month: "long", day: "numeric", hour: "numeric", minute: "numeric" };
> +        let systemDate = new Services.intl.DateTimeFormat(undefined, dateOptions).format(new Date());
> +        doc.getElementById("wrongSystemTime_systemDate").textContent = systemDate;

wrongSystemTime_systemDate is a duplicate id, so please use a different one

In fact we already made this mistake previously, I think: https://searchfox.org/mozilla-central/search?q=wrongSystemTime_systemDate&case=false&regexp=false&path=

We should probably open a follow-up bug to fix this.

::: browser/modules/NetErrorContent.jsm:475
(Diff revision 1)
> +          doc.getElementById("certificateErrorReporting").style.display = "none";
> +          if (desc) {
> +            // eslint-disable-next-line no-unsanitized/property
> +            desc.innerHTML = clockErrDesc.innerHTML;
> +          }
> +          updateContainerPosition();

Please also update the illustration background-position here.

::: browser/themes/shared/aboutNetError-new.css:245
(Diff revision 1)
> +body.clockSkewError #returnButton {
> +  display: none;
> +}
> +
> +body.clockSkewError #advancedButton {
> +    display: none;

nit: indentation
Attachment #8997640 - Flags: review?(jhofmann) → review-
Comment on attachment 8997640 [details]
Bug 1476509 - Implement a new clock skew error page

https://reviewboard.mozilla.org/r/261334/#review268932


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

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


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


::: browser/modules/NetErrorContent.jsm:479
(Diff revision 2)
> +          }
> +          let errorPageContainer = doc.getElementById("errorPageContainer");
> +          let textContainer = doc.getElementById("text-container");
> +          errorPageContainer.style.backgroundPosition = `left top calc(50vh - ${textContainer.clientHeight / 2}px)`;
> +        } else {
> +            let dateOptions = { year: "numeric", month: "long", day: "numeric", hour: "numeric", minute: "numeric" };

Error: 'dateOptions' is already declared in the upper scope. [eslint: no-shadow]

::: browser/modules/NetErrorContent.jsm:480
(Diff revision 2)
> +          let errorPageContainer = doc.getElementById("errorPageContainer");
> +          let textContainer = doc.getElementById("text-container");
> +          errorPageContainer.style.backgroundPosition = `left top calc(50vh - ${textContainer.clientHeight / 2}px)`;
> +        } else {
> +            let dateOptions = { year: "numeric", month: "long", day: "numeric", hour: "numeric", minute: "numeric" };
> +            let systemDate = new Services.intl.DateTimeFormat(undefined, dateOptions).format(new Date());

Error: 'systemDate' is already declared in the upper scope. [eslint: no-shadow]
Comment on attachment 8997640 [details]
Bug 1476509 - Implement a new clock skew error page

https://reviewboard.mozilla.org/r/261334/#review269054

Looks good, thank you, r=me with the lint failures and nits fixed :)

::: browser/modules/NetErrorContent.jsm:466
(Diff revision 2)
> +        if (clockSkew) {
> +          doc.body.classList.add("illustrated", "clockSkewError");
> +          let clockErrTitle = doc.getElementById("et_clockSkewError");
> +          let clockErrDesc = doc.getElementById("ed_clockSkewError");
> +          // eslint-disable-next-line no-unsanitized/property
> +          doc.querySelector(".title-text").innerHTML = clockErrTitle.innerHTML;

I think you can just use .textContent instead of .innerHTML here, right?

::: browser/modules/NetErrorContent.jsm:473
(Diff revision 2)
> +          doc.getElementById("errorShortDesc").style.display = "block";
> +          doc.getElementById("wrongSystemTimePanel").style.display = "none";
> +          doc.getElementById("certificateErrorReporting").style.display = "none";
> +          if (desc) {
> +            // eslint-disable-next-line no-unsanitized/property
> +            desc.innerHTML = clockErrDesc.innerHTML;

And just a note that I'd really like to move away from these innerHTML assignments as soon as we can use fluent on this page...

::: browser/modules/NetErrorContent.jsm:494
(Diff revision 2)
> +              // eslint-disable-next-line no-unsanitized/property
> +              es.innerHTML = errWhatToDo.innerHTML;
> +            }
> +            if (est) {
> +              // eslint-disable-next-line no-unsanitized/property
> +              est.innerHTML = errWhatToDoTitle.innerHTML;

I know this is existing code but this can probably use .textContent, I didn't pay enough attention to that last time :)

::: browser/themes/shared/aboutNetError-new.css:245
(Diff revision 2)
> +
> +body.clockSkewError #returnButton {
> +  display: none;
> +}
> +
> +body.clockSkewError #advancedButton {

nit: this is a little inconsistent, please either use body.clockSkewError or just .clockSkewError in your rules.

::: browser/themes/shared/aboutNetError-new.css:254
(Diff revision 2)
> +.clockSkewError #errorTryAgain {
> +  display: block;
> +  margin-top: 0.3em;
> +}
> +
> +.clockSkewError #advancedButton {

nit: please join this with the rule above:

.clockSkewError #errorTryAgain,
.clockSkewError #advancedButton {
  display: block;
  margin-top: 0.3em;
}
Attachment #8997640 - Flags: review?(jhofmann) → review+
Comment on attachment 8997640 [details]
Bug 1476509 - Implement a new clock skew error page

https://reviewboard.mozilla.org/r/261334/#review268932

> Error: 'systemDate' is already declared in the upper scope. [eslint: no-shadow]

Regarding this, I don't see any reason why you couldn't just reuse systemDate and dateOptions from above.
Comment on attachment 8997640 [details]
Bug 1476509 - Implement a new clock skew error page

https://reviewboard.mozilla.org/r/261334/#review269072

::: browser/modules/NetErrorContent.jsm:493
(Diff revisions 2 - 3)
>                es.innerHTML = errWhatToDo.innerHTML;
>              }
>              if (est) {
>                // eslint-disable-next-line no-unsanitized/property
> -              est.innerHTML = errWhatToDoTitle.innerHTML;
> +              est.textContent = errWhatToDoTitle.textContent;
> +              est.style.fontWeight = 'bold';

nit: please use double quotes here
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef30529d43dd
Implement a new clock skew error page r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef30529d43dd
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment on attachment 8997640 [details]
Bug 1476509 - Implement a new clock skew error page

https://reviewboard.mozilla.org/r/261334/#review269306

::: browser/locales/en-US/chrome/overrides/netError.dtd:169
(Diff revision 6)
>  <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>
> +<p>Your computer clock is set to <span id='wrongSystemTime_systemDate2'/>. 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>

This is a change to an existing string, that will basically break all existing translations. It needs a new ID.

Why is this even needed?
Attachment #8997640 - Flags: review-
I'm going to ask for a backout of this patch, since it breaks localization badly. Either localization won't notice the change and break 63, or will update their translations accordingly and break 62.
Backed out based on https://bugzilla.mozilla.org/show_bug.cgi?id=1476509#c16

Backout: https://hg.mozilla.org/mozilla-central/rev/d0a17fc80dabd76aca148d42684a385ba41f9ed8
Status: RESOLVED → REOPENED
status-firefox63: fixed → ---
Resolution: FIXED → ---
Target Milestone: Firefox 63 → ---
(In reply to Francesco Lodolo [:flod] from comment #15)
> This is a change to an existing string, that will basically break all
> existing translations. It needs a new ID.
> 
> Why is this even needed?

Found the reason in the meantime

> wrongSystemTime_systemDate is a duplicate id, so please use a different one
Blocks: 1463693
Comment on attachment 8997640 [details]
Bug 1476509 - Implement a new clock skew error page

https://reviewboard.mozilla.org/r/261334/#review269342

l10n looks good, might be worth asking Johann to take a quick look to confirm everything else is good.
Attachment #8997640 - Flags: review?(francesco.lodolo) → review+
Flags: needinfo?(jhofmann)
This looks good, thank you. Sorry for missing this in my review!
Flags: needinfo?(jhofmann)
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5389dab7be8
Implement a new clock skew error page r=flod,johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f5389dab7be8
Status: REOPENED → RESOLVED
Closed: 3 years ago → 3 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.