Skip to content

Date validation rule 299#468

Merged
kwwall merged 30 commits into
ESAPI:developfrom
jeremiahjstacey:DateValidationRule_299
Jan 23, 2019
Merged

Date validation rule 299#468
kwwall merged 30 commits into
ESAPI:developfrom
jeremiahjstacey:DateValidationRule_299

Conversation

@jeremiahjstacey

Copy link
Copy Markdown
Collaborator

These commits offer a possible solution to issue #299.
Summary of changes:

  • DateValidationRule API extended to include a new sanitize method which accepts a ValidationErrorList
  • DefaultValidator updated to use the DateValidationRule sanatize method
  • Tests around implementation including sample strings from 299 and 258

The changes to DateValidationRule make a distinction between the getValid and sanitize API's. getValid will make the best possible effort, given configuration and formatter, to determine if any date can be parsed from the input string with no guarantee of safety or accuracy of the input. This route relies soley on the backing DateFormat for success.

sanitize extends the getValid behavior by attempting validate that the input can be considered 'safe' for application use.

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.
Adding test cases presented in the github issue
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.
Updating test to actually use assertions in execution so that the intended
state is verified.
Adding additional test case content.
Converting ValidationErrorList Test to use Junit4 syntax.
Updating assertions used in ValidationErrorList for more information in
Junit test failure cases.
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.
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.
ValidationErrorListTest:
  Removed sysouts
  Cleaned unused imports
  Formatted file (removed tab chars & adjusted indent)
Converting test to Junit4 Syntax.
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
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.
Initial basic tests for general class functionality.
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.
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.
Ignoring the explictly fail case in the test.
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.
Adding tests for new sanitize API method.
Altering method chaining to defer to the getValidDate which uses the ValidationErrorList which now contains the delegation to the DateValidationRule.
Creating a new test class that focuses on testing the DATE API of
DefaultValidator.

This is a wireframe commit.
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.
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.
Verifying workflow and response from the DateAPI when the
DateValidationRule would result in a ValidationException being thrown.
No functional changes. Mostly updating static imports and references.
Fixing inconsistent indentation in updated method block.
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).
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.
@Test
public void testWhitelistSetExtendedCharacterSets() {
String myString = "𡘾𦴩<𥻂";
//This was an issue wiht HTMLEntityCodec at one point, so it seems like we should be doing this test.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@xeno6696 Since you worked on the HTMLEntityCodec I thought you may have context into this test.
This test is currently marked as Ignored, but it feels like something that should be verified.
Interested in your thoughts.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is linked to the major update I did in 2017. I obviously missed that you @ignored these. They shouldn't be ignored.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see you got the string from the HTMLEntityCodecTest... I'm not sure why you didn't copy over the actual test....

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

	@Test
	public void testEntityDecoding(){
		assertEquals("<", codec.decode("&lt;"));
        assertEquals( "<", codec.decode("&LT"));
        assertEquals( "<", codec.decode("&lt;"));
        assertEquals( "<", codec.decode("&LT;"));
	}
	
	@Test
	public void test32BitCJK(){
		String s = "𡘾𦴩𥻂";
		String expected = "&#x2163e;&#x26d29;&#x25ec2;";
		String bad = "&#xd845;&#xde3e;&#xd85b;&#xdd29;&#xd857;&#xdec2;";
		assertEquals(false, expected.equals(bad));
		assertEquals(expected, codec.encode(new char[0], s));
	}
	
	@Test
	public void test32BitCJKMixedWithBmp(){
		String s = "𡘾𦴩<𥻂";
		String expected = "&#x2163e;&#x26d29;&lt;&#x25ec2;";
		String bad = "&#xd845;&#xde3e;&#xd85b;&#xdd29;&#xd857;&#xdec2;";
		assertEquals(false, expected.equals(bad));
		assertEquals(expected, codec.encode(new char[0], s));
	}
	
	@Test
	public void testDecodeforChars(){
		String s = "!@$%()=+{}[]";
		String expected = "!@$%()=+{}[]";
		assertEquals(expected, codec.decode(s));
	}
	
	@Test
	public void testMixedBmpAndNonBmp(){
		String nonBMP = new String(new int[]{0x2f804}, 0, 1);
		String bmp = "<a";
		String expected = "&lt;a&#x2f804;";
		String input = bmp + nonBMP;
		assertEquals(expected, codec.encode(new char[0], input));
	}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

obviously that very first test has a couple of mistakes that we missed, but these are the core tests.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In this case we're not encoding/decoding. We're stripping characters from a string.
I believe I have reasonable tests for the UTF-8 characters, but the utf-16 gave me some heartburn.

I think I tried to remove one of the characters from the origin string and ended up creating a test failure. Since this was ancillary to the fix I was chasing I used the fail clause to point out that I wasn't sure what exactly I was doing, or trying to do, so I could follow up on it later.

I think I was writing a bad test, or I had bad expectations with the character whitelist behavior. I can take another crack at it. Looking at it again, I think I need to whitelist the character pair that make up the symbol display. That may suffice for showing that utf-16 can be whitelisted through BasicValidationRule.

I don't think the other tests add value here, which is why I only pulled this data set.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I probably was not treating the utf-16 correctly. This implementation shows my desired intention:

@Test
   
   public void testWhitelistSetExtendedCharacterSets() {
       String myString = "𡘾𦴩<𥻂";
       //(55365 56894) (55387 56617) 60 (55383 57026)
       Set<Character> whitelist = new HashSet<>();
       whitelist.add((char) 60);
       whitelist.add((char) 55387);
       whitelist.add((char) 56617);
       String result = uit.whitelist(myString, whitelist);
       assertEquals("𦴩<", result);
   }

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This test is added to the pull request.

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

kwwall commented Jan 16, 2019

Copy link
Copy Markdown
Contributor

@jeremiahjstacey Wasn't this issue #299 the one that we thought that @xeno6696 added a dependency for joda-time:joda-time:2.10 in the pom.xml but then you found out that we really didn't need it? IIIRC, I don't think we are using it anywhere so could you add a commit to this PR that would delete that dependency from the pom.xml? (Or if we are using it for JUnit tests now, just change its scope to 'test'.) Thanks!

@jeremiahjstacey

Copy link
Copy Markdown
Collaborator Author

@kwwall , JodaTime Dependency was cleaned up in one of my first commits to ensure I wasn't building on an incorrect assumption:

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.
d9aa1a8

@kwwall kwwall merged commit bba046e into ESAPI:develop Jan 23, 2019
@jeremiahjstacey jeremiahjstacey deleted the DateValidationRule_299 branch April 24, 2022 12:40
Sign up for free to 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