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: resolving relative paths in symlinks #224

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

shqld
Copy link
Contributor

@shqld shqld commented Aug 14, 2023

This fixes nodejs/node#49107 (relative paths in symlinks resolved incorrectly).

When following a symbolic link that points to a target using a relative path, the uvwasi__resolve_path() function resolves and normalizes the link's target as a relative path starting from fd->real_path. This should be treated as a relative path starting from the parent directory of the symbolic link.

For example, given the directory structure below:

.
├── source_file
└── dir
    └── link -> ../source_file

When fd->real_path is ., the link target for link is resolved as ../source_file, not ./source_file. As a result, this resolution ends with an ENOTCAPABLE error.

@shqld shqld marked this pull request as ready for review August 14, 2023 10:25
@shqld shqld force-pushed the fix-resolving-symlink-target branch from feff9b7 to 89b6941 Compare August 14, 2023 10:34
@cjihrig
Copy link
Collaborator

cjihrig commented Aug 14, 2023

Thanks for the PR. It looks like there is a compilation error: https://github.com/nodejs/uvwasi/actions/runs/5854703133/job/15876962601

@shqld
Copy link
Contributor Author

shqld commented Aug 14, 2023

Sorry, I forgot to remove the unused variable 🤦🏼 and fixed another memory problem in tests.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

I think this would be safe to merge provided we can include documentation to the project along the lines of it not providing a security sandbox so that we don't need to consider this PR from a security perspective.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson merged commit 0427f19 into nodejs:main Dec 7, 2023
7 checks passed
yagehu pushed a commit to yagehu/uvwasi that referenced this pull request Dec 8, 2023
* fix: resolving relative paths in symlinks

* remove unused variable

* fix memory leak in tests

* fix mistaken assertions for windows in tests
@shqld shqld deleted the fix-resolving-symlink-target branch December 12, 2023 03:37
mhdawson added a commit to mhdawson/io.js that referenced this pull request Jan 3, 2024
- update uvwasi to 0.0.20
- adjust tests to reflect udpated behaviour from
  nodejs/uvwasi#224 included
  in uvwasi 0.0.20

Signed-off-by: Michael Dawson <[email protected]>
mhdawson added a commit to nodejs/node that referenced this pull request Jan 8, 2024
- update uvwasi to 0.0.20
- adjust tests to reflect udpated behaviour from
  nodejs/uvwasi#224 included
  in uvwasi 0.0.20

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #51355
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Jan 12, 2024
- update uvwasi to 0.0.20
- adjust tests to reflect udpated behaviour from
  nodejs/uvwasi#224 included
  in uvwasi 0.0.20

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: nodejs#51355
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Medhansh404 pushed a commit to Medhansh404/node that referenced this pull request Jan 19, 2024
- update uvwasi to 0.0.20
- adjust tests to reflect udpated behaviour from
  nodejs/uvwasi#224 included
  in uvwasi 0.0.20

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: nodejs#51355
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request Feb 15, 2024
- update uvwasi to 0.0.20
- adjust tests to reflect udpated behaviour from
  nodejs/uvwasi#224 included
  in uvwasi 0.0.20

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #51355
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
richardlau pushed a commit to nodejs/node that referenced this pull request Mar 25, 2024
- update uvwasi to 0.0.20
- adjust tests to reflect udpated behaviour from
  nodejs/uvwasi#224 included
  in uvwasi 0.0.20

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #51355
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
richardlau pushed a commit to nodejs/node that referenced this pull request Mar 25, 2024
- update uvwasi to 0.0.20
- adjust tests to reflect udpated behaviour from
  nodejs/uvwasi#224 included
  in uvwasi 0.0.20

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #51355
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WASI preview 1: symlinks aren't handled correctly
4 participants