Re: [PR] Added properties for base64 formatting [santuario -xml-security-java]

seanjmullan commented on code in PR #504:
URL: https://github.com/apache/santuario-xml-security-java/pull/504#discussion_r2482640633


##########
src/main/java/org/apache/xml/security/utils/XMLUtils.java:
##########
@@ -56,14 +56,63 @@
 /**
  * DOM and XML accessibility and comfort functions.
  *
+ * @implNote
+ * Following system properties affect XML formatting:

Review Comment:
   Nit: "The following ..."



##########
src/main/java/org/apache/xml/security/stax/impl/processor/output/AbstractEncryptOutputProcessor.java:
##########
@@ -24,12 +24,7 @@
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
 import java.security.spec.AlgorithmParameterSpec;
-import java.util.ArrayDeque;
-import java.util.ArrayList;
-import java.util.Deque;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
+import java.util.*;

Review Comment:
   Can you revert this change? I think it is better not to wildcard imports.



##########
src/main/java/org/apache/xml/security/utils/XMLUtils.java:
##########
@@ -1068,4 +1147,52 @@ public static byte[] getBytes(BigInteger big, int bitlen) {
 
         return resizedBytes;
     }
+
+    /**
+     * Aggregates formatting options for base64Binary values.
+     */
+    static class Base64FormattingOptions {
+        private boolean ignoreLineBreaks = false;
+        private Base64LineSeparator lineSeparator = Base64LineSeparator.CRLF;
+        private int lineLength = 76;
+
+        public boolean isIgnoreLineBreaks() {
+            return ignoreLineBreaks;
+        }
+
+        public void setIgnoreLineBreaks(boolean ignoreLineBreaks) {

Review Comment:
   Instead of set methods, why not add a constructor that takes the options and also checks for illegal syntax?



##########
src/main/java/org/apache/xml/security/utils/XMLUtils.java:
##########
@@ -56,14 +56,63 @@
 /**
  * DOM and XML accessibility and comfort functions.
  *
+ * @implNote
+ * Following system properties affect XML formatting:
+ * <ul>
+ *     <li>{@systemProperty org.apache.xml.security.ignoreLineBreaks} - ignores all line breaks,
+ *     making a single-line document. Overrides all other formatting options. Default: false</li>
+ *     <li>{@systemProperty org.apache.xml.security.base64.ignoreLineBreaks} - ignores line breaks in base64Binary values.
+ *     Takes precedence over line length and separator options (see below). Default: false</li>
+ *     <li>{@systemProperty org.apache.xml.security.base64.lineSeparator} - Sets the line separator sequence in base64Binary values.
+ *     Possible values: crlf, lf. Default: crlf</li>
+ *     <li>{@systemProperty org.apache.xml.security.base64.lineLength} - Sets maximum line length in base64Binary values.
+ *     The value is rounded down to nearest multiple of 4. Values less than 4 are ignored. Default: 76</li>
+ * </ul>
  */
 public final class XMLUtils {
 
+    private static final Logger LOG = System.getLogger(XMLUtils.class.getName());
+
+    private static final String IGNORE_LINE_BREAKS_PROP = "org.apache.xml.security.ignoreLineBreaks";
+    private static final String BASE64_IGNORE_LINE_BREAKS_PROP = "org.apache.xml.security.base64.ignoreLineBreaks";
+    private static final String BASE64_LINE_SEPARATOR_PROP = "org.apache.xml.security.base64.lineSeparator";
+    private static final String BASE64_LINE_LENGTH_PROP = "org.apache.xml.security.base64.lineLength";
+
     private static boolean ignoreLineBreaks =
             AccessController.doPrivileged(
-                    (PrivilegedAction<Boolean>) () -> Boolean.getBoolean("org.apache.xml.security.ignoreLineBreaks"));
+                    (PrivilegedAction<Boolean>) () -> Boolean.getBoolean(IGNORE_LINE_BREAKS_PROP));
+
+    private static Base64FormattingOptions base64Formatting =
+            AccessController.doPrivileged((PrivilegedAction<Base64FormattingOptions>) () -> {
+                Base64FormattingOptions options = new Base64FormattingOptions();
+                options.setIgnoreLineBreaks(Boolean.getBoolean(BASE64_IGNORE_LINE_BREAKS_PROP));
+
+                String lineSeparator = System.getProperty(BASE64_LINE_SEPARATOR_PROP);
+                if (lineSeparator != null) {
+                    try {
+                        options.setLineSeparator(Base64LineSeparator.valueOf(lineSeparator.toUpperCase()));
+                    } catch (IllegalArgumentException e) {
+                        LOG.log(Level.WARNING, "Illegal value of {0} property ignored: {1}",
+                                BASE64_LINE_SEPARATOR_PROP, lineSeparator);
+                    }
+                }
 
-    private static final Logger LOG = System.getLogger(XMLUtils.class.getName());
+                Integer lineLength = Integer.getInteger(BASE64_LINE_LENGTH_PROP);

Review Comment:
   I think it would be better to use `System.getProperty` and then `Integer.valueOf(String)` because `Integer.getInteger` does not distinguish between the property not being set and an invalid value for the integer.



##########
src/main/java/org/apache/xml/security/utils/XMLUtils.java:
##########
@@ -56,14 +56,63 @@
 /**
  * DOM and XML accessibility and comfort functions.
  *
+ * @implNote
+ * Following system properties affect XML formatting:
+ * <ul>
+ *     <li>{@systemProperty org.apache.xml.security.ignoreLineBreaks} - ignores all line breaks,
+ *     making a single-line document. Overrides all other formatting options. Default: false</li>
+ *     <li>{@systemProperty org.apache.xml.security.base64.ignoreLineBreaks} - ignores line breaks in base64Binary values.
+ *     Takes precedence over line length and separator options (see below). Default: false</li>
+ *     <li>{@systemProperty org.apache.xml.security.base64.lineSeparator} - Sets the line separator sequence in base64Binary values.
+ *     Possible values: crlf, lf. Default: crlf</li>
+ *     <li>{@systemProperty org.apache.xml.security.base64.lineLength} - Sets maximum line length in base64Binary values.
+ *     The value is rounded down to nearest multiple of 4. Values less than 4 are ignored. Default: 76</li>
+ * </ul>
  */
 public final class XMLUtils {
 
+    private static final Logger LOG = System.getLogger(XMLUtils.class.getName());
+
+    private static final String IGNORE_LINE_BREAKS_PROP = "org.apache.xml.security.ignoreLineBreaks";
+    private static final String BASE64_IGNORE_LINE_BREAKS_PROP = "org.apache.xml.security.base64.ignoreLineBreaks";
+    private static final String BASE64_LINE_SEPARATOR_PROP = "org.apache.xml.security.base64.lineSeparator";
+    private static final String BASE64_LINE_LENGTH_PROP = "org.apache.xml.security.base64.lineLength";
+
     private static boolean ignoreLineBreaks =
             AccessController.doPrivileged(
-                    (PrivilegedAction<Boolean>) () -> Boolean.getBoolean("org.apache.xml.security.ignoreLineBreaks"));
+                    (PrivilegedAction<Boolean>) () -> Boolean.getBoolean(IGNORE_LINE_BREAKS_PROP));
+
+    private static Base64FormattingOptions base64Formatting =

Review Comment:
   Also should log a warning if the other properties are set, since they have no impact if org.apache.xml.security.ignoreLineBreaks is true.



##########
src/main/java/org/apache/xml/security/utils/XMLUtils.java:
##########
@@ -56,14 +56,63 @@
 /**
  * DOM and XML accessibility and comfort functions.
  *
+ * @implNote
+ * Following system properties affect XML formatting:
+ * <ul>
+ *     <li>{@systemProperty org.apache.xml.security.ignoreLineBreaks} - ignores all line breaks,
+ *     making a single-line document. Overrides all other formatting options. Default: false</li>
+ *     <li>{@systemProperty org.apache.xml.security.base64.ignoreLineBreaks} - ignores line breaks in base64Binary values.
+ *     Takes precedence over line length and separator options (see below). Default: false</li>
+ *     <li>{@systemProperty org.apache.xml.security.base64.lineSeparator} - Sets the line separator sequence in base64Binary values.
+ *     Possible values: crlf, lf. Default: crlf</li>
+ *     <li>{@systemProperty org.apache.xml.security.base64.lineLength} - Sets maximum line length in base64Binary values.
+ *     The value is rounded down to nearest multiple of 4. Values less than 4 are ignored. Default: 76</li>
+ * </ul>
  */
 public final class XMLUtils {
 
+    private static final Logger LOG = System.getLogger(XMLUtils.class.getName());
+
+    private static final String IGNORE_LINE_BREAKS_PROP = "org.apache.xml.security.ignoreLineBreaks";
+    private static final String BASE64_IGNORE_LINE_BREAKS_PROP = "org.apache.xml.security.base64.ignoreLineBreaks";
+    private static final String BASE64_LINE_SEPARATOR_PROP = "org.apache.xml.security.base64.lineSeparator";
+    private static final String BASE64_LINE_LENGTH_PROP = "org.apache.xml.security.base64.lineLength";
+
     private static boolean ignoreLineBreaks =
             AccessController.doPrivileged(
-                    (PrivilegedAction<Boolean>) () -> Boolean.getBoolean("org.apache.xml.security.ignoreLineBreaks"));
+                    (PrivilegedAction<Boolean>) () -> Boolean.getBoolean(IGNORE_LINE_BREAKS_PROP));
+
+    private static Base64FormattingOptions base64Formatting =

Review Comment:
   if org.apache.xml.security.ignoreLineBreaks is true, none of these options matter, so did you consider skipping the getting of the other properties?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]