[Mukhtar003]

Summary

• Replace GNU-only tar flags and human-readable listing parsing with the Node tar library.
• Validate archive paths and reject links/special files, extract into a private staging directory, then atomically replace regular files with rollback if an import fails.
• Preserve existing directory permissions, support symlinked target ancestors, and add real-archive regression tests.
• Regenerate the pnpm lockfile and add a patch changeset for @caatinga/cli.

Closes #221

Verification

• pnpm install --frozen-lockfile --ignore-scripts (pnpm 9.15.4): passed.
• pnpm --filter @caatinga/cli test: passed (41 files, 305 tests).
• pnpm typecheck: passed.
• pnpm test: on this macOS machine, three existing @caatinga/zk tests fail because they hardcode Linux circom-linux-amd64 paths. The CLI suite passes locally.

[drips-wave]

@Mukhtar003 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

[Dione-b]

Thanks for tightening the import path. I went through identity.command.ts on the PR branch, and two points are worth addressing before merge. A third is about test coverage.

1. TOCTOU between lstat and copyFile (identity.command.ts:107-116)

mergeStagedFiles checks the target with lstat and then calls copyFile, which follows symlinks. If the target is swapped for a symlink between the two calls, the secret key material is written through it to an arbitrary location. That undermines the hardening this PR is adding.

The merge is also not transactional. Files are copied and chmoded one by one (:116-117). If a later entry fails (for example the non-file conflict at :103-109), the earlier files are already overwritten and nothing is rolled back.

The exposure is limited when the target is the user's own ~/.config/stellar, since an attacker would already need write access there. It matters more for CI or for --path pointing at a shared directory. Possible fixes: copy to a temp name in the target directory and rename it into place, or open with O_NOFOLLOW/O_EXCL and write through the file descriptor.

2. Symlinked ancestors are rejected (identity.command.ts:71-90)

ensureSafeDirectory runs lstat on every path segment from the root and throws if any of them is a symlink. A legitimate target under a symlinked ancestor therefore fails with "Refusing to import into non-directory or symbolic link". Examples are macOS $TMPDIR (/var -> /private/var), a ~/.config symlinked into a dotfiles repo, and CI workspaces mounted through a symlink.

The check should apply only to the final component, or run on realpath(parent).

3. Tests for the validation path are mostly mocked

identity.command.test.ts:10 mocks tar entirely, so validateArchiveEntry and the staging/merge logic are never exercised against real archives there. identity.roundtrip.test.ts uses the real tar, but it has a single happy-path test.

I'd suggest adding real-archive tests for at least: a ../ entry, an absolute path, a symlink entry, a hardlink entry, and the ./ entry that tar -C dir . emits. These are the regressions this change is meant to prevent.

(Note: onReadEntry is available in tar@7.4.3, so the ^7.4.3 range is fine and no bump is needed.)

[Dione-b]

lstat followed by copyFile is a check-then-act race, and copyFile follows symlinks. A swap between the two calls would write the key material outside the target. Consider writing to a temp file in the target directory and rename-ing it into place, or opening with O_NOFOLLOW. Also, a failure on a later entry leaves earlier files overwritten, so it may be worth staging the whole merge or documenting the partial-import behavior.

[Dione-b]

This rejects a symlink at any ancestor, not just the final directory, so legitimate paths like macOS $TMPDIR or a symlinked ~/.config fail. Could we apply the check only to the last component, or resolve the parent with realpath first?

[Dione-b]

With tar mocked here, the archive validation is not actually exercised. A few real-archive cases (../, absolute path, symlink, hardlink, ./) in the round-trip suite would cover the traversal and link rejection this PR adds.

[Dione-b]

Following up on my earlier review: those three points (TOCTOU between lstat and copyFile, rejection of symlinked ancestors, and tests that mock tar) are still open on 64e241e. I checked the head locally and found a few more things:

1. The lockfile breaks CI installs (blocking). pnpm-lock.yaml adds the yallist@5.0.0 snapshot (a tar@7 dependency) but no matching yallist@5.0.0 entry under packages:. With pnpm 9.15.4, pnpm install --frozen-lockfile (what CI runs) crashes with Cannot use 'in' operator to search for 'directory' in undefined. Regenerate the lockfile with pnpm install on pnpm 9.15.4 (the packageManager version) and commit the result.

2. chmod 0700 on an existing target. ensureSafeDirectory and mergeStagedFiles run chmod(…, 0o700) on the target even when it already exists, which silently changes the permissions of the user's own config directory. Only apply the mode to directories this command creates.

3. Missing changeset. Please add a .changeset/*.md with a patch bump for @caatinga/cli.

4. Tests. Please add the real-archive cases to identity.roundtrip.test.ts (../, absolute path, symlink, hardlink, ./, symlinked target ancestor), and say in the PR body that pnpm test and pnpm typecheck pass locally.

Once the lockfile is fixed I'll approve the CI run. Thanks!

[Mukhtar003]

@Dione-b I addressed the requested identity import fixes: the lockfile installs with pnpm 9.15.4, destination updates use staged atomic replacements with rollback, existing directory permissions are preserved, symlinked target ancestors work, and real archive tests cover traversal, absolute paths, symlinks, hardlinks, and dot-directory entries. The full CLI suite (305 tests) and pnpm typecheck pass. Could you review the update? The full workspace test command has three unrelated macOS failures in existing @caatinga/zk tests that hardcode Linux circom paths; I documented that in the PR.

[Dione-b]

Thanks @Mukhtar003, this addresses everything from both reviews. I merged 9ec3c66 onto current main locally (including #303, which touched the same file). The merge was clean, and these all pass:

• pnpm install --frozen-lockfile with pnpm 9.15.4;
• build, typecheck, lint and Prettier;
• the full test suite (cli 320, core 658, client 185, zk 100), including the 8 real-archive round-trip tests.

What I checked:

• Lockfile: yallist@5.0.0 now has its packages: entry, so frozen installs work.
• TOCTOU / partial imports:
  • Every destination is pre-flighted before any write.
  • Files are copied to an exclusive temp file beside the destination and installed with rename, which doesn't follow a final symlink.
  • On failure, backups are restored and created directories removed.
• Symlinked ancestors: only the final component is rejected; realpath resolves the rest. This is covered by a test.
• Permissions: existing directory modes are preserved. New files are written as 0600, and directories the import creates get 0700.
• Archive validation: ../, absolute paths, symlinks, hardlinks and the ./ entry are all tested without mocking tar.

Two residual edge cases, neither blocking:

• If a subdirectory inside the target is swapped for a symlink between pre-flight and copy, the copy follows it. That requires write access to the config dir itself.
• A crash mid-import can leave .caatinga-identity-*.tmp / .bak files behind.

Approving. I'll merge once CI is green.