Skip to content

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

' -P --proxy Fallback proxy if the request cannot be resolved. e.g.: http://someurl.com',
' -P --proxy Fallback proxy if the request cannot be resolved. e.g.: http://someurl.com',
' --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