You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by Musachy Barroso <mu...@gmail.com> on 2008/07/17 23:28:52 UTC
hacking OGNL and parameter binding
Yeah I am set to fix those security holes ;). Doing the change below,
all tests pass, with the exception of some tests in
ParameterInterceptorTest, that need to inject dependencies, and others
that check for the order of the values added to the stack (new context
is created here, so they fail)
+ ValueStack emptyStack = valueStackFactory.createValueStack(stack);
+ Map<String, Object> context = emptyStack.getContext();
+ ((OgnlContext)context).getValues().clear(); /// THIS IS BAD
+ ReflectionContextState.setCreatingNullObjects(context, true);
+ ReflectionContextState.setDenyMethodExecution(context, true);
+ ReflectionContextState.setReportingConversionErrors(context, true);
+
for (Map.Entry<String, Object> entry :
acceptableParameters.entrySet()) {
String name = entry.getKey();
Object value = entry.getValue();
@@ -233,7 +265,7 @@
String name = entry.getKey();
Object value = entry.getValue();
try {
- stack.setValue(name, value);
+ emptyStack.setValue(name, value);
} catch (RuntimeException e) {
if (devMode) {
String developerNotification =
LocalizedTextUtil.findText(ParametersInterceptor.class,
"devmode.notification", ActionContext.getContext().getLocale(),
"Developer Notification:\n{0}", new Object[]{
@@ -246,6 +278,9 @@
}
}
}
+ stack.getContext().putAll(acceptableParameters);
+
The 2 big things to be addressed are:
1. ((OgnlContext)context).getValues().clear();
I cannot just do context.clear(), because that method not only removes
the values from the stack, but it clears the root, type converter and
other stuff, so we will have to add another "clear" method to the
OgnlContext, that just clears the values.
2. throwPropertyExceptions which needs to be the same in the new value
stack, but I think it is getting cleared.
what do you guys think?
musachy
--
"Hey you! Would you help me to carry the stone?" Pink Floyd
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: hacking OGNL and parameter binding
Posted by Musachy Barroso <mu...@gmail.com>.
I have committed my changes to xwork trunk, I will be testing more on
Showcase and trying to write some integration tests as I go. If
anybody else could help testing this, have fun :). If nothing is
broken I will also commit the changes to the xwork branch.
musachy
On Sat, Jul 19, 2008 at 8:50 AM, Musachy Barroso <mu...@gmail.com> wrote:
>> I'm not sure how to get permission to make comments in crucible.
>
> I guess we would have to benchmark it, but I don't see anything big
> going on, as the objects(like root, context, etc) are just set into
> the new stack.
>
> musachy
>
--
"Hey you! Would you help me to carry the stone?" Pink Floyd
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: hacking OGNL and parameter binding
Posted by Musachy Barroso <mu...@gmail.com>.
> I'm not sure how to get permission to make comments in crucible.
I guess we would have to benchmark it, but I don't see anything big
going on, as the objects(like root, context, etc) are just set into
the new stack.
musachy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: hacking OGNL and parameter binding
Posted by Jeromy Evans <je...@blueskyminds.com.au>.
Musachy Barroso wrote:
> I opened a code review here:
>
> http://fisheye6.atlassian.com/cru/CR-9
>
> I added a new interface ClearableValueStack, which if implemented will
> make the OGNL parameter binding run in a clean context.
>
>
I'm not sure how to get permission to make comments in crucible.
General comment:
Seems okay, but need to check impact on the s:action tag and chain
result. From memory, they create an invocation that set params of the
next action via the param interceptor (?) and assume the context is not
cleared. I could be wrong about that, but I don't recall the params
being set by any other means.
Any estimate on the potential performance hit of creating a new stack?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: hacking OGNL and parameter binding
Posted by Musachy Barroso <mu...@gmail.com>.
duh..done :)
On Sat, Jul 19, 2008 at 3:44 AM, Don Brown <mr...@twdata.org> wrote:
> Musachy, you need to mark the box that lets anyone join the review as
> a reviewer. As it is now, no one can comment.
>
> Don
>
> On Sat, Jul 19, 2008 at 12:47 AM, Musachy Barroso <mu...@gmail.com> wrote:
>> I opened a code review here:
>>
>> http://fisheye6.atlassian.com/cru/CR-9
>>
>> I added a new interface ClearableValueStack, which if implemented will
>> make the OGNL parameter binding run in a clean context.
>>
>> musachy
>>
>> On Thu, Jul 17, 2008 at 5:46 PM, Musachy Barroso <mu...@gmail.com> wrote:
>>> I think it would be the same, we would just need to add a method to
>>> ValueStack, to clear the context.
>>>
>>> musachy
>>>
>>> On Thu, Jul 17, 2008 at 5:32 PM, Chris Pratt <th...@gmail.com> wrote:
>>>> Will it be pluggable between the new-and-improved ValueStack and the
>>>> OGNL ValueStack so that we can make the transition as painless as
>>>> possible?
>>>> (*Chris*)
>>>>
>>>> On Thu, Jul 17, 2008 at 2:28 PM, Musachy Barroso <mu...@gmail.com> wrote:
>>>>> Yeah I am set to fix those security holes ;). Doing the change below,
>>>>> all tests pass, with the exception of some tests in
>>>>> ParameterInterceptorTest, that need to inject dependencies, and others
>>>>> that check for the order of the values added to the stack (new context
>>>>> is created here, so they fail)
>>>>>
>>>>> + ValueStack emptyStack = valueStackFactory.createValueStack(stack);
>>>>> + Map<String, Object> context = emptyStack.getContext();
>>>>> + ((OgnlContext)context).getValues().clear(); /// THIS IS BAD
>>>>> + ReflectionContextState.setCreatingNullObjects(context, true);
>>>>> + ReflectionContextState.setDenyMethodExecution(context, true);
>>>>> + ReflectionContextState.setReportingConversionErrors(context, true);
>>>>> +
>>>>> for (Map.Entry<String, Object> entry :
>>>>> acceptableParameters.entrySet()) {
>>>>> String name = entry.getKey();
>>>>> Object value = entry.getValue();
>>>>> @@ -233,7 +265,7 @@
>>>>> String name = entry.getKey();
>>>>> Object value = entry.getValue();
>>>>> try {
>>>>> - stack.setValue(name, value);
>>>>> + emptyStack.setValue(name, value);
>>>>> } catch (RuntimeException e) {
>>>>> if (devMode) {
>>>>> String developerNotification =
>>>>> LocalizedTextUtil.findText(ParametersInterceptor.class,
>>>>> "devmode.notification", ActionContext.getContext().getLocale(),
>>>>> "Developer Notification:\n{0}", new Object[]{
>>>>> @@ -246,6 +278,9 @@
>>>>> }
>>>>> }
>>>>> }
>>>>> + stack.getContext().putAll(acceptableParameters);
>>>>> +
>>>>>
>>>>> The 2 big things to be addressed are:
>>>>>
>>>>> 1. ((OgnlContext)context).getValues().clear();
>>>>>
>>>>> I cannot just do context.clear(), because that method not only removes
>>>>> the values from the stack, but it clears the root, type converter and
>>>>> other stuff, so we will have to add another "clear" method to the
>>>>> OgnlContext, that just clears the values.
>>>>>
>>>>> 2. throwPropertyExceptions which needs to be the same in the new value
>>>>> stack, but I think it is getting cleared.
>>>>>
>>>>> what do you guys think?
>>>>>
>>>>> musachy
>>>>> --
>>>>> "Hey you! Would you help me to carry the stone?" Pink Floyd
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
>>>>> For additional commands, e-mail: dev-help@struts.apache.org
>>>>>
>>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
>>>> For additional commands, e-mail: dev-help@struts.apache.org
>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> "Hey you! Would you help me to carry the stone?" Pink Floyd
>>>
>>
>>
>>
>> --
>> "Hey you! Would you help me to carry the stone?" Pink Floyd
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
>> For additional commands, e-mail: dev-help@struts.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
--
"Hey you! Would you help me to carry the stone?" Pink Floyd
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: hacking OGNL and parameter binding
Posted by Don Brown <mr...@twdata.org>.
Musachy, you need to mark the box that lets anyone join the review as
a reviewer. As it is now, no one can comment.
Don
On Sat, Jul 19, 2008 at 12:47 AM, Musachy Barroso <mu...@gmail.com> wrote:
> I opened a code review here:
>
> http://fisheye6.atlassian.com/cru/CR-9
>
> I added a new interface ClearableValueStack, which if implemented will
> make the OGNL parameter binding run in a clean context.
>
> musachy
>
> On Thu, Jul 17, 2008 at 5:46 PM, Musachy Barroso <mu...@gmail.com> wrote:
>> I think it would be the same, we would just need to add a method to
>> ValueStack, to clear the context.
>>
>> musachy
>>
>> On Thu, Jul 17, 2008 at 5:32 PM, Chris Pratt <th...@gmail.com> wrote:
>>> Will it be pluggable between the new-and-improved ValueStack and the
>>> OGNL ValueStack so that we can make the transition as painless as
>>> possible?
>>> (*Chris*)
>>>
>>> On Thu, Jul 17, 2008 at 2:28 PM, Musachy Barroso <mu...@gmail.com> wrote:
>>>> Yeah I am set to fix those security holes ;). Doing the change below,
>>>> all tests pass, with the exception of some tests in
>>>> ParameterInterceptorTest, that need to inject dependencies, and others
>>>> that check for the order of the values added to the stack (new context
>>>> is created here, so they fail)
>>>>
>>>> + ValueStack emptyStack = valueStackFactory.createValueStack(stack);
>>>> + Map<String, Object> context = emptyStack.getContext();
>>>> + ((OgnlContext)context).getValues().clear(); /// THIS IS BAD
>>>> + ReflectionContextState.setCreatingNullObjects(context, true);
>>>> + ReflectionContextState.setDenyMethodExecution(context, true);
>>>> + ReflectionContextState.setReportingConversionErrors(context, true);
>>>> +
>>>> for (Map.Entry<String, Object> entry :
>>>> acceptableParameters.entrySet()) {
>>>> String name = entry.getKey();
>>>> Object value = entry.getValue();
>>>> @@ -233,7 +265,7 @@
>>>> String name = entry.getKey();
>>>> Object value = entry.getValue();
>>>> try {
>>>> - stack.setValue(name, value);
>>>> + emptyStack.setValue(name, value);
>>>> } catch (RuntimeException e) {
>>>> if (devMode) {
>>>> String developerNotification =
>>>> LocalizedTextUtil.findText(ParametersInterceptor.class,
>>>> "devmode.notification", ActionContext.getContext().getLocale(),
>>>> "Developer Notification:\n{0}", new Object[]{
>>>> @@ -246,6 +278,9 @@
>>>> }
>>>> }
>>>> }
>>>> + stack.getContext().putAll(acceptableParameters);
>>>> +
>>>>
>>>> The 2 big things to be addressed are:
>>>>
>>>> 1. ((OgnlContext)context).getValues().clear();
>>>>
>>>> I cannot just do context.clear(), because that method not only removes
>>>> the values from the stack, but it clears the root, type converter and
>>>> other stuff, so we will have to add another "clear" method to the
>>>> OgnlContext, that just clears the values.
>>>>
>>>> 2. throwPropertyExceptions which needs to be the same in the new value
>>>> stack, but I think it is getting cleared.
>>>>
>>>> what do you guys think?
>>>>
>>>> musachy
>>>> --
>>>> "Hey you! Would you help me to carry the stone?" Pink Floyd
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
>>>> For additional commands, e-mail: dev-help@struts.apache.org
>>>>
>>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
>>> For additional commands, e-mail: dev-help@struts.apache.org
>>>
>>>
>>
>>
>>
>> --
>> "Hey you! Would you help me to carry the stone?" Pink Floyd
>>
>
>
>
> --
> "Hey you! Would you help me to carry the stone?" Pink Floyd
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: hacking OGNL and parameter binding
Posted by Musachy Barroso <mu...@gmail.com>.
I opened a code review here:
http://fisheye6.atlassian.com/cru/CR-9
I added a new interface ClearableValueStack, which if implemented will
make the OGNL parameter binding run in a clean context.
musachy
On Thu, Jul 17, 2008 at 5:46 PM, Musachy Barroso <mu...@gmail.com> wrote:
> I think it would be the same, we would just need to add a method to
> ValueStack, to clear the context.
>
> musachy
>
> On Thu, Jul 17, 2008 at 5:32 PM, Chris Pratt <th...@gmail.com> wrote:
>> Will it be pluggable between the new-and-improved ValueStack and the
>> OGNL ValueStack so that we can make the transition as painless as
>> possible?
>> (*Chris*)
>>
>> On Thu, Jul 17, 2008 at 2:28 PM, Musachy Barroso <mu...@gmail.com> wrote:
>>> Yeah I am set to fix those security holes ;). Doing the change below,
>>> all tests pass, with the exception of some tests in
>>> ParameterInterceptorTest, that need to inject dependencies, and others
>>> that check for the order of the values added to the stack (new context
>>> is created here, so they fail)
>>>
>>> + ValueStack emptyStack = valueStackFactory.createValueStack(stack);
>>> + Map<String, Object> context = emptyStack.getContext();
>>> + ((OgnlContext)context).getValues().clear(); /// THIS IS BAD
>>> + ReflectionContextState.setCreatingNullObjects(context, true);
>>> + ReflectionContextState.setDenyMethodExecution(context, true);
>>> + ReflectionContextState.setReportingConversionErrors(context, true);
>>> +
>>> for (Map.Entry<String, Object> entry :
>>> acceptableParameters.entrySet()) {
>>> String name = entry.getKey();
>>> Object value = entry.getValue();
>>> @@ -233,7 +265,7 @@
>>> String name = entry.getKey();
>>> Object value = entry.getValue();
>>> try {
>>> - stack.setValue(name, value);
>>> + emptyStack.setValue(name, value);
>>> } catch (RuntimeException e) {
>>> if (devMode) {
>>> String developerNotification =
>>> LocalizedTextUtil.findText(ParametersInterceptor.class,
>>> "devmode.notification", ActionContext.getContext().getLocale(),
>>> "Developer Notification:\n{0}", new Object[]{
>>> @@ -246,6 +278,9 @@
>>> }
>>> }
>>> }
>>> + stack.getContext().putAll(acceptableParameters);
>>> +
>>>
>>> The 2 big things to be addressed are:
>>>
>>> 1. ((OgnlContext)context).getValues().clear();
>>>
>>> I cannot just do context.clear(), because that method not only removes
>>> the values from the stack, but it clears the root, type converter and
>>> other stuff, so we will have to add another "clear" method to the
>>> OgnlContext, that just clears the values.
>>>
>>> 2. throwPropertyExceptions which needs to be the same in the new value
>>> stack, but I think it is getting cleared.
>>>
>>> what do you guys think?
>>>
>>> musachy
>>> --
>>> "Hey you! Would you help me to carry the stone?" Pink Floyd
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
>>> For additional commands, e-mail: dev-help@struts.apache.org
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
>> For additional commands, e-mail: dev-help@struts.apache.org
>>
>>
>
>
>
> --
> "Hey you! Would you help me to carry the stone?" Pink Floyd
>
--
"Hey you! Would you help me to carry the stone?" Pink Floyd
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: hacking OGNL and parameter binding
Posted by Musachy Barroso <mu...@gmail.com>.
I think it would be the same, we would just need to add a method to
ValueStack, to clear the context.
musachy
On Thu, Jul 17, 2008 at 5:32 PM, Chris Pratt <th...@gmail.com> wrote:
> Will it be pluggable between the new-and-improved ValueStack and the
> OGNL ValueStack so that we can make the transition as painless as
> possible?
> (*Chris*)
>
> On Thu, Jul 17, 2008 at 2:28 PM, Musachy Barroso <mu...@gmail.com> wrote:
>> Yeah I am set to fix those security holes ;). Doing the change below,
>> all tests pass, with the exception of some tests in
>> ParameterInterceptorTest, that need to inject dependencies, and others
>> that check for the order of the values added to the stack (new context
>> is created here, so they fail)
>>
>> + ValueStack emptyStack = valueStackFactory.createValueStack(stack);
>> + Map<String, Object> context = emptyStack.getContext();
>> + ((OgnlContext)context).getValues().clear(); /// THIS IS BAD
>> + ReflectionContextState.setCreatingNullObjects(context, true);
>> + ReflectionContextState.setDenyMethodExecution(context, true);
>> + ReflectionContextState.setReportingConversionErrors(context, true);
>> +
>> for (Map.Entry<String, Object> entry :
>> acceptableParameters.entrySet()) {
>> String name = entry.getKey();
>> Object value = entry.getValue();
>> @@ -233,7 +265,7 @@
>> String name = entry.getKey();
>> Object value = entry.getValue();
>> try {
>> - stack.setValue(name, value);
>> + emptyStack.setValue(name, value);
>> } catch (RuntimeException e) {
>> if (devMode) {
>> String developerNotification =
>> LocalizedTextUtil.findText(ParametersInterceptor.class,
>> "devmode.notification", ActionContext.getContext().getLocale(),
>> "Developer Notification:\n{0}", new Object[]{
>> @@ -246,6 +278,9 @@
>> }
>> }
>> }
>> + stack.getContext().putAll(acceptableParameters);
>> +
>>
>> The 2 big things to be addressed are:
>>
>> 1. ((OgnlContext)context).getValues().clear();
>>
>> I cannot just do context.clear(), because that method not only removes
>> the values from the stack, but it clears the root, type converter and
>> other stuff, so we will have to add another "clear" method to the
>> OgnlContext, that just clears the values.
>>
>> 2. throwPropertyExceptions which needs to be the same in the new value
>> stack, but I think it is getting cleared.
>>
>> what do you guys think?
>>
>> musachy
>> --
>> "Hey you! Would you help me to carry the stone?" Pink Floyd
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
>> For additional commands, e-mail: dev-help@struts.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
--
"Hey you! Would you help me to carry the stone?" Pink Floyd
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: hacking OGNL and parameter binding
Posted by Chris Pratt <th...@gmail.com>.
Will it be pluggable between the new-and-improved ValueStack and the
OGNL ValueStack so that we can make the transition as painless as
possible?
(*Chris*)
On Thu, Jul 17, 2008 at 2:28 PM, Musachy Barroso <mu...@gmail.com> wrote:
> Yeah I am set to fix those security holes ;). Doing the change below,
> all tests pass, with the exception of some tests in
> ParameterInterceptorTest, that need to inject dependencies, and others
> that check for the order of the values added to the stack (new context
> is created here, so they fail)
>
> + ValueStack emptyStack = valueStackFactory.createValueStack(stack);
> + Map<String, Object> context = emptyStack.getContext();
> + ((OgnlContext)context).getValues().clear(); /// THIS IS BAD
> + ReflectionContextState.setCreatingNullObjects(context, true);
> + ReflectionContextState.setDenyMethodExecution(context, true);
> + ReflectionContextState.setReportingConversionErrors(context, true);
> +
> for (Map.Entry<String, Object> entry :
> acceptableParameters.entrySet()) {
> String name = entry.getKey();
> Object value = entry.getValue();
> @@ -233,7 +265,7 @@
> String name = entry.getKey();
> Object value = entry.getValue();
> try {
> - stack.setValue(name, value);
> + emptyStack.setValue(name, value);
> } catch (RuntimeException e) {
> if (devMode) {
> String developerNotification =
> LocalizedTextUtil.findText(ParametersInterceptor.class,
> "devmode.notification", ActionContext.getContext().getLocale(),
> "Developer Notification:\n{0}", new Object[]{
> @@ -246,6 +278,9 @@
> }
> }
> }
> + stack.getContext().putAll(acceptableParameters);
> +
>
> The 2 big things to be addressed are:
>
> 1. ((OgnlContext)context).getValues().clear();
>
> I cannot just do context.clear(), because that method not only removes
> the values from the stack, but it clears the root, type converter and
> other stuff, so we will have to add another "clear" method to the
> OgnlContext, that just clears the values.
>
> 2. throwPropertyExceptions which needs to be the same in the new value
> stack, but I think it is getting cleared.
>
> what do you guys think?
>
> musachy
> --
> "Hey you! Would you help me to carry the stone?" Pink Floyd
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org