Skip to content

JS: Add model for shelljs#1196

Merged
xiemaisi merged 5 commits intogithub:masterfrom
asger-semmle:shelljs
Apr 5, 2019
Merged

JS: Add model for shelljs#1196
xiemaisi merged 5 commits intogithub:masterfrom
asger-semmle:shelljs

Conversation

@asger-semmle
Copy link
Contributor

After turning on full type extraction I noticed from false negatives from not modelling shelljs methods as TaintedPath sinks.

Evaluation on default.slugs is uneventful.

@asger-semmle asger-semmle requested a review from a team as a code owner April 3, 2019 08:49
xiemaisi
xiemaisi previously approved these changes Apr 3, 2019
Copy link

@xiemaisi xiemaisi left a comment

Choose a reason for hiding this comment

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

LGTM, but perhaps deserves a change note, seeing how massively popular shelljs is.

Consistently using the range pattern is a little verbose, but I like how it allows for a clean model of shelljs.exec 👍

@asger-semmle
Copy link
Contributor Author

Consistently using the range pattern is a little verbose

Yeah it might be overkill to use it twice for this model. Should we get rid of Instance and just have Member?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

GitHub repo > npm page?

* Support for the following frameworks and libraries has been improved:
- [koa](https://github.com/koajs/koa)
- [socket.io](http://socket.io)
- [shelljs](https://www.npmjs.com/package/shelljs)
Copy link

Choose a reason for hiding this comment

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

Suggested change
- [shelljs](https://www.npmjs.com/package/shelljs)
- [shelljs](https://github.com/shelljs/shelljs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are we linking to github repos instead of the NPM page, again?

Copy link

Choose a reason for hiding this comment

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

GitHub repo > npm page ?

I would say <

Copy link

Choose a reason for hiding this comment

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

The github repository is the official definition of the package, in the absence of a proper home page (like https://socket.io has). The npm page is just derived from the repository information, and our models are not tied to npm in any way.

Copy link

Choose a reason for hiding this comment

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

The npm page looks much more useful to me, and it has a link to the github repo.

Copy link

Choose a reason for hiding this comment

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

But this is not a matter of usefulness: it is a matter of documenting what we are doing, not how to use a package. I am fine either way, as long as we are consistent.
Should the preference be homepage > npm > repo?

Copy link

Choose a reason for hiding this comment

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

Yes, I would prefer that preference order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The NPM page is also the more stable link, as it's not unusual for github repos to be renamed or moved into an org as the project grows.

@xiemaisi
Copy link

xiemaisi commented Apr 3, 2019

Should we get rid of Instance and just have Member?

If you don't have a concrete need for it at the moment, perhaps we can simplify it for now.

xiemaisi
xiemaisi previously approved these changes Apr 3, 2019
ghost
ghost previously approved these changes Apr 3, 2019
@asger-semmle asger-semmle dismissed stale reviews from ghost and xiemaisi via d594e55 April 4, 2019 10:46
@xiemaisi xiemaisi added the JS label Apr 5, 2019
@xiemaisi xiemaisi merged commit cb22192 into github:master Apr 5, 2019
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.

2 participants