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

Add SES lockdown to testrunners #605

Merged
merged 3 commits into from
Jan 19, 2021
Merged

Add SES lockdown to testrunners #605

merged 3 commits into from
Jan 19, 2021

Conversation

willclarktech
Copy link
Contributor

  • root: Add SES dependency
  • Add SES lockdown to testrunners

Part of #412

The CLI package still has the obscure error, so I couldn’t add SES to the testrunner there. Do we
want SES added to our applications? (Eg adding it now to the faucet.)

@willclarktech willclarktech added this to In progress in CosmJS via automation Jan 7, 2021
@willclarktech willclarktech moved this from In progress to Awaiting Review in CosmJS Jan 7, 2021
@webmaster128
Copy link
Member

webmaster128 commented Jan 7, 2021

@willclarktech do you get this error locally as well (see CI)?

@cosmjs/faucet-client: /home/circleci/project/node_modules/ses/lockdown.js:17
@cosmjs/faucet-client: import { assign } from './src/commons.js';
@cosmjs/faucet-client:        ^
@cosmjs/faucet-client: SyntaxError: Unexpected token {
@cosmjs/faucet-client:     at Module._compile (internal/modules/cjs/loader.js:723:23)
@cosmjs/faucet-client:     at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
@cosmjs/faucet-client:     at Module.load (internal/modules/cjs/loader.js:653:32)
@cosmjs/faucet-client:     at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
@cosmjs/faucet-client:     at Function.Module._load (internal/modules/cjs/loader.js:585:3)
@cosmjs/faucet-client:     at Module.require (internal/modules/cjs/loader.js:692:17)
@cosmjs/faucet-client:     at require (internal/modules/cjs/helpers.js:25:18)
@cosmjs/faucet-client:     at Object.<anonymous> (/home/circleci/project/packages/faucet-client/jasmine-testrunner.js:4:1)
@cosmjs/faucet-client:     at Module._compile (internal/modules/cjs/loader.js:778:30)
@cosmjs/faucet-client:     at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
@cosmjs/faucet-client: error Command failed with exit code 1.

Is it because node_modules/ses/lockdown.js contains JS syntax too modern for Node.js 10?

@willclarktech
Copy link
Contributor Author

@webmaster128 Yes, I missed this locally because I was running the tests with Node v12.

@willclarktech
Copy link
Contributor Author

@webmaster128
Copy link
Member

@kriskowal what Node.js versions are supported by the "ses" package? Is this documented somewhere?

Node.js 10 will EOL at the end of April, so we could think about dropping it. But it would be good to get this documented.

@webmaster128
Copy link
Member

@willclarktech what about a separate SES CI job that runs the tests in node.js 12 and we have something like

if (provess.env.SES_ENABLED) {
  require("ses/lockdown");
  // eslint-disable-next-line no-undef
  lockdown();
}

@willclarktech willclarktech moved this from Awaiting Review to In progress in CosmJS Jan 7, 2021
.circleci/config.yml Outdated Show resolved Hide resolved
@willclarktech willclarktech moved this from In progress to Awaiting Review in CosmJS Jan 19, 2021
@webmaster128 webmaster128 merged commit d0833a3 into master Jan 19, 2021
CosmJS automation moved this from Awaiting Review to Done Jan 19, 2021
@webmaster128 webmaster128 deleted the 412-ses branch January 19, 2021 18:58
@kriskowal
Copy link

kriskowal commented Jan 19, 2021

@kriskowal what Node.js versions are supported by the "ses" package? Is this documented somewhere?

Node.js 10 will EOL at the end of April, so we could think about dropping it. But it would be good to get this documented.

We at least informally support Node.js 10 since our partners at MetaMask use that too. cc @kumavis. For those cases, SES ships with a rolled-up CommonJS distribution like ses/dist/lockdown.cjs. In Node.js 10, you get ses/dist/ses.cjs by virtue of the package.json "main" entry when you require("ses"), and on anything more recent, require("ses/lockdown") also works. I think you need to expressly dig up require("ses/dist/lockdown.cjs") instead of require("ses/lockdown") that far back in time.

@hxrts
Copy link

hxrts commented Sep 6, 2021

Hey, what's the status of this? Should be part of our service agreement with Agoric.

@webmaster128
Copy link
Member

This is done and running. The remaining trouble was resolved by dropping support for the now unmaintained Node.js 10.

Does this answer your question @hxrts? I don't know about this service agreement.

@hxrts
Copy link

hxrts commented Sep 6, 2021

Ah, should have read above. Yes it does, though @kriskowal can maybe comment on whether Agoric plans to do anything further.

@kriskowal
Copy link

The best person to discuss the scope of Agoric’s service agreement is Vanessa Pestritto at Agoric.

My expectation is that I am to be available to assist if ever a problem arises with compatibility of a new dependency and to provide help upgrading SES, particularly for breaking-changes that might impact compatibility with CosmJS dependencies.

Notably, we’re planning a version bump (0.+1) to tighten security by forbidding the use of the Node.js domain module in any application that uses SES. Incorporating 0.minor version bump is not automatic and manually upgrading could reveal that some applications have an existing dependency on domain. I expect that I will need to be available to assist in relieving those dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants