[Freeipa-devel] [PATCH] 0015 Only split CSV strings once

[Jan Cholasta]

On 23.2.2012 15:29, Petr Viktorin wrote:
> https://fedorahosted.org/freeipa/ticket/2227 (Unable to add certain sudo
> commands to groups). What an interesting bug to get :)
>
>
> One problem with our CSV splitting is that it's not idempotent
> (baskslashes are "eaten" when there are escaped commas), but when we
> forward a call it gets done on both the client and the server. The
> attached patch fixes that in a somewhat hacky way. It's not a complete
> fix for the issue though, for that I need to ask what to do.

Can't you just re-escape the values before forwarding the call? That 
would be a fairly straightforward fix and it would remove the need for 
all the _forwarded_call hackery.

>
>
> Some Params use the CSV format when we just need a list of
> comma-separated values. Our flavor of CSV does escaping in two different
> ways. This is pretty bad for predictability, least-surprise,
> documentation, ...
>
> To recap, the two ways are escaping (escaped commas are ignored,
> backslashes also need to be escaped) and quoting (values are optionally
> enclosed in double quotes, to include a '"' in a quoted string, use '""').
> Escaping is good because users are used to it, but currently literal
> backslashes need to be doubled, making multivalue syntax different from
> single-value syntax, even if there are no commas in the value.
> Quoting is weird, but complete by itself so it doesn't also need escaping.
>
> Do we need to use both? Is this documented somewhere? Some of our tests
> check only for one, some assume the other. Users are probably even more
> confused.

I think there was a discussion going on about removing the quoting 
sometime in the past.

>
>
> If we only keep one of those, the fix for #2227 should be quite easy.
> If not (backwards compatibility), we need to document this properly,
> test all the corner cases, and fix the UI to handle CSV escaping.
> Since it's subtly broken in lots of places, maybe it's best to break
> backwards compatibility and go for a simple solution now.
>
> Anyway, if I want to do a proper fix, CSV handling should be only done
> on the client. Unfortunately turning off CSV handling in the server will
> break the UI, which at places uses `join(',')` (no escaping commas, no
> single place to change it), even though it can just send a list.
>

+1, but I'm not sure if that's acceptable (see 
http://www.redhat.com/archives/freeipa-devel/2012-January/msg00197.html)

Honza

-- 
Jan Cholasta