Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

org.owasp.esapi.reference.ValidatorTest issues #39

Closed
GoogleCodeExporter opened this issue May 20, 2015 · 5 comments
Closed

org.owasp.esapi.reference.ValidatorTest issues #39

GoogleCodeExporter opened this issue May 20, 2015 · 5 comments

Comments

@GoogleCodeExporter
Copy link

What steps will reproduce the problem?
1. checkout current trunk on a unix system (linux treid)
2. mvn -Dtest=ValidatorTest test

What is the expected output? What do you see instead?

Tests should pass. These tests fail:
  testGetValidDirectoryPath(org.owasp.esapi.reference.ValidatorTest)
  testIsInvalidFilename(org.owasp.esapi.reference.ValidatorTest)
  testIsValidDirectoryPath(org.owasp.esapi.reference.ValidatorTest)
  testIsValidFileUpload(org.owasp.esapi.reference.ValidatorTest)


What version of the product are you using? On what operating system?

trunk revision 900 on Linux (debian stable & unstable).

Please provide any additional information below.

testGetValidDirectoryPath, testIsValidDirectoryPath & testIsValidFileUpload

All suffer from the distributed ESAPI.properties not allowing a '/' in a
directory path (Validator.DirectoryName). A backslash '\\' is allowed so
this test probably works in windows tough will fail on anything using '/'
as a directory separator. Adding '/' to the properties.

Adding a '/' easily solves this problem but I wonder if there is a better
way. It would be nice if there was a way to add the current platform's
directory separator to the regex without listing the separators for other
platforms.

testIsValidDirectoryPath:

fails on line 330:
assertTrue(instance.isValidDirectoryPath("test", "/bin/sh", parent,
false));    // Standard shell
The test here, and not isValidDirectoryPath, is incorrect.
isValidDirectoryPath should, and does, return false. The path, though
valid, points to a file and not a directory. 

testIsInvalidFilename:

This test fails on a filename being passed as valid when it has a backslash
('\\') in it. The test expects this to be rejected as invalid which is
probably a good idea. The problem is that during the validation the
filename is canonicalized using the encoder. The encoder includes the
JavaScript codec which removes the backslash. When the canonicalized
filename is validated it no longer contains the backslash and validation
succeeds. 

I am not familiar enough with the ESAPI.properties, but changing
"Encoder.DefaultCodecList" is not having any affect on the encoders
actually used (validated by inserting printlns). Canonicalize is also
applying the codecs repeatedly until nothing changes which seems to be
contrary to the default Encoder.AllowMultipleEncoding=false as well.

Original issue reported on code.google.com by schal...@darkmist.net on 19 Oct 2009 at 9:16

@GoogleCodeExporter
Copy link
Author

Patch for add '/' to Validator.DirectoryName property and /bin/sh
isValidDirectoryPath. This fixes 3 out of 4 test failures for me.

Original comment by schal...@darkmist.net on 20 Oct 2009 at 2:32

Attachments:

@GoogleCodeExporter
Copy link
Author

ack... s/revision 900/revision 700/ I swear my brain works sometimes...

Original comment by schal...@darkmist.net on 20 Oct 2009 at 2:23

@GoogleCodeExporter
Copy link
Author

Updated patch to work against trunk revision 741

Original comment by schal...@darkmist.net on 2 Nov 2009 at 10:46

Attachments:

@GoogleCodeExporter
Copy link
Author

patch applied in revision 747

There is still the issue of testIsInvalidFilename so I'm not labeling this as 
fixed.
It might be worth splitting this out into a separate issue though as the rest 
are fixed. 

Original comment by schal...@darkmist.net on 4 Nov 2009 at 7:48

  • Changed state: Started

@GoogleCodeExporter
Copy link
Author

isInvalidFilename issue split out into issue 54

Original comment by schal...@darkmist.net on 10 Nov 2009 at 2:24

  • Changed state: Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant