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

Disable upnp from being discovered #17937

Merged
merged 1 commit into from Oct 29, 2018

Conversation

Projects
None yet
5 participants
@balloob
Member

balloob commented Oct 29, 2018

Description:

This disables upnp component from being discovered.

I realized that by making this discoverable, we are now suggesting every new user to open a port on their router and expose Home Assistant. We shouldn't do this.

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

@pvizeli pvizeli merged commit 98dfbf2 into dev Oct 29, 2018

6 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 92.935%
Details

@pvizeli pvizeli deleted the disable-upnp-discovery branch Oct 29, 2018

@wafflebot wafflebot bot removed the in progress label Oct 29, 2018

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

@StevenLooman

This comment has been minimized.

Contributor

StevenLooman commented Nov 11, 2018

As per #18066

StevenLooman commented 4 hours ago

Apparently upnp is forbidden completely from now on via #17937 by @pvizeli. While I agree with the danger by of the port forward, it was included after discussion with @dgomes.

Still, simply disabling discovery, effectively destroying the functionality was a bit of an overkill. @pvizeli was there no other way, such as discussing this with @dgomes or me about other options?

dgomes commented 4 hours ago

It would be dangerous if auto-discovery would enable the port-forwarding automatically. But users still needed to tick the option to open the port. Or did I miss a step ?

StevenLooman commented 4 hours ago

I agree that it is too easy to enable the port-forward. If a user does not know what it is, it is very easy to simply tick it.

If a port-forward can only be enabled via the configuration file, then users will be safer.

dgomes commented 4 hours ago

On the other hand the all point of using UPnP to port-forward stuff is not to mess with configurations :)

I think a warning in the GUI should have been enough

(BTW I think these comments should be moved to the other thread)

@StevenLooman

This comment has been minimized.

Contributor

StevenLooman commented Nov 11, 2018

A solution would be that port-forward can only be enabled by using the configuration and not showing the port-forward option in the Integrations menu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment