JS: Support sub modules #12975

tyage · 2023-04-30T09:44:51Z

Some npm packages have sub modules which have path suffix after the module name.
For example, @github/webauthn-json has sub module @github/webauthn-json/extended and its structure looks like this.

node_modules/
  - @github/webauthn-json/
    - package.json
    - extended/
      - package.json

I updated NPM.qll so that the CodeQL finds these modules.

https://nodejs.org/api/modules.html#loading-from-node_modules-folders

It is possible to require specific files or sub modules distributed with a module by including a path suffix after the module name. For instance require('example-module/path/to/file') would resolve path/to/file relative to where example-module is located. The suffixed path follows the same module resolution semantics.

erik-krogh

Mostly OK 👍

But I got a few comments.

erik-krogh · 2023-05-01T10:31:35Z

javascript/ql/lib/semmle/javascript/NPM.qll

+      parentPackageName = parentPackage.getPropStringValue("name") and
+      parentDir.indexOf("node_modules") != -1 and
+      currentDir != parentDir and
+      currentDir.indexOf(parentDir) = 0 and


You are doing a string-comparison here, and I'm not a fan of that.

You could instead drop the .getAbsolutePath(), and then do something like:
parentDir.getAChildContainer+() = currentDir

Thank you!
That looks better!
I refactored the codes here.

erik-krogh · 2023-05-01T10:32:46Z

javascript/ql/test/library-tests/NPM/src/node_modules/parent-modue/main.js

@@ -0,0 +1 @@
+export default "parent";


You have a typo in the folder name.
You have named the folder parent-modue instead of parent-module.

I tried to run your test-case locally using node (to confirm that the behavior is correct), and it failed due to the typo.

Oops, sorry.
I'll fix it soon.

erik-krogh · 2023-05-02T07:19:11Z

javascript/ql/lib/semmle/javascript/NPM.qll

