You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by Michael Dürig <mi...@gmail.com> on 2012/06/07 13:07:10 UTC
NodeDelegate leakage from NodeImpl
Hi,
I noticed that since revision 1344662 NodeImpl has an (package private)
accessor for NodeDelegate. This defeats the original intent of the
separation of NodeImpl and NodeDelegate (OAK-84): users should not be
able to gain access to internals by hacking NodeImpl. But precisely this
is now possible when a user put his code into the
org.apache.jackrabbit.oak.jcr package.
Michael
Re: NodeDelegate leakage from NodeImpl
Posted by Michael Dürig <md...@apache.org>.
I opened OAK-136 for the immediate issue at hand.
Michael
On 7.6.12 15:43, Michael Dürig wrote:
>
>
> On 7.6.12 14:55, Angela Schreiber wrote:
>> On 6/7/12 1:55 PM, Julian Reschke wrote:
>>> On 2012-06-07 13:07, Michael Dürig wrote:
>>>>
>>>> Hi,
>>>>
>>>> I noticed that since revision 1344662 NodeImpl has an (package private)
>>>> accessor for NodeDelegate. This defeats the original intent of the
>>>> separation of NodeImpl and NodeDelegate (OAK-84): users should not be
>>>> able to gain access to internals by hacking NodeImpl. But precisely
>>>> this
>>>> is now possible when a user put his code into the
>>>> org.apache.jackrabbit.oak.jcr package.
>>>>
>>>> Michael
>>>
>>> Apparently because UserManagerImpl deals with nodes, but does want to
>>> set properties directly using the delegates. (Performance? Skipping
>>> checks?)
>>
>> being able to set protected properties (currently this check
>> is missing but we will need to add it once the node type stuff
>> is implemented)... feel free to change the implementation of
>> SessionDelegate#getDelegate(Node jcrNode)... i didn't want to
>> use the method taking a path since i have the node at hand
>> already.... that looked really bad to me.
>
> Implementations which need access to internals should be done against
> the delegates. The delegatee can always be retrieved from the respective
> delegate when necessary but not vice versa. See OAK-81. The delegates
> are this strictly more general.
>
>>
>> after all i am not convinced that the separation between delegate
>> and impl is really the right approach... to much duplicate
>> methods with no actual benefit IMO.
>
> Ok we should then open an issue for that and reconsider that. The
> current implementation however breaks the intention of OAK-81.
>
> Michael
>
>>
>> regards
>> angela
>>
Re: NodeDelegate leakage from NodeImpl
Posted by Michael Dürig <md...@apache.org>.
On 7.6.12 14:55, Angela Schreiber wrote:
> On 6/7/12 1:55 PM, Julian Reschke wrote:
>> On 2012-06-07 13:07, Michael Dürig wrote:
>>>
>>> Hi,
>>>
>>> I noticed that since revision 1344662 NodeImpl has an (package private)
>>> accessor for NodeDelegate. This defeats the original intent of the
>>> separation of NodeImpl and NodeDelegate (OAK-84): users should not be
>>> able to gain access to internals by hacking NodeImpl. But precisely this
>>> is now possible when a user put his code into the
>>> org.apache.jackrabbit.oak.jcr package.
>>>
>>> Michael
>>
>> Apparently because UserManagerImpl deals with nodes, but does want to
>> set properties directly using the delegates. (Performance? Skipping
>> checks?)
>
> being able to set protected properties (currently this check
> is missing but we will need to add it once the node type stuff
> is implemented)... feel free to change the implementation of
> SessionDelegate#getDelegate(Node jcrNode)... i didn't want to
> use the method taking a path since i have the node at hand
> already.... that looked really bad to me.
Implementations which need access to internals should be done against
the delegates. The delegatee can always be retrieved from the respective
delegate when necessary but not vice versa. See OAK-81. The delegates
are this strictly more general.
>
> after all i am not convinced that the separation between delegate
> and impl is really the right approach... to much duplicate
> methods with no actual benefit IMO.
Ok we should then open an issue for that and reconsider that. The
current implementation however breaks the intention of OAK-81.
Michael
>
> regards
> angela
>
Re: NodeDelegate leakage from NodeImpl
Posted by Angela Schreiber <an...@adobe.com>.
On 6/7/12 1:55 PM, Julian Reschke wrote:
> On 2012-06-07 13:07, Michael Dürig wrote:
>>
>> Hi,
>>
>> I noticed that since revision 1344662 NodeImpl has an (package private)
>> accessor for NodeDelegate. This defeats the original intent of the
>> separation of NodeImpl and NodeDelegate (OAK-84): users should not be
>> able to gain access to internals by hacking NodeImpl. But precisely this
>> is now possible when a user put his code into the
>> org.apache.jackrabbit.oak.jcr package.
>>
>> Michael
>
> Apparently because UserManagerImpl deals with nodes, but does want to
> set properties directly using the delegates. (Performance? Skipping checks?)
being able to set protected properties (currently this check
is missing but we will need to add it once the node type stuff
is implemented)... feel free to change the implementation of
SessionDelegate#getDelegate(Node jcrNode)... i didn't want to
use the method taking a path since i have the node at hand
already.... that looked really bad to me.
after all i am not convinced that the separation between delegate
and impl is really the right approach... to much duplicate
methods with no actual benefit IMO.
regards
angela
Re: NodeDelegate leakage from NodeImpl
Posted by Julian Reschke <ju...@gmx.de>.
On 2012-06-07 13:07, Michael Dürig wrote:
>
> Hi,
>
> I noticed that since revision 1344662 NodeImpl has an (package private)
> accessor for NodeDelegate. This defeats the original intent of the
> separation of NodeImpl and NodeDelegate (OAK-84): users should not be
> able to gain access to internals by hacking NodeImpl. But precisely this
> is now possible when a user put his code into the
> org.apache.jackrabbit.oak.jcr package.
>
> Michael
Apparently because UserManagerImpl deals with nodes, but does want to
set properties directly using the delegates. (Performance? Skipping checks?)
Best regards, Julian
Re: NodeDelegate leakage from NodeImpl
Posted by Michael Dürig <md...@apache.org>.
On 13.6.12 8:04, Thomas Mueller wrote:
> Hi,
>
>> This defeats the original intent of the
>> separation of NodeImpl and NodeDelegate (OAK-84)
>> users should not be able to gain access to internals by hacking NodeImpl.
>
>
> Hm, is this a security problem? Do we want to protect the data from users
> of the JCR API?
No, its about making it as difficult as possible to mess things up by
using internals. I.e. its about avoiding the
((NodeImpl) node).messWithMe();
pattern.
Michael
>
> Or do we want to protect the data within the Oak implementation (use a
> better abstraction)?
>
>> But precisely this
>> is now possible when a user put his code into the
>> org.apache.jackrabbit.oak.jcr package.
>
> A attacker could always use reflection (setAccessible(true)). If we want
> real protection, we would have to enforce using a SecurityManager. Then we
> could seal the package [1]
>
> [1] http://docs.oracle.com/javase/tutorial/deployment/jar/sealman.html
>
> Regards,
> Thomas
>
>
>
>
>
>
>
> On 6/7/12 1:07 PM, "Michael Dürig"<mi...@gmail.com> wrote:
>
>>
>> Hi,
>>
>> I noticed that since revision 1344662 NodeImpl has an (package private)
>> accessor for NodeDelegate. This defeats the original intent of the
>> separation of NodeImpl and NodeDelegate (OAK-84): users should not be
>> able to gain access to internals by hacking NodeImpl. But precisely this
>> is now possible when a user put his code into the
>> org.apache.jackrabbit.oak.jcr package.
>>
>> Michael
>>
>>
>
Re: NodeDelegate leakage from NodeImpl
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
> This defeats the original intent of the
> separation of NodeImpl and NodeDelegate (OAK-84)
> users should not be able to gain access to internals by hacking NodeImpl.
Hm, is this a security problem? Do we want to protect the data from users
of the JCR API?
Or do we want to protect the data within the Oak implementation (use a
better abstraction)?
> But precisely this
> is now possible when a user put his code into the
> org.apache.jackrabbit.oak.jcr package.
A attacker could always use reflection (setAccessible(true)). If we want
real protection, we would have to enforce using a SecurityManager. Then we
could seal the package [1]
[1] http://docs.oracle.com/javase/tutorial/deployment/jar/sealman.html
Regards,
Thomas
On 6/7/12 1:07 PM, "Michael Dürig" <mi...@gmail.com> wrote:
>
>Hi,
>
>I noticed that since revision 1344662 NodeImpl has an (package private)
>accessor for NodeDelegate. This defeats the original intent of the
>separation of NodeImpl and NodeDelegate (OAK-84): users should not be
>able to gain access to internals by hacking NodeImpl. But precisely this
>is now possible when a user put his code into the
>org.apache.jackrabbit.oak.jcr package.
>
>Michael
>
>