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

Module aliasing for Javascript 2 #3745

Open
inkz opened this issue Aug 23, 2021 · 5 comments
Open

Module aliasing for Javascript 2 #3745

inkz opened this issue Aug 23, 2021 · 5 comments

Comments

@inkz
Copy link
Member

inkz commented Aug 23, 2021

Intro

I proposed module aliasing for JavaScript a long time ago #285. But at that time there were higher priority issues with JavaScript (from the rule writing point of view), so I closed the issue when individual patterns for all possible ways to import a dependency were supported.

Now JS/TS support is much better. Also, the year of experience shows that we use patterns for identifying specific module usage quite a lot. That is why I would like to ask for a module aliasing feature again.

Here is the list of the most common ways of how you can introduce a dependency in JS/TS code and patterns for finding them:

1. CommonJS modules (https://nodejs.org/dist/latest-v14.x/docs/api/modules.html)

1.A.

let foo = require('./foo');
foo.something('yo');
$FOO = require('./foo');
$FOO.something();

1.B.

let fooSome = require('./foo').something;
fooSome('yo1');
$FOOSOME = require('./foo').something;
$FOOSOME();

1.C.

let { something } = require('./foo');
something('yo1');
{ something } = require('./foo')
something(...)

1.D.

let { something: smth } = require('./foo');
something('yo1');
{ something: $SMTH } = require('./foo')
$SMTH(...)

1.E. (antipattern, usually modules are not built to be inited asynchronously, no need to alias)

require("./foo").then(foo2 => {
  foo2.something('yo6');
})
require("./foo").then(($FOO) => {
  $FOO.something(...);
})

1.F. (antipattern, usually modules are not built to be inited asynchronously, no need to alias)

(async function() {
  const foo3 = await require('./foo');
  foo3.something('yo7')
})()
$FOO = await require('./foo');
$FOO.something(...);

2. ES modules (https://nodejs.org/dist/latest-v14.x/docs/api/esm.html)

2.A.

import * as foo1 from './foo';
foo1.something('yo2')
import * as $FOO from './foo';
$FOO.something(...);

2.B.

import {something} from './foo';
something('yo3')
import {something} from './foo';
something(...);

2.C.

import foofoo from "./foo";
foofoo.something('yo4')
import $FOO from './foo';
$FOO.something(...);

2.D.

import { something as smth } from "./foo";
smth('yo5')
import {something as $SMTH} from './foo';
$SMTH(...);

2.E. (used only for side-effect, no need to alias)

import './foo'
import './foo'

2.F. (antipattern, usually modules are not built to be inited asynchronously, no need to alias)

import("./foo").then(foo2 => {
  foo2.something('yo6');
})
import("./foo").then(($FOO) => {
  $FOO.something(...);
})

2.G. (antipattern, usually modules are not built to be inited asynchronously, no need to alias)

(async function() {
  const foo3 = await import('./foo');
  foo3.something('yo7')
})()
$FOO = await import('./foo');
$FOO.something(...);

all of this patterns work, but we need all of them to cover 100% of situations

Problem

the problem is that in our rules we use only one pattern, usually $FOO = require('./foo');, sometimes { $SMTH } = require('./foo'), or something contributed by the users that reflects their current coding style

  • on top of that, since the last issue (Module aliasing for Javascript #285) Node.js implemented native ES modules support and its usage is going to increase, many modules already moving to "ES only" versions (I'm using CommonJS! What should I do? sindresorhus/got#1789), we need to adapt and be able to support both systems in our rules
  • but there is no "good practice" way to import/require a module, that we could stick to, they are all useful in different situations and I believe it makes sense to cover all the situations in our rules, for now, it would require to have at least 8 more patterns added to each rule (at minimum, because it probably would also require to rewrite/add other patterns)
  • potentially implementing module aliasing for JS/TS can give us x8 times better coverage
  • we have a module aliasing for Python and Java, which helps a lot in writing rules, so I believe it makes sense to have this feature for JS/TS also
  • the difference between Python and JS is that both import and require allow specifying path to the module, not just the name (or namespace), e.g. require('../../src/foo/bar')

Implementation

(this is my opinion on how the new patterns might look like, maybe there is a better solution for that)

for CommonJS

use patterns like:

require('./foo').$SOMETHING(...)

to alias patterns from 1.A, 1.B, 1.C, 1.D

examples:

const { unlink } = require('fs');

unlink('/tmp/hello')

and

const foo = require('fs').unlink;

foo('/tmp/hello')

and

const fs = require('fs');

fs.unlink('/tmp/hello')

will be tackled by:

require('fs').unlink(...)

this also means that

require('./foo')(...)

going to find:

const foo = require('./foo')
foo('hello world')

for ES modules

use patterns like:

import('./foo').$SOMETHING(...)

to alias patterns from 2.A, 2.B, 2.C, 2.D

examples:

import { unlink } from 'fs';

unlink('/tmp/hello')

and

import { unlink as foo } from 'fs';

foo('/tmp/hello')

and

import * as fs from 'fs';

fs.unlink('/tmp/hello')

will be tackled by:

import('fs').unlink(...)

this also means that

import('./foo')(...)

going to find:

import * as foo from './foo';
foo('hello world')
@stale
Copy link

stale bot commented Nov 21, 2021

This issue is being marked stale because there hasn't been any activity in 30 days. Please leave a comment if you think this issue is still relevant and should be prioritized, otherwise it will be automatically closed in 7 days (you can always reopen it later).

@stale stale bot added the stale needs prioritization; label applied by stalebot label Nov 21, 2021
@inkz
Copy link
Member Author

inkz commented Nov 22, 2021

👋

@stale stale bot removed the stale needs prioritization; label applied by stalebot label Nov 22, 2021
@christophetd
Copy link

Definitely needed! Honestly, I mostly gave up on writing Javascript Semgrep rules due to this complexity

@ben-elttam
Copy link

I think all patterns even the anti-patterns (bad practices) should be covered so that a rule trying to find something (e.g. XSS) will work no matter how the module was imported.

@mschwager
Copy link
Contributor

It'd be really helpful if JS equivalences caught all these cases: https://semgrep.dev/playground/s/gx815.

Otherwise you need to manually specify each import / require style to, for example, simply look for a fs module call.

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

No branches or pull requests

6 participants
@christophetd @mschwager @inkz @IagoAbal @ben-elttam and others