-
Notifications
You must be signed in to change notification settings - Fork 813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WW-5379 Implement alternative mechanism for Velocity directives to obtain ValueStack #822
Conversation
5c17230
to
450ee91
Compare
(Again, SonarCloud is using the wrong base) |
@@ -96,11 +96,15 @@ protected Object chainedContextGet(String key) { | |||
return null; | |||
} | |||
for (VelocityContext chainedContext : chainedContexts) { | |||
Object val = chainedContext.internalGet(key); | |||
Object val = chainedContext.get(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was technically a bug as we were failing to look at chained contexts within our chained contexts
// get the bean | ||
ValueStack stack = (ValueStack) ctx.get("stack"); | ||
ValueStack stack = extractValueStack(ctx); | ||
if (stack == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't occur but left it for backwards compatibility in case there is a scenario I didn't foresee
public ValueStack getValueStack() { | ||
return stack; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to have this public? Also this requires to cast to StrutsVelocityContext
, wouldn't be better to have a marking interface ValueStackAware
or ValueStackProvider
to expose the ValueStack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it has to be but I wonder if Directives can simply obtain the ValueStack from the ActionContext? It's not clear to me if the ValueStack on the ActionContext changes between the time of Velocity context creation and directive rendering. I recall this was how the ValueStack was obtained in WebWork 2.1 but I presume it was changed for a reason?
But also, there's no change in terms of security as the stack was already exposed on the StrutsVelocityContext
instance using internalGet("stack")
or get("stack")
.
And yep I can definitely use a marker interface to allow more flexibility in the Velocity context implementation used by applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't change as far I understand, yet I'm not sure if this is true :) Let's leave it as is, at some point I will solve this puzzle ;-)
…implementation flexibility
29027f3
to
ee600f9
Compare
Quality Gate failedFailed conditions 10 Security Hotspots See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
WW-5379
This affords applications the ability to not include the ValueStack as
$stack
in the Velocity context and reduce risk of SSTI escalation.