Skip to content
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

Add secure false, to allow HTTPS + proxy #720

Closed
wants to merge 1 commit into from

Conversation

dwjohnston
Copy link

@dwjohnston dwjohnston commented Aug 16, 2021

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? (bug fix, enhancement, new feature,...)

Allow using the proxy feature with HTTPS

#719

What changes did you make?

Set secure flag to false for http-proxy (so it won't try verify self signed certs).

  • Question: Is the purpose of this library for development only? If not, this probably isn't a good solution.
  • Possibly want a 'verifySslCerts' option, that you can set to false, for this use case.

Provide some example code that this change will affect, if applicable:

Starting a server with:

http-server --proxy https://localhost:8080? ./lib -S -C localhost.crt -K localhost.key -p 8080

Where we are using a selfsigned certificate.

Is there anything you'd like reviewers to focus on?

Please provide testing instructions, if applicable:

Run the above command, check that you can access the main page at https://localhost:8080/foobar

I haven't written any tests yet! Working that out now.

@dwjohnston
Copy link
Author

I've tried adding tests here, but running into all sorts of issues: https://github.com/dwjohnston/http-server/tree/try-tests

@thornjad
Copy link
Member

thornjad commented Oct 5, 2021

Thank you for your PR!

Question: Is the purpose of this library for development only? If so, this probably isn't a good solution.

While http-server has development usage as its primary goal, we do want to be strong enough for production applications as well. So I agree, just setting it to false might be a bit heavy-handed.

It's also possible this will be covered by #688?

@dwjohnston
Copy link
Author

It's also possible this will be covered by #688?

Definitely looks like it is covered there.

@thornjad
Copy link
Member

thornjad commented Oct 6, 2021

👍 Then I'm going to close this in favor of the more general #688

@thornjad thornjad closed this Oct 6, 2021
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.

None yet

2 participants