-
-
Notifications
You must be signed in to change notification settings - Fork 8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: For security purposes, remove the output of Capabilities information from the logs. #13710
base: trunk
Are you sure you want to change the base?
Conversation
PR Description updated to latest commit (0a8c20e)
|
PR Review
✨ Review tool usage guide:Overview:
With a configuration file, use the following template:
See the review usage page for a comprehensive guide on using this tool. |
PR Code Suggestions
✨ Improve tool usage guide:Overview:
With a configuration file, use the following template:
See the improve usage page for a more comprehensive guide on using this tool. |
@zhangwt-cn - Not everyone would want this to be the default behaviour no? What if this were to be behind a flag, which when flipped will give this behaviour? Either way, I would wait to hear from @diemol before you take up the recommendation of obfuscating the capabilities for security reasons. |
@krmahadevan Users should have the capability to produce their own logs, and I believe that the output of sensitive information should not come from the logs of the underlying framework. |
Sure. But the point of contention is as to what does and what does not denote/represent as sensitive information. For majority of the users, they aren't packing in any sensitive data into the capabilities. The capabilities itself does not have any such information apart from the browser version and the optional flags that are being passed to the underlying driver. So there's no reason as to why this needs to be hidden out from the regular user. It looks to me that you are adding additional stuff into the capabilities object which you wouldn't want to be logged. That was why I am suggesting that this behaviour be controlled via a flag which can be toggled (It should be switched off by default) |
Okay, I understand what you mean. I'll make some adjustments based on your suggestions. |
@zhangwt-cn Please hold on. Lets hear from @diemol before you make any changes. I would like to ensure that you spend more time on this based on his inputs. I was just throwing in my thoughts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for starting to work on this, @zhangwt-cn!
I agree with @krmahadevan. Not all users want this to be the default behavior. As an example, this information helps debug as well.
The fix could be to obfuscate any given capability or a pattern (e.g., wss://admin:admin@org-se...
), which should be under a flag and configuration.
Added a
Users can utilize the FirefoxOptions options = new FirefoxOptions();
options.setCapability("privacyMode", false); If privacyMode is true, then it will display *******, if it is false, it will display the real content. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this would not work because:
privacyMode
is not a W3C capability, so it would be filtered and ignored.- This change modifies the capability value, and we only want to obfuscate it. The client bindings must use the actual value to connect to the server.
We probably want a configuration on the server side that enables or disables the obfuscation, following the same pattern we use through flags and options: https://github.com/SeleniumHQ/selenium/tree/trunk/java/src/org/openqa/selenium/grid/distributor/config
Okay, I will make some modifications. |
@@ -31,6 +33,16 @@ class SharedCapabilitiesMethods { | |||
|
|||
private static final String[] EMPTY_ARRAY = new String[0]; | |||
|
|||
private static Boolean PRIVACY_MODE = true; | |||
|
|||
private static final Set<String> PRIVACY_KEYS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now target Java 11+, so it's okay to use Set.of("se:vnc", "se:cdp")
@diemol Based on your advice, I made the following changes public class DistributorOptions {
public static boolean PRIVACY_MODE = true;
private final Config config;
public DistributorOptions(Config config) {
this.config = config;
PRIVACY_MODE = getPrivacyMode();
}
public boolean getPrivacyMode() {
return config
.getBool(DISTRIBUTOR_SECTION, "privacy-mode")
.orElse(PRIVACY_MODE);
}
} public class DistributorFlags implements HasRoles {
@Parameter(
names = "--privacy-mode",
description = "If privacy-mode is true, then sensitive information will display *******, if it is false, it will display the real content. ")
@ConfigValue(section = DISTRIBUTOR_SECTION, name = "privacy-mode", example = "true")
private boolean privacyMode = PRIVACY_MODE;
} but I encountered a problem: |
User description
Fixes #13648
Description
For security purposes, remove the output of Capabilities information from the logs.
Motivation and Context
fix #13648
Types of changes
Checklist
Type
Bug fix
Description
LocalDistributor
andLocalNode
to enhance security.Changes walkthrough
LocalDistributor.java
Remove Session Capabilities from Log Output in LocalDistributor
java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java
session creation.
LocalNode.java
Remove Session Capabilities from Log Output in LocalNode
java/src/org/openqa/selenium/grid/node/local/LocalNode.java
session creation.