Skip to content
This repository has been archived by the owner on Aug 3, 2020. It is now read-only.
This repository has been archived by the owner on Aug 3, 2020. It is now read-only.

Always set the header, even in HTTP mode #16

Closed
EvanHahn opened this issue Nov 12, 2016 · 5 comments
Closed

Always set the header, even in HTTP mode #16

EvanHahn opened this issue Nov 12, 2016 · 5 comments

Comments

@EvanHahn
Copy link
Member

To quote the spec:

If an HTTP response is received over insecure transport, the MUST ignore any present STS header field(s).

It seems that we can ignore req.secure and always set the header, even in HTTP mode.

This is arguably a breaking change, but I don't think it is--it doesn't matter what we do when in HTTP mode.

@mathiasose
Copy link

I used https://www.hardenize.com to test a service that uses this package, and for what it's worth, Hardenize thinks this is wrong.

This is the warning that is given:

HSTS policies must not be transmitted over insecure channels.

I don't know if this warrants any change to this package, just thought I'd share. If I really really want the Hardenize warning to go away for my domain I can just use setIf.

@EvanHahn
Copy link
Member Author

Hardenize's behavior seems to contradict the spec. That doesn't necessarily mean they're wrong—specs don't always reflect reality—but it certainly suggests it.

I can reach out to Hardenize about this issue. Would you like me to do that?

In the meantime, as you say, you can quiet the warning with setIf.

@mathiasose
Copy link

Reaching out might be a good idea. I can't find very many arguments about this issue when I search the web, but maybe the people behind Hardenize can explain their reasoning.

@EvanHahn
Copy link
Member Author

EvanHahn commented Jan 31, 2019 via email

@EvanHahn
Copy link
Member Author

EvanHahn commented Feb 5, 2019

In short, you should use setIf.

Hardenize replied (quickly, I might add—the delay here is all mine). They quoted this part of the spec:

An HSTS Host MUST NOT include the STS header field in HTTP responses conveyed over non-secure transport.

Unfortunately, it's impossible for Helmet to reliably determine whether the incoming request is over HTTPS because it might be behind a reverse proxy or similar. That means that this middleware has three options:

  1. Set the header unconditionally, but give the developer the option to opt out.
  2. Only set the header when the developer explicitly says so.
  3. Do a best-effort guess, but give the developer the option to control it.

Because the header is ignored over insecure HTTP, I opted with choice 1—better to set it too often than not enough. If you want to do things per spec (and satisfy Hardenize), you should use setIf to only set the header over HTTPS.

I'll plan to update the documentation to encourage this, but none of this middleware's internal code will change.

The folks at Hardenize also mentioned some improvements around these warnings, so stay tuned for that on their front.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

2 participants