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

Include default headers by default in API mode #32484

Merged
merged 1 commit into from Apr 10, 2018

Conversation

Projects
None yet
5 participants
@kddeisz
Contributor

kddeisz commented Apr 6, 2018

ActionDispatch's default headers are now moved into their own module that are by default included in both Base and API. This allows API-mode applications to take advantage of the default security headers, as well as providing an easy way to add more.

Fixes #32483.

Include default headers by default in API mode
ActionDispatch's default headers are now moved into their own module that are by default included in both Base and API. This allows API-mode applications to take advantage of the default security headers, as well as providing an easy way to add more.
@rails-bot

This comment has been minimized.

rails-bot commented Apr 6, 2018

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.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@kddeisz

This comment has been minimized.

Contributor

kddeisz commented Apr 6, 2018

@rafaelfranca I realized I didn't actually need to put it into metal to get it into both API and Base. If it's too much to push it into API I can strip line out, but it seems like it might be a worthwhile change for 6.

@guilleiguaran

This comment has been minimized.

Member

guilleiguaran commented Apr 6, 2018

I'm not sure we want to have the default headers in API mode since most of them are present to be interpreted by browsers, not by API clients

Per #32483, I thought your idea was moving this to a module that can be optionally included in ApplicationController in API applications, not making it a default

@kddeisz

This comment has been minimized.

Contributor

kddeisz commented Apr 6, 2018

@guilleiguaran it was both - as mentioned above I can definitely move it out, but it seemed like a good feature to have. APIs are still vulnerable to these things even if they're only meant to be consumed by API clients, seems like a push toward secure-by-default.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 6, 2018

Yes, it make sense to me to be default in all controllers. If an API application don't want one of the headers they can just remove it from the defaults.

@guilleiguaran

This comment has been minimized.

Member

guilleiguaran commented Apr 6, 2018

I don't see any benefit in making it a default when most of the security headers we added are specific to give advice/instructions to browsers about how to act in certain circumstances. This is a summary of the headers that we include by default:

"X-Frame-Options" => can be used to indicate whether or not a browser should be allowed to render a page in a , <iframe> or . Sites can use this to avoid clickjacking attacks, by ensuring that their content is not embedded into other sites

"X-XSS-Protection" => instructs the XSS Auditor from browsers to not render the site when it detects a reflected XSS attempt.

"X-Content-Type-Options" => prevents browsers from making assumptions about the content type if the site didn't declare the type correctly.

"X-Download-Options" => Prevent auto-opening of files, this only for IE8

"X-Permitted-Cross-Domain-Policies" => This one is specific for Adobe Flash Player/Acrobat

"Referrer-Policy" => This is for browser Referrer header

There are any cases where you want to use those headers in the context of JSON/XML APIs?

@kddeisz

This comment has been minimized.

Contributor

kddeisz commented Apr 6, 2018

We don't know what kind of content people are serving from their APIs. Any one of those headers could apply. Just because one usage of api_only is JSON/XML doesn't mean other content types couldn't be served.

For reference, here's all the stuff github's API responds with:

$ curl -I https://api.github.com
HTTP/1.1 200 OK
Server: GitHub.com
Date: Fri, 06 Apr 2018 20:58:49 GMT
Content-Type: application/json; charset=utf-8
Content-Length: 2165
Status: 200 OK
X-RateLimit-Limit: 60
X-RateLimit-Remaining: 4
X-RateLimit-Reset: 1523048580
Cache-Control: public, max-age=60, s-maxage=60
Vary: Accept
ETag: "7dc470913f1fe9bb6c7355b50a0737bc"
X-GitHub-Media-Type: github.v3; format=json
Access-Control-Expose-Headers: ETag, Link, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval
Access-Control-Allow-Origin: *
Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
X-Frame-Options: deny
X-Content-Type-Options: nosniff
X-XSS-Protection: 1; mode=block
Referrer-Policy: origin-when-cross-origin, strict-origin-when-cross-origin
Content-Security-Policy: default-src 'none'
X-Runtime-rack: 0.008902
X-GitHub-Request-Id: 9D0C:7512:19CFC7:33B9DD:5AC7DF89

I really don't see why you wouldn't want this. Is there some kind of downside that I'm not seeing?

Regardless, the feature I actually wanted was for default_headers to even be applied to API controllers, because it was confusing to set them and then not see them applied. If this is an issue, I can just strip down the default headers but still allow them to be configured.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 6, 2018

I personally don't see any downside. And if people don't want it they can still configure the default headers to be what they want.

@kddeisz

This comment has been minimized.

Contributor

kddeisz commented Apr 7, 2018

Not sure why those tests failed but they don't seem to be related.

@rafaelfranca rafaelfranca merged commit 6e4e692 into rails:master Apr 10, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@oyeanuj

This comment has been minimized.

oyeanuj commented Apr 20, 2018

And if people don't want it they can still configure the default headers to be what they want.

@rafaelfranca Any docs (or list of headers) on this so one can configure these?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 20, 2018

Search for default_headers in http://guides.rubyonrails.org/configuring.html

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