[
https://issues.apache.org/jira/browse/WW-5712?focusedWorklogId=1039519&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1039519
]
ASF GitHub Bot logged work on WW-5712:
--------------------------------------
Author: ASF GitHub Bot
Created on: 03/Sep/26 18:24
Start Date: 03/Sep/26 18:24
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on PR #1889:
URL: https://github.com/apache/struts/pull/1889#issuecomment-5530239163
Thanks for this, and apologies for the wait — the review is on us, not you.
The shape is what we agreed: sink-level consent on the any-setter, default
off
(`struts.rest.anySetter.requireAnnotations=false`), wrapper not even
installed unless an
application opts in. I'm happy with that part and with the `REJECTED_VALUE`
sentinel handling —
I checked that it can't leak through `PropertyValue.AnyProperty.assign` or
`AnyParameter.assign`,
since both route through `SettableAnyProperty.set`, which you override.
A couple of notes, none of them blocking.
**Depth semantics — checked, and they're right**
I went in suspecting the dynamic key might not consume its share of the
depth budget, and that the
method form and the `Map`-typed field form might disagree because
`prefixForNested`
(`AuthorizingSettableAnyProperty.java:259-265`) only appends `[0]` for the
map-like case. I wrote a
parity test for both forms and it came out clean — same grant, same body,
same reach:
METHOD depth=1 -> ACCEPTED home.city=Warsaw
FIELD depth=1 -> ACCEPTED home.city=Warsaw
which is as it should be: an any-setter on the root object takes its keys as
root-level properties,
so `{"home":{"city":"Warsaw"}}` authorizes `home` and then `home.city` — one
nesting character,
`depth = 1`, exactly the rule `@StrutsParameter` uses everywhere else. The
gate is `valueDepth`
computed on the JSON, so it doesn't vary by declaration form. Mentioning it
only so you know it was
checked rather than assumed.
The one gap it did show up: the suite pins the nested cases only for the
method form
(`testDynamicKeyDepthZero/One/TwoAnySetterBean`), while the field form is
covered for scalars alone.
Worth adding a nested field-form case so the equivalence is held by a test
rather than by
inspection — happy to hand you the one I wrote.
I should also say plainly, since it's the thing most likely to be raised at
you later: within a
granted dynamic-key scope the annotation authorizer is deliberately not
consulted for members
beneath the key, and your tests assert exactly that (a deny-everything
authorizer, nested member
still set). That is correct and consistent with the rest of the framework —
`StrutsParameterAuthorizer.isAuthorized` authorizes the root property plus a
total depth budget, so
`user.admin` needs an annotation on `user` and never on `User.admin`. No
change wanted there.
**Smaller items**
- `AuthorizingSettableAnyProperty.java:155` — `parser.currentName()` is null
when Jackson routes
through `BeanDeserializer._deserializeUsingPropertyBasedWithUnwrapped`,
which passes a value-only
`TokenBuffer`. A bean combining `@JsonUnwrapped` with a `@JsonCreator` and
an any-setter therefore
drops every dynamic property, even when correctly annotated. Fail-closed,
but silent — worth at
least a distinguishable log, ideally handling.
- `:209-211` — `allowedDepth()` short-circuits on `creatorParameter` before
looking at `permission`,
so a creator-parameter any-setter is rejected unconditionally with no
opt-in. The annotation is
discoverable there (`prop.getMember()` is the `AnnotatedParameter`); if
the intent is that it's
unsupported, the WARN should say so permanently rather than read like a
missing annotation.
- `:221` — one WARN per rejected key, and unlike bean properties the key
space is attacker-
controlled and unbounded. We've had a run of DoS-shaped reports lately, so
please log once per
body with a count instead.
- `ParameterAuthorizingModule.java:94` — `setRequireAnySetterAnnotations`
only takes effect at
deserializer-build time and Jackson caches those per mapper, so a call
after the mapper is in use
is silently a no-op. Both handlers already have the `(boolean)`
constructor; injecting the
constant there removes the hazard rather than documenting it.
- `TokenBuffer.asCopyOfValue(JsonParser)` (`:122,168,243`) is deprecated
since Jackson 2.13 in
favour of `DeserializationContext.bufferAsCopyOfValue(p)`, and `context`
is in scope at both sites.
The `@StrutsParameter` documentation correction is on me, not you — it's
tracked on WW-5712 and I'll
handle it alongside the ModelDriven wording.
One process note: because this touches parameter authorization, I'll run our
security review over
the branch before merging. That's routine for this area and not a comment on
the change.
Credit for the report and the patch is yours in the release notes.
Issue Time Tracking
-------------------
Worklog Id: (was: 1039519)
Time Spent: 40m (was: 0.5h)
> ParameterAuthorizingModule does not wrap the Jackson any-setter in the REST
> plugin
> ----------------------------------------------------------------------------------
>
> Key: WW-5712
> URL: https://issues.apache.org/jira/browse/WW-5712
> Project: Struts 2
> Issue Type: Bug
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> {{ParameterAuthorizingModule.updateBuilder}} wraps the properties returned by
> {{BeanDeserializerBuilder.getProperties()}} with
> {{AuthorizingSettableBeanProperty}}, but it never consults
> {{BeanDeserializerBuilder.getAnySetter()}}. The {{SettableAnyProperty}} is
> therefore left unwrapped, so unknown keys dispatched to a Jackson
> {{@JsonAnySetter}} are set without passing through the {{@StrutsParameter}}
> authorization context — while an ordinary unannotated setter on the same
> class, in the same request body, is correctly rejected.
> All three any-setter forms are affected (method, field and {{@JsonCreator}}
> parameter). Both {{JacksonJsonHandler}} and {{JacksonXmlHandler}} register
> the module, so JSON and XML bodies behave the same. {{@JsonUnwrapped}} is not
> affected, and an any-setter beneath a rejected parent is already skipped
> through {{skipChildren()}}.
> The obvious fix does not work: path-based authorization resolves a
> parameter's root name against declared members of the target, so
> {{pathFor(key)}} matches nothing for a dynamic key by construction. Routing
> any-setter keys through the existing check would reject every key and break
> every existing {{@JsonAnySetter}} user. Consent has to be expressed at the
> sink rather than at the key.
> Proposed shape:
> * A new annotation member, {{@StrutsParameter(allowDynamicKeys = true)}},
> marking one any-setter as permitted to receive arbitrary keys. A plain
> {{@StrutsParameter}} is deliberately not sufficient — arbitrary-key access
> should have to be typed out, not granted by accident.
> * Enforcement behind a plugin-local constant,
> {{struts.rest.anySetter.requireAnnotations}}, defaulting to {{false}}, so
> nothing changes for existing applications until they opt in. Same shape as
> {{struts.chaining.requireAnnotations}}; the default can be revisited in a
> major release.
> * {{depth()}} keeps its current meaning, counted beneath the dynamic key. The
> default {{depth = 0}} accepts a scalar value under any key and rejects a
> nested object under it; {{depth = 1}} permits one level of nesting. That is
> the rule an annotated {{Map}} property already follows.
> * {{@Target}} stays {{METHOD, FIELD}}. A {{@JsonCreator}}-parameter
> any-setter has nowhere to carry the marker and is rejected with a warning
> when enforcement is on. Narrowing that rare form now is preferable to
> advertising an annotation placement the framework does not honour elsewhere,
> and {{@Target}} can be widened later but never narrowed.
> * The wrapper mirrors {{AuthorizingSettableBeanProperty}}: delegate untouched
> when the authorization context is inactive; on rejection log a warning, mark
> the context redacted and call {{skipChildren()}} so nested graphs are never
> constructed.
> Test coverage should include default-off compatibility, the method and field
> forms, missing and explicit consent, depth 0/1/2, creator parameters,
> unauthorized parents, {{@JsonUnwrapped}}, both the JSON and XML handlers, and
> context cleanup.
> Documentation follow-up: the {{@StrutsParameter}} page states that the JSON
> and REST plugins authorize each property during deserialization "so that
> unauthorized fields are never set". That is not accurate as written — correct
> the wording and document the any-setter behaviour explicitly.
> Reported by Darren Carreras (GitHub: carrerasdarren-cell), who has a patch
> implementing the above and will open the pull request.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
