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

Remove new line from basic auth #185

Merged
merged 4 commits into from
Jul 12, 2017

Conversation

absrd
Copy link
Contributor

@absrd absrd commented Jun 6, 2017

closes #64

Copy link
Contributor

@perlun perlun left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks!

@junaruga
Copy link
Contributor

junaruga commented Jun 18, 2017

Umm, this PR's traivs test does not complete ..
@absrd Can you rebase your commit message or something to restart Travis?

git commit --amend
git push xxxxx your_branch -f

@absrd absrd force-pushed the 64_remove_new_line_from_basic_auth branch from 90f8234 to 7c04367 Compare June 18, 2017 10:23
@absrd
Copy link
Contributor Author

absrd commented Jun 18, 2017

@junaruga Done

@junaruga
Copy link
Contributor

@absrd thanks!
@perlun the Travis test still does not start. Do you know why?

@dennissivia
Copy link

@junaruga Even though I think the change is nice in the sense, that it will make future uses easier to write/read, I just want to want to mention that this looks like a breaking change to the public API. Isn't it?

@perlun
Copy link
Contributor

perlun commented Jul 12, 2017

@scepticulous

Even though I think the change is nice in the sense, that it will make future uses easier to write/read, I just want to want to mention that this looks like a breaking change to the public API. Isn't it?

Technically yes, but the previous approach is clearly wrong. Newlines should not be included in the HTTP headers.

So, in that sense it is a "bug fix" because the previous approach is incorrect. I think it's permissible by SemVer rationale to include bug fixes like this. (and, we aren't at 1.0 yet so in that sense "anything is permissible", even though we must of course be careful with the thousands/millions of people who use the gem)

@dennissivia
Copy link

Good point. You are right, we are not creating a valid real world header, thus I agree with it being a bugfix. Thanks.

@perlun perlun changed the title Fixes #64 - Removes new line from basic auth Remove new line from basic auth Jul 12, 2017
@perlun
Copy link
Contributor

perlun commented Jul 12, 2017

Thanks. I'm unsure why Travis fails on this one, will download it locally and test it before merge as a workaround.

@perlun
Copy link
Contributor

perlun commented Jul 12, 2017

Works fine for me, let's merge and hope Travis runs correctly after that. Thanks @absrd for your contribution! 👏

@perlun perlun merged commit 0987ddc into rack:master Jul 12, 2017
alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this pull request Apr 5, 2021
* Fixes rack#64 - Removes new line from basic auth
* Fixed grammar
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.

basic auth header should not insert linebreaks
4 participants