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
Block some specific characters in module command names #11434
Block some specific characters in module command names #11434
Conversation
Today we don't place any specific restrictions on module command names. This can cause ambiguous scenarios. For example, someone might name a command like "module|feature" which would be incorrectly parsed by the ACL system as a subcommand. In this PR, we will block some chars that we know can mess things up. Specifically ones that can appear ok at first and cause problems in some cases (we rather surface the issue right away). There are these characters: - `\r`, `\n` (newline) - can mess up the protocol on acl error replies - ` ` (space) - issues with old inline protocol - `|` - sub-commands - `@` - ACL - `=` - info commandstats - `:` - info commandstats - `,` - info commandstats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. just need to decide when we release it. arguably we can make it part of 7.2.
@redis/core-team please advise.
in theory this is a breaking change, but i suppose we can assume (hope) that there are no modules who actually use any of these characters in reality?
i guess it's safe to say about all the chars in that list, maybe except for :
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the API change. Have a question about the testing but it's not necessarily something that needs to be addressed.
already approved in the core-team meeting (mentioned in the linked issue). merging |
Today we don't place any specific restrictions on module command names. This can cause ambiguous scenarios. For example, someone might name a command like "module|feature" which would be incorrectly parsed by the ACL system as a subcommand. In this PR, we will block some chars that we know can mess things up. Specifically ones that can appear ok at first and cause problems in some cases (we rather surface the issue right away). There are these characters: * ` ` (space) - issues with old inline protocol. * `\r`, `\n` (newline) - can mess up the protocol on acl error replies. * `|` - sub-commands. * `@` - ACL categories * `=`, `,` - info and client list fields. note that we decided to leave `:` out as it's handled by `getSafeInfoString` and is more likely to already been used by existing modules.
Today we don't place any specific restrictions on module command names. This can cause ambiguous scenarios. For example, someone might name a command like "module|feature" which would be incorrectly parsed by the ACL system as a subcommand. In this PR, we will block some chars that we know can mess things up. Specifically ones that can appear ok at first and cause problems in some cases (we rather surface the issue right away). There are these characters: * ` ` (space) - issues with old inline protocol. * `\r`, `\n` (newline) - can mess up the protocol on acl error replies. * `|` - sub-commands. * `@` - ACL categories * `=`, `,` - info and client list fields. note that we decided to leave `:` out as it's handled by `getSafeInfoString` and is more likely to already been used by existing modules.
Today we don't place any specific restrictions on module command names. This can cause ambiguous scenarios. For example, someone might name a command like "module|feature" which would be incorrectly parsed by the ACL system as a subcommand. In this PR, we will block some chars that we know can mess things up. Specifically ones that can appear ok at first and cause problems in some cases (we rather surface the issue right away). There are these characters: * ` ` (space) - issues with old inline protocol. * `\r`, `\n` (newline) - can mess up the protocol on acl error replies. * `|` - sub-commands. * `@` - ACL categories * `=`, `,` - info and client list fields. note that we decided to leave `:` out as it's handled by `getSafeInfoString` and is more likely to already been used by existing modules.
Today we don't place any specific restrictions on module command names.
This can cause ambiguous scenarios. For example, someone might name a
command like "module|feature" which would be incorrectly parsed by the
ACL system as a subcommand.
In this PR, we will block some chars that we know can mess things up.
Specifically ones that can appear ok at first and cause problems in some
cases (we rather surface the issue right away).
There are these characters:
\r
,\n
(newline) - can mess up the protocol on acl error replies.|
- sub-commands.@
- ACL categories=
,,
- info and client list fields.note that we decided to leave
:
out as it's handled bygetSafeInfoString
and is more likely to already been used by existing modules.This is a breaking change, closes #11390.