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

feat(message-resolver): search in cache when only given ID #438

Merged
merged 4 commits into from
Jun 29, 2022
Merged

feat(message-resolver): search in cache when only given ID #438

merged 4 commits into from
Jun 29, 2022

Conversation

noftaly
Copy link
Contributor

@noftaly noftaly commented May 19, 2022

No description provided.

favna
favna previously approved these changes May 19, 2022
Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

This is troublesome, both in security and in performance.

If we call resolveMessage with an explicit channel, we want to get the message from said channel, and fail if it's not from the same channel.

If we specify a channel and get a message from another channel, there would be a channel mismatch which can lead to some issues in many codebases, including channel permission checking (usually done before trying to get a message). In this kind of code, one could potentially create a security issue if unchecked, where you have a message ID from a channel you can't see (i.e. revealed by a bot, starboard, or something), you pass a channel you can see, and then the ID. Because this function would resolve the message from a channel that isn't one which permissions were checked, there would be a bypass, and therefore, you could accidentally leak information if you don't check for the resulting message's channel ID.

Furthermore, it doesn't make much sense to check for the resolved message's channel ID when you have specified the channel to look at. It's similar to "You have this box, find the pencils from it", logically you would look for the "pencils" (messages) from the "box" (channel) you were given, you wouldn't start looking into other "boxes" if you were told not to.

To solve this, you can pass options to resolveById instead of trying to resolve the channel there, and see if options.channel is set, you fetch the message from this channel. Otherwise you may scan other channels, although I'm unsure if this behaviour is truly desirable for everyone, and might be a breaking change.

Maybe also add an scan option to opt-in guild-wide message scanning?

@noftaly
Copy link
Contributor Author

noftaly commented May 24, 2022

If we call resolveMessage with an explicit channel, we want to get the message from said channel, and fail if it's not from the same channel.

Maybe also add an scan option to opt-in guild-wide message scanning?

Landed in 00f7cbe!
I've made that if both options.channel and options.scan are given, then scan takes the lead. However, scan is disabled by default.
Changed in 32791ce: if both options.channel and options.scan are given, then channel takes precedence over scan

I've also added this new parameter to the CoreMessage argument class, and added some documentation about those options.

@vladfrangu vladfrangu requested a review from kyranet May 28, 2022 12:39
src/lib/resolvers/message.ts Outdated Show resolved Hide resolved
@noftaly noftaly requested review from vladfrangu and favna June 6, 2022 11:24
favna
favna previously approved these changes Jun 6, 2022
@noftaly noftaly requested a review from favna June 27, 2022 23:06
@favna
Copy link
Member

favna commented Jun 28, 2022

@vladfrangu "I receive emails" fluffinator, this needs a re-review / approval

@favna favna merged commit cfb3547 into sapphiredev:main Jun 29, 2022
@noftaly noftaly deleted the feat/message-resolver-cache branch June 29, 2022 20:16
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

4 participants