Skip to content

auto HTTPS redirect #600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

thisIsTheFoxe
Copy link

@thisIsTheFoxe thisIsTheFoxe commented Jan 18, 2020

Please ensure that your pull request fulfills these requirements:

  • The pull request is being made against the master branch
  • Tests for the changes have been added (for bug fixes / features)

What is the purpose of this pull request? (enhancement)
It enables user to auto-redirects their site from HTTP -> HTTPS. To do that it listens on two ports. On one runs the (main) HTTPS server and on the other just a redirect server which takes requests and redirects them to the (main) HTTPS server.

Resolves #598
Resolves #273

What changes did you make?

  • added name to contributors
  • added --https-redirect / -R to the CLI
    • checked if -R is set and log errors if --port is wrongly configured
    • if everything checks out:
      make two listeners: one for http-redirect, one for the main https server
  • added a script that runs in before on the server to check whether it's a redirect server and do the redirect action

Provide some example code that this change will affect, if applicable:
The only thing that effects the other parts is that I check whether -p is set and log an error if it is, but no port is passed as argument

$ node http-server -p
^^^^ this now throws an error (earlier)

Is there anything you'd like reviewers to focus on?
I did a npm pretest so lint should be ok...
The way I handled the actual redirect might not be prefect, but it was the only thing that worked for me .-.
I hope https-redirect, redirectPort and isSSLServer are understandable ^^

Please provide testing instructions, if applicable:
For testing I would need a SSL certificate, since it only works with SSL (-S -C <...>)... ¯\(ツ)
I did test it tho, with a self-signed one on my machine..

Copy link

@raphaellueckl raphaellueckl left a comment

Choose a reason for hiding this comment

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

lgtm

@damianobarbati
Copy link

@raphaellueckl is this happening any time soon? 🙏🏼

@raphaellueckl
Copy link

@damianobarbati Sorry, I am not a dev on the project. :/ I am also waiting for that feature to be shipped.

@WORMSS
Copy link

WORMSS commented Aug 16, 2021

any reason this PR has stalled?

@thisIsTheFoxe
Copy link
Author

good question... only thing I can think of is that tests are missing, but nobody ever complained, soo..? idk 🤷‍♂️
And as explained in the PR, there's no tests for SSL/HTTPS at all (afaik), so one would have to add those before anyway (together with test-certificates)..

Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

So sorry, I thought I made this review, but I find today it's still pending! I'll approve tests to run as well

' --proxy-options Pass options to proxy using nested dotted objects. e.g.: --proxy-options.secure false',
' -R --https-redirect Auto redirects http -> https',
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that -R properly captures the option, how do you feel about using only --https-redirect?

Copy link
Author

Choose a reason for hiding this comment

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

Heyy thanks so much for the review ^^

And well, I don't have a problem removing it.. I just thought that it's a somewhat much requested and often used feature and it'd be nice to have a shortcut for Redirection.. your call.. ^^

@Stan-Stani
Copy link

Why isn't this approved yet?

@jonathanfishbein1
Copy link

I'd love this merge, I really don't wanna learn nginx!

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

Successfully merging this pull request may close these issues.

SLL mode does not serve HTTP request Enabling HTTP to HTTPS redirects
7 participants