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

Return 307 status instead of 301 when rerouting POST requests to SSL #23941

Merged
merged 1 commit into from Aug 22, 2016

Conversation

Projects
None yet
7 participants
@chiragsinghal
Contributor

chiragsinghal commented Feb 28, 2016

When config.force_ssl is set to true, any POST/PUT/DELETE requests coming in to non-secure url are being redirected with a 301 status.
However, when that happens, the request is converted to a GET request and ends up hitting a different action on the controller.

Since we can not do non-GET redirects, we can instead redirect with a 307 status code instead to indicate to the caller that a fresh request should be tried preserving the original request method.

rack-ssl gem which was used to achieve this before we had this middleware directly baked into Rails also used to do the same, ref: https://github.com/josh/rack-ssl/blob/master/lib/rack/ssl.rb#L54

This would be specially important for any apps switching from older version of Rails or apps which expose an API through Rails.

@rails-bot

This comment has been minimized.

rails-bot commented Feb 28, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@chiragsinghal

This comment has been minimized.

Contributor

chiragsinghal commented Feb 28, 2016

cc: @josh since he was maintaining the rack-ssl gem

@chiragsinghal

This comment has been minimized.

Contributor

chiragsinghal commented Feb 28, 2016

Couple of questions if this change is acceptable:

  1. Should similar change be made to https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/force_ssl.rb#L82
  2. Should we use 308 status code instead of 307 here, as 308 pertains to permanent redirect?
@chiragsinghal

This comment has been minimized.

Contributor

chiragsinghal commented Mar 20, 2016

r? @josh

@rails-bot rails-bot assigned josh and unassigned rafaelfranca Mar 20, 2016

@matthewd matthewd assigned rafaelfranca and unassigned josh Mar 20, 2016

@jeremy

This comment has been minimized.

Member

jeremy commented Apr 19, 2016

308 status is still a proposed spec. Browsers that don't support it will either break or treat it as a 300 response, which doesn't redirect. We can't use it widely yet.

Not sure about 307 availability. What would you tell Rails users about 307 browser support and upgrade risks?

@chiragsinghal

This comment has been minimized.

Contributor

chiragsinghal commented Apr 19, 2016

308 status - Yes, it's fairly new (was defined in 2012) so we may not have wide support yet. Current versions of Firefox and Chrome support it, IE11 doesn't, didn't test other browsers.

307 status has been around since 1999, so we should have good browser support. Couldn't find much info on support except this (scroll to bottom to see that most common browsers supported it as tested in 2011)

This change will most likely have very limited exposure, because 307 pertains to only non-(GET or HEAD) requests, so it has much lesser potential of affecting regular browser based clients.
As, generally the Rails apps won't allow non-GET requests from outside of the app. If the request is submitted from within the app, then the user would have to load up a GET resource which would auto-redirect to secure version of that resource and hence would PUT/POST also to secure version from then on.

On my app, the problem was detected for API clients. After setting config.force_ssl = true all browser clients were correctly redirecting all traffic to https, but the POST requests from API consumers started failing, so a http POST to create action would return 301 which would cause the API client to resend the request as a https GET which will then get routed to index and fail.

@jeremy

This comment has been minimized.

Member

jeremy commented Apr 20, 2016

Sounds good. We'll need a changelog update to share that POST/PUT/etc to http:// will redirect to https:// with a 307 instead of a 301 and that most/all user agents support that. May need to update our security guide that discusses force-ssl also.

@chiragsinghal chiragsinghal force-pushed the chiragsinghal:patch-1 branch Apr 21, 2016

@chiragsinghal

This comment has been minimized.

Contributor

chiragsinghal commented Apr 21, 2016

@jeremy Added a changelog entry and a test for this change. Both are added as separate commits for ease of review. I can squash them into one commit if everything looks good.

For the security guide, the only mention of force_ssl it has is for session hijacking which may not be relevant section to mention about this change. Also looked at other references to this setting across the guides but didn't find any place where this could be worth highlighting.

There is another similar redirection logic here (https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/force_ssl.rb#L82) which can be used to selectively force ssl for specific controllers/actions. Should we make changes there as well to use 307 for non-GET requests?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 21, 2016

r? @jeremy

@rails-bot rails-bot assigned jeremy and unassigned rafaelfranca Apr 21, 2016

@chiragsinghal chiragsinghal force-pushed the chiragsinghal:patch-1 branch Aug 3, 2016

@chiragsinghal

This comment has been minimized.

Contributor

chiragsinghal commented Aug 3, 2016

@jeremy pinging back to see if you were able to review the changelog entry and specs.

@kaspth

View changes

actionpack/lib/action_dispatch/middleware/ssl.rb Outdated
@@ -133,7 +133,8 @@ def flag_cookies_as_secure!(headers)
end
def redirect_to_https(request)
[ @redirect.fetch(:status, 301),
status = %w[GET HEAD].include?(request.request_method) ? 301 : 307

This comment has been minimized.

@kaspth

kaspth Aug 7, 2016

Member

Let's pull this into a method:

def redirect_to_https(request)
  [ @redirect.fetch(:status, redirection_status(request)),
  # ...
end

def redirection_status(request)
  if request.get? || request.head?
    301 # Issue a permanent redirect via a GET request.
  else
    307 # Issue a fresh request redirect to preserve the HTTP method.
  end
end
@kaspth

View changes

actionpack/test/dispatch/ssl_test.rb Outdated
}
post from
assert_response redirect[:status] || 307

This comment has been minimized.

@kaspth

kaspth Aug 7, 2016

Member

Let's pull this up into the test. See below.

@kaspth

View changes

actionpack/test/dispatch/ssl_test.rb Outdated
from: 'http://a/b?c=d', to: from.sub('http', 'https'))
self.app = build_app ssl_options: { redirect: redirect,
host: deprecated_host, port: deprecated_port

This comment has been minimized.

@kaspth

kaspth Aug 7, 2016

Member

Let's just remove the deprecated stuff from this method altogether, they aren't long for this world.

@kaspth

View changes

actionpack/test/dispatch/ssl_test.rb Outdated
@@ -58,6 +70,10 @@ def assert_redirected(redirect: {}, deprecated_host: nil, deprecated_port: nil,
assert_redirected
end
test 'http POST is redirected to https with status 307' do
assert_post_redirected

This comment has been minimized.

@kaspth

kaspth Aug 7, 2016

Member
assert_post_redirected redirect: { status: 307 }

This comment has been minimized.

@chiragsinghal

chiragsinghal Aug 19, 2016

Contributor

@kaspth if we explicitly pass the redirect status here, will it not render the test useless as the app will always assign this status code. We want to test that when no status is explicitly specified, it responds with 307 status code for non-GET or non-HEAD requests. Or am I missing something here?

This comment has been minimized.

@kaspth

kaspth Aug 21, 2016

Member

Ah right, I see what you mean 👍

@kaspth

This comment has been minimized.

Member

kaspth commented Aug 7, 2016

You will need to rebase on current master and squash your commits too 👍

@kaspth kaspth self-assigned this Aug 7, 2016

@kaspth kaspth added this to the 5.0.1 milestone Aug 7, 2016

@chiragsinghal chiragsinghal force-pushed the chiragsinghal:patch-1 branch Aug 19, 2016

@chiragsinghal

This comment has been minimized.

Contributor

chiragsinghal commented Aug 19, 2016

@kaspth sorry for the delayed response, I was on vacation and didn't have access to laptop. Rebase and squash is done. I haven't squashed the latest commit to make it easier to review. Let me know when you have had a chance to look at it and will squash that one as well.

@kaspth

This comment has been minimized.

Member

kaspth commented Aug 21, 2016

@chiragsinghal hey thanks for the update! It's fine to just squash the commits completely when the code change is this tiny. Can you squash again? Then I'll merge 😄

Return 307 status instead of 301 when rerouting POST requests to SSL
When `config.force_ssl` is set to `true`, any POST/PUT/DELETE requests coming in to non-secure url are being redirected with a 301 status.
However, when that happens, the request is converted to a GET request and ends up hitting a different action on the controller.

Since we can not do non-GET redirects, we can instead redirect with a 307 status code instead to indicate to the caller that a fresh request should be tried preserving the original request method.

`rack-ssl` gem which was used to achieve this before we had this middleware directly baked into Rails also used to do the same, ref: https://github.com/josh/rack-ssl/blob/master/lib/rack/ssl.rb#L54

This would be specially important for any apps switching from older version of Rails or apps which expose an API through Rails.

@chiragsinghal chiragsinghal force-pushed the chiragsinghal:patch-1 branch to 64f9802 Aug 22, 2016

@chiragsinghal

This comment has been minimized.

Contributor

chiragsinghal commented Aug 22, 2016

@kaspth done :)

@kaspth kaspth merged commit 46a4424 into rails:master Aug 22, 2016

2 checks passed

codeclimate Code Climate has skipped analysis of this commit.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

kaspth added a commit that referenced this pull request Aug 22, 2016

Merge pull request #23941 from chiragsinghal/patch-1
Return 307 status instead of 301 when rerouting POST requests to SSL
@kaspth

This comment has been minimized.

Member

kaspth commented Aug 22, 2016

Backported to 5-0-stable: 2f594ba

Thanks for the pull request!

@chiragsinghal chiragsinghal deleted the chiragsinghal:patch-1 branch Aug 22, 2016

@chiragsinghal

This comment has been minimized.

Contributor

chiragsinghal commented Aug 22, 2016

@kaspth thanks for review and feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment