Re: svn commit: r520056 - in /incubator/roller: branches/roller_2.3/ branches/roller_2.3/src/org/apa

On 3/19/07, Allen Gilliland <allen.gillilandsun.com> wrote:
> I'd like to suggest that we do one more thing to fix
this problem
> starting in the current trunk.  I'd like to go ahead
and make our pojo
> wrappers static so that we can place custom code in
various methods to
> handle situations like this.  The problem with the
current fix is that
> it relies on the fact that people are using the macros
and that can't be
> guaranteed, so to truly solve this problem we need the
functionality to
> be in the pojo wrappers themselves so that there is no
way to get
> unescaped data.
>
> So to do this all I am planning to do is copy the
current generated
> wrappers into the actual source tree and commit them,
then modify the
> various getXXX() methods on the CommentDataWrapper so
that they escape
> the data.
>
> Not only does this help fix this security issue at the
very root level,
> but it will also open up opportunities to do more with
our wrappers
> general.  So is anyone else opposed to making the pojo
wrappers static?
>
> I don't think this change would need to be back ported
to older
> releases, so it would just go into the current trunk.

I'm +0 on this.

It eliminates one reason we need XDoclet, that's good.

It opens opportunities for smarter POJO wrappers, which is
good --
but, other than comments security what else can we use that
for?

It adds more code to maintain and more code to update when a
new field
or POJO is added, but I guess with a good IDE that work is
negligible.

So unless somebody else has some good solid objections, go
for it.

- Dave

Dave wrote:
> On 3/19/07, Allen Gilliland <allen.gillilandsun.com> wrote:
>> I'd like to suggest that we do one more thing to
fix this problem
>> starting in the current trunk.  I'd like to go
ahead and make our pojo
>> wrappers static so that we can place custom code in
various methods to
>> handle situations like this.  The problem with the
current fix is that
>> it relies on the fact that people are using the
macros and that can't be
>> guaranteed, so to truly solve this problem we need
the functionality to
>> be in the pojo wrappers themselves so that there is
no way to get
>> unescaped data.
>>
>> So to do this all I am planning to do is copy the
current generated
>> wrappers into the actual source tree and commit
them, then modify the
>> various getXXX() methods on the CommentDataWrapper
so that they escape
>> the data.
>>
>> Not only does this help fix this security issue at
the very root level,
>> but it will also open up opportunities to do more
with our wrappers
>> general.  So is anyone else opposed to making the
pojo wrappers static?
>>
>> I don't think this change would need to be back
ported to older
>> releases, so it would just go into the current
trunk.
> 
> I'm +0 on this.
> 
> It eliminates one reason we need XDoclet, that's good.
> 
> It opens opportunities for smarter POJO wrappers, which
is good --
> but, other than comments security what else can we use
that for?
> 
> It adds more code to maintain and more code to update
when a new field
> or POJO is added, but I guess with a good IDE that work
is negligible.
> 
> So unless somebody else has some good solid objections,
go for it.
> 
> - Dave
> 

I think I know what you are trying to fix, but when we did
it for Lotus
Connections we didn't do it in the wrappers. I'm not sure
why wrappers
has to be the place, if so, it does make it uglier because
of more
changes to make everytime we change/add to the data model.

-Elias

Dave wrote:
> On 3/19/07, Allen Gilliland <allen.gillilandsun.com> wrote:
>> I'd like to suggest that we do one more thing to
fix this problem
>> starting in the current trunk.  I'd like to go
ahead and make our pojo
>> wrappers static so that we can place custom code in
various methods to
>> handle situations like this.  The problem with the
current fix is that
>> it relies on the fact that people are using the
macros and that can't be
>> guaranteed, so to truly solve this problem we need
the functionality to
>> be in the pojo wrappers themselves so that there is
no way to get
>> unescaped data.
>>
>> So to do this all I am planning to do is copy the
current generated
>> wrappers into the actual source tree and commit
them, then modify the
>> various getXXX() methods on the CommentDataWrapper
so that they escape
>> the data.
>>
>> Not only does this help fix this security issue at
the very root level,
>> but it will also open up opportunities to do more
with our wrappers
>> general.  So is anyone else opposed to making the
pojo wrappers static?
>>
>> I don't think this change would need to be back
ported to older
>> releases, so it would just go into the current
trunk.
> 
> I'm +0 on this.
> 
> It eliminates one reason we need XDoclet, that's good.
> 
> It opens opportunities for smarter POJO wrappers, which
is good --
> but, other than comments security what else can we use
that for?

Well, comment security is one place, but there's lots of
other places 
where we do little things like escape the data for html/xml
or other 
little formatting changes which can all be applied
automatically in the 
wrapper so that we don't have to rely on template writers to
do that. 
That makes templates cleaner and nicer looking and
ultimately more 
usable IMO.

Plugins are another good example.  I think I had mentioned
this before, 
but IMO when you call $entry.text in your template you
should get back 
the fully formatted entry body rather than the original
text.  There 
would be various places that we can make use of
functionality like this.

And a third option is that we can then add methods which
only exist in 
the wrappers but not in the pojo, which makes sense in some
situations.


> 
> It adds more code to maintain and more code to update
when a new field
> or POJO is added, but I guess with a good IDE that work
is negligible.

I agree that this means a little more work, but I also think
this is 
good work.  We don't change the wrapped methods very often
so I don't 
think the work is going to be very much, and doing this
manually is more 
secure.  For example I believe at one point I noticed that
some of the 
pojo getId() methods were wrapped and really they shouldn't
have been 
because there is no use for them and it's most likely that
was just 
copied code from somewhere and nobody noticed.  So doing
this manually 
makes some sense to me because we are forced to make sure
that only the 
data we want available to users is exposed and it's less
likely that 
data would exposed accidentally.

-- Allen


> 
> So unless somebody else has some good solid objections,
go for it.
> 
> - Dave