Skip to content

/cpython

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

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

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
+21 −1

Conversation

@vstinner
Copy link
Member

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

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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
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)
Loading status checks…
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 force-pushed the vstinner:3.5 branch from 14d328c to 84b60fc Oct 8, 2019
@vstinner

This comment has been minimized.

Sign in to view
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.
@corona10

This comment has been minimized.

Sign in to view
Copy link
Member

corona10 commented Oct 12, 2019

This PR now looks okay to merge it :)
@aeros
aeros reviewed Oct 15, 2019
View changes
Copy link
Member

aeros left a comment •
edited

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 comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Oct 15, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Oct 15, 2019

@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 changes
Copy link
Member

corona10 left a 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

This comment has been minimized.

Sign in to view
Copy link
Member

aeros commented Oct 15, 2019 •
edited

@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

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Oct 15, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

aeros commented Oct 15, 2019 •
edited

@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

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Oct 21, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

corona10 commented Oct 21, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Oct 21, 2019

@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
Merge branch '3.5' into 3.5
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
95cc72c
@larryhastings larryhastings merged commit 3fe1b19 into python:3.5 Oct 29, 2019
4 checks passed
4 checks passed
bedevere/issue-number Issue number 38243 found
Details
bedevere/maintenance-branch-pr Valid maintenance branch PR title.
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Oct 29, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

larryhastings commented Oct 29, 2019

Thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeros aeros

@corona10 corona10

Assignees

@larryhastings larryhastings

Labels
CLA signed type-security
Projects
None yet
Milestone
No milestone
6 participants
@vstinner @corona10 @aeros @bedevere-bot @larryhastings @the-knights-who-say-ni
You can’t perform that action at this time.