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 New-Item to create symlink to relative path target #12797

Merged

Conversation

iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented May 26, 2020

PR Summary

Fix #9127.

Before the fix we check an item existence relative to system CWD. After the fix we do the check based on current runspace CWD.

PR Context

PR Checklist

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label May 26, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.4 milestone May 26, 2020
@iSazonov iSazonov requested a review from anmenaga as a code owner May 26, 2020 16:17
@vexx32
Copy link
Collaborator

vexx32 commented May 26, 2020

@iSazonov thanks for taking the time to sort this one out. 😊

Quick question -- should / will this affect the -ItemType Junction / -ItemType Hardlink modes for New-Item and should we be testing those here as well?

@iSazonov iSazonov force-pushed the filesystemprovider-relsymlink branch from 3f947dd to 1c0b49a Compare May 26, 2020 19:56
@adityapatwardhan
Copy link
Member

@iSazonov Please have a look at the test failure

@iSazonov
Copy link
Collaborator Author

@mklement0 Could you please help with the MacOs fail? Do you have any thoughts?

@adityapatwardhan adityapatwardhan added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 29, 2020
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 1, 2020
@iSazonov iSazonov force-pushed the filesystemprovider-relsymlink branch from e4d4a1f to 906b318 Compare June 1, 2020 09:25
@iSazonov iSazonov force-pushed the filesystemprovider-relsymlink branch from 08bcd9c to f2b56f7 Compare June 1, 2020 13:34
@adityapatwardhan adityapatwardhan merged commit 3de9069 into PowerShell:master Jun 1, 2020
@adityapatwardhan
Copy link
Member

@iSazonov Thank you for your contribution!

@ghost
Copy link

ghost commented Jun 25, 2020

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

@MatejKafka
Copy link

MatejKafka commented Apr 14, 2021

Is this really how relative path for symlink should behave? This PR changes the behavior of the checks from having -Target relative to .NET CWD to PowerShell CWD, but it's still inconsistent with how filesystem actually resolves the symlink.

Relative path in the symlink target is resolved by filesystem relative to the directory containing the symlink, not relative to CWD of pwsh instance that created it.


This should create a working directory symlink:

mkdir ./test
mkdir ./test/dir
ni -Type SymbolicLink ./test/dir_link -Target ./dir

Instead, the check uses path relative to current working directory (./dir) instead of relative to symlink path (./test/dir).

@iSazonov
Copy link
Collaborator Author

@MatejKafka Please create new issue if you see an inconsistency.

@mklement0
Copy link
Contributor

mklement0 commented Apr 14, 2021

@MatejKafka, while you are correct, this problem doesn't currently surface, although there are related ones:

With respect to creating the symlink, i.e. what target path is stored in its definition:

With respect to validating the existence of the target path on creation:

  • This validation - which should indeed happen relative to the symlink's location - is currently broken on all platforms, so that targeting a non-existing path is quietly possible, with the exception of using full paths with non-existent drive names / UNC paths.

@MatejKafka
Copy link

MatejKafka commented Apr 14, 2021

@mklement0 Actually, I think it currently surfaces. Because the checks are relative to cwd, they incorrectly detect whether the target is file or directory. Let me try to illustrate it with more examples:

(I'll use D:\test as our testing directory to make it clear when I'm talking about absolute paths)

mkdir D:/test
mkdir D:/test/dir
ni -Type SymbolicLink D:/test/dir_link -Target ./dir

Expected: directory symlink to D:/test/dir, with target ./dir
Got: file symlink to D:/test/dir, with target ./dir
The target type detection code checks the relative path ./dir (D:/dir), which doesn't exist, so the symlink incorrectly defaults to file symlink.

mkdir D:/test
mkdir D:/test/dir
cd D:/test
ni -Type SymbolicLink D:/test/dir_link -Target ./dir

Expected: directory symlink to D:/test/dir, with target ./dir
Got: what I expected
Only difference from previous example is the cd D:/test. This correctly creates a directory symlink, because the code checks type of target for ./dir (D:/test/dir), which now matches the real target.

mkdir D:/test/subdir
mkdir D:/test/item # <-- directory
ni D:/test/subdir/item # <-- file
cd D:/test
ni -Type SymbolicLink D:/test/subdir/item_link -Target ./item

Expected: file symlink to D:/test/subdir/item, with target ./item
Got: directory symlink D:/test/subdir/item, with target ./item
The the file D:/test/item is used for target check, it's a directory, so a directory symlink is created (but the actual target is D:/test/subdir/item, which is a file).


There are 2 possible correct behaviors I see here:

  1. Set absolute path as a symlink target, and resolve the target path to be absolute. This effectively prevents PowerShell from creating symlinks with relative paths to target, but still solves this issue.
  2. Allow relative paths, but when checking for target type (file/directory), use the path Join-Path $Path $Target internally, but still write $Target as provided by user (relative) into the symlink itself. This is imo the better solution, allows the user to create symlinks with relative paths, but still correctly detects the type.

@mklement0
Copy link
Contributor

Good point, @MatejKafka. As @iSazonov has advised, please create a new issue with your findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New-Item -Type SymbolicLink doesn't define symlinks with relative directory targets as directory symlinks
6 participants