WW-5440 Fix OGNL allowlist compat with Convention plugin #986
Conversation
kusalk
force-pushed
the
WW-5440-convention
branch
from
611933c to
899b35b
Compare
kusalk
force-pushed
the
WW-5440-convention
branch
from
899b35b to
27e4d0d
Compare
|
@lukaszlenart Sorry about the massive diff on this one - it should be a bit easier to review commit by commit. Otherwise, let me know and I'll try split it up into smaller PRs |
| @@ -28,9 +28,11 @@ | |||
| * a HttpRequest parameter. | |||
| * | |||
| * @author martin.gilday | |||
| * @deprecated since 6.6.0, use {@link org.apache.struts2.interceptor.parameter.StrutsParameter}. | |||
| @@ -46,7 +46,10 @@ | |||
| * </p> | |||
| * | |||
| * @author martin.gilday | |||
| * @deprecated since 6.6.0, integrated into {@link ParametersInterceptor} with {@link StrutsParameter} using | |||
There was a problem hiding this comment.
Do we have a task to remove this interceptor in Struts 7?
There was a problem hiding this comment.
I've just updated the description of WW-5411 - if you'd prefer a separate card I can create one too
There was a problem hiding this comment.
That's fine, just to not forget about this :)
| public String execute() throws Exception { | ||
| executionCount++; | ||
| LOG.info("executing ExecutionCountTestAction. Current count is " + executionCount); | ||
| LOG.info("executing ExecutionCountTestAction. Current count is {}", executionCount); |
| try { | ||
| Object action = objectFactory.buildAction(actionName, namespace, config, null); | ||
| properties = reflectionProvider.getPropertyDescriptors(action); | ||
| } catch (Exception e) { | ||
| LOG.error("Unable to get properties for action " + actionName, e); | ||
| addActionError("Unable to retrieve action properties: " + e.toString()); | ||
| LOG.error("Unable to get properties for action {}", actionName, e); |
There was a problem hiding this comment.
This won't work, you must use ParameterizedMessage like this
LOG.error(new ParameterizedMessage("Unable to get properties for action {}", actionName), e);There was a problem hiding this comment.
So I double checked this because IntelliJ insisted this was correct despite the JavaDoc for the method suggesting it was incorrect. Turns out Log4J 2 will actually log this correctly as it will identify that the last argument is an exception (also tested locally). Here is the relevant Log4J 2 code - https://github.com/apache/logging-log4j2/blob/rel/2.23.1/log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableParameterizedMessage.java#L128
And the relevant documentation - https://logging.apache.org/log4j/2.x/manual/api.html#substituting-parameters
There was a problem hiding this comment.
Ah, nice! I were always using ParameterizedMessage in such case, good to know!
|
Thanks a lot for all your work, this is huge! One small thing and I'm good |
|
conflict |
|
Resolved! |
|
WW-5440
struts2-convention-plugincompatibility with OGNL allowlist, Actions are now auto-allowlisted as expectedstruts2-config-browser-plugincompatibility withstruts.parameters.requireAnnotations=trueAnnotationParameterFilterInterceptorwhich is superseded by@StrutsParametercapabilityI also went through and did a batch application of
@StrutsParametereven in unit test Actions in which they may not be strictly required. Did this as a precautionary measure against any other regressions or unintended test behaviour.