Last Comment Bug 787433 - IRC component should return a proper CTCP VERSION response.
: IRC component should return a proper CTCP VERSION response.
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: 15 Branch
: All All
: -- minor (vote)
: Thunderbird 18.0
Assigned To: Patrick Cloke [:clokep]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-31 08:21 PDT by Sergio C. G.
Modified: 2012-09-14 07:39 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Instantbird patch (2.93 KB, patch)
2012-09-04 19:36 PDT, Patrick Cloke [:clokep]
aleth: review+
Details | Diff | Splinter Review
Patch v2 (2.72 KB, patch)
2012-09-06 18:40 PDT, Patrick Cloke [:clokep]
no flags Details | Diff | Splinter Review
Simple patch (2.46 KB, patch)
2012-09-07 13:24 PDT, Patrick Cloke [:clokep]
florian: review+
Details | Diff | Splinter Review

Description Sergio C. G. 2012-08-31 08:21:37 PDT
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120824154833

Steps to reproduce:

Connected to an IRC server with Thunderbrid, and check the Thunderbird's response to a CTCP VERSION request.


Actual results:

Only returned the string "Thunderbird".


Expected results:

According to the IRC's RFC (http://www.irchelp.org/irchelp/rfc/) the client should return the client name and the version.

Sometimes bugs appear in the clients and Network Administrators must get rid of that buggy clients in order to ensure security of the Network users. One of the best "weapons" to detect the buggy versions is CTCP VERSION. If Thunderbird returns only the name of the client a bug on the client can result into a ban to all versions of the client.

I think that should be convenient to add the version number. At least that's my suggestion as a Network Administrator and as a future user of Thunderbird client that I found useful to have it at work.
Comment 1 Patrick Cloke [:clokep] 2012-08-31 09:56:40 PDT
(In reply to Sergio C. G. from comment #0)
> According to the IRC's RFC (http://www.irchelp.org/irchelp/rfc/) the client
> should return the client name and the version.
Yes, it does say that and I usually try very hard to abide by specifications. This was a design decision: I originally had it that way and changed it during a code review to prevent security/privacy leaks about what particular client is being used (with the thought being that if you know the exact client of another user you could more easily attack it).

I'm not sure I'm really qualified to decide whether this is an issue or not. To me it's essentially the same as the User-Agent, which does contain the version...

> Sometimes bugs appear in the clients and Network Administrators must get rid
> of that buggy clients in order to ensure security of the Network users. One
> of the best "weapons" to detect the buggy versions is CTCP VERSION. If
> Thunderbird returns only the name of the client a bug on the client can
> result into a ban to all versions of the client.
I understand the concept behind actually having the version number, yes. :) Does this actually happen in practice? Do most other clients show version numbers (Pidgin/Adium/Finch I believe just report "purple").

> I think that should be convenient to add the version number. At least that's
> my suggestion as a Network Administrator and as a future user of Thunderbird
> client that I found useful to have it at work.
Thanks, it's great to hear back from people running networks.

For fun, would this report the Gecko version or the application version?
Comment 2 Sergio C. G. 2012-08-31 10:13:06 PDT
(In reply to Patrick Cloke [:clokep] from comment #1)
> Yes, it does say that and I usually try very hard to abide by
> specifications. This was a design decision: I originally had it that way and
> changed it during a code review to prevent security/privacy leaks about what
> particular client is being used (with the thought being that if you know the
> exact client of another user you could more easily attack it).
>
> I'm not sure I'm really qualified to decide whether this is an issue or not.
> To me it's essentially the same as the User-Agent, which does contain the
> version...

For me is also the same as User-Agent, if the decision is to hide the version number, why not from the emails? Can appear an exploit for the email component and could be also exploited.

I've seen many IRC exploits and usually don't care about the version just try to trigger it.

> Does this actually happen in practice? Do most other clients show version
> numbers (Pidgin/Adium/Finch I believe just report "purple").

Most of the major clients and major engines show its version number but I had to download Adium and check it because I was not sure if libpurple does it  (is not a usual client for IRC) and I found that don't shows version number. I'll check if Pidgin does it on Monday... 

> Thanks, it's great to hear back from people running networks.

First I found it funny but then I thought that is a good feature.

> For fun, would this report the Gecko version or the application version?

Hmmm I think that it must report the version number that will change if there is a change in the IRC core and then a release. I think that application version should be enough.
Comment 3 Patrick Cloke [:clokep] 2012-08-31 11:33:38 PDT
(In reply to Sergio C. G. from comment #2)
> For me is also the same as User-Agent, if the decision is to hide the
> version number, why not from the emails? Can appear an exploit for the email
> component and could be also exploited.
I should probably say that this decision was made by Instantbird developers, not Mozilla developers. ;) So it should have no bearing on what is done for email.

> I've seen many IRC exploits and usually don't care about the version just
> try to trigger it.
> 
> > Does this actually happen in practice? Do most other clients show version
> > numbers (Pidgin/Adium/Finch I believe just report "purple").
> Most of the major clients and major engines show its version number but I
> had to download Adium and check it because I was not sure if libpurple does
> it  (is not a usual client for IRC) and I found that don't shows version
> number. I'll check if Pidgin does it on Monday... 
I know for a fact that libpurple does not: http://lxr.instantbird.org/pidgin2.6.3/source/libpurple/protocols/irc/parse.c#563

> > For fun, would this report the Gecko version or the application version?
> Hmmm I think that it must report the version number that will change if
> there is a change in the IRC core and then a release. I think that
> application version should be enough.
Probably makes sense since Instantbird and Thunderbird have differing versions of Gecko, but potentially the same version of chat/.

I'm debating this with some of the other developers, we'll see what happens...
Comment 4 Patrick Cloke [:clokep] 2012-09-04 19:36:35 PDT
Created attachment 658340 [details] [diff] [review]
Instantbird patch

This adds a version of "Instantbird 1.3a1pre" to Instantbird, it's also customizable per account (since that's something Mook had asked me for a long time ago and I felt like being a nice guy). I don't actually have a comm-central tree and __APP_VERSION__ doesn't seem to be used anywhere in c-c...does this exist in the build system?

If nothing else, we fall back to brandShortName (which is what is currently used), since you can't have an empty VERSION response.
Comment 5 Mark Banner (:standard8, afk until Dec) 2012-09-04 23:02:09 PDT
Comment on attachment 658340 [details] [diff] [review]
Instantbird patch

Why not just use nsIXULAppInfo.name and .version? Even simpler you may be able to use the Application object available to js.
Comment 6 Florian Quèze [:florian] [:flo] 2012-09-05 01:59:18 PDT
(In reply to Patrick Cloke [:clokep] from comment #4)
> I don't actually have a
> comm-central tree and __APP_VERSION__ doesn't seem to be used anywhere in
> c-c...does this exist in the build system?

For Instantbird, it relies on:
DEFINES += -DAPP_VERSION="$(MOZ_APP_VERSION)"
in instantbird/app/Makefile.in

Thunderbird has the same define at http://mxr.mozilla.org/comm-central/source/mail/app/Makefile.in#40
Comment 7 Florian Quèze [:florian] [:flo] 2012-09-05 02:02:01 PDT
(In reply to Mark Banner (:standard8) from comment #5)

> Why not just use nsIXULAppInfo.name and .version? Even simpler you may be
> able to use the Application object available to js.

The preprocessed default preference value is a nice solution because it lets the user customize the value but is updated automatically if the value is the same as the default one.

I don't think the Application object is available in Instantbird.
Comment 8 Patrick Cloke [:clokep] 2012-09-05 03:53:42 PDT
(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> (In reply to Mark Banner (:standard8) from comment #5)
> 
> > Why not just use nsIXULAppInfo.name and .version? Even simpler you may be
> > able to use the Application object available to js.
> 
> The preprocessed default preference value is a nice solution because it lets
> the user customize the value but is updated automatically if the value is
> the same as the default one.
This was the reasoning I chose to do it this way. I wouldn't be against using nsIXULAppInfo, however, and just checking to see if that pref exists (or maybe check only the account pref).
Comment 9 aleth [:aleth] 2012-09-05 11:37:30 PDT
Comment on attachment 658340 [details] [diff] [review]
Instantbird patch

Looks good to me - though I do wonder what the use case for per-account version string overrides was ;)
Comment 10 Patrick Cloke [:clokep] 2012-09-06 18:40:45 PDT
Created attachment 659072 [details] [diff] [review]
Patch v2

Patch based on Standard8's suggestion.

This checks the version option of the account, if that's not found (or is empty) it will default to "Instantbird <version>" or "Thunderbird <version>". I also changed the username to use Services.appinfo.name, so these two sets of code are done in a similar way.
Comment 11 Florian Quèze [:florian] [:flo] 2012-09-07 02:24:24 PDT
What's the use case for customizing this per-account? (afaik the user who you think requested it doesn't remember about why he would want that).
Isn't it slightly more useful to customize it for all accounts at once? + A global pref with a default value is discoverable for people playing with about:config.
The "version" account-specific pref isn't discoverable at all (except for people reading the code).
Comment 12 Patrick Cloke [:clokep] 2012-09-07 03:52:22 PDT
(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> What's the use case for customizing this per-account? (afaik the user who
> you think requested it doesn't remember about why he would want that).
> Isn't it slightly more useful to customize it for all accounts at once? + A
> global pref with a default value is discoverable for people playing with
> about:config.
> The "version" account-specific pref isn't discoverable at all (except for
> people reading the code).
I did it this way mostly to match the username code (which also isn't discoverable, by the way). It allows more fine grained control (per account), without being too complicated of checking both a global pref + a account pref.

Perhaps it is debatable whether it should even be customizable.
Comment 13 Florian Quèze [:florian] [:flo] 2012-09-07 05:34:41 PDT
I think the only thing I could want to customize is "not send the version number", so I think just a boolean pref to send the client name only (ie the previous behavior) as opposed to the full client name+version would do.
Comment 14 Patrick Cloke [:clokep] 2012-09-07 05:47:35 PDT
Comment on attachment 659072 [details] [diff] [review]
Patch v2

I'll code that up this afternoon.
Comment 15 Patrick Cloke [:clokep] 2012-09-07 13:24:50 PDT
Created attachment 659339 [details] [diff] [review]
Simple patch

Simpler patch that doesn't give the user any options.
Comment 16 Florian Quèze [:florian] [:flo] 2012-09-13 04:34:03 PDT
Checked-in as http://hg.instantbird.org/instantbird/rev/46d0d1e824eb for Instantbird.

Adding checkin-needed for comm-central.
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-09-13 14:08:54 PDT
https://hg.mozilla.org/comm-central/rev/4316b336946c
Comment 18 Florian Quèze [:florian] [:flo] 2012-09-14 07:13:36 PDT
Using nsIXULAppInfo.name has the (unintended?) consequence of displaying "Thunderbird" for Daily and Earlybird builds too. Should we revert that change or is it ok?
Comment 19 Patrick Cloke [:clokep] 2012-09-14 07:28:08 PDT
(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> Using nsIXULAppInfo.name has the (unintended?) consequence of displaying
> "Thunderbird" for Daily and Earlybird builds too. Should we revert that
> change or is it ok?

I think that's fine as it will have the version number with it? Although is it reported as "Thunderbird 18" or is it "Thunderbird 18a1pre" or something like that? If it's the former, I'd think we'd want it to say something else...
Comment 20 Florian Quèze [:florian] [:flo] 2012-09-14 07:39:40 PDT
I was actually talking about the username from irc.js, so no version number.

But for the CTCP version, I'm not sure it's ok to show "Thunderbird 18.0a1pre". Isn't the Thunderbird trademark reserved for releases and betas?

Note You need to log in before you can comment on or make changes to this bug.