click-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Lorenzo Simionato <lore...@simionato.org>
Subject Re: Request Parameter Autobinding
Date Sat, 20 Nov 2010 14:49:38 GMT
On Nov 20, 2010, at 14:35 , Bob Schellink wrote:

> Hi,
> 
> 
> On 20/11/2010 22:43, Lorenzo Simionato wrote:
>> 
>> 1-Is it possible to disable the feature that: "binds automatically any request parameter
values to public Page fields with the same name"?
> 
> 
> Yep, just set autobinding="none"

OK. From the user guide it seemed that autobinding="none" was only for Page Autobinding and
not for Request Parameters.

> 
> Binding request parameters will only be done for "public" or Bindable fields. Java devs
generally
> delare fields as private or protected so unlikely to be an issue.
> 
>> I see that it is possible for page autobinding but not for request parameters. I
find this feature very subtle, makes the code less clear and is 
> 
> I don't like the feature much either. One thing I'd like to do for a future release is
revamping the
> click-examples to not use autobinding.
> 
>> possibly dangerous (class fields can be set by an attacker in a way that is not evident
and it is easy to make mistakes).
> 
> If you declare the field as @Bindable how is it more dangerous than using a Form or link?
An
> attacker can tamper with any value.

Here what i meant is that if you declare a field as @Bindable you are clearly aware that it
can be set in some way by the user.
If you have a public field (ok, it's rare) this is not that obvious.

>> 
>> 2-According to the documentation: "When binding these values Click will also attempt
to convert them to the correct type". However, if the 
>> conversion is not successful is the intended behavior to throw an exception?
>> Say i have a page:
>> public class MyPage extends Page {
>>   @Bindable
>>    protected Integer customerId;
>> }
>> and the following request is made: mypage.htm?customerId=xxx
>> 
>> In this case an exception is thrown.
> 
> Why is this a problem? If your an attacker is tampering with data what should your application
do?

Maybe set the field to null, but the exception is okay. Just asking if this was the indented
behavior.

> 
> 
>> 3-Why the @Bindable annotation is used both for request parameters and for page autobinding
(if autobinding for pages is enabled of course)?
>> This makes it very confusing. It is not clear if @Bindable is used to get a parameter
or put something on the page.
> 
> Yeah I don't like it either. Ideally Bindable should only work for request parameters
not Controls.
> However this will be difficult to add as majority of existing apps will break. First
thing is to
> revamp the examples so people aren't encouraged to use this pattern.

A possibility could be to introduce a new annotation like @InputParameter or something like
this to clearly separate the two things.
Backward compatibility can be possibly maintained by adding an option into click.xml (e.g.
a new value for the aubinding property)

> 
>> In addition, it could lead to security problems.
>> For example, consider the page:
>> MyPage.java:
>> public class MyPage extends Page {
>>   @Bindable
>>    protected String welcomeMessage = "Welcome to my web site";
> 
> 
> The security problem doesn't come from Bindable but from rendering without escaping.
The Velocity
> template model can be populated from anywhere including forms, arbitrary request params,
web
> services, external systems etc. For example:
> 
>  addModel("msg", getContext().getRequestParameter("welcomeMessage"));
> 
> If you are rendering output that could be dangerous you should escape it:
> 
> $format.html($msg);
> 
> In our app we've gone even further, specifying a Velocity ReferenceInsertionEventHandler.
> 
>  eventhandler.referenceinsertion.class=org.apache.velocity.app.event.implement.EscapeHtmlReference
>  eventhandler.escape.html.match = /_.*/
> 
> So any variable in the template that starts with _ is escaped. In our border template
we overrode
> addModel to ensure all non control objects added are prefixed with an underscore. Since
Controls
> escape their values and attributes we had to exclude them otherwise they are doubly escaped.
The net
> effect is that when people rendered Velocity model objects they had to use:
> 
>  $_customer
> 
> Not that user-friendly, but safe.
> 
> Kind regards
> 
> Bob

Here the XSS was just an example. The fact that one can set a value that i intended only for
output is disturbing.
As a couple of other examples:
-suppose the welcomeMessage is the title of the page. It's not nice that one can put an arbitrary
title on the page, even if it is escaped properly.

-suppose one modifies the RequestTypeConverter as explained in the documentation to dynamically
load customers from the db.
In a page one would like to do something with a customer object and then print the details,
so we could have something like:
MyPage.class
public class MyPage extends Page {
   @Bindable protected Customer customer = loadCustomer(3);

    pubic void action() {
         customer.set.....();
    }

MyPage.html
$customer.name

a different customer can be loaded with a request like mypage.htm?customer=56
(this example is a little weird but is just to get the idea)


These are just examples and maybe if all is handled very carefully by the programmer there
would not be any problems.
However, they demonstrate that it is easy to make something that does not work as intended.
As a last example consider SQL Injections: if you escape the input properly you do not have
the problem. On the other hand,
to prevent the problem even if you are not that careful PHP has introduced magic quotes and
in Java we have preparedStatements (yes i'm simplifying a lot the things here!). The concept
is that this double role of public field and ones annotated with @Bindable (as parameters
and variables added to the page) it does not seem a good idea to me.

--
Lorenzo
Mime
View raw message