I'm not sure this solves the problem Rick... partially it does...
As Paul pointed out, the cancel function itself is a legitimate case.
In that situation, you wouldn't want the form to be populated (you
wouldn't care if it was or wasn't I suppose, but ideally why bother
doing the extra work?) and you wouldn't want validate() to be called
because it might reject the request when it shouldn't...
For example... say I have a 3-page wizard flow. After each page I stick
the entered values in session. On the third page I have the capability
of clicking cancel. When this occurs, I'm going to want to clear out
the stored values from session. Imagine that the last page *also* has
some entry fields that are usually validated, and let's say one of them
is required. If the user clicks cancel, whether you call validate from
the Action or not, the user is forced to enter a value just to get past
validation, even if they click cancel! The UI would appear broken to
the user, and rightly so.
Now, you may say that "yeah, but I would call isCancelled() before I do
the validation", and indeed, that would make the UI work as it should.
The problem though is that you will have to remember to do that check IN
EVERY ACTION, or risk having a similar UI issue elsewhere. It's just
extra code you have to copy-and-paste, or have your own base Action
class to extend from.
I agree, if everyone did as you suggest this issue wouldn't be as
severe. In fact, you would take care of the security issue (assuming
*every* Action called validate()), but you would trade it in for a
broken UI mechanism anytime the cancel button was involved. Not as big
a deal, but still not the best answer :)
Frank
Rick Reumann wrote:
All of this just adds *ONE MORE REASON* to my list of *NEVER EVER* use
validate="true". I always call validation manually from my Action class
and the sooner people get into a habit of this the way better off they
will be. If you validate or call the form's validate method manually
from your Action class all these problems go away including the problem
we get posted here on the list at least a couple times a week concerning
list items going out of scope when validation fails. Validating manually
also solves the problem Paul addresses as form validation ALWAYS takes
place.
I think the bug is sort of in validate='true' :) I think that should
just be dropped and when people need to validate they just call it from
their Action class.
http://www.learntechnology.net/validate-manually.do
The code is this simple to add:
ActionErrors errors = form.validate( mapping, request );
if ( errors != null && !errors.isEmpty ) {
saveErrors(request, errors);
setUp(request); //preps request for lists and stuff
return (mapping.findForward(UIconstants.VALIDATION_FAILURE));
}
You could even add that to a utility method so it's just one line of code.
Now you have either one line of code in your Action or one line of code
in your ActionMapping, yet the first scenario solves all these problems.
(side note, I don't even really like using the validator so I don't even
call form.validate() but call a validation method in my action class
that does my validation, but I know most like to use the validator so
I'm showing how easy it is to call manually).
Frank W. Zammetti wrote:
Rick Reumann wrote:
Maybe I'm missing how the above would happen. How would passing in
the canceled parameter end up getting them access to a table? Oh
wait, maybe this is with regular Actions with just an execute? It's
been so long since I used a non Dispatch Action I'm not aware of the
behavior. So I have an Action called "GetMeATable" with an execute
method that returns a table from the db, if I pass in through a get
parameter the canceled param it will still execute that action?
Right, ordinary Action we're talking about. All Struts does is put a
request attribute in place to indicate the Action is being canceled.
It then goes on to populate the form, but *NOT* call validate() on
it. So, the problem arises if your Action doesn't check for the
canceled attribute and validate() is meant to stop a given request (as
in my scenario where it was checking for one of the allowed table names).
As I Wrote this, I see that Paul responded and said it's a problem
with DispatchActions too, and I think he's right. In fact, his
response sums up my own pretty well :)
Even if the above is true, I'm not so sure that's a big deal as far
as Struts goes in regard to it being a 'security problem with
Struts'. You can always find a ton of security loop holes if you
don't truly check that the logged in user has access to some backend
procedure. For example if you have an update dispatch method or an
"UpdateAction" you could easily type in the url
someUrl?dispath=update&id=42&whatever=something etc and have it go
through and be processed.
Well, let me put it this way... if I worked for Microsoft, to me at
least, this would be a non-critical bug. Patch it on the regular
second Tuesday of the month cycle :) But I *do* think it is a bug (a
logic bug in this case) and *is* a security hole, even if arguably a
small one. Your absolutely right though, there are far more severe
security holes that a developer can open up without help from Struts
:) I don't think that should make anyone less interested in plugging
this one though.
Bottom line is if security is an issue, no one should ever rely on
the validate method. That's just silly. I'm sure the docs even state
that the purpose of the validate method is to just validate data
entry (required fields, no Strings in number fields, etc.).
But, how someone defines "data entry" can come into play... for
instance, in the scenario I described, selecting a table, from a
select say, would count as data entry, wouldn't it? Try this... let's
say we're talking about a *REALLY* bad developer and for some reason
he has a *TEXT* field for entering the table name, and then goes on to
check if it is an allowed value in validate(). Stupid design, in the
extreme! But an improper use of validate()? Probably not (debatable
at least).
Let me try and come up with a better example...
Let's say my bank's web site allows me to have a primary user account,
and I can link to this account all my checking accounts and savings
account to see them all in one "dashboard" display. I have to fill
out a form to have an account added. The form has account number on it.
Let's say one of the validations is to be sure the checksum of the
entered account number matches one that would only be valid for me.
It's a relatively simple math check, so it's done in validate()
because really we're checking that the entered value is "valid".
Now, the Action behind it calls some DAO to add the account for me.
But, it reasonably assumes that the data it gets in the form is valid.
So, if validate() is not executed, I could conceivably enter an
account number that isn't mine and have it attached to my user account
and see its details.
None of this sounds especially out of the ordinary to me (I work in
the financial industry), and I can certainly see where some developers
would do it exactly this way (assuming my mythical checksum thing were
real- I've never seen that though). I doubt too many people would say
it's a "wrong" approach (we could of course argue architecture, that's
always the case, but nothing stands out as patently "wrong" to me
about this design).
Yea there are plenty. I remember someone on the list here a while
back came up with this pretty cool scheme to mask dispatch action
method names. It was pretty cool even though I never needed it. If I
remember correctly, it acted sort of like the token mechanism - a
random string was created before going to a page and that string was
decoded on the server side into finding the proper dispatch method to
call. To me it's just much simpler to make sure you validate the user
logged in really can do what they are attempting to do somewhere on
the server side or with filters.
I think the problem here though is that this is a fairly subtle thing,
and something that could possibly be exploited in such a way that the
request looks legitimate, and in fact is, as far as the filters and
security checks go, but are not in terms of the logic of the application.
I don't want to blow this out of proportion... I think the way most
people architect their solutions, this probably wouldn't be a problem
for most people.
But, even if we all agreed that it wasn't a security problem per se,
it just logically does not make sense as far as I can see... if Struts
is going to populate the form, I can't think of a good reason it
wouldn't call validate() too, like any other request, just because the
action was canceled. Like I said, maybe someone can come up with a
reasonable explanation for that behavior, but I can't see it :)
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
--
Frank W. Zammetti
Founder and Chief Software Architect
Omnytex Technologies
http://www.omnytex.com
AIM: fzammetti
Yahoo: fzammetti
MSN: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
