Skip to content
This repository has been archived by the owner on Feb 12, 2019. It is now read-only.

add a couple HTTP response headers #1450

Merged
merged 2 commits into from
Jan 16, 2018
Merged

add a couple HTTP response headers #1450

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2955028
add a couple HTTP response headers
songgao Jan 15, 2018
fee3a96
typo fix as suggested by strib and remove nosniff
songgao Jan 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions libpages/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,27 @@ func (s *Server) logRequest(sri *ServedRequestInfo, requestPath string) {
)
}

func (s *Server) setCommonResponseHeaders(w http.ResponseWriter) {
// Since http.FileServer already sets MIME type properly, disable MIME type
// sniffing on browser-side. This would prevent e.g. an attack where a
// malicious html file with .jpg suffix being executed by browser without
// site visitors' awareness. References:
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Content-Type-Options
// https://helmetjs.github.io/docs/dont-sniff-mimetype/
w.Header().Set("X-Content-Type-Options", "nosniff")
Copy link
Contributor Author

@songgao songgao Jan 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually leaning towards not having this header, for three reasons:

  1. Keybase Pages doesn't have visitor-uploaded content; all content is provided by the user who hosts with us. This means an evil user Evan has to already be hosting with us in order to launch an attack using this, on his own site. For example, Evan hosts an html under jpg, and sends a link to Alice, in which case we are hosting hostile content from Evan to Alice, when Evan controls the domain. This is pretty rare, and arguably as a hosting provider, we shouldn't be worried about this kind of attack.

  2. This attack in example didn't work in my Chrome without the to-be-added nosniff header, so modern browsers might have already been doing the right thing.

  3. As mentioned on MDN A naive implementation of nosniff may break image rendering, and it was fixed in standard only recently, so enabling this header globally for all returned content may still break some use cases.

That being said, keybase.pub has this header returned for both webpages and images. So removing this would be a change.

cc @malgorithms

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, your call on this since I'm not sure.

// Enforce XSS protection. References:
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection
// https://blog.innerht.ml/the-misunderstood-x-xss-protection/
w.Header().Set("X-XSS-Protection", "1; mode=block")
// Only allow HTTPS on this domain, and make this policy it expire in a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"policy it" -> "policy"?

// week. This means if user decides to migrate off Keybase Pages, there's a
// 1-week gap before they can use HTTP again. Note that we don't use the
// 'preload' directive, for the same reason we use 302 instead of 301 for
// HTTP->HTTPS redirection. Reference: https://hstspreload.org/#opt-in
w.Header().Set("Strict-Transport-Security", "max-age=604800; includeSubDomains")
// TODO: allow user to opt-in some directives of Content-Security-Policy?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSP is present on a neocities site as following:

content-security-policy: upgrade-insecure-requests; default-src 'unsafe-inline' 'unsafe-eval' 'self' data: blob: *

But it seems these all should be opt-in by user, perhaps through the .kbp_config, thus the TODO. But let me know if anybody feels differently.

}

// ServeHTTP implements the http.Handler interface.
func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
sri := &ServedRequestInfo{
Expand All @@ -301,12 +322,13 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}
defer s.logRequest(sri, r.URL.Path)

s.setCommonResponseHeaders(w)

// Don't serve the config file itself.
if path.Clean(strings.ToLower(r.URL.Path)) == config.DefaultConfigFilepath {
// TODO: integrate this check into Config?
w.WriteHeader(http.StatusForbidden)
fmt.Fprintf(w, "Reading %s directly is forbidden.",
config.DefaultConfigFilepath)
http.Error(w, fmt.Sprintf("Reading %s directly is forbidden.",
config.DefaultConfigFilepath), http.StatusForbidden)
return
}

Expand Down Expand Up @@ -340,6 +362,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
sri.CloningShown = true
// TODO: replace this with something nicer when fancy error pages and
// landing pages are ready.
w.Header().Set("Content-Type", "text/html; charset=utf-8")
w.WriteHeader(http.StatusServiceUnavailable)
w.Write(cloningLandingPage)
return
Expand Down