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

cli/pack: erase temp file on failure #107

Merged
merged 1 commit into from
Jul 5, 2022

Conversation

pleshevskiy
Copy link
Contributor

Moved path validation above. Added zip archive cleanup on encryption error.

I want to move password retrieval from encryption, but it will take more time and effort)

@@ -7,7 +7,7 @@ use std::io::Read;
// this hashes the input file
// it reads it in blocks, updates the hasher, and finalises/displays the hash
// it's used by hash-standalone mode
pub fn hash_stream(files: &Vec<String>) -> Result<()> {
pub fn hash_stream(files: &[String]) -> Result<()> {
Copy link
Owner

Choose a reason for hiding this comment

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

This is a good change - I don't know how I missed this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks clippy for finding this :)

@brxken128
Copy link
Owner

Moved path validation above. Added zip archive cleanup on encryption error.

I want to move password retrieval from encryption, but it will take more time and effort)

I'm not too sure how good of an idea it would be to move password-handling from outside of the encryption function. I understand that it's one of the sources of possible errors (when a keyfile is supplied), but I'm not too sure we should move it out. If we did, we wouldn't be able to check for missing files beforehand.

One way around this could be to have a kind of "entry/validation function", something that globally checks for common mishaps. This function could check that the input file exists, checks that the keyfile exists/isn't empty, get the user's password, ask if they'd like to overwrite the output file, etc. It'd only be needed for some functions (not the header manipulation ones), but it could be an ideal way to handle this I think.

The other functions could then receive the user's key, in the form of a Protected<Vec<u8>> (and continue life as usual).

@pleshevskiy
Copy link
Contributor Author

If we did, we wouldn't be able to check for missing files beforehand.

I want to get the password after checking the files, but before creating the compressed file) I think this is correct) in fact for this reason I moved the walker with the file path checker to the top.

@pleshevskiy
Copy link
Contributor Author

Just give me a chance to finish this task :)

@pleshevskiy
Copy link
Contributor Author

@brxken128 actually, I think we should merge this PR ) My commit solves #99, but I want to continue in separate branch.

.into_iter()
.filter_map(|res| res.ok())
.collect::<Vec<walkdir::DirEntry>>();
// let item_data = item.context("Unable to get path of item, skipping")?;
Copy link
Owner

Choose a reason for hiding this comment

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

Dead code that could do with being removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed... but I was too late

Copy link
Owner

Choose a reason for hiding this comment

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

No worries, I'll remove it in a sec :)

@brxken128
Copy link
Owner

I don't have the chance to test this this myself, but the tests pass and the code looks great so I'll merge. Great work! @pleshevskiy

@brxken128 brxken128 merged commit 9f6a8fe into brxken128:master Jul 5, 2022
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.

2 participants