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

[Code] check file path in lsp requests #33916

Merged
merged 1 commit into from
Mar 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
60 changes: 58 additions & 2 deletions x-pack/plugins/code/server/lsp/workspace_handler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import Git from '@elastic/nodegit';
import fs from 'fs';
import path from 'path';

Expand All @@ -18,6 +19,9 @@ const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'code_test'));
const workspaceDir = path.join(baseDir, 'workspace');
const repoDir = path.join(baseDir, 'repo');

// @ts-ignore
Git.enableThreadSafety();

function handleResponseUri(wh: WorkspaceHandler, uri: string) {
const dummyRequest: LspRequest = {
method: 'textDocument/edefinition',
Expand All @@ -40,9 +44,9 @@ function handleResponseUri(wh: WorkspaceHandler, uri: string) {

function makeAFile(
workspacePath: string = workspaceDir,
file = 'src/controllers/user.ts',
repo = 'github.com/Microsoft/TypeScript-Node-Starter',
revision = 'master',
file = 'src/controllers/user.ts'
revision = 'master'
) {
const fullPath = path.join(workspacePath, repo, '__randomString', revision, file);
mkdirp.sync(path.dirname(fullPath));
Expand Down Expand Up @@ -112,6 +116,58 @@ test('should throw a error if url is invalid', async () => {
expect(() => handleResponseUri(workspaceHandler, uri)).toThrow();
});

async function prepareProject(repoPath: string) {
mkdirp.sync(repoPath);
const repo = await Git.Repository.init(repoPath, 0);
const content = 'console.log("test")';
const subFolder = 'src';
fs.mkdirSync(path.join(repo.workdir(), subFolder));
fs.writeFileSync(path.join(repo.workdir(), 'src/app.ts'), content, 'utf8');

const index = await repo.refreshIndex();
await index.addByPath('src/app.ts');
index.write();
const treeId = await index.writeTree();
const committer = Git.Signature.create('tester', 'test@test.com', Date.now() / 1000, 60);
const commit = await repo.createCommit(
'HEAD',
committer,
committer,
'commit for test',
treeId,
[]
);
return { repo, commit };
}

test('should throw a error if file path is external', async () => {
const workspaceHandler = new WorkspaceHandler(
repoDir,
workspaceDir,
// @ts-ignore
null,
new ConsoleLoggerFactory()
);
const repoUri = 'github.com/microsoft/typescript-node-starter';
await prepareProject(path.join(repoDir, repoUri));
const externalFile = 'node_modules/abbrev/abbrev.js';
const request: LspRequest = {
method: 'textDocument/hover',
params: {
position: {
line: 8,
character: 23,
},
textDocument: {
uri: `git://${repoUri}/blob/master/${externalFile}`,
},
},
};
await expect(workspaceHandler.handleRequest(request)).rejects.toEqual(
new Error('invalid fle path in requests.')
);
});

beforeAll(() => {
mkdirp.sync(workspaceDir);
mkdirp.sync(repoDir);
Expand Down
49 changes: 43 additions & 6 deletions x-pack/plugins/code/server/lsp/workspace_handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { Clone, Commit, Error as GitError, Repository, Reset } from '@elastic/nodegit';
import { Clone, Commit, Error as GitError, Repository, Reset, TreeEntry } from '@elastic/nodegit';
import Boom from 'boom';
import del from 'del';
import fs from 'fs';
Expand All @@ -31,7 +31,7 @@ export class WorkspaceHandler {
private git: GitOperations;
private revisionMap: { [uri: string]: string } = {};
private log: Logger;
private objectClient: RepositoryObjectClient;
private readonly objectClient: RepositoryObjectClient | undefined = undefined;

constructor(
readonly repoPath: string,
Expand All @@ -41,7 +41,9 @@ export class WorkspaceHandler {
) {
this.git = new GitOperations(repoPath);
this.log = loggerFactory.getLogger(['LSP', 'workspace']);
this.objectClient = new RepositoryObjectClient(this.client);
if (this.client) {
this.objectClient = new RepositoryObjectClient(this.client);
}
}

/**
Expand All @@ -54,7 +56,7 @@ export class WorkspaceHandler {
const tryGetGitStatus = async (retryCount: number) => {
let gitStatus;
try {
gitStatus = await this.objectClient.getRepositoryGitStatus(repositoryUri);
gitStatus = await this.objectClient!.getRepositoryGitStatus(repositoryUri);
} catch (error) {
throw Boom.internal(`checkout workspace on an unknown status repository`);
}
Expand All @@ -71,7 +73,9 @@ export class WorkspaceHandler {
}
}
};
await tryGetGitStatus(0);
if (this.objectClient) {
await tryGetGitStatus(0);
}

const bareRepo = await this.git.openRepo(repositoryUri);
const targetCommit = await this.git.getCommit(bareRepo, revision);
Expand Down Expand Up @@ -299,6 +303,12 @@ export class WorkspaceHandler {
if (uri.startsWith('git://')) {
const { repoUri, file, revision } = parseLspUrl(uri)!;
const { workspaceRepo, workspaceRevision } = await this.openWorkspace(repoUri, revision);
if (file) {
const isValidPath = await this.checkFile(workspaceRepo, file);
if (!isValidPath) {
throw new Error('invalid fle path in requests.');
}
}
return {
workspacePath: workspaceRepo.workdir(),
filePath: this.fileUrl(path.resolve(workspaceRepo.workdir(), file || '/')),
Expand Down Expand Up @@ -333,7 +343,8 @@ export class WorkspaceHandler {
}

private async workspaceDir(repoUri: string) {
const randomStr = await this.objectClient.getRepositoryRandomStr(repoUri);
const randomStr =
this.objectClient && (await this.objectClient.getRepositoryRandomStr(repoUri));
const base = path.join(this.workspacePath, repoUri);
if (randomStr) {
return path.join(base, `__${randomStr}`);
Expand Down Expand Up @@ -382,4 +393,30 @@ export class WorkspaceHandler {
const workspaceRelativePath = path.relative(this.workspacePath, workspaceRepo.workdir());
this.revisionMap[workspaceRelativePath] = headCommit.sha().substring(0, 7);
}

/**
* check whether the file path specify in the request is valid. The file path must:
* 1. exists in git repo
* 2. is a valid file or dir, can't be a link or submodule
*
* @param workspaceRepo
* @param filePath
*/
private async checkFile(workspaceRepo: Repository, filePath: string) {
const headCommit = await workspaceRepo.getHeadCommit();
Copy link
Contributor

Choose a reason for hiding this comment

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

the revision from line 300 might not necessarily to be HEAD in here. do you think it makes more sense to pass around that revision in here instead of always using HEAD?

Copy link
Contributor Author

@spacedragon spacedragon Mar 27, 2019

Choose a reason for hiding this comment

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

Actually here the head commit of workspaceRepo is pointing to that revision.

try {
const entry = await headCommit.getEntry(filePath);
switch (entry.filemode()) {
case TreeEntry.FILEMODE.TREE:
case TreeEntry.FILEMODE.BLOB:
case TreeEntry.FILEMODE.EXECUTABLE:
return true;
default:
return false;
}
} catch (e) {
// filePath may not exists
return false;
}
}
}