florianheinemann / express-sslify Goto Github PK

I was looking at the code and it seems that now, there is no way to specify to which SSL port will I perform the redirect.

This is an issue when I am testing locally with a custom HTTPS server that is listening on port, say, 3001 which is different from the default 443.

I will gladly make the necessary fixes and submit a PR, but would like to listen to your opinion first before start coding anything.

as far as the example goes, i'm not sure how do you listen to both ports at the same time?

var express = require('express');
var http = require('http');
var enforce = require('express-sslify');

var app = express();
app.use(enforce.HTTPS());

http.createServer(app).listen(app.get('port'), function() {
    console.log('Express server listening on port ' + app.get('port'));
});

why do you wrap express app with another server ?

If Strict-Transport-Security headers are not set, it's trivial to strip SSL on connections made later from insecure networks (see https://vimeo.com/50018478#t=23m30s).

As such, I think it makes sense for express-sslify to:
(a) have an option to set HSTS headers
(b) for that option to be enabled by default

Whilst currently users can combine express-sslify with https://github.com/helmetjs/hsts , I think having this functionality in-built makes sense.

This would be similar to what wgsi-sslify (a Python WGSI equivalent of this project) does:
https://github.com/jacobian/wsgi-sslify/blob/c2d25e5cb735029d7f1f37af8ad9d30988373f89/wsgi_sslify.py#L20-L23

The comment in the example code snippet in the readme says:

// use HTTPS(true) in case you are behind a load balancer (e.g. Heroku)

This is now deprecated. The comment should either refer to the new enforce.HTTPS({ trustProtoHeader: true }) option or else just be removed entirely, since the later section in the readme deals with it.

I do not test it but I think that redirecting should not work with cross domain request request because OPTIONS method will produce 403.

https://github.com/florianheinemann/express-sslify/blob/master/index.js#L25

I just had a bug (on heroku) where initially I was able to load the site over http but after a refresh it redirected to the https address.

Turned out I had to put the enforce.HTTPS before any other middleware. This is not mentioned in the docs so I hope this helps people who are having the same issue.

So to sum it up:
WORKS:
app.use(enforce.HTTPS({trustProtoHeader: true}));
app.use(express.static(path.join(__dirname, 'dist')));

DOESN'T WORK
app.use(express.static(path.join(__dirname, 'dist')));
app.use(enforce.HTTPS({trustProtoHeader: true}));

Hello 👋

I run a security community that finds and fixes vulnerabilities in OSS. A researcher (@yakirk) has found a potential issue, which I would be eager to share with you.

Could you add a SECURITY.md file with an e-mail address for me to send further details to? GitHub recommends a security policy to ensure issues are responsibly disclosed, and it would help direct researchers in the future.

Looking forward to hearing from you 👍

(cc @huntr-helper)

The balancing proxy at the hosting platform we use includes port 80 in req.headers.host (domain.com:80). When this module redirects a request, the redirect url looks like https://domain:80. Browsers seem to auto-fix it, but some other tools actually try to make a secure connection on port 80, and they fail.

I believe req.hostname should be used instead of req.headers.host when generating the redirect url. The trustProtoHeader setting won't be necessary then - app.set('trust proxy', true); would control the behavior.

I know the project doesn't get much updates, so this is mostly a warning for people to be aware of a potential issue.

The version in npm (0.1.1) does not include the latest fix that stops using the deprecated express function. A new version would be appreciated. Thanks.

Everything is working properly on my heroku app, but now when I try to run the app on my localhost I get the following error in my browser window:

Secure Connection Failed

An error occurred during a connection to localhost:3000. SSL received a record that exceeded the maximum permissible length.

Error code: SSL_ERROR_RX_RECORD_TOO_LONG

Any tips? I tried adding

if (app.get("env") === "production") {
   app.use(enforce.HTTPS({ trustProtoHeader: true }));
}

It did not work. Thanks!

In many cases, I would want to handle error in a different way than sending a 403 with a hardcoded text message ("Please use HTTPS when submitting data to this server.").

This alternative falls short if I am rendering and serving a custom HTML view, serving an API with standarized error structure, a public website with customized error pages, or want to perform some custom action before actually redirecting, and a big, big etc.

Without going any further, just trying to localise the app, I have no way of at least customize the text message.

So for flexibility, I think it would be better to just throw a SSLRequiredError and let the developer decide what to do with that. He/she can catch it in an error middleware and handle it accordingly.

For compatibility issues, maybe you can pass a throwError boolean property in options that throws this SSLRequired error if activated, and sends the current 403 with the hardcoded message, otherwise.

I will gladly make the necessary fixes and submit a PR, but would like to listen to your opinion before start coding anything.

Whenever I use this package, it doesn't work in my local environment because https://localhost:### throws errors. Can we add another check like this?

if(req.headers && req.headers.host && req.headers.host === 'localhost:####') {
next();
}

You get a redirect loop on Azure.

// Second, if the request headers can be trusted (e.g. because they
// are sent by a trusted proxy), check if x-forward-proto is set to https
if (!isHttps && trustProxy) {
  if (req.headers['x-forwarded-proto'] === 'https') {
    isHttps = true;
  } else if (req.headers['x-arr-ssl']) {  // for Azure*
    isHttps = true;
  }
}

I think arguments are absolutely menaingless

app.use(enforce.HTTPS(false, true))

instead that should be something like

app.use(enforce.HTTPS({azure: true, heroku: true}))