FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[3.9] bpo-43882 Remove the newline, and tab early. From query and fragments. by orsenthil · Pull Request #25853 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

[3.9] bpo-43882 Remove the newline, and tab early. From query and fragments. - #25853

Merged
orsenthil merged 2 commits into
python:3.9from
orsenthil:issue43882_39
May 3, 2021
ConversationCommits (2)ChecksFiles changed
Merged

[3.9] bpo-43882 Remove the newline, and tab early. From query and fragments.#25853
orsenthil merged 2 commits into
python:3.9from
orsenthil:issue43882_39

Conversation

orsenthil commented May 3, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Based on the review comment #25726 (review)

Moving the fix for removing newline and tabs early to include both query strings and fragments, (Entire URL).

https://bugs.python.org/issue43882

orsenthil requested a review from gpshead May 3, 2021 14:09
orsenthil changed the title bpo-43882 Remove the newline, and tab early. From query and fragments. [3.9] bpo-43882 Remove the newline, and tab early. From query and fragments. May 3, 2021
gpshead requested changes May 3, 2021
View reviewed changes
Comment thread Lib/test/test_urlparse.py
Comment thread Lib/test/test_urlparse.py Outdated
def test_urlsplit_remove_unsafe_bytes(self):
# Remove ASCII tabs and newlines from input
url = "http://www.python.org/java\nscript:\talert('msg\r\n')/#frag"
url = "http\t://www.python.org/java\nscript:\talert('msg\r\n')/?query\n=\tsomething#frag\nment"

gpshead May 3, 2021

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

i suggest also adding a \r in the middle of the netloc for good measure. www.python\r.org perhaps?

bedevere-bot commented May 3, 2021

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

gpshead commented May 3, 2021

Copy link
Copy Markdown
Member

fwiw this PR is against 3.9. i assume you'll "backport to main and 3.10" once those aren't actively being release branched and renamed?

orsenthil commented May 3, 2021

Copy link
Copy Markdown
Member Author

i assume you'll "backport to main and 3.10" once those aren't actively being release branched and renamed?

Yes, exactly. I wanted to time window to be utilized for 3.9 and 3.8.
I will port it to rest of the active branches immediately.

orsenthil merged commit 8a59574 into python:3.9 May 3, 2021

bedevere-bot commented May 3, 2021

Copy link
Copy Markdown

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

gpshead mentioned this pull request May 3, 2021
Merged
adamjstewart reviewed Jul 2, 2021
View reviewed changes
Comment thread Lib/urllib/parse.py

for b in _UNSAFE_URL_BYTES_TO_REMOVE:
url = url.replace(b, "")
scheme = scheme.replace(b, "")

adamjstewart Jul 2, 2021

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

According to the typeshed, the scheme argument to urlparse is Optional. However, with this change, a value of None will result in an error like:

AttributeError: 'NoneType' object has no attribute 'replace'

I'm not sure whether the typeshed is wrong or if this line should be guarded with if scheme is not None, but we should definitely fix one or the other and perhaps improve the documentation on this option.

Thanks to @branchvincent for doing the digging and reporting this issue to me.

orsenthil mentioned this pull request Aug 25, 2022
Closed
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gpshead gpshead approved these changes
+1 more reviewer
adamjstewart adamjstewart left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL