pombreda / formalchemy

Automatically exported from code.google.com/p/formalchemy
MIT License
0 stars 0 forks source link

helpers.escape_once should not exist #111

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
helpers.escape_once ensures that HTML entities are only ever singly escaped
when generating form code. This is a bad idea: if I have a row in the
database containing the string '<', edit that row via formalchemy, and
save it back unaltered, the database will now contain '<', because
helpers.escape_once removed a layer of HTML escaping. Saving a row
unaltered should not affect the database!

Original issue reported on code.google.com by adb...@gmail.com on 15 Aug 2009 at 4:17

GoogleCodeExporter commented 9 years ago
I don't really understand the problem. Can you give a code snippet to reproduce 
the
problem ?

Original comment by gael.pas...@gmail.com on 18 Aug 2009 at 10:15

GoogleCodeExporter commented 9 years ago
Okay, we store Model objects in a database table with one column, a String.

>>> import sqlalchemy as sa
>>> from sqlalchemy.ext import declarative
>>> Base = declarative.declarative_base()
>>> class Model(Base):
>>>     __tablename__ = 'table'
>>>     text = sa.Column(sa.String)

We add a Model object to the database, with the string '<'.

>>> model = Model()
>>> model.text = '<'

We use formalchemy to render a HTML form for that object.

>>> import formalchemy as fa
>>> html = fa.FieldSet(model).render()

We pass this HTML to the user, who submits the form without changes, and our 
webapp 
syncs the submitted data back to the database.

>>> data = <data submitted by user>
>>> fa.FieldSet(model, data=data).sync()

At this point, model.text is changed from '<' to '<', even though the user made 
no
changes to the form.

The issue is that FieldSet.render uses helpers.escape_once to HTML escape the 
<input>
tags it generates. The text field should be rendered as '<input ...
value="&lt;">', i.e. a text field with value '<'. Instead, helpers.escape_once
decides that one level of HTML escaping is enough, and renders it as '<input ...
value="<">', i.e. a text field with value '<'. I can think of no reason for ever
doing this: the browser will remove exactly one level of HTML escaping, so we 
should
add exactly one level when rendering, but helpers.escape_once adds zero or one, 
as it
sees fit.

Original comment by adb...@gmail.com on 18 Aug 2009 at 10:49

GoogleCodeExporter commented 9 years ago

Original comment by gael.pas...@gmail.com on 18 Aug 2009 at 10:59

GoogleCodeExporter commented 9 years ago
After a few tries I think that this is not a good idea.

Why do you want "&lgt;" instead of ">" in the field value ? Have you some 
non-human
users ? :)

If it's because you want to use a WYSIWYG editor you can always write a custom
renderer for that.

Original comment by gael.pas...@gmail.com on 19 Aug 2009 at 1:22

GoogleCodeExporter commented 9 years ago
It could be that those characters turned up in a user's password, or in a blog 
post
about HTML escaping, but why they're there isn't the point. If I ask 
formalchemy to
render the text '<' in HTML, it should do so! It shouldn't decide that what I
really wanted was to unescape the text first. Explicit is better than implicit, 
and
all that.

What's your use case for the present behaviour? If you want to render '<', just 
store
'<' in the database; that's what FieldSet.sync will do when a user enters '<' 
in a
form. If I store '<' in the database, it is because that's the value I want to
render! I don't see why you think it's better to mangle any database values
containing HTML entities, just in case the developer felt like unescaping them, 
and
forgot to ask.

Original comment by adb...@gmail.com on 19 Aug 2009 at 1:45

GoogleCodeExporter commented 9 years ago
Just because this will break your form if you have a " in your value.

Escaping is a common feature of form libraries:

>>> from webhelpers.html.tags import text
>>> text('field', value='"')
literal(u'<input id="field" name="field" type="text" value=""" />')

Original comment by gael.pas...@gmail.com on 19 Aug 2009 at 2:27

GoogleCodeExporter commented 9 years ago
Oh, sorry, I should have been clearer! I'm not saying we should replace calls to
helpers.escape_once with nothing, that obviously leads to problems. I'm saying 
we
should replace them with helpers.html_escape, which will provide the correct 
escaping
behaviour in all circumstances, rather than just in the more obvious ones.

Original comment by adb...@gmail.com on 19 Aug 2009 at 2:42

GoogleCodeExporter commented 9 years ago

Original comment by gael.pas...@gmail.com on 27 Aug 2009 at 10:03

GoogleCodeExporter commented 9 years ago
Sorry, I've closed the issue without reading your last post.

I did'nt know why FA use this. Maybe jbellis can explain that.

I've try to replace escape_once with html_excape and all tests works.

But since I don't have problem with that myself I don't want to change this if 
it can
break some existing code.

Original comment by gael.pas...@gmail.com on 27 Aug 2009 at 10:10

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
escape_once do not exist since WebHelper is used to render fields

Original comment by gael.pas...@gmail.com on 5 Mar 2010 at 9:13