Some `secrets` commands throw an `undefined` error

[aryanjassal]

Describe the bug

When writing to a directory, the command should provide appropriate feedback bail out. This should either be done immediately (by preemptively checking if the target is a valid file path or not), or later, by raising an appropriate error.

Currently, it just returns this output, which is not really explicit or apparent in what went wrong:

[aryanj@matrix-34xx:~]$ pk secrets write vault:dir
writing to dir <Ctrl-D>
undefined

[aryanj@matrix-34xx:~]$ echo $?
255

To Reproduce

1. Try writing to a directory using stdin
2. Observe issue

Expected behavior

As in secrets cat, an error could be raised:

[aryanj@matrix-34xx:~]$ pk secrets cat vault:dir     
ErrorSecretsIsDirectory: dir: Is a directory

Screenshots

Platform

• Device: Dell Precision 3480
• OS: NixOS
• Version: ["0.10.0","1.14.0","1","1"]

Additional context

Notify maintainers

@aryanjassal

[linear[bot]]

ENG-434 `secrets write` doesn't gracefully handle writing to a directory

[aryanjassal]

A similar behaviour is seen when trying to remove the vault root using secrets rm. It just prints undefined and fails. Doing this should yield a proper error message.

[CMCDragonkai]

I find that occurrences like this indicates a significant lack of quality control and specification review. You need go back to the drawing board and review the entire behaviour and isolate the tests and fastcheck the commands.

[aryanjassal]

Brian and I investigated this, and we have found the culprit for this issue.

The root cause is that EncryptedFSErrors are not deserialised properly from Polykey, as it is not an instance of any ErrorPolykey error. As such, the error itself gets serialised as JSON instead of an error.

This is then reconstructed incorrectly in the client side in Polykey CLI. The output formatter expects an object of type Error, but it gets a malformed JSON object instead.

This happens because the type of the error when we pass it into the output formatter is any, which bypasses any error handling. This results in the error being identified and printed as an error, resulting in the error code being 1.

When deserialising the error, the formatter appends error.name to the output, which is undefined. Thus, we get undefined as output in the error with no additional information.

This needs twofold action. First, all operations in VaultOps domain interacting with the efs must explicitly catch all the errors and wrap them in a Polykey error type. Second, Polykey CLI must be updated to wrap any unexpected error in a separate error type. For example, if an error is detected with a type different to Polykey's errors, then instead of assuming that they will be of type Error, wrap the object in a new error, say, ErrorPolykeyCLIUnexpectedError with the data as the serialised object we got. This would let the developer know that the error they are getting is malformed. It also returns the error type so they know exactly what error it is and why it could be malformed.

In the future, any operations which could raise errors must be explicitly managed and wrapped for the errors to be properly propagated in Polykey.

[aryanjassal]

async function mkdir(...) {
  try {
    // Throws an `EncryptedFSError`
  } catch (e) {
    if (e.code === 'EEXIST') {
      // Wrap the error
    }
    throw e;
    // Unwrapped error can potentially escape
  }
}

This code can still technically throw an unwrapped error, which can propagate through the RPC and become an undefined error. While I am working on adding more details to any undefined errors, this might also need to be quelled from here.

However, chances are, in this function, it can return only throw the errors that we are explicitly catching, so it's fine here, but how would this be handled in other vaultOps functions?

@tegefaulkes

[CMCDragonkai]

Why not have a generic catch all? What do you mean unwrapped?

[aryanjassal]

Why not have a generic catch all? What do you mean unwrapped?

When we catch exceptions, we check if the exception is the one we expect. In that case, the relevant exception is caught, wrapped in, say, ErrorSecretsSecretUndefined (or any relevant error), then re-thrown. Otherwise, the error is thrown as-is. By unwrapped error, I mean an error which would not be manually wrapped.

This comment was made before our discussion of dealing with unexpected errors at the root, from within fromError itself. In this case, this is no longer relevant, as all errors will be wrapped automatically. If we encounter an unexpected error, it will be wrapped accordingly. Otherwise, everything works as normal.