[Kss-devel] Cleanup of kss.core - sanitization of string and html params

[Balazs Ree]

On Thu, 13 Mar 2008 21:58:07 +0100, Jeroen Vloothuis wrote:

> The kss.core branch which builds on kss.zope has been cleaned of all the
> unneeded code. What is left is either tests (or related documents) or
> bbb imports or code.
> 
> While cleaning out kss.core I came across a few module which seem to
> have some use but that I am uncertain of how to handle these.
> 
> The first one is BeautifulSoup.py, this is imported from a plone.app.kss
> directly. Next is the unicode_quirks.py file. It is used from
> plone.app.kss as well (plonekssview.py).

I'll try to explain first, how these currently work in kss.core, and what 
is their functionality.

BeautifulSoup.py is an html parser that is used for sanitizing the html 
output, and its usage is enforced each time html is sent to the client. 
This is done each time an HTML parameter is added, by the addHtmlParam 
method, ie any time when the semantics of a parameter is "HTML".

The force_unicode method works similarly for adding parameters with a 
"string" semantics, ie. when addString is used. This makes sure that 
either plain ascii, or proper unicode is added (with an additional 
parameter that makes it possible to _explicitely_ enforce conversion from 
a given codepage other then utf8.) In case of faulty input an exception 
is raised on the server side.

> My suggestion is to just to leave them in kss.core so that we can
> deprecate them at some later time.

Before getting to the bbb policy at all, there is a more important 
question that raises.

The AddHtmlParam and AddStringParam, besides implementation reason that 
may occur differently in the case of kss.base, have a very important 
function. We do not only handle these parameter types disctintly from 
each other because we marshal them disctinctly on the kss command payload 
(which we may change in the next implementation), but precisely because 
we want to check and sanitize the parameter content on the server side. 
This altogether improves the quality of applications based on top of kss, 
and enables earlier caching of certain application errors.

I would like to refer to some of our earlier discussions when I tried to 
emphasize the importance of keeping the different addXXXParam methods 
based on the parameter's semantics, versus having a single addBlobParam 
method. (As in the IKSSCommand interface.) One of my reasons for doing so 
is that I am strongly on the point of view, that these sanitizations are 
very important. I keep standing on this point still at the moment. Apart 
from this importance, the other reason is that I believe that the switch 
to kss.zope should not cause any observable difference to the 
applications that are running on top of kss. It is important to nail down 
that he goal of kss.base - although it was a complete rewrite - is not 
redesigning the kss server side behaviour but to enable exactly the same 
behaviour that we currently have, on the level of plain python 
applications. We may want to do refactorizazion in the future but it must 
occur in a consecutive step, separated from the current porting effort.

Now, if implemented properly, this functionality belongs to the level of 
kss.base. I am not sure if this checking is implemented in kss.base and 
on what extent, but it seems not or not identically implemented to the 
way they currently are in kss.core. However I may be wrong at this point.

So the question that is raised: I offered some reasons above why I find 
this functionality important and to be kept. Do we have any reason to 
drop this functionality in the next release (based on kss.base)?

And what problems may possibly arise for existing applications and Plone, 
in particular, if we do so?

I believe we must discuss and answer these questions first, and decide 
the implementation and deprecation policy based on the decision we make.
 

> The buildout for the kss.zope related work has been updated to match the
> trunk of Plone. All tests in de demo's pass and a Grok demo works.

Obviously, tests were (again) missing in this case, otherwise they would 
not pass.

After deciding on the above mentioned question, what would follow?

If we decide to keep this checks, we must add additional tests to 
kss.core that check for the existence of the string and html checking and 
sanitization. Following this the tests must be moved or implemented in 
kss.base as well (together with the actual implementation of the 
sanitization, if missing.)

In case we decide to drop these checks, we must carefully check in real 
life scenarios, what errors or changed behaviour we get in case broken 
html or sting content is inserted to a kss command. If this is the case 
we may still consider if we redistribute BeautifulSoup in kss.zope 
because it is a very handy html parser that we used several times in 
applications that produce html content for kss, but in this case it will 
be just provided as a utility and not be used by kss.base/kss.core itself.


Best wishes

-- 
Balazs Ree