On Mon, 16 Jun 2025 13:49:38 GMT, Mikhail Yankelevich <myankelev...@openjdk.org> wrote:
>> Based on the javadoc of `javax.crypto.Cipher` class, the cipher >> transformation should be either "algorithm/mode/padding" or >> "algorithm". When parsing the transformation, space(s) is trimmed off and >> empty strings are considered as "unspecified". This PR adds checks to ensure >> that transformations with empty "mode" and/or "padding" value in the >> "algorithm/mode/padding" form leads to `NoSuchAlgorithmException`. This >> reverts some changes made in >> [https://bugs.openjdk.org/browse/JDK-8358159](https://bugs.openjdk.org/browse/JDK-8358159) >> which allows empty mode and/or padding in the transformations. >> >> >> Thanks in advance for the review~ > > src/java.base/share/classes/javax/crypto/Cipher.java line 348: > >> 346: transformation.substring(0, endIdx).trim()); >> 347: if (algo.isEmpty()) { >> 348: throw new NoSuchAlgorithmException("Invalid transformation: >> " + > > I don't think this exception is tested. It could be by adding `test(" ", > provider);` in > [test/jdk/javax/crypto/Cipher/TestEmptyModePadding.java](https://github.com/openjdk/jdk/pull/25808/files#diff-5442ba4fbe85af64a99ca3c2d2b5795026be4c40e23af8337d52a64cefd6af35) > on line 68 Sure, I can add it. > test/jdk/javax/crypto/Cipher/TestEmptyModePadding.java line 75: > >> 73: try { >> 74: Cipher c = Cipher.getInstance(transformation, provider); >> 75: throw new RuntimeException("Expected NSAE not thrown"); > > Minor: Do you think it would be a good idea to print out `transformation` and > `provider` data for debug? In `System.err` that is. Otherwise afaik the > exception is thrown and no trace in system error, just in > `System.out.println`, making debugging quite challenging > > Just something like this > ```suggestion > System.err.println("Error while testing " + transformation); > throw new RuntimeException("Expected NSAE not thrown"); > > or just add it to the `RuntimeException` message Ok, I will update the test to include the `transformation` info in the exception message. As for `provider`, it's somewhat related, but given that it stays the same for the test, I printed it out just once in the beginning of the test. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25808#discussion_r2150929140 PR Review Comment: https://git.openjdk.org/jdk/pull/25808#discussion_r2150927529
