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

feat(marxan-runner): use output directory provided via input.dat #331

Merged
merged 2 commits into from
Jul 19, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@ beforeEach(async () => {
}).compile();
fileReader = sandbox.get(FileReader);
sut = sandbox.get(MarxanDirectory);
workspace = new Workspace(cwd as WorkingDirectory, bin, () =>
Promise.resolve(),
workspace = new Workspace(
cwd as WorkingDirectory,
bin,
() => Promise.resolve(),
() => Promise.resolve(),
);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,7 @@ import { WorkingDirectory } from '../../../ports/working-directory';
export abstract class TemporaryDirectory {
abstract get(): Promise<WorkingDirectory>;

abstract createOutputDirectory(inside: WorkingDirectory): Promise<void>;

abstract cleanup(directory: string): Promise<void>;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { SharedStorage, SharedStoragePath } from './shared-storage';
import { Test } from '@nestjs/testing';
import { MarxanDirectory } from '../marxan-directory.service';
import { v4 } from 'uuid';

let sut: SharedStorage;
const randomSubdirectory = v4();

beforeEach(async () => {
const sandbox = await Test.createTestingModule({
providers: [
SharedStorage,
{
provide: MarxanDirectory,
useValue: {},
},
{
provide: SharedStoragePath,
useValue: `/tmp/${randomSubdirectory}`,
},
],
}).compile();

sut = sandbox.get(SharedStorage);
});

describe(`when trying to reach disallowed directory`, () => {
it(`should throw`, async () => {
await expect(sut.cleanup('../../etc/passwd')).rejects.toMatchInlineSnapshot(
`[Error: Directory traversal is not allowed.]`,
);
});
});

describe(`when contains null byte`, () => {
it(`should throw`, async () => {
await expect(
sut.cleanup('random-subdir/\0cat /etc/passwd'),
).rejects.toMatchInlineSnapshot(`[Error: Hacking is not allowed.]`);
});
});
Original file line number Diff line number Diff line change
@@ -1,28 +1,30 @@
import { Injectable } from '@nestjs/common';
import { Inject, Injectable } from '@nestjs/common';
import { v4 } from 'uuid';
import { promises } from 'fs';
import { resolve } from 'path';

import { AppConfig } from '@marxan-geoprocessing/utils/config.utils';
import { assertDefined } from '@marxan/utils';

import { TemporaryDirectory } from './ports/temporary-directory';
import { WorkingDirectory } from '../../ports/working-directory';
import { MarxanDirectory } from '../marxan-directory.service';

export const SharedStoragePath = Symbol('shared storage temporary directory');

@Injectable()
export class SharedStorage implements TemporaryDirectory {
readonly #tempDirectory: string;

constructor() {
const storagePath = AppConfig.get<string>(
'storage.sharedFileStorage.localPath',
);
assertDefined(storagePath);
this.#tempDirectory = storagePath;
}
constructor(
@Inject(SharedStoragePath) private readonly tempDirectory: string,
private readonly marxanDirectory: MarxanDirectory,
) {}

async cleanup(directory: string): Promise<void> {
// TODO check if starts with tempDirectory
if (this.#hasPoisonNullByte(directory)) {
throw new Error(`Hacking is not allowed.`);
}
Comment on lines 19 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we validate it on input, before cleaning up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dyostiq would you mind to elaborate? It is stateless, so you can pass any directory and it validates it on every use 🤔


if (this.#isDirectoryTraversal(directory, this.tempDirectory)) {
throw new Error(`Directory traversal is not allowed.`);
}

await promises.rm(directory, {
recursive: true,
force: true,
Expand All @@ -32,10 +34,20 @@ export class SharedStorage implements TemporaryDirectory {

async get(): Promise<WorkingDirectory> {
const directory = v4();
const fullPath = resolve(this.#tempDirectory, directory);
await promises.mkdir(resolve(this.#tempDirectory, directory));
// TODO replace output with the name from params
await promises.mkdir(resolve(this.#tempDirectory, directory, 'output'));
const fullPath = resolve(this.tempDirectory, directory) as WorkingDirectory;

await promises.mkdir(fullPath);

return fullPath as WorkingDirectory;
}

#hasPoisonNullByte = (path: string): boolean => path.indexOf('\0') !== -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

what about escaping by ..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


#isDirectoryTraversal = (fullPath: string, rootDirectory: string) =>
!fullPath.startsWith(rootDirectory);

async createOutputDirectory(inside: WorkingDirectory): Promise<void> {
const outputPath = this.marxanDirectory.get('OUTPUTDIR', inside);
await promises.mkdir(outputPath.fullPath);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@ import { WorkspaceBuilder } from '../../ports/workspace-builder';
import { LinkMarxan } from './ports/link-marxan';
import { TemporaryDirectory } from './ports/temporary-directory';
import { SymlinkBinary } from './symlink-binary';
import { SharedStorage } from './shared-storage';
import { SharedStorage, SharedStoragePath } from './shared-storage';

import { WorkspaceService } from './workspace.service';
import { MarxanConfig } from '../../marxan-config';
import { MarxanDirectory } from '../marxan-directory.service';
import { FileReader } from '../../adapters/file-reader';
import { AppConfig } from '@marxan-geoprocessing/utils/config.utils';

@Module({
providers: [
Expand All @@ -24,7 +27,15 @@ import { MarxanConfig } from '../../marxan-config';
provide: WorkspaceBuilder,
useClass: WorkspaceService,
},
{
provide: SharedStoragePath,
useFactory: () => {
return AppConfig.get<string>('storage.sharedFileStorage.localPath');
},
},
WorkspaceService,
FileReader,
MarxanDirectory,
],
exports: [WorkspaceBuilder],
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@ export class WorkspaceService implements WorkspaceBuilder {
await this.binaryLinker.link(this.marxanConfig.binPath, marxanBinaryPath);

console.log(`doing things in..`, workingDirectory);
return new Workspace(workingDirectory, marxanBinaryPath, async () =>
this.workingDirectoryService.cleanup(workingDirectory),
return new Workspace(
workingDirectory,
marxanBinaryPath,
async () => this.workingDirectoryService.cleanup(workingDirectory),
async () =>
this.workingDirectoryService.createOutputDirectory(workingDirectory),
Comment on lines +27 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't just pass an object here?
also, why isn't port abstract?

);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export class MarxanSandboxRunnerService {

await interruptIfKilled();
await inputFiles.include(workspace, assets);
await workspace.arrangeOutputSpace();

return new Promise(async (resolve, reject) => {
marxanRun.on('error', async (result) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,14 @@ export class Workspace implements Cancellable {
public readonly workingDirectory: WorkingDirectory,
public readonly marxanBinaryPath: string,
public readonly cleanup: () => Promise<void>,
public readonly assembleOutputDirectory: () => Promise<void>,
) {}

cancel(): Promise<void> {
return Promise.resolve(undefined);
return this.cleanup();
}

arrangeOutputSpace() {
return this.assembleOutputDirectory();
}
}