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

Resolve issue #41 #42

Closed
wants to merge 2 commits into from
Closed

Resolve issue #41 #42

wants to merge 2 commits into from

Conversation

iwan
Copy link

@iwan iwan commented Mar 13, 2012

Hi,
here there are my changes to force the gem to generate a service url with a https scheme.

You will obtain
https://localhost/cas/login?service=https%3A%2F%2Flocalhost%2Fusers%2Fservice
in place of
https://localhost/cas/login?service=http%3A%2F%2Flocalhost%2Fusers%2Fservice

when set
Devise.setup do |config|
config.cas_force_ssl_service = true
end

@nbudin
Copy link
Owner

nbudin commented Mar 13, 2012

I'd prefer not to do this with yet another config parameter if there is a cleaner way. As I suggested in the comments to issue #41, there may be a way to use the HTTP request headers (e.g. the X-Forwarded-For header) to see what's being requested to the proxy.

Have you investigated that possibility?

@nbudin
Copy link
Owner

nbudin commented Mar 16, 2012

Also, looking at this pull request again, this potentially breaks servers that are actually serving HTTPS. What happens in the case where base_url is https://something, but the user has not set config.cas_force_ssl_service? Without this patch, their service URL will correctly contain https, but with this patch, they must set config.cas_force_ssl_service. This means that existing sites could break if this patch is merged.

As I said above, I'd prefer to do this without another config parameter if that is at all possible, but if it turns out we need to add an extra config parameter for this, I would definitely want to correct that bug before accepting this pull request.

@nbudin nbudin deleted the branch nbudin:master January 3, 2022 16:33
@nbudin nbudin closed this Jan 3, 2022
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.

2 participants