From 869d3b8afa34e24ee541f9f97a7a703f8043a80f Mon Sep 17 00:00:00 2001 From: somiljain2006 Date: Sat, 11 Apr 2026 00:34:16 +0530 Subject: [PATCH] Improve secret detection with caching and setter support --- .../io/jenkins/plugins/casc/Attribute.java | 94 +++++++++++-------- .../jenkins/plugins/casc/AttributeTest.java | 67 +++++++++++-- 2 files changed, 113 insertions(+), 48 deletions(-) diff --git a/plugin/src/main/java/io/jenkins/plugins/casc/Attribute.java b/plugin/src/main/java/io/jenkins/plugins/casc/Attribute.java index 8f95e5d131..d92b180358 100644 --- a/plugin/src/main/java/io/jenkins/plugins/casc/Attribute.java +++ b/plugin/src/main/java/io/jenkins/plugins/casc/Attribute.java @@ -19,9 +19,11 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -48,9 +50,12 @@ public class Attribute { // Nop }; - // TODO: Concurrent cache? - // private static final HashMap SECRET_ATTRIBUTE_CACHE = - // new HashMap<>(); + private static final ClassValue> SECRET_ATTRIBUTE_CACHE = new ClassValue<>() { + @Override + protected Map computeValue(@NonNull Class type) { + return new ConcurrentHashMap<>(); + } + }; protected final String name; protected final Class type; @@ -398,7 +403,6 @@ private static Field locatePrivateFieldInHierarchy(Class clazz, @NonNull Stri return ExtraFieldUtils.getFieldNoForce(clazz, fieldName); } - // TODO: consider Boolean and third condition /** * This is a method which tries to guess whether an attribute is {@link Secret}. * @param targetClass Class of the target object. {@code null} if unknown @@ -416,48 +420,50 @@ public static boolean calculateIfSecret(@CheckForNull Class targetClass, @Non } if (targetClass == null) { - LOGGER.log( - Level.FINER, - "Attribute {0} is assumed to be non-secret, because there is no class instance in the call. " - + "This call is used only for fast-fetch caching, and the result may be adjusted later", - new Object[] {fieldName}); return false; // All methods below require a known target class } - // TODO: Cache decisions? + return SECRET_ATTRIBUTE_CACHE.get(targetClass).computeIfAbsent(fieldName, key -> { + Method m = locateGetter(targetClass, fieldName); + if (m != null && m.getReturnType() == Secret.class) { + LOGGER.log( + Level.FINER, + "Attribute {0}#{1} is secret, because there is a getter {2} which returns a Secret type", + new Object[] {targetClass.getName(), fieldName, m}); + return true; + } - Method m = locateGetter(targetClass, fieldName); - if (m != null && m.getReturnType() == Secret.class) { - LOGGER.log( - Level.FINER, - "Attribute {0}#{1} is secret, because there is a getter {2} which returns a Secret type", - new Object[] {targetClass.getName(), fieldName, m}); - return true; - } + Field f = locatePublicField(targetClass, fieldName); + if (f != null && f.getType() == Secret.class) { + LOGGER.log( + Level.FINER, + "Attribute {0}#{1} is secret, because there is a public field {2} which has a Secret type", + new Object[] {targetClass.getName(), fieldName, f}); + return true; + } - Field f = locatePublicField(targetClass, fieldName); - if (f != null && f.getType() == Secret.class) { - LOGGER.log( - Level.FINER, - "Attribute {0}#{1} is secret, because there is a public field {2} which has a Secret type", - new Object[] {targetClass.getName(), fieldName, f}); - return true; - } + f = locatePrivateFieldInHierarchy(targetClass, fieldName); + if (f != null && f.getType() == Secret.class) { + LOGGER.log( + Level.FINER, + "Attribute {0}#{1} is secret, because there is a private field {2} which has a Secret type", + new Object[] {targetClass.getName(), fieldName, f}); + return true; + } - f = locatePrivateFieldInHierarchy(targetClass, fieldName); - if (f != null && f.getType() == Secret.class) { - LOGGER.log( - Level.FINER, - "Attribute {0}#{1} is secret, because there is a private field {2} which has a Secret type", - new Object[] {targetClass.getName(), fieldName, f}); - return true; - } + if (hasSecretSetter(targetClass, fieldName)) { + LOGGER.log( + Level.FINER, + "Attribute {0}#{1} is secret, because there is a setter which takes a Secret type", + new Object[] {targetClass.getName(), fieldName}); + return true; + } - // TODO(oleg_nenashev): Consider setters? Gonna be more interesting since there might be many of them - LOGGER.log(Level.FINER, "Attribute {0}#{1} is not a secret, because all checks have passed", new Object[] { - targetClass.getName(), fieldName + LOGGER.log(Level.FINER, "Attribute {0}#{1} is not a secret, because all checks have passed", new Object[] { + targetClass.getName(), fieldName + }); + return false; }); - return false; } private Type _getValue(Owner target) throws ConfiguratorException { @@ -545,4 +551,16 @@ public int hashCode() { public static Setter noop() { return NOOP; } + + private static boolean hasSecretSetter(Class clazz, @NonNull String fieldName) { + final String setterName = "set" + StringUtils.capitalize(fieldName); + for (Method method : clazz.getMethods()) { + if (method.getName().equals(setterName) && method.getParameterCount() == 1) { + if (method.getParameterTypes()[0] == Secret.class) { + return true; + } + } + } + return false; + } } diff --git a/plugin/src/test/java/io/jenkins/plugins/casc/AttributeTest.java b/plugin/src/test/java/io/jenkins/plugins/casc/AttributeTest.java index 122e0693cc..e8ac478dbf 100644 --- a/plugin/src/test/java/io/jenkins/plugins/casc/AttributeTest.java +++ b/plugin/src/test/java/io/jenkins/plugins/casc/AttributeTest.java @@ -70,16 +70,26 @@ void checkNonSecretPatterns() { assertFieldIsNotSecret(NonSecretField.class, "passwordPath"); } + @Test + void checkSecretFromSetter() { + assertFieldIsSecret(SecretFromSetter.class, "secretField"); + } + + @Test + void checkUnknownClassCondition() { + assertFalse( + Attribute.calculateIfSecret(null, "someField"), + "calculateIfSecret should return false when targetClass is unknown"); + } + public static void assertFieldIsSecret(Class clazz, String fieldName) { String displayName = clazz != null ? (clazz.getName() + "#" + fieldName) : fieldName; - assertTrue(Attribute.calculateIfSecret(clazz, fieldName), "Field is not secret: " + displayName); + assertTrue(Attribute.calculateIfSecret(clazz, fieldName), "Field should be a secret: " + displayName); } public static void assertFieldIsNotSecret(Class clazz, String fieldName) { String displayName = clazz != null ? (clazz.getName() + "#" + fieldName) : fieldName; - assertFalse( - Attribute.calculateIfSecret(clazz, fieldName), - "Field is a secret while it should not be considered as one: " + displayName); + assertFalse(Attribute.calculateIfSecret(clazz, fieldName), "Field should not be a secret: " + displayName); } public static class WellDefinedField { @@ -104,21 +114,22 @@ public WellDefinedField2(Secret secret) { public static class SecretFromGetter { - Secret secretField; + private final String hiddenData; @DataBoundConstructor public SecretFromGetter(String secretField) { - this.secretField = Secret.fromString(secretField); + this.hiddenData = secretField; } - public Secret getSecret() { - return secretField; + @SuppressWarnings("unused") + public Secret getSecretField() { + return Secret.fromString(hiddenData); } } public static class SecretFromGetter2 extends SecretFromGetter { - public SecretFromGetter2(String secret) { - super(secret); + public SecretFromGetter2(String secretField) { + super(secretField); } } @@ -183,4 +194,40 @@ public String getMySecretValue() { return mySecretValueField.getPlainText(); } } + + public static class SecretFromSetter { + + private String secretField; + + @DataBoundConstructor + public SecretFromSetter() {} + + @SuppressWarnings("unused") + public String getSecretField() { + return secretField; + } + + @SuppressWarnings("unused") + public void setSecretField(Secret secretField) { + this.secretField = secretField.getPlainText(); + } + } + + @Test + void checkCalculationIsIdempotent() { + boolean firstTrue = Attribute.calculateIfSecret(SecretFromSetter.class, "secretField"); + boolean secondTrue = Attribute.calculateIfSecret(SecretFromSetter.class, "secretField"); + assertTrue(firstTrue); + assertTrue(secondTrue, "Subsequent calls should return the same TRUE result"); + + boolean firstFalse = Attribute.calculateIfSecret(NonSecretField.class, "passwordPath"); + boolean secondFalse = Attribute.calculateIfSecret(NonSecretField.class, "passwordPath"); + assertFalse(firstFalse); + assertFalse(secondFalse, "Subsequent calls should return the same FALSE result"); + + boolean firstUnknown = Attribute.calculateIfSecret(null, "someField"); + boolean secondUnknown = Attribute.calculateIfSecret(null, "someField"); + assertFalse(firstUnknown); + assertFalse(secondUnknown, "Subsequent calls should return the same fallback FALSE result"); + } }