PRIVMSG is malformed for DMs?

[fancypantalons]

I'm honestly amazed I didn't noticed this before. In the IRC specification, a private message to an individual takes the form:

:<originator> PRIVMSG <target> <text>

Ultimately, the code to send a PRIVMSG in response to a Slack message looks like this:

self.ircd.write(ircUser.socket, ircNick, 'PRIVMSG', [ ircTarget, ':' + line ]);

Where ircTarget is determined as follows:

let [ircNick, ircTarget] = await this.resolveSlackTarget(ircUser, event);

If we dive into the codepath, here, we ultimately end up at resolveSlackChannel, which contains this snippet:

// If it's an im, pass to resolveSlackUser
if (convo.channel.is_im) {
  return this.resolveSlackUser(ircUser, convo.channel.user);
}

And so, for a DM, the target is set to the person who sent the message. But... that's not right! The target is supposed to be the receiver of the message (i.e., ourselves). This code ends up producing the following message:

:<originator> PRIVMSG <originator> <text>

I'm pretty sure this code should read:

// If it's an im, we're the target, so return our own nick
if (convo.channel.is_im) {
  return ircUser.ircNick;
}

I think it just happens that IRC clients are willing to handle these malformed messages. But it produces all kinds of weird results, not the least being that ZNC refuses to buffer DMs coming from irslackd.

I would've just generated a PR, but I wanted to get a second set of eyes on this first. Does this look right?

[adsr]

Good catch @fancypantalons. Sounds right to me. I tested your patch[1] and it works as expected. Feel free to file a PR and I'll merge.

[1] https://gist.github.com/adsr/0dd720439046ae8bb3e4ea7efac5a489

[fancypantalons]

Just a quick update, I need to keep messing with the PR, as it's a bit more complex than I initially thought. In particular, if you use the official Slack client to send a DM, then the IRC message has to list yourself as the source and the other person as the target (versus when the message originates with the other side of the DM and those parameters are reversed). Looks like this'll require a bit more surgery in the codepath to get it to work.

[fancypantalons]

Just a quick note, I've pushed a final PR that I think is in good shape, including a few new unit tests that cover this behaviour!

[adsr]

Fixed in bde2f5a. Thank you @fancypantalons.