Transform which ran with v1 not working with v3

[rreusser]

trafficstars

Hi! cwise has had some long-running issues/PRs to upgrade its usage of static-module and get rid of some security warnings, but it seems that the particular usage static-module is no longer functioning under static-module 3.*.

The cwise transform finds references to var cwise = require('cwise'); cwise({...}) and replaces them with evaluated code.

I'm guessing a bit, but it seems like maybe the usage as a bare function require as opposed to properties on the require (i.e. require('cwise')(...) as opposed to require('fs).readFileSync(...)) are not working.

FWIW, the transform has not changed and so does still function and get triggered correctly.

I've detailed a test case here.

Glad to debug a bit further, but I'd thought I'd check to see if this might just need a small API usage update instead of involved debugging. Thanks!

/cc @archmoj @etpinard

[goto-bus-stop]

i'll have a look at this tomorrow. the big change in v3 was that unknown uses of a module are now kept, instead of removed (risking runtime errors). but v3 also added scope tracking which was a fairly large refactor that may have broken some stuff!

require('xyz')('') is at least "intended" to work, but maybe it's bugged.

[goto-bus-stop]

the new static-eval doesn't support function expressions like the body option in:

cwise({
  body: () => {}
})

I think because of this patch https://github.com/substack/static-eval/pull/18

static-module has a special case for function arguments as callbacks:

readFile('xyz', function () {})

but not for object expressions. It might be a bit harder to do the same for those :/

A possible solution may be to have a static-module option or a method like staticModule.ast that passes the parsed AST nodes to functions, instead of statically evaluated values. cwise could use static-eval to evaluate strings and other primitive types and stringify the AST for function arguments using escodegen.

[archmoj]

@goto-bus-stop possibly any update on this?

[goto-bus-stop]

no, i forgot about it :smile: i'll aim to work on this on Friday. a PR would be welcome as well of course. I think the way to go here is to change the current code to pass in ASTs, which can be exposed as staticModule.ast, then implement the current API in terms of that.

[archmoj]

Many thanks @goto-bus-stop. That would be great news for maintaining many webgl modules. cc: @etpinard

[archmoj]

@goto-bus-stop My apologies for asking one more time. Wondering if there may be progress on this issue? Many thanks.

[goto-bus-stop]

The refactor is a bit trickier than I anticipated, and I haven't had the time to make it work yet unfortunately :(

[Domino987]

Hi @goto-bus-stop , i was wondering if you could refactor it?

[telamonian]

Hey @goto-bus-stop, thanks for working on fixing the issues with cwise and static-module. I have a professional interest in seeing cwise get upgraded past its security issues. Currently at my company, cwise's security issues are a blocker for all of the (way upstream) Jupyterlab Plotly extensions, which all depend on cwise through numerous dependency chains.

If you're still interested in working on this fix, I'd be more than glad to pitch in some of my time to help. Is your work so far on the refactor publicly viewable anywhere?

[goto-bus-stop]

@telamonian I think I tried a few approaches but none of them worked out so I discarded them.

For now @archmoj's approach where we just add an option to opt back into the unsafe behaviour is likely best—it's only insecure if you use it on untrusted code anyway…

[goto-bus-stop]

#56 updates to the static-eval with @archmoj's fix but I'm not sure if it's enough. I tried using that branch in my local cwise clone and passing { allowAccessToMethodsOnFunctions: true } as options but the output of the snippet in https://github.com/scijs/cwise/pull/25#issuecomment-442942342 still contains a full JS parser. could one of you confirm?

[archmoj]

#56 updates to the static-eval with @archmoj's fix but I'm not sure if it's enough. I tried using that branch in my local cwise clone and passing { allowAccessToMethodsOnFunctions: true } as options but the output of the snippet in scijs/cwise#25 (comment) still contains a full JS parser. could one of you confirm?

@goto-bus-stop thanks for the follow ups. It works. And with cwise transform option there is no unused parser in the bundle. Please see my new PR at https://github.com/scijs/cwise/pull/32 that make use of your #56 PR.

[archmoj]

@goto-bus-stop update: you are right. There is still that parser problem.

[goto-bus-stop]

hm, static-eval might be bailing out somewhere else as well then :/

[archmoj]

Please note that the static-module/static-eval bump issue was addressed in plotly.js v1.54.4 by not using cwise.

[telamonian]

Please note that the static-module/static-eval bump issue was addressed in plotly.js v1.54.4 by not using cwise.

Hooray!