Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Incomplete rack dependency #663

Closed
aitor opened this Issue · 3 comments

2 participants

Aitor García Rey Erik Michaels-Ober
Aitor García Rey

The current rack dependency definition in the gemspec is not including any version and therefore suggesting that any release (including old rack 1.1.x versions) should work properly:

  spec.add_dependency 'rack'

from https://github.com/intridea/omniauth/blob/master/omniauth.gemspec#L8

This was correct until d6b1797 that included a call to Rack::Request#ssl?. This method was included for first time in Rack 1.3 (see https://github.com/rack/rack/blob/rack-1.3/lib/rack/request.rb) and it's not defined in previous rack versions.

I don't know if the actual intention of the gem's authors is to relay on rack for this check (and therefore the dependency in the gem should be updated to reflect that) or if the change should be reverted to keep omniauth compatible with rack <1.3... but if this point is made clear I could pull-request the change.

/cc @daniel-nelson @sferik

Erik Michaels-Ober
Owner

Thanks for reporting this. I'll revert that change and push a new gem.

Erik Michaels-Ober sferik closed this issue from a commit
Commit has since been removed from the repository and is no longer available.
Erik Michaels-Ober sferik closed this in dc4a827
Erik Michaels-Ober sferik referenced this issue from a commit
Erik Michaels-Ober sferik Revert "Rack::Request has an #ssl? method that handles this case. cal…
…l that rather than reproducing its logic"

This reverts commit d6b1797, which
introduces an unspecified dependency on rack 1.3.

Closes #663.
6d48f03
Aitor García Rey

Hi again @sferik,

Thank you very much for taking care of this issue so fast :). I don't know if this is needed but rack has improved/changed the way they test for ssl scheme in the last versions. Actually they fallback sequentially for the following env vars:

request.env['HTTPS']
request.env['HTTP_X_FORWARDED_SSL']
request.env['HTTP_X_FORWARDED_PROTO']

extracted from https://github.com/rack/rack/blob/master/lib/rack/request.rb#L70

Given those env vars are covering some edge cases, maybe it would be useful to add them to the omniauth check too? I know that not duplicating the logic was the motivation for the initial change, but I think those are useful fallbacks.

Erik Michaels-Ober
Owner

@aitor Could you put together a pull request for this? I’m hoping to ship version 1.1.3 later today or tomorrow.

Aitor García Rey aitor referenced this issue from a commit in aitor/omniauth
Aitor García Rey aitor Refactor ssl check to mimic Rack::Request#ssl? behavior as stated in #…
…663. I've removed the query assigment since it looked redundant just after the gsub and added a few test for a custom full_host that were not provided before.
41551da
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.