davsclaus commented on code in PR #26803:
URL: https://github.com/apache/camel/pull/26803#discussion_r4086388425
##########
core/camel-util/src/main/java/org/apache/camel/util/URISupport.java:
##########
@@ -88,16 +89,19 @@ private URISupport() {
* @param keywords keywords separated by comma
*/
public static synchronized void addSanitizeKeywords(String keywords) {
Review Comment:
🟠**This changes `addSanitizeKeywords` from replace to accumulate-forever,
which changes what the one production caller does.**
The only non-test caller is
`DefaultCamelContextExtension.setAdditionalSensitiveKeywords()`, whose comment
reads *"re-configure sensitive keywords asap so they take effect immediately"*.
Before this PR each call replaced the keyword set; now it unions into a
JVM-global set that can never shrink. So in a JVM with more than one
`CamelContext` — Spring Boot tests, jbang, Quarkus dev mode — context B
inherits context A's keywords, and changing the property at runtime can no
longer narrow the set.
The direction is the safe one (more masking, never less), and this method's
own javadoc already says *"when a key has been added it cannot be removed"* —
so I read this as deliberate, and probably as the fix rather than the
regression. Two consequences worth settling though:
1. The setter's "take effect immediately" comment now promises something the
method can't deliver for removals; worth rewording it there.
2. The PR body says *"Only the masked output changes, so there is no upgrade
guide entry."* The masked output now changes for URIs where the user never
configured the keyword in question, which is the kind of behaviour change
`CLAUDE.md` asks to be recorded. A short note in
`docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc` would
cover it.
##########
core/camel-util/src/main/java/org/apache/camel/util/URISupport.java:
##########
@@ -106,21 +110,101 @@ public static synchronized void
addSanitizeKeywords(String keywords) {
* @param uri The uri to sanitize.
* @return Returns null if the uri is null, otherwise the URI with the
passphrase, password or secretKey
* sanitized.
- * @see #ALL_SECRETS and #USERINFO_PASSWORD for the matched pattern
+ * @see #USERINFO_PASSWORD for the matched pattern
*/
public static String sanitizeUri(String uri) {
// use xxxxx as replacement as that works well with JMX also
String sanitized = uri;
if (uri != null) {
- sanitized = ALL_SECRETS.matcher(sanitized).replaceAll("$1=xxxxxx");
- if (EXTRA_SECRETS != null) {
- sanitized =
EXTRA_SECRETS.matcher(sanitized).replaceFirst("$1=xxxxxx");
- }
- sanitized =
USERINFO_PASSWORD.matcher(sanitized).replaceFirst("$1xxxxxx$3");
+ sanitized = sanitizeQueryParameters(sanitized);
+ sanitized =
USERINFO_PASSWORD.matcher(sanitized).replaceAll("$1xxxxxx$3");
}
return sanitized;
}
+ /**
+ * Returns a copy of the parameters where the values of sensitive
parameters (such as passwords) are masked, using
+ * the same rules as {@link #sanitizeUri(String)}.
+ *
+ * @param parameters the parameters
+ * @return a copy of the parameters with the sensitive values
masked
+ */
+ public static Map<String, Object> sanitizeParameters(Map<String, Object>
parameters) {
+ Map<String, Object> answer = new LinkedHashMap<>(parameters.size());
Review Comment:
🟡 **`sanitizeParameters(null)` throws an NPE**, on `parameters.size()`:
```
java.lang.NullPointerException: Cannot invoke "java.util.Map.size()" because
"parameters" is null
```
Its sibling `sanitizeUri(null)` returns `null` rather than throwing, and
this is new public API on a util class that components reach for. The three
call sites added in this PR all pass a non-null map, so nothing is broken today
— it's about the contract the method offers to the next caller.
I'm not attaching a suggestion because there are two defensible answers:
return `null` for symmetry with `sanitizeUri`, or return an empty map. Either
is fine, worth one of them being chosen explicitly.
##########
core/camel-util/src/main/java/org/apache/camel/util/URISupport.java:
##########
@@ -51,20 +53,18 @@ public final class URISupport {
public static final char[] RAW_TOKEN_START = { '(', '{' };
public static final char[] RAW_TOKEN_END = { ')', '}' };
- @SuppressWarnings("RegExpUnnecessaryNonCapturingGroup")
- private static final String PRE_SECRETS_FORMAT =
"([?&][^=]*(?:%s)[^=]*)=(RAW(([{][^}]*[}])|([(][^)]*[)]))|[^&]*)";
+ // Match the key of a query parameter as first capture group (the value
starts after the = sign)
+ private static final Pattern QUERY_PARAMETER_KEY =
Pattern.compile("[?&]([^?&=]*)=");
- // Match any key-value pair in the URI query string whose key contains
- // "passphrase" or "password" or secret key (case-insensitive).
- // First capture group is the key, second is the value.
- private static final Pattern ALL_SECRETS
- =
Pattern.compile(PRE_SECRETS_FORMAT.formatted(SensitiveUtils.getSensitivePattern()),
- Pattern.CASE_INSENSITIVE);
+ // Match any of the sensitive keywords (such as passphrase, password or
secret key) in a query parameter key
+ private static final Pattern SENSITIVE_KEYWORDS
+ = Pattern.compile(SensitiveUtils.getSensitivePattern(),
Pattern.CASE_INSENSITIVE);
// Match the user password in the URI as second capture group
// (applies to URI with authority component and userinfo token in the form
- // "user:password").
- private static final Pattern USERINFO_PASSWORD =
Pattern.compile("(.*://.*?:)(.*)(@)");
+ // "user:password"). The authority ends at the first / or ? and the
userinfo
+ // ends at the last @ in the authority, which is how normalizeUri reads it.
+ private static final Pattern USERINFO_PASSWORD =
Pattern.compile("(://[^/?:]*:)([^/?]*)(@)");
Review Comment:
🔴 **This narrowing stops masking passwords that contain `/` or `?`.**
Excluding those two characters from the password group changes the outcome
for inputs that `main` masks today. Measured against both builds:
| input | `main` | this PR |
|---|---|---|
| `ftp://joe:pa/ss@host/dir` | `ftp://joe:xxxxxx@host/dir` |
`ftp://joe:pa/ss@host/dir` |
| `ftp://joe:pa?ss@host/dir` | `ftp://joe:xxxxxx@host/dir` |
`ftp://joe:pa?ss@host/dir` |
This is the one direction a sanitizer shouldn't regress in. Over-masking is
cosmetic; under-masking puts a credential into a log line, a JMX object name,
or a `ResolveEndpointFailedException` message.
On the justification in the comment — "the authority ends at the first `/`
or `?` ... which is how `normalizeUri` reads it" — I couldn't confirm that:
```
URISupport.normalizeUri("ftp://joe:pa/ss@host/dir";) ->
ftp://joe:pa/ss@host/dir
```
`normalizeUri` returns it unchanged; it neither rejects the URI nor re-cuts
the authority. (`java.net.URI` does parse the authority as `joe:pa`, but
nothing on this path uses that.) And `sanitizeUri` is routinely applied to free
text — route labels, log messages — that never went through the parser at all,
so parser semantics aren't a safe bound here.
I don't think there's one obvious fix, so I'd rather not push a suggestion:
widening the password class re-breaks the `smtp://host:[email protected]`
case that the new anchor gets right, so it probably wants either a two-stage
match (narrow first, wider fallback when the narrow one finds no `@` in the
authority) or an explicit decision that unencoded `/` and `?` in userinfo are
out of scope — in which case please pin that in a test and say so in the
comment, so the next person doesn't read it as an oversight.
--
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]
