Skip to content

Conversation

@CDeltakai
Copy link
Contributor

@CDeltakai CDeltakai commented Jun 4, 2025

Description

This PR addresses a bug in the polykey secrets rename command where providing as a fully qualified path (e.g., MyVault:/NewSecret) would cause an ENOENT error. This occurred because the underlying rename operation expects a simple base name for the new secret, not a full path.

Issues Fixed

Tasks

  • 1. Change the CommandRename.ts script in src/secrets to allow for both simple base names someSecret and fully qualified paths vaultName:/someSecret

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CDeltakai CDeltakai self-assigned this Jun 4, 2025
Copy link
Contributor

@aryanjassal aryanjassal left a comment

Choose a reason for hiding this comment

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

The logic intuition is correct, but the implementation does not align with how it has been done in rest of the codebase. So, the implementation needs a bit of refactoring before it is ready for merging.

@aryanjassal
Copy link
Contributor

As matthew should focus on MatrixAI/Polykey#822, I will instead take this over and complete it.

@aryanjassal aryanjassal self-assigned this Jun 11, 2025
@aryanjassal
Copy link
Contributor

Another thing I've observed is that logic preventing invalid actions was written to polykey CLI. This is incorrect, and all such logical interactions should be present in Polykey itself. I'm pretty sure any illegal interactions like attempting to rename a vault root is intercepted by the filesystem, and an EPERM is thrown.

Okay maybe not an EPERM but this is what I get when I attempt to rename vault root -- it is handled automatically. The error is not easy to understand, but invalid actions are protected against, and that's what matters for now.

-> polykey secrets rename vault:/ hi                                                           
ErrorPolykeyRemote: Remote error from RPC call
  localHost	::1
  localPort	35154
  remoteHost	::1
  remotePort	42609
  command	vaultsSecretsRename
  timestamp	Wed Jun 11 2025 18:01:40 GMT+1000 (Australian Eastern Standard Time)
  cause: ErrorPolykeyUnexpected: An error originating outside Polykey was thrown - Unexpected error occurred: ErrorEncryptedFSError
    cause: {"type":"ErrorEncryptedFSError","description":"resource busy or locked","data":{"message":"EBUSY: resource busy or locked, / -> hi","timestamp":"2025-06-11T08:01:38.821Z","data":{},"stack":"ErrorEncryptedFSError: EBUSY: resource busy or locked, / -> hi\n    at file:///nix/store/bkqcawsmimlham4mwnr7pp2fdy6rcias-polykey-cli-0.18.17/lib/node_modules/polykey-cli/dist/polykey.mjs:111057:23\n    at async withF (file:///nix/store/bkqcawsmimlham4mwnr7pp2fdy6rcias-polykey-cli-0.18.17/lib/node_modules/polykey-cli/dist/polykey.mjs:488:12)\n    at async constructor_.withTransactionF (file:///nix/store/bkqcawsmimlham4mwnr7pp2fdy6rcias-polykey-cli-0.18.17/lib/node_modules/polykey-cli/dist/polykey.mjs:104708:16)\n    at async file:///nix/store/bkqcawsmimlham4mwnr7pp2fdy6rcias-polykey-cli-0.18.17/lib/node_modules/polykey-cli/dist/polykey.mjs:111034:11\n    at async maybeCallback (file:///nix/store/bkqcawsmimlham4mwnr7pp2fdy6rcias-polykey-cli-0.18.17/lib/node_modules/polykey-cli/dist/polykey.mjs:104488:12)\n    at async file:///nix/store/bkqcawsmimlham4mwnr7pp2fdy6rcias-polykey-cli-0.18.17/lib/node_modules/polykey-cli/dist/polykey.mjs:166256:5\n    at async file:///nix/store/bkqcawsmimlham4mwnr7pp2fdy6rcias-polykey-cli-0.18.17/lib/node_modules/polykey-cli/dist/polykey.mjs:129230:13\n    at async withF (file:///nix/store/bkqcawsmimlham4mwnr7pp2fdy6rcias-polykey-cli-0.18.17/lib/node_modules/polykey-cli/dist/polykey.mjs:488:12)","_errno":10,"_code":"EBUSY","_description":"resource busy or locked","_syscall":"rename"}}

@aryanjassal aryanjassal force-pushed the feature-make-rename-accept-vault-path-for-both-params branch from 5e52833 to 8d9e54b Compare June 12, 2025 00:45
@aryanjassal
Copy link
Contributor

This should now be implemented correctly to take in a fully-qualified path for both the source and destination parameters. This can now be merged.

@aryanjassal aryanjassal merged commit bf8e1c5 into staging Jun 12, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Secret Renaming 2nd Parameter Does not Recognize Fully Qualified Paths

3 participants