[ https://issues.apache.org/jira/browse/WW-5352?focusedWorklogId=899567&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-899567 ]
ASF GitHub Bot logged work on WW-5352: -------------------------------------- Author: ASF GitHub Bot Created on: 15/Jan/24 05:52 Start Date: 15/Jan/24 05:52 Worklog Time Spent: 10m Work Description: lukaszlenart commented on code in PR #832: URL: https://github.com/apache/struts/pull/832#discussion_r1451950824 ########## core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java: ########## @@ -295,13 +340,150 @@ protected void notifyDeveloperParameterException(Object action, String property, * @return true if parameter is accepted */ protected boolean isAcceptableParameter(String name, Object action) { - return acceptableName(name) && isAcceptableParameterNameAware(name, action); + return acceptableName(name) && isAcceptableParameterNameAware(name, action) && isParameterAnnotatedAndAllowlist(name, action); } protected boolean isAcceptableParameterNameAware(String name, Object action) { return !(action instanceof ParameterNameAware) || ((ParameterNameAware) action).acceptableParameterName(name); } + /** + * Checks if the Action class member corresponding to a parameter is appropriately annotated with + * {@link StrutsParameter} and OGNL allowlists any necessary classes. + * <p> + * Note that this logic relies on the use of {@link DefaultAcceptedPatternsChecker#NESTING_CHARS} and may also + * be adversely impacted by the use of custom OGNL property accessors. + */ + protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { + if (!requireAnnotations) { + return true; + } + + long paramDepth = name.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); + if (requireAnnotationsTransitionMode && paramDepth == 0) { + return true; + } + + int nestingIndex = indexOfAny(name, NESTING_CHARS_STR); + String rootProperty = nestingIndex == -1 ? name : name.substring(0, nestingIndex); + + return hasValidAnnotatedMember(rootProperty, action, paramDepth); + } + + /** + * Note that we check for a public field last or only if there is no valid, annotated property descriptor. This is + * because this check is likely to fail more often than not, as the relative use of public fields is low - so we + * save computation by checking this last. + */ + protected boolean hasValidAnnotatedMember(String rootProperty, Object action, long paramDepth) { + BeanInfo beanInfo = getBeanInfo(action); + if (beanInfo == null) { + return hasValidAnnotatedField(action, rootProperty, paramDepth); + } + + Optional<PropertyDescriptor> propDescOpt = Arrays.stream(beanInfo.getPropertyDescriptors()) + .filter(desc -> desc.getName().equals(rootProperty)).findFirst(); + if (!propDescOpt.isPresent()) { + return hasValidAnnotatedField(action, rootProperty, paramDepth); + } + + if (hasValidAnnotatedPropertyDescriptor(propDescOpt.get(), paramDepth)) { + return true; + } + + return hasValidAnnotatedField(action, rootProperty, paramDepth); + } + + protected boolean hasValidAnnotatedPropertyDescriptor(PropertyDescriptor propDesc, long paramDepth) { + Method relevantMethod = paramDepth == 0 ? propDesc.getWriteMethod() : propDesc.getReadMethod(); + if (relevantMethod == null) { + return false; + } + StrutsParameter annotation = getParameterAnnotation(relevantMethod); + if (annotation == null || annotation.depth() < paramDepth) { + return false; + } + if (paramDepth >= 1) { + allowlistClass(relevantMethod.getReturnType()); + } + if (paramDepth >= 2) { + allowlistReturnTypeIfParameterized(relevantMethod); + } + return true; + } + + protected void allowlistReturnTypeIfParameterized(Method method) { + allowlistParameterizedTypeArg(method.getGenericReturnType()); + } + + protected void allowlistParameterizedTypeArg(Type genericType) { + if (!(genericType instanceof ParameterizedType)) { + return; + } + Type[] paramTypes = ((ParameterizedType) genericType).getActualTypeArguments(); + allowlistParamType(paramTypes[0]); + if (paramTypes.length > 1) { + // Probably useful for Map or Map-like classes + allowlistParamType(paramTypes[1]); + } + } + + protected void allowlistParamType(Type paramType) { + if (paramType instanceof Class) { + allowlistClass((Class<?>) paramType); + } + } + + protected void allowlistClass(Class<?> clazz) { + threadAllowlist.allowClass(clazz); + ClassUtils.getAllSuperclasses(clazz).forEach(threadAllowlist::allowClass); + ClassUtils.getAllInterfaces(clazz).forEach(threadAllowlist::allowClass); + } + + protected boolean hasValidAnnotatedField(Object action, String fieldName, long paramDepth) { + Field field; + try { + field = action.getClass().getDeclaredField(fieldName); + } catch (NoSuchFieldException e) { + return false; + } + if (!Modifier.isPublic(field.getModifiers())) { + return false; + } + StrutsParameter annotation = getParameterAnnotation(field); + if (annotation == null || annotation.depth() < paramDepth) { + return false; + } + if (paramDepth >= 1) { + allowlistClass(field.getType()); + } + if (paramDepth >= 2) { + allowlistFieldIfParameterized(field); + } + return true; + } + + protected void allowlistFieldIfParameterized(Field field) { + allowlistParameterizedTypeArg(field.getGenericType()); + } + + /** + * Annotation retrieval logic. Can be overridden to support extending annotations or some other form of annotation + * inheritance. + */ + protected StrutsParameter getParameterAnnotation(AnnotatedElement element) { + return element.getAnnotation(StrutsParameter.class); Review Comment: There is `AnnotationUtils#isAnnotatedBy()` - wouldn't be better to use it here or instead of `getParameterAnnotation()`? Issue Time Tracking ------------------- Worklog Id: (was: 899567) Time Spent: 6h 40m (was: 6.5h) > Implement annotation mechanism for injectable fields via parameters > ------------------------------------------------------------------- > > Key: WW-5352 > URL: https://issues.apache.org/jira/browse/WW-5352 > Project: Struts 2 > Issue Type: Improvement > Components: Core, Core Interceptors > Reporter: Kusal Kithul-Godage > Priority: Minor > Fix For: 6.4.0 > > Time Spent: 6h 40m > Remaining Estimate: 0h > > struts.parameters.requireAnnotations > > Require an explicit annotation '@StrutsParameter' on one of: > Getter/Setter/Field/ReturnType for injecting parameters. > > This mechanism is intended to be a more usable replacement for > 'ParameterNameAware' -- This message was sent by Atlassian Jira (v8.20.10#820010)