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

fix -d/--decrypt-only not working correctly for binary data #164

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

whentze
Copy link
Contributor

@whentze whentze commented Feb 24, 2023

I had first used printf for outputting the data, but that breaks if the secret itself contains null bytes.

One could fix this by using e.g. cat, but looking a bit more at the code I realized that in the -d case we never need to mktemp at all and can just ask age to write directly to stdout by not setting -o.

I had first used `printf` for outputting the data,
but that breaks if the secret itself contains null bytes.

One could fix this by using e.g. `cat`, but looking a bit more at the code
I realized that in the -d case we never need to `mktemp` at all and can
just ask `age` to write directly to stdout by not setting -o.
@whentze
Copy link
Contributor Author

whentze commented Feb 24, 2023

@ryantm this changes the separation of responsibilities between decrypt and edit a bit, let me know if that's alright or if I should write a more conservative fix instead (e.g. using cat).

Also, do you think we should add a test for binary data?

Copy link
Owner

@ryantm ryantm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice to me. Thanks for fixing this!

@ryantm ryantm merged commit 833f87c into ryantm:main Feb 24, 2023
@n8henrie
Copy link
Collaborator

Also, do you think we should add a test for binary data?

yes, that would be great! Also, I would think printf would work for binary data, just perhaps not with %s, which tells it to interpret arguments as a string. But seems like you figured out something that works!

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.

3 participants