View Issue Details

IDProjectCategoryView StatusLast Update
0001317unrealircdpublic2019-01-27 14:47
ReporterthiloAssigned Tosyzop 
PrioritynormalSeverityfeatureReproducibilityalways
Status resolvedResolutionfixed 
Product Version 
Target VersionFixed in Version3.2.7-RC2 
Summary0001317: Inclusion of topic_setter in topic message when set by services really necessary?
DescriptionWhenever services enforce an old topic, be it the locking of the topic, be it a services restart, the topic_setter is included in braces at the end of the topic message, although the daemon stores the topic without the nick internally. This does cause a client/server desync.
Furthermore, if TOPIC_NICK_IS_NUHOST was enabled in the config.h, the host of a user is also included in the topic message .. it looks particularly nasty on long hosts.
I have coded a small "fix" in channel.c in the function m_topic that resends the "topic has been set by ..." numeric to all users on the channel once a topic has been reset.

FYI: The patch below patches against the channel.c in the package 3.2beta-18
Additional Information--- channel.c 2003-10-12 01:17:06.000000000 +0200
+++ channel.c.orig 2003-10-12 01:21:18.000000000 +0200
@@ -4149,9 +4149,6 @@
                         * some services do this in their topic enforcement -- codemastr
                         */
                        {
- /* we need this to search through the chained list and send the topic setter message to each client. */
- Member *lp;
-
                                /* setting a topic */
                                topiClen = strlen(topic);
                                nicKlen = strlen(tnick);
@@ -4182,16 +4179,9 @@
                                    chptr->chname, chptr->topic_nick,
                                    chptr->topic_time, chptr->topic);
                                sendto_channel_butserv(chptr, sptr,
- ":%s TOPIC %s :%s", parv[0],
- chptr->chname, chptr->topic);
-
- for(lp = chptr->members; lp; lp = lp->next)
- {
- sendto_one(lp->cptr,
- rpl_str(RPL_TOPICWHOTIME), me.name,
- lp->cptr->name, chptr->chname,
- chptr->topic_nick, chptr->topic_time);
- }
+ ":%s TOPIC %s :%s (%s)", parv[0],
+ chptr->chname, chptr->topic,
+ chptr->topic_nick);
                        }
                }
                else if (((chptr->mode.mode & MODE_TOPICLIMIT) == 0 ||
TagsNo tags attached.
3rd party modules

Relationships

related to 0003385 closedsyzop ./Config (-advanced) option to allow (nickname) to be appended to topics when set by U:line 

Activities

thilo

2003-10-24 14:01

reporter   ~0003851

it gets embarassing if i do the diff the wrong way round .. correct patch:

--- channel.c.orig 2003-10-12 01:21:18.000000000 +0200
+++ channel.c 2003-10-12 01:17:06.000000000 +0200
@@ -4149,6 +4149,9 @@
                         * some services do this in their topic enforcement -- codemastr
                         */
                        {
+ /* we need this to search through the chained list and send the topic setter message to each client. */
+ Member *lp;
+
                                /* setting a topic */
                                topiClen = strlen(topic);
                                nicKlen = strlen(tnick);
@@ -4179,9 +4182,16 @@
                                    chptr->chname, chptr->topic_nick,
                                    chptr->topic_time, chptr->topic);
                                sendto_channel_butserv(chptr, sptr,
- ":%s TOPIC %s :%s (%s)", parv[0],
- chptr->chname, chptr->topic,
- chptr->topic_nick);
+ ":%s TOPIC %s :%s", parv[0],
+ chptr->chname, chptr->topic);
+
+ for(lp = chptr->members; lp; lp = lp->next)
+ {
+ sendto_one(lp->cptr,
+ rpl_str(RPL_TOPICWHOTIME), me.name,
+ lp->cptr->name, chptr->chname,
+ chptr->topic_nick, chptr->topic_time);
+ }
                        }
                }
                else if (((chptr->mode.mode & MODE_TOPICLIMIT) == 0 ||

Praetorian_

2003-10-24 14:09

reporter   ~0003852

I agree that the nick shouldn't be set in brackets after the end
eg.

*** service changes topic to 'weee (praetorian)'

but i also disagree that the setby line should be sent when it does happen. meaning

*** Set by praetorian at 11:11:11 1/1/1111

In my view i would think just set the topic. if anyone cares who set it, they can /topic #chan and get the setby line manually..

my 2 cents ;)

thilo

2003-10-24 14:24

reporter   ~0003856

Praetorian: You still have a server/client desync, and that is to be avoided in my humble opinion, on the other hand, the topic_setter version seems to work quite well here, and it does only happen when the services enforce a topic lock, which is probably not too often.

Regarding the patch: I have also prepared a link where to get the patch, as I noticed now, that the web version here does not preserve spaces and tabs.

You can download the patch from
http://home.bawue.de/~arny/unreal_topicsetter.patch

syzop

2003-10-24 15:12

administrator   ~0003859

Well, suddenly sending something at an unexpected stage is usually considered wrong and might lead to problems (that's a general thing, not specific for this although this is unlikely to be accepted)...
I agree that this current topic thing isn't that nice and that it causes this "topic desynch", so I can understand that you are searching for a solution to it. I personally don't have any problem with it because I know how it works but I can understand that inexperienced (or even pretty experienced) users will be confused by this.
Let's see what other coders have to say about the general issue :p.

thilo

2003-10-24 17:34

reporter   ~0003860

Yes. maybe it is not very nice to send a numeric reply, when no query was sent. (although, when joining a channel, you get a bunch of numerics for which there were no specific questions)
If it is not feasable, you should still consider completely removing the topic_setter in the topic message completely.

aquanight

2004-06-04 02:06

reporter   ~0006573

How about instead just put the setter in the 'source' prefix when you send the topic?
No numeric, no nick in brackets... but the client gets the right info. They don't need to know who really set the topic (ChanServ/BotServ-bot), do they?

Instead of:

* ChanServ sets topic to 'Blahblahblah (aquanight)'

It will show as:
* aquanight sets topic to 'Blahblahblah'

Clients should handle it fine whether 'aquanight' is actually in the channel or opped with +t or not.

Rocko

2004-06-04 07:41

reporter   ~0006575

Bad suggestion aquanight ...

codemastr

2004-06-04 13:15

reporter   ~0006582

How do you know clients will handle that fine? I think it would screw them up rather badly! First off, it could screw Unreal up. Unreal has an option to show nick!user@host. Well if the user isn't online, where does the user@host part come from? Services doesn't store that, and neither does Unreal. Second, clients could very well become confused from seeing a topic change from someone who is not in the channel. I don't understand how you could assume they wouldn't. It's illegal, so how can we expect clients to accept and understand something that is illegal?

aquanight

2004-06-04 13:31

reporter   ~0006583

Wouldn't the user@host come from the TOPIC message that is coming from the remote server? And yes, it most likely would be absent for services or offline users, in which case, there's no real need to pass it on to clients. (No sense trying to tell them something you don't know ;) .) And if it is a problem, then you could just pull it from the whowas data (since you already store the user@host there, one of them has to be correct ;) ). Also, how many clients have you seen that actually display the user@host in the "blablah sets topic to" message (though mIRC, for example, could be scripted to do so)?

An oper using operoverride to op himself or banwalk is "illegal" and so is ChanServ opping people when it's not even in the channel, but have you seen a client have problems with that? And then there's +u... Have you ever been in a +u-t channel where one of the normals set the topic? Did the client have problems then? Personally, I don't think it's right for a client to be "validating" what's coming from the server. If the server says it's right, then by golly, it's right. If the client has problems with that, then it should just /quit... because clients have no business telling the server something isn't right ;) .

Also, as it is now, a topic enforced by services still comes from an "invalid" user (ChanServ). This kind of topic change isn't illegal?

syzop

2004-06-04 14:01

administrator   ~0006588

Well I don't like this 'spoofing' either, it's not a good idea and very confusing.

Anyway, back to the point... why do we do include the topic setter? If you do '/topic #chan' you can see who really set the last topic (since that info is updated correctly). So we are only talking about letting the user know who set it without requiring any effort (by appending the name after the topic etc)...
Either way ("don't show who set it when updating the topic" or "add the nick in brackets after the topic") you get a little 'desynch', in the 2nd case even a topic-text-desynch which is IMO a lot more annoying.
I think if someone really wonders who set the topic they could just do /topic #chan again, in most cases people won't care.

Just my thoughts... [hm basically that's just what Praetorian_ said ;p]

Rocko

2004-06-04 14:38

reporter   ~0006591

Last edited: 2004-06-04 14:46

Yes, the reason I think, why this was reported is:

*** ChanServ changes topic to 'blah (Rocko)'
and I want to change the topic, mIRC shows the topic 'blah (Rocko)' in the editbox, instead of 'blah', so when I forget to make a /topic #channel prior to editing the topic, the (Rocko) will be added into the topic.

When I make a /topic #channel first, the (Rocko) isn't there anymore. Thats the originally problem.

And ChanServ sets a topic only in the case of a topic_lock (enforce), or if a new channel is created (topic Retention).
So, thats right, when someone really cares about, who set the topic, he should make a /topic #channel.

bearbeitet am: 2004-06-04 14:46

thilo

2004-06-04 15:04

reporter   ~0006593

"How do you know clients will handle that fine? I think it would screw them up rather badly!"

aquanight raised a valid point, that then something like ChanServ topic enforcement would screw up the client on those networks, that do not join ChanServ when a channel has been registered.
Also, at the moment IRCops can change topics even when they are not on the channel. Anyways, I don't like his way of topic change either, the topic setter time is not being preserved :P

"Unreal has an option to show nick!user@host. Well if the user isn't online, where does the user@host part come from? Services doesn't store that, and neither does Unreal."

The whole part is stored in the topic_setter space. This is independent from whether the user is online or not ... in fact I have built in support for the TOPIC_NICK_IS_NUHOST option in my services. Like I said, it looks really nasty when the whole host is also being shown in the topic.

"Either way ("don't show who set it when updating the topic" or "add the nick in brackets after the topic") you get a little 'desynch'"

My solution suggested above has worked on my server without problems. Whenever a u:lined server enforces a topic with a foreign topic_setter, the server can send out the topic_setter numeric. This way, you ensure that there is no desynch. I don't know whether this violates any rules, even if the client hasn't explicitly requested for a topic. But then again, I can't find the topic_setter numeric in the RFCs anyways, and the numeric is also being sent to the client automatically when it joins a channel. Thinking about client architectures, I would actually be too lazy to build something in that would check whether really some query was sent that would justify receiving a numeric ... actually, I can't see why anyone would want to check this. My patch above basically works, you still need to check though, whether the cptr is a client on the current server before sending the numeric to that client, then it's perfect.

syzop

2004-06-04 15:16

administrator   ~0006596

'My solution suggested above has worked on my server without problems.'
Yes, but that solution was rejected already. Just because you tested it and it seems to works fine does not mean it's good, there a lot of such patches that are just horrible ;P.

thilo

2004-06-04 16:19

reporter   ~0006599

Last edited: 2004-06-04 16:37

as far as i can see this, noone rejected anything yet, not even you.

My point is: per definition in RFC the client also does not expect a topic_setter message when it joins the channel, because there numeric 333 simply does not exist.
If you were strict, you would have to at least limit the topic_setter numeric reply to when someone issues a topic command without a parameter, not on Join.

edited on: 2004-06-04 16:37

syzop

2004-06-04 17:07

administrator   ~0006600

'as far as i can see this, noone rejected anything yet, not even you.'
8 months ago already, although no.. not in strong terms ['unlikely to be accepted'].. sometimes I'm nice :P.
The keyword is 'unexpected' here.. no client expects to suddenly receive a topicsetby message after the user is idling for 2 hours in a channel. It's ugly, bad practice / not good, and might even break things.
It's a bit like the guys shouting a NAMES list should be sent when you do -u or +u... God.. ;p

thilo

2004-06-04 17:26

reporter   ~0006601

Yeah, whatever, pls pls just get rid of the topic setter in the topic text ;-D

Rocko

2004-06-04 17:59

reporter   ~0006602

Mh, I found out, bahamut doesn't send that "topic_setter" too. (My nice examples with bahamut ;P)

There is only this: *** ChanServ changes topic to 'test'
Bahamut has this (little) "desynch", that clients could think, ChanServ sets the topic too, but mIRC doesn't show this anyway.

After a /topic #channel, everythings ok, and you see the topic_setter.

aquanight

2004-06-05 22:23

reporter   ~0006603

Well, personally, I could care less who it's coming from, only what it says (and if I do need to change it, I usually will notice the nick at the end, if it doesn't get removed ;) ).

stskeeps

2007-04-27 06:10

reporter   ~0013859

Bump. Still a issue?

Rocko

2007-04-27 10:45

reporter   ~0013881

Yes it is. Actually I solve this "problem" by editing the source manually, because you aren't changing it in the main source code.

It's only a change in src/modules/m_topic.c.

Change
:%s TOPIC %s :%s (%s)", parv[0],
to
:%s TOPIC %s :%s", parv[0],

For people who don't want this topic setter. Because it is really anoying, if chanserv enforces a topic, and you see "Muuh..Topic.. (chanserv). Than I want to change the topic from my script and it gives me "Muuh..Topic.. (chanserv)" as the actual topic, which it isn't, bahamut doesn't set it too (topic setter), so why should unreal?

In advance, I also modified the IRCd, so that when I delete the topic, it's really deleted, and not like "'". We/I also mentioned that, that this should be changed too, but I think there was an other bug report !?

syzop

2007-04-27 10:56

administrator   ~0013882

just to resay my comments: I don't like suddenly sending a numeric (this is unlikely to happen, sts will agree with me on that), but I do dislike the 'xx (setby)' thingy, so I agree it should be changed (dropped).. or otherwise be configurable at least.

vonitsanet

2007-04-27 11:31

reporter   ~0013883

I dont like it "Topic (mynickhere)".
I like the idea of removal.

Bock

2007-04-27 11:33

reporter   ~0013884

+1 to vonitsanet

WolfSage

2007-06-09 18:38

reporter   ~0014292

Removed in .2426

syzop

2011-07-19 17:54

administrator   ~0016712

re-port this patch, don't go send numerics ;)

dboyz

2015-10-02 06:33

reporter   ~0018730

Doing some archeology here. This seems to be fixed already. I am unable to reproduce this on irc.unrealircd.org (runs on Unreal3.2.10.2 as I speak).

syzop

2019-01-27 14:47

administrator   ~0020464

DBoyz is correct, this was fixed in f3a2d7f6a45c007a68397b620163febbf0257366 in UnrealIRCd 3.2.7, about 11,5 years ago.

commit f3a2d7f6a45c007a68397b620163febbf0257366
Author: stskeeps <devnull@localhost>
Date: Fri Jun 15 19:47:26 2007 +0000

    - 0001317 reported by thilo regarding removal of (username) being
      appended to topics set by U:Lined servers.

Issue History

Date Modified Username Field Change
2003-10-24 13:59 thilo New Issue
2003-10-24 14:01 thilo Note Added: 0003851
2003-10-24 14:09 Praetorian_ Note Added: 0003852
2003-10-24 14:24 thilo Note Added: 0003856
2003-10-24 15:12 syzop Note Added: 0003859
2003-10-24 17:34 thilo Note Added: 0003860
2004-06-04 02:06 aquanight Note Added: 0006573
2004-06-04 07:41 Rocko Note Added: 0006575
2004-06-04 13:15 codemastr Note Added: 0006582
2004-06-04 13:31 aquanight Note Added: 0006583
2004-06-04 14:01 syzop Note Added: 0006588
2004-06-04 14:38 Rocko Note Added: 0006591
2004-06-04 14:42 Rocko Note Edited: 0006591
2004-06-04 14:46 Rocko Note Edited: 0006591
2004-06-04 15:04 thilo Note Added: 0006593
2004-06-04 15:16 syzop Note Added: 0006596
2004-06-04 16:19 thilo Note Added: 0006599
2004-06-04 16:36 thilo Note Edited: 0006599
2004-06-04 16:37 thilo Note Edited: 0006599
2004-06-04 17:07 syzop Note Added: 0006600
2004-06-04 17:26 thilo Note Added: 0006601
2004-06-04 17:59 Rocko Note Added: 0006602
2004-06-05 22:23 aquanight Note Added: 0006603
2007-04-27 06:10 stskeeps Note Added: 0013859
2007-04-27 06:10 stskeeps Status new => feedback
2007-04-27 10:45 Rocko Note Added: 0013881
2007-04-27 10:56 syzop Note Added: 0013882
2007-04-27 11:31 vonitsanet Note Added: 0013883
2007-04-27 11:33 Bock Note Added: 0013884
2007-06-09 18:38 WolfSage Note Added: 0014292
2007-06-09 18:38 WolfSage Assigned To => WolfSage
2007-06-09 18:38 WolfSage Status feedback => resolved
2007-06-09 18:38 WolfSage Resolution open => fixed
2007-06-09 18:38 WolfSage Fixed in Version => 3.3-alpha0
2007-06-09 18:38 WolfSage Description Updated
2007-06-09 18:38 WolfSage Additional Information Updated
2007-06-09 20:47 WolfSage Relationship added related to 0003385
2011-07-19 17:54 syzop Note Added: 0016712
2011-07-19 17:54 syzop Assigned To WolfSage =>
2011-07-19 17:54 syzop Status resolved => needs re porting
2015-08-08 17:54 syzop Severity tweak => feature
2015-10-02 06:33 dboyz Note Added: 0018730
2019-01-27 14:47 syzop Assigned To => syzop
2019-01-27 14:47 syzop Status needs re porting => resolved
2019-01-27 14:47 syzop Fixed in Version 3.3-alpha0 => 3.2.7-RC2
2019-01-27 14:47 syzop Note Added: 0020464