You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2020/08/05 06:54:30 UTC
[GitHub] [qpid-broker-j] vavrtom opened a new pull request #55: QPID-8460 [Broker-j] Do not expose private information to exception message
vavrtom opened a new pull request #55:
URL: https://github.com/apache/qpid-broker-j/pull/55
Implementation of https://issues.apache.org/jira/browse/QPID-8460.
----------------------------------------------------------------
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
[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
Posted by 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
[GitHub] [qpid-broker-j] rgodfrey merged pull request #55: QPID-8460 [Broker-j] Do not expose private information to exception message
Posted by GitBox <gi...@apache.org>.
rgodfrey merged pull request #55:
URL: https://github.com/apache/qpid-broker-j/pull/55
----------------------------------------------------------------
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
[GitHub] [qpid-broker-j] vavrtom commented on a change in pull request #55: QPID-8460 [Broker-j] Do not expose private information to exception message
Posted by GitBox <gi...@apache.org>.
vavrtom commented on a change in pull request #55:
URL: https://github.com/apache/qpid-broker-j/pull/55#discussion_r466329664
##########
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:
Added function <code>Strings.decodePrivateBase64(base64String, description)</code>, which is used in places, where private data is decoded. Description is added to exception message instead of converted string.
----------------------------------------------------------------
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
[GitHub] [qpid-broker-j] vavrtom commented on a change in pull request #55: QPID-8460 [Broker-j] Do not expose private information to exception message
Posted by GitBox <gi...@apache.org>.
vavrtom commented on a change in pull request #55:
URL: https://github.com/apache/qpid-broker-j/pull/55#discussion_r466325548
##########
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:
Done in last commit.
----------------------------------------------------------------
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