Add the missing Javadoc for the Validator interface

[kwwall]

Most of the methods are missing detailed Javadoc for the parameters for the org.owasp.esapi.Validator interface, yet they are present for many of the implementation methods in org.owasp.esapi.reference.DefaultValidator.

This should be fixed. Generally the implementation classes (in this case, the DefaultValidator class) should just use the {@inheritDoc} construct and inherit documentation from their interfaces rather than providing their own unless they are imposing a change in the semantics or the programming contract (which they really should NOT be doing).

Mostly, I think we can just clean this up by moving most of the DefaultValidator Javadoc back up to the Validator interface.

[stevebosman-oc]

I can look into improving the javadoc for Validator/DefaultValidator if you like.

From a quick glance I suppose the two biggest issues are methods which say how they work rather than what they achieve, e.g. "Calls assertValidHTTPRequestParameterSet and returns true if no exceptions are thrown.", and the lack of documentation on the parameters.

A single method example of the level I think the javadoc should go to

    /**
     * Determine if {@code input} is a valid item in {@code list}.
     * This will be true if a call to {@link #getValidListItem(String, String, List)}
     * does not throw a {@link ValidationException}.
     *
     * @param context
     *        A descriptive name of the parameter that you are validating (e.g., LoginPage_UsernameField).
     *        This value is used by any logging or error handling that is done with respect to the value passed in.
     * @param input
     *        The value to search {@code list} for.
     * @param list
     *        The list to search for {@code input}.
     * @param errorList
     *        A mandatory {@link ValidationErrorList} instance.
     *        If a call to {@link #getValidListItem(String, String, List)} raises a {@link ValidationException}
     *        the exception will be added to this list.
     *
     * @throws IntrusionException Input is clearly an attack.
     */
    boolean isValidListItem(String context, String input, List<String> list, ValidationErrorList errorList) throws IntrusionException;

[stevebosman-oc]

It turns out there is a subtle difference in behaviour in the DefaultValidator between is methods which capture the validation exceptions in a ValidationErrorList and those that don't. Is methods capturing into an errorlist don't catch other exceptions like IntrusionException, but Is methods without the errorList swallow all exceptions and return false.

I'm not sure if this is intentional, but I will capture it in the javadoc

[planetlevel]

Hmm…. I would have thought this would work exactly the opposite way. The thinking is that if you’re capturing into an errorlist, then you probably have some logic to go through those errors and do something with them. In this situation, you most likely want to catch the IntrusionException and add it to the list of things to handle. If you’re not capturing an errorlist, then it might be useful to know the difference between a user error (return false) and an attack (throw exception).

…

--Jeff From: Steve Bosman ***@***.***> Date: Friday, May 20, 2022 at 6:23 AM To: ESAPI/esapi-java-legacy ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [ESAPI/esapi-java-legacy] Add the missing Javadoc for the Validator interface (Issue #674) It turns out there is a subtle difference in behaviour in the DefaultValidator between is methods which capture the validation exceptions in a ValidationErrorList and those that don't. Is methods capturing into an errorlist don't catch other exceptions like IntrusionException, but Is methods without the errorList swallow all exceptions and return false. I'm not sure if this is intentional, but I will capture it in the javadoc — Reply to this email directly, view it on GitHub<#674 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAUUFTFDTJUVIM6DSJSPXDDVK5RYXANCNFSM5R3VFWPQ>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[kwwall]

IIRC (and this is just off the top of my head), I thought the actual code referenced a GitHub issue that describes all the whys and wherefores of this, and it is the one I'm thinking of, we may have even added a new property in ESAPI.properties (whose name presently escapes me and I'm too lazy to look up) that reverses the behavior.

[kwwall]

IIRC (and this is just off the top of my head), I thought the actual code referenced a GitHub issue that describes all the whys and wherefores of this, and it is the one I'm thinking of, we may have even added a new property in ESAPI.properties (whose name presently escapes me and I'm too lazy to look up) that reverses the behavior.

My bad. I think I was thinking of issue #509 although that may impact this as it is in ValidationRule.

[kwwall]

Hmm…. I would have thought this would work exactly the opposite way. The thinking is that if you’re capturing into an errorlist, then you probably have some logic to go through those errors and do something with them. In this situation, you most likely want to catch the IntrusionException and add it to the list of things to handle. If you’re not capturing an errorlist, then it might be useful to know the difference between a user error (return false) and an attack (throw exception).

Indeed @planetlevel, I now see what you mean. I was thinking he was referring to #509. But the dilemma now given that the behavior is actually counter-intuitive (and therefore on some level at least, could be considered incorrect), we have just officially documented it as "That's not a bug, that's a feature". :) Of course, given this is a library, the only way to "fix" this without breaking ESAPI client code is to introduce a new ESAPI property that allows clients to change the behavior and change it so that it is the intuitive (and correct??) behavior. Sigh. @xeno6696 and @jeremiahjstacey - what do you think about all this?

[kwwall]

@stevebosman-oc - Steve, special thanks again for doing this. It is really helpful.