Skip to content
This repository was archived by the owner on Nov 16, 2019. It is now read-only.

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Sep 23, 2015

Without this, any file named config.gypi outside of node-gyp's build directory will be ignored.

Fixes: #13

Without this, any file named config.gypi outside of node-gyp's
build directory will be ignored.

Fixes: #13
@mscdex
Copy link
Contributor Author

mscdex commented Oct 21, 2015

@othiym23 Do you see any problems with this PR?

@othiym23
Copy link
Contributor

It looks fine, but I haven't had the time to play with it yet and make sure that it doesn't replicate the same information disclosure problems we were seeing before we blacklisted config.gypi. To evaluate that properly is going to take me sitting down and figuring out how gyp uses these files in general and look at a few use cases where config.gypi might need to be included with the package. If you have some examples, they'd be useful.

Also, we've had a pretty large backlog of stuff pile up on the npm CLI team, so it's probably going to take us a little time to get to some of these PRs on npm's ancillary dependencies. Thanks for putting this together!

@mscdex
Copy link
Contributor Author

mscdex commented Oct 21, 2015

As far as examples go, I can only point to my own, which for right now is mariasql. There are others in mscdex/node-mariasql#102 that are running into the same problem. I'm having to publish using an older npm version for now...

@mscdex
Copy link
Contributor Author

mscdex commented Oct 21, 2015

I should also note that it's not just publishing but also merely installing the module in some cases.

@othiym23 othiym23 self-assigned this Oct 30, 2015
@othiym23
Copy link
Contributor

Fair enough, the real problem is that I personally don't understand the ins and outs of how gyp produces and configures these files, so I'm assigning this to myself to do the necessary research to figure out if this is going to be safe to land. @ChALkeR, do you have thoughts on this?

@ChALkeR
Copy link

ChALkeR commented Oct 30, 2015

@othiym23
All the config.gypi-related leaks that I observed indeed happened only in build/config.gypi, I haven't seen any credentials in config.gypi files outside of build.

On the other hand, several config.gypi files that are outside from build (not build/config.gypi, so they will be re-introduced with this patch) claim that they are generated by node-gyp's "configure" step (as all the build/config.gypi files):

contextify_win32-0.1.12.tgz/runtime/config.gypi:# Do not edit. File was generated by node-gyp's "configure" step
contextify_win64-0.1.17.tgz/runtime/config.gypi:# Do not edit. File was generated by node-gyp's "configure" step
devil-windows-0.1.4.tgz/lib/v8-profiler/bld/config.gypi:# Do not edit. File was generated by node-gyp's "configure" step
devil-windows-0.1.4.tgz/lib/heapdump/bld/config.gypi:# Do not edit. File was generated by node-gyp's "configure" step
devil-windows-0.1.4.tgz/lib/ws/bld/config.gypi:# Do not edit. File was generated by node-gyp's "configure" step
dropper-1.1.0.tgz/.node/include/node/config.gypi:# Do not edit. Generated by the configure script.
dropper-1.1.0.tgz/.node/src/node-v0.10.33-darwin-x64/include/node/config.gypi:# Do not edit. Generated by the configure script.
exhibition-runner-1.0.0.tgz/.exhibition/iojs/include/node/config.gypi:# Do not edit. Generated by the configure script.
nodebb-0.8.2.tgz/node_modules_0.8.1/sitemap/env/include/node/config.gypi:# Do not edit. Generated by the configure script.
nodebb-0.8.2.tgz/node_modules_0.8.1/sitemap/env/src/node-v0.12.0-linux-x64/include/node/config.gypi:# Do not edit. Generated by the configure script.
pokitdok-nodejs-0.0.7.tgz/venv/include/node/config.gypi:# Do not edit. Generated by the configure script.

In theory, those could contain sensitive information.

@othiym23
Copy link
Contributor

othiym23 commented Nov 5, 2015

Rebased and landed as 02ffe21. @ChALkeR, since the purpose of this patch was to close an information leak, not break the ability of developers to ship native packages, I'm comfortable shipping this until we have some evidence that it's having problems (which, given that npm has been patched to no longer include credentials in the child process environment, should be less risky all around). Thanks for putting this together, @mscdex, and thanks for including a test.

@othiym23 othiym23 closed this Nov 5, 2015
@mscdex
Copy link
Contributor Author

mscdex commented Nov 5, 2015

Thanks! :-)

othiym23 added a commit to npm/npm that referenced this pull request Nov 5, 2015
Only filter config.gypi when it's in the build directory.

Credit: @mscdex
PR-URL: npm/fstream-npm#14
othiym23 added a commit to npm/npm that referenced this pull request Nov 5, 2015
Only filter config.gypi when it's in the build directory.

Credit: @mscdex
PR-URL: npm/fstream-npm#14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants