Feature cli polish

[addievo]

Description

This PR aims to fix a lot of UI/UX issues of Polykey CLI to improve clarity and security of command outputs.

The main issues to be fixed include:

1. Fixes nodes getall:

This previous threw an error related to undefined host, this has now been resolved in the corresonding Polykey Handler by getting the valid address and port from the info field which is of type NodeData

          host: info.address.host,
          port: info.address.port,

2. Improve nodes connections Output:

Previously, the output was rather unclear and lacked corresponding labels to relevant labels for Node Host, UsageCount and Timeout, now labels are added for the same.

Previously we used a hardcoded amount of tabbed spaces between columns, this has now been changed to be padded based on length of row with the max length.

// Old Code
const outputLine = `${hostString}\t${usageCount}\t${timeout}`;

// New Code
const outputLine = hostString.padEnd(maxHostStringLength) + "\t" + usageCount.padEnd(maxUsageCountLength) + "\t" + timeout.padEnd(maxTimeoutLength);

3. Sanitizing JSON formatted output for nodes connections:

This was a critical security issue where in the sensitive information of authentication data (although this is the user's authentication data itself) was being included in the JSON output, this has been sanitized out.

// Implementation Snippet
const sanitizedConnections = JSON.parse(JSON.stringify(connections));
delete connection.metadata;

4. Refactor agent status output:

JWK and PEM have very little usage in status, and correspondingly are being removed from agent status, this is a very simple fix of just removing the following keys from the output

// Old Code
data: { ... , publicKeyJWK: response.publicKeyJwk, certChainPEM: response.certChainPEM }

// New Code
data: { ... , /* publicKeyJWK and certChainPEM removed */ }

5. Additional scope added to this PR - Standardisation of table layout for outputFormatter :

Table output in outputFormatter was intiially very static and did not take into account variety in terms of padding, this is altered now to allow field for headers, and a boolean to toggle numbering of rows.

Table format now includes a standardised TSV padding.

Table format handles \t and \n by enclosing them in quotation marks to prevent disruption of the layout.

Additional supported is added for stream based padding to improve efficiency to O(2n).

Issues Fixed

• Related CLI polish and fixes for deployment #44

Tasks

• 1. nodes connection human format label.
• 2. nodes connection json format remove metadata (sanitize).
• 3. Remove JWK and PEM from agent status.
• 4. Add a jest for nodes connection.
• 5. Update jests which rely on agent status.

Final checklist

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

[ghost]

👇 Click on the image for a new way to code review

Legend

[addievo]

Sanitization isn't a good solution for num 2.

This issue is related to this issue:

MatrixAI/Polykey#572

[addievo]

Console output from handler for number 2:

{
  nodeId: IdInternal(32) [Uint8Array] [
    211, 206, 216, 221, 137,  66, 215, 114,
      8, 228,  98, 250, 193, 250, 150,  56,
    210,  34,  94, 186,  71, 180, 100, 130,
    229, 191, 254,  19,  45,  18, 227, 171
  ],
  address: { host: '127.0.0.1', port: 55551, hostname: undefined },
  usageCount: 0,
  timeout: undefined
}

So it is more than obvious that this JSON:

[{"host":"127.0.0.1","hostname":"","nodeIdEncoded":"vqf7dhnc98bbn4274cbtc3ukm73924nlq8uq690n5nvv16b8iselg","port":55551,"timeout":-1,"usageCount":0,"metadata":{"timeout":15000,"authorization":"Bearer {\"payload\":\"eyJpYXQiOjE2OTgyMTY4NTguNzA1LCJpc3MiOiJ2cTQ4MG9mbGQ3dnJvbDB1cmExdmlpZjk0NzRwYjdwdHJxbTlzOGxkMjN2bjV0OTJvaWY0ZyIsInN1YiI6InZxNDgwb2ZsZDd2cm9sMHVyYTF2aWlmOTQ3NHBiN3B0cnFtOXM4bGQyM3ZuNXQ5Mm9pZjRnIn0\",\"signatures\":[{\"protected\":\"eyJhbGciOiJCTEFLRTJiIn0\",\"signature\":\"UbIRKl5a8AWuNIAK2aCRmgatv0e940ZNZJ2Zvzi_WF4\"}]}"}}]

The metadata field isn't from the backend, consequently the issue lies in the frontend.

[addievo]

Considering the issue arises in the frontend itself, I think a sensitization should suffice for number 2.

[CMCDragonkai]

Console output from handler for number 2:

{
  nodeId: IdInternal(32) [Uint8Array] [
    211, 206, 216, 221, 137,  66, 215, 114,
      8, 228,  98, 250, 193, 250, 150,  56,
    210,  34,  94, 186,  71, 180, 100, 130,
    229, 191, 254,  19,  45,  18, 227, 171
  ],
  address: { host: '127.0.0.1', port: 55551, hostname: undefined },
  usageCount: 0,
  timeout: undefined
}

So it is more than obvious that this JSON:

[{"host":"127.0.0.1","hostname":"","nodeIdEncoded":"vqf7dhnc98bbn4274cbtc3ukm73924nlq8uq690n5nvv16b8iselg","port":55551,"timeout":-1,"usageCount":0,"metadata":{"timeout":15000,"authorization":"Bearer {\"payload\":\"eyJpYXQiOjE2OTgyMTY4NTguNzA1LCJpc3MiOiJ2cTQ4MG9mbGQ3dnJvbDB1cmExdmlpZjk0NzRwYjdwdHJxbTlzOGxkMjN2bjV0OTJvaWY0ZyIsInN1YiI6InZxNDgwb2ZsZDd2cm9sMHVyYTF2aWlmOTQ3NHBiN3B0cnFtOXM4bGQyM3ZuNXQ5Mm9pZjRnIn0\",\"signatures\":[{\"protected\":\"eyJhbGciOiJCTEFLRTJiIn0\",\"signature\":\"UbIRKl5a8AWuNIAK2aCRmgatv0e940ZNZJ2Zvzi_WF4\"}]}"}}]

The metadata field isn't from the backend, consequently the issue lies in the frontend.

Where did you do this?

[CMCDragonkai]

Tasks should be not just fix 1, 2, 3. Summarise the titles.

[addievo]

Console output from handler for number 2:

{
  nodeId: IdInternal(32) [Uint8Array] [
    211, 206, 216, 221, 137,  66, 215, 114,
      8, 228,  98, 250, 193, 250, 150,  56,
    210,  34,  94, 186,  71, 180, 100, 130,
    229, 191, 254,  19,  45,  18, 227, 171
  ],
  address: { host: '127.0.0.1', port: 55551, hostname: undefined },
  usageCount: 0,
  timeout: undefined
}

So it is more than obvious that this JSON:

[{"host":"127.0.0.1","hostname":"","nodeIdEncoded":"vqf7dhnc98bbn4274cbtc3ukm73924nlq8uq690n5nvv16b8iselg","port":55551,"timeout":-1,"usageCount":0,"metadata":{"timeout":15000,"authorization":"Bearer {\"payload\":\"eyJpYXQiOjE2OTgyMTY4NTguNzA1LCJpc3MiOiJ2cTQ4MG9mbGQ3dnJvbDB1cmExdmlpZjk0NzRwYjdwdHJxbTlzOGxkMjN2bjV0OTJvaWY0ZyIsInN1YiI6InZxNDgwb2ZsZDd2cm9sMHVyYTF2aWlmOTQ3NHBiN3B0cnFtOXM4bGQyM3ZuNXQ5Mm9pZjRnIn0\",\"signatures\":[{\"protected\":\"eyJhbGciOiJCTEFLRTJiIn0\",\"signature\":\"UbIRKl5a8AWuNIAK2aCRmgatv0e940ZNZJ2Zvzi_WF4\"}]}"}}]

The metadata field isn't from the backend, consequently the issue lies in the frontend.

Where did you do this?

@CMCDragonkai, console.log'ed the handler and ran PK with --verbose to see the output

Further clarifying on this

Handler

...
  ): AsyncGenerator<ClientRPCResponseResult<NodeConnectionMessage>> {
    const { nodeConnectionManager } = this.container;
    const connections = nodeConnectionManager.listConnections();
    for (const connection of connections) {
      if (ctx.signal.aborted) throw ctx.signal.reason;
      console.log('THIS IS THE OUTPUT FROM THE HANDLER IN PK');
      console.log(connection);
      yield {
        host: connection.address.host,
        hostname: connection.address.hostname ?? '',
        nodeIdEncoded: nodesUtils.encodeNodeId(connection.nodeId),
        port: connection.address.port,
        timeout: connection.timeout ?? -1,
        usageCount: connection.usageCount,
      };
...

Output in CLI

DEBUG:polykey.PolykeyAgent.ClientService.WebSocketServer.WebSocketConnection 0.WebSocketStream 0:458 bytes have been pushed onto stream buffer
INFO:polykey.PolykeyAgent.ClientService.RPCServer:Handling stream with method (nodesListConnections)
THIS IS THE OUTPUT FROM THE HANDLER IN PK
{
  nodeId: IdInternal(32) [Uint8Array] [
    211, 206, 216, 221, 137,  66, 215, 114,
      8, 228,  98, 250, 193, 250, 150,  56,
    210,  34,  94, 186,  71, 180, 100, 130,
    229, 191, 254,  19,  45,  18, 227, 171
  ],
  address: { host: '127.0.0.1', port: 55551, hostname: undefined },
  usageCount: 0,
  timeout: undefined
}
DEBUG:polykey.PolykeyAgent.ClientService.WebSocketServer.WebSocketConnection 0.WebSocketStream 0:575 bytes need to be written into a receiver buffer of 1048576 bytes

[CMCDragonkai]

What's the status on this PR?

[addievo]

What's the status on this PR?

Now it is ready to review

[tegefaulkes]

Is nodesConnection is is sometimes lising the IP as as a ipv6 mapped ipv4 address. This needs to be looked into. addressed from the issue?

[addievo]

Is nodesConnection is is sometimes lising the IP as as a ipv6 mapped ipv4 address. This needs to be looked into. addressed from the issue?

No, this issue does not address that. I dont think that has to do with Polykey-CLI, and is rather a Polykey related issue, no?

[CMCDragonkai]

That should be looked into the ipv4-mapped-ipv6 addresses.

[CMCDragonkai]

Can you rewrite the spec, I think some of the spec here is just plain incorrect.

[CMCDragonkai]

Not ready to be merged. Spec does not match implementation details.

[addievo]

As
#44

has been bifurcated into a new issue in Polykey, this PR is ready to be reviewed and merged.

I am doing that now.

[CMCDragonkai]

Go to the output formatter issue and solve things there first then review this in accordance.

[addievo]

Go to the output formatter issue and solve things there first then review this in accordance.

Oh okay

[addievo]

@CMCDragonkai,

So this is what I got from #22

1. TSV standardization : Output should be tab seperated values.
2. Handling special characters: Tabs and newlines should be wrapped in quotes.
3. Padding: Make a single pass thru data, and for streams, use a max len and then alter according to data being streamed.

Along with that, I also need to add a optional header row, and a row count.

[addievo]

@CMCDragonkai the issue with stream based padding, i.e. updating counter for every item in stream is that, that would have to be implemented on a case-to-case basis, since the data is fed into output formatter after it has been all collected and added to an array.

[CMCDragonkai]

@CMCDragonkai the issue with stream based padding, i.e. updating counter for every item in stream is that, that would have to be implemented on a case-to-case basis, since the data is fed into output formatter after it has been all collected and added to an array.

No it can be done in the output formatter. Do a scan.

[tegefaulkes]

I see we strip out the "" on strings when formatting the output. Is that something we really want? How would I tell if 1235 is different from "12345" or if the whitespace was part of the value?

[CMCDragonkai]

I see we strip out the "" on strings when formatting the output. Is that something we really want? How would I tell if 1235 is different from "12345" or if the whitespace was part of the value?

You cannot. We are making a UI decision here. In fact we do space padding anyway in tables to make this work.

Actually now thinking about it, however for a "password " to work. I imagine you'd want to show the spaces. That's a good question... and perhaps in that case we would want to show the whitespace different from padded.

One issue is then some passwords are password \n\n. Right? So in that case, we cannot encode it with \n at all.

In fact in such cases, it's not really useful at all to simply print it with padded whitespace or otherwise.

Maybe then we should keep the double quote....

Yes let's keep the double quotes, they are the only way to know. But I can imagine an issue with things like \t or \v and other whitespace characters.

I think generally though if we are printing out a secret data, we would print it out literally. For example: singular outputs, list outputs. I believe singular outputs and list outputs is currently the same. I now realise they should be considered separate output formats.

The double quotations only come into play in the more complex outputs... That would be for list, dict, table.

[addievo]

I see we strip out the "" on strings when formatting the output. Is that something we really want? How would I tell if 1235 is different from "12345" or if the whitespace was part of the value?

You cannot. We are making a UI decision here. In fact we do space padding anyway in tables to make this work.

Actually now thinking about it, however for a "password " to work. I imagine you'd want to show the spaces. That's a good question... and perhaps in that case we would want to show the whitespace different from padded.

One issue is then some passwords are password \n\n. Right? So in that case, we cannot encode it with \n at all.

In fact in such cases, it's not really useful at all to simply print it with padded whitespace or otherwise.

Maybe then we should keep the double quote....

Yes let's keep the double quotes, they are the only way to know. But I can imagine an issue with things like \t or \v and other whitespace characters.

I think generally though if we are printing out a secret data, we would print it out literally. For example: singular outputs, list outputs. I believe singular outputs and list outputs is currently the same. I now realise they should be considered separate output formats.

The double quotations only come into play in the more complex outputs... That would be for list, dict, table.

I have never seen a password with any whitespace character in between.t

[CMCDragonkai]

I have passwords with whitepsaces.

[addievo]

Apparently Polykey includes spaces typed while setting a password, which I think is a big UI/UX flaw.

https://security.stackexchange.com/questions/17192/why-disallow-special-characters-in-a-password

[addievo]

Also, why would we ever want to print out the password? Isn't that also a big security flaw? Printing our plaintext passwords

[CMCDragonkai]

It's not just passwords. There are other kinds of text too. I'm thinking secrets in the vaults. I think you need to add the quotes back. And then special encode any non printable characters except spaces.

[addievo]

#50