-
Notifications
You must be signed in to change notification settings - Fork 251
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
Conversation
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
IMO it would also be nice to use rspec |
Agree; the |
There was a problem hiding this 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.
Good change to make 👍, however I have left the org that opened this request. Perhaps @rafaelgonzalez could update this PR on my behalf? |
Thanks @rafaelgonzalez 🎉 |
Looks good! Approving this and squashing it in to |
@junaruga Would you have a chance to update the history file accordingly? |
@perlun Yes I can, but it will be after we will have a permission for RubyGems. |
* 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)
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.