954195 - JS-Socket fails to properly handle SSL errors

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 761 at 2011-04-21 17:30:00 UTC ***

As reported by varuna, the JS-Socket coughs on SSL errors, seems I'm not actually telling Mozilla what interfaces Socket implements (i.e. the getInterface method needs to be added).

This is probably something that needs to be taken care of before we can say JS-protocols really work.

11:58:49 <varuna> clokep_work: hi
11:59:22 <varuna> is the socket.js in the irc extension handling invalid certificates?
11:59:23 <clokep_work> Hello varuna.
11:59:53 <clokep_work> I'm not sure if it'll handle invalid certificates or not, it should at least throw into the SSL error handler I believe.
11:59:53 <varuna> it didn't work for me :P ... and got it working a bit after changing some stuff (add the getInterface method etc)
12:00:20 <varuna> but still the adding exception dialog didn't work when opened from he extension
12:00:28 <varuna> yeah it gave the ssl error
12:01:05 <clokep_work> It's quite possible it's not implemented fully. It was only briefly tested.
12:01:12 <varuna> i was trying to login to gtalk with starttls and failed
12:01:17 <varuna> worked with ssl easily though
12:01:21 <varuna> ok
12:01:37 <clokep_work> Wait, you mean you tried to log in without SSL and it failed, but worked OK w/ SSL?
...
12:01:55 <varuna> no
12:01:58 <varuna> with starttls
12:02:07 <clokep_work> Ah, I see.
12:02:09 <varuna> the server said tls required
12:02:12 <varuna> i do start tls
12:02:15 <varuna> and proceed
12:02:19 <clokep_work> Got it.
...
12:04:21 <clokep_work> Do you happen to have a diff of what you've changed? I'm just curious. :)
12:05:45 <varuna> it's filled with a lot of junk code i think :P
12:05:53 <clokep_work> Ah OK. Don't worry about it then.
12:06:02 <varuna> the main changes were
12:07:09 <varuna> adding the method
12:07:25 <varuna> getInterface to this.transport.securityCallbacks
12:07:38 <varuna> without that it didn't seem to be invoking the badcert callback
12:08:10 <clokep_work> Oh, OK.
12:09:13 <varuna> and handling the cert to open up the exceptions dialog
12:09:19 <varuna> which didn't quite work for me
12:09:26 <varuna> the dialog opened but was kind of stuck
12:09:44 <clokep_work> Hmmmm...I see.

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 761 at 2011-04-21 17:32:56 UTC ***

Meant to include a link to the discussion from IRC, even though it's all here pretty much: http://log.bezut.info/instantbird/110418/#m50

Comment 2

[Florian Quèze [:florian]]

*** Original post on bio 761 at 2011-04-21 18:08:52 UTC ***

(In reply to comment #0)
> seems I'm not
> actually telling Mozilla what interfaces Socket implements (i.e. the
> getInterface method needs to be added).

You mean 'it needs to implement nsIClassInfo', right? ;)

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 761 at 2011-04-21 18:13:25 UTC ***

(In reply to comment #2)
> (In reply to comment #0)
> > seems I'm not
> > actually telling Mozilla what interfaces Socket implements (i.e. the
> > getInterface method needs to be added).
> You mean 'it needs to implement nsIClassInfo', right? ;)

Yes, I mean we need it to implement nsIClassInfo. Which just means we need to use the ClassInfo helper you wrote. I'm not sure if that'll fix the issue with the dialog, however.

Comment 4

[Patrick Cloke [:clokep]]

*** Original post on bio 761 at 2011-04-29 01:23:27 UTC ***

I think this is mostly fixed by [1], I have some more testing to do though. This lets me connect to freenode over SSL now, which I wasn't able to previously. I'm going to test this with varuna's code tonight or tomorrow. (This is pretty much the code varuna has in his repository [2], combined with some code I had written the other night and reformatted.) I want to perform some more testing with this before formalizing it into a patch (and seeing if any other changes to the socket code need to be done.)

[1] - http://hg.instantbird.org/experiments/rev/cb0b3803edf8 and http://hg.instantbird.org/experiments/rev/79615f6717ab
[2] - https://github.com/vpj/xmpp-js/blob/master/modules/socket.jsm

Comment 5

[Patrick Cloke [:clokep]]

*** Original post on bio 761 at 2011-09-09 13:51:48 UTC ***

I need to verify these changes and create a patch.

Comment 6

[Patrick Cloke [:clokep]]

Attachment: Current patch

*** Original post on bio 761 as attmnt 948 at 2011-10-27 00:12:00 UTC ***

This patch might contain a few more changes than just the SSL changes, but it's what I have currently and I wanted to get it up here.

Comment 7

[Patrick Cloke [:clokep]]

*** Original post on bio 761 at 2011-11-16 17:26:19 UTC ***

A random thought: this patch should probably add varuna's name to the header block as I adapted quite a bit of his code for this.

Comment 8

[Florian Quèze [:florian]]

*** Original post on bio 761 at 2011-11-24 14:46:46 UTC ***

(In reply to comment #2)
> (In reply to comment #0)
> > seems I'm not
> > actually telling Mozilla what interfaces Socket implements (i.e. the
> > getInterface method needs to be added).
> 
> You mean 'it needs to implement nsIClassInfo', right? ;)

I'm afraid I mislead you here, I hadn't looked enough into the issue at the time I wrote this comment :-(. The getInterface Varuna was talking about is the nsIInterfaceRequestor interface, and I don't think we really need nsIClassInfo here.

Comment 9

[Florian Quèze [:florian]]

Attachment: Patch v2

*** Original post on bio 761 as attmnt 1019 at 2011-11-24 15:24:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8352690 [details] [diff] [review]
Current patch

*** Original change on bio 761 attmnt 948 at 2011-11-24 15:24:08 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8352762 [details] [diff] [review]
Patch v2

*** Original change on bio 761 attmnt 1019 at 2011-11-24 15:51:41 UTC ***

This looks good. Thanks for fixing my bug with the readWriteTimeout! :) I agree the API can probably use a once over, but let's handle that in a different bug.

Comment 12

[Patrick Cloke [:clokep]]

*** Original post on bio 761 at 2011-11-26 13:33:37 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/13280f6f3093 thanks for fixing this!