fix: lowercase doctype not recognized as DOCTYPE (fixes #870) - #871
Closed
afonsojanu wants to merge 1 commit into
Closed
fix: lowercase doctype not recognized as DOCTYPE (fixes #870)#871afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
…s set The tag-type checks for DOCTYPE (in the parser and in the validator) only matched an uppercase D, so <!doctype html> from an HTML source fell through to the generic tag handler and showed up in the output as a bogus '!doctype' node instead of being skipped like a normal <!DOCTYPE ...> would be. XML itself is case sensitive here, but a lot of real-world input handed to this parser is HTML rather than strict XML, and HTML's doctype keyword is case-insensitive by spec, so it's worth treating both the same way. Made the comparison case-insensitive in both spots and added a test covering the html-style lowercase form.
Member
|
Thanks for your effort and time. But this PR is break backward compatibility. Hence, needs to be closed. Additionally, HTML is not the primary goal of this library thought it supports many features. So we'll have to tackle such cases through config or in https://github.com/nodable/flexible-xml-parser |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #870.
The check for a DOCTYPE tag only matched an uppercase
D(both in the actual parser and, separately, in the input validator), so something like this:produced
{ '!doctype': { html: '' } }instead of just{ html: '' }, because the lowercase version fell through to the normal opening-tag path and got treated as a real element.Strict XML does require DOCTYPE to be uppercase, but a lot of what gets thrown at this library in practice is HTML rather than strict XML (this is basically what the issue's own repro does), and HTML's doctype keyword has always been case-insensitive. So I made both checks case-insensitive rather than only matching
D.Also fixed the same case-sensitivity gap in
src/validator.js's comment/CDATA/DOCTYPE tag reader, since with strict validation on (parser.parse(data, true)) it threwchar 'd' is not expectedon the same input before this could even reach the actual parsing step.Added a test for the lowercase form next to the existing DOCTYPE tests in
spec/entities_spec.js. Ran the full jasmine suite locally, 326 specs pass (2 pre-existing pending/xit specs unrelated to this).