Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add @+channel-context #498

Merged
merged 3 commits into from
Jun 27, 2022
Merged

Add @+channel-context #498

merged 3 commits into from
Jun 27, 2022

Conversation

delthas
Copy link
Contributor

@delthas delthas commented Apr 22, 2022

@emersion
Copy link
Contributor

Looks good to me. I was a bit worried about security considerations, but I don't think we can really do better here. Ultimately clients are free to ignore/drop the tag.

How should this interact with CHATHISTORY? Should the messages be returned as part of e.g. CHATHISTORY LATEST #chan, or CHATHISTORY LATEST bot? Since this is a client-tag, then I guess the latter, but then these messages will get omitted when scrolling up channel buffers, then will magically appear when scrolling up the bot's buffer.

@jesopo
Copy link

jesopo commented Apr 22, 2022

bitbot-irc/bitbot#344

Copy link
Contributor

@progval progval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice spec, thanks.

client-tags/channel-context.md Outdated Show resolved Hide resolved
client-tags/channel-context.md Outdated Show resolved Hide resolved
@progval
Copy link
Contributor

progval commented Apr 22, 2022

Implemented in Limnoria: progval/Limnoria@109f938

This applies to all plugins using the standard reply facility and is gated behind the same feature switch as +draft/reply.

And it applies indiscriminately to both PRIVMSG and NOTICE

@ValwareIRC
Copy link

Third-party UnrealIRCd implementation

Not restricted to notices because of privmsg replies being very common
Basic channel existence check, all the rest of checks is delegated to client

@ValwareIRC
Copy link

ValwareIRC commented May 11, 2022

Third-party UnrealIRCd implementation

Actually this is being committed to source now, instead of third party[¹][²]

@jwheare
Copy link
Member

jwheare commented May 14, 2022

This is supported in IRCCloud now.

Try it in #limnoria-bots on ircs://testnet.oragono.io:6697

image

@jwheare
Copy link
Member

jwheare commented May 14, 2022

This is probably ok to merge. Does anyone have any strong bikesheddy thoughts around the name of the tag? This is clearly based on the precedent of the WHISPER IRCX feature, but I think I prefer channel-context as a more explicit name. Something like target-channel might be even more direct, but I don't feel strongly either way. A rename can wait for ratification stage anyway and doesn't need to block merge.


This section is non-normative.

When clients receive a private PRIVMSG or NOTICE message from a user with this tag, they can display that message in the buffer specified in the tag, as if it was a channel message.
Copy link
Contributor

@kylef kylef May 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the use case of the tag, would it make sense to add a constraint for the clients to only perform this when the recipient for the PRIVMSG target is the current nick. To prevent abuse from a sender using this tag in other channels?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already says "a private PRIVMSG"

@jwheare
Copy link
Member

jwheare commented May 21, 2022

Should there be a client implementation constraint that this should only be redirected to a channel if the sender is also a member of that channel?

@progval
Copy link
Contributor

progval commented May 21, 2022

it was present in the original version, but delthas removed it, following #498 (comment)

@jwheare
Copy link
Member

jwheare commented May 21, 2022

I guess that was already discussed and changed #498 (comment)

Kinda why I don't like force pushes within PRs so changes can be tracked more obviously. (Yes I know force pushes show a compare link)

@jwheare
Copy link
Member

jwheare commented May 21, 2022

The main concern with non-shared channel redirections is that it lets spammers clutter up your channel history, which is harder to clean up than just deleting a PM. /ignore is a thing but not always retroactive in all clients.

@ValwareIRC
Copy link

Just to mention, the unrealircd implementation (in the next release) allows this behaviour only from U-Lines, so as to take into account cases like where ChanServ replies with channel-context about a channel that a botserv bot is in, but we still want to allow it to be displayed in the contexted channel =]

@progval
Copy link
Contributor

progval commented May 21, 2022

What about requiring +draft/reply with the id of a message in the channel? It's not perfect (message ids could be leaked), but it solves the spam issue

@jwheare
Copy link
Member

jwheare commented May 21, 2022

That would limit the usefulness of the feature I think. Sometimes you'd just want to whisper to someone without replying to another message.

@jwheare
Copy link
Member

jwheare commented May 21, 2022

Perhaps the risk and any possible recourse (e.g. a setting) would be worth mentioning in the non-normative section at least.

@ValwareIRC
Copy link

ValwareIRC commented May 21, 2022

could it be helpful to do something like this, but say for example you send a message to a bot/service outside of the channel about the channel, you include the channel context and msgid. and if the msgid in the reply and channel context match, then show it in the channel, if not, don't show it in the channel?
doesn't feel too complicated, maybe I am overlooking something?

edit: and give them like, 6 seconds to reply, after which time it would be considered not
(as a thought for a 'setting')

@ValwareIRC
Copy link

ValwareIRC commented May 21, 2022

allows general filtering from people without 'access' to put things in that channel unless you have specifically asked something recently about it by specifying the context, limiting it "sort of" to those who can reply fast enough to prove they have a pre-worded response (a bot with channel information)

@jwheare
Copy link
Member

jwheare commented May 22, 2022

@ValwareIRC that's kinda convoluted imo.

@ValwareIRC
Copy link

ValwareIRC commented May 22, 2022

@jwheare turns out I'm really bad at explaining things.
Check out this -slightly better- explanation I gave to @progval

[17:34:46] <~Valware> right
[17:36:34] <~Valware> if we get channel contexted message:
[17:36:34] <~Valware>   if the user is on the channel:
[17:36:34] <~Valware>     we have a normal out of the blue whisper to process normally, show in the channel;
[17:36:34] <~Valware>   else if the user is not on the channel:
[17:36:34] <~Valware>     if they are replying to a msgid where we asked for the channel to be contexted:
[17:36:34] <~Valware>       show in the channel;
[17:36:34] <~Valware>     else
[17:36:34] <~Valware>       do not show in the channel;

@jwheare
Copy link
Member

jwheare commented May 22, 2022

where we asked for the channel to be contexted

^ this is the part that's convoluted.

@ValwareIRC
Copy link

Again, maybe I didn't explain correctly =]

maybe it is simpler to say, have the client remember (for a short period) a list of users it has messaged outside of the channel with the channel as channel-context. This message could go to ChanServ for example, or it could go to a user called Bob.

Upon the reply from the user outside of the channel with the channel-context, check to see if the channel and user match what we remember, and then show it or not show it.

I say "a short time", I would say only a few seconds, which limits showing these types of messages in the channel to entities which can reply quick enough to prove they have a pre-worded response (like ChanServ, not like Bob).

I don't think that's at all convoluted...

@delthas
Copy link
Contributor Author

delthas commented Jun 4, 2022

maybe it is simpler to say, have the client remember (for a short period) a list of users it has messaged outside of the channel with the channel as channel-context. This message could go to ChanServ for example, or it could go to a user called Bob.

It is up to each client to decide how to prevent abuse of this feature. I'd like the spec to enable clients to specify which channel the message should go to, in a best-effort manner, then let clients decide if they want to add specific logic for limiting when this can be used / for displaying a specific tooltip on this kind of messages. It doesn't have to be specced.

Any other thoughts on this spec?

Copy link
Contributor

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jwheare
Copy link
Member

jwheare commented Jun 4, 2022

@delthas I agree we don't need to change any spec behaviour, and to let clients decide. But to help them decide, I still think we need to mention something more about the risk of abuse in client implementation considerations. See my earlier comment #498 (comment)

Perhaps the risk and any possible recourse (e.g. a setting) would be worth mentioning in the non-normative section at least.

@delthas delthas requested a review from jwheare June 6, 2022 19:34
emersion added a commit to emersion/soju that referenced this pull request Jun 24, 2022
@emersion
Copy link
Contributor

Ping

@delthas
Copy link
Contributor Author

delthas commented Jun 24, 2022

@jwheare Updated according to your comment 😀

@delthas delthas requested a review from jwheare June 24, 2022 15:29
@jwheare
Copy link
Member

jwheare commented Jun 24, 2022

Sounds good. OK to merge I think.

@jwheare jwheare merged commit cd9f6cb into ircv3:master Jun 27, 2022
emersion added a commit to emersion/soju that referenced this pull request Jun 28, 2022
kkartaltepe pushed a commit to kkartaltepe/gamja that referenced this pull request Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants