1075081 - Enhance pinning test to ensure the neterror page is actually the one found

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Camilo Viecco (:cviecco)]

On landing of  787133 I missed checking that the actual error was the pinning failure and not other. (Tough this check is implied by the subsequent passing with removal of pins).

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: add-checking-is-pinnning-error

Comment 2

[Camilo Viecco (:cviecco)]

Attachment: add-checking-is-pinnning-error

Comment 3

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8497723 [details] [diff] [review]
add-checking-is-pinnning-error

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

Looks like there's still some cleanup to do.

::: browser/base/content/test/general/browser_blockHPKP.js
@@ +70,3 @@
>  // to load the pinning domain again, this time removing the pinning information
>  let certErrorProgressListener = {
>    buttonClicked: false,

don't need this

@@ +71,5 @@
>  let certErrorProgressListener = {
>    buttonClicked: false,
>    onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
>      if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
>        let self = this;

probably don't need this

@@ +73,5 @@
>    onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
>      if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
>        let self = this;
>        // Can't directly call button.click() in onStateChange
>        executeSoon(function() {

we're not calling button.click(), so we probably don't need the executeSoon

@@ +79,1 @@
>          // If about:neterror hasn't fully loaded, the button won't be present.

again, we don't care about the button, but it looks like we care about 'errorShortDescText'

@@ +79,3 @@
>          // If about:neterror hasn't fully loaded, the button won't be present.
>          // It will eventually be there, however.
>          if (button && !self.buttonClicked) {

again, this isn't what we're predicating on

@@ +81,5 @@
>          if (button && !self.buttonClicked) {
> +          let textElement = content.document.getElementById("errorShortDescText");
> +          let text = textElement.innerHTML;
> +          ok(text.indexOf("mozilla_pkix_error_key_pinning_failure") > 0,
> +             "Got an pinning error page");

nit: "a pinning..."

@@ +86,1 @@
>            gBrowser.removeProgressListener(self);

without the executeSoon, we can use 'this' instead of 'self'

Comment 4

[Camilo Viecco (:cviecco)]

Attachment: add-checking-is-pinnning-error (v2)

Comment 5

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8498930 [details] [diff] [review]
add-checking-is-pinnning-error (v2)

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

Great.

Comment 6

[Camilo Viecco (:cviecco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/944246c0d39f

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/944246c0d39f