+    exists(PackageJson parentPkg, Container currentDir, Container parentDir |
+      currentDir = this.getJsonFile().getParentContainer() and
+      parentDir = parentPkg.getJsonFile().getParentContainer() and
+      parentDir.getParentContainer+().getBaseName() = "node_modules" and


Suggested change

parentDir.getParentContainer+().getBaseName() = "node_modules" and

parentDir.getParentContainer().getBaseName() = "node_modules" and

I'm not sure I like that transitive closure.
Your test only covers that the parentDir is directly contained within node_modules, and I'm not sure what happens if parentDir is not directly inside node_modules.

There is a scoped package such as @babel/core.
In this case pakcage.json located in node_modules/@babel/core/ and there is no package in node_modules/@babel/.
AFAIK, this is the only case that parentDir is not directly inside node_modules.
Do you think it is better to rewrite this code to the following one?

parentDir.getParentContainer().getBaseName() = "node_modules" or parentDir.getParentContainer().getParentContainer().getBaseName() = "node_modules" and

Do you think it is better to rewrite this code to the following one?

Yes, that could work.
And in the .getParentContainer().getParentContainer() case you can check that the packagename of the parentPkg contains "/".

Hi, I fixed the code.
Thanks for your review!

erik-krogh · 2023-05-03T08:32:35Z

javascript/ql/lib/semmle/javascript/NPM.qll

+      parentPkgName = parentPkg.getPropStringValue("name") and
+      (
+        parentDir.getParentContainer().getBaseName() = "node_modules"
+        or
+        // Scoped package is located in node_modules/@scope/pkgname
+        parentDir.getParentContainer().getParentContainer().getBaseName() = "node_modules" and
+        exists(parentPkgName.indexOf("/"))
+      ) and


Suggested change

parentPkgName = parentPkg.getPropStringValue("name") and

(

parentDir.getParentContainer().getBaseName() = "node_modules"

or

// Scoped package is located in node_modules/@scope/pkgname

parentDir.getParentContainer().getParentContainer().getBaseName() = "node_modules" and

exists(parentPkgName.indexOf("/"))

) and

parentPkgName = parentPkg.getPropStringValue("name") and

On further thought (sorry for the many rounds of feedback).
Do we need to restrict to just packages contained within a node_modules folder?

Monorepos that contain many packages could experience this pattern without a node_module folder being part of it?

I think developers need to define exports property in package.json to access other packages, if they aren't located in node_modules.
I think removing these codes might cause some false positives, but they might be rare case, so we might be able to accept them...?

I looked at some packages and in the case of @github/webauthn-json, they defined exports property so that @github/webauthn-json/extended will work.
This means that the package name is defined independently of the directory structure.

https://github.com/github/webauthn-json/blob/e932b3585fa70b0bd5b5a4012ba7dbad7b0a0d0f/package.json#L18
https://nodejs.org/api/packages.html#exports-sugar

If we remove the exports property, @github/webauthn-json/extended will not work.

$ node -e 'console.log(require("@github/webauthn-json/extended"))' { base64urlToBuffer: [Getter], ... supported: [Getter] } $ gsed -i 's/exports/foobar/' package.json $ node -e 'console.log(require("@github/webauthn-json/extended"))' node:internal/modules/cjs/loader:936 throw err; ^ Error: Cannot find module '@github/webauthn-json/extended'

In https://github.com/keystonejs/keystone, we can call subpath modules like @keystone-6/core/admin-ui/router after npm install.
In this case, npm workspaces put the symlinks in the node_modules directory.

https://github.com/keystonejs/keystone/blob/5ac8ef453/package.json#L109
https://docs.npmjs.com/cli/v7/using-npm/workspaces

I think developers need to define exports property in package.json to access other packages, if they aren't located in node_modules.

We already have some support for "exports" elsewhere.

My thinking was that we most of the time ignore node_modules folder when we parse a project, so your contribution is very rarely relevant.

but they might be rare case, so we might be able to accept them...?

I'll run an evaluation if you try to remove the node_modules restriction.
If the results are noisy (or if the performance goes bad), then we can revert the change again.

Ok!
I committed your suggestion.👌

The failing tests (see the CI below) revealed something interesting.

We get some package names that include node_modules.
E.g. the Dependencies.ql test finds a new package-name: test-package/node_modules/bundled-package.
I don't think that was what you meant with this contribution? so maybe filter those out?

I did some evaluations (internal links: one, two), and that looks fine.
Nothing happened in those evaluations, everything is the same.

Oh I see.
I'll filter them out.

It looks fine now?

It looks OK.
I'll run an evaluation on it later, but our system for doing so is having some issues right now.
And I'm on PTO thursday+friday, so it might be until early next week before I can check it out.
Don't worry, I haven't forgotten about your PR.

Thanks!
Enjoy your holiday! 😀

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

javascript/ql/lib/semmle/javascript/NPM.qll

erik-krogh

Looks good 👍

Evaluation (internal link) looks OK.

tyage added 3 commits April 30, 2023 18:07

JS: support submodules

c0cf0b4

JS: Add test for sub module

71952fe

JS: change note

80d401f

tyage requested a review from a team as a code owner April 30, 2023 09:44

github-actions bot added documentation JS labels Apr 30, 2023

tyage mentioned this pull request Apr 30, 2023

JavaScript: Add some new XSS sinks and sources of Next.js (and some extra improvements) github/securitylab#747

Closed

2 tasks

Fix comment.

f52c845

erik-krogh self-assigned this May 1, 2023

erik-krogh reviewed May 1, 2023

View reviewed changes

tyage added 2 commits May 2, 2023 12:00

Fix typo in test

0d99157

JS: drop string comparison

be9c8d2

erik-krogh reviewed May 2, 2023

View reviewed changes

JS: check scoped package and normal package

22f5b7a

erik-krogh reviewed May 3, 2023

View reviewed changes

tyage and others added 3 commits May 10, 2023 19:36

Update javascript/ql/lib/semmle/javascript/NPM.qll

f6a8cd2

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

JS: ignoresub pkgs in node_modules directory

6f66c04

formatting

93af0d0

erik-krogh reviewed May 22, 2023

View reviewed changes

javascript/ql/lib/semmle/javascript/NPM.qll Outdated Show resolved Hide resolved

update qldoc

653cd86

erik-krogh approved these changes May 22, 2023

View reviewed changes

erik-krogh merged commit e658177 into github:main May 23, 2023

tyage deleted the support-sub-modules branch May 23, 2023 07:26

	parentDir.getParentContainer+().getBaseName() = "node_modules" and
	parentDir.getParentContainer().getBaseName() = "node_modules" and

JS: Support sub modules #12975

JS: Support sub modules #12975

Uh oh!

Conversation

tyage commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyage May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tyage commented Apr 30, 2023 •

edited

Loading

erik-krogh May 2, 2023 •

edited

Loading

tyage May 4, 2023 •

edited

Loading

erik-krogh May 11, 2023 •

edited

Loading