Comma in strong password incompatible with connection string format

[tillig]

We have an auto-provisioned Redis instance in a cloud environment. The password we're provided is generated for us, is very long, and has a lot of special characters... including, sometimes, commas.

We then have systems that try to attach to Redis using a connection string. Unfortunately, the way SE.Redis parses connection strings based on comma delimiters, it means if our generated password has a comma in it, then part of the password gets interpreted as another host. Exception messages have stuff like this buried in them: SocketFailure on *9?9pi?aK__6fAKSH:6379/Subscription (where *9?9pi?aK__6fAKSH is the half of the password after the comma).

Looking through the parsing code it doesn't appear there's a way to escape commas. That might be a good addition to help in edge cases like this.

[mgravell]

Agreed - this needs some kind of escape syntax. Totally valid point.

[NickCraver]

I was thinking about how to make this work with existing connection strings. What if in the parser, checked if the contents after a comma following password= were a valid config option name? If not, we go to the next comma. Just a thought, maybe we don't need escaping? This could also be a terrible idea...

[tillig]

I've always had kinda bad luck special-casing things like this. Sometimes it works great; most times [for me] I get caught with a situation where it works out now but then there's one more thing it turns out needs special casing and back-compat means I'm stuck.

Curious if you could have something in the parser that allows for, like, metadata in the connection string. Maybe a value indicating the parse version first, like

.v1,redis0:6380,redis1:6380,allowAdmin=true

If the first arg starts with .v you can pick a parser version and opt in to an escape syntax. If the first arg doesn't start with .v or isn't parseable into \.v\d+ (regex style) then it defaults to the current non-escaped parsing for back-compat.

Another idea might be to allow the parser to support quoted CSV style things, so this would work...

redis0:6380,redis1:6380,allowAdmin="true"

and your password could be in quotes...

redis0:6380,redis1:6380,password="abc,def"

and if you need a quote in your password, the stuff inside quotes could support escaping

redis0:6380,redis1:6380,password="abc,def\"ghi"

If the value after = doesn't both start and end with a quote, consider the quotes as literals. Something like that would also allow for pretty easy back-compat and opting in to escape syntax. You might find some really crazy edge cases where someone actually has a password that really does start and end in quotes... that would break for them. I'm guessing that's pretty rare.

Of course, maybe passwords with commas are also rare.

[mgravell]

what concerns me is that most scenarios change how existing values could be interpreted; for example - password="abcdef works today and the password is the literal "abcdef

Idea (@NickCraver thoughts?):

what about if quoted values are allowed, but only if the key is also quoted? today, quoted keys aren't a thing, so there is no ambiguity; for example

"password"="abc,def"

would assign the password abc,def; if we implemented this

• we would continue to allow unquoted key/value pairs
• quoted and unquoted are allowed in the same string
• the generated output (object=>string) should only use quoted keys if the value needs quoting (i.e. includes a comma)
• so the output remains the same unless you have quoted values

questions:

• inside a quoted string - should a quote be \" ? or "" ?
• should we also support ' as equivalent? or strictly "?

[NickCraver]

I'd say it's not very intuitive (so my fear is someone hitting the issue ever finding the fix), but: I have no better ideas! Any thoughts on how we could assist a user in finding this? Perhaps as part of a parse error message?

[tillig]

Dockerfile uses a directive to indicate the escape character. Perhaps something like that?

esc=\,password=abc\,def

No directive means parse as it works today.

[mgravell]

@tillig I think I actually prefer that to my other suggestion, although I think if we went that route, we'd define that it must be specified at most once, and as the first option, and as a single character

@NickCraver in terms of discoverability: we can generate ToString() with this from the ConfigurationOptions type; the problem with parsing errors is the ambiguity - it is theoretically possible that they genuinely do mean server1,password=abc,server2,option=value to mean 2 servers. I don't think we should reject it necessarily; we could perhaps flag it as "suspect", and append extra data if we get connection errors?