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: reject on listen error, listen on hostname #86

Merged
merged 6 commits into from
May 18, 2018
Merged

Conversation

trs
Copy link
Contributor

@trs trs commented May 15, 2018

Thanks to @TimLuq (#85)

@trs trs added bug A defect or bug that affects the original indended use of the application in progress labels May 15, 2018
@trs trs self-assigned this May 15, 2018
@trs
Copy link
Contributor Author

trs commented May 15, 2018

@TimLuq I can see some impacts with the hostname fix.

Because it wasn't specified, it would listen on all available addresses (0.0.0.0), so this might break some builds.

I'll do some further testing before pulling this into master.

@TimLuq
Copy link
Contributor

TimLuq commented May 15, 2018

Yes, when I saw that the checks failed I did a quick skim through the test cases and assumed that the tests were incomplete in this regard.

Since the new behavior is the one previously documented most dependents should probably have correct listen calls. But there are sure to be some that are not.

Perhaps releasing this fix under a new minor version instead of a patch? Although that is technically bad practice. But so is breaking projects with a patch update. (Also with a minor update, but since it follows both the documented and intended behavior it could be done.) Tough call.

@TimLuq
Copy link
Contributor

TimLuq commented May 16, 2018

This could also be seen as a security patch since users who are not behind a firewall and have configured what they believe to be a localhost-only server currently accepts external connections. Maybe they have given full filesystem access to an anonymous connection as they assume only localhost may establish connections.

@trs trs force-pushed the fix-server-listen branch 3 times, most recently from fd149fb to 5f9db31 Compare May 16, 2018 18:57
@trs trs added the breaking label May 16, 2018
@trs trs force-pushed the fix-server-listen branch 19 times, most recently from fe41a69 to 8db502c Compare May 16, 2018 20:22
@trs
Copy link
Contributor Author

trs commented May 16, 2018

I think I will release this as a breaking change, so a new major version. It has a chance of breaking some peoples usages, so it's the safest way to release this change.

@trs
Copy link
Contributor Author

trs commented May 18, 2018

Looking at others that are using ftp-srv, I don't believe this will have any negative impacts on their projects. I think a minor version will suffice.

trs added 5 commits May 18, 2018 14:49
By default, `net.Server` listens on `0.0.0.0`, which is any available address. This ensures that ftp-srv only listens on the set hostname.
Instead of a direct equal comparison, use the `ip` package
This was an auto generated document, actually make it
@trs trs merged commit 3a7b3d4 into master May 18, 2018
@trs trs deleted the fix-server-listen branch May 18, 2018 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect or bug that affects the original indended use of the application chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants