To answer this, I need to rearrange your reply just a little :) ...
On Mon, January 23, 2006 4:20 pm, Joe Germuska said:
> The fact that you can bypass validation and still have your action
> execute is a "feature" not a "bug", at least in so far as at some
> time, someone saw a need for it, and documented clearly the behavior.
> If someone were using this feature, one would assume that they had
> coded for the possibility of the action being cancelled and the
> action being presented with a non-validated form.
Yes, I've been convinced of this point over the past 24 hours or so... at
first I did see it as a bug, but Paul himself convinced me it really
isn't, it's just a feature with an unintended consequence. The actual
cancel case is the justification for it... populating the form is OK, just
in case the Action needs it (it probably won't, but no harm). And
validating you definitely wouldn't want to happen in that case, lest the
user have to enter valid data just to cancel. And in such a truly
cancelable Action, it is the developers' responsibility to check
isCancelled() first.
It's that case of Actions that aren't intended to be cancelable where this
breaks down, at least right now.
> The problem is that a user can pass a well-known request parameter to
> any request and disable validation.
There's my rearrangement :) I think this section only makes sense after
the paragraph above...
This is the part I don't think I agree with... it isn't simply a matter of
it being a well-known parameter that triggers this behavior but is the
fact that there *is* a parameter that will trigger it when not intended.
Obscuring the trigger doesn't remove the hole, it just makes it harder to
find. But...
> The proposed solution requires that the person intentionally using
> this feature make one further indication in the struts-config file,
> reinforcing the logical contract that says "it is ok to invoke this
> action with an invalid form even though I have set "validate='true'".
> The key point here is that you are explicitly turning on attention to
> the "cancel" parameter for any action (path) that accepts it, leaving
> all other actions invulnerable to the "attack".
This is where I'm not understanding... are you saying that if the
arbitrary mapping parameter MAPPING_PARAMETER is *not* present, then
validate would *never* be bypassed, or are you saying things would
function as they do today except that you could name the request parameter
to interrogate per-mapping? If it's the former then I get it and think
that would work, if it's the later then I'm still missing something :)
(and if it's something else entirely, that speaks for itself! LOL)
I do think this is a case where backwards-compatibility shouldn't be a
primary concern by the way... we're talking about closing a potential
security hole, however big one deems it to be, and if that requires an
incompatible solution, I think that's acceptable. It's not an enhancement
after all, not primarily at least.
> I certainly don't think it's a very elegant solution, but it seems to
> be relatively low impact. I could probably brainstorm some more
> elegant approaches, but in the absence of champions for the basic
> functionality, I would save that for a time when there was at least
> beer, which alas, is not now.
I think the other suggested solution may be less difficult even... add a
wasCancelled() method to Action. When the cancelled parameter comes in,
populate the form, skip calling validate() and call wasCancelled() instead
of execute(). The default implementation would render a simple page
saying "wasCancelled() not implemented for mapping", write a message to
the log indicating the requested path, and return null.
This should deal with all variants of Action... it would also close the
hole in such a way that isn't massively disruptive for anyone not using
cancel functionality already. Lastly, it should be a relatively minor
change for those that are, because their Actions probably look something
like this:
if (isCancelled()) {
// do something
} else {
// main Action work
}
So, all they need to do is remove the positive branch there, move is to
wasCancelled(), and remove the rest of the if block.
Anyone else see that as a reasonable solution? Is there a flaw in it I'm
missing?
> Joe
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
