Bug 1754217 - remove brittle time format APIs from nsIX509CertValidity r=jschanck

nsIX509CertValidity had a handful of APIs that would return formatted time
values. Some of these APIs were unused, and the rest were prone to error due to
platform differences. This patch simplifies this interface by removing those
APIs and having callers perform their own formatting using the remaining APIs
that return PRTime values.

Differential Revision: https://phabricator.services.mozilla.com/D138363

security/manager/pki/resources/content/clientauthask.js

@@ -141,12 +141,16 @@ async function setDetails() {
   let index = parseInt(document.getElementById("nicknames").value);
   let cert = certArray.queryElementAt(index, Ci.nsIX509Cert);
 
+  const formatter = new Intl.DateTimeFormat(undefined, {
+    dateStyle: "medium",
+    timeStyle: "long",
+  });
   let detailLines = [
     bundle.getFormattedString("clientAuthIssuedTo", [cert.subjectName]),
     bundle.getFormattedString("clientAuthSerial", [cert.serialNumber]),
     bundle.getFormattedString("clientAuthValidityPeriod", [
-      cert.validity.notBeforeLocalTime,
-      cert.validity.notAfterLocalTime,
+      formatter.format(new Date(cert.validity.notBefore / 1000)),
+      formatter.format(new Date(cert.validity.notAfter / 1000)),
     ]),
   ];
   let parsedCert = await parse(pemToDER(cert.getBase64DERString()));

security/manager/ssl/X509CertValidity.cpp

@@ -4,13 +4,9 @@
 
 #include "X509CertValidity.h"
 
-#include "mozilla/Assertions.h"
 #include "mozpkix/pkixder.h"
 #include "mozpkix/pkixutil.h"
-#include "nsCOMPtr.h"
 #include "nsComponentManagerUtils.h"
-#include "nsReadableUtils.h"
-#include "seccomon.h"
 #include "secder.h"
 
 NS_IMPL_ISUPPORTS(X509CertValidity, nsIX509CertValidity)
@@ -87,40 +83,6 @@ X509CertValidity::GetNotBefore(PRTime* aNotBefore) {
   return NS_OK;
 }
 
-nsresult X509CertValidity::FormatTime(
-    const PRTime& aTimeDate, PRTimeParamFn aParamFn,
-    const Maybe<intl::DateTimeFormat::Style> aTimeStyle,
-    nsAString& aFormattedTimeDate) {
-  if (!mTimesInitialized) return NS_ERROR_FAILURE;
-
-  PRExplodedTime explodedTime;
-  PR_ExplodeTime(const_cast<PRTime&>(aTimeDate), aParamFn, &explodedTime);
-  intl::DateTimeFormat::StyleBag style;
-  style.date = Some(intl::DateTimeFormat::Style::Long);
-  style.time = aTimeStyle;
-  return intl::AppDateTimeFormat::Format(style, &explodedTime,
-                                         aFormattedTimeDate);
-}
-
-NS_IMETHODIMP
-X509CertValidity::GetNotBeforeLocalTime(nsAString& aNotBeforeLocalTime) {
-  return FormatTime(mNotBefore, PR_LocalTimeParameters,
-                    Some(intl::DateTimeFormat::Style::Long),
-                    aNotBeforeLocalTime);
-}
-
-NS_IMETHODIMP
-X509CertValidity::GetNotBeforeLocalDay(nsAString& aNotBeforeLocalDay) {
-  return FormatTime(mNotBefore, PR_LocalTimeParameters, Nothing(),
-                    aNotBeforeLocalDay);
-}
-
-NS_IMETHODIMP
-X509CertValidity::GetNotBeforeGMT(nsAString& aNotBeforeGMT) {
-  return FormatTime(mNotBefore, PR_GMTParameters,
-                    Some(intl::DateTimeFormat::Style::Long), aNotBeforeGMT);
-}
-
 NS_IMETHODIMP
 X509CertValidity::GetNotAfter(PRTime* aNotAfter) {
   NS_ENSURE_ARG(aNotAfter);
@@ -132,22 +94,3 @@ X509CertValidity::GetNotAfter(PRTime* aNotAfter) {
   *aNotAfter = mNotAfter;
   return NS_OK;
 }
-
-NS_IMETHODIMP
-X509CertValidity::GetNotAfterLocalTime(nsAString& aNotAfterLocaltime) {
-  return FormatTime(mNotAfter, PR_LocalTimeParameters,
-                    Some(intl::DateTimeFormat::Style::Long),
-                    aNotAfterLocaltime);
-}
-
-NS_IMETHODIMP
-X509CertValidity::GetNotAfterLocalDay(nsAString& aNotAfterLocalDay) {
-  return FormatTime(mNotAfter, PR_LocalTimeParameters, Nothing(),
-                    aNotAfterLocalDay);
-}
-
-NS_IMETHODIMP
-X509CertValidity::GetNotAfterGMT(nsAString& aNotAfterGMT) {
-  return FormatTime(mNotAfter, PR_GMTParameters,
-                    Some(intl::DateTimeFormat::Style::Long), aNotAfterGMT);
-}

security/manager/ssl/X509CertValidity.h

@@ -5,7 +5,6 @@
 #ifndef X509CertValidity_h
 #define X509CertValidity_h
 
-#include "mozilla/intl/AppDateTimeFormat.h"
 #include "mozpkix/Input.h"
 #include "nsIX509CertValidity.h"
 #include "prtime.h"
@@ -24,11 +23,6 @@ class X509CertValidity : public nsIX509CertValidity {
   virtual ~X509CertValidity() = default;
 
  private:
-  nsresult FormatTime(
-      const PRTime& aTime, PRTimeParamFn aParamFn,
-      const mozilla::Maybe<mozilla::intl::DateTimeFormat::Style> aTimeStyle,
-      nsAString& aFormattedTimeDate);
-
   PRTime mNotBefore;
   PRTime mNotAfter;
   bool mTimesInitialized;

security/manager/ssl/nsCertTree.cpp

@@ -6,6 +6,8 @@
 
 #include "ScopedNSSTypes.h"
 #include "mozilla/Logging.h"
+#include "mozilla/Maybe.h"
+#include "mozilla/intl/AppDateTimeFormat.h"
 #include "nsArray.h"
 #include "nsArrayUtils.h"
 #include "nsHashKeys.h"
@@ -533,6 +535,15 @@ nsCertTree::GetCellValue(int32_t row, nsTreeColumn* col, nsAString& _retval) {
   return NS_OK;
 }
 
+static void PRTimeToLocalDateString(PRTime time, nsAString& result) {
+  PRExplodedTime explodedTime;
+  PR_ExplodeTime(time, PR_LocalTimeParameters, &explodedTime);
+  intl::DateTimeFormat::StyleBag style;
+  style.date = Some(intl::DateTimeFormat::Style::Long);
+  style.time = Nothing();
+  Unused << intl::AppDateTimeFormat::Format(style, &explodedTime, result);
+}
+
 NS_IMETHODIMP
 nsCertTree::GetCellText(int32_t row, nsTreeColumn* col, nsAString& _retval) {
   if (!mTreeArray) return NS_ERROR_NOT_INITIALIZED;
@@ -588,14 +599,22 @@ nsCertTree::GetCellText(int32_t row, nsTreeColumn* col, nsAString& _retval) {
 
     rv = cert->GetValidity(getter_AddRefs(validity));
     if (NS_SUCCEEDED(rv)) {
-      validity->GetNotBeforeLocalDay(_retval);
+      PRTime notBefore;
+      rv = validity->GetNotBefore(&notBefore);
+      if (NS_SUCCEEDED(rv)) {
+        PRTimeToLocalDateString(notBefore, _retval);
+      }
     }
   } else if (u"expiredcol"_ns.Equals(colID) && cert) {
     nsCOMPtr<nsIX509CertValidity> validity;
 
     rv = cert->GetValidity(getter_AddRefs(validity));
     if (NS_SUCCEEDED(rv)) {
-      validity->GetNotAfterLocalDay(_retval);
+      PRTime notAfter;
+      rv = validity->GetNotAfter(&notAfter);
+      if (NS_SUCCEEDED(rv)) {
+        PRTimeToLocalDateString(notAfter, _retval);
+      }
     }
   } else if (u"serialnumcol"_ns.Equals(colID) && cert) {
     rv = cert->GetSerialNumber(_retval);

security/manager/ssl/nsIX509CertValidity.idl

@@ -11,62 +11,15 @@
  */
 [scriptable, uuid(e701dfd8-1dd1-11b2-a172-ffa6cc6156ad)]
 interface nsIX509CertValidity : nsISupports {
-
   /**
    *  The earliest point in time where
    *  a certificate is valid.
    */
   readonly attribute PRTime notBefore;
 
-  /**
-   *  "notBefore" attribute formatted as a time string
-   *  according to the environment locale,
-   *  according to the environment time zone.
-   */
-  [must_use]
-  readonly attribute AString notBeforeLocalTime;
-
-  /**
-   *  The day portion of "notBefore" formatted as a time string
-   *  according to the environment locale,
-   *  according to the environment time zone.
-   */
-  readonly attribute AString notBeforeLocalDay;
-
-  /**
-   *  "notBefore" attribute formatted as a string
-   *  according to the environment locale,
-   *  displayed as GMT / UTC.
-   */
-  [must_use]
-  readonly attribute AString notBeforeGMT;
-
   /**
    *  The latest point in time where
    *  a certificate is valid.
    */
   readonly attribute PRTime notAfter;
-
-  /**
-   *  "notAfter" attribute formatted as a time string
-   *  according to the environment locale,
-   *  according to the environment time zone.
-   */
-  [must_use]
-  readonly attribute AString notAfterLocalTime;
-
-  /**
-   *  The day portion of "notAfter" formatted as a time string
-   *  according to the environment locale,
-   *  according to the environment time zone.
-   */
-  readonly attribute AString notAfterLocalDay;
-
-  /**
-   *  "notAfter" attribute formatted as a time string
-   *  according to the environment locale,
-   *  displayed as GMT / UTC.
-   */
-  [must_use]
-  readonly attribute AString notAfterGMT;
 };

security/manager/ssl/tests/mochitest/browser/browser_clientAuth_ui.js

@@ -60,9 +60,9 @@ function openClientAuthDialog(cert) {
  *
  * @param {window} win The cert chooser window.
  * @param {String} notBefore
- *        The notBeforeLocalTime attribute of mochitest.client.
+ *        The formatted notBefore date of mochitest.client.
  * @param {String} notAfter
- *        The notAfterLocalTime attribute of mochitest.client.
+ *        The formatted notAfter date of mochitest.client.
  */
 function checkDialogContents(win, notBefore, notAfter) {
   is(
@@ -143,11 +143,15 @@ add_task(async function setup() {
 // Test that the contents of the dialog correspond to the details of the
 // provided cert.
 add_task(async function testContents() {
+  const formatter = new Intl.DateTimeFormat(undefined, {
+    dateStyle: "medium",
+    timeStyle: "long",
+  });
   let [win] = await openClientAuthDialog(cert);
   checkDialogContents(
     win,
-    cert.validity.notBeforeLocalTime,
-    cert.validity.notAfterLocalTime
+    formatter.format(new Date(cert.validity.notBefore / 1000)),
+    formatter.format(new Date(cert.validity.notAfter / 1000))
   );
   await BrowserTestUtils.closeWindow(win);
 });

security/manager/ssl/tests/unit/test_nsIX509CertValidity.js

@@ -5,17 +5,6 @@
 
 // This file tests the nsIX509CertValidity implementation.
 
-function fuzzyEqual(attributeName, dateString, expectedTime) {
-  info(`${attributeName}: ${dateString}`);
-  let absTimeDiff = Math.abs(expectedTime - Date.parse(dateString));
-  const ONE_DAY_IN_MS = 24 * 60 * 60 * 1000;
-  lessOrEqual(
-    absTimeDiff,
-    ONE_DAY_IN_MS,
-    `Time difference for ${attributeName} should be <= one day`
-  );
-}
-
 function run_test() {
   // Date.parse("2013-01-01T00:00:00Z")
   const NOT_BEFORE_IN_MS = 1356998400000;
@@ -33,38 +22,4 @@ function run_test() {
     NOT_AFTER_IN_MS * 1000,
     "Actual and expected notAfter should be equal"
   );
-
-  // The following tests rely on the implementation of nsIX509CertValidity
-  // providing long formatted dates to work. If this is not true, a returned
-  // short formatted date such as "12/31/12" may be interpreted as something
-  // other than the expected "December 31st, 2012".
-  //
-  // On Android, the implementation of nsIDateTimeFormat currently does not
-  // return a long formatted date even if it is asked to. This, combined with
-  // the reason above is why the following tests are disabled on Android.
-  if (AppConstants.platform != "android") {
-    fuzzyEqual(
-      "notBeforeLocalTime",
-      cert.validity.notBeforeLocalTime,
-      NOT_BEFORE_IN_MS
-    );
-    fuzzyEqual(
-      "notBeforeLocalDay",
-      cert.validity.notBeforeLocalDay,
-      NOT_BEFORE_IN_MS
-    );
-    fuzzyEqual("notBeforeGMT", cert.validity.notBeforeGMT, NOT_BEFORE_IN_MS);
-
-    fuzzyEqual(
-      "notAfterLocalTime",
-      cert.validity.notAfterLocalTime,
-      NOT_AFTER_IN_MS
-    );
-    fuzzyEqual(
-      "notAfterLocalDay",
-      cert.validity.notAfterLocalDay,
-      NOT_AFTER_IN_MS
-    );
-    fuzzyEqual("notAfterGMT", cert.validity.notAfterGMT, NOT_AFTER_IN_MS);
-  }
 }