Skip to content

fix(service-provider-core): allow non-hostname chars in DB name MONGOSH-792 #906

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 26, 2021
Merged

fix(service-provider-core): allow non-hostname chars in DB name MONGOSH-792 #906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/cli-repl/test/e2e-direct.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ describe('e2e direct connection', () => {
});
it('when specifying multiple seeds through --host with a wrong replsetid', async() => {
const hostlist = 'wrongreplset/' + await rs2.hostport() + ',' + await rs1.hostport() + ',' + await rs0.hostport();
const shell = TestShell.start({ args: ['--host', hostlist, 'admin?serverSelectionTimeoutMS=2000'] });
const shell = TestShell.start({ args: ['--host', hostlist, 'admin'] });
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The legacy shell would have interpreted the previous string as an entire database name here (and it would be a valid one)

await shell.waitForExit();
shell.assertContainsOutput('MongoServerSelectionError');
});
Expand Down
86 changes: 40 additions & 46 deletions packages/cli-repl/test/e2e.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,57 +77,51 @@ describe('e2e', function() {
expect(await onExit).to.equal(0);
});
});

describe('set db', () => {
describe('via host:port/test', () => {
let shell;
beforeEach(async() => {
shell = TestShell.start({ args: [`${await testServer.hostport()}/testdb1`] });
await shell.waitForPrompt();
shell.assertNoErrors();
});
it('db set correctly', async() => {
await shell.executeLine('db');
shell.assertNoErrors();

await eventually(() => {
shell.assertContainsOutput('testdb1');
for (const { mode, dbname, dbnameUri } of [
{ mode: 'no special characetrs', dbname: 'testdb1', dbnameUri: 'testdb1' },
{ mode: 'special characters', dbname: 'ä:-,🐈_\'[!?%', dbnameUri: 'ä:-,🐈_\'[!%3F%25' }
]) {
context(mode, () => {
describe('via host:port/test', () => {
let shell;
beforeEach(async() => {
shell = TestShell.start({ args: [`${await testServer.hostport()}/${dbname}`] });
await shell.waitForPrompt();
shell.assertNoErrors();
});
it('db set correctly', async() => {
expect(await shell.executeLine('db')).to.include(dbname);
shell.assertNoErrors();
});
});
});
});
describe('via mongodb://uri', () => {
let shell;
beforeEach(async() => {
shell = TestShell.start({ args: [`mongodb://${await testServer.hostport()}/testdb2`] });
await shell.waitForPrompt();
shell.assertNoErrors();
});
it('db set correctly', async() => {
await shell.executeLine('db');
shell.assertNoErrors();

await eventually(() => {
shell.assertContainsOutput('testdb2');
describe('via mongodb://uri', () => {
let shell;
beforeEach(async() => {
shell = TestShell.start({ args: [`mongodb://${await testServer.hostport()}/${dbnameUri}`] });
await shell.waitForPrompt();
shell.assertNoErrors();
});
it('db set correctly', async() => {
expect(await shell.executeLine('db')).to.include(dbname);
shell.assertNoErrors();
});
});
});
});
describe('legacy db only', () => {
let shell;
beforeEach(async() => {
const port = await testServer.port();
shell = TestShell.start({ args: ['testdb3', `--port=${port}`] });
await shell.waitForPrompt();
shell.assertNoErrors();
});
it('db set correctly', async() => {
await shell.executeLine('db');
shell.assertNoErrors();

await eventually(() => {
shell.assertContainsOutput('testdb3');
describe('legacy db only', () => {
let shell;
beforeEach(async() => {
const port = await testServer.port();
shell = TestShell.start({ args: [dbname, `--port=${port}`] });
await shell.waitForPrompt();
shell.assertNoErrors();
});
it('db set correctly', async() => {
expect(await shell.executeLine('db')).to.include(dbname);
shell.assertNoErrors();
});
});
});
});
}
});

describe('with connection string', () => {
Expand Down
37 changes: 33 additions & 4 deletions packages/service-provider-core/src/uri-generator.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,30 @@ describe('uri-generator.generate-uri', () => {
});
});

context('when no additional options are provided with db with special characters', () => {
const uri = '192.0.0.1:27018/föö-:?%ab💙,\'_.c';
const options = { connectionSpecifier: uri };

it('returns the uri with the scheme', () => {
expect(generateUri(options)).to.equal('mongodb://192.0.0.1:27018/f%C3%B6%C3%B6-%3A%3F%25ab%F0%9F%92%99%2C\'_.c?directConnection=true');
});
});

context('when the db part does not start with a slash', () => {
const uri = '192.0.0.1:27018?foo=bar';
const options = { connectionSpecifier: uri };

it('throws an exception', () => {
try {
generateUri(options);
expect.fail('expected error');
} catch (e) {
expect(e).to.be.instanceOf(MongoshInvalidInputError);
expect(e.code).to.equal(CommonErrors.InvalidArgument);
}
});
});

context('when additional options are provided', () => {
context('when providing host with URI', () => {
const uri = '192.0.0.1:27018/foo';
Expand Down Expand Up @@ -277,23 +301,23 @@ describe('uri-generator.generate-uri', () => {

context('when providing a URI with query parameters', () => {
context('that do not conflict with directConnection', () => {
const uri = '192.0.0.1:27018?readPreference=primary';
const uri = 'mongodb://192.0.0.1:27018/?readPreference=primary';
const options = { connectionSpecifier: uri };
it('still includes directConnection', () => {
expect(generateUri(options)).to.equal('mongodb://192.0.0.1:27018/?readPreference=primary&directConnection=true');
});
});

context('including replicaSet', () => {
const uri = '192.0.0.1:27018/db?replicaSet=replicaset';
const uri = 'mongodb://192.0.0.1:27018/db?replicaSet=replicaset';
const options = { connectionSpecifier: uri };
it('does not add the directConnection parameter', () => {
expect(generateUri(options)).to.equal(`mongodb://${uri}`);
expect(generateUri(options)).to.equal(uri);
});
});

context('including explicit directConnection', () => {
const uri = '192.0.0.1:27018?directConnection=false';
const uri = 'mongodb://192.0.0.1:27018/?directConnection=false';
const options = { connectionSpecifier: uri };
it('does not change the directConnection parameter', () => {
expect(generateUri(options)).to.equal('mongodb://192.0.0.1:27018/?directConnection=false');
Expand Down Expand Up @@ -346,5 +370,10 @@ describe('uri-generator.generate-uri', () => {
const options = { host: 'replsetname/host1:123,host2,host3:456', connectionSpecifier: 'admin' };
expect(generateUri(options)).to.equal('mongodb://host1:123,host2,host3:456/admin?replicaSet=replsetname');
});

it('returns a URI for the hosts and ports specified in --host and database name with escaped chars', () => {
const options = { host: 'replsetname/host1:123,host2,host3:456', connectionSpecifier: 'admin?foo=bar' };
expect(generateUri(options)).to.equal('mongodb://host1:123,host2,host3:456/admin%3Ffoo%3Dbar?replicaSet=replsetname');
});
});
});
28 changes: 16 additions & 12 deletions packages/service-provider-core/src/uri-generator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ function generateUri(options: CliOptions): string {
return '';
}
const connectionString = generateUriNormalized(options);
if (connectionString.hosts.length === 1 &&
['localhost', '127.0.0.1'].includes(connectionString.hosts[0].split(':')[0])) {
if (connectionString.hosts.every(host =>
['localhost', '127.0.0.1'].includes(host.split(':')[0]))) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is more of a drive-by change to account for the fact that the e2e-direct test cannot use serverSelectionTimeoutMS directly anymore, but I feel like it still just makes sense

const params = connectionString.searchParams;
if (!params.has('serverSelectionTimeoutMS')) {
params.set('serverSelectionTimeoutMS', '2000');
Expand All @@ -147,7 +147,7 @@ function generateUriNormalized(options: CliOptions): ConnectionString {
/^(?<replSetName>[^/]+)\/(?<hosts>([A-Za-z0-9.-]+(:\d+)?,?)+)$/);
if (replSetHostMatch) {
const { replSetName, hosts } = replSetHostMatch.groups as { replSetName: string, hosts: string };
const connectionString = new ConnectionString(`${Scheme.Mongo}replacemeHost/${uri ?? DEFAULT_DB}`);
const connectionString = new ConnectionString(`${Scheme.Mongo}replacemeHost/${encodeURIComponent(uri ?? DEFAULT_DB)}`);
connectionString.hosts = hosts.split(',').filter(host => host.trim());
connectionString.searchParams.set('replicaSet', replSetName);
return addShellConnectionStringParameters(connectionString);
Expand All @@ -169,17 +169,22 @@ function generateUriNormalized(options: CliOptions): ConnectionString {
}

// Capture host, port and db from the string and generate a URI from
// the parts.
const uriMatch = /^([A-Za-z0-9][A-Za-z0-9.-]+):?(\d+)?[\/]?(\S+)?$/gi;
const parts = uriMatch.exec(uri);
// the parts. If there is a db part, it *must* start with /.
const uriMatch = /^([A-Za-z0-9][A-Za-z0-9.-]+):?(\d+)?(?:\/(\S*))?$/gi;
let parts: string[] | null = uriMatch.exec(uri);

if (parts === null) {
throw new MongoshInvalidInputError(`Invalid URI: ${uri}`, CommonErrors.InvalidArgument);
if (/[/\\. "$]/.test(uri)) {
// This cannot be a database name because 'uri' contains characters invalid in a database.
throw new MongoshInvalidInputError(`Invalid URI: ${uri}`, CommonErrors.InvalidArgument);
} else {
parts = [ uri, uri ];
}
}

let host: string | undefined = parts[1];
const port = parts[2];
let dbAndQueryString = parts[3];
let host: string | undefined = parts?.[1];
const port = parts?.[2];
let dbAndQueryString = parts?.[3];

// If there is no port and db, host becomes db if there is no
// '.' in the string. (legacy shell behaviour)
Expand All @@ -193,9 +198,8 @@ function generateUriNormalized(options: CliOptions): ConnectionString {
if (host || port) {
validateConflicts(options);
}

return addShellConnectionStringParameters(new ConnectionString(
`${Scheme.Mongo}${host || generateHost(options)}:${port || generatePort(options)}/${dbAndQueryString ?? DEFAULT_DB}`));
`${Scheme.Mongo}${host || generateHost(options)}:${port || generatePort(options)}/${encodeURIComponent(dbAndQueryString || DEFAULT_DB)}`));
}

/**
Expand Down