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

Fixing cgi.escape DeprecationWarning #23939

Merged
merged 1 commit into from
Jun 23, 2020

Conversation

leomartinez2020
Copy link
Contributor

@leomartinez2020 leomartinez2020 requested a review from a team May 7, 2020 16:52
@openedx-webhooks
Copy link

Thanks for the pull request, @leomartinez2019! I've created OSPR-4490 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels May 7, 2020
@morenol
Copy link
Contributor

morenol commented May 8, 2020

Hi @natabene, could you trigger the tests?

@natabene
Copy link
Contributor

natabene commented May 8, 2020

jenkins run all

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels May 8, 2020
@leomartinez2020 leomartinez2020 requested a review from a team May 18, 2020 04:45
@leomartinez2020 leomartinez2020 requested a review from a team as a code owner May 18, 2020 04:45
@leomartinez2020 leomartinez2020 requested review from a team May 18, 2020 04:45
@openedx-webhooks
Copy link

@leomartinez2019 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@leomartinez2020 leomartinez2020 deleted the lmz/fixing_cgi_escape_warning branch May 18, 2020 05:20
@leomartinez2020 leomartinez2020 restored the lmz/fixing_cgi_escape_warning branch May 18, 2020 06:31
@natabene
Copy link
Contributor

jenkins run all

@nedbat
Copy link
Contributor

nedbat commented May 19, 2020

jenkins run quality

@natabene
Copy link
Contributor

@leomartinez2019 Please address the failing quality test.

@leomartinez2020
Copy link
Contributor Author

leomartinez2020 commented May 20, 2020

@leomartinez2019 Please address the failing quality test.

Jenkins reports these quality warnings:
common/lib/xmodule/xmodule/poll_module.py: 218:21: python-wrap-html: b" poll_str = u'<{tag_name}>{text}</{tag_name}>'.format("

common/lib/xmodule/xmodule/poll_module.py: 226:26: python-wrap-html: b' child_str = u\'<{tag_name} id="{id}">{text}</{tag_name}>\'.format('

Here the solution is suggested:
https://edx.readthedocs.io/projects/edx-developer-guide/en/latest/preventing_xss/preventing_xss.html#python-wrap-html

Which consists of using the function HTML() from: openedx.core.djangolib.markup
I can update this branch to address these two warnings

With the suggested changes this test passes:
./scripts/xsslint/xss_linter.py common/lib/xmodule/xmodule/poll_module.py

0 violations total

@nedbat
Copy link
Contributor

nedbat commented May 20, 2020

jenkins run all

@leomartinez2020
Copy link
Contributor Author

Unfortunately when this test passes:
./scripts/xsslint/xss_linter.py common/lib/xmodule/xmodule/poll_module.py

This other fails:
pytest common/lib/xmodule/xmodule/tests/test_poll.py
With the message: AssertionError: '&lt; 18' != '< 18'

And viceversa, so I am still researching to find a solution

@morenol
Copy link
Contributor

morenol commented May 29, 2020

@leomartinez2019

I would say that &lt; is the escaped version of <. So, I think that the way to solve this is just to change the assertion in the test

@nedbat
Copy link
Contributor

nedbat commented May 29, 2020

It's not that simple. The assertion is a real failure.

AssertionError: '&lt; 18' != '< 18'

In this case, the XML actually contains &amp;lt; 18, so that the interpreted text is &lt;

@morenol morenol mentioned this pull request Jun 22, 2020
@natabene
Copy link
Contributor

jenkins run all

@morenol morenol force-pushed the lmz/fixing_cgi_escape_warning branch from dddc56b to 8cea844 Compare June 22, 2020 14:57
@morenol
Copy link
Contributor

morenol commented Jun 22, 2020

Hi @natabene, could you trigger them again?

@awais786
Copy link
Contributor

jenkins run all

@awais786
Copy link
Contributor

jenkins run py38 python

@natabene
Copy link
Contributor

jenkins run all

@awais786
Copy link
Contributor

jenkins run py38 python

1 similar comment
@jmbowman
Copy link
Contributor

jenkins run py38 python

- Change cgi.escape to html.escape

- Add quote=False to html.escape

- Use function HTML() to address python-wrap-html warning
@morenol morenol force-pushed the lmz/fixing_cgi_escape_warning branch from 8cea844 to 943bd6f Compare June 22, 2020 18:42
@jmbowman
Copy link
Contributor

jenkins run all
jenkins run py38 python

@jmbowman
Copy link
Contributor

jenkins run py38 python

@edx-status-bot
Copy link

Your PR has finished running tests. The following contexts failed:

  • jenkins/python-3.8/python

@awais786 awais786 merged commit e34b052 into openedx:master Jun 23, 2020
@openedx-webhooks
Copy link

@leomartinez2019 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@openedx-webhooks openedx-webhooks added merged and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants