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.

We should probably constrain module commands to some reasonable set of characters like [a-zA-z0-9._] or something. Also a note for the future, we should make sure that new fields have some naming constraints.

Comment From: oranagra

it's a breaking change, but i seriously doubt it'll cause any harm to anyone, i'm voting for it. @redis/core-team @MeirShpilraien WDYT?

Comment From: itamarhaber

I say aye.

Comment From: soloestoy

+1

Comment From: yossigo

Since module authors already have so many unconstrained ways to mess things up, I don't see the justification for addressing this specific issue. After all, it is a breaking change, and it's not unlikely that someone out there already uses special characters in module command names.

Comment From: oranagra

@yossigo one example for where this can bite us (or them), is this: https://github.com/redis/redis/pull/11160#discussion_r985275624

i.e. a command with a newline in it, will cause a broken protocol (breaking a client connection) when we return an ACL error trying to access it.

we can fix it in redis (replace invalid chars with something else. but maybe it's best to keep the assumption that command names can't hold crazy chars.

Comment From: MeirShpilraien

I agree with @yossigo, modules are considered trusted. Modules can mess up Redis in so many ways that I see no reason to check spacifically this. I believe documenting it is enough.

Comment From: oranagra

discussed in a core-team meeting, we concluded we just want to block 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)

specifically: * \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