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

Set Load external DTD feature to be enabled #3605

Closed
liutikas opened this issue Dec 2, 2016 · 9 comments
Closed

Set Load external DTD feature to be enabled #3605

liutikas opened this issue Dec 2, 2016 · 9 comments

Comments

@liutikas
Copy link

liutikas commented Dec 2, 2016

I realize this is a somewhat odd request, but here it goes. In our project (Android OS) we use ENTITY in our config to allow composing our config files from many different files as some subprojects of Android OS have different style requirements. This normally works great with Checkstyle, however our default java installation has load-external-dtd feature disabled (due to security reasons). This is a request to add
final SAXParserFactory factory = SAXParserFactory.newInstance(); factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", true);

to com.puppycrawl.tools.checkstyle.api.AbstractLoader.

This would not affect anyone, except to allow us to keep using Checkstyle without any downstream modifications.

both features load-external-dtd and external-general-entities are normally default set to true. Our company has set their java defaults to false as it can lead to cross scripting attacks if not handled correctly. load-external-dtd feature allows to load external DTD into a an XML document and external-general-entities feature allows to these these external DTDs in elements. Using these two we are able to compose configuration files from multiple XML files. Since both of these features are enabled by default in default java set ups it should be a no-op for most checkstyle users.

https://xerces.apache.org/xerces2-j/features.html#nonvalidating.load-external-dtd

Execution is done by CLI - https://android.googlesource.com/platform/prebuilts/checkstyle/+/7de5d4001767b7929fae914e088a724af9597c1a/checkstyle.py#164


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@romani
Copy link
Member

romani commented Dec 2, 2016

https://docs.oracle.com/javase/tutorial/jaxp/properties/scope.html

https://xerces.apache.org/xerces2-j/features.html#nonvalidating.load-external-dtd , default is "true"... but in issue specified as disabled in java installation due to security reason.

Should be fine to do such update as we actually enforce default value.

@romani romani added the approved label Dec 2, 2016
@romani
Copy link
Member

romani commented Dec 2, 2016

@liutikas , please provide PR with update.

@romani
Copy link
Member

romani commented Dec 6, 2016

fix is merged.

@romani
Copy link
Member

romani commented Dec 9, 2018

@liutikas , can you share an example on how different projects override path to ENTITY ?

@liutikas
Copy link
Author

@romani
Copy link
Member

romani commented Dec 11, 2018

@liutikas , yes, I got this from issue description.

to allow composing our config files from many different files as some subprojects of Android OS have different style requirements.

Please share a link to project that override default.

@romani
Copy link
Member

romani commented Jan 31, 2019

@liutikas , we are planning to disable(set to "false") such features by default and provide CLI parameter to enable them. Please let me know if this might be problem by you.

@liutikas
Copy link
Author

liutikas commented Feb 1, 2019

Sure, disable it.

@romani
Copy link
Member

romani commented Feb 25, 2019

@liutikas , changed at scope of #6474 , released as 8.18.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants