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

[vstinner]

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]

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

cc @larryhastings

[vstinner]

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

[vstinner]

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

[corona10]

This PR now looks okay to merge it :)

[aeros]

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]

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]

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

[corona10]

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]

@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]

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]

@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]

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]

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

[vstinner]

@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

[bedevere-bot]

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

[larryhastings]

Thanks!