fix webRequest.getSecurityInfo validity timestamps and test

RESOLVED FIXED in Firefox 62

Status

defect
P1
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

unspecified
mozilla63
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox62 fixed, firefox63 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

11 months ago
A doc comment on bug 1322748 regarding the validity timestamps clued me into the error I created in the test for bug 1470516.  This fixes that test, and may as well make the timestamp milliseconds to be consistent (and directly usable with js Date) with other timestamps in webext apis.
Comment hidden (mozreview-request)

Comment 2

11 months ago
mozreview-review
Comment on attachment 8991045 [details]
Bug 1474626 - fix timestamp test and values,

https://reviewboard.mozilla.org/r/256032/#review262896

::: toolkit/modules/addons/SecurityInfo.jsm:217
(Diff revision 1)
>  
>      let certData = {
>        subject: cert.subjectName,
>        issuer: cert.issuerName,
>        validity: {
> -        start: cert.validity.notBefore,
> +        start: cert.validity.notBefore && cert.validity.notBefore / 1000,

Should we also ensure that the result is still an integer number? (e.g. using Math.round, Math.trunc or Math.floor)

(and if we should, it may be also worth to make a related assertion in the test case).
Attachment #8991045 - Flags: review?(lgreco)
Comment hidden (mozreview-request)

Comment 4

11 months ago
mozreview-review
Comment on attachment 8991045 [details]
Bug 1474626 - fix timestamp test and values,

https://reviewboard.mozilla.org/r/256032/#review263148

::: toolkit/modules/addons/SecurityInfo.jsm:217
(Diff revisions 1 - 2)
>  
>      let certData = {
>        subject: cert.subjectName,
>        issuer: cert.issuerName,
>        validity: {
> -        start: cert.validity.notBefore && cert.validity.notBefore / 1000,
> +        start: cert.validity.notBefore ? Math.floor(cert.validity.notBefore / 1000) : 0,

Not sure if it would really make a huge difference, but it may worth to mention that with `Math.floor`:

```
Math.floor(100.1) => 100
Math.floor(100.6) => 100
Math.floor(100.8) => 100
Math.floor(-100.8) => -101
Math.floor(-100.1) => -101
```

On the contrary with `Math.round`:

```
Math.round(100.1) => 100
Math.round(100.6) => 101
Math.round(100.8) => 101
Math.round(-100.8) => -101
Math.round(-100.1) => -100
```

How about adding an explicit assertion to verify that these properties are going to be integer as expected?

I guess that something like `browser.test.assertEq(Math.trunc(cert.validity.start), cert.validity.start, "Got an integer cert.validity.start number as expected");` should be enough.
Attachment #8991045 - Flags: review?(lgreco)
Comment hidden (mozreview-request)

Comment 6

11 months ago
mozreview-review
Comment on attachment 8991045 [details]
Bug 1474626 - fix timestamp test and values,

https://reviewboard.mozilla.org/r/256032/#review263152

Thanks Shane, it looks good, there is only a small issue with one of the assertion that it seems to be actually copied twice by mistake.

I'm setting r+ it in the meantime (with the above comment), because it is a super small issue in the test case and a new review round doesn't really seem necessary.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_hsts.html:44
(Diff revisions 2 - 3)
>        }
>        let cert = securityInfo.certificates[0];
>        let now = Date.now();
> +      browser.test.assertTrue(Number.isInteger(cert.validity.start), "cert start is integer");
> +      browser.test.assertTrue(Number.isInteger(cert.validity.end), "cert end is integer");
> +      browser.test.assertTrue(cert.validity.start < now, "cert start validity is correct");

is reviewboard that is messing with this interdiff or `browser.test.assertTrue(cert.validity.start < now, "cert start validity is correct");` is actually done twice here by mistake?
Attachment #8991045 - Flags: review?(lgreco) → review+
Comment hidden (mozreview-request)

Comment 8

11 months ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8a4ba4e8e53e
fix timestamp test and values, r=rpl

Comment 9

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8a4ba4e8e53e
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee

Comment 10

11 months ago
Comment on attachment 8991045 [details]
Bug 1474626 - fix timestamp test and values,

Approval Request Comment
[Feature/Bug causing the regression]: We didn't catch the logic error in the tests for bug 1470516 which resulted in the api being slightly wrong.
[User impact if declined]: Incorrect time stamp values (microseconds in place of milliseconds) in ssl api
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: minimal change with fixed tests
[String changes made/needed]: none
Attachment #8991045 - Flags: approval-mozilla-beta?
Comment on attachment 8991045 [details]
Bug 1474626 - fix timestamp test and values,

Fix for upcoming new feature, let's get this onto beta 8.
Attachment #8991045 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 13

11 months ago
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(mixedpuppy)
Assignee

Comment 14

11 months ago
The change here is api level so you'd have to write something using the api to validate.  Given that it has a unit test I don't think further qa is necessary.  

April has written an extensive cert viewer extension using this api, so I'll just ask for external dev validation of this change based on that work.
Flags: qe-verify-
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(april)

Comment 15

11 months ago
I use the dates inside the DER, not the dates sent by the API. But I would be happy to take a look at it and verify that it looks correct. Is it pushed into Nightly?
Flags: needinfo?(april)
You need to log in before you can comment on or make changes to this bug.