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

Fix code scanning alert no. 6: Resolving XML external entity in user-controlled data #3251

Merged
merged 2 commits into from
Sep 26, 2024

Conversation

hazendaz
Copy link
Member

Fixes https://github.com/mybatis/mybatis-3/security/code-scanning/6

To fix the problem, we need to disable the parsing of DTDs and external entities in the DocumentBuilderFactory. This can be done by setting specific features on the DocumentBuilderFactory instance. The changes should be made in the createDocument method of the XPathParser class.

  1. Disable DTDs by setting the feature http://apache.org/xml/features/disallow-doctype-decl to true.
  2. Disable external general entities by setting the feature http://xml.org/sax/features/external-general-entities to false.
  3. Disable external parameter entities by setting the feature http://xml.org/sax/features/external-parameter-entities to false.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

…controlled data

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@hazendaz hazendaz marked this pull request as ready for review September 26, 2024 00:07
remove extra items, AI didn't add them on ibatis-2 so why here.  Just remove to see if it works then.
@coveralls
Copy link

Coverage Status

coverage: 87.113% (-0.009%) from 87.122%
when pulling 650146b on autofix/alert-6-2d7812d9b9
into 45953df on master.

@hazendaz
Copy link
Member Author

@harawata Any idea why we would have this value set to true when its known to be vulnerable? ibatis-2 has that too. Switching it all tests work. I'm wanting to merge it as it is the right thing to do but trying to understand who/why we would have had that for probably 20 years since ibatis has same issue.

@harawata
Copy link
Member

Thank you, @hazendaz ,

I checked the logs, but couldn't find anything (and the default value is true actually).
Let's merge this PR and see what happens.

@hazendaz hazendaz merged commit 7e8d836 into master Sep 26, 2024
34 of 36 checks passed
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.

3 participants