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.

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

Jump to bottom

[Code] Provide more reasons for git url validation #34914

Merged
merged 1 commit into from
Apr 11, 2019
Merged

[Code] Provide more reasons for git url validation #34914

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
40 changes: 23 additions & 17 deletions x-pack/plugins/code/common/git_url_utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,46 +4,52 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { isValidGitUrl } from './git_url_utils';
import { validateGitUrl } from './git_url_utils';

test('Git url validation', () => {
// An url ends with .git
expect(isValidGitUrl('https://github.com/elastic/elasticsearch.git')).toBeTruthy();
expect(validateGitUrl('https://github.com/elastic/elasticsearch.git')).toBeTruthy();

// An url ends without .git
expect(isValidGitUrl('https://github.com/elastic/elasticsearch')).toBeTruthy();
expect(validateGitUrl('https://github.com/elastic/elasticsearch')).toBeTruthy();

// An url with http://
expect(isValidGitUrl('http://github.com/elastic/elasticsearch')).toBeTruthy();
expect(validateGitUrl('http://github.com/elastic/elasticsearch')).toBeTruthy();

// An url with ssh://
expect(isValidGitUrl('ssh://elastic@github.com/elastic/elasticsearch.git')).toBeTruthy();
expect(validateGitUrl('ssh://elastic@github.com/elastic/elasticsearch.git')).toBeTruthy();

// An url with ssh:// and port
expect(isValidGitUrl('ssh://elastic@github.com:9999/elastic/elasticsearch.git')).toBeTruthy();
expect(validateGitUrl('ssh://elastic@github.com:9999/elastic/elasticsearch.git')).toBeTruthy();

// An url with git://
expect(isValidGitUrl('git://elastic@github.com/elastic/elasticsearch.git')).toBeTruthy();
expect(validateGitUrl('git://elastic@github.com/elastic/elasticsearch.git')).toBeTruthy();

// An url with an invalid protocol
expect(
isValidGitUrl('file:///Users/elastic/elasticsearch', [], ['ssh', 'https', 'git'])
).toBeFalsy();
expect(() => {
validateGitUrl('file:///Users/elastic/elasticsearch', [], ['ssh', 'https', 'git']);
}).toThrow('Git url protocol is not whitelisted.');

// An url without protocol
expect(isValidGitUrl('/Users/elastic/elasticsearch', [], ['ssh', 'https', 'git'])).toBeFalsy();
expect(
isValidGitUrl('github.com/elastic/elasticsearch', [], ['ssh', 'https', 'git'])
).toBeFalsy();
expect(() => {
validateGitUrl('/Users/elastic/elasticsearch', [], ['ssh', 'https', 'git']);
}).toThrow('Git url protocol is not whitelisted.');
expect(() => {
validateGitUrl('github.com/elastic/elasticsearch', [], ['ssh', 'https', 'git']);
}).toThrow('Git url protocol is not whitelisted.');

// An valid git url but without whitelisted host
expect(isValidGitUrl('https://github.com/elastic/elasticsearch.git', ['gitlab.com'])).toBeFalsy();
expect(() => {
validateGitUrl('https://github.com/elastic/elasticsearch.git', ['gitlab.com']);
}).toThrow('Git url host is not whitelisted.');

// An valid git url but without whitelisted protocol
expect(isValidGitUrl('https://github.com/elastic/elasticsearch.git', [], ['ssh'])).toBeFalsy();
expect(() => {
validateGitUrl('https://github.com/elastic/elasticsearch.git', [], ['ssh']);
}).toThrow('Git url protocol is not whitelisted.');

// An valid git url with both whitelisted host and protocol
expect(
isValidGitUrl('https://github.com/elastic/elasticsearch.git', ['github.com'], ['https'])
validateGitUrl('https://github.com/elastic/elasticsearch.git', ['github.com'], ['https'])
).toBeTruthy();
});
29 changes: 14 additions & 15 deletions x-pack/plugins/code/common/git_url_utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,27 @@

import GitUrlParse from 'git-url-parse';

export function isValidGitUrl(
// return true if the git url is valid, otherwise throw Error with
// exact reasons.
export function validateGitUrl(
url: string,
hostWhitelist?: string[],
protocolWhitelist?: string[]
): boolean {
try {
const repo = GitUrlParse(url);
const repo = GitUrlParse(url);

let isHostValid = true;
if (hostWhitelist && hostWhitelist.length > 0) {
const hostSet = new Set(hostWhitelist);
isHostValid = hostSet.has(repo.source);
if (hostWhitelist && hostWhitelist.length > 0) {
const hostSet = new Set(hostWhitelist);
if (!hostSet.has(repo.source)) {
throw new Error('Git url host is not whitelisted.');
}
}

let isProtocolValid = true;
if (protocolWhitelist && protocolWhitelist.length > 0) {
const protocolSet = new Set(protocolWhitelist);
isProtocolValid = protocolSet.has(repo.protocol);
if (protocolWhitelist && protocolWhitelist.length > 0) {
const protocolSet = new Set(protocolWhitelist);
if (!protocolSet.has(repo.protocol)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if no protocol specified, we should say protocol is needed for url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has nothing to do with missing protocol. here it's checking if the given protocol is in the whitelist.

throw new Error('Git url protocol is not whitelisted.');
}

return isHostValid && isProtocolValid;
} catch (error) {
return false;
}
return true;
}
13 changes: 7 additions & 6 deletions x-pack/plugins/code/server/queue/clone_worker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { delay } from 'lodash';

import { isValidGitUrl } from '../../common/git_url_utils';
import { validateGitUrl } from '../../common/git_url_utils';
import { RepositoryUtils } from '../../common/repository_utils';
import {
CloneProgress,
Expand Down Expand Up @@ -38,14 +38,15 @@ export class CloneWorker extends AbstractGitWorker {

public async executeJob(job: Job) {
const { url } = job.payload;
if (
!isValidGitUrl(
try {
validateGitUrl(
url,
this.serverOptions.security.gitHostWhitelist,
this.serverOptions.security.gitProtocolWhitelist
)
) {
this.log.error(`Invalid git url ${url}`);
);
} catch (error) {
this.log.error(`Validate git url ${url} error.`);
this.log.error(error);
return {
uri: url,
// Return a null repo for invalid git url.
Expand Down
14 changes: 8 additions & 6 deletions x-pack/plugins/code/server/routes/repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import Boom from 'boom';

import { Server } from 'hapi';
import { isValidGitUrl } from '../../common/git_url_utils';
import { validateGitUrl } from '../../common/git_url_utils';
import { RepositoryUtils } from '../../common/repository_utils';
import { RepositoryConfig, RepositoryUri } from '../../model';
import { RepositoryIndexInitializer, RepositoryIndexInitializerFactory } from '../indexer';
Expand Down Expand Up @@ -37,14 +37,16 @@ export function repositoryRoute(
const log = new Logger(req.server);

// Reject the request if the url is an invalid git url.
if (
!isValidGitUrl(
try {
validateGitUrl(
repoUrl,
options.security.gitHostWhitelist,
options.security.gitProtocolWhitelist
)
) {
return Boom.badRequest('Invalid git url.');
);
} catch (error) {
log.error(`Validate git url ${repoUrl} error.`);
log.error(error);
return Boom.badRequest(error);
}

const repo = RepositoryUtils.buildRepository(repoUrl);
Expand Down