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

[minor] added --print-reason cli argument for ws close reason #109

Closed
wants to merge 1 commit into from

Conversation

towc
Copy link
Contributor

@towc towc commented Oct 31, 2019

intended to solve #108

functionality

when using -r or --print-reason, when the ws closes and the server sends a reason string, the console will exit with Disconnected (code: 4001, reason: some reason) instead of the current Disconnected (code: 4001).

This is opt-in, to keep the change minor.

display considerations

Maybe we want to give even more space to the reason, and bring it in its own line, without wrapping it in brackets, as the server might send a multi-line reason

implementation considerations

I've implemented this as a method of wsConsole to avoid code duplication, but it doesn't feel like it would belong there. Having a helper function to do this would need to have wsConsole passed to it, as the disconnect message is sent in two places that reference different wsConsoles. I tend to avoid f(obj, ..params) when almost everywhere obj.f(...params) is used. I'll leave this up for discussion.

security considerations

Currently reason is simply injected into the string to print, it's not sanitized by me. At best this means the server can change colors or ring bells in your terminal when the ws closes, at worst it could open the user up to remote code execution, depending on their terminal.

Maybe this is fine, as wscat is mostly a debugging tool for APIs that you don't tend to consider malign. If it's not, I don't have in mind a best strategy to sanitize for all terminals other than only keeping ascii printable characters, or JSON.stringifying the whole string

@lpinca
Copy link
Member

lpinca commented Nov 16, 2019

I would print it unconditionally without adding a new cli argument for it. WRT security I think this is ok as the same concerns are valid when handling normal messages.

@towc
Copy link
Contributor Author

towc commented Nov 16, 2019

@lpinca oh wow, I didn't even look to see if normal messages were sanitized. They aren't. It'd be nice to create another issue to discuss whether sanitizing is a good idea for a debugging tool, and possible strategies that will keep the usage bearable

@towc
Copy link
Contributor Author

towc commented Nov 16, 2019

For now I'd merge this so at least there's a way to do it. I don't have the energy to prepare another PR right now. Same for #110

@lpinca
Copy link
Member

lpinca commented Nov 16, 2019

There is no need to create another PR. Amend the commit in this one and force push.

@towc
Copy link
Contributor Author

towc commented Nov 16, 2019

too late, #112 is here :)

I prefer keeping them separate, in case it's then decided that changing the current default interface breaks something, and someone would need to go back and revert changes, and it's a whole mess. You can still close this one

@lpinca lpinca closed this Nov 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants