-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
base: master
Are you sure you want to change the base?
auto HTTPS redirect #600
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@raphaellueckl is this happening any time soon? 🙏🏼 |
@damianobarbati Sorry, I am not a dev on the project. :/ I am also waiting for that feature to be shipped. |
any reason this PR has stalled? |
good question... only thing I can think of is that tests are missing, but nobody ever complained, soo..? idk 🤷♂️ |
There was a problem hiding this 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', |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.. ^^
Why isn't this approved yet? |
I'd love this merge, I really don't wanna learn nginx! |
Please ensure that your pull request fulfills these requirements:
master
branchWhat 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?
--https-redirect
/-R
to the CLI-R
is set and log errors if--port
is wrongly configuredmake two listeners: one for http-redirect, one for the main https server
before
on the server to check whether it's a redirect server and do the redirect actionProvide 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
andisSSLServer
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..