Add overwrite checks - Githubissues

magic-wormhole / magic-wormhole.rs

Rust implementation of Magic Wormhole, with new features and enhancements

European Union Public License 1.2

722 stars 78 forks source link

Add overwrite checks #107

Closed brightly-salty closed 3 years ago

brightly-salty commented 3 years ago

Fixes #105

brightly-salty commented 3 years ago

Okay @piegamesde I've made the changes I'm open to implementing the other changes (exposing it in the API, and adding tests) but I will probably need more guidance/mentoring to do something like that. I can continue in this PR or we can merge it and open a new one for those changes, whichever you prefer.

piegamesde commented 3 years ago

I've had a look at it and the tests are in a rather bad shape at the moment, so doing it here would probably be scope creep. Regarding the API, I propose the following: The check is moved into main.rs and accept takes in a overwrite: bool. This allows for the following behavior:

The application will check the file if it already exists and then pass the user's answer. If it doesn't, it will pass false. This should prevent any TOCTTOU bugs.
One can safely always say false and never overwrite anything (for a future noninteractive mode)
One can also pass true if the destination location is a temporary folder etc. (this will probably be useful for the tests as well)

brightly-salty commented 3 years ago

@piegamesde So will the RecieveRequest::accept() function still check for overwriting? If it does, then we're duplicating effort in the application and the library.

piegamesde commented 3 years ago

We have one explicit check and one implicit one when opening the file. If we move the check into the application, the library needs not do it anymore. The implicit check happens when we open the file: if overwrite = false, the create_new function will fail if the file already exists. If overwrite = true, the truncate option will handle existing files. By the way this still needs to be implemented, the starting point there is let mut file = File::open in send_records. I haven't tried it out myself, but your current code should fail to overwrite any files even when the user confirms it.

piegamesde commented 3 years ago

I just tried it out and I got a panic: "thread 'main' panicked at 'called Result::unwrap() on an Err value: TransitType { abilities_v1: [DirectTcpV1, RelayV1], hints_v1: [DirectTcpV1(DirectHint { priority: 0.0, hostname: "192.168.1.1", port: 40900 }), RelayV1(RelayHint { hints: [DirectHint { priority: 0.0, hostname: "transit.magic-wormhole.io", port: 4001 }] })] }', src/transfer.rs:326:64"

Also a small typo in the dialog: Override=>Overwrite.

piegamesde commented 3 years ago

Thank you for your contribution. I hope you don't mind me going ahead on the last meters :)