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

Adds decodeURIComponent in ConnectionConfig#parseUrl #1621

Closed
wants to merge 2 commits into from
Closed

Adds decodeURIComponent in ConnectionConfig#parseUrl #1621

wants to merge 2 commits into from

Conversation

Paultje52
Copy link

Currently, you cannot use special characters like @ in usernames or passwords. This is because those characters are getting uri encoded by the URL nodejs package.

This PR adds decodeURIComponent() to ConnectionConfig#parseUrl, for options.user and options.password. This way, users can use special characters like @ in usernames and passwords!

This also resolves #1384.
It also makes sure mysql2 can be used in nodejs apps hosted by pterodactyl with databases. When creating a database in pterodactyl, it generates a password automatically. That password also contains special characters, which make them useless when using mysql2.

This commit adds decodeURIComponent to the ConnectionConfig#parseUrl, for options.user and options.password.
@kykungz
Copy link

kykungz commented Sep 2, 2022

@Paultje52 I'm really looking forward to seeing this PR merged, as I cannot use mysql2 with connection string at all because my password contains special characters (generated) to be considered a strong password.

I have tested with this list of available characters in MySQL strong passwords (~!@$^&*()_-+={}[]<>,.;':|#?/%), here's the result:

For this list ~!@$^&*()_-+={}[]<>,.;':| (without #, ?, /, %), it is successfully parsed with no errors:

const password = "~!@$^&*()_-+={}[]<>,.;':|"
const connectionString = `mysql://username:${password}@127.0.0.1/database`
const parsedUrl = new URL(connectionString)
const parsedPassword = decodeURIComponent(parsedUrl.password)

console.log(parsedPassword === password) // true

But if the password is including one of #, ?, /, Invalid URL error will be thrown at new URL( ... ) due to the fact that #, ?, / is a path identifier of a URL.

const password = "~!@$^&*()_-+={}[]<>,.;':|" + "#?/"
const connectionString = `mysql://username:${password}@127.0.0.1/database`
const parsedUrl = new URL(connectionString) // Invalid URL

Also if the password is including %, URI malformed will be thrown at decodeURIComponent( ... ) because % is not encoded properly from new URL( ... ).

const password = "~!@$^&*()_-+={}[]<>,.;':|" + "%"
const connectionString = `mysql://username:${password}@127.0.0.1/database`
const parsedUrl = new URL(connectionString)
const parsedPassword = decodeURIComponent(parsedUrl.password) // URI malformed

However we can fix the % problem by using encodeURI( ... ) before passing it to new URL( ... )

- const parsedUrl = new URL(url);
+ const parsedUrl = new URL(encodeURI(url));
const options = {
  host: parsedUrl.hostname,
  port: parsedUrl.port,
  database: parsedUrl.pathname.slice(1),
  user: decodeURIComponent(parsedUrl.username),
  password: decodeURIComponent(parsedUrl.password)
};

This will make % available in the password:

const password = "~!@$^&*()_-+={}[]<>,.;':|" + "%"
const connectionString = `mysql://username:${password}@127.0.0.1/database`
const parsedUrl = new URL(encodeURI(connectionString))
const parsedPassword = decodeURIComponent(parsedUrl.password)

console.log(parsedPassword === password) // true

@sidorares Despite not covering every edge case (#, ?, / still unusable), we should still push forward this PR as it has already covered a wide range of special characters (~!@$^&*()_-+={}[]<>,.;':|%) while the current version does not do well with them.

Version ✅ Supported ❌ Unsupported
Current ~!$&*()_-+,.%' @^={}[]<>;:|#?/
This PR ~!$&*()_-+,.%'@^={}[]<>;:| #?/

@sidorares
Copy link
Owner

Thanks @Paultje52 !

Would you be able to add a unit test for this?

@Paultje52
Copy link
Author

Paultje52 commented Sep 3, 2022

@kykungz and @sidorares I changed the parser to support all special characters. Let me know what you think!

If you approve this, I can start working on the unit test :-)

@sidorares
Copy link
Owner

sidorares commented Sep 4, 2022

@Paultje52 whats the reason you decided to switch to a manual parsing? Can you give an example where password = decodeURIComponent(url.password); username = decodeURIComponent(url.username); would not work? nevermind, I see its only used for username/password part. Still not clear to me why regular new URL() won't work ( with decodeURIComponent on username/password part )

@kykungz
Copy link

kykungz commented Sep 4, 2022

@sidorares @Paultje52 After digging around with MySQL's URI-Like Connection Strings, I found that both solution (f08b3be and ed0cb99) will never cover every use-case.

For example, using manual parsing will cover more use-cases, but it will open up another limitation of:

username cannot contain :

while MySQL natively supported : in a username.

Although there will be only a few people who hit this wall, it's not great that a widely-used lib like this cannot match what MySQL can support.

It is explicitly stated in MySQL docs that you must encode all special characters before using it:

Important

Percent encoding must be used for reserved characters in the elements of the URI-like string. For example, if you specify a string that includes the @ character, the character must be replaced by %40. If you include a zone ID in an IPv6 address, the % character used as the separator must be replaced with %25.

So you cannot use mysql://username:p@$$w&rd@127.0.0.1:3306/database natively with MySQL's tools:

mysqlsh --uri "mysql://username:p@$$w&rd@127.0.0.1:3306/database"
# ❌ Error - Invalid URI

You must encodeURIComponent manually for each field:

mysqlsh --uri "mysql://username:p%40%24%24w%26rd@127.0.0.1:3306/database"
# ✅ Connected

It wouldn't make sense if you can connect to MySQL through node-mysql2 with p@$$w&rd, but couldn't do so on native MySQL tools. That sounded like a magic feature of this lib, but it's a feature that causes other unsolvable bugs.

So I think we should re-consider this and make this lib consistent with MySQL's standard, to eliminate all confusions about the lib vs actual MySQL. Instead of auto parsing connection string, we should be thinking of how to support encoded connection string.

We can simply revert back to f08b3be, and maybe add more decodeURIComponent so that we also support encoded IPv6 Zone ID in host and special characters in database:

const options = {
-  host: parsedUrl.hostname,
+  host: decodeURIComponent(parsedUrl.hostname),
   port: parsedUrl.port,
-  database: parsedUrl.pathname.slice(1),
+  database: decodeURIComponent(parsedUrl.pathname.slice(1)),
  user: decodeURIComponent(parsedUrl.username),
  password: decodeURIComponent(parsedUrl.password)
};

After that we can add more documentation on how to use URI-Like Connection Strings (every special character must be encoded) by referring to this document.

What do you think?

@sidorares
Copy link
Owner

@kykungz I'm happy with the proposed solution ( throwing in few decodeURIComponent )

while we are at this, might worth checking/adding support for socket path in uri connection:

socket: The path to a Unix socket file or the name of a Windows named pipe. Values are local file paths. In URI-like strings, they must be encoded, using either percent encoding or by surrounding the path with parentheses. Parentheses eliminate the need to percent encode characters such as the / directory separator character. For example, to connect as root@localhost using the Unix socket /tmp/mysql.sock, specify the path using percent encoding as root@localhost?socket=%2Ftmp%2Fmysql.sock, or using parentheses as root@localhost?socket=(/tmp/mysql.sock).

I think at this stage its more important to keep compatibility with mysqljs/mysql rather than mysql cli or other tools but ideally we should be following the spec in the https://dev.mysql.com/doc/refman/8.0/en/connecting-using-uri-or-key-value-pairs.html

@estyrke
Copy link

estyrke commented Feb 22, 2023

Any progress on this one? Running into this right now, unable to connect to my database at all due to crazy passsword...

@estyrke
Copy link

estyrke commented Feb 22, 2023

I ended up hand-patching connection_config.js and it worked perfectly, FWIW. :)

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.

decodeURIComponent after parsing URL for initial connection
4 participants