Skip to content

Conversation

@smolnar82
Copy link
Contributor

❗❗❗IMPORTANT❗❗❗
This PR is an almost identical copy of #914 (including this description), except:

  • sanitizer configuration init is extracted to a private method
  • a new factory method was created in a singleton class to ensure the new SanitizedException still complies with the Single Responsibility principal, but sanitization is centralized and reusable.
  • new test cases added to the sanitization logic
  • new exception constructor w/o message

What changes were proposed in this pull request?

This pull request introduces a mechanism to sanitize error messages in the GatewayServlet to improve security by hiding IP addresses from exception messages. The following changes were made:

  • Added a isErrorMessageSanitizationEnabled flag to the GatewayServlet to control whether error messages should be sanitized.
  • Implemented the sanitizeException and sanitizeAndRethrow methods in GatewayServlet to handle exception sanitization.
  • Updated the GatewayConfig interface and its implementation GatewayConfigImpl to include a new method isErrorMessageSanitizationEnabled.
  • Created the GatewayServletTest class to parameterize tests for scenarios where sanitization is enabled and disabled.

How was this patch tested?

This patch was tested using the following methods:

  • Parameterized unit tests were added to GatewayServletTest to cover both scenarios where error message sanitization is enabled and disabled.
  • Manual review and inspection of the code changes to ensure accuracy and completeness.

Test steps:

  1. Added unit tests in GatewayServletTest to check for sanitized and non-sanitized error messages.
  2. Verified the new tests pass successfully, ensuring error messages are appropriately sanitized or left unchanged based on the configuration.

Copy link
Contributor

@hanicz hanicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a need to sanitise log messages? I believe it would be more beneficial to use safer logging practices in the first place.

That aside I think using log4j appenders could be better to apply sanitisation globally. I don't like the fact that a class has to prepare for sanitisation which suggests that its aware of the problem and the security risk.

Copy link
Contributor

@moresandeep moresandeep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @smolnar82 for picking this up!
The PR looks good, I have a few questions/comments I marked up on the code.
Also, looks like this only deals with GatewayServlet, what about other places which might leak IP addresses?

public static final String GATEWAY_DESCRIPTOR_LOCATION_PARAM = "gatewayDescriptorLocation";

private static boolean isErrorMessageSanitizationEnabled = true;
private static String errorMessageSanitizationPattern = "\\b(?:\\d{1,3}\\.){3}\\d{1,3}\\b";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add support for ipv6?


public static SanitizedException from(final Exception e, final boolean isSanitizationEnabled, final String pattern) {
if (Objects.isNull(e) || Objects.isNull(e.getMessage())) {
return new SanitizedException();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case there is no offending message (i.e. the message does not contain IP address) shouldn't we just throw the same exception instead of SanitizedException? looks like this might a) hide the original exception b) add new stack trace that won't be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants