Don’t expand globs via symbolic links #14627

andersk · 2023-03-29T23:49:44Z

Description

Users don’t expect Prettier to follow symlinks:

If a source tree contains a symlink an external location outside the source tree, Prettier should stay within the tree it was asked to format, and shouldn’t check or modify those external files.
If a source tree contains a symlink to an internal location within itself, Prettier will already find that location via its normal path; symlinks should not cause Prettier to format those files multiple times.
If a source tree contains a symlink to a location that’s ignored via .prettierignore, Prettier should ignore that location without the user needing to separately list the symlinks as ignored too.

Following symlinks without special care can lead to problems:

Cycles of symlinks would cause Prettier to recurse infinitely or for a very long time, depending on the filesystem.
A symlink to (say) your home directory could cause Prettier to rewrite every file in your home directory.

Fixes #12080.

Checklist

I’ve added tests to confirm my change works.
(If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
(If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

✨Try the playground for this PR✨

Users don’t expect Prettier to follow symlinks: • If a source tree contains a symlink an external location outside the source tree, Prettier should stay within the tree it was asked to format, and shouldn’t check or modify those external files. • If a source tree contains a symlink to an internal location within itself, Prettier will already find that location via its normal path; symlinks should not cause Prettier to format those files multiple times. • If a source tree contains a symlink to a location that’s ignored via .prettierignore, Prettier should ignore that location without the user needing to separately list the symlinks as ignored too. Following symlinks without special care can lead to problems: • Cycles of symlinks would cause Prettier to recurse infinitely or for a very long time, depending on the filesystem. • A symlink to (say) your home directory could cause Prettier to rewrite every file in your home directory. Fixes prettier#12080. Signed-off-by: Anders Kaseorg <andersk@mit.edu>

tests/integration/__tests__/patterns-dirs.js

fisker · 2023-03-30T01:44:33Z

@andersk Thanks for working on this, we have more than glob patterns, so please fix the new tests I added.

fisker · 2023-03-30T01:47:06Z

Maybe just skip if stat.isSymbolicLink() in this if

prettier/src/cli/expand-patterns.js

Line 69 in 053bc69

if (stat) {

andersk · 2023-03-30T02:04:52Z

If Prettier throws an error when the user provides an argument that matches only non-supported files, it seems consistent that Prettier should also throw an error when the user provides an argument that matches only symlinks. I don’t think it should silently skip them as a special case.

fisker · 2023-03-30T02:34:49Z

it seems consistent that Prettier should also throw an error when the user provides an argument that matches only symlinks

We already did that?

prettier/src/cli/expand-patterns.js

Line 36 in 053bc69

error: `No matching files. Patterns: ${context.filePatterns.join(" ")}`,

Maybe we should discuss what's expected first.

Patterns only

prettier a/* b/*

Skip symlink, if non-match, throws

argument is symlink

prettier symlink/to/file

prettier symlink/to/directory

throws since no "file" matched.

pattern + symlink

prettier a/* symlink/to/file symlink/to/directory

Skip symlink, if non-match, throws

Sounds good?

So, if stat.isSymbolicLink() === true, we just skip it should already work as expected.

andersk · 2023-03-30T02:44:00Z

Okay, updated with an specific error for that case (Explicitly specified symbolic link was ignored rather than the generic No matching files).

tests/integration/__tests__/patterns-dirs.js

tests/integration/cli/patterns-symlinks/.gitignore

src/cli/expand-patterns.js

Signed-off-by: Anders Kaseorg <andersk@mit.edu>

src/cli/expand-patterns.js

tests/integration/__tests__/patterns-dirs.js

Co-authored-by: fisker <lionkay@gmail.com> Signed-off-by: Anders Kaseorg <andersk@mit.edu>

Signed-off-by: Anders Kaseorg <andersk@mit.edu>

fisker

Good job.

fisker · 2023-04-22T14:28:34Z

@sosukesuzuki @kachkaev Can you help review?

sosukesuzuki

LGTM!

sanmai-NL · 2023-08-15T15:20:27Z

@andersk You open with a fine problem statement, but ultimately the solution design is very crude: report an error and exit with a fault exit status whenever a symbolic link happens to be specified on the command line. In CI, it's not always possible to filter the list of files beforehand based on some special characteristics. Instead, Prettier should handle the scenarios you describe itself.

sanmai-NL · 2023-08-15T15:22:48Z

If Prettier throws an error when the user provides an argument that matches only non-supported files, it seems consistent that Prettier should also throw an error when the user provides an argument that matches only symlinks. I don’t think it should silently skip them as a special case.

@andersk

It does not follow that Prettier should skip uncontrollably. Right now, Prettier cannot be controlled in the way it handles symlinks. It now simply fails and fails CI pipelines.

andersk · 2023-08-15T19:32:48Z

@sanmai-NL Neither prettier . nor prettier "*.js" will error in the presence of skipped symlinks (unless there are only symlinks). Is it possible you accidentally wrote prettier *.js, where the unquoted wildcard is expanded by the shell before Prettier even sees it?

sanmai-NL · 2023-08-16T08:06:57Z

I didn't specify globs or relative paths (or rather, our tooling), but literal paths.

I note that Prettier does have CLI flags to ignore unmatched paths, etc., but nothing to ignore symlinks.

Whenever something changes to a matched explicit path that is a symlink (e.g., file type changed to symlink), some CI tooling will specify it to Prettier and the fault will trigger.

sanmai-NL · 2023-10-09T15:00:31Z

@andersk What can you do to fix this situation? We have pipelines failing on this with no recourse ...

Otherwise, I'll have to submit PR that restores the original behavior.

andersk · 2023-10-09T17:38:48Z

@sanmai-NL I can’t do anything—I’m not a Prettier maintainer.

But if I understand the linked issue correctly, it looks like your use case will be addressed by evilmartians/lefthook#538, so you might try helping there.

If you don’t want to wait for that, you can use the lefthook exclude option to avoid passing unsupported files.

sanmai-NL · 2023-10-09T21:25:45Z

@andersk patching other tools to work-around a breakage you introduced (unwittingly) is at best a stop-gap measure. You could've taken on to fix your work in a new PR. At least one compromise solution has been proposed in this thread.

I'll create a PR to fix your PR. Or perhaps revert it, if that takes undue time.

andersk · 2023-10-09T22:39:09Z

@sanmai-NL The change was not made unwittingly. It is consistent with the way Prettier handles other unsupported files, and was discussed with the maintainer ahove.

I’m sorry that your CI had been unintentionally relying on the previous behavior, and I understand you’re unhappy that it fails after the Prettier 3 upgrade, but such things happen—major releases sometimes intentionally make breaking changes, and the purpose of CI is to fail when things break. Although there is always some value in avoiding breaking changes, here I still believe there was more value gained by eliminating unexpected behavior around symlinks, and throwing an error is better than either silently processing unexpected files or silently skipping them.

fisker · 2023-10-10T00:05:26Z

@sanmai-NL I'm sorry that this change breaks your tool, but please open an new issue to continue discussion. This is an merged PR.

andersk force-pushed the symlinks branch 2 times, most recently from 6cb4b40 to f4bfebf Compare March 29, 2023 23:56

andersk force-pushed the symlinks branch from f4bfebf to 19b3784 Compare March 30, 2023 00:33

Improve test

5954913

andersk commented Mar 30, 2023

View reviewed changes

tests/integration/__tests__/patterns-dirs.js Outdated Show resolved Hide resolved

More tests

b2a9647

fisker added 2 commits March 30, 2023 09:50

One more test

64d5d0f

Typo

72178e1

andersk force-pushed the symlinks branch from 58cdaff to 4343c2e Compare March 30, 2023 02:44