bug: ApacheBase64ToJavaBase64 does not transform Base64.encodeBase64URLSafeString

[yeikel]

Leaving the import import org.apache.commons.codec.binary.Base64 leads to a conflict with import java.util.Base64 and a compiler error

It also does not transform Base64.encodeBase64URLSafeString

From what I understand and tested, the closest to that is Base64.getUrlEncoder().withoutPadding().encodeToString

Sample class to reproduce this problem :

import org.apache.commons.codec.binary.Base64;

import javax.crypto.Mac;
import javax.crypto.SecretKey;
import javax.crypto.spec.SecretKeySpec;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;

public class TokenGenerator {

    private String generateSignature(final String appId, final String secret, final String version, final long timestamp) throws NoSuchAlgorithmException, InvalidKeyException {
        final Mac mac = Mac.getInstance("HmacSHA256");
        final byte[] hmacKeyBytes = Base64.decodeBase64(secret.getBytes());
        final SecretKey secretKey = new SecretKeySpec(hmacKeyBytes, mac.getAlgorithm());
        mac.init(secretKey);
        final String input = appId + "-" + version + "-" + timestamp;
        final byte[] rawHmac = mac.doFinal(input.getBytes());
        return Base64.encodeBase64URLSafeString(rawHmac);
    }
}

command I executed :

mvn rewrite:run -Drewrite.activeRecipes="org.openrewrite.java.migrate.apache.commons.codec.ApacheBase64ToJavaBase64"

diff :

@@ -16,6 +16,7 @@ import java.net.URI;
 import java.net.URISyntaxException;
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
+import java.util.Base64;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
@@ -111,7 +112,7 @@ public class TokenGenerator {
 
     private String generateSignature(final String appId, final String secret, final String version, final long timestamp) throws NoSuchAlgorithmException, InvalidKeyException {
         final Mac mac = Mac.getInstance("HmacSHA256");
-        final byte[] hmacKeyBytes = Base64.decodeBase64(secret.getBytes());
+        final byte[] hmacKeyBytes = Base64.getDecoder().decode(secret.getBytes());
         final SecretKey secretKey = new SecretKeySpec(hmacKeyBytes, mac.getAlgorithm());
         mac.init(secretKey);
         final String input = appId + "-" + version + "-" + timestamp;

Expected :

private String generateSignature(final String appId, final String secret, final String version, final long timestamp) throws NoSuchAlgorithmException, InvalidKeyException {
        final Mac mac = Mac.getInstance("HmacSHA256");
        final byte[] hmacKeyBytes = Base64.getDecoder().decode(secret.getBytes());
        final SecretKey secretKey = new SecretKeySpec(hmacKeyBytes, mac.getAlgorithm());
        mac.init(secretKey);
        final String input = appId + "-" + version + "-" + timestamp;
        final byte[] rawHmac = mac.doFinal(input.getBytes());
        return Base64.getUrlEncoder().withoutPadding().encodeToString(rawHmac);
    }

[pway99]

Thanks Yiekel,

From what I understand and tested, the closest to that is Base64.getUrlEncoder().withoutPadding().encodeToString

I recall being uncertain with that conversion and most likely chose not to migrate those statements. If we can prove the results are exactly the same then I would not hesitate to update the recipe accordingly.

This is an excellent example of intentionally not converting a statement in the spirit of do no harm. Consider a case where the encoding is not precisely the same after the conversion; the app is deployed to production, and every request associated with the encoding process fails.

[yeikel]

I understand your point, but if we can't do a full conversion, shouldn't we skip the recipe and print a warning?

Leaving a broken build is not acceptable in my opinion

I'll research a bit about this specific example and I'll get back to you

[pway99]

Good point, and thanks for the suggestion. It would be easy to skip the SourceFile if that method exists or add a comment as you suggest. I will mark this as a bug and wait to hear from you on the suggested changes.

[tkvangorder]

We definitely should not be leaving a source file in a broken state.

[pway99]

Hi @yeikel, Looking at the code and running a few tests it appears to be a safe conversion. I will update the recipe.

apache commons
https://github.com/apache/commons-codec/blob/5f90731d40e6ed12d1ebfddfeb5dca5550d19cdd/src/main/java/org/apache/commons/codec/binary/Base64.java#L207-L209

java util
https://github.com/apache/commons-codec/blob/5f90731d40e6ed12d1ebfddfeb5dca5550d19cdd/src/main/java/org/apache/commons/codec/binary/Base64.java#L207-L209

Thanks again for the suggestion :)