Skip to content

Certain MODE messages could access on undefined #144

Closed
awwright opened this Issue Feb 2, 2013 · 12 comments

7 participants

@awwright
awwright commented Feb 2, 2013

Presently I'm looking at https://github.com/martynsmith/node-irc/blob/35ee099b0517f47c51577403973b7416d1d29201/lib/irc.js#L229-L239 which does not verify that "user" is actually a user present in the channel.

This has occasionally crashed when a non-user value is passed as an argument:

[01 06:44:12] *** Mode #defocus +q *!*@adsl-76-237-180-172.dsl.chcgil.sbcglobal.net by mrmist
 TypeError: Cannot call method 'indexOf' of undefined
     at irc/lib/irc.js:233:54

Usually it works fine though, and I can't quite figure out why. But examining the code, almost certainly the server is capable of crafting a response that will throw a TypeError, disconnecting the IRC connection, and this should never happen.

On a similar note, the debug should probably look more like so:

                if ( self.opt.debug )
                    util.log("MODE:" + message.args[0] + " sets mode: " + message.args.slice(1).join(' '));
@awwright
awwright commented Feb 2, 2013

Here's another error, after modifying the file like so:

                        // channel user modes
                        var user = modeArgs.shift();console.log('MODE', user, channel.users[user]);
1 Feb 20:51:45 - MODE:#defocus sets mode: +o eir
MODE eir 
1 Feb 20:51:46 - MODE:#defocus sets mode: -qo *!*@adsl-76-237-180-172.dsl.chcgil.sbcglobal.net eir
MODE *!*@adsl-76-237-180-172.dsl.chcgil.sbcglobal.net undefined

irc/lib/irc.js:637
                    throw err;
                          ^
TypeError: Cannot call method 'replace' of undefined
    at irc/lib/irc.js:239:71
    at Array.forEach (native)
    at Client.<anonymous> (irc/lib/irc.js:226:26)
    at Client.EventEmitter.emit (events.js:126:20)
    at irc/lib/irc.js:634:22
    at Array.forEach (native)
    at Socket.<anonymous> (irc/lib/irc.js:631:15)
    at Socket.EventEmitter.emit (events.js:96:17)
    at TCP.onread (net.js:392:31)

Line in question: https://github.com/martynsmith/node-irc/blob/35ee099b0517f47c51577403973b7416d1d29201/lib/irc.js#L239

@awwright
awwright commented Feb 2, 2013

It seems that it's not actually shifting off arguments when it's supposed to be:

1 Feb 21:13:45 - MODE:#defocus sets mode: +o eir
MODE o { o: '@', v: '+' } eir 
1 Feb 21:13:46 - MODE:#defocus sets mode: -qo *!*@gateway/web/freenode/ip.98.251.112.151 eir
MODE o { o: '@', v: '+' } *!*@gateway/web/freenode/ip.98.251.112.151 undefined

with the following line changed:

                        var user = modeArgs.shift();console.log('MODE', mode, self.prefixForMode, user, channel.users[user]);
@Fauntleroy

I think I just ran into a similar issue--mode change caused node-irc to attempt to run a method on undefined:

+MODE #freenode ChanServ o eir
-MODE #freenode eir q undefined

C:\Users\Timothy\Repos\irchub\node_modules\irc\lib\irc.js:548
                    throw err;
                          ^
TypeError: Cannot call method 'replace' of undefined
    at self.addListener.channels (C:\Users\Timothy\Repos\irchub\node_modules\irc\lib\irc.js:156:71)
    at Array.forEach (native)
    at Client.<anonymous> (C:\Users\Timothy\Repos\irchub\node_modules\irc\lib\irc.js:143:26)
    at Client.EventEmitter.emit (events.js:126:20)
    at Client.connect (C:\Users\Timothy\Repos\irchub\node_modules\irc\lib\irc.js:545:22)
    at Array.forEach (native)
    at Socket.Client.connect (C:\Users\Timothy\Repos\irchub\node_modules\irc\lib\irc.js:542:15)
    at Socket.EventEmitter.emit (events.js:96:17)
    at TCP.onread (net.js:391:31)
@awwright
awwright commented Feb 2, 2013

I believe the problem lies here:

                        var modeArg;
                        // channel modes
                        if ( mode.match(/^[bkl]$/) ) {
                            modeArg = modeArgs.shift();
                            if ( modeArg.length === 0 )
                                modeArg = undefined;
                        }

The module is not handling Freenode's +-q argument, and so the +-o mode is getting the argument intended for +-q. This can be replicated in any channel by doing the same, OPing yourself, then passing MODE -qo pattern opuser

I'd make a few changes here, first, the length property is typically reserved for Arrays, for strings, standard boolean identity suffices, and prevents a length lookup on modeArg if it's undefined due to there being no more arguments to be shifted off:

                        var modeArg;
                        // channel modes
                        if ( mode.match(/^[bkl]$/) ) {
                            modeArg = modeArgs.shift() || undefined;
                        }

Second, the module needs to respect the CHANMODES property, and properly differentiate between channel-user modes, channel modes with arguments, channel-user-address modes, and no-argument modes, and emit an error when it doesn't understand one of the modes not explicitly on one of those four lists.

This is hinted nicely on line 252: https://github.com/martynsmith/node-irc/blob/35ee099b0517f47c51577403973b7416d1d29201/lib/irc.js#L239

                        // TODO - deal nicely with channel modes that take args
@qsheets
qsheets commented Feb 2, 2013

I added the basic support for differentiating between the types of modes in late October (see lib/irc.js:64) for servers that send out 005 rpl_isupport upon successfully connecting to them. However neither myself, nor Martyn, have gotten around to fix the parsing of MODE lines received from the server even though it is definitely a big item on the list. If you'd like you could write a fix and submit a pull request and I'm sure Martyn would be happy to take a look.

@awwright
awwright commented Feb 2, 2013

I'm working on that right now

@seiyria
seiyria commented Jan 20, 2014

Any word on this?

@awwright

My patch is still awaiting merge.

/me pokes

@seiyria
seiyria commented Jan 20, 2014

Yeah, I saw -- I believe this is the bug that's causing our IRC bot to crash occasionally, so I figured I'd give it a prod anyway.

@basert
basert commented May 1, 2014

any news to the merge status? My bot keeps crashing on slack IRC because of this

@Jnull
Jnull commented Sep 4, 2015

This can be closed.

@jirwin
Collaborator
jirwin commented Jan 29, 2016

Fixed in #351.

@jirwin jirwin closed this Jan 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.