struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Craig R. McClanahan" <craig...@apache.org>
Subject Re: Tying Up Loose Ends
Date Tue, 24 Dec 2002 03:08:48 GMT


On Mon, 23 Dec 2002, Martin Cooper wrote:

> Date: Mon, 23 Dec 2002 14:59:50 -0800 (PST)
> From: Martin Cooper <martinc@apache.org>
> Reply-To: Struts Developers List <struts-dev@jakarta.apache.org>
> To: Struts Developers List <struts-dev@jakarta.apache.org>
> Subject: Re: Tying Up Loose Ends
>
>
>
> On Mon, 23 Dec 2002, Craig R. McClanahan wrote:
>
> > Given a little time to work on Struts (finally), I'd like to tackle the
> > top two bugs (based on severity) in our remaining list:
> >
> > * 14669 -- reset() in DynaActionForm acts the same as in ActionForm
> >
> > * 14800 -- Fix initialization bug and size parameter to form-property
> >
> > What I propose to do for 14669 is based on the patch proposed by Peter
> > Pilgrim, but with the "loaded" property renamed to "cleared" -- for
> > consistency with the method name clear() -- and the "cleared" property
> > stuff being protected instead of public.  For general users of action
> > forms, I don't see why the cleared state would be relevant, although it
> > might to advanced DynaActionForm subclasses.  The clear() methods will, as
> > proposed, be public so that application logic can call them as needed.
> >
> > For 14800, I think we do need the "size" attribute to pre-initialize
> > arrays to a fixed size when the "initial" attribute is not present.  I'm
> > less persuaded by the "potential security hole" argument related to
> > copying the initial values for an array) because the only values that are
> > shared are the constants that were parsed from the XML configuration in
> > the first place.  For the vast majority of practical uses of this (Strings
> > and primitives) the underlying objects are immutable and modifications to
> > the property value in one form bean won't be propogated to other form
> > beans anyway.  But I'll certainly look into this more as I dive in to the
> > code.  (The placement of the actual logic for this will be updated to
> > reflect the changes made for 14669, and will be added as a separate
> > commit.)
>
> I agree that the "size" attribute is useful. I also think it would be
> useful to be able to initialise all of the elements of an array to an
> initial value - perhaps something like this:
>
>   <form-property name="foo"
>                  type="java.lang.String[]"
>                  size="10"
>                  initial="1"/>
>
> so that each element of the array is initialised to "1". This would be a
> bit less error-prone than listing out N entries in the "initial"
> attribute. However, it's really an enhancement (to an enhancement :), so
> we can leave it for later.
>

I'm not totally convinced about the utility of a repeating list, but agree
that it fits the "later" category.

> For the other part of the bug report, the concern seems to be that, if the
> specified type is something like a customer bean, which might have a
> credit card property, then the object created to hold the initial values
> would be the same object that is populated from one user's request, and
> that those changed values would also be used to populate another user's
> response, thus exposing one user's credit card number to another user.
>

OK, let's explore this particular scenario.  Assume we have a form bean
whose configuration looks like this:

  <form-bean name="customer"
             type="org.apache.struts.action.DynaActionForm">
    ...
    <form-property name="sensitive" type="java.lang.String[]"
                iniital="{secret-1, secret-2, secret-3}"/>
    ...
  </form-bean>

Now, when you call RequestUtils.createActionForm(), you get a
DynaActionForm instance of this class with an indexed property named
"sensitive".  The array will have three elements, with the values
"secret-1", "secret-2", and "secret-3".

Note, however, that the *array* holding these values is unique per form
bean instance (because of the cloning logic in the current
implementation).  So, what happens when an Action that sets up this form
bean before forwarding to an input page does something like this:

  ActionForm form = RequestUtils.createActionForm(...);
  form.setIndexedProperty(form, "sensitive", "New secret-2 Value", 1);

or, equivalently:

  ActionForm form = RequestUtils.createActionForm(...);
  ((DynaBean) form).set("sensitive", 1, "New secret-2 Value");

This modification will *only* affect *this* form bean instance, not all of
them, because Strings are immutable.  The net effect is that the array
will hold "secret-1", "New secret-2 Value", "secret-3" for this particular
instance, but no other instances will be affected.  (Hint:  this is why
the array cloning is done in the first place -- my original implementation
did not clone, and the arrays themselves were shared, which is bad).

> I haven't dug into the code sufficiently to determine whether or not this
> is a real problem, but if it is, I would have expected all sorts of other
> problems with DynaBeans to have surfaced before now, resulting from the
> same underlying cause.

I believe that the only scenario that can cause a problem would exhibit
all of the following:

* The element class for the indexed property is mutable.  This means
  that arrays of Strings or primitives (of any standard type) are immune
  to any problems, and that covers nearly every common case for form bean
  properties that I can think of.

* The application has registered a custom Converter that creates
  an array of objects of the appropriate type from the String-valued
  "initial" attribute.

* The application then modifies the values on a per-user or per-request
  basis (rather than using this technique to initialize some sort of
  readonly collection).

While the above conditions are technically possible, they don't seem like
the kind of thing that real applications would use the "initial" attribute
of a <form-property> element for.  Is there some common use case that I am
missing?

> Martin Cooper

Craig


--
To unsubscribe, e-mail:   <mailto:struts-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:struts-dev-help@jakarta.apache.org>


Mime
View raw message