struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Laurie Harper <>
Subject Re: Validation Security Hole?
Date Sun, 22 Jan 2006 23:59:55 GMT
Transplanted from the user list, w/ Frank's response [FYI, if you 
haven't seen the thread on the user list: this is a summary of 
discussion around the handling of 'cancelled' actions as a potential 
security hole...]

Frank W. Zammetti wrote:
> Excellent summary Laurie, thanks!  I think you captured all the 
> pertinent information clearly and concisely.  I just suggested to Paul 
> to open a ticket for this as well, that should probably be mentioned 
> (with a ticket # if available when you post).  Two minor comments below...
> Frank
> Laurie Harper wrote:
>> [Moved to a top-level thread, as this doesn't have anything to do with 
>> (either of) the thread(s) it was nested in! :-)]
> Was it nested?  I didn't even notice :)
>> I think this thread deserves discussion on the dev list, but before I 
>> move it over I thought I'd post a summary to make sure I've captured 
>> all the arguments. I've also added preliminary thoughts in how to 
>> resolve the issue at the end of this post, though that discussion 
>> definitely ought to proceed on the dev list I guess.
>> I'll re-post this message to the dev list later today if I haven't 
>> missed anything important below:
>> * Issue: addition of a 'org.apache.struts.action.CANCEL' parameter to 
>> any request will cause validation to be skipped, but the rest of the 
>> request processing / action invocation cycle to proceed normally
>> * Consequence: any action which proceeds assuming that validation has 
>> completed successfully and which doesn't explicitly check isCanceled() 
>> is proceeding on a broken assumption
>> * Questions:
>> - why doesn't Struts call validate() on a cancelled request?
>>     If a request is canceled it usually means validations don't
>>     apply since the implication is that any user input will be
>>     thrown away. Users shouldn't be required to supply valid
>>     inputs for actions they are canceling.
>> - why does Struts still call Action.execute() for a canceled request?
>>     Since you may still want to act on a canceled request (e.g.
>>     to clean up resources stored in the session). (Some of?) the
>>     DispactAction variants dispatch to a special method and aren't
>>     subject to the consequences listed above, but most action
>>     implementations don't.
>> - why does Struts still populate the action form on a cancelled request?
>>     If inputs are going to be thrown away anyway, why process
>>     them by populating the action form? [Commentary: I believe
>>     this behaviour makes sense since it preserves a standard
>>     way to access the request data, should you want to, regardless
>>     of whether the action was canceled. You could argue that
>>     either way...]
> I tend to agree with your commentary, even though I find it hard to 
> envision a situation where you'd want to get at the parameters. 
> Certainly better to be able to though.
>> Here's my first thoughts on possible approaches to addressing the 
>> problem, to kick off further discussion on the dev list:
>> - SAF1.2 and before: ? document, don't fix? add config req'm'ts on 
>> action mapping? Refer to discussion on user list for various options.
>> - SAF1.3+: make cancel processing a command which you have to include 
>> in your request processing chain, and perhaps disclude it by default? 
>> [I'm not familiar enough with how you deploy chains on a per-action 
>> basis to know if this is the right way to do it...]
> I think this would be affected by what is done with 1.2... if it is 
> modified to by default not allow Actions to be cancelable for instance, 
> I would think it would be better to replicate that change into 1.3, 
> which would likely entail changing the default chain.  Open for 
> discussion obviously :)
>> - WW2/SAF2: implement cancel processing as an interceptor and either 
>> disclude it from default stack or require an action to implement an 
>> interface declaring that cancel processing should happen?
>> L.
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail:
>> For additional commands, e-mail:

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message