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

hotfix: unquoting vhost in path #202

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

ofekashery
Copy link
Contributor

@ofekashery ofekashery commented Sep 3, 2024

On August 30, 2024, yarl released version 1.9.5. On this version, they introduced breaking changes for parsing the %2F char in path, which is frequently used by vhosts in the AMQP connection strings.

url = yarl.URL("amqp://localhost/%2f")
print(url.path)
# Yarl v1.9.4 -> `//` -> self.vhost = '/' 
# Yarl v1.9.5 -> `/%2F` -> self.vhost = '%2F'

This bug broke our critical production system, which relied on %2F as the vhost and led into protocol issues.
I just wrote an quick hotfix for that issue that allows connect successfully to the RMQ with both yarl versions.

Fixes #203.

@mosquito mosquito merged commit 120965c into mosquito:master Sep 4, 2024
9 of 10 checks passed
@ofekashery
Copy link
Contributor Author

Thank you, @mosquito ❤️

@Dreamsorcerer
Copy link

FYI, we've decided to revert this in yarl as it's caused a few more issues. If you need to know the path separators for safety, we're adding a path_safe attribute instead which skips decoding %2F and %25.

@decaz
Copy link
Contributor

decaz commented Sep 24, 2024

@ofekashery @mosquito due to information from @Dreamsorcerer - should this PR be reverted or these changes can be left?

@mosquito
Copy link
Owner

Probably not.

@Dreamsorcerer
Copy link

Not reverting would create 2 minor issues. First is obviously just performance, it's a wasted call. Second is the possibility of double unquoting, e.g. %252F would now become /, when it should really be %2F.

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.

yarl 1.9.5 causes AMQPInternalError
4 participants