[
https://issues.apache.org/jira/browse/WW-5715?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5715:
------------------------------
Description:
Found while triaging WW-5712; independent of it and present on main today.
{{AuthorizingSettableBeanProperty.deserializeAndSet}} (line 85) and
{{deserializeSetAndReturn}} (line 105) derive the authorization path from
{{getName()}}, which is Jackson's _external_ property name — the key as it
appears in the request body. The authorizer on the other side resolves that
string against the _Java member_:
{{StrutsParameterAuthorizer.hasValidAnnotatedMember}} introspects the target
for a bean property or public field of that name and reads its
{{@StrutsParameter}}.
The two names are identical until the application renames a property —
{{@JsonProperty}}, {{@JsonAlias}}, or a configured {{PropertyNamingStrategy}}.
Once they diverge, the authorization decision is made about one member while
the value is assigned to another.
Two directions, both wrong:
_Wrongly denied._ With a snake-case naming strategy, an annotated member
{{userName}} arrives as {{user_name}}. The authorizer looks for a member called
{{user_name}}, finds none, and rejects — a correctly annotated property is
silently dropped, and the WARN names a property the developer never wrote.
_Wrongly allowed._ Given an unannotated setter renamed onto an annotated
member's name:
@JsonProperty("name")
public void setAdmin(boolean admin) { ... } // not annotated
@StrutsParameter
public void setName(String name) { ... } // annotated
a request body that sends the key {{name}} with the value {{true}} authorizes
the path {{name}}, which _is_ annotated, and Jackson then routes the value
through the external name to {{setAdmin}}. The member that receives the value
was never the member that was authorized.
The same derivation feeds {{AuthorizingValueDeserializer}} (line 75) for
creator properties, so constructor-bound parameters inherit the behaviour.
Scope: this needs the application to rename properties, and for the permissive
direction the renaming has to land on an annotated member's name, so it is not
a default-configuration issue. Filed as a correctness gap in the control rather
than anything more.
Fix shape: derive the path from the Java member — {{getMember()}}, the way
{{AuthorizingSettableAnyProperty}} already does when it reads the annotation —
rather than the wire name. Note that the path prefix for nested objects is
currently built from wire names, so member-based and prefix-based naming have
to be made consistent, not changed in one place only.
A regression test should cover both directions: an annotated member renamed by
a naming strategy must still bind, and an unannotated member renamed onto an
annotated member's name must not.
was:
Found while triaging WW-5712; independent of it and present on main today.
{{AuthorizingSettableBeanProperty.deserializeAndSet}} (line 85) and
{{deserializeSetAndReturn}} (line 105) derive the authorization path from
{{getName()}}, which is Jackson's _external_ property name — the key as it
appears in the request body. The authorizer on the other side resolves that
string against the _Java member_:
{{StrutsParameterAuthorizer.hasValidAnnotatedMember}} introspects the target
for a bean property or public field of that name and reads its
{{@StrutsParameter}}.
The two names are identical until the application renames a property —
{{@JsonProperty}}, {{@JsonAlias}}, or a configured {{PropertyNamingStrategy}}.
Once they diverge, the authorization decision is made about one member while
the value is assigned to another.
Two directions, both wrong:
_Wrongly denied._ With a snake-case naming strategy, an annotated member
{{userName}} arrives as {{user_name}}. The authorizer looks for a member called
{{user_name}}, finds none, and rejects — a correctly annotated property is
silently dropped, and the WARN names a property the developer never wrote.
_Wrongly allowed._ Given an unannotated setter renamed onto an annotated
member's name:
@JsonProperty("name")
public void setAdmin(boolean admin) { ... } // not annotated
@StrutsParameter
public void setName(String name) { ... } // annotated
a body of {{{"name": true}}} authorizes the path {{name}}, which _is_
annotated, and Jackson then routes the value through the external name to
{{setAdmin}}. The member that receives the value was never the member that was
authorized.
The same derivation feeds {{AuthorizingValueDeserializer}} (line 75) for
creator properties, so constructor-bound parameters inherit the behaviour.
Scope: this needs the application to rename properties, and for the permissive
direction the renaming has to land on an annotated member's name, so it is not
a default-configuration issue. Filed as a correctness gap in the control rather
than anything more.
Fix shape: derive the path from the Java member — {{getMember()}}, the way
{{AuthorizingSettableAnyProperty}} already does when it reads the annotation —
rather than the wire name. Note that the path prefix for nested objects is
currently built from wire names, so member-based and prefix-based naming have
to be made consistent, not changed in one place only.
A regression test should cover both directions: an annotated member renamed by
a naming strategy must still bind, and an unannotated member renamed onto an
annotated member's name must not.
> REST body authorization keys off the Jackson external property name instead
> of the Java member
> ----------------------------------------------------------------------------------------------
>
> Key: WW-5715
> URL: https://issues.apache.org/jira/browse/WW-5715
> Project: Struts 2
> Issue Type: Bug
> Components: Plugin - REST
> Reporter: Lukasz Lenart
> Priority: Minor
> Fix For: 7.4.0
>
>
> Found while triaging WW-5712; independent of it and present on main today.
> {{AuthorizingSettableBeanProperty.deserializeAndSet}} (line 85) and
> {{deserializeSetAndReturn}} (line 105) derive the authorization path from
> {{getName()}}, which is Jackson's _external_ property name — the key as it
> appears in the request body. The authorizer on the other side resolves that
> string against the _Java member_:
> {{StrutsParameterAuthorizer.hasValidAnnotatedMember}} introspects the target
> for a bean property or public field of that name and reads its
> {{@StrutsParameter}}.
> The two names are identical until the application renames a property —
> {{@JsonProperty}}, {{@JsonAlias}}, or a configured
> {{PropertyNamingStrategy}}. Once they diverge, the authorization decision is
> made about one member while the value is assigned to another.
> Two directions, both wrong:
> _Wrongly denied._ With a snake-case naming strategy, an annotated member
> {{userName}} arrives as {{user_name}}. The authorizer looks for a member
> called {{user_name}}, finds none, and rejects — a correctly annotated
> property is silently dropped, and the WARN names a property the developer
> never wrote.
> _Wrongly allowed._ Given an unannotated setter renamed onto an annotated
> member's name:
> @JsonProperty("name")
> public void setAdmin(boolean admin) { ... } // not annotated
> @StrutsParameter
> public void setName(String name) { ... } // annotated
> a request body that sends the key {{name}} with the value {{true}} authorizes
> the path {{name}}, which _is_ annotated, and Jackson then routes the value
> through the external name to {{setAdmin}}. The member that receives the value
> was never the member that was authorized.
> The same derivation feeds {{AuthorizingValueDeserializer}} (line 75) for
> creator properties, so constructor-bound parameters inherit the behaviour.
> Scope: this needs the application to rename properties, and for the
> permissive direction the renaming has to land on an annotated member's name,
> so it is not a default-configuration issue. Filed as a correctness gap in the
> control rather than anything more.
> Fix shape: derive the path from the Java member — {{getMember()}}, the way
> {{AuthorizingSettableAnyProperty}} already does when it reads the annotation
> — rather than the wire name. Note that the path prefix for nested objects is
> currently built from wire names, so member-based and prefix-based naming have
> to be made consistent, not changed in one place only.
> A regression test should cover both directions: an annotated member renamed
> by a naming strategy must still bind, and an unannotated member renamed onto
> an annotated member's name must not.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
