WW-5698 Scope the ModelDriven exemption in StrutsParameterAuthorizer to the model object - #1872
Open
lukaszlenart wants to merge 2 commits into
Open
WW-5698 Scope the ModelDriven exemption in StrutsParameterAuthorizer to the model object#1872lukaszlenart wants to merge 2 commits into
lukaszlenart wants to merge 2 commits into
Conversation
isAuthorized returned true for every parameter name once the action implemented ModelDriven. OGNL then resolves that name against the whole CompoundRoot, which holds the model on top of the action, so authorization was decided about the model while the write could land on the action. In effect the @StrutsParameter requirement did not apply to a ModelDriven action's own members: an unannotated setter declared on the action was bound, where the identical setter on a plain action is rejected. The exemption now covers what it was meant to cover. A property declared by the model is exempt, since returning an object from getModel() declares it request surface. A property declared by the action is subject to the annotation requirement as usual. A property declared by neither is still allowed, because it cannot be reaching a member of the action - that case is typically a model bound through a custom OGNL property accessor, such as a Map-backed model, and rejecting it would break those applications. The model is checked first so that a model property shadowing an action property still binds without an annotation, matching OGNL's own resolution against the stack top. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ModelDriven branch returned before the transition mode check, so requireAnnotations.transitionMode never applied to a ModelDriven action. That did not matter while the exemption authorized everything, but once it is scoped to the model the action's own members are rejected, and those are exactly the members transition mode exists to keep binding during migration. Checking transition mode first gives the affected applications the same migration path they would have on any other action. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Scopes Struts’ ModelDriven parameter authorization exemption to properties declared on the model object, ensuring @StrutsParameter requirements still apply to action members and restoring transition-mode behavior for ModelDriven actions.
Changes:
- Reorders and refines
ModelDrivenhandling to authorize model-declared properties while applying annotation checks to action-declared properties. - Moves transition-mode depth-0 exemption earlier so it also applies to ModelDriven actions.
- Adds regression tests covering ModelDriven action-member rejection/authorization, shadowing, “declared on neither” compatibility, and transition mode.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java | Implements model-scoped ModelDriven exemption logic and reorders transition-mode evaluation. |
| core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java | Adds test cases validating the new ModelDriven scoping and transition-mode behavior. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+173
to
+178
| protected boolean declaresProperty(Object target, String property) { | ||
| BeanInfo beanInfo = getBeanInfo(target); | ||
| if (beanInfo != null && Arrays.stream(beanInfo.getPropertyDescriptors()) | ||
| .anyMatch(desc -> desc.getName().equals(property))) { | ||
| return true; | ||
| } |
| return true; | ||
| } | ||
| int nestingIndex = indexOfAny(parameterName, NESTING_CHARS_STR); | ||
| String rootProperty = nestingIndex == -1 ? parameterName : parameterName.substring(0, nestingIndex); |
| return true; | ||
| } | ||
| try { | ||
| return Modifier.isPublic(ultimateClass(target).getDeclaredField(property).getModifiers()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes WW-5698
Targets 7.4.0. This is a behavioural change (see Compatibility), and
requireAnnotations.transitionModeis the supported migration path for applications it affects — that turned out not to be a separate gating decision, because transition mode already is that lever once wired correctly (see the transition mode section).Problem
StrutsParameterAuthorizer.isAuthorized(...)returnedtruefor any parameter name once the action implementedModelDriven:OGNL then resolves that name against the whole
CompoundRoot, which holds the model on top of the action. Authorization was decided about the model while the write could land on the action. The practical result: the@StrutsParameterrequirement did not apply to aModelDrivenaction's own members.Same unannotated setter, declared on the action class in both cases, with
struts.parameters.requireAnnotations=true:Change
The exemption now covers what it was meant to cover:
getModel()declares it request surface, and that is the whole point of the exemptionThat third case matters for compatibility. A model bound through a custom OGNL property accessor — a Map-backed model, most commonly — declares no bean property, and such a name cannot be reaching a member of the action either. Rejecting it would break those applications, so it is explicitly allowed.
The model is checked first, so a model property that shadows an action property still binds without an annotation, matching OGNL's own resolution against the stack top.
Compatibility
An application whose
ModelDrivenaction relies on binding unannotated members declared on the action will stop binding them and will need those members annotated with@StrutsParameter. That is the same migration those members would have needed had the action not beenModelDriven. Model binding itself is unchanged.Transition mode
requireAnnotations.transitionModeexempts depth-0 parameters so an application can enablerequireAnnotationswhile it works through annotating. It was checked after the ModelDriven branch, so it never applied to a ModelDriven action at all. That was harmless while the exemption authorized everything, but it means the actions this change affects would have had no migration path — the one lever built for this situation was unreachable for exactly the actions that need it.It is now checked first, so an application broken by this change can set
requireAnnotations.transitionMode=trueand its depth-0 action members keep binding, which is the same migration path any other action already has. This is also why no new opt-out flag is proposed.Tests
Seven new cases in
ParameterAuthorizerTest, covering the rejection, the annotated action member, the model property, the shadowed property, and the declared-on-neither escape.Both new branches were mutation-checked rather than trusted because they passed:
modelDriven_targetIsModel_allAuthorized— so that escape is what preserves existing behaviourGreen:
core3202,json166,rest124 (includingParameterAuthorizingModuleTest).Note: a full-reactor
mvn testcurrently fails to compilestruts2-tiles-plugin(package org.apache.velocity.tools.view does not exist). That is pre-existing — it reproduces identically on unmodifiedmain— and unrelated to this change, but it does mean the four modules after tiles were not exercised.Related
WW-5697 / #1871 came from the same triage. Different cause, different fix; the two overlap only in that a
ModelDrivenaction is the easiest way to reach both.🤖 Generated with Claude Code