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

Hash username to avoid invalid file paths #31

Merged
merged 2 commits into from
Apr 18, 2017
Merged

Conversation

elldritch
Copy link
Contributor

This resolves follow-up issues on #22 where some usernames create invalid file paths for the cache file.

const exclusions = ['--help'];

const hash = crypto.createHash('md5');
hash.update(user);
Copy link
Member

Choose a reason for hiding this comment

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

I was pretty sure this was how md5 hashes were done in node, but I just tried it and it didn't work. Can you please add a test for this also? Thanks

Copy link
Member

Choose a reason for hiding this comment

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

This looks to be due to encoding.

Copy link
Member

Choose a reason for hiding this comment

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

Could you string together the calls, e.g. crypto.createHash('md5').update(user, encoding).digest(encoding)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phated, what version are you using? The default encoding works fine for me on Node 5.10.1.

Fixed the call.

Copy link
Contributor Author

@elldritch elldritch Apr 19, 2016

Choose a reason for hiding this comment

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

Also, the old tests still work because they depend on the configfile exported by this file.

@elldritch
Copy link
Contributor Author

@phated does this look good to merge?

@phated
Copy link
Member

phated commented Jun 6, 2016

@ilikebits can you add a test where env.USER is set to something that would produce an invalid path on the filesystem? That's the one thing that is still outstanding.

@preppypiet
Copy link

Could you please complete this PR? Ran into this issue again today and this would fix it perfectly. Instead I had to run a custom script with code I grabbed from this lib to manually make the directory in case username had a / in it.

@phated
Copy link
Member

phated commented Apr 18, 2017

@tkellen are we good to merge this?

@tkellen
Copy link

tkellen commented Apr 18, 2017

I'm gunshy given how many issues we've had with this project, but yes, I think we are.

@phated
Copy link
Member

phated commented Apr 18, 2017

@tkellen me too, but I figured I'd be around my computer today and could field all the backlash 😛

@phated phated merged commit 317b166 into gulpjs:master Apr 18, 2017
@phated
Copy link
Member

phated commented Apr 18, 2017

Published as 2.1.0 (seems like a feature and to give more wiggle room in semver).

@phated
Copy link
Member

phated commented Apr 18, 2017

@tkellen gunshy not in vain, but I have it fixed with only a really weird, rare edge case that might encounter issues (multiple accounts without USER env that end up with the same json config but not matching).

@tkellen
Copy link

tkellen commented Apr 18, 2017

Ha! Thank you for taking the brunt of this @phated

phated pushed a commit that referenced this pull request Dec 17, 2018
phated pushed a commit that referenced this pull request Dec 17, 2018
phated pushed a commit that referenced this pull request Dec 17, 2018
phated pushed a commit that referenced this pull request Dec 17, 2018
phated pushed a commit that referenced this pull request Dec 17, 2018
phated pushed a commit that referenced this pull request Dec 17, 2018
phated pushed a commit that referenced this pull request Dec 17, 2018
phated pushed a commit that referenced this pull request Dec 17, 2018
phated pushed a commit that referenced this pull request Dec 17, 2018
phated pushed a commit that referenced this pull request Dec 17, 2018
phated pushed a commit that referenced this pull request Dec 17, 2018
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.

4 participants