Closed Bug 1075081 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: cviecco, Assigned: cviecco)

References

Details

Attachments

(1 file, 2 obsolete files)

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).
Attached patch add-checking-is-pinnning-error (obsolete) — Splinter Review
Attached patch add-checking-is-pinnning-error (obsolete) — Splinter Review
Attachment #8497720 - Attachment is obsolete: true
Attachment #8497723 - Flags: review?(dkeeler)
Assignee: nobody → cviecco
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'
Attachment #8497723 - Flags: review?(dkeeler) → review-
Attachment #8497723 - Attachment is obsolete: true
Attachment #8498930 - Flags: review?(dkeeler)
Comment on attachment 8498930 [details] [diff] [review]
add-checking-is-pinnning-error (v2)

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

Great.
Attachment #8498930 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/944246c0d39f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.