Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undefined behaviour when piping #139

Closed
tegefaulkes opened this issue Feb 25, 2024 · 4 comments · Fixed by #194
Closed

Undefined behaviour when piping #139

tegefaulkes opened this issue Feb 25, 2024 · 4 comments · Fixed by #194
Assignees
Labels
bug Something isn't working r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management

Comments

@tegefaulkes
Copy link
Contributor

Describe the bug

So it's possible to use the pipe operation to input into a polykey command. For example, echo password | npm run polykey -- agent start. This will actually input the password on the first prompt.

// Start an agent normally
brian@matrix-precision-3480-syzygy:~/workspace/Polykey-CLI/ > npm run polykey -- agent start -np ./tmp/test1 

> polykey-cli@0.1.8 polykey
> ts-node src/polykey.ts agent start -np ./tmp/test1

✔ Enter new password … ******** // Password entered manually
✔ Confirm new password … ********
(node:1189978) [DEP0112] DeprecationWarning: Socket.prototype._handle is deprecated
(Use `node --trace-deprecation ...` to show where the warning was created)
pid             1189978
nodeId          v9q4nj2j4sqipj9nlm197fsfofvn61pcvghreml5cjpiv748o6960
clientHost      ::1
clientPort      43369
agentHost       ::
agentPort       41059
recoveryCode    celery grit genre truly weapon tilt cause plug donate forward tackle junior sphere cave sun cloth assist catalog risk bomb staff rate goddess poverty
^C

// With state created we run again while piping in the password
brian@matrix-precision-3480-syzygy:~/workspace/Polykey-CLI/ > echo "password" | npm run polykey -- agent start -np ./tmp/test1

> polykey-cli@0.1.8 polykey
> ts-node src/polykey.ts agent start -np ./tmp/test1

✔ Please enter the password … ******** // Password is provided by the pipe
(node:1190120) [DEP0112] DeprecationWarning: Socket.prototype._handle is deprecated
(Use `node --trace-deprecation ...` to show where the warning was created)
pid             1190120
nodeId          v9q4nj2j4sqipj9nlm197fsfofvn61pcvghreml5cjpiv748o6960
clientHost      ::1
clientPort      46567
agentHost       ::
agentPort       37515
^C

If we try and start Polykey using a pipe and starting from scratch we end up with a promise deadlock.

brian@matrix-precision-3480-syzygy:~/workspace/Polykey-CLI/ > echo "password" | npm run polykey -- agent start -np ./tmp/test3

> polykey-cli@0.1.8 polykey
> ts-node src/polykey.ts agent start -np ./tmp/test3

✔ Enter new password … ********
? Confirm new password › ErrorPolykeyCLIAsynchronousDeadlock

Trying with a newline has the same problem

brian@matrix-precision-3480-syzygy:~/workspace/Polykey-CLI/ > echo "password\npassword" | npm run polykey -- agent start -np ./tmp/test3

> polykey-cli@0.1.8 polykey
> ts-node src/polykey.ts agent start -np ./tmp/test3

✔ Enter new password … ****************

? Confirm new password › ErrorPolykeyCLIAsynchronousDeadlock

My guess is that the pipe closing is causing the problem here.

To Reproduce

  1. start polykey with echo password | agent start from fresh state
  2. polykey shold throw an ErrorPolykeyCLIAsynchronousDeadlock error.

Expected behavior

Hard to say, maybe it should input the first password no problem, but still wait for the confirmation input? Is it also possible to include newlines in the password like this? Do we want to support that? Do we need to sanitise password inputs?

Platform (please complete the following information)

  • Device: precision laptop
  • OS: nixos
  • Version 23

Notify maintainers

@tegefaulkes

@CMCDragonkai
Copy link
Member

Take note of process redirection too.

@tegefaulkes
Copy link
Contributor Author

Looking into this, It seems commander doesn't do any special handling of input pipes. It piped into process.stdin and as far as commander cares, it's business as usual.

That said, It seems that in the case of prompting a new password we end up with a ErrorPolykeyCLIAsynchronousDeadlock likely due to ending the stream before creating any macro tasks resulting in the deadlock. I can do some more digging about that.

Otherwise, the only thing we really need to consider here is the pipe ending while waiting more input. Or really just throwing a more useful error at this stage than the deadlock error. I'll explore some solutions.

Copy link
Contributor Author

The only thing that really needs to be fixed for this is the prompt utility functions.

The problem is that the utility function awaits the promp, but we can end up with the stdin stream ending before awaiting that. It seems that the promp library doesn't handle this properly? resulting in us awaiting a promise that will not resolve, we haven't started anything so there are no macrotasks that hold the process open resulting in the promise deadlock.

To fix this, wherever we await the prompt call, we need to race this with a promise that resolves when the stream ends. This will prevent the deadlock and result in a `ErrorPolykeyCLIPasswordMissing` error as if the password wasn't entered properly.

@CMCDragonkai
Copy link
Member

Sometimes commands expect the usage of - to force the usage of stdin. Alternatively it should be possible to do <(echo pass) and pass it as a CMD parameter.

Yea if it's expecting to read from prompt then usually piping in from stdin should work too.

@CMCDragonkai CMCDragonkai added the r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management label Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management
Development

Successfully merging a pull request may close this issue.

2 participants