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

Allow Dependencies on npm Packages #56

Closed
rodrigok opened this issue Aug 29, 2018 · 10 comments
Closed

Allow Dependencies on npm Packages #56

rodrigok opened this issue Aug 29, 2018 · 10 comments
Labels
❔ question A question that needs discussed. 🔍 r&td Research and testing development 🙋‍♂️ help wanted We want YOU to help! triaged This has been triaged

Comments

@rodrigok
Copy link
Member

@graywolf336 commented on Tue May 15 2018

Currently Apps can not bring in third party packages in a supported manner (yes there are ways around it), however this is a big need and if we don't allow this then Apps will be severely limited. However, just allowing Apps to include any and every npm package will open the door to security issues and even malware packages.

So, we need a way to provide "secure" way for Apps to define the npm packages they use. This will be a multi-step process. First up, we will add a npmDepends configuration section of the App.json configuration file. Then whenever someone submits an App to the Marketplace we will look through that section and ensure they seem to be trusted modules. After it has been marked as reviewed and "trusted", an automated system will package in the npm modules (we will need to look into the license restrictions here) into the App.

However, there will be Apps that want to not distribute themselves via the Marketplace for various reasons (let's say an in-house App). Due to this, we will provide a way for Apps to package the npm modules themselves but a big, not so nice, warning will be displayed when installing and enabling the App that warns of potential risks associated with that App.


I would like the thoughts of any and everyone on this manner. Does this make sense and is it logical to do this? Yes, the Apps Marketplace will already have a manual review process so adding one more task to it just makes sense.


@timkinnane commented on Wed May 16 2018

Hey Bradley, on a technical note, is npmDepends a common approach or specific to Meteor? My understanding is that the apps will not be Meteor packages, so why not just use standard package.json manifest? Seems like reinventing the wheel otherwise?

Generally, I would err on the side of less restriction rather than the tight controls you're suggesting. Every node package is just the tip of its own tree of dependencies, there's no feasible way to enforce security standards for all the code in use, so it seems arbitrary and unnecessarily restrictive to enforce oversight at that level. Not to mention an intensive task for manual reviewers.

I'd suggest just listing the included dependencies on the app's marketplace profile, perhaps with some stats for each package, e.g: number of downloads, number of dependents, up to date dependencies (david-dm.org badge). There's services available to link to or integrate, for auditing an NPM package too, e.g. packagequality.
Just found this article sharing some similar approaches: https://bytearcher.com/articles/evaluating-packages-1-check-community/

I think, in the interest of keeping the system open for app creators, it's reasonable to assume some responsibility lies with the people installing the app to make their own determination on it's quality. If you make the controls too restrictive, the approval process will become a bottleneck and add to the cognitive load of how to navigate this ecosystem and get something out there, which is already high coming in.


@JSzaszvari commented on Thu May 17 2018

@timkinnane Agreed ++


@graywolf336 commented on Fri May 18 2018

Thanks for the input guys 👍 we will keep this in mind when actually starting to work on this. :)


@graywolf336 commented on Wed May 30 2018

To do: Evaluate how vscode's extension manifest (as the App.json was influenced by it) https://code.visualstudio.com/docs/extensionAPI/extension-manifest

@rodrigok rodrigok added 🙋‍♂️ help wanted We want YOU to help! ❔ question A question that needs discussed. 🔍 r&td Research and testing development labels Aug 29, 2018
@graywolf336 graywolf336 mentioned this issue Sep 24, 2018
6 tasks
@wreiske
Copy link

wreiske commented Oct 12, 2018

(yes there are ways around it)

For the time being, can you provide the workaround? I've tried including the node_modules directory in the packaged.zip file, but that doesn't work.

Does the workaround include just installing the package globally on the rocket.chat server and then installing the package?

@rodrigok
Copy link
Member Author

@graywolf336 any ideas here? I think it's not possible by now.

@graywolf336
Copy link
Contributor

@rodrigok yes, there are workarounds, but I would rather not mention those as I don't want us to be forced to support them down the road since they're workarounds and not official ways..

@cardoso
Copy link
Contributor

cardoso commented Jul 19, 2019

Is this feature still missing?

@murtaza98
Copy link
Contributor

@graywolf336 @rodrigok can u provide the workaround which u mentioned above. I need this for an App that I'm developing. I understand that this is not the official way n that it has some security concern, however since the App-Engine currently doesn't support any new package installation, this is my only option.
Thanks in advance.

@rodrigok
Copy link
Member Author

@murtaza98 the only workaround that I know is to get the code of the package you want to use and copy to inside your application, the downside here is that you need to convert the code to typescript.

@cuonghuunguyen
Copy link
Contributor

@murtaza98 the only workaround that I know is to get the code of the package you want to use and copy to inside your application, the downside here is that you need to convert the code to typescript.

In case that npm dependency has the sub-dependencies (recursively) then it will be very painful to who is using that workaround :D

@matheusbsilva137 matheusbsilva137 added triaged This has been triaged and removed Backlog This has been triaged and should be worked on labels Jan 21, 2021
@mrsimpson
Copy link

mrsimpson commented Sep 22, 2021

Is it true that this issue still persists? You can't be serious about app development without allowing external dependencies!

@vmarinogg
Copy link

Hey, @mrsimpson! We are just launching the NPM support in our Apps Engine. In the next release, you will be able to use this feature. You can check here some notes about it https://github.com/RocketChat/Rocket.Chat/releases/tag/4.0.0-rc.0.

Important to mention, we are deploying this feature as a "beta" feature due to security concerns and permission management some NPM modules potentially require. So, if you face issues or restrictions, please let us know to check and adapt the solution and provide a better experience using it.

We will also update the documentation next few days with more details. :)

@graywolf336
Copy link
Contributor

graywolf336 commented Apr 30, 2022

I wanted to provide an update to everyone. The Apps Engine supports NPM packages! Unfortunately, we are still working on our internal processes for Apps submitted to Marketplace. We have opted for safety and blocked Apps with NPM packages for now while we finish them up. If you would like to use them in your App, please submit an email requesting NPM Modules be allowed on your publisher to: apps@rocket.chat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❔ question A question that needs discussed. 🔍 r&td Research and testing development 🙋‍♂️ help wanted We want YOU to help! triaged This has been triaged
Projects
None yet
Development

No branches or pull requests

10 participants