mattcasters commented on code in PR #8737:
URL: https://github.com/apache/hop/pull/8737#discussion_r4172761012
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java:
##########
@@ -940,23 +952,54 @@ private static boolean namesASecret(Field field, String
pattern) {
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.
+ * {@code NO_HARDCODED} only ever looks at secrets; any other condition on a
field named like one
+ * is treated the same, with the default name patterns as well as the rule's
own.
+ */
+ private static boolean holdsASecret(
+ CustomLintRule rule, RuleCondition condition, String fieldName) {
+ if (condition == RuleCondition.NO_HARDCODED) {
+ return true;
+ }
+ if (Utils.isEmpty(fieldName)) {
+ return false;
+ }
+ String name = fieldName.toLowerCase();
+ for (List<String> patterns :
+ List.of(DEFAULT_SECRET_FIELD_PATTERNS,
getPasswordFieldPatterns(rule))) {
+ for (String pattern : patterns) {
+ if (!Utils.isEmpty(pattern) &&
name.endsWith(pattern.trim().toLowerCase())) {
Review Comment:
**[bug]** A non-`NO_HARDCODED` value is redacted only when the rule's field
string ends with one of these camelCase suffixes. That is not the same set of
fields Hop stores as passwords. `@HopMetadataProperty(password = true)` names
that do not end with the list still go through `generateErrorMessage` and are
appended as `(current value: ...)`. Concrete cases: `accessKey` (S3 and Minio
metadata), `keyPassphrase`, `privateKeyPassphrase`, `authorizationHeaderValue`
/ `auth_header_value`, and `oauthJwtPrivateKey` / `oauth_jwt_private_key`. The
same `endsWith` check misses the serialized keys rule authors copy from
metadata JSON when the pattern is a compound word: `secret_key` does not end
with `secretkey`, nor do `api_key` and `secret_access_key`.
`extractFieldFromObject` documents that serialized name as the one a rule uses,
while `namesASecret` only ever sees the Java field name, so this gap is new. A
`MATCHES_PATTERN` rule of `^\\$\\{.*\\}$` on `accessKey` — the same shape as th
e new composed-password test — still prints the key. `NOT_EMPTY` does not,
because it only fires when the value is empty.
**Suggestion:** Treat the field that was actually read as secret when its
`@HopMetadataProperty.password()` is true, and compare suffixes after stripping
`_` so `secret_key` matches `secretKey`. Cover `accessKey` and
`oauth_jwt_private_key` with a `MATCHES_PATTERN` test that asserts the value is
absent.
##########
plugins/misc/lint/src/main/java/org/apache/hop/lint/CustomRuleExecutor.java:
##########
@@ -940,23 +952,54 @@ private static boolean namesASecret(Field field, String
pattern) {
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.
+ * {@code NO_HARDCODED} only ever looks at secrets; any other condition on a
field named like one
+ * is treated the same, with the default name patterns as well as the rule's
own.
+ */
+ private static boolean holdsASecret(
+ CustomLintRule rule, RuleCondition condition, String fieldName) {
+ if (condition == RuleCondition.NO_HARDCODED) {
Review Comment:
**[suggestion]** Every `NO_HARDCODED` clause is treated as a secret, so the
current value is dropped even when the field is not one.
`RuleCondition.NO_HARDCODED` is "Field must use variables, not hardcoded
values", and `RuleTargetFields.getCompatibleConditions` offers it for every
string field. A custom hostname, URL, or SQL rule that used this condition
previously reported `(current value: ...)`, which is what makes the finding
actionable, and now omits it. The comment that this condition "only ever looks
at secrets" does not match the enum or the rule builder; the core pack happens
to use it only for passwords, but that is not the condition's contract. DB-001
stays redacted either way, because its field is `password`.
**Suggestion:** Remove this unconditional branch and decide from the field
(name suffix and `password = true`). Keep a test that a non-secret
`NO_HARDCODED` rule still includes the current value.
--
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]
