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

Set scheme when using ENV to enable SSL #155

Merged
merged 5 commits into from May 12, 2017
Merged

Set scheme when using ENV to enable SSL #155

merged 5 commits into from May 12, 2017

Conversation

neilang
Copy link
Contributor

@neilang neilang commented Jan 4, 2017

When testing with secure cookies the cookie jar expects a scheme to be set. However the scheme will be nil if not provided but SSL was requested via the env hash.

e.g.

it 'should make SSL request and set secure cookies' do
  get '/path', {}, 'HTTPS' => 'on'
end

@@ -240,6 +240,7 @@ def env_for(path, env)
def process_request(uri, env)
uri = URI.parse(uri)
uri.host ||= @default_host
uri.scheme ||= "https" if env["HTTPS"] == "on"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should go with "true" and "false" here instead of on and off? What do you think @neilang? /cc @junaruga

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My memory is a little vague around the details of this change. My guess is that this follows an already defined convention when testing with SSL. Ideally you could support both versions e.g. %w(on true).include?(env["HTTPS"]). However I would delegate that decision to the maintainers, there is probably good reason for using "on" that I'm not aware of.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, if we have used "on" previously I'm fine with that. Thanks for the input @neilang

@dennissivia
Copy link

IMO it would also be nice to use rspec expect instead of should

@perlun
Copy link
Contributor

perlun commented Apr 29, 2017

IMO it would also be nice to use rspec expect instead of should

Agree; the should syntax is deprecated.

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.

Please change according to @scepticulous comment.

@neilang
Copy link
Contributor Author

neilang commented May 1, 2017

Good change to make 👍, however I have left the org that opened this request. Perhaps @rafaelgonzalez could update this PR on my behalf?

@rafaelgonzalez
Copy link

Hi there @neilang 👋

Updated the PR @perlun, @scepticulous. Let me know if you are happy with the changes.

@neilang
Copy link
Contributor Author

neilang commented May 11, 2017

Thanks @rafaelgonzalez 🎉

@perlun
Copy link
Contributor

perlun commented May 12, 2017

Looks good! Approving this and squashing it in to master.

@perlun perlun merged commit 2b23d2a into rack:master May 12, 2017
@perlun
Copy link
Contributor

perlun commented May 12, 2017

@junaruga Would you have a chance to update the history file accordingly?

@junaruga
Copy link
Contributor

@perlun Yes I can, but it will be after we will have a permission for RubyGems.
I want to update the history file at once as a preparation of the next release.

alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this pull request Apr 5, 2021
* Set scheme if `nil` when using ENV to enable SSL
* Replace should syntax with expect syntax in Rack::Test::Session spec

Thanks to @neilang and @rafaelgonzalez for doing this. (@perlun was merely the person rebasing their work into master)
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