Skip to content

Fix encoding/decoding issue of UserInfo#41

Closed
safareli wants to merge 1 commit into
purescript-contrib:masterfrom
safareli:fix-userinfo
Closed

Fix encoding/decoding issue of UserInfo#41
safareli wants to merge 1 commit into
purescript-contrib:masterfrom
safareli:fix-userinfo

Conversation

@safareli
Copy link
Copy Markdown
Contributor

@safareli safareli commented Jan 25, 2018

Before this PR one couldn't use : or @ in username or password now it's possible.

To achieve this representation of UserInfo changed to record instead of just string as we need to encode username and password separately (without "connecting" :)

@safareli safareli requested a review from garyb January 25, 2018 13:35
@safareli
Copy link
Copy Markdown
Contributor Author

This issue raised when @ was part of the username for mongo connection url. Here I see that username and password are passed through decodeURIComponent http://mongodb.github.io/node-mongodb-native/2.2/tutorials/connect/authenticating/.

I adopted the encoding part from go as it's most straight forward, the one from node is way to cryptic
https://github.com/nodejs/node/blob/e22b8d0c46728ebdaf64176191ffa2bdd0f56be9/lib/url.js#L972

Before this PR one couldn't use `:` or `@` in username or password now it's possible.

To achieve this representation of UserInfo changed to record instead of just string as we need to encode username and password separately (without "connecting" `:`)
@garyb
Copy link
Copy Markdown
Member

garyb commented Jan 25, 2018

The problem with this approach is it assumes the userinfo stuff has a particular structure, which is not necessarily the case - what we have here is the most common case, but according to the spec, the format in there has no specific grammar (aside from the allowable characters).

There is some problem here though, probably just we should do percent encode/decode, since the grammar is:

userinfo    = *( unreserved / pct-encoded / sub-delims / ":" )

@safareli
Copy link
Copy Markdown
Contributor Author

will be fixed in #43

@safareli safareli closed this Feb 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants