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

Conversation

songgao
Copy link
Contributor

@songgao songgao commented Jan 15, 2018

No description provided.

@songgao songgao requested review from jzila and strib January 15, 2018 19:14
// 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

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

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.

// '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.

Copy link
Contributor

@jzila jzila left a comment

Choose a reason for hiding this comment

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

LGTM. Do we also want to set CORS headers to block cross-origin resource retrieval?

// 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"?

@songgao
Copy link
Contributor Author

songgao commented Jan 15, 2018

@jzila Is there a specific CORS header you are thinking of? If I understand correctly, they enable CORS in specific scenarios rather than disabling it. Browsers already block cross-origin by default.

@songgao songgao merged commit c4038fc into master Jan 16, 2018
@songgao songgao deleted the songgao/http-response-headers branch January 16, 2018 01:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants