Date validation rule 299 (#468)

* DateValidationRule Logic Updates

Updating the parsing logic of the DateValidationRule to not only verify a
date can be constructed from the input string in a known format, but also
that the generated Date can be reformatted back to the same String
content.  This adds additional security for cases where the DateFormatter
accepts characters postfixed to an otherwise valid date.

As a result two tests in ValidationTest.java had to be updated to use the
correct explicit format.  The data sets were using full-name month, day,
year content, but supplying the MEDIUM format. Updating the provided
format to LONG allowed the new check to function as desired.

* DateValidationRule Test Content

Adding test cases presented in the github issue

* Dependency Cleanup: JodaTime

Removing JodaTime dependency from Pom.  It had been introduced to
potentially address date/time parsing issues, but had not yet been applied
to the baseline.

* DateValidationRule Test Content

Updating test to actually use assertions in execution so that the intended
state is verified.

* DateValidationRule Test Content

Adding additional test case content.

* Validation Test Cleanup

Converting ValidationErrorList Test to use Junit4 syntax.

* Validation Test Cleanup

Updating assertions used in ValidationErrorList for more information in
Junit test failure cases.

* Validation Test Cleanup

Breaking the exception cases of the ValidationErrorListTest into separate
test blocks.  Updated to use the ExpectedException Rule.

Corrected wording in implementation error message on null context.
Corrected message in implementation on null ValidationException reference.

* Validation Test Cleanup

Test updates to use class-scope references rather than on a per-test
basis.

Removed method to create ValidationException.  Catch block indicated that
a specific exception had been thrown at one point.  I checked the call
heirarchy and verified that exception is not being explicitly thrown at
this time.

* Validation Test Cleanup

ValidationErrorListTest:
  Removed sysouts
  Cleaned unused imports
  Formatted file (removed tab chars & adjusted indent)

* Validation Test Cleanup BaseValidationRuleTest

Converting test to Junit4 Syntax.

* Validation Test Cleanup: BaseValidationRuleTest

Commenting out the current test.  Upon inspection, the test is not fulfilling the commented intent.  Instead of validating that the BaseValidationRule throws exceptions, it is only verifying that the test-scope stub implementation throws. Going to keep it around so I can verify I'm fulfilling the intent in the new implementation content before deleting that code

* Validation Test Cleanup: BaseValidationRuleTest

Re-implementing the tests for BaseValidationRule.

** This commit contains a failing test (testWhitelistSetExtendedCharacterSets)**
I do not know how to test utf-16 character sets, but I think it is something that needs to be done.

I have listed a concern with potential side-effects of using ValidationErrorList in the BaseValidationRule.  Refer to testGetValidMultipleExceptionSameContextThrowsRuntimeException for more information.

* Validation Test Cleanup: DateValidationRuleTest

Initial basic tests for general class functionality.

* Validation Test Cleanup: DateValidationRuleTest

Adding validations to ensure that DateFormat leniency is being set in accordance with the SecurityConfiguration ACCEPT_LENIENT_DATES value.

Still light in the context that I'm not verifying the value is derived from the SecurityConfiguration, but I am at least checking that the date format is being updated and that the value being applied matches the test-scope settings.

* DateValidationRule Logic & Test Updates

Updating the implementation so the process of generating a String from the parsed date and comparing it with the canonicalized input only happens during the sanitize workflow.  This will allow for the DateFormat contract for leniency and ESAPI's contract for security to remain intact (I think).

In essence, getValid is asking if the configured DateFormat can parse any date out of String being provided. That's completely on the DateFormat instance. If it works then return it, if it doesn't the throw the ValidationException.

On the other hand, sanitize is asking for a safe representation of the String. For that we can add the additional cross check and be more stringent in the criteria. Meaning if the same String that was used to generate the Date cannot be recreated from that date then we don't trust the input.

Tests were updated to reflect this workflow, and the data set used to check 299 in the ValidatorTest has been integrated into DateValidationRuleTest.

* Validation Test Updates: BaseValidationRuleTest

Ignoring the explictly fail case in the test.

* DateValidation Corrections for DefaultValidator

Adding another sanitize method to the DateValidationRule which accepts the ValidationErrorList in addition to the other parameters.

Updating DefaultValidator to use the new sanitize method and check the error list to verify the returned date before passing it along to the client.  If there are no errors, we're gtg; otherwise, the ValidationError at the first index is rethrown.

* DateValidationTest API Extension

Adding tests for new sanitize API method.

* DefaultValidator Date Validation Logic Update

Altering method chaining to defer to the getValidDate which uses the ValidationErrorList which now contains the delegation to the DateValidationRule.

* DefaultValidator Date API Testing

Creating a new test class that focuses on testing the DATE API of
DefaultValidator.

This is a wireframe commit.

* DefaultValidator Date API Testing

Successful interception of the DateValidatorRule construction. When
DefaultValidator instantiates a new instance internally, it receives the
test-scope spy.

This will allow us to check that the intended workflow of how the
DefaultValidator delegates to the DateValidatorRule is operating as
expected without needing to re-test the conditions already verified in the
DateValidatorRuleTest.

* DefaultValidator Logic Update & Date API Tests

Working tests to verify happy-path workflow through DefaultValidator Date
API.

Logic update to isValidDate with ValidationErrorList to use the
ValidationErrorList that is passed in with the delegated
DateValidationRule.sanitize call.

* DefaultValidator Date API Negative Tests

Verifying workflow and response from the DateAPI when the
DateValidationRule would result in a ValidationException being thrown.

* DefaultValidator Date API Test Cleanup

No functional changes. Mostly updating static imports and references.

* DefaultValidator cleanup

Fixing inconsistent indentation in updated method block.

* ValidatorTest Cleanup :: New ESAPI API Test

Created a new test for the static ESAPI class which verifies the workflow by which the runtime Validator is derived.

Removed the date validity tests from the ValidatorTest.  All pertinent content has been divided into the BaseValidationRuleTest, DateValidationRuleTest, ValidationErrorListTest, DefaultValidatorDateAPITest, and ESAPIContractAPITests.

The specific conditions have been shifted to be exclusively against the implementation (BaseValidationRuleTest). It is shown that the DefaultValidator uses BaseValidationRule in all date-related checks (DefaultValidatorDateAPITest).  And we know that if the user configures the ESAPI validator property to the DefaultValidator it will resolve at runtime (ESAPIContractAPITests).

* DateValdationRuleTest Additions

Adding Powermock tests that show the lenient setting is derived from the
SecurityConfiguration and is applied to the DateFormat reference both when
a DateValidationRule is constructed, and when the setDateFormat method is
invoked.

I chose to implement this as a unique test since PowermockRunner impacts my ability to see code coverage.

* BaseValidationRuleTest UTF-16 Whitelist Addition

Replacing ignore/fail of test impl with content showing that extended
character sets can be whitelisted through the implementation.

Author: jeremiahjstacey

pom.xml

@@ -129,12 +129,6 @@
     </properties>
 
     <dependencies>
-        <!-- https://mvnrepository.com/artifact/joda-time/joda-time -->
-        <dependency>
-            <groupId>joda-time</groupId>
-            <artifactId>joda-time</artifactId>
-            <version>2.10</version>
-        </dependency>
         <dependency>
             <groupId>commons-configuration</groupId>
             <artifactId>commons-configuration</artifactId>

src/main/java/org/owasp/esapi/ValidationErrorList.java

@@ -91,8 +91,8 @@ public class ValidationErrorList {
 	 * @param vex	A {@code ValidationException}.
 	 */
 	public void addError(String context, ValidationException vex) {
-		if ( context == null ) throw new RuntimeException( "Context for cannot be null: " + vex.getLogMessage(), vex );
-		if ( vex == null ) throw new RuntimeException( "Context (" + context + ") cannot be null" );
+		if ( context == null ) throw new RuntimeException( "Context cannot be null: " + vex.getLogMessage(), vex );
+		if ( vex == null ) throw new RuntimeException( "ValidationException cannot be null for context  (" + context + ")" );
 		if (getError(context) != null) throw new RuntimeException("Context (" + context + ") already exists, must be unique");
 		errorList.put(context, vex);
 	}

src/main/java/org/owasp/esapi/reference/DefaultValidator.java

@@ -277,35 +277,38 @@ public boolean isValidDate(String context, String input, DateFormat format, bool
 	 * {@inheritDoc}
 	 */
 	public boolean isValidDate(String context, String input, DateFormat format, boolean allowNull, ValidationErrorList errors) throws IntrusionException {
-		try {
-			getValidDate( context, input, format, allowNull);
-			return true;
-		} catch( ValidationException e ) {
-            errors.addError(context, e);
-			return false;
-		}
+	    getValidDate( context, input, format, allowNull, errors);
+	    return errors.isEmpty();
 	}
 
 	/**
 	 * {@inheritDoc}
 	 */
 	public Date getValidDate(String context, String input, DateFormat format, boolean allowNull) throws ValidationException, IntrusionException {
-		DateValidationRule dvr = new DateValidationRule( "SimpleDate", encoder, format);
-		dvr.setAllowNull(allowNull);
-		return dvr.getValid(context, input);
+		
+		ValidationErrorList vel = new ValidationErrorList();
+		Date validDate =  getValidDate(context, input, format, allowNull, vel);
+		
+		if (vel.isEmpty()) {
+		    return validDate;
+		}
+		
+		throw vel.errors().get(0);
 	}
 
 	/**
 	 * {@inheritDoc}
 	 */
 	public Date getValidDate(String context, String input, DateFormat format, boolean allowNull, ValidationErrorList errors) throws IntrusionException {
-		try {
-			return getValidDate(context, input, format, allowNull);
-		} catch (ValidationException e) {
-			errors.addError(context, e);
-		}
-		// error has been added to list, so return null
-		return null;
+	    Date safeDate = null;
+	    DateValidationRule dvr = new DateValidationRule( "SimpleDate", encoder, format);
+	    dvr.setAllowNull(allowNull);
+	    safeDate = dvr.sanitize(context, input, errors);
+	    if (!errors.isEmpty()) {
+	        safeDate = null;
+	    }
+	    // error has been added to list, so return null
+	    return safeDate;
 	}
 
 	/**

src/main/java/org/owasp/esapi/reference/validation/DateValidationRule.java

@@ -21,6 +21,7 @@
 import org.owasp.esapi.ESAPI;
 import org.owasp.esapi.Encoder;
 import org.owasp.esapi.StringUtilities;
+import org.owasp.esapi.ValidationErrorList;
 import org.owasp.esapi.errors.ValidationException;
 
 /**
@@ -60,7 +61,7 @@ public final void setDateFormat( DateFormat newFormat ) {
      * {@inheritDoc}
      */
 	public Date getValid( String context, String input ) throws ValidationException {
-		return safelyParse(context, input);
+		return safelyParse(context, input, false);
 	}
 
     /**
@@ -70,16 +71,25 @@ public Date getValid( String context, String input ) throws ValidationException
      */
 	@Override
 	public Date sanitize( String context, String input )  {
-		Date date = new Date(0);
-		try {
-			date = safelyParse(context, input);
-		} catch (ValidationException e) {
-			// do nothing
-	    }
-		return date;
+		return sanitize(context, input, null);
 	}
+	
+	/**
+     * {@inheritDoc}
+     */
+    public Date sanitize( String context, String input, ValidationErrorList errorList )  {
+        Date date = new Date(0);
+        try {
+            date = safelyParse(context, input, true);
+        } catch (ValidationException e) {
+            if (errorList != null) {
+                errorList.addError(context, e);
+            }
+        }
+        return date;
+    }
 
-	private Date safelyParse(String context, String input)
+	private Date safelyParse(String context, String input, boolean sanitize)
 			throws ValidationException {
 		// CHECKME should this allow empty Strings? "   " use IsBlank instead?
 		if (StringUtilities.isEmpty(input)) {
@@ -91,10 +101,16 @@ private Date safelyParse(String context, String input)
 							+ input, context);
 		}
 
-	    String canonical = encoder.canonicalize(input);
-
-		try {
-			return format.parse(canonical);
+		 String canonical = encoder.canonicalize(input);
+	        try {
+	            Date rval = format.parse(canonical);
+	            if (sanitize) {
+	                String cycled = format.format(rval);
+	                if (!cycled.equals(canonical)) {
+	                    throw new Exception("Parameter date is not a clean translation between String and Date contexts.  Check input for additional characters");
+	                }
+	            }
+	            return rval;
 		} catch (Exception e) {
 			throw new ValidationException(context
 					+ ": Invalid date must follow the "

src/test/java/org/owasp/esapi/ESAPIContractAPITest.java

@@ -0,0 +1,52 @@
+package org.owasp.esapi;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.internal.verification.VerificationModeFactory;
+import org.owasp.esapi.util.ObjFactory;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({ObjFactory.class})
+public class ESAPIContractAPITest {
+
+    @Mock
+    private SecurityConfiguration mockSecConfig;
+    
+    @Mock
+    private Validator mockValidator;
+    
+    @Before
+    public void configureStaticContexts() throws Exception {
+        PowerMockito.mockStatic(ObjFactory.class);
+        PowerMockito.when(ObjFactory.class, "make", ArgumentMatchers.anyString(), ArgumentMatchers.eq("SecurityConfiguration")).thenReturn(mockSecConfig);
+        PowerMockito.when(ObjFactory.class, "make", ArgumentMatchers.eq("MOCK_TEST_VALIDATOR"), ArgumentMatchers.eq("Validator")).thenReturn(mockValidator);
+        
+        PowerMockito.when(mockSecConfig.getValidationImplementation()).thenReturn("MOCK_TEST_VALIDATOR");
+    }
+    
+    @Test
+    public void testValidatorFromConfiguration() {
+        Validator validator = ESAPI.validator();
+        Assert.assertEquals("ESAPI Configuration should return Validator as specified by the SecurityConfiguration", mockValidator, validator);
+        
+        PowerMockito.verifyStatic(ObjFactory.class, VerificationModeFactory.times(1));
+        ObjFactory.make(ArgumentMatchers.anyString(), ArgumentMatchers.eq("SecurityConfiguration"));
+        
+        PowerMockito.verifyStatic(ObjFactory.class, VerificationModeFactory.times(1));
+        ObjFactory.make(ArgumentMatchers.eq("MOCK_TEST_VALIDATOR"), ArgumentMatchers.eq("Validator"));
+        
+        PowerMockito.verifyNoMoreInteractions(ObjFactory.class);
+        
+        Mockito.verify(mockSecConfig, Mockito.times(1)).getValidationImplementation();
+    }
+   
+}

src/test/java/org/owasp/esapi/ValidationErrorListTest.java

@@ -15,126 +15,76 @@
  */
 package org.owasp.esapi;
 
-import junit.framework.Test;
-import junit.framework.TestCase;
-import junit.framework.TestSuite;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
-import org.owasp.esapi.errors.IntrusionException;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TestName;
 import org.owasp.esapi.errors.ValidationException;
 
+
 /**
  * @author Jeff Williams ([email protected])
  */
-public class ValidationErrorListTest extends TestCase {
-
-	/**
-	 * Instantiates a new executor test.
-	 * 
-	 * @param testName
-	 *            the test name
-	 */
-	public ValidationErrorListTest(String testName) {
-		super(testName);
-	}
-
-	/**
-     * {@inheritDoc}
-     *
-     * @throws Exception
-     */
-	protected void setUp() throws Exception {
-		// none
-	}
+public class ValidationErrorListTest {
+    @Rule
+    public ExpectedException exEx = ExpectedException.none();
+    @Rule
+    public TestName testName = new TestName();
 
-	/**
-     * {@inheritDoc}
-     *
-     * @throws Exception
-     */
-	protected void tearDown() throws Exception {
-		// none
-	}
+    ValidationErrorList vel = new ValidationErrorList();
+    ValidationException vex = new ValidationException(testName.getMethodName(), testName.getMethodName());
+    @Test
+    public void testAddErrorNullContextThrows() {
+        exEx.expect(RuntimeException.class);
+        exEx.expectMessage("Context cannot be null");
+        vel.addError(null, vex);
+    }
 
-	/**
-	 * Suite.
-	 * 
-	 * @return the test
-	 */
-	public static Test suite() {
-		TestSuite suite = new TestSuite(ValidationErrorListTest.class);
-		return suite;
-	}
-	
-	public void testAddError() throws Exception {
-		System.out.println("testAddError");
-		ValidationErrorList vel = new ValidationErrorList();
-		ValidationException vex = createValidationException();
-		vel.addError("context", vex );
-		try {
-			vel.addError(null, vex );
-			fail();
-		} catch( Exception e ) {
-			// expected
-		}
-		try {
-			vel.addError("context1", null );
-			fail();
-		} catch( Exception e ) {
-			// expected
-		}
-		try {
-			vel.addError("context", vex );  // add the same context again
-			fail();
-		} catch( Exception e ) {
-			// expected
-		}
-	}
+    @Test
+    public void testAddErrorNullExceptionThrows() {
+        exEx.expect(RuntimeException.class);
+        exEx.expectMessage("ValidationException cannot be null");
+        vel.addError(testName.getMethodName(), null);
+    }
+    public void testAddErrorDuplicateContextThrows() {
+        exEx.expect(RuntimeException.class);
+        exEx.expectMessage("already exists, must be unique");
+        vel.addError(testName.getMethodName(), vex);
+        vel.addError(testName.getMethodName(), vex);
+    }
 
-	public void testErrors() throws Exception {
-		System.out.println("testErrors");
-		ValidationErrorList vel = new ValidationErrorList();
-		ValidationException vex = createValidationException();
-		vel.addError("context",  vex );
-		assertTrue( vel.errors().get(0) == vex );
-	}
+    @Test
+    public void testErrors() throws Exception {
+        vel.addError("context",  vex );
+        assertTrue("Validation Errors List should contain the added ValidationException Reference",vel.errors().contains( vex) );
+    }
 
-	public void testGetError() throws Exception {
-		System.out.println("testGetError");
-		ValidationErrorList vel = new ValidationErrorList();
-		ValidationException vex = createValidationException();
-		vel.addError("context",  vex );
-		assertTrue( vel.getError( "context" ) == vex );
-		assertTrue( vel.getError( "ridiculous" ) == null );
-	}
+    @Test
+    public void testGetError() throws Exception {
+        vel.addError("context",  vex );
+        assertTrue( vel.getError( "context" ) == vex );
+        assertNull( vel.getError( "ridiculous" ) );
+    }
 
-	public void testIsEmpty() throws Exception {
-		System.out.println("testIsEmpty");
-		ValidationErrorList vel = new ValidationErrorList();
-		assertTrue( vel.isEmpty() );
-		ValidationException vex = createValidationException();
-		vel.addError("context",  vex );
-		assertFalse( vel.isEmpty() );
-	}
+    @Test
+    public void testIsEmpty() throws Exception {
+        assertTrue( vel.isEmpty() );
+        vel.addError("context",  vex );
+        assertFalse( vel.isEmpty() );
+    }
 
-	public void testSize() throws Exception {
-		System.out.println("testSize");
-		ValidationErrorList vel = new ValidationErrorList();
-		assertTrue( vel.size() == 0 );
-		ValidationException vex = createValidationException();
-		vel.addError("context",  vex );
-		assertTrue( vel.size() == 1 );
-	}
+    @Test
+    public void testSize() throws Exception {
+        assertEquals(0, vel.size() );
+        vel.addError("context",  vex );
+        assertEquals(1, vel.size());
+    }
 
-	private ValidationException createValidationException() {
-		ValidationException vex = null;
-		try {
-			vex = new ValidationException("User message", "Log Message");
-		} catch( IntrusionException e ) {
-			// expected occasionally
-		}
-		return vex;
-	}
-	
 }