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

[Petr Viktorin]

On 02/23/2012 04:08 PM, Jan Cholasta wrote:
 > 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.

All right, I'll do that once we decide on how to escape. Self-NACK for 
the patch.

...

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

Was there an agreement? Was backwards compatibility the only problem?

...

On 02/23/2012 04:14 PM, Martin Kosek wrote:
> On Thu, 2012-02-23 at 16:08 +0100, Jan Cholasta wrote:
>> On 23.2.2012 15:29, Petr Viktorin wrote:
> ...
>>> 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)
>>

There's still only one place to change.
And since this is input, not output; and we want backwards 
compatibility, a formatting change is pretty hard to push through as it 
is :)

> I tend to like this solution as well, it is true that CSV parsing is
> really a client thing. Our life as a server would be much easier if we
> just accept scalar values in an array, either from XMLRPC or JSON
> interface. It is doable, but it would break IPAv2.x clients which then
> would not be able to use CSV formatted values at all. And we want to
> avoid that.
>
> Martin
>

Keep in mind that currently the parsing is done on *both* sides. The CSV 
splitting is first done at the client, then validated at the client, and 
then split and validated on the server. For example (without my patch):

--option='"a,b",c,"""d""""e"""' --option='a\\\,b,c\\d'

Client gets (r'"a\,b",c,"""d""""e"""', r'a\\\,b,c\\d')
Cli splits to ('a,b' 'c', '"d""e"' r'a\,b', r'c\d'), validates, forwards
Srv splits to ('a', 'b', 'c', 'd"e', r'a,b', r'cd'), validates, executes

You need four backslashes for a literal backslash, three to escape a 
comma. I think 2.1 clients are already broken, and the backwards 
incompatibility would only affect workarounds.


As far as I can see the Web UI is tied to the server version, so 
changing the JSON communication shouldn't be a problem?
Anyway, since the JSON code for this is scattered across the codebase, 
we need to introduce a common JS function first, and only then switch 
both that and the server to use plain arrays.

-- 
Petr³