Disable HSTS headers by default on localhost

[wesleytodd]

I searched around a bit but didn't see any previous discussion of this. Has it been discussed to check if the request is on localhost and avoid setting that by default? It is regularly problematic when that header gets set on a local dev setup, so like a good default.

[EvanHahn]

Interesting idea.

I believe HSTS is ignored over insecure HTTP, but maybe not if you have a local HTTPS setup.

Is there a way to reliably detect that a request is being made to localhost?

[wesleytodd]

I would have to dig a bit as I have not gotten up to date on this lately, but we were discussing this in a work meeting and lots of folks have hit it so I know that even if it is ignored over plain http there are setups where some projects are setting it and effecting others. Our theory was that inclusion of helmet in some projects was causing other projects which are intended to only be used locally over http are getting redirected.

is there a way to reliably detect that a request is being made to localhost?

We should be able to use the host header for this. There are cases where that might not cover it but since it is tied to the domain and really the goal is to prevent accidental use of it in local dev I think it is alright to do something naive to start.

[EvanHahn]

In the short term, you might be able to solve your problem like this:

app.use(
  helmet({
    strictTransportSecurity: app.get("env") === "development",
  }),
);

However, you're looking for a long term solution.

I don't think we can use the Host header trick because it can be spoofed. If we had logic like this pseudocode...

if req.headers["Host"] != "localhost":
  includeHstsMiddleware()

...then someone could disable the HSTS header by setting the Host header to localhost. This might work fine for most users but I worry that someone's deployment will get messed up with this heuristic.

Maybe there's another trick we can use for this?

[wesleytodd]

In the short term, you might be able to solve your problem like this:

I don't have a specific problem. I am on a platform team and was investigating why folks keep on complaining about this issue. We searched all our repos and found many projects using helmet with the default config so we made some assumptions that in local dev that was causing this across the whole company. So yes I was hoping this would result in a long term solution.

don't think we can use the Host header trick because it can be spoofed. If we had logic like this pseudocode...

Valid issue and exactly why I opened this issue to make sure this discussion had happened since I couldn't find it in the issues history. I don't believe there is a way to address this poor UX with the default's set by this package in a way doesn't relax the security posture or require the user to configure the package.

Is asking users (required or warning) to configure the package like you do in your example something you would consider? Since most folks just do .use(helmet()) right now AFAICT, that would be a clear change in direction I thought it might be against your ideal UX.

If you are alright with that it seems like a change which would improve the UX, and if not maybe we could just add some documentation on this specific thing which everyone can point to when folks come complaining about this behavior?

[EvanHahn]

I am on a platform team and was investigating why folks keep on complaining about this issue. We searched all our repos and found many projects using helmet with the default config so we made some assumptions that in local dev that was causing this across the whole company.

No one else has reported anything like this. It's possible that the culprit lies elsewhere, or it's possible that nobody has used Helmet quite like this. (If I've learned anything maintaining Helmet, it's that there are a lot of really different project setups out there.)

How sure are you that this is a Helmet HSTS problem?

I don't believe there is a way to address this poor UX with the default's set by this package in a way doesn't relax the security posture or require the user to configure the package.

Is asking users (required or warning) to configure the package like you do in your example something you would consider? Since most folks just do .use(helmet()) right now AFAICT, that would be a clear change in direction I thought it might be against your ideal UX.

If you are alright with that it seems like a change which would improve the UX, and if not maybe we could just add some documentation on this specific thing which everyone can point to when folks come complaining about this behavior?

I think you understand the tension well. Unless there's a way to reliably detect this in code (which I suspect there isn't), documentation seems like the ideal solution.

However, I don't want to document this without being sure that it's really a problem with Helmet. Do you have a reliable way to reproduce this issue?

[wesleytodd]

No one else has reported anything like this. It's possible that the culprit lies elsewhere
How sure are you that this is a Helmet HSTS problem?

We made some assumptions, so not sure in the least. That said I am pretty sure I could put together a reduced test case if that would help.

If I've learned anything maintaining Helmet, it's that there are a lot of really different project setups out there.

Haha, yeah one of the best things about helping maintain popular OSS or doing support at a big org is learning all these wild things people do. Between express and supporting node.js at netflix I have seen some things. One other thing I have learned though is that things like this behavior also go under reported because tracking down the source can be difficult and not rewarding for most. Which I wouldn't be surprised to hear is contributing to why you haven't gotten reports for it, especially since it is not a "bug".

I think you understand the tension well. Unless there's a way to reliably detect this in code (which I suspect there isn't), documentation seems like the ideal solution.

Yeah after spending a bit more time thinking about it I agree fully here. No clear feature change which doens't open other issues. Frankly my goal someday is to work out a better project init for express apps which come with the right instructions which can be more opinionated than libs like hemlet can. Similar to the react meta frameworks.

[EvanHahn]

A reduced test case would be super helpful.

[wesleytodd]

So I spent a bit more time this morning looking at the 100 some apps I see using the default helmet setup, and at least 2 of them were doing something like this:

app.use(helmet());
app.use((req, res) => {
  res.redirect('https://' + req.hostname + req.url);
});

One had it behind a guard like if (process.env.NODE_ENV === "production") but not the other. This is very clearly programmer error, so I am not saying it is anything helmet does wrong, but it certianly does make it easy to make this mistake.

Seems to me like a an addition like this might be a good resolution:

note: for local development, if the HSTS header is sent over https it will cause all localhost servers to try to redirect from http to https which might cause errors. If you have any redirects like res.redirect('https://' + req.hostname + req.url) be sure you protect against running both that redirect and hsts: true in helmet while serving from localhost.

[EvanHahn]

Added a small note to the readme in 7674c63.

Thanks for opening this issue, and hope it helps!