10000 [3.5] bpo-38243, xmlrpc.server: Escape the server_title (GH-16373) (GH-16441) by vstinner · Pull Request #16516 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

[3.5] bpo-38243, xmlrpc.server: Escape the server_title (GH-16373) (GH-16441) #16516

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 29, 2019
Merged

[3.5] bpo-38243, xmlrpc.server: Escape the server_title (GH-16373) (GH-16441) #16516

merged 2 commits into from
Oct 29, 2019

Conversation

vstinner
Copy link
Member
@vstinner vstinner commented Oct 1, 2019 •
edited by bedevere-bot
Loading

Escape the server title of xmlrpc.server.DocXMLRPCServer
when rendering the document page as HTML.

(cherry picked from commit e8650a4)
(cherry picked from commit 1698cac)

https://bugs.python.org/issue38243

@vstinner vstinner added the type-security A security issue label Oct 1, 2019
@vstinner
Copy link
Member Author
vstinner commented Oct 1, 2019

Let's see if this PR also fails because of the Sphinx issue :-)

cc @larryhastings

@vstinner
Copy link
Member Author
vstinner commented Oct 1, 2019

The doc job of Travis CI fails: see https://bugs.python.org/issue38339

@vstinner
bpo-38243, xmlrpc.server: Escape the server_title (GH-16373) (GH-16441)
84b60fc 
Escape the server title of xmlrpc.server.DocXMLRPCServer
when rendering the document page as HTML.

(cherry picked from commit e8650a4)
(cherry picked from commit 1698cac)
@vstinner
Copy link
Member Author
vstinner commented Oct 8, 2019

I rebased my PR on top of the commit edd9bc9 to fix the doc job of Travis CI.
8000

@corona10
Copy link
Member

This PR now looks okay to merge it :)

aeros
aeros reviewed Oct 15, 2019
View reviewed changes
Copy link
Contributor
@aeros aeros left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Minor readability recommendation to use serv = self.serv:

    def test_server_title_escape(self):
        # bpo-38243: Ensure that the server title and documentation
        # are escaped for HTML.
        serv = self.serv
        serv.set_server_title('test_title<script>')
        serv.set_server_documentation('test_documentation<script>')
        self.assertEqual('test_title<script>', serv.server_title)
        self.assertEqual('test_documentation<script>',
                serv.serv_documentation)

        generated = serv.generate_html_documentation()
        # ...

This isn't at all critical as far as functionality goes, but it makes it easier to read by reducing some of the white noise. This is frequently done in other areas, such as the asyncio tests. For example, loop = self.loop; loop.create_future() instead of self.loop.create_future(), when the same instance attribute is repeatedly referenced (generally 3+ times, in this case it's 5).

Edit: Never mind, this would have been addressed in the original PR #16373 and not just for the backport. It's probably not important enough to be worth adjusting separately. I only saw this specific PR because of the message from @vstinner in python-dev.

@vstinner
Copy link
Member Author

This PR now looks okay to merge it :)

says @corona10, the author of the origin fix in the master branch e8650a4 ;-)

Minor readability recommendation (...)

This change is a backport to an old security branch (3.5). If you would like to enhance the readability, please propose a change on the master branch ;-)

@vstinner
Copy link
Member Author

@corona10: Would you mind to use a GitHub review to officially "approve" the change? It's only to track than comments.

corona10
corona10 approved these changes Oct 15, 2019
View reviewed changes
Copy link
Member
@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

This change is a backport to an old security branch (3.5). If you would like to enhance the readability, please propose a change on the master branch ;-)

I agree :)

@corona10: Would you mind to use a GitHub review to officially "approve" the change? It's only to track than comments.

No problem. Thanks for the tip.
LGTM

@aeros
Copy link
Contributor
aeros commented Oct 15, 2019 •
edited
Loading

@vstinner

This change is a backport to an old security branch (3.5). If you would like to enhance the readability, please propose a change on the master branch ;-)

Alright, if you think it would be worthwhile, I can open a PR to master. Usually readability changes are hard to get merged, especially for the tests.

@vstinner
Copy link
Member Author

Alright, if you think it would be worthwhile, I can open a PR to master. Usually readability changes are hard to get merged, especially for the tests.

I don't think that it's worth it.

@aeros
Copy link
Contributor
aeros commented Oct 15, 2019 •
edited
Loading

@vstinner

I don't think that it's worth it.

Neither did I. That's why I mentioned it in the edit for my initial comment:

Never mind, this would have been addressed in the original PR #16373 and not just for the backport. It's probably not important enough to be worth adjusting separately.

IMO, readability changes should only be made when the original PR is in progress for test changes. But the tests should not be changed solely for readability purposes. Primarily because it's not worth the potential cost of breaking the functionality, not to mention the added review time.

@vstinner
Copy link
Member Author

corona10 assigned vstinner 1 hour ago

I'm the author of the PR. @corona10: What does it mean when you assign my own PR to myself?

This PR should be merged by @larryhastings. Only the 3.5 release manager can merge changes into the 3.5 branch.

@corona10
Copy link
Member

@vstinner oh sorry. I didn't notice that.

@vstinner
Copy link
Member Author

@corona10: No problem, only 3.5 and 3.6 branches are protected and only accept security fixes: https://devguide.python.org/#status-of-python-branches

@larryhastings larryhastings merged commit 3fe1b19 into python:3.5 Oct 29, 2019
@bedevere-bot
Copy link

@larryhastings: Please replace # with GH- in the commit message next time. Thanks!

@larryhastings
Copy link
Contributor

Thanks!

@vstinner vstinner deleted the 3.5 branch September 21, 2021 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeros aeros aeros left review comments

@corona10 corona10 corona10 approved these changes

Assignees

@larryhastings larryhastings

Labels
type-security A security issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vstinner @corona10 @aeros @bedevere-bot @larryhastings @the-knights-who-say-ni
0