This is an automated email from the ASF dual-hosted git repository.
mattcasters pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new 845389afae Fixes #8730 : Keep the values of password fields out of
lint findings (#8737)
845389afae is described below
commit 845389afae13202c746572d82f8e8f43241df8da
Author: Bart Maertens <[email protected]>
AuthorDate: Fri Oct 9 15:20:53 2026 +0200
Fixes #8730 : Keep the values of password fields out of lint findings
(#8737)
* Fixes #8730 : Keep the values of password fields out of lint findings
* Issue #8730 : Hide the values of fields Hop stores as passwords in lint
findings, not only those named like one
---
.../org/apache/hop/lint/CustomRuleExecutor.java | 177 +++++++++----
.../apache/hop/lint/SecretValueInFindingTest.java | 290 +++++++++++++++++++++
2 files changed, 421 insertions(+), 46 deletions(-)
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java
index 65b133bb7e..a2bf58c678 100644
---
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java
+++
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java
@@ -171,7 +171,12 @@ public class CustomRuleExecutor {
}
if (evaluateCondition(
clause.getCondition(), clauseValue, clause.getConditionValue(),
rule)) {
- violated.add(clause.describe() + " (actual: " +
describeValue(clauseValue) + ")");
+ violated.add(
+ clause.describe()
+ + " (actual: "
+ + describeValue(
+ clauseValue, holdsASecret(rule, hopObject,
clause.getTargetField()))
+ + ")");
} else if (rule.getCombinator() == RuleCombinator.ALL_OF &&
rule.isComposed()) {
// allOf needs every clause broken, so one satisfied clause ends it.
return results;
@@ -220,7 +225,9 @@ public class CustomRuleExecutor {
boolean violatesRule = rule.isComposed() ? !violated.isEmpty() :
violated.size() == 1;
if (violatesRule) {
- String message = generateErrorMessage(rule, fieldValue);
+ String message =
+ generateErrorMessage(
+ rule, holdsASecret(rule, hopObject, rule.getTargetField()) ?
null : fieldValue);
if (rule.isComposed()) {
message =
message
@@ -775,34 +782,12 @@ public class CustomRuleExecutor {
try {
Class<?> clazz = obj.getClass();
- List<Field> fields = getAllFields(clazz);
-
- // The name Hop serialises the property under comes first. That is the
name a rule author
- // actually sees, in the .hpl or .hwf file and in the metadata JSON, and
it is the one that
- // survives a rename of the Java field behind it.
- for (Field field : fields) {
- if (fieldName.equals(serialisedNameOf(field))) {
- field.setAccessible(true);
- return field.get(obj);
- }
- }
-
- // Then a declared field anywhere in the hierarchy, and its value is
returned as-is.
- // Skipping null or empty values here used to make them
indistinguishable from a missing
- // field, so a rule like "url NOT_EMPTY" could never fire.
- for (Field field : fields) {
- if (field.getName().equals(fieldName)) {
- field.setAccessible(true);
- return field.get(obj);
- }
- }
- // Then the same match ignoring case, because rules are hand-written
YAML and Hop's own
- // field names are inconsistent about it ("fileName" here, "filename"
there).
- for (Field field : fields) {
- if (field.getName().equalsIgnoreCase(fieldName)) {
- field.setAccessible(true);
- return field.get(obj);
- }
+ // The value is returned as-is. Skipping null or empty values here used
to make them
+ // indistinguishable from a missing field, so a rule like "url
NOT_EMPTY" could never fire.
+ Field named = fieldNamed(clazz, fieldName);
+ if (named != null) {
+ named.setAccessible(true);
+ return named.get(obj);
}
// Then a getter, which covers metas that expose a value they do not
store directly.
@@ -875,10 +860,13 @@ public class CustomRuleExecutor {
}
/** A field value as it should read inside a composed rule's message. */
- private static String describeValue(Object value) {
+ private static String describeValue(Object value, boolean secret) {
if (value == null) {
return "null";
}
+ if (secret) {
+ return "hidden";
+ }
String text = value.toString();
return text.length() > 60 ? text.substring(0, 57) + "..." : text;
}
@@ -918,6 +906,40 @@ public class CustomRuleExecutor {
return Utils.isEmpty(property.key()) ? field.getName() : property.key();
}
+ /**
+ * The declared field a rule's name refers to, or null when no field carries
that name.
+ *
+ * @param clazz the class to look in, superclasses included
+ * @param fieldName the name the rule uses
+ * @return the field, or null
+ */
+ private static Field fieldNamed(Class<?> clazz, String fieldName) {
+ List<Field> fields = getAllFields(clazz);
+
+ // The name Hop serialises the property under comes first. That is the
name a rule author
+ // actually sees, in the .hpl or .hwf file and in the metadata JSON, and
it is the one that
+ // survives a rename of the Java field behind it.
+ for (Field field : fields) {
+ if (fieldName.equals(serialisedNameOf(field))) {
+ return field;
+ }
+ }
+ // Then a declared field anywhere in the hierarchy.
+ for (Field field : fields) {
+ if (field.getName().equals(fieldName)) {
+ return field;
+ }
+ }
+ // Then the same match ignoring case, because rules are hand-written YAML
and Hop's own
+ // field names are inconsistent about it ("fileName" here, "filename"
there).
+ for (Field field : fields) {
+ if (field.getName().equalsIgnoreCase(fieldName)) {
+ return field;
+ }
+ }
+ return null;
+ }
+
/** Get all fields from a class hierarchy */
static List<Field> getAllFields(Class<?> clazz) {
List<Field> fields = new ArrayList<>();
@@ -1020,23 +1042,86 @@ public class CustomRuleExecutor {
return
field.getName().toLowerCase().endsWith(pattern.trim().toLowerCase());
}
+ /**
+ * Whether the value a clause reads is a secret, and so must stay out of the
finding.
+ *
+ * <p>A finding's message ends up in CI build logs, JSON and SARIF reports
and the GUI. DB-001
+ * used to report a hardcoded database password as "(current value:
secret123)", decrypting an
+ * {@code Encrypted} one on the way, so the rule meant to catch exposed
passwords exposed them.
+ *
+ * <p>The field decides, not the condition: Hop stores it as a password, or
its name ends like a
+ * secret's, with the default name patterns as well as the rule's own.
Underscores are ignored so
+ * the serialised {@code secret_access_key} matches {@code secretAccessKey}.
+ */
+ private static boolean holdsASecret(CustomLintRule rule, Object hopObject,
String fieldName) {
+ if (Utils.isEmpty(fieldName)) {
+ return false;
+ }
+ if (storedAsPassword(hopObject, fieldName)) {
+ return true;
+ }
+ String name = withoutUnderscores(fieldName);
+ for (List<String> patterns :
+ List.of(DEFAULT_SECRET_FIELD_PATTERNS,
getPasswordFieldPatterns(rule))) {
+ for (String pattern : patterns) {
+ if (!Utils.isEmpty(pattern) &&
name.endsWith(withoutUnderscores(pattern))) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ private static String withoutUnderscores(String name) {
+ return name.trim().replace("_", "").toLowerCase();
+ }
+
+ /**
+ * Whether the field a rule reads is one Hop stores as a password ({@code
@HopMetadataProperty
+ * password = true}), which covers secrets such as {@code accessKey} or
{@code
+ * oauth_jwt_private_key} that no name pattern catches.
+ */
+ private static boolean storedAsPassword(Object hopObject, String fieldName) {
+ Object holder = hopObject;
+ if (hopObject instanceof TransformMeta transformMeta) {
+ holder = transformMeta.getTransform();
+ } else if (hopObject instanceof ActionMeta actionMeta) {
+ holder = actionMeta.getAction();
+ }
+ String name = fieldName;
+ int dot = fieldName.lastIndexOf('.');
+ if (dot > 0) {
+ holder = extractFieldFromObject(holder, fieldName.substring(0, dot));
+ name = fieldName.substring(dot + 1);
+ }
+ if (holder == null || holder == FIELD_NOT_FOUND) {
+ return false;
+ }
+ Field field = fieldNamed(holder.getClass(), name);
+ HopMetadataProperty property =
+ field != null ? field.getAnnotation(HopMetadataProperty.class) : null;
+ return property != null && property.password();
+ }
+
+ private static final List<String> DEFAULT_SECRET_FIELD_PATTERNS =
+ Arrays.asList(
+ "password",
+ "pwd",
+ "passwd",
+ "secret",
+ "secretKey",
+ "credential",
+ "credentials",
+ "apiKey",
+ "apikey",
+ "secretAccessKey",
+ "token",
+ "accessToken",
+ "authToken");
+
/** Get password field patterns from rule parameters or return defaults */
private static List<String> getPasswordFieldPatterns(CustomLintRule rule) {
- List<String> defaultPatterns =
- Arrays.asList(
- "password",
- "pwd",
- "passwd",
- "secret",
- "secretKey",
- "credential",
- "credentials",
- "apiKey",
- "apikey",
- "secretAccessKey",
- "token",
- "accessToken",
- "authToken");
+ List<String> defaultPatterns = DEFAULT_SECRET_FIELD_PATTERNS;
if (rule.getAdditionalParameters() != null) {
Object patternsObj = rule.getAdditionalParameters().get("fieldPatterns");
diff --git
a/plugins/misc/lint/src/test/java/org/apache/hop/lint/SecretValueInFindingTest.java
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/SecretValueInFindingTest.java
new file mode 100644
index 0000000000..d626e9885f
--- /dev/null
+++
b/plugins/misc/lint/src/test/java/org/apache/hop/lint/SecretValueInFindingTest.java
@@ -0,0 +1,290 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hop.lint;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.List;
+import org.apache.hop.core.Result;
+import org.apache.hop.core.database.DatabaseMeta;
+import org.apache.hop.core.encryption.Encr;
+import org.apache.hop.metadata.api.HopMetadata;
+import org.apache.hop.metadata.api.HopMetadataBase;
+import org.apache.hop.metadata.api.HopMetadataProperty;
+import org.apache.hop.pipeline.transform.BaseTransformMeta;
+import org.apache.hop.pipeline.transform.ITransform;
+import org.apache.hop.pipeline.transform.ITransformData;
+import org.apache.hop.pipeline.transform.TransformMeta;
+import org.apache.hop.workflow.action.ActionBase;
+import org.apache.hop.workflow.action.ActionMeta;
+import org.junit.jupiter.api.Test;
+
+/**
+ * A finding never carries the value of a secret.
+ *
+ * <p>DB-001 reported a hardcoded database password as "(current value:
secret123)", in CI logs,
+ * JSON and SARIF reports and the GUI, so the rule meant to catch exposed
passwords exposed them.
+ *
+ * @see <a href="https://github.com/apache/hop/issues/8730";>#8730</a>
+ */
+public class SecretValueInFindingTest {
+
+ private static DatabaseMeta connection(String hostname, String username,
String password) {
+ DatabaseMeta databaseMeta = new DatabaseMeta();
+ databaseMeta.setName("CRM");
+ databaseMeta.setHostname(hostname);
+ databaseMeta.setUsername(username);
+ databaseMeta.setPassword(password);
+ return databaseMeta;
+ }
+
+ /** The core pack's DB-001, as it ships. */
+ private static CustomLintRule hardcodedDatabasePassword() {
+ CustomLintRule rule = new CustomLintRule();
+ rule.setId("DB-001");
+ rule.setEnabled(true);
+ rule.setSeverity("ERROR");
+ rule.setTarget(RuleTarget.DATABASE_CONNECTION);
+ rule.setTargetField("password");
+ rule.setCondition(RuleCondition.NO_HARDCODED);
+ rule.setName("Hardcoded Database Password");
+ rule.setDescription("Database connections must take the password from a
variable");
+ return rule;
+ }
+
+ private static String onlyMessage(CustomLintRule rule, DatabaseMeta
databaseMeta) {
+ List<LintResult> results =
+ CustomRuleExecutor.executeRule(rule, databaseMeta,
"/tmp/metadata/rdbms/CRM.json");
+ assertEquals(1, results.size(), "expected exactly one finding");
+ return results.get(0).getMessage();
+ }
+
+ @Test
+ public void aHardcodedPasswordIsReportedWithoutItsValue() {
+ String message =
+ onlyMessage(hardcodedDatabasePassword(), connection("localhost",
"crm", "secret123"));
+
+ assertFalse(message.contains("secret123"), message);
+ }
+
+ @Test
+ public void anEncryptedPasswordIsReportedWithoutEitherForm() {
+ String encrypted = Encr.encryptPasswordIfNotUsingVariables("secret123");
+
+ String message =
+ onlyMessage(hardcodedDatabasePassword(), connection("localhost",
"crm", encrypted));
+
+ assertFalse(message.contains("secret123"), message);
+ assertFalse(message.contains(encrypted), message);
+ }
+
+ @Test
+ public void aComposedRuleHidesThePasswordClause() {
+ CustomLintRule rule = new CustomLintRule();
+ rule.setId("CUSTOM-001");
+ rule.setEnabled(true);
+ rule.setSeverity("WARNING");
+ rule.setTarget(RuleTarget.DATABASE_CONNECTION);
+ rule.setTargetField("password");
+ rule.setCondition(RuleCondition.MATCHES_PATTERN);
+ rule.setConditionValue("^\\$\\{.*\\}$");
+ rule.setAdditionalClauses(List.of(new RuleClause("username",
RuleCondition.NOT_EMPTY, null)));
+ rule.setDescription("Use a variable for the password, and name a user");
+
+ String message = onlyMessage(rule, connection("localhost", "",
"secret123"));
+
+ assertFalse(message.contains("secret123"), message);
+ assertTrue(message.contains("actual: hidden"), message);
+ }
+
+ @Test
+ public void aValueThatIsNoSecretIsStillShown() {
+ CustomLintRule rule = new CustomLintRule();
+ rule.setId("CUSTOM-002");
+ rule.setEnabled(true);
+ rule.setSeverity("WARNING");
+ rule.setTarget(RuleTarget.DATABASE_CONNECTION);
+ rule.setTargetField("hostname");
+ rule.setCondition(RuleCondition.MATCHES_PATTERN);
+ rule.setConditionValue("^db\\..*");
+ rule.setDescription("Connections point at a db.* host");
+
+ String message = onlyMessage(rule, connection("localhost", "crm",
"secret123"));
+
+ assertTrue(message.contains("current value: localhost"), message);
+ }
+
+ /**
+ * A connection whose secrets Hop stores as passwords under names the
default patterns miss, as
+ * the S3, MinIO and Salesforce connections do.
+ */
+ @HopMetadata(key = "lint-test-storage-connection", name = "Storage
connection")
+ public static class StorageConnection extends HopMetadataBase {
+ @HopMetadataProperty private String endpoint =
"https://storage.example.com";;
+
+ @HopMetadataProperty(password = true)
+ private String accessKey = "AKIAEXAMPLEKEY";
+
+ @HopMetadataProperty(key = "oauth_jwt_private_key", password = true)
+ private String oauthJwtPrivateKey = "-----BEGIN PRIVATE KEY-----";
+
+ /** A third-party plugin that names its secret but never flags it as a
password. */
+ @HopMetadataProperty(key = "secret_key")
+ private String signingKey = "s3cr3t-signing-key";
+
+ public StorageConnection() {
+ super("storage");
+ }
+ }
+
+ private static CustomLintRule metadataRule(String field, RuleCondition
condition, String value) {
+ CustomLintRule rule = new CustomLintRule();
+ rule.setId("CUSTOM-003");
+ rule.setEnabled(true);
+ rule.setSeverity("WARNING");
+ rule.setTarget(RuleTarget.METADATA);
+ rule.setTargetField(field);
+ rule.setCondition(condition);
+ rule.setConditionValue(value);
+ rule.setDescription("Use a variable");
+ return rule;
+ }
+
+ private static String onlyMessage(CustomLintRule rule, StorageConnection
connection) {
+ List<LintResult> results =
+ CustomRuleExecutor.executeRule(rule, connection,
"/tmp/metadata/storage/storage.json");
+ assertEquals(1, results.size(), "expected exactly one finding");
+ return results.get(0).getMessage();
+ }
+
+ @Test
+ public void aPasswordPropertyIsHiddenWhateverItIsNamed() {
+ String message =
+ onlyMessage(
+ metadataRule("accessKey", RuleCondition.MATCHES_PATTERN,
"^\\$\\{.*\\}$"),
+ new StorageConnection());
+
+ assertFalse(message.contains("AKIAEXAMPLEKEY"), message);
+ }
+
+ @Test
+ public void aPasswordPropertyNamedByItsKeyIsHidden() {
+ String message =
+ onlyMessage(
+ metadataRule("oauth_jwt_private_key",
RuleCondition.MATCHES_PATTERN, "^\\$\\{.*\\}$"),
+ new StorageConnection());
+
+ assertFalse(message.contains("BEGIN PRIVATE KEY"), message);
+ }
+
+ @Test
+ public void aSnakeCaseSecretKeyIsHidden() {
+ String message =
+ onlyMessage(
+ metadataRule("secret_key", RuleCondition.MATCHES_PATTERN,
"^\\$\\{.*\\}$"),
+ new StorageConnection());
+
+ assertFalse(message.contains("s3cr3t-signing-key"), message);
+ }
+
+ @Test
+ public void aComposedRuleHidesAPasswordPropertyClause() {
+ CustomLintRule rule = metadataRule("endpoint", RuleCondition.NOT_EMPTY,
null);
+ rule.setCondition(RuleCondition.MATCHES_PATTERN);
+ rule.setConditionValue("^\\$\\{.*\\}$");
+ rule.setAdditionalClauses(
+ List.of(new RuleClause("accessKey", RuleCondition.MATCHES_PATTERN,
"^\\$\\{.*\\}$")));
+
+ String message = onlyMessage(rule, new StorageConnection());
+
+ assertFalse(message.contains("AKIAEXAMPLEKEY"), message);
+ assertTrue(message.contains("actual: hidden"), message);
+ }
+
+ @Test
+ public void aHardcodedValueThatIsNoSecretIsStillShown() {
+ String message =
+ onlyMessage(
+ metadataRule("endpoint", RuleCondition.NO_HARDCODED, null), new
StorageConnection());
+
+ assertTrue(message.contains("current value: https://storage.example.com";),
message);
+ }
+
+ /** A transform that stores a secret as a password under a name no pattern
catches. */
+ public static class StreamTransformMeta extends
BaseTransformMeta<ITransform, ITransformData> {
+ @HopMetadataProperty(key = "access_key", password = true)
+ private String accessKey = "AKIATRANSFORMKEY";
+ }
+
+ /** The same for an action. */
+ public static class UploadAction extends ActionBase {
+ @HopMetadataProperty(password = true)
+ private String accessKey = "AKIAACTIONKEY";
+
+ @Override
+ public Result execute(Result previousResult, int nr) {
+ return previousResult;
+ }
+ }
+
+ private static CustomLintRule rule(RuleTarget target, String field) {
+ CustomLintRule rule = metadataRule(field, RuleCondition.MATCHES_PATTERN,
"^\\$\\{.*\\}$");
+ rule.setTarget(target);
+ return rule;
+ }
+
+ private static String onlyMessage(CustomLintRule rule, Object hopObject) {
+ List<LintResult> results = CustomRuleExecutor.executeRule(rule, hopObject,
"/tmp/test.hpl");
+ assertEquals(1, results.size(), "expected exactly one finding");
+ return results.get(0).getMessage();
+ }
+
+ private static TransformMeta streamTransform() {
+ TransformMeta transformMeta = new TransformMeta();
+ transformMeta.setName("Stream");
+ transformMeta.setTransformPluginId("StreamConsume");
+ transformMeta.setTransform(new StreamTransformMeta());
+ return transformMeta;
+ }
+
+ @Test
+ public void aTransformPasswordPropertyIsHiddenByItsJavaName() {
+ String message = onlyMessage(rule(RuleTarget.TRANSFORM, "accessKey"),
streamTransform());
+
+ assertFalse(message.contains("AKIATRANSFORMKEY"), message);
+ }
+
+ @Test
+ public void aTransformPasswordPropertyIsHiddenByItsKey() {
+ String message = onlyMessage(rule(RuleTarget.TRANSFORM, "access_key"),
streamTransform());
+
+ assertFalse(message.contains("AKIATRANSFORMKEY"), message);
+ }
+
+ @Test
+ public void anActionPasswordPropertyIsHidden() {
+ UploadAction action = new UploadAction();
+ action.setPluginId("Upload");
+ ActionMeta actionMeta = new ActionMeta(action);
+
+ String message = onlyMessage(rule(RuleTarget.ACTION, "accessKey"),
actionMeta);
+
+ assertFalse(message.contains("AKIAACTIONKEY"), message);
+ }
+}
