Skip to content

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Jun 5, 2020

Gets a TP for CVE-2018-6342

Does 2 things, and I'm somewhat unsure about both.

  1. Recognizes route-handlers from webpack-dev-server.

Example:

const webpackDevServer = require('webpack-dev-server');
new webpackDevServer(.., {
  before: function (app) {
    app.use(someRouteHandler());
  }
});
  1. Adds very basic support for importing from other packages.
    It is essentially very basic non-TypeScript monorepo support.

The resolveNeighbourPackage predicate is outside Import class because I got join-order issues otherwise. (Same reason for pragma[noinline]).

TODO:

  • tests
  • change note
  • evaluation

@erik-krogh erik-krogh added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Jun 5, 2020
@erik-krogh erik-krogh requested a review from a team June 5, 2020 14:53
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Nice and simple.

The import resolution is indeed something I wanted to handle via the monorepo support I've been working on, but given the simplicity of this solution I see no harm in landing that first.

@@ -733,7 +736,7 @@ module Express {
/**
* An Express router.
*/
class RouterDefinition extends InvokeExpr {
class RouterDefinition extends Expr {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, an unfortunate breaking change. Would it make sense to add a case in isRouter instead of this change? That's also how we incorporate type information in here.

@erik-krogh
Copy link
Contributor Author

erik-krogh commented Jun 19, 2020

A performance evaluation came back, and it doesn't look too good.

But, the performance penalty comes from resolving extra imports. For example: azure-sdk-for-node has 3613 new resolved imports.
The rest doesn't seem to affect performance.
I think @asgerf should make the call whether we continue with this or not.

(The rebase below didn't change anything, it just added tests and a change-note).

@erik-krogh erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Jun 19, 2020
@asgerf
Copy link
Contributor

asgerf commented Jun 22, 2020

I think we should go ahead with this. The performance cost is unfortunate but not really the fault of this PR. Resolving imports is definitely the right thing to do.

@erik-krogh erik-krogh marked this pull request as ready for review June 22, 2020 11:36
@erik-krogh erik-krogh requested a review from a team June 22, 2020 11:37
@semmle-qlci semmle-qlci merged commit 7a5aae7 into github:master Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants