Improper invalidation of mysql connection string

[kossidts]

Describe the bug
I am getting the error Invalid connection string format when trying to connect to the mysql db.

How To Reproduce (best to provide workable code or tests!)
Use a password that contains special characters or even just "-" in the connection string.
E.g. mysql://user:pass-1@localhost:3306/dbname

The source of the error seems to be in the parseConnectionString function located in the pool.js file. I guess the regex
const uriRegex = /^(\w+):\/\/(\w+)(?::(\w+))?@([\w.]+)(?::(\d+))?\/(\w+)$/;
is kept too simple. It should allow special characters especially for passwords.

[jaredwray]

Looks like this is has been fixed with the following pull request #808

[jaredwray]

@Ice-Hazymoon and @kossidts looks like the #808 was not valid after I added in some tests. Instead I moved to URL to parse it out and it seems to support the following pretty well and I validated with this unit test:

test.serial('validate connection strings', t => {
	const options = parseConnectionString('mysql://root:password@localhost:3306/keyv_test');
	t.is(options.user, 'root');
	t.is(options.password, 'password');
	t.is(options.host, 'localhost');
	t.is(options.port, 3306);
	t.is(options.database, 'keyv_test');

	const options2 = parseConnectionString('mysql://root-1:pass-1@localhost:3306/dbname');
	t.is(options2.user, 'root-1');
	t.is(options2.password, 'pass-1');
	t.is(options2.host, 'localhost');
	t.is(options2.port, 3306);
	t.is(options2.database, 'dbname');

	const options3 = parseConnectionString('mysql://test_stg:test@test-stg-cluster.cluster-hqpowufs.ap-dqhowd-1.rds.amazonaws.com:3306/test_beta');
	t.is(options3.user, 'test_stg');
	t.is(options3.password, 'test');
	t.is(options3.host, 'test-stg-cluster.cluster-hqpowufs.ap-dqhowd-1.rds.amazonaws.com');
	t.is(options3.port, 3306);
	t.is(options3.database, 'test_beta');
});

Please let me know if there are other ways that need to be parsed so I can test also.

[kossidts]

Correct @jaredwray but there it still a problem with approach. Parsing the connection string as URL introduces encoding issues. E.g. the following test will fail:

const pwd = 'f.[;@4IWS0,vvXdDeFLn';
const options4 = parseConnectionString(`mysql://root-1:${pwd}@localhost:3306/dbname`);
t.is(options4.user, 'root-1');
t.is(options4.password, pwd);
t.is(options4.host, 'localhost');
t.is(options4.port, 3306);
t.is(options4.database, 'dbname');

Because password is now the encoded url friendly version of pwd i.e. f.%5B%3B%404IWS0,vvXdDeFLn. Hence the parts/properties need to be decoded e.g.:

t.is(decodeURIComponent(options4.password), pwd);

This means that if parsing with new URL remains the preferred way to go, then the properties will have to be decoded before using them to establish a MySql connection.

[jaredwray]

@kossidts - I see what you are saying. Would you like to do a fix for this to handle it better as we could in parseConnectionString do a decodeURIComponent on each item where it makes sense. Thoughts?

[kossidts]

I have updated the parseConnectionString function and added more test cases. The tests now include special characters and whitespaces. The tests cover cases with and without password or port (kossidts@c943ff9).

Almost all tests have passed. Almost. When a password includes the # character the test fails. The connection string is no longer a valid URL. That's where I take a look into the RFC (https://datatracker.ietf.org/doc/html/rfc1738) and I guess using new URL to parse the connection string might not be the right or easiest solution to handle everything right.

Also depending on the environment where this package is used new URL might return different results. E.g. the result of the following snippet in node differs from the browser.

let  pwd = 'f.[;@4IWS0,vvXdDeFLn'; 
let user = 'liäémaer Noêl'; 
let host = '[::1]'; 
let db = 'tablau-èdd';
new URL(`mysql://${user}:${pwd}@${host}:9382/${db}`)

Even though this package will not be used in the browser, it's just an example to say that I'm not sure if new URL is consistent across other environments like deno for example.

All this is to say that the simplest solution might be to let the user set the mysql connection object:

new Keyv({
    // uri: connectionStr,
    // connectionOptions or connetion
    connection: {
       host: '[::1]',
       user: 'John Noêl',
       password : 'f.[;@4IWS0,vv)X-dDe FLn+Ün#',
       database : 'some database',
       // more connection options
       connectionLimit : 10,
    }, 
    adapter: "mysql",
    table: "....",
    keySize: 255,
    namespace: '...',
});

The connection will be handed over to the MySql adapter. This will perfectly mirror the connection interface of the MySql library. But it is a breaking change though. That's option 1.

Option 2 would be to use another library as a dependency to the mysql adapter for parsing the connection string e.g. the npm connection-string-parser package. I've never used it before. Since I've just discovered it, I cannot tell how good it is or how well it's written.

The last option is to use regex. It can be very loose or very complex and can introduce security issues if not done right.

I would choose option 1.

[jaredwray]

Thanks for the update and I added some fixes on here with encoding and decoding to help. #826

[kossidts]

Hi @jaredwray try testing against a very simple connection sample like:

{
    username: 'root',
    password: 'password?hdg',
    host: 'localhost',
    port: 3306,
    database: 'keyv_dbname',
}

The test will still fail unless ? is also escaped.

This is to raise a concern regarding parsing a connection string with new URL.
While it may work for a majority of users, there might be cases where it may not work due to the fact that a username/password combination doesn't necessarily have to satisfy a URL definition. This limitation makes the solution prone to potential issues that might not be immediately evident.
It's a good approach though.

[jaredwray]

that is correct and gets more complicated in the fact that on a url ? is used for additional parameters.