Open noloader opened 2 years ago
noloader
commented
2 years ago @kwwall, @xeno6696
What do you recommend to avoid the test failures? Should the tests simply be removed?
(I'm getting ready to show a few engineers how to build and test ESAPI at $dayjob. I'd like to get these failures off the books).
xeno6696
commented
2 years ago The short answer is that all of these need to be rewritten with new versions of Power mock and mockito.
Those frameworks utilize some reflection methods that have gone away. That’s why you’re getting the unsupported operation exceptions.
noloader
commented
2 years ago Thanks @xeno6696,
The short answer is that all of these need to be rewritten with new versions of Power mock and mockito.
Ok, thanks. I commented them out for the time being. It is easier to skip the test than it is to field questions about why they don't work. Also see PR #728 PR #730.
Those frameworks utilize some reflection methods that have gone away.
This might also be related to ReferenceEncryptedProperties.java. When I search for "This method has been removed for security", it lands in ReferenceEncryptedProperties.java:
$ grep -IR 'This method has been removed for security'
src/main/java/org/owasp/esapi/reference/crypto/ReferenceEncryptedProperties.java: throw new UnsupportedOperationException("This method has been removed for security.");
src/main/java/org/owasp/esapi/reference/crypto/ReferenceEncryptedProperties.java: throw new UnsupportedOperationException("This method has been removed for security.");
src/main/java/org/owasp/esapi/reference/crypto/ReferenceEncryptedProperties.java: throw new UnsupportedOperationException("This method has been removed for security.");
src/main/java/org/owasp/esapi/reference/crypto/ReferenceEncryptedProperties.java: throw new UnsupportedOperationException("This method has been removed for security.");
src/main/java/org/owasp/esapi/reference/crypto/ReferenceEncryptedProperties.java: throw new UnsupportedOperationException("This method has been removed for security.");I admit that I didn't read into this or the PR very carefully. I've known for some time that all the mocking unit tests we started working with would eventually have to be redone, but what you're referencing here in ReferenceEncryptedProperties wasn't part of that.
My comment might be totally off base in this instance, my apologies.
noloader
commented
2 years ago @xeno6696, @kwwall,
No problems.
I cut-over ReferenceEncryptedPropertiesTest to use a dynamic test to decide whether to run the self tests. I did not realize KWW's version of Java was not having problems.
Is the updated patch more acceptable?
noloader
commented
2 years ago @kwwall, @xeno6696,
On my Java 11 machine I pass the self tests. On GitHub with Java 8 (?) it is failing with:
Error: Failures:
Error: ReferenceEncryptedPropertiesTest.testStoreLoadWithReader:274 Key one was never seen
[INFO]
Error: Tests run: 4274, Failures: 1, Errors: 0, Skipped: 0The code in question is here. The line numbers are off a bit, but the reference file line number is 140.
Would you take a look at what I am doing wrong with this PR?
kwwall
commented
2 years ago @noloader - Wait, what? You're saying if you run
mvn -Dtest=org.owasp.esapi.reference.crypto.ReferenceEncryptedPropertiesTest test
that you get test failures under Linux for Java 8??? I get:
[INFO] Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.154 s - in org.owasp.esapi.reference.crypto.ReferenceEncryptedPropertiesTest
What does running 'uname -a', 'lsb_release -a' and 'java -version' show as output? I'm running on a fully patched Linux Mint 19.2 on x64_64 hardware and running a Linux 4.15.0-189-generic kernel and I get all tests passing. But if we can't figure it out, I can see if I can install a VM to mimic your setup to try reproduce it; however, I gotta get the 2.5.0.0 release out the door first.
Java... write once, debug everywhere.
kwwall
commented
2 years ago @noloader wrote:
$ grep -IR 'This method has been removed for security' src/main/java/org/owasp/esapi/reference/crypto/ReferenceEncryptedProperties.java: throw new UnsupportedOperationException("This method has been removed for security."); src/main/java/org/owasp/esapi/reference/crypto/ReferenceEncryptedProperties.java: throw new UnsupportedOperationException("This method has been removed for security."); src/main/java/org/owasp/esapi/reference/crypto/ReferenceEncryptedProperties.java: throw new UnsupportedOperationException("This method has been removed for security."); src/main/java/org/owasp/esapi/reference/crypto/ReferenceEncryptedProperties.java: throw new UnsupportedOperationException("This method has been removed for security."); src/main/java/org/owasp/esapi/reference/crypto/ReferenceEncryptedProperties.java: throw new UnsupportedOperationException("This method has been removed for security.");
Looking at the code, that makes sense. UnsupportedOperationException is an unchecked exception, so devs are not going catch it. And this has absolutely ZERO to do with PowerMock and/or Mockito, because the JUnit test ReferenceEncryptedPropertiesTest uses neither of those.
But, if it is also failing for you on a Linux with Java 8, that would also seem to rule out any weirdness from reflection related things in the JDK. It seems to go far deeper than that. You didn't change the version of JUnit or any other dependencies, did you. I could see maybe switching up JUnit could cause weirdness like this, but short of that, I can't think of anything.
noloader
commented
2 years ago Wait, what? You're saying if you run mvn -Dtest=org.owasp.esapi.reference.crypto.ReferenceEncryptedPropertiesTest test that you get test failures under Linux for Java 8???
I'm probably doing something wrong with the way I am testing if setProp or store is available.
kwwall
commented
2 years ago @noloader - So, please explain what you mean by "Patched with dynamic detection"? You mean in the test itself? If so, can you point to the specific line #s or the specific commit(s)? (I've not yet looked at your changes.)
Note that I can confirm your test failures under Java 11. If I change my JDK to Java 11 and run the same test, I get the same failure that you do. And given the failure, I'm surprised that you are getting any of the tests to run successfully with Java 11.
noloader
commented
2 years ago So, please explain what you mean by "Patched with dynamic detection"? You mean in the test itself? If so, can you point to the specific line #s or the specific commit(s)?
From https://github.com/ESAPI/esapi-java-legacy/pull/728/files :
protected boolean hasSettersAndStores() {
ReferenceEncryptedProperties props = new ReferenceEncryptedProperties();
try {
props.setProperty("x", "y");
props.store(new ByteArrayOutputStream(), "XYZ");
}
catch (UnsupportedOperationException ex) {
return false;
}
catch (IOException ex) {
return false;
}
finally {
try {
props.remove("x");
props.remove("XYZ");
}
catch (Exception ex) {
}
}
return true;
}Then later, an existing test guarded by hasSettersAndStores():
@Test public void testStoreLoad() throws Exception
{
if ( hasSettersAndStores() == false ) {
// https://github.com/ESAPI/esapi-java-legacy/issues/721
System.out.println("testStoreLoadWithReader removed due to deprecation of ReferenceEncryptedProperties.store()");
}
else {
//create an EncryptedProperties to store
ReferenceEncryptedProperties toStore = new ReferenceEncryptedProperties();
toStore.setProperty("one", "two");
toStore.setProperty("two", "three");
toStore.setProperty("seuss.schneier", "one fish, twofish, red fish, blowfish");
...
}Long ago, in a galaxy far, far away, @xeno6696 scribed thusly:
The short answer is that all of these need to be rewritten with new versions of Power mock and mockito.
Those frameworks utilize some reflection methods that have gone away. That’s why you’re getting the unsupported operation exceptions.
Looking at this more closely, it looks like the failure is org.apache.maven.plugin.MojoFailureException in the mave-surefire-plugin, so I'm going to take a stab and say maybe it has something to do with that plugin?
First hit on Googling for 'java 11 maven-surefire-plugin' is https://stackoverflow.com/questions/53437819/maven-surefire-and-jdk-11 so seems as though we're not the only one. It also happens to mention a solution toward the end; namely adding this to the plugin's configuration:
<argLine>
--illegal-access=permit
</argLine>@noloader - Maybe you can take a look and give it a try???
noloader
commented
2 years ago @kwwall.
I wanted to forgo options like this. This makes folks RTFM:
<argLine>
--illegal-access=permit
</argLine>Instead, I switched to a try/catch around the operation. This is compatible with all Java versions, and works with/without command line arguments:
boolean supported = true;
try {
EncryptedPropertiesUtils.storeProperties(encryptedFilePath, props, "<property value>");
}
catch (UnsupportedOperationException ex) {
supported = false;
}
if ( supported ) {
// Continue with self test
}
else {
// Print message that test was skipped
}This workaround also fixes the 4 failed self tests with Java 13.
kwwall
commented
1 year ago Weird. I run on Linux Mint 19.2, and I don't get that error. I just check our 'develop' branch ( https://github.com/ESAPI/esapi-java-legacy/blob/develop/src/test/java/org/owasp/esapi/reference/crypto/EncryptedPropertiesUtilsTest.java#L79) and that method is still present. We do know you have to use Java 8 for testing. It fails with Java 11.
Can you maybe attach the 2 Surefire dump files for this?
-kevin
On Sat, Jul 16, 2022, 10:05 PM Jeffrey Walton @.***> wrote:
Hi Everyone/Kevin,
I'm building ESAPI develop from GitHub on Ubuntu 20.04, x86_64, fully patched.
mvn test is failing:
$ mvn test
...
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.994 s - in org.owasp.esapi.crypto.ESAPICryptoMACByPassTest
[INFO]
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] EncryptedPropertiesUtilsTest.testCreateNew:93 » UnsupportedOperation This method has been removed for security.
[ERROR] EncryptedPropertiesUtilsTest.testLoadEncryptedAndAdd:165 » UnsupportedOperation This method has been removed for security.
[ERROR] EncryptedPropertiesUtilsTest.testLoadPlaintextAndEncrypt:131 » UnsupportedOperation This method has been removed for security.
[ERROR] ReferenceEncryptedPropertiesTest.testStoreLoad:160 » UnsupportedOperation This method has been removed for security.
[INFO]
[ERROR] Tests run: 4276, Failures: 0, Errors: 4, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 33.429 s
[INFO] Finished at: 2022-07-16T22:01:31-04:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M7:test (default-test) on project esapi:
[ERROR]
[ERROR] Please refer to /home/jwalton/Desktop/esapi-legacy-fork/target/surefire-reports for the individual test results.
[ERROR] Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
[ERROR] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
Please let me know what else I can supply for you.
— Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/issues/721, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO6PGYGY4SZGH5SC7RN7NLVUNS7RANCNFSM53Y65WRQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>
kwwall
commented
1 year ago One possibility maybe we can leverage the maven-surefire-plugim to allow skipping of specific tests. Then you could just script that to ignore what is failing for you.
That may require pom changes to allow that, but it is preferred over marking tests as ignored because we need to see if things start failing. -kevin
On Tue, Jul 19, 2022, 3:21 PM Matt Seil @.***> wrote:
The short answer is that all of these need to be rewritten with new versions of Power mock and mockito.
Those frameworks utilize some reflection methods that have gone away. That’s why you’re getting the unsupported operation exceptions.
— Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/issues/721#issuecomment-1189464783, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO6PG27KSI3MT4SS6GK5VDVU3535ANCNFSM53Y65WRQ . You are receiving this because you were mentioned.Message ID: @.***>
Hi Everyone/Kevin,
I'm building ESAPI develop from GitHub on Ubuntu 20.04, x86_64, fully patched. I'm also seeing this on Fedora 36, x86_64, fully patched.
mvn testis failing:Here is the surefire-reports files: surefire-reports.zip
Please let me know what else I can supply for you.
And here is the result of
mvn -e: