Skip to content

Fix for colon between username and password being encoded #15

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

seancunningham
Copy link

At very least, Mongo v. 2.4.6 does not like the colon between the username and password encoded. This update simply encodes them separately to avoid such issues.

@Marsup
Copy link
Collaborator

Marsup commented Mar 16, 2016

A more solid solution would probably be to use node's native url.format to build that thing.

@seancunningham
Copy link
Author

Yes, I agree. Truthfully it was a quick fix at first, as it was blocking me from something deemed more important. Circled back and fixed based on the suggestion.

@Marsup
Copy link
Collaborator

Marsup commented Mar 17, 2016

You shouldn't have forced the add on lib/queue. Also is it really solving your problem now ? Because it does a global encode on the auth as well.

@seancunningham
Copy link
Author

Yes, it is. I'm assuming it has to do with the assumption the colon needs to be encoded when using encodeURIComponent, where it is expected in the auth field.

@seancunningham
Copy link
Author

Also, I can remove lib/queue, if you want to merge it. I didn't want to publish my branch onto npm, but still needed it for a project - so I added lib/queue so it could be installed directly from github using npm.

@Marsup
Copy link
Collaborator

Marsup commented Mar 17, 2016

It already can, that's the whole point of the prepublish script. I'm doubtful it fixes your problem but ok.

@seancunningham
Copy link
Author

URL.format, and just not encoding the colon in the auth field, both work because the colon gets encoded with encodeURIComponent. Does your method work for you? I would be surprised to hear that user%3Apassword works as an auth field with mongo, but maybe I'm just running an older version. If you dig into URL format you'll find they ended up doing the same thing as my original fix (encoding each separately and just concating the string. (see here for ref).

@Marsup
Copy link
Collaborator

Marsup commented Mar 17, 2016

I mean in the current state of this PR, it doesn't seem any different from before on this encoding part.

@seancunningham
Copy link
Author

Have you tested it? Because what I am saying is that your version yields username**%3Apassword, whereas my first commit and this current commit both give username:**password, and I am having a hard time telling whether you are saying that you can't see the difference while thinking about it or that you have tested it and the encoded colon in the auth field works for the version of mongodb you are running (because it doesn't work for mine, mongo sees it as a single username string with no password).

@Marsup
Copy link
Collaborator

Marsup commented Mar 17, 2016

I haven't tested it but conceptually both versions encode user:password, the whole string, node also does it in a single pass (https://github.com/nodejs/node/blob/master/lib/url.js#L543).

@seancunningham
Copy link
Author

They don't escape the colon though - (https://github.com/nodejs/node/blob/master/lib/url.js#L933)

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