-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add headers that provide extra security protection in browsers #12521
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some indentation problems (tabs vs spaces maybe?), but changes LGTM
@@ -52,6 +52,15 @@ func GzipHandler(h http.Handler) http.Handler { | |||
}) | |||
} | |||
|
|||
func BrowserSecurityHandler(h http.Handler) http.Handler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a small preference for SecurityHeadersHandler
, which makes it more obvious what the handler is adding
yeah I need to run gofmt, forgot about that |
updated the handler name and ran gofmt, just need to rebuild to double check everything is still working |
ok verified this builds and is working, @liggitt can you take a quick glance and then i think it is good to merge |
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.Header().Add("X-Content-Type-Options", "nosniff") | ||
w.Header().Add("X-XSS-Protection", "1; mode=block") | ||
w.Header().Add("X-Frame-Options", "DENY") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add
or Set
?
// http://www.iana.org/assignments/media-types/application/font-sfnt | ||
registerIfNeeded(".ttf", "application/font-sfnt") | ||
registerIfNeeded(".otf", "application/font-sfnt") | ||
|
||
// Flash | ||
registerIfNeeded(".swf", "application/x-shockwave-flash") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave this just in case an extension was including a swf
a couple nits, LGTM |
i will switch these to Set since this is new code, and I highly doubt
anything else in our handler chain would be setting these, but we should
think about whether our other header Adds should be changed as a separate
issue
…On Tue, Jan 17, 2017 at 5:08 PM, Jordan Liggitt ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/assets/handlers.go
<#12521 (review)>
:
> @@ -52,6 +52,15 @@ func GzipHandler(h http.Handler) http.Handler {
})
}
+func SecurityHeadersHandler(h http.Handler) http.Handler {
+ return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.Header().Add("X-Content-Type-Options", "nosniff")
+ w.Header().Add("X-XSS-Protection", "1; mode=block")
+ w.Header().Add("X-Frame-Options", "DENY")
Add or Set?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#12521 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABZk7c6OZ5ZWQqwjmV8Lvyzmw77gy9ZNks5rTTvBgaJpZM4LlxfL>
.
|
6124c0a
to
08615c7
Compare
updated, but i need to rebuild/retest in the morning |
ok all comments have been addressed [merge] |
fixed the extension unit test that was expecting text/javascript [merge] |
Evaluated for origin merge up to 4222a4d |
[Test]ing while waiting on the merge queue |
Evaluated for origin test up to 4222a4d |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13004/) (Base Commit: 5d6f8b4) |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13014/) (Base Commit: ba792d2) (Image: devenv-rhel7_5719) |
No description provided.