Incomplete rack dependency #663

Closed
aitor opened this Issue Feb 13, 2013 · 3 comments

Projects

None yet

2 participants

@aitor

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

@sferik

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

@sferik sferik closed this in dc4a827 Feb 13, 2013
@sferik sferik added a commit that referenced this issue Feb 13, 2013
@sferik 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

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.

@sferik

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

@aitor aitor added a commit to aitor/omniauth that referenced this issue Feb 13, 2013
@aitor 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