[GitHub] [qpid-broker-j] rgodfrey commented on a change in pull request #55: QPID-8460 [Broker-j] Do not expose private information to exception message

[GitBox <gi...@apache.org>]

rgodfrey commented on a change in pull request #55:
URL: https://github.com/apache/qpid-broker-j/pull/55#discussion_r465567642



##########
File path: broker-core/src/main/java/org/apache/qpid/server/util/Strings.java
##########
@@ -134,7 +134,8 @@ public static Resolver chain(Resolver... resolvers)
         base64String = base64String.replaceAll("\\s","");
         if(!base64String.matches("^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$"))
         {
-            throw new IllegalArgumentException("Cannot convert string '"+ base64String+ "'to a byte[] - it does not appear to be base64 data");
+            // do not add base64String to exception message as it can contain private data
+            throw new IllegalArgumentException("Cannot convert string to a byte[] - it does not appear to be base64 data");

Review comment:
       My concern here is that while I understand the desire not to leak sensitive information; for an end user trying to track down the invalid configuration information, removing the actual value in error here would seem to make it much more difficult.
   
   Are there particular instances where you have identified that this could be a problem, and can we isolate them in some way that ensures that enough context information is provided so the user can resolve the underlying issue?

##########
File path: broker-core/src/main/java/org/apache/qpid/server/model/ConfiguredAutomatedAttribute.java
##########
@@ -255,7 +255,8 @@ public T convert(final Object value, C object)
             Type returnType = getGetter().getGenericReturnType();
             String simpleName = returnType instanceof Class ? ((Class) returnType).getSimpleName() : returnType.toString();
 
-            throw new IllegalArgumentException("Cannot convert '" + value
+            throw new IllegalArgumentException("Cannot convert '" +
+                                               (User.PASSWORD.equals(getName()) ? AbstractConfiguredObject.SECURED_STRING_VALUE : value)

Review comment:
       Rather than use `User.PASSWORD.equals(getName())` why not use `isSecure()`? The secure annotation on an attribute is designed specifically for this use case of identifying attributes whose value should be considered confidential information




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org