GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [3.9] bpo-43882 Remove the newline, and tab early. From query and fragments. by orsenthil · Pull Request #25853 · python/cpython

Conversation

@orsenthil

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

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@orsenthil
Remove the newline, and tab early. From query and fragments.
7a44060
@bedevere-bot bedevere-bot added the awaiting core review label May 3, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 3, 2021
@orsenthil
orsenthil requested a review from gpshead May 3, 2021 14:09
@orsenthil orsenthil added the skip news label May 3, 2021
@orsenthil 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
gpshead requested changes May 3, 2021
View reviewed changes
Comment thread Lib/test/test_urlparse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
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 gpshead May 3, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting core review label May 3, 2021
@bedevere-bot

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label May 3, 2021
@orsenthil
Address Review Comments.
f184bef
gpshead
gpshead approved these changes May 3, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels May 3, 2021
@gpshead

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@orsenthil

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.
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@orsenthil
orsenthil merged commit 8a59574 into python:3.9 May 3, 2021
@bedevere-bot

bedevere-bot commented May 3, 2021

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label May 3, 2021
@gpshead gpshead mentioned this pull request May 3, 2021
bpo-44002: Switch to lru_cache in urllib.parse. #25798
Merged
adamjstewart
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 adamjstewart Jul 2, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@orsenthil orsenthil mentioned this pull request Aug 25, 2022
[security] CVE-2022-0391: urllib.parse should sanitize urls containing ASCII newline and tabs. #88048
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@orsenthil @bedevere-bot @gpshead @adamjstewart @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.