1174389 - Add result strings to PSM OCSP xpcshell tests

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

To make debugging easier, result strings should be added to the PSM test_ocsp* xpcshell tests.

Comment 1

[:Cykesiopka]

Attachment: MozReview Request: Bug 1174389 - Add result strings to PSM OCSP xpcshell tests.

Bug 1174389 - Add result strings to PSM OCSP xpcshell tests.

Comment 2

[:Cykesiopka]

https://reviewboard.mozilla.org/r/12185/#review10647

Apologies ahead of time if some of the strings turn out to be incorrect - I had a harder time writing strings for this batch of tests for some reason.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf3e4e136fff

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:162
(Diff revision 1)
> -  add_test(function() { do_check_eq(gFetchCount, 1); run_next_test(); });
> +  add_test(function() { 

I'll fix this before check-in.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8627071 [details]
MozReview Request: Bug 1174389 - Add result strings to PSM OCSP xpcshell tests.

https://reviewboard.mozilla.org/r/12187/#review10763

Looks great. I just had some suggestions for wording.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:7
(Diff revision 1)
> +// Checks various aspects of the OCSP cache, mainly to ensure it does not

Maybe something more like "to ensure we do not fetch responses more than necessary".

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:97
(Diff revision 1)
> +          "Cached Unknown case: fetch count should be 0 after getting a" +

Maybe "Should not have attempted to fetch OCSP given a stapled Unknown response" or something.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:109
(Diff revision 1)
> +          "Cached Unknown case: fetch count should be 1 after not getting a" +

Maybe "no stapled OCSP response, so a fetch should have been attempted"

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:135
(Diff revision 1)
> +          "Responder override case: fetch count should be 2 after not getting" +

Maybe "another fetch should have been attempted" or something.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:147
(Diff revision 1)
> +          "Responder override case: fetch count should still be 2 after not" +

How about "cached Good response should have prevented attempting to fetch another OCSP response"

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:164
(Diff revision 1)
> +          "Error entry case: fetch count should be 1 after not getting a" +

Maybe "no stapled response -> should have attempted to fetch OCSP"

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:174
(Diff revision 1)
> +          "Error entry case: fetch count should still be 1 after not getting" +

"Noted server failure -> should not have attempted to fetch another response"

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:186
(Diff revision 1)
> +          "Error entry case: fetch count should still be 1 after getting a" +

"Stapled Revoked response -> should not have attempted to fetch another response"

::: security/manager/ssl/tests/unit/test_ocsp_no_hsts_upgrade.js:50
(Diff revision 1)
> +     "Domain for the OCSP AIA URI should be considered a HSTS host");

This is more of a consistency check to ensure we're testing what we think we're testing.

::: security/manager/ssl/tests/unit/test_ocsp_stapling_with_intermediate.js:42
(Diff revision 1)
> +          "Should have 0 OCSP requests for an intermediate that signed a cert" +

This should probably be "no OCSP requests should have been made"

::: security/manager/ssl/tests/unit/test_ocsp_timeout.js:74
(Diff revision 1)
> +         "Hard-fail time difference (with fuzz) should be greater than 10s");

Maybe "automatic OCSP timeout should be about 10s for hard-fail"

::: security/manager/ssl/tests/unit/test_ocsp_timeout.js:77
(Diff revision 1)
> +         "Soft-fail time difference (with fuzz) should be greater than 2s");

(similarly here)

::: security/manager/ssl/tests/unit/test_ocsp_timeout.js:83
(Diff revision 1)
> -    do_check_true(timeDifference < 60000);
> +    ok(timeDifference < 60000, "Time difference should be less than 60s");

Maybe "automatic OCSP timeout shouldn't be more than 60 seconds"

Comment 4

[:Cykesiopka]

Attachment: bug1174389_add-result-strings-ocsp_v2.patch

+ Make suggested wording changes, with modifications

Comment 5

[:Cykesiopka]

Thanks for the review!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25f43815b8b6

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1918e65eeb04

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1918e65eeb04