Skip to content
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

Fix httpx is not encoding with symbol '%s' (#3140) #3141

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Tunglies
Copy link

@Tunglies Tunglies commented Mar 14, 2024

Fix httpx is not encoding with symbol '%s' (#3140)
Add url parsing test

Summary

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

Comment on lines 428 to 434
i = 0
while i < len(string):
char = string[i]
if char in NON_ESCAPED_CHARS:
i += 1
continue
# Check if the character is a start of a valid percent-encoded sequence
elif (
char == "%"
and i + 2 < len(string)
and string[i + 1 : i + 3].isalnum()
and len(string[i + 1 : i + 3]) == 2
):
i += 3
else:
return False

return True
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
i = 0
while i < len(string):
char = string[i]
if char in NON_ESCAPED_CHARS:
i += 1
continue
# Check if the character is a start of a valid percent-encoded sequence
elif (
char == "%"
and i + 2 < len(string)
and string[i + 1 : i + 3].isalnum()
and len(string[i + 1 : i + 3]) == 2
):
i += 3
else:
return False
return True
# All characters must already be non-escaping or valid percent-encoded
for index, char in enumerate(string):
if char not in NON_ESCAPED_CHARS and not PERCENT_ENCODED_REGEX.match(
string[index : index + 3]
):
return False
return True

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for review! I have applied the suggestion.

Add parsing symbol '%s' test

rewrite fn::_urlparse::is_safe
Copy link
Contributor

@T-256 T-256 left a comment

Choose a reason for hiding this comment

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

Thanks, I think it deserves a note in changelog.

@heysarthak
Copy link

Hi @T-256,
will the code change encode every "%" sent into uri as "%25" ?

@T-256
Copy link
Contributor

T-256 commented Mar 19, 2024

Hi @T-256, will the code change encode every "%" sent into uri as "%25" ?

Yes, if it is invalid percent-encoded.

@heysarthak
Copy link

when can we merge this changes? @tomchristie

@jhominal
Copy link
Member

jhominal commented Mar 27, 2024

Hello, could you look at if this PR fixes the cases of bad % character encoding that are raised in #3135?

I think the change in this PR (to only encode % if it would result in invalid percent-encoding) is a distraction from the real issue.

@tomchristie
Copy link
Member

Thanks. I think this is superseeded by #3187.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants