-
Notifications
You must be signed in to change notification settings - Fork 275
Add option to enforce TLS by redirecting HTTP requests to HTTPS (#1092) #1093
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: dev
Are you sure you want to change the base?
Conversation
IDisposable
left a comment
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.
This looks really great! Thanks!
Just a couple concerns about defensively protecting against accidental lockout.
… AND apply button. (jetkvm#1092)
…ebRTC setting changes. (jetkvm#1092)
…tring to identify disabled. Default to disabled. (jetkvm#1092)
…bled before enforcing. (jetkvm#1092)
|
I've tried to address all the issues here. If there are any more please let me know. I've tried to focus on enforce TLS not being enabled unless everything is working okay.... and making the user jump through some hoops to make it happen. |
|
I should note explicitly there are two other semi-related problems fixed here. That need attention (maybe). I don't really know Go (or TS for that matter).
|
…orce TLS to Enforce HTTP (TLS).
… HTTPS server to start. (jetkvm#1092)
IDisposable
left a comment
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.
Looks good to me ![]()
adamshiervani
left a comment
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.
"Enforce HTTPS (TLS)" doesn't work for me. It gets checked in the UI, but after a refresh of the page, it's unchecked, and I can access it through HTTP.
Could you please also add a E2E test for this? AI can be very helpful writing the E2E tests.
|
I just pushed a change that should fix what you're seeing in "self-signed" mode. The button should now appear, to apply TLS settings, when "self-signed" mode is used. The intent is for the user to agree to the popup AND click the apply button to enforce TLS. As for E2E tests, I'll probably pass on that sorry. If someone wants to take that up, they are more than welcome. If that means this sits here, that's fine. I'll maintain my own fork rebasing against release versions for those that want the feature, for as long as I own a JetKVM. Unfortunately, the complete lack of response from JetKVM's support channels has made me re-think the investment here. Sorry about that. |
Closes #1092
Summary
Checklist
make test_e2elocally and passedCloses #<issue-number>)