fixing panic when root is symlink #1429

Merged
merged 6 commits into from Feb 16, 2017

Projects

None yet

2 participants

@pennywisdom
Collaborator

Re-submitting PR (tests passing running locally!)
When the root path that is set is a symlink, caddyserver will not start and panics.
This PR is adding a check to determine if the path is a symlink first and if it is then delegate the responsibility that it is a valid path to the underlying OS, otherwise continue with the existing checks.

pennywisdom added some commits Feb 13, 2017
@pennywisdom pennywisdom fixing panic when root is symlink
checking root path is a symlink before os.Stat which panics
aaee9ae
@pennywisdom pennywisdom fixing formatting
70bf923
@mholt
Owner
mholt commented Feb 14, 2017

Thanks @pennywisdom! I am surprised that it straight-up panicked in that case... do you think you could write a test so that it preserves the fix contained in your patch?

@pennywisdom
Collaborator

yes @mholt i'll get one/some created.

pennywisdom added some commits Feb 14, 2017
@pennywisdom pennywisdom Merge branch 'master' into fix-rootsymlinkcheck
cdb004a
@pennywisdom pennywisdom adding test to verify symlink root path check ef89555
@pennywisdom pennywisdom Merge branch 'fix-rootsymlinkcheck' of github.com:pennywisdom/caddy i…
…nto fix-rootsymlinkcheck
285f4ea
@pennywisdom pennywisdom fixing typo
a3db78e
@pennywisdom
Collaborator
pennywisdom commented Feb 14, 2017 edited

Not sure how to best to handle this issue on Windows (not server). You need to be running as Administrator to create a symlink and if you are not then an error throw, however I'm not sure this is a good case for failing the tests, seems like a bit of an edge case, and we only need to create the symlink in the test, its not part of running Caddy.
For this reason, the test verifies that the error is thrown when privileges need elevating and skips the test, which will be logged by the runner.

@mholt
mholt approved these changes Feb 16, 2017 View changes

I have nothing to add. 👍 Thanks for fixing this!

@mholt mholt merged commit 55bded6 into mholt:master Feb 16, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@mholt
Owner
mholt commented Feb 16, 2017

I also really appreciate your effort in writing the test case and considering the permissions issues. I think you handled it fine. Would you like to be added as a collaborator on the project? That means you could contribute without having to fork, you could just do it in a branch, and you could help review pull requests and issues.

@pennywisdom
Collaborator

No probs at all, really happy to help out. Yes please do add me as a collaborator that would be great!

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