bpo-35603: Escape table header of make_table output that can cause potential XSS by tirkarthi · Pull Request #11341 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation7 Commits3 Checks0 Files changed
Conversation
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
[ Show hidden characters]({{ revealButtonHref }})
fromdescandtodescshould be escaped since rendering unescaped HTML might lead to code execution in the browser rendering them as tags.
https://bugs.python.org/issue35603
| @@ -2036,6 +2036,10 @@ def make_table(self,fromlines,tolines,fromdesc='',todesc='',context=False, |
|---|
| s.append( fmt % (next_id[i],next_href[i],fromlist[i], |
| next_href[i],tolist[i])) |
| if fromdesc or todesc: |
| fromdesc = fromdesc.replace("&", "&").replace(">", ">") \ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use html.escape()?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about the same but text which was encoded was using the logic inlined and there was some discussion in the issue about just inlining the logic to avoid dependency at https://bugs.python.org/issue914575#msg45518 which I realize now is about xml.sax and not html.
html module was also importing entities at
| from html.entities import html5 as _html5 |
|---|
that seemed little import heavy for me. But if it's okay to import html then I think it's good to use html.escape across the module to avoid duplicate code.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, for a bug fix we can keep an inlined code and consider using html.escape() in a separate issue.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, thanks for the review.
| @@ -2036,6 +2036,10 @@ def make_table(self,fromlines,tolines,fromdesc='',todesc='',context=False, |
|---|
| s.append( fmt % (next_id[i],next_href[i],fromlist[i], |
| next_href[i],tolist[i])) |
| if fromdesc or todesc: |
| fromdesc = fromdesc.replace("&", "&").replace(">", ">") \ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, for a bug fix we can keep an inlined code and consider using html.escape() in a separate issue.
Thanks @tirkarthi for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
…tential XSS. (pythonGH-11341)
(cherry picked from commit 78de011)
Co-authored-by: Xtreak tir.karthi@gmail.com
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
…tential XSS. (pythonGH-11341)
(cherry picked from commit 78de011)
Co-authored-by: Xtreak tir.karthi@gmail.com