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

Basic Auth allows for ':' character in passwords #197

Merged
merged 2 commits into from Feb 6, 2014

Conversation

tobym
Copy link
Contributor

@tobym tobym commented Feb 5, 2014

Closes #193

@softprops
Copy link
Member

I wonder if we can do this in a way that that avoids creating lists at matches on a regex that more closely describes this

val Credentials = "([^:]*):(.*)".r
":" match { case Credentials(user, pass) =>  (user, pass) } // ("","")
"a:b" match { case Credentials(user, pass) =>  (user, pass) } // (a,b)
"a:b:c" match { case Credentials(user, pass) =>  (user, pass) } // (a,b:c)
"a:" match { case Credentials(user, pass) =>  (user, pass) } // (a,"")
":b" match { case Credentials(user, pass) =>  (user, pass) } // ("",b)

What are your thoughts on that approach?

@n8han
Copy link
Member

n8han commented Feb 5, 2014

I'm not a fan of solving it by regex, but I had the same thoughts about this re-joining solution when it was proposed in the issue. Actually, I mentioned this to @tobym in an email about the bug bash, and suggested

I think it would be better to use String.split(":", 1)

So when I saw this PR I assumed my idea didn't work out somehow, and was going to merge. But since we're all here, @tobym , what was the issue with that approach?

@tobym
Copy link
Contributor Author

tobym commented Feb 5, 2014

string.split(":", 2) is a good option. Will update, along with other test cases. Tests are looking a bit unwieldy though.

@n8han
Copy link
Member

n8han commented Feb 5, 2014

Oh, cool. I guess I disagree with java about what "the number of times the pattern is applied" means, but 2 looks like it does what I had in mind.

n8han pushed a commit that referenced this pull request Feb 6, 2014
Basic Auth allows for ':' character in passwords
@n8han n8han merged commit ef87dcb into unfiltered:master Feb 6, 2014
@tobym tobym deleted the feature/193 branch February 6, 2014 14:34
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.

None yet

3 participants