Skip to content

Disable upnp from being discovered#17937

Merged
pvizeli merged 1 commit intodevfrom
disable-upnp-discovery
Oct 29, 2018
Merged

Disable upnp from being discovered#17937
pvizeli merged 1 commit intodevfrom
disable-upnp-discovery

Conversation

@balloob
Copy link
Copy Markdown
Member

@balloob 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.

@ghost ghost assigned balloob Oct 29, 2018
@ghost ghost added the in progress label Oct 29, 2018
Copy link
Copy Markdown

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

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
@pvizeli pvizeli deleted the disable-upnp-discovery branch October 29, 2018 14:52
@ghost ghost removed the in progress label Oct 29, 2018
@balloob balloob mentioned this pull request Nov 9, 2018
@StevenLooman
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants