Vault names can have `.` character when creating them, but they aren't valid paths for secrets commands

[CMCDragonkai]

Describe the bug

cmcdragonkai ➜ matrix-ml-1  ➜ ~/Projects/Polykey-CLI
 $ ./dist/polykey.js secrets edit 'polykey.com:/POLYKEY_COM_GITHUB_TOKEN'
error: command-argument value 'polykey.com:/POLYKEY_COM_GITHUB_TOKEN' is invalid for argument 'secretPath'. polykey.com:/POLYKEY_COM_GITHUB_TOKEN is not of the format <vaultName>:<directoryPath>

Notice that polykey.com is a valid vault name, but it cannot be referred to when editing a secret path.

The regex must be aligned between the 2. Also new fast check inputs need to be checked to ensure that all usages of "secret path" or "vault name" are consistent.

To Reproduce

1. Create polykey.com vault
2. Try to edit a file inside it like polykey.com:/somesecret

Expected behavior

Given that we expect that : to be the separation, pretty much any alphanum plus basic symbols.

The question is what symbols should be allowed for vault names. We should allow most characters on standard keyboards, and even utf-8 characters. Actually why not allow anything except :, since that's the distinguishing character.

Notify maintainers

@tegefaulkes @aryanjassal

[linear[bot]]

ENG-379 Vault names can have `.` character when creating them, but they aren't value paths for secrets commands

[CMCDragonkai]

Consistent secret paths and vault names needs to be specified and tested consistently across all commands in #32.

[tegefaulkes]

This may tie into #223 but otherwise should be an easy issue for @aryanjassal to take on.

[aryanjassal]

https://github.com/MatrixAI/Polykey-CLI/blob/755e97f0ab6346c04c373adf8c0550da5ce71afb/src/vaults/CommandCreate.ts#L14

This line is responsible for allowing vault creation, as there are no checks for forbidden characters. As such, it isn't a matter of unifying the regex for both vault path during creation or otherwise, but to add a new parser during vault creation.

One solution is to just make a new parser which will use regex to parse the vault name, and then the same regex can be used in other parsers/commands to validate a vault name. However, we already have too many parsers, most of which do things slightly differently than the other. (https://github.com/MatrixAI/Polykey-CLI/pull/255#discussion_r1731175246). We need a way to unify (or simplify) the parsers, otherwise our codebase will become polluted with parsers which are basically the same but have slight differences.

The simplest solution would be to always return vault name, and if it exists, the secret path, and let the user deal with parsing errors.

Another solution could be to write parser "modules". One of the module would look for the vault name, and another would look for the secret path, and we can somehow chain them together to make more complex parsers, but I'm not sure if it even is possible, or how to approach this.

I will do research on both these solutions.

[CMCDragonkai]

We already have a constraint parser issue related to this. And please read the input validation in our issues in the Matrix AI graph which explains the structure of our input processing functions. Don't reinvent the wheel yet.

[aryanjassal]

https://github.com/MatrixAI/Polykey-CLI/pull/255#discussion_r1732089125

It is possible to compose a parser. I'm including this here for completeness.

Don't reinvent the wheel yet.

Then, do I make another parser which will run parsing during vault creation, and then align that parser with the regex found in other commands?

[CMCDragonkai]

Our parsers are already structural. The basic idea is that their exception structure is consistent. They also sort of produce the remainder of whatever and that allows other parsers to continue working. Some low level parsers will parse the entire input and produce exactly the thing you want.

223

[tegefaulkes]

There was a little bit of work done in https://github.com/MatrixAI/Polykey-CLI/pull/255 by @aryanjassal. He added a small amount of code which I'm going to remove from that PR for now.

// src/utils/parsers.ts:69
function parseVaultName(vaultName: string): string {
  // E.g. If 'vault1, 'vault1' is returned
  //      If 'vault1:a/b/c', an error is thrown
  if (!vaultNameRegex.test(vaultName)) {
    throw new commander.InvalidArgumentError(
      `${vaultName} is not of the format <vaultName>`,
    );
  }
  // Returns match[1], or the parsed vaultName
  return vaultName.match(secretPathNameRegex)![1];
}

[aryanjassal]

This issue is basically done in Polykey-CLI#312, but UTF8 characters are still not allowed into vault names.

This will be worked upon in Polykey-CLI#305 which also implements other regex changes for parsers.

[tegefaulkes]

Is this addressed now? @aryanjassal

[aryanjassal]

Yes. I will go through and close all the issues addressed by Polykey-CLI#305