Skip to content

feat: handle command injection optionally#1548

Merged
khushal87 merged 28 commits intomasterfrom
handle-command-injection
Jun 19, 2025
Merged

feat: handle command injection optionally#1548
khushal87 merged 28 commits intomasterfrom
handle-command-injection

Conversation

@khushal87
Copy link
Member

@khushal87 khushal87 commented May 28, 2025

The goal of the PR is to handle command injection and apply the command settings to the text using middleware that the SDK or apps can plug in. The existing commands middleware will be responsible for setting the command state to the textComposer, which can be used by the other middleware.

SDKs can basically add something like this to support the commands UI on message input:

 messageComposer.compositionMiddlewareExecutor.insert({
    middleware: [createCommandInjectionMiddleware(messageComposer)],
    position: { after: 'stream-io/message-composer-middleware/attachments' },
  });

  messageComposer.draftCompositionMiddlewareExecutor.insert({
    middleware: [createDraftCommandInjectionMiddleware(messageComposer)],
    position: { after: 'stream-io/message-composer-middleware/draft-attachments' },
  });

  messageComposer.textComposer.middlewareExecutor.insert({
    middleware: [createActiveCommandGuardMiddleware() as TextComposerMiddleware],
    position: { before: 'stream-io/text-composer/commands-middleware' },
  });

  messageComposer.textComposer.middlewareExecutor.insert({
    middleware: [createCommandStringExtractionMiddleware() as TextComposerMiddleware],
    position: { after: 'stream-io/text-composer/commands-middleware' },
  });

@github-actions
Copy link
Contributor

github-actions bot commented May 28, 2025

Size Change: +1.91 kB (+0.44%)

Total Size: 432 kB

Filename Size Change
dist/cjs/index.browser.cjs 125 kB +635 B (+0.51%)
dist/cjs/index.node.cjs 169 kB +671 B (+0.4%)
dist/esm/index.js 138 kB +603 B (+0.44%)

compressed-size-action

},
});

export const isTextMatched = (input: string, command: string): boolean => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a duplicate logic already present in another util function in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which utility are you talking about here?

@MartinCupela
Copy link
Contributor

I think this PR should also solve the scenario, when a user wants to cancel the command immediately after its selection - for example by pressing backspace - when there is no change event, but rather keyDown event.

@khushal87 khushal87 marked this pull request as ready for review June 17, 2025 04:47

const commands = searchSource?.query(searchQuery).items;

const matchedCommand = commands?.find((command) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

@khushal87 does it make sense to perform query(searchQuery) and then again commands.find()? Isn't it doing the same with the difference that commands.find in this case would just be commands[0] (as find returns on the first matched and the first matched is the at 0 index before query returns all the matches)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will simplify it. Makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


const query = triggerWithToken.slice(1);

const searchQuery = getFirstWordFromText(query);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it necessary to get first word from query if the query itself is just a trigger followed by chars that are not white space?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now, if you allow spaces, the user can paste anything let's say /giphy good morning, in that case,e the query will be giphy good morning but the search source cannot return anything as the pattern didn't match. That is why we get the first word from the query just for the command search.

new RegExp(
isCommand
? `^[${trigger}]${triggerNorWhitespace}$`
? `^[${trigger}]${notTrigger}$`
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we allow white space?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is the function returns null for any text except the pattern /command which, we cannot rely on if we want to use it as an input for search later in the middleware. That is why I allowed it.

* @param textToBeMatchedWith
* @returns
*/
export const startsWithTextAndSpace = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may not need this function. It is used in a single place and it actually removes white space that was kept previously due to allowing white space in getTriggerWithToken

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this function just checks if the query starts with the command name or not, and we are obviously allowing at least one or more spaces so that only then it would return true for a match, i.e. /unban will be a matched command and /unban would not for the onChange.

* @param text - The input text from which to extract the first word.
* @returns The first word found in the text, or an empty string if no word is found.
*/
export const getFirstWordFromText = (text: string): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

* @param trigger - The trigger string to be removed from the start of the text.
* @returns The text with the trigger removed from the start.
*/
export const stripTriggerFromText = (text: string, trigger: string) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not strip trigger. In the single place where it is used it strips a trigger with command name. Trigger is for example "/" but not "/ban".
The function itself could also be renamed to stripTextFromText

Copy link
Member Author

@khushal87 khushal87 Jun 19, 2025

Choose a reason for hiding this comment

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

How about stripTextFromStartOfTheText as it strips the text from the start of the text?

};

setCommand = (command: CommandResponse | null) => {
if (command === this.command) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This may not work if the objects have not the same memory reference, but their content refer to the same command. Maybe something like command.name === this.command.name would be more appropriate or to have a function that evaluates that two command objects are equal.

Copy link
Member Author

@khushal87 khushal87 Jun 19, 2025

Choose a reason for hiding this comment

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

Fixed using the 1st way

@khushal87 khushal87 merged commit ea912f9 into master Jun 19, 2025
6 checks passed
@khushal87 khushal87 deleted the handle-command-injection branch June 19, 2025 15:14
github-actions bot pushed a commit that referenced this pull request Jun 19, 2025
## [9.8.0](v9.7.0...v9.8.0) (2025-06-19)

### Bug Fixes

* allow to nullify reminder's remind_at value ([#1569](#1569)) ([c1c6cf4](c1c6cf4))
* propagate stopTimerRefreshBoundaryMs to all reminder timers on ReminderManager config update ([#1568](#1568)) ([944d1c2](944d1c2))
* run onChange middleware on TextComposer.insertText ([#1570](#1570)) ([3768007](3768007))

### Features

* handle command injection optionally ([#1548](#1548)) ([ea912f9](ea912f9))

### Refactors

* protect ReminderPaginator filters and sort properties ([#1567](#1567)) ([1a3f4c4](1a3f4c4))
@stream-ci-bot
Copy link

🎉 This PR is included in version 9.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants