Repository: pallets/click (python)

Issue:
Regression in click>=8; click.Path says file doesn't exist when it does
<!--
This issue tracker is a tool to address bugs in Click itself. Please use
Pallets Discord or Stack Overflow for questions about your own code.

Replace this comment with a clear outline of what the bug is.
-->

https://github.com/pallets/click/pull/1825 seems to introduce a regression. A script which worked with `click<8` does not work when upgraded to `click>=8`, and the call to `os.readlink` this PR introduces seems to be the cause. I.e. it looks like fixing symlinks in this case for old Pythons on Windows has broken symlinks for more current Pythons on Linux, however I have not tested as thoroughly as I should be making that claims like that!

I have a kubernetes "secret" (could just as easily be a configmap) and these create multiple symlinks do enable atomic replacement of all files in a secret or config map at once. An example secret might look like this:

```
# ls -lart
total 0
lrwxrwxrwx    1 root     root            14 May 20 14:12 mysecret -> ..data/mysecret
lrwxrwxrwx    1 root     root            31 May 20 14:12 ..data -> ..2021_05_20_14_12_50.273872148
drwxr-xr-x    2 root     root           100 May 20 14:12 ..2021_05_20_14_12_50.273872148
drwxrwxrwt    3 root     root           140 May 20 14:12 .
drwxr-xr-x    1 root     root            50 May 20 14:12 ..
```

(Yes, those files and folders have a `..` prefix which is confusing, and no I can't make kubernetes not do that).

Lets say this secret was mounted as `/foo`. Prior to https://github.com/pallets/click/pull/1825, the code would call `os.path.realpath` on `/foo/mysecret` it would resolve the symlink correctly and end up with `/foo/..2021_05_20_14_12_50.273872148/mysecret`

After https://github.com/pallets/click/pull/1825, it calls `os.readlink` and gets `..data/mysecret` (not an abspath). When you call `os.path.realpath` on that it prefixes the current directory, so if you are in `/` you get `/..data/tls.crt`. `os.stat` on `/..data/mysecret` fails. But `os.stat` on `/foo/mysecret` does not. So definitely a regression, and I think a wrong behaviour.

I think calling `os.readlink` is not a replacement for a proper symlink resolver. Given click only supports Python 3.6 or later, I wonder if using `pathlib.Path(...).resolve()` here would be a better option. It does have a Windows symlink resolver (https://github.com/python/cpython/blob/3.6/Lib/pathlib.py#L181) and a Posix one (https://github.com/python/cpython/blob/3.6/Lib/pathlib.py#L303). Though I don't have a Windows machine to test it out.

<!--
Describe how to replicate the bug.

Include a minimal reproducible example that demonstrates the bug.
Include the full traceback if there was an exception.
-->

For a reproducer I took the example for `click.Path` from the docs and enabled the resolve_path and exists checks:

```
import click

@click.command()
@click.argument('filename', type=click.Path(exists=True, resolve_path=True))
def touch(filename):
    """Print FILENAME if the file exists."""
    click.echo(click.format_filename(filename))


if __name__ == '__main__':
    touch()
```

Then I made an empty file with a symlink pointing to it:

```
# mkdir tst
# cd tst
# touch foo
# ln -s foo bar
# ls
-rw-r--r--    1 root     root             0 May 20 15:10 foo
lrwxrwxrwx    1 root     root             3 May 20 15:10 bar -> foo
```

```
/ # /app/bin/python /app/test.py /tst/bar
Usage: test.py [OPTIONS] FILENAME
Try 'test.py --help' for help.

Error: Invalid value for 'FILENAME': Path '/tst/bar' does not exist.
```

However if I downgrade click:

```
/ # /app/bin/pip install 'click<8'
Collecting click<8
  Using cached click-7.1.2-py2.py3-none-any.whl (82 kB)
Installing collected packages: click
  Attempting uninstall: click
    Found existing installation: click 8.0.1
    Uninstalling click-8.0.1:
      Successfully uninstalled click-8.0.1
Successfully installed click-7.1.2
```

```
/ # /app/bin/python /app/test.py /tst/bar
/tst/foo
```

Because i'm paranoid, i upgraded click again and confirmed the reproducer fails again. It does.

<!--
Describe the expected behavior that should have happened but didn't.
-->

Environment:

- Linux (Alpine)
- Python version: 3.8.10
- Click version: >8

PS, transcript from running the reproducer on macOS Catalina too, with python 3.9 from brew:

```
% mkdir click-bug
% cd click-bug 
% python3 -m venv venv
% source venv/bin/activate
(venv) % pip install click
Collecting click
  Downloading click-8.0.1-py3-none-any.whl (97 kB)
     |████████████████████████████████| 97 kB 5.7 MB/s 
Installing collected packages: click
Successfully installed click-8.0.1
(venv)  % mkdir tst
(venv)  % cd tst
(venv)  % touch foo
(venv)  % ln -s foo bar
(venv)  % cd ..
(venv)  % vi test.py
(venv)  % python test.py tst/bar 
Usage: test.py [OPTIONS] FILENAME
Try 'test.py --help' for help.

Error: Invalid value for 'FILENAME': Path 'tst/bar' does not exist.
(venv) % pip install "click<8"
Collecting click<8
  Downloading click-7.1.2-py2.py3-none-any.whl (82 kB)
     |████████████████████████████████| 82 kB 3.0 MB/s 
Installing collected packages: click
  Attempting uninstall: click
    Found existing installation: click 8.0.1
    Uninstalling click-8.0.1:
      Successfully uninstalled click-8.0.1
Successfully installed click-7.1.2
(venv) % python test.py tst/bar
/Users/me/click-bug/tst/foo
```

Pull request description:
fix resolution of relative symlinks
This resolves (relative) symlinks to the right directory. This is done by prepending the absolute directory path of the relative symlink to the resolved path.

- fixes #1921 

Checklist:

- [x] Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
- [ ] Add or update relevant docs, in the docs folder and in code.
- [x] Add an entry in `CHANGES.rst` summarizing the change and linking to the issue.
- [ ] Add `.. versionchanged::` entries in any relevant code docs.
- [x] Run `pre-commit` hooks and fix any issues.
- [x] Run `pytest` and `tox`, no tests failed.


Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index e5876af..2193f96 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -12,6 +12,8 @@ Unreleased
     :issue:`1969`
 -   Fix shell completion for arguments that start with a forward slash
     such as absolute file paths. :issue:`1929`
+-   ``Path`` type with ``resolve_path=True`` resolves relative symlinks
+    to be relative to the containing directory. :issue:`1921`
 
 
 Version 8.0.1
diff --git a/src/click/types.py b/src/click/types.py
index 21f0e4f..cd39469 100644
--- a/src/click/types.py
+++ b/src/click/types.py
@@ -836,11 +836,20 @@ class Path(ParamType):
 
         if not is_dash:
             if self.resolve_path:
-                # realpath on Windows Python < 3.8 doesn't resolve symlinks
+                # Get the absolute directory containing the path.
+                dir_ = os.path.dirname(os.path.abspath(rv))
+
+                # Resolve a symlink. realpath on Windows Python < 3.9
+                # doesn't resolve symlinks. This might return a relative
+                # path even if the path to the link is absolute.
                 if os.path.islink(rv):
                     rv = os.readlink(rv)
 
-                rv = os.path.realpath(rv)
+                # Join dir_ with the resolved symlink. If the resolved
+                # path is relative, this will make it relative to the
+                # original containing directory. If it is absolute, this
+                # has no effect.
+                rv = os.path.join(dir_, rv)
 
             try:
                 st = os.stat(rv)
@@ -871,7 +880,7 @@ class Path(ParamType):
                     param,
                     ctx,
                 )
-            if self.writable and not os.access(value, os.W_OK):
+            if self.writable and not os.access(rv, os.W_OK):
                 self.fail(
                     _("{name} {filename!r} is not writable.").format(
                         name=self.name.title(), filename=os.fsdecode(value)
@@ -879,7 +888,7 @@ class Path(ParamType):
                     param,
                     ctx,
                 )
-            if self.readable and not os.access(value, os.R_OK):
+            if self.readable and not os.access(rv, os.R_OK):
                 self.fail(
                     _("{name} {filename!r} is not readable.").format(
                         name=self.name.title(), filename=os.fsdecode(value)


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 9440804..d33df11 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -1,3 +1,7 @@
+import os
+import shutil
+import tempfile
+
 import pytest
 
 from click.testing import CliRunner
@@ -6,3 +10,22 @@ from click.testing import CliRunner
 @pytest.fixture(scope="function")
 def runner(request):
     return CliRunner()
+
+
+def check_symlink_impl():
+    """This function checks if using symlinks is allowed
+    on the host machine"""
+    tempdir = tempfile.mkdtemp(prefix="click-")
+    test_pth = os.path.join(tempdir, "check_sym_impl")
+    sym_pth = os.path.join(tempdir, "link")
+    open(test_pth, "w").close()
+    rv = True
+    try:
+        os.symlink(test_pth, sym_pth)
+    except (NotImplementedError, OSError):
+        # Creating symlinks on Windows require elevated access.
+        # OSError is thrown if the function is called without it.
+        rv = False
+    finally:
+        shutil.rmtree(tempdir, ignore_errors=True)
+    return rv
diff --git a/tests/test_types.py b/tests/test_types.py
index e7e531e..58d1b68 100644
--- a/tests/test_types.py
+++ b/tests/test_types.py
@@ -1,6 +1,8 @@
+import os.path
 import pathlib
 
 import pytest
+from conftest import check_symlink_impl
 
 import click
 
@@ -100,3 +102,28 @@ def test_path_type(runner, cls, expect):
     result = runner.invoke(cli, ["a/b/c.txt"], standalone_mode=False)
     assert result.exception is None
     assert result.return_value == expect
+
+
+@pytest.mark.skipif(not check_symlink_impl(), reason="symlink not allowed on device")
+@pytest.mark.parametrize(
+    ("sym_file", "abs_fun"),
+    [
+        (("relative_symlink",), os.path.basename),
+        (("test", "absolute_symlink"), lambda x: x),
+    ],
+)
+def test_symlink_resolution(tmpdir, sym_file, abs_fun):
+    """This test ensures symlinks are properly resolved by click"""
+    tempdir = str(tmpdir)
+    real_path = os.path.join(tempdir, "test_file")
+    sym_path = os.path.join(tempdir, *sym_file)
+
+    # create dirs and files
+    os.makedirs(os.path.join(tempdir, "test"), exist_ok=True)
+    open(real_path, "w").close()
+    os.symlink(abs_fun(real_path), sym_path)
+
+    # test
+    ctx = click.Context(click.Command("do_stuff"))
+    rv = click.Path(resolve_path=True).convert(sym_path, None, ctx)
+    assert rv == real_path