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

Fix #981 Use -webserverip to set http listen address #1032

Merged
merged 7 commits into from May 24, 2021

Conversation

leuc
Copy link
Contributor

@leuc leuc commented May 21, 2021

Followed the HTTP port code with an extra IP address validation in the router.

It works via command line flag, but not sure what is needed for configuration from the admin web UI.

Literally my first Go code :)

Copy link
Member

@gabek gabek left a comment

Choose a reason for hiding this comment

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

Thank you! Just one comment about the default.

config/config.go Outdated Show resolved Hide resolved
config/defaults.go Outdated Show resolved Hide resolved
@gabek gabek linked an issue May 21, 2021 that may be closed by this pull request
@leuc
Copy link
Contributor Author

leuc commented May 21, 2021

default listen address changed.

Added admin REST API endpoint, with type validation. Only tested with curl, not against the actual admin UI.

This should validate the IP, so people can't brick the config. Not sure about the UI interaction in case of an error, so left out for now.

Comment on lines 297 to 300
_, is_float := configValue.Value.(float64)
if !is_float {
return
}
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this check! Instead of asserting the type once here to check it, and then another time to actually use it, could this be swaped around a bit (excuse the bad code inside a comment box):

if port, ok := configValue.Value.(float64); ok {
...setHttpPort(port)
} else {
BadRequestHandler(w, "bad request or some message")
}

And the same can probably be done with the SetWebServerIP check/use as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

71aca7e should have a better logic and also checks port range and validates given IP

@gabek
Copy link
Member

gabek commented May 22, 2021

@leuc Would you mind commenting on the issue at #981 so I can make sure you get credit for closing it? :) It won't let me assign somebody who hasn't been involved in the discussion haha.

@gabek
Copy link
Member

gabek commented May 24, 2021

Thanks for working on this! I'm not sure yet if we want this configurable in the admin or not (I'm kind of leaning to it being a cli flag only), so I'll probably pull out the APIs if that's the case, but I'll leave it for now so we have the option. Thanks again!

@gabek gabek merged commit 5ab901b into owncast:develop May 24, 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.

Allow binding the web server port to a specific address
2 participants