Skip to content
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

[DRAFT] deep nesting check #576

Closed
wants to merge 4 commits into from
Closed

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Mar 5, 2023

relies on FasterXML/jackson-core#943

It may be better to doc that StreamReadConstraints is ignored for jackson-dataformat-xml and that users are better off using WstxInputFactory and creating XmlMapper using an instance of WstxInputFactory.

WstxInputFactory.getConfig() gives you a ReaderConfig which already has a default max element depth of 1000.

https://fasterxml.github.io/woodstox/javadoc/6.0/com/ctc/wstx/api/ReaderConfig.html

@pjfanning pjfanning self-assigned this Mar 5, 2023
@pjfanning pjfanning changed the title deep nesting check [DRAFT] deep nesting check Mar 5, 2023
@pjfanning pjfanning marked this pull request as draft March 5, 2023 20:04
@pjfanning
Copy link
Member Author

closing as you can use woodstox to do the nesting check

@pjfanning pjfanning closed this Mar 6, 2023
@cowtowncoder
Copy link
Member

Yeah this is tricky. On one hand I agree, use of native facilities is generally better way. At the same time, shared configurability and checking has its benefits; including but not limited to Jackson-specific exceptions, context.

To further complicate things, theoretically nesting levels between actual XML and logical "converted" JSON events can have slight discrepancy too.

So for now I think let's use Jackson's new checks and not change Woodstox defaults. We can revisit this in future as necessary.

@pjfanning
Copy link
Member Author

pjfanning commented Mar 7, 2023

Maybe worth having the 2 ways to check for depth (woodstox and jackson-specific). I'm still going to treat this as a lower priority than adding nesting check for Smile and CBOR formats. Possibly, other dataformats too.

There is already a StreamReadConstraints setting that is not implemented as Jackson-specific - that you need to use Woodstox config - #571

StreamReadConstraints number len check is applied though

@cowtowncoder
Copy link
Member

Ah. Yes... String length checks would be much more efficient to apply at Woodstox level.

@pjfanning pjfanning deleted the deep-nesting branch March 13, 2023 10:03
pjfanning pushed a commit to pjfanning/jackson-dataformat-xml that referenced this pull request Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants