Skip to content

stop dropping ENTITY_REFERENCE events from text in XmlTokenStream - #901

Open
Sahana2524 wants to merge 1 commit into
FasterXML:3.xfrom
Sahana2524:entity-reference-text
Open

stop dropping ENTITY_REFERENCE events from text in XmlTokenStream#901
Sahana2524 wants to merge 1 commit into
FasterXML:3.xfrom
Sahana2524:entity-reference-text

Conversation

@Sahana2524

Copy link
Copy Markdown
Contributor

Running the new EntityReferenceReadTest against the unpatched tree:

EntityReferenceReadTest.testDeclaredEntityNotReplacedByReader:56 expected: <fooxxbar> but was: <foobar>
EntityReferenceReadTest.testUndeclaredEntityNotReplacedByReader:73 Expected tools.jackson.core.exc.StreamReadException to be thrown, but nothing was thrown.

With a reader that has IS_REPLACING_ENTITY_REFERENCES disabled (Woodstox honors it, and the same goes for any reader handed to createParser(XMLStreamReader)), &e; inside element text arrives as an ENTITY_REFERENCE event, and XmlTokenStream only collects CHARACTERS/CDATA, so the reference just vanished from the value with no error. Now it contributes the reader's replacement text, and fails with a located StreamReadException when the reader has none (undeclared entity), which seemed better than a quietly truncated string. Default readers expand entities themselves and never produce these events, so nothing changes there. The same code is on 2.x and 3.1; happy to retarget if you would rather have it land there first.

@github-actions

Copy link
Copy Markdown

🧪 Code Coverage Report

Metric Coverage Change
Instructions coverage 74.21% 📈 +0.130%
Branches branches 68.89% 📈 +0.170%

Coverage data generated from JaCoCo test results

@pjfanning

Copy link
Copy Markdown
Member

entity references are really frowned on - turned out be a real security issue - so adding back support for them is probably not something that we would likely want to backport

anything that gets added needs to be thoroughly tested to avoid reopening security attack vectors

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.

2 participants