search

neovim / node-client

Nvim Node.js client and plugin host
https://neovim.io/node-client/
MIT License
492 stars 53 forks source link

Update dependencies to the latest #182

Closed rhysd closed 3 years ago

rhysd commented 3 years ago

This PR updates all dependencies to the latest.

I fixed almost all errors, but one error which I could not fix is remaining in integration tests:

FAIL src/factory.test.ts
  ● Plugin Factory (decorator api) › cannot write to process.umask

    expect(received).toThrow()

    Received function did not throw

      138 |     const nvim = {} as any;
      139 |     const plugin = loadPlugin('@neovim/example-plugin-decorators', nvim, {});
    > 140 |     expect(() => plugin.functions.Umask.fn(123)).toThrow();
          |                                                  ^
      141 |   });
      142 |
      143 |   it('can read process.umask()', () => {

      at Object.<anonymous> (src/factory.test.ts:140:50)

This failure said that process.umask did not raise an error when an argument is given. Giving an argument to process.umask means trying to change the file creation mode mask. Expected behavior is throwing an exception that it cannot change the mask. node-client imports plugin package in sandbox with wrapping process.umask here. But it seems not working.

https://github.com/neovim/node-client/blob/5bfdd65ed4e0ec030567c944c327ad4839eaa30e/packages/example-plugin-decorators/src/index.js#L56

This umask function call actually can change the file creation mode mask without throwing any errors.

I did some investigation and found that sandbox.require() seems not working as intended in neovim/src/host/factory.ts. Even if a source file is imported via sandbox.require, process.umask is still [Function: wrappedUmask]. It is not monkey-patched correctly. When I call process.umask with vm.runInContext explicitly, it worked as expected:

vm.runInContext('process.umask(123)', sandbox);
// ERROR: Cannot use process.umask() to change mask (read-only)

I'm not understanding the sandboxed-require mechanism, but the implementation is highly depending on undocumented node's module implementation. I guess it was changed recently (for example, ESM was implemented recently in Node.js so commonjs implementation would also be updated).

https://github.com/neovim/node-client/blob/5bfdd65ed4e0ec030567c944c327ad4839eaa30e/packages/neovim/src/host/factory.ts#L62-L87

Close #176

rhysd commented 3 years ago

CI failed due to an error I mentioned: https://dev.azure.com/neovim/node-client/_build/results?buildId=202&view=logs&j=10ebf0cf-a747-5e7d-396f-3bca3359ee9a&t=07e6453b-2a65-536c-9763-92ba8a8be3e7&l=89

rhysd commented 3 years ago

@billyvg

BTW, sandbox.require is sandboxed, but Neovim plugin still can use require() and module.require(). For example, it can import fs module by require('fs'). node-client tries to hide stdin/stdout/stderr from plugins but it'd be still possible to access them by using fs.createReadStream or something. (please correct me if I'm wrong)

I wonder if this sandbox mechanism is effective. I feel it is difficult to remove all vulnerable APIs without removing require(). If so, I feel removing the sandbox mechanism would be better keeping plugin architecture simple and avoiding problems from using node's internal APIs.

What do you think?

justinmk commented 3 years ago

If so, I feel removing the sandbox mechanism would be better keeping plugin architecture simple and avoiding problems from using node's internal APIs.

yes. Looking at the git logs, I don't see an explanation of why sandbox was introduced. Looks like it's intended to isolate Nvim node.js plugins running in the node-client plugin host? Interesting, but...

rhysd commented 3 years ago

@justinmk @billyvg

looks like we are messing with the module cache?

Maybe. But I'm not sure since I don't know how to check the cache was hit or not. And the Module._cache object is too large to check manually. Outputting it with console.log(Module._cache) instantly scrolled my terminal over 10000 lines...

As far as I had some trials, this _compile method is not called while loading plugin via sandbox.require. Since sandbox.require relies on monkey-patching this _compile method, it would not be expected that it is not called.

https://github.com/neovim/node-client/blob/5bfdd65ed4e0ec030567c944c327ad4839eaa30e/packages/neovim/src/host/factory.ts#L127-L129

I don't know why, but the logic to import commonjs modules seemed to be changed. Since sandboxing relies on node's internal (=unstable) APIs, they may be changed even if major version is not changed. I know that the sandboxing worked previously (on Node.js v10 or v12), though.

it's not documented (what do plugin authors need to know/expect?)

Since this project was largely rewritten while I was leaving this project temporarily, I also don't know the reason.

I have created a draft PR #183 on top of this PR, which removes sandboxing. I confirmed all tests passed there after fixing some very flaky unit tests.

rhysd commented 3 years ago

@billyvg Sure, thanks for the quick reply. I'll rebase #183.