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

harden v8flags for people with jacked up environment variables #15

Merged
merged 1 commit into from
Apr 1, 2015

Conversation

tkellen
Copy link

@tkellen tkellen commented Mar 29, 2015

would love some feedback on this mess if you have time @contra @phated @sindresorhus.

basically, this tries to deal with fucked up environments where a temp directory cannot be found using os.tmpdir(), which is (sadly) not all that uncommon. i hate this module.

@tkellen tkellen force-pushed the harden branch 3 times, most recently from a3d56b0 to 512dbd1 Compare March 29, 2015 02:27
@phated
Copy link
Member

phated commented Mar 29, 2015

@tkellen could a lot of this logic be handled by a library like https://github.com/bruce/node-temp ??

@tkellen
Copy link
Author

tkellen commented Mar 29, 2015

Doesn't seem like it, it relies on os.tmpdir as well and will fall over in the same way v8flags currently does already :/

@phated
Copy link
Member

phated commented Mar 29, 2015

@tkellen okay, but these are kind of config files, why don't we use a module that stores config files in the user's home directory instead?

@tkellen
Copy link
Author

tkellen commented Mar 29, 2015

That's a fantastic idea. Do you know of any widely used and reliable modules for that?

@phated
Copy link
Member

phated commented Mar 29, 2015

https://www.npmjs.com/package/configstore is what yeoman uses

@phated
Copy link
Member

phated commented Mar 29, 2015

derp, that uses yml files, could probably just use https://www.npmjs.com/package/xdg-basedir to get the config directory

@tkellen
Copy link
Author

tkellen commented Apr 1, 2015

Well, unless anyone has any objections, I'm going to publish this and see if it does any better.

@tkellen
Copy link
Author

tkellen commented Apr 1, 2015

(I've updated it to use user-home)

@phated
Copy link
Member

phated commented Apr 1, 2015

let me review, didn't see the update

@tkellen
Copy link
Author

tkellen commented Apr 1, 2015

Roger. Just updated the error message.

// to write to it.
function openConfig (cb) {
var userHome = require('user-home');
var dir = '/cow'+path.join(userHome || os.tmpdir(), configfile);
Copy link
Member

Choose a reason for hiding this comment

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

why /cow?

Copy link
Author

Choose a reason for hiding this comment

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

heh, was testing the failure condition while updating the error message. removed.

@tkellen tkellen force-pushed the harden branch 2 times, most recently from f4b9de8 to af4f80e Compare April 1, 2015 00:12
function openConfig (cb) {
var userHome = require('user-home');
var dir = path.join(userHome || os.tmpdir(), configfile);
fs.open(dir, 'a+', function (firstOpenErr, firstFd) {
Copy link
Member

Choose a reason for hiding this comment

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

are you sure you want to append? If that file is already there, we could end with invalid json

Copy link
Author

Choose a reason for hiding this comment

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

not going to append, but i need it to throw if the file isn't there, i can't use w+

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, need to double check this makes any sense, actually. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

Okay yeah, this is fine. If the file has a size larger than 0 it will never be written to.

@tkellen tkellen force-pushed the harden branch 2 times, most recently from d95e583 to 5fc7dc1 Compare April 1, 2015 00:37
@tkellen tkellen merged commit cf81d5b into master Apr 1, 2015
@Siilwyn Siilwyn deleted the harden branch July 15, 2017 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants