◐ Shell
reader mode source ↗
Skip to content

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

Merged
larryhastings merged 2 commits into
python:3.5from
vstinner:3.5
Oct 29, 2019
Merged

[3.5] bpo-38243, xmlrpc.server: Escape the server_title (GH-16373) (GH-16441)#16516
larryhastings merged 2 commits into
python:3.5from
vstinner:3.5

Conversation

@vstinner

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

Copy link
Copy Markdown
Member

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 commented Oct 1, 2019

Copy link
Copy Markdown
Member Author

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

cc @larryhastings

@vstinner

vstinner commented Oct 1, 2019

Copy link
Copy Markdown
Member Author

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

vstinner commented Oct 8, 2019

Copy link
Copy Markdown
Member Author

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

@corona10

corona10 commented Oct 12, 2019

Copy link
Copy Markdown
Member

This PR now looks okay to merge it :)

aeros
aeros reviewed Oct 15, 2019
View reviewed changes

@aeros aeros left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

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

vstinner commented Oct 15, 2019

Copy link
Copy Markdown
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

vstinner commented Oct 15, 2019

Copy link
Copy Markdown
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

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

aeros commented Oct 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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

vstinner commented Oct 15, 2019

Copy link
Copy Markdown
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

aeros commented Oct 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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

vstinner commented Oct 21, 2019

Copy link
Copy Markdown
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

corona10 commented Oct 21, 2019

Copy link
Copy Markdown
Member

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

@vstinner

vstinner commented Oct 21, 2019

Copy link
Copy Markdown
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

bedevere-bot commented Oct 29, 2019

Copy link
Copy Markdown

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

@larryhastings

larryhastings commented Oct 29, 2019

Copy link
Copy Markdown
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

@corona10 corona10 corona10 approved these changes

+1 more reviewer

@aeros aeros aeros left review comments

Reviewers whose approvals may not affect merge requirements

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