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

fix: added regex for blocking illegal characters in usernames #10

Closed
wants to merge 2 commits into from

Conversation

bcoe
Copy link

@bcoe bcoe commented Oct 20, 2016

The Problem

We ran into an issue recently where single-quotes (') were accepted by npm-user-validate, but rejected by CouchDB.

Linux and OSX systems do not play nice with ' in folder paths; and I think the correct solution is black-listing this character.

The Solution

I've added a regex that allows us to black-list specific illegal characters, starting with '.

To make sure I was writing appropriate tests, etc., I've upgraded tap and turned on coverage; I've also added Travis CI testing for modern Node.js.


var illegalCharacterRe = new RegExp('([' + [
"'"
].join(',') + '])')

Choose a reason for hiding this comment

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

I think ',' may be incorrect here?

@coveralls
Copy link

coveralls commented Oct 20, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 68635d9 on illegal-characters into 688552a on master.

Copy link

@chrisdickinson chrisdickinson left a comment

Choose a reason for hiding this comment

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

🐑

Copy link

@zkat zkat left a comment

Choose a reason for hiding this comment

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

@bcoe could you rebase this PR and get rid of the unrelated project changes? If you wanna update that stuff, could you make a separate PR?

@zkat zkat requested a review from iarna March 8, 2017 00:36
@zkat
Copy link

zkat commented Mar 8, 2017

@iarna looping you in because BIG HUGE EFFECT maybe

Copy link
Contributor

@iarna iarna left a comment

Choose a reason for hiding this comment

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

Bleeergh, ok, so, the concern here is that users of third party registries might be using usernames that conflict with this. Ok, probably not, but it could happen. =D

So, given that, I'm going to take this as a breaking change and we can include it npm@5. Just the same, if we do end up getting reports from third party registries then we will likely revert it, but I don't consider that likely.

@iarna
Copy link
Contributor

iarna commented Apr 21, 2017

Conveniently, we haven't had a 1.0 release yet, so 1.0.0 it is I guess. =D

@iarna
Copy link
Contributor

iarna commented Apr 21, 2017

Released as 1.0.0

@iarna iarna closed this Apr 21, 2017
@wraithgar wraithgar deleted the illegal-characters branch September 21, 2021 19:26
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.

None yet

5 participants