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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
