Skip to content

Issue-452 : Add support for annotation based input validation - #903

Open
aabashkin wants to merge 14 commits into
ESAPI:developfrom
aabashkin:issue-452-signed
Open

Issue-452 : Add support for annotation based input validation#903
aabashkin wants to merge 14 commits into
ESAPI:developfrom
aabashkin:issue-452-signed

Conversation

@aabashkin

Copy link
Copy Markdown

@kwwall please review at your convenience, thanks.

CopilotAI review requested due to automatic review settings January 5, 2026 21:44

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements annotation-based input validation support for ESAPI using the Java Bean Validation (JSR 380) API. It introduces 17 custom validation annotations that wrap existing ESAPI validation methods, enabling declarative validation of fields, parameters, and method return values.

Key changes:

  • Created 17 custom validation annotations (ValidString, ValidCreditCard, ValidURI, etc.) with corresponding validator implementations
  • Added ValidationUtil helper class for centralized error handling
  • Comprehensive test suite covering all validation annotations
  • Added javax.validation-api (2.0.1.Final) as a compile dependency and Hibernate Validator (6.2.5.Final) as a test dependency

Reviewed changes

Copilot reviewed 36 out of 37 changed files in this pull request and generated 23 comments.

Show a summary per file
FileDescription
ValidationAnnotationsTest.javaComprehensive test suite covering all 17 validation annotations with positive and negative test cases
ValidationUtil.javaUtility class for converting ValidationErrorList to constraint violations
ValidURIValidator.java / ValidURI.javaURI validation annotation and validator
ValidStringValidator.java / ValidString.javaString validation annotation and validator with type and length constraints
ValidSafeHTMLValidator.java / ValidSafeHTML.javaSafe HTML validation annotation and validator
ValidRedirectLocationValidator.java / ValidRedirectLocation.javaRedirect location validation annotation and validator
ValidPrintableValidator.java / ValidPrintableStringValidator.java / ValidPrintable.javaPrintable character validation for char arrays and strings
ValidNumberValidator.java / ValidNumber.javaNumber validation annotation and validator with range constraints
ValidListItemValidator.java / ValidListItem.javaList item validation annotation and validator
ValidIntegerValidator.java / ValidInteger.javaInteger validation annotation and validator with range constraints
ValidHTTPRequestParameterSetValidator.java / ValidHTTPRequestParameterSet.javaHTTP request parameter set validation
ValidFileUploadValidator.java / ValidFileUpload.javaFile upload validation with size and location constraints
ValidFileNameValidator.java / ValidFileName.javaFile name validation with extension constraints
ValidFileContentValidator.java / ValidFileContent.javaFile content validation with size constraints
ValidDoubleValidator.java / ValidDouble.javaDouble validation annotation and validator with range constraints
ValidDirectoryPathValidator.java / ValidDirectoryPath.javaDirectory path validation with parent directory constraints
ValidDateValidator.java / ValidDate.javaDate validation with format and locale support
ValidCreditCardValidator.java / ValidCreditCard.javaCredit card validation using Luhn algorithm
DefaultValidator.javaMinor formatting change (trailing whitespace)
pom.xmlAdded javax.validation-api, hibernate-validator, and javax.el dependencies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadsrc/main/java/org/owasp/esapi/reference/DefaultValidator.java Outdated
Comment on lines +29 to +31
if (input == null) {
return true;
}

CopilotAIJan 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check returns true even when allowNull is false. This means null values will always pass validation regardless of the allowNull setting. The validator should return allowNull when input is null to properly respect the allowNull configuration.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make a design decision here:

  1. null values are accepted unless @NotNull is present. Since the user would already be using annotations, it could make sense to assume that they would leverage an existing, often used constraint.
    OR
  2. Require allowNull everywhere

I was leaning towards 1 initially but I'm starting to consider 2 more seriously. Thoughts?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwwall ping

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aabashkin - So for the delay; my wife has been having medical problems (still is, in fact), so I've had to prioritize that.

I have been looking at this though a bit though and thinking about it deeply. The potential problem that I foresee with approach 1 is that, what happens if someone uses the @NotNull annotation, but then also specifies allowNull as true? How do we resolve that case? I'm not can be resolved at compile-time, but even at runtime, it seems as though may some RuntimeException or perhaps even some sort of custom subclass of java.lang.Error should be thrown.

The other way might be eliminate allowNull as an explicit parameter in all these annotations where it's used and simply treat it as true unless @NotNull is present, but I'm not sure how that would work because I'm not sure if annotation order matters or not. I would think it would, and in that case, if someone did something like this:

@NotNull@ValidNumber(context = "num", minValue = 1, maxValue = 10000)
StringqueryParmNum;

versus an ordering like this:

@ValidNumber(context = "num", minValue = 1, maxValue = 10000)
@NotNullStringqueryParmNum;

my intuition tells me that that difference may be significant from the way code gets generated,

Somewhat related to all of this is the testing. For example, a quick perusal of src/test/java/org/owasp/esapi/reference/validation/annotations/ValidationAnnotationsTest.java seems to indicate that only "sunny-day" tests are provided. In ESAPI, we need to test failure cases as well...ideally all of them, but at least enough to get code-coverage of 70%. Yes, there are some older, rarely used components that don't have anywhere near that. I hope to address that by getting genAI to create some additional test cases, but that's probably still 5-6 months out. But I have no objection to you using genAI with something like Copilot or Claude Code, etc. as long as the prompts are saved and included in GitHub somewhere and the generated code is maintainable by a human.

Lastly, the Javadoc documentation needs some enhancing. I haven't looked through it in detail yet, but I did notice the total absence of package.html in all your new packages. (We don't need to have that for src/test stuff, but do want it for src/main.)

I will try to look at this more closely in the following weeks, but as I mention, family comes first, so no promise of how soon. Thanks for your understanding in this matter.

@Override
public boolean isValid(String input, ConstraintValidatorContext constraintValidatorContext) {
if (input == null) {
return true;

CopilotAIJan 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check returns true even when allowNull is false. This means null values will always pass validation regardless of the allowNull setting. The validator should return allowNull when input is null to properly respect the allowNull configuration.

Suggested change
returntrue;
returnallowNull;

Copilot uses AI. Check for mistakes.

public class ValidationUtil {

private ValidationUtil(){}

CopilotAIJan 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after closing brace. The constructor should have a space before the opening brace for consistency with Java formatting conventions.

Copilot uses AI. Check for mistakes.
Comment on lines +23 to +25
if (input == null) {
return true;
}

CopilotAIJan 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check returns true even when allowNull is false. This means null values will always pass validation regardless of the allowNull setting. The validator should return allowNull when input is null to properly respect the allowNull configuration.

Copilot uses AI. Check for mistakes.
@Override
public boolean isValid(String input, ConstraintValidatorContext constraintValidatorContext) {
if (input == null) {
return true;

CopilotAIJan 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check returns true even when allowNull is false. This means null values will always pass validation regardless of the allowNull setting. The validator should return allowNull when input is null to properly respect the allowNull configuration.

Suggested change
returntrue;
returnallowNull;

Copilot uses AI. Check for mistakes.
@Override
public boolean isValid(String input, ConstraintValidatorContext constraintValidatorContext) {
if (input == null) {
return true;

CopilotAIJan 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check returns true even when allowNull is false. This means null values will always pass validation regardless of the allowNull setting. The validator should return allowNull when input is null to properly respect the allowNull configuration.

Suggested change
returntrue;
returnallowNull;

Copilot uses AI. Check for mistakes.
@Override
public boolean isValid(String input, ConstraintValidatorContext constraintValidatorContext) {
if (input == null) {
return true;

CopilotAIJan 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check returns true even when allowNull is false. This means null values will always pass validation regardless of the allowNull setting. The validator should return allowNull when input is null to properly respect the allowNull configuration.

Suggested change
returntrue;
returnallowNull;

Copilot uses AI. Check for mistakes.
@Override
public boolean isValid(String input, ConstraintValidatorContext constraintValidatorContext) {
if (input == null) {
return true;

CopilotAIJan 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check returns true even when allowNull is false. This means null values will always pass validation regardless of the allowNull setting. The validator should return allowNull when input is null to properly respect the allowNull configuration.

Suggested change
returntrue;
returnallowNull;

Copilot uses AI. Check for mistakes.
@Override
public boolean isValid(String input, ConstraintValidatorContext constraintValidatorContext) {
if (input == null) {
return true;

CopilotAIJan 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check returns true even when allowNull is false. This means null values will always pass validation regardless of the allowNull setting. The validator should return allowNull when input is null to properly respect the allowNull configuration.

Suggested change
returntrue;
returnallowNull;

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@aabashkin@kwwall