openfn / core Goto Github PK

While dealing with OpenFn/language-http#15, we found that VM2's Contextify/Decontextify functions were breaking the object structure of the Agent object.

Internally Node's _http_client.js checks the Agent looks correct by checking for the existance of the addRequest function.
In certain situations this function ceases to exist (or is placed somewhere else in the object) when the check fails.
See: https://github.com/nodejs/node/blob/master/lib/_http_client.js#L149

This issue can be recreated like this:

const { VM } = require('vm2');
const https = require('https');

console.log(typeof new require('https').Agent().addRequest);

function myFunc(args) {

  return {...args};
}

new VM({ sandbox: { https, myFunc,console } }).run(`
try {
  function threeFunc(args) {
      debugger;
      return {...args};
  }

  const three = threeFunc(new https.Agent());
  console.log(typeof three.addRequest);

  console.log(typeof (https.Agent()).addRequest);
  const one = myFunc(new https.Agent());
  console.log(typeof one.addRequest);

  const two = myFunc.apply(undefined, new https.Agent());
  console.log(typeof two.addRequest);


} catch (err) {
  console.error(err)
}
`);

NOTE this may very well not be solvable, or at least in a way that benefits us - i.e. we need proxied and sandboxed functions in order to protect against malicious code.

Can we somehow work with VM2 in a way that doesn't expose this problem, and if so - what does that look like?

We'd provided an option for test mode which would try to intercept http requests made with request (so... lots of language-packages until recently) but that no longer works with axios and request is deprecated. We had used https://www.npmjs.com/package/mitm , but after a hard look it feels like this really shouldn't be the responsibility of core.

I'd move to drop support for test mode and let folks set up their own proxies with axios if they really want. Thoughts @stuartc , @lakhassane , @chaiwa-berian ?

This issue is to track the new security vulnerabilities reported in vm2. The details we have right now are vague but it's something like:

1. Inject code into a promise via the @@species decorator: GHSA-cchq-frgv-rjh5 (More on this hack here: https://nvd.nist.gov/vuln/detail/CVE-2023-37466)
2. Inject code by the built-in node inspector GHSA-g644-9gfx-q4q4

I'll explore this a bit further but my feeling on the fixes is something like:

• Fail to compile any job with @@ annotations (no-one needs that stuff, let's just blanket ban it). If this is too broad we can just fail @@species annotations.
• Restrict access to the inspect module in the vm2 sandbox, if we haven't already. There's no need for anyone to try and use it.

In order to know how long an expression takes to compile and then execute is really useful when testing from external calls (i.e. engine).

As a start I propose we add the ability to log out precise timing information to at least:

• before & after compile
• before & after the expression is executed
• perhaps before & after the state is parsed

The link that appears on top of the repo, in the title (or description of the repo, I'm not sure) is not valid.
I think that it should be: https://www.openfn.org/.
;)

(I don't know if this is the correct place to put this concerns of mine, and if the proposed solution is good enough or not)

If I use credentials of type OAuth 2.0 in the Credentials page of https://openfn.org, then I will get accessToken as part of the state.configuration. But, if the accessToken expires, then I would no longer be able to use it for accessing protected resources. (unless I delete the expired Credentials and add new Credentials with the same information, redo all the previous steps for getting the token)

I think it might be best in OpenFn.org use the client_credentials grant type in the Credentials of type OAuth 2.0, then also add mechanism if the time has passed for the same seconds as expiresIn since the token issuance OR if when make HTTP request to API path and the response is telling that the access token is expired, then automatically get new access token by using the same information for getting the access token with client_credentials grant. (maybe put maximum retries of 3 for the getting new token phase, if cannot get new token then abort)

What do you think? Am I understanding them correctly? Is that a good way to approach the issue?

Might be useful references:
https://developers.google.com/identity/protocols/oauth2
https://stackoverflow.com/a/43349958
https://cloud.google.com/apigee/docs/api-platform/security/oauth/oauth-20-client-credentials-grant-type

In order to make calling the runtime via a node require, it would be nice to be able to call the individual commands directly i.e.:

node -r @openfn/core/lib/cli -e 'execute(...)

Or similar..

In order to facilitate expressions that run based off a previous execution, the final/resulting state needs to be accessible. The approach we take in this initial release of this feature will be to add a runtime argument to a compiled expression that on completion logs the final state to disk.

$ diesl compile ... #as usual
$ ./compiled_exp.js -S final_state.json

It looks like our yargs implementation is woefully out of date - and I have a feeling it's making calling the cli (in slightly different ways) really difficult.

Allow @openfn/language-pack to be a valid argument for -l

This could be used to more reliably determine both the location of the module, but also
it's version number etc.

require.resolve("./priv/openfn/runtime/node_modules/@openfn/core/package.json")

If you try and create an expression like this:

get(
  "http://localhost:9292?flip=left"
).then(console.log.bind(console))

Turns into:

(function() {
  return execute(get(
    "http://localhost:9292?flip=left"
  ).then(console.log.bind(console)))(state);
})();

The compiler attaches the ...(state) callExpression onto the .then instead of the operation. This makes sense given how everything was originally designed - however it would be really nice not to have to use alterState in order to do this.

What we really want is the compiler to know that (in this case), get is an operation that needs to be called with state and add the call expression to that instead:

(function() {
  return execute(get(
    "http://localhost:9292?flip=left"
  )(state).then(console.log.bind(console)));    // <== callExpression on Operation instead
})();

While testing #27, we got an error in the validate transform where the adaptor was being incorrectly passed to the transformer where the correct error should have been "Can't find function", we got an error on the ASTs .loc property.

Why is that, and how can we fix it?

Currently the execute command on Diesl does not use the compiler.
The command is also only used during development flows, and so has been
safely ignored for now.

Execute is extremely useful as it has no static dependency tracking - sacrificing safety for ease of use.

There appears to be a happy middle ground, by dropping doclets from the compile step.
We can achieve a similar level of safety by introspecting the language pack module for exports
during compile, and then detect call expressions in the user expression using the scope lookup facility in ast-types and esprima.

The compile step has become a little cumbersome to work with, and efforts to align execute and compile will go a long way in bringing developing and testing expressions and language packs closer to how production executes them.

Instead of having a growing list of configuration keys, or writing the state to disk before; consider those directives existing in the expression itself (being either added automatically by the server or directly by the user).

const state = getState(...)
const configuration = getConfiguration(...)

This would allow other things like multiple credentials for more complex jobs...

const myCert = OpenFn.getCredential('myCert')
...

And last but not least, unpacking finalState from core.

OpenFn.writeState(state)

The OpenFn provided functions would need to be bound to the job on the server in order to authenticate and tie back any data to the specific invocation etc.

This pattern is only achievable once we are able to write more than just callExpressions on the root - same as #21