Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get recent bad certs #157

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Get recent bad certs #157

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 10, 2014

@daveschaefer: from short guide on the proper way to get the exceptions dialog we are apparently supposed to retrieve the nsITransportSecurityInfo and hence the SSLState in the onSecurityChange() method of nsIWebProgressListener. Could you please verify if this works as intended! I chose to implement this a hotfix as I might find a cleaner way with all the tabinfo_restructuring anyway (maybe it's already implemented, I don't remember) which makes heavy use of the nsIWebProgressListener functionality.

@ghost
Copy link
Author

ghost commented Nov 15, 2014

Bump!

@ghost
Copy link
Author

ghost commented Nov 16, 2014

This is superseded by the merge_tabinfo_restructuring. But the questions still apply.

@daveschaefer
Copy link
Collaborator

@lambdor By "superceded" do you mean you no longer want me to merge this pull request? If you still want both is there a particular order you need them in? Otherwise I will test this small change first, merge, and then test the larger PR.

@ghost
Copy link
Author

ghost commented Nov 17, 2014

By "superceded" do you mean you no longer want me to merge this pull request?
If you still want both is there a particular order you need them in?

The large PR takes longer to verify and the getRecentBadCerts fix is implemented the same way. If we want to get a new update out soon you should merge and publish the smaller one. But if we are going to release the tabinfo restructuring later we don't need this changes.

@daveschaefer
Copy link
Collaborator

Roger; I'll test and pull this in first so we can release. The AMO process hasn't released the v4.6 update yet, so it will be interesting to see what happens if we send two updates into the queue at once :)

@daveschaefer
Copy link
Collaborator

@lambdor Okay, I tried running the code from this PR out of the box, but when I do things like open a new blank tab I get this error: "an internal error occurred: onSecurityChange - TypeError: aWebProgress.SecurityInfo is undefined".

I think perhaps we need some additional checks to make sure we're dealing with security change events of the correct type. I think we should also remove the aBrowser parameter so there is no confusion on what is being passed.

What do you think of adding some error checks like this? daveschaefer@f559888

Here are my test cases:

  • Can visit https site with CA-approved HTTPS cert, and it works normally
  • Can visit https site with self-signed cert, and it works normally
    x firefox security error for the site is automatically overridden, and you don't see the warning page inbetween (this doesn't seem to work for me, even in the main branch. We can address it after your PR is in)
  • Can open blank tabs open normally and the Perspectives icon remains blue with no error (about:blank)

Do you have any good examples of sites that trigger the "getRecentBadCerts is not a function" error (#143) in v4.6? I have tried several of the reported sites - https://www.pcwebshop.co.uk/ , https://lists.bufferbloat.net/ - but I am not able to reliably trigger the error, which makes it difficult to have confidence in the test results.

@ghost
Copy link
Author

ghost commented Jan 3, 2015

On 2014-12-07 22:00, Dave wrote:

@lambdor https://github.com/lambdor Okay, I tried running the code
from this PR out of the box, but when I do things like open a new blank
tab I get this error: "an internal error occurred: onSecurityChange -
TypeError: aWebProgress.SecurityInfo is undefined".

You're right. I get "aWebProgress is null" though. (FF 35a2)

I think perhaps we need some additional checks to make sure we're
dealing with security change events of the correct type. I think we
should also remove the aBrowser parameter so there is no confusion on
what is being passed.

Yes. I just took over the parameter list from the "restructuring" branch
which however uses a TabProgressListener (with aBrowser parameters)
instead of a plain ProgressListener. Please also keep that in mind when
working with the restructured branch!

What do you think of adding some error checks like this?
daveschaefer/Perspectives@f559888
daveschaefer@f559888

Sure, if it works. Personally I don't like the multiple-return-points
style though but it's a hot-fix anyway.

Do you have any good examples of sites that trigger the
"getRecentBadCerts is not a function" error (#143
#143) in v4.6? I have
tried several of the reported sites - https://www.pcwebshop.co.uk/ ,
https://lists.bufferbloat.net/ - but I am not able to reliably trigger
the error, which makes it difficult to have confidence in the test results.

From what I remember this error occured with pretty much every site
because the function was removed in FF 33 and this PR is the very fix to
remove the error :) If you still get this error then it's a bug.

Btw: Do you have any idea why the 4.6 version is still not released on AMO?

Did you already have a chance to look at and test the restructuring branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants