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

Introduce default_headers. closes #6311 #6515 #7302

Merged
merged 2 commits into from Aug 9, 2012
Merged

Conversation

@homakov
Copy link
Contributor

homakov commented Aug 9, 2012

4.0 feature, everybody agreed to merge it.
Default headers are really useful for various security options and mitigations.

discuss: should we add "Sniff content type" header

@rafaelfranca
Copy link
Member

rafaelfranca commented Aug 9, 2012

Related with #6311 and #6515 (commenting since github doesn't link issues at the title)

@rafaelfranca
Copy link
Member

rafaelfranca commented Aug 9, 2012

@homakov Thank you. Could you add some tests?

@homakov
Copy link
Contributor Author

homakov commented Aug 9, 2012

@rafaelfranca thanks, here they are

@rafaelfranca
Copy link
Member

rafaelfranca commented Aug 9, 2012

@steveklabnik
Copy link
Member

steveklabnik commented Aug 9, 2012

So, it's now considered not best practice to use the X-* pattern when minting new HTTP headers: http://tools.ietf.org/html/rfc6648

@homakov
Copy link
Contributor Author

homakov commented Aug 9, 2012

@steveklabnik yea have seen it and lold :) this 'over standardizing' looks funny.

You sure all browsers support Frame-Options along X-Frame-Options? if yes I will change it.

@homakov
Copy link
Contributor Author

homakov commented Aug 9, 2012

@steveklabnik
Copy link
Member

steveklabnik commented Aug 9, 2012

Is 'X-Frame-Options' an existing header, or a new one? I'm not super familiar with security issues.

If it's old, then obviously, we use it verbatim. If 'browser support' means do browsers can handle a new header without the X- prefix, then yes, as far as I know, they all do.

@homakov
Copy link
Contributor Author

homakov commented Aug 9, 2012

@steveklabnik yes, both X- headers are extremely old so we hardly can remove 'X-' prefix.
X-XSS-Protection controls built in IE and Chrome anti xss auditor.
X-Frame-Options detects should website be visible in iframes or not. 99% of websites don't need to be shown in iframes.

@steveklabnik
Copy link
Member

steveklabnik commented Aug 9, 2012

Ah! Then continue. I thought they were something new you were making. No worries. TIL.

@et
Copy link

et commented Aug 9, 2012

+1 These are great defaults that everyone will probably want turned on.

@aantix
Copy link
Contributor

aantix commented Aug 9, 2012

+1 We're currently manually adding these defaults for the current client I am working with. It's coupled with complimentary frame busting code, but that's probably a separate story.

@homakov
Copy link
Contributor Author

homakov commented Aug 9, 2012

@aantix regards framebusting - make sure you use "modern framekiller" when page is hidden by default and then shown if everything is ok. other framekillers don't work.

c'mon, merge or die trying

@tenderlove
Copy link
Member

tenderlove commented Aug 9, 2012

Seems good. Thanks @homakov!

tenderlove added a commit that referenced this pull request Aug 9, 2012
Introduce default_headers. closes #6311 #6515
@tenderlove tenderlove merged commit 6794e92 into rails:master Aug 9, 2012
@drogus

This comment has been minimized.

Copy link
Member

drogus commented on 98c18d0 Aug 9, 2012

Can we have better commit messages next time? As showed here: http://guides.rubyonrails.org/contributing_to_ruby_on_rails.html#commit-your-changes

@homakov
Copy link
Contributor Author

homakov commented Aug 9, 2012

@drogus my bad, forgot that commit message is what i write in -m and not in title of PR.

@tenderlove Spasibo :)

@@ -41,6 +41,11 @@ class Application < Rails::Application
# Configure sensitive parameters which will be filtered from the log file.
config.filter_parameters += [:password]

config.action_dispatch.default_headers = {

This comment has been minimized.

Copy link
@josevalim

josevalim Aug 9, 2012

Contributor

I don't think these configs should be in config/application.rb, but rather in the action dispatch railtie.

This comment has been minimized.

Copy link
@homakov

homakov Aug 10, 2012

Author Contributor

IMO this is a perfect place. Developer sees clearly where and how default headers are defined. Also, how is he supposed to remove them if he doesn't need Frame-Options? We cannot "hardcode" these values.

This comment has been minimized.

Copy link
@josevalim

josevalim Aug 10, 2012

Contributor

config.action_dispatch.default_headers.clear would clear them. Rails has many defaults, we don't list them all on config/application.rb for a reason. Documentation solves the problem of knowing what is on by default and how to clear it.

@spastorino
Copy link
Member

spastorino commented Aug 9, 2012

Can we have a CHANGELOG entry for this?

@homakov
Copy link
Contributor Author

homakov commented Aug 10, 2012

@spastorino do we still add changelog changes in PRs? :) You can add it if dont mind

@spastorino
Copy link
Member

spastorino commented Aug 10, 2012

@homakov we shouldn't be merging PRs without CHANGELOG entries or we should merge and add CHANGELOG entries later but I prefer asking contributors for that. I have merged things without CHANGELOG entries but I don't consider that a good practice anyway. I'm guilty too ;).

@spastorino
Copy link
Member

spastorino commented Aug 10, 2012

pushed 0b11dbe

@homakov
Copy link
Contributor Author

homakov commented Aug 10, 2012

@spastorino thannnkss :)

@josevalim i see your point but cannot do so. Explicit definition here looks like common sense to me.

for example we have config.filter_parameters += [:password] but we also could have it built in and ask developers to write config.filter_parameters -= [:password] if they don't want to filter passwords. It is a bit 'monkey patching'. Please, let's leave it as is, I think it's a better option.

@drogus
Copy link
Member

drogus commented Aug 11, 2012

To confirm how should we deal with CHANGELOG entries, I've pushed a commit describing it to contributing guide: adf3ea3 I'm serial offender, too, and I probably haven't added CHANGELOG entries to a lot of things myself, but just as with commit messages, I would like to hold any PR that needs fixing this - it may make merging harder and this green button is really tempting, but as we have more commiters and more commits in general, it will be much easier to deal with it this way.

@spastorino
Copy link
Member

spastorino commented Aug 18, 2012

Is there a reason for not adding also "X-Content-Type-Options" => "nosniff" ?.
@homakov what about the other headers you proposed in the first place?

@homakov
Copy link
Contributor Author

homakov commented Aug 18, 2012

@spastorino we should add it too. Yet another header to make IE less mad. Go ahead, it can be default :)

Also, if you will write a tutorial, you can find default_headers feature useful for Access-Control-Allow-Origin and X-Content-Security-Policy which are popular headers(http://recxltd.blogspot.com/2012/03/seven-web-server-http-headers-that.html)

@josevalim now I can see values hardcoded inside of ActionDispatcher. It's OK, but i'm afraid that people will get broken framing by updating rails to 4 ver. They will be surprised :(

@aantix
Copy link
Contributor

aantix commented Aug 18, 2012

@homakov @spastorino @josevalim Just submitted a pull request to add the X-Content-Type-Options header.

#7390

@spastorino
Copy link
Member

spastorino commented Aug 21, 2012

@aantix
Copy link
Contributor

aantix commented Aug 21, 2012

I don't think any of the other headers, (Cache-Control, X-Content-Security-Policy, Strict-Transport-Security, Access-Control-Allow-Origin) should have defaults.

@homakov
Copy link
Contributor Author

homakov commented Aug 21, 2012

@spastorino they should be per-app configured.. up to developers.

@aantix
Copy link
Contributor

aantix commented Aug 21, 2012

My comment above agrees with @homakov

@spastorino
Copy link
Member

spastorino commented Aug 21, 2012

Yeah, so could be nice to document about them and that as a developer you could change default_headers if you want to.

@homakov
Copy link
Contributor Author

homakov commented Aug 21, 2012

@spastorino I will manage it. As those are mostly security-related headers I want to update this document http://guides.rubyonrails.org/security.html

@spastorino
Copy link
Member

spastorino commented Aug 21, 2012

@homakov sure, go ahead

@aantix
Copy link
Contributor

aantix commented Aug 21, 2012

@homakov Let me know if you want any help.

@homakov
Copy link
Contributor Author

homakov commented Aug 27, 2012

@spastorino @aantix guys pls have a look at https://github.com/lifo/docrails/pull/108/files
I described the most common headers. if you wish to add some more headers - welcome. (and, yeah, welcome to fix my grammar :) )

@spastorino
Copy link
Member

spastorino commented Aug 27, 2012

@homakov 👍 you're free to push that to docrails

@homakov
Copy link
Contributor Author

homakov commented Aug 27, 2012

@spastorino yes i pushed :) Just want to see it "here":http://guides.rubyonrails.org/security.html

@steveklabnik
Copy link
Member

steveklabnik commented Aug 27, 2012

Well it won't be out on that page until a release. And since this went into master, that'd be the release of Rails 4.

@frodsan
Copy link
Contributor

frodsan commented Aug 27, 2012

Also, you will see it "here":http://edgeguides.rubyonrails.org/security.html when docrails changes get merged in master.

@homakov
Copy link
Contributor Author

homakov commented Aug 27, 2012

@steveklabnik @frodsan got it, no rush.
I think mass assignment section also will need to get updated when we get strong_parameters merged.

@aantix
Copy link
Contributor

aantix commented Aug 28, 2012

@spastorino @homakov I added some clarifications to the security documentation surrounding what the default values are and how to clear them.

https://github.com/lifo/docrails/pull/109

@spastorino
Copy link
Member

spastorino commented Aug 29, 2012

@aantix as @fxn told you, please push the changes directly to docrails, everyone has commit bit there :)

@aantix
Copy link
Contributor

aantix commented Aug 29, 2012

@spastorino Merged in my changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.