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(core): use current user when hashing native file & enable setting its directory via env #24326

Merged
merged 3 commits into from
May 24, 2024

Conversation

MaxKless
Copy link
Collaborator

@MaxKless MaxKless commented May 22, 2024

Current Behavior

The native file hashing takes only the nx version & workspace path into account.
There is also no way to control the storage location of native .node files used by nx.

Expected Behavior

The native file hashing takes the nx version, workspace path & current username into account, making sure the cache isn't shared between multiple users on the same device.
It's also possible to control the exact location of the native file cache by setting the NX_NATIVE_FILE_CACHE_DIRECTORY environment variable if you need to move it somewhere else.

Related Issue(s)

Fixes #

@MaxKless MaxKless requested review from a team as code owners May 22, 2024 10:22
@MaxKless MaxKless requested review from Cammisuli and isaacplmann May 22, 2024 10:22
Copy link

vercel bot commented May 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview May 23, 2024 6:15pm

@MaxKless MaxKless requested a review from FrozenPandaz May 22, 2024 13:59
.update(userInfo().username)
.digest('hex')
.substring(0, 7);
return join(tmpdir(), `nx-native-file-cache-${shortUserHash}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a shortUserHash here as well as in the hash for the child directory?

right now we have...

/tmp/[F(username)]/[F(username, workspaceRoot]

We should be able to have..

/tmp/[F(username, workspaceRoot)] right?

@MaxKless MaxKless requested a review from FrozenPandaz May 24, 2024 06:21
@MaxKless MaxKless enabled auto-merge (squash) May 24, 2024 06:22
@MaxKless MaxKless merged commit 10f97b9 into master May 24, 2024
6 checks passed
@MaxKless MaxKless deleted the native-cache-tweaks branch May 24, 2024 13:10
FrozenPandaz pushed a commit that referenced this pull request May 24, 2024
… its directory via env (#24326)

(cherry picked from commit 10f97b9)
@Ten0
Copy link

Ten0 commented May 24, 2024

because the hash is deterministic and tmp is world-writable and regularly cleaned up, if the client assumes that the node binary is correct, isn't that a security issue?
Another user could place a trojaned binary in the path that this user would use as a cache hit, no?
(This is the attack scenario I described at #23904 (comment))

@MaxKless
Copy link
Collaborator Author

MaxKless commented May 25, 2024

if you're worried about other users on your system, you can set the location to a folder only you have access to via the new environment variable

@Ten0
Copy link

Ten0 commented May 25, 2024

It looks like alternately this could be stored in ~/.cache.

I have this as a transitive dependency that I didn't particularly ask for.

Expecting from all users that they realize that they have this dependency, inspect all of its inner workings to realize there is a security issue, and finally set an env variable to mitigate it does not seem like a sound way to handle security issues.

@MaxKless
Copy link
Collaborator Author

That's a valid point. I'm hesitant to set the directory to something user-scoped because of potentially different permission handling across OS. My biggest priority is avoiding errors when using Nx because copying that file is required and if there is a permission issue, we could cause errors for everyone - not just those with hypothetical malicious other users on the same machine.
For now the environment variable is the way to go but I see your point and will reconsider this.

Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants