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

Swap in defusedxml.parseString #35417

Merged
merged 2 commits into from
Nov 25, 2024
Merged

Swap in defusedxml.parseString #35417

merged 2 commits into from
Nov 25, 2024

Conversation

minhaminha
Copy link
Contributor

@minhaminha minhaminha commented Nov 21, 2024

Technical Summary

Jira Ticket
Code Scan alert

This PR swaps out lxml.fromString to defusedxml.minidom.parseString to mitigate risks of an XXE attack.

Safety Assurance

Safety story

Manually tested in shell using the example xml string '<bspostevent><field name="MobileNumber" type="string">000000000</field><field name="Text" type="string">sample text</field></bspostevent>' and verified it returns the same names, numbers and text.

QA Plan

I don't have a way of testing this locally so I think it's a good idea to ask QA to take a quick pass on it.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@minhaminha minhaminha added Open for review: do not merge A work in progress product/invisible Change has no end-user visible impact labels Nov 21, 2024
Copy link
Contributor

@esoergel esoergel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think QA will meaningfully be able to test this, unfortunately. It looks like the backend for some sms provider called "push", so we'd need access to that. Automated tests might be the best bet - it looks like the existing (presumed functional) implementation is super simple, so you could probably infer what the XML this receives looks like and then just ensure that both before and after handle it the same way.

@MartinRiese
Copy link
Contributor

Looks like you are in luck and there already is a test that now fails:

FAILED corehq/apps/sms/tests/test_backends.py::AllBackendTest::test_push_inbound_sms - AttributeError: 'Text' object has no attribute 'getAttribute'

@minhaminha
Copy link
Contributor Author

@esoergel gotcha. I used the sample xml string found in the previously failing test (also updated in the description above) to test the function before and after my changes - does that look right to you? (I did chop off the first ?xml tag since trying to parse as is with etree.fromstring triggers a valueerror)

Copy link
Contributor

@esoergel esoergel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hooray for tests!

@minhaminha minhaminha merged commit ba320e4 into master Nov 25, 2024
12 of 13 checks passed
@minhaminha minhaminha deleted the ml/use-defusedxml branch November 25, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Open for review: do not merge A work in progress product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants