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

Fixed passwords getting percent_encoded #292

Closed
wants to merge 1 commit into from

Conversation

@Thomspoon
Copy link

Thomspoon commented Apr 18, 2017

When testing mysql database connectors, I came across this squirrelly response:

thread 'main' panicked at 'called Result::unwrap()on anErr value: InvalidPort', /Users/rustbuild/src/rust-buildbot/slave/stable-dist-rustc-mac/build/src/libcore/result.rs:86

My connection string was similar to:

mysql://root:pass#word@example.com

Which immediately prompted me to wonder what was happening. I cloned the repo for the connector I was using and started debugging, before I dug deeper and found it was due to the url::Url type panicking. So I delved even deeper and ended up here.

I found that parse_userinfo was breaking at the #, which would then cause some errors downstream when parsing the rest of the url.

After this fix, I found that the password, since it wasn't expecting these characters, was percent_encoding these characters, so the connection string would turn into:

mysql://root:pass%23word@example.com

My proposed change would allow additional special characters in the password, accepting the ? and # characters that were previously disallowed. IBM and various others promote the usage of specifically # and ? in passwords, which is why I think those restrictions should be taken out.

This is my first pull request, so please let me know if this was too much.


This change is Reviewable

@Thomspoon
Copy link
Author

Thomspoon commented Apr 18, 2017

On second though, this only works for my use-case, not the main use case of this crate probably. All of the SQL connector repos that I have encountered use the URL crate, so I'm kind of wondering what the workaround for this would be. Url::parse_no_percent_encode?

Sorry for the close and open, I don't know my own strength.

@Thomspoon Thomspoon closed this Apr 18, 2017
@Thomspoon Thomspoon reopened this Apr 18, 2017
@SimonSapin
Copy link
Member

SimonSapin commented Apr 18, 2017

# is not simply disallowed as an arbitrary restriction, it is a separator for the "fragment" component of an URL. For example, structs is the fragment of https://docs.rs/url/1.4.0/url/#structs. As you can see on Travis-CI, your change makes some tests fail.

Using percent-encoding is the correct way.

@Thomspoon
Copy link
Author

Thomspoon commented Apr 18, 2017

Ah, that makes sense.

I'm aware why the tests fail, the question now is, would a better way around this problem be to have a parse_without_encoding function? The url crate is such a strong crate, that it wouldn't make sense to have repos such as rust-mysql-simple change their codebase when they could change one function to have the same effect. I'm sure a lot of crates could benefit from that if they are using their own scheme.

@Thomspoon Thomspoon closed this Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